Re: [PATCH RFC v1 02/15] iommu: Add a simple PASID table library

[Vivek Kumar Gautam]

Hi Jean,


On 3/3/21 10:41 PM, Jean-Philippe Brucker wrote:
> Hi Vivek,
>
> Thanks again for working on this. I have a few comments but it looks
> sensible overall.

Thanks a lot for reviewing the patch-series. Please find my responses 
inline below.

> Regarding the overall design, I was initially assigning page directories
> instead of whole PASID tables, which would simplify the driver and host
> implementation. A major complication, however, is SMMUv3 accesses PASID
> tables using a guest-physical address, so there is a messy negotiation
> needed between host and guest when the host needs to allocate PASID
> tables. Plus vSMMU needs PASID table assignment, so that's what the host
> driver will implement.

By assigning the page directories, you mean setting up just the stage-1 
page table ops, and passing that information to the host using ATTACH_TABLE?
Right now when using kvmtool, the struct iommu_pasid_table_config is 
populated with the correct information, and this whole memory is mapped 
between host and guest by creating a mem bank using 
kvm__for_each_mem_bank().
Did I get you or did I fail terribly in understanding the point you are 
making here?
If it helps, I will publish my kvmtool branch.

> On Fri, Jan 15, 2021 at 05:43:29PM +0530, Vivek Gautam wrote:
> > Add a small API in iommu subsystem to handle PASID table allocation
> > requests from different consumer drivers, such as a paravirtualized
> > iommu driver. The API provides ops for allocating and freeing PASID
> > table, writing to it and managing the table caches.
> >
> > This library also provides for registering a vendor API that attaches
> > to these ops. The vendor APIs would eventually perform arch level
> > implementations for these PASID tables.
>
> Although Arm might be the only vendor to ever use this, I think the
> abstraction makes sense and isn't too invasive. Even if we called directly
> into the SMMU driver from the virtio one, we'd still need patch 3 and
> separate TLB invalidations ops.

Right, the idea was to make users of iommu-pasid-table - virtio-iommu or 
the arm-smmu-v3 - consistent. I also noticed that the whole process of 
allocating the pasid tables (or cd tables) and populating them with 
stage-1 page tables in viommu is also in-line with how things are in 
arm-smmu-v3 or atleast that's how the design can be in general - 
allocate pasid_table, and program stage-1 information into it, and then 
pass it across to host.

> > Signed-off-by: Vivek Gautam <vivek.gautam@xxxxxxx>
> > Cc: Joerg Roedel <joro@xxxxxxxxxx>
> > Cc: Will Deacon <will.deacon@xxxxxxx>
> > Cc: Robin Murphy <robin.murphy@xxxxxxx>
> > Cc: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> > Cc: Eric Auger <eric.auger@xxxxxxxxxx>
> > Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> > Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > Cc: Liu Yi L <yi.l.liu@xxxxxxxxx>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>
> > ---
> >   drivers/iommu/iommu-pasid-table.h | 134 ++++++++++++++++++++++++++++++
> >   1 file changed, 134 insertions(+)
> >   create mode 100644 drivers/iommu/iommu-pasid-table.h
> >
> > diff --git a/drivers/iommu/iommu-pasid-table.h b/drivers/iommu/iommu-pasid-table.h
> > new file mode 100644
> > index 000000000000..bd4f57656f67
> > --- /dev/null
> > +++ b/drivers/iommu/iommu-pasid-table.h
> > @@ -0,0 +1,134 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PASID table management for the IOMMU
> > + *
> > + * Copyright (C) 2021 Arm Ltd.
> > + */
> > +#ifndef __IOMMU_PASID_TABLE_H
> > +#define __IOMMU_PASID_TABLE_H
> > +
> > +#include <linux/io-pgtable.h>
> > +
> > +#include "arm/arm-smmu-v3/arm-smmu-v3.h"
> > +
> > +enum pasid_table_fmt {
> > +	PASID_TABLE_ARM_SMMU_V3,
> > +	PASID_TABLE_NUM_FMTS,
> > +};
> > +
> > +/**
> > + * struct arm_smmu_cfg_info - arm-smmu-v3 specific configuration data
> > + *
> > + * @s1_cfg: arm-smmu-v3 stage1 config data
> > + * @feat_flag: features supported by arm-smmu-v3 implementation
> > + */
> > +struct arm_smmu_cfg_info {
> > +	struct arm_smmu_s1_cfg	*s1_cfg;
> > +	u32			feat_flag;
> > +};
> > +
> > +/**
> > + * struct iommu_vendor_psdtable_cfg - Configuration data for PASID tables
> > + *
> > + * @iommu_dev: device performing the DMA table walks
> > + * @fmt: The PASID table format
> > + * @base: DMA address of the allocated table, set by the vendor driver
> > + * @cfg: arm-smmu-v3 specific config data
> > + */
> > +struct iommu_vendor_psdtable_cfg {
> > +	struct device		*iommu_dev;
> > +	enum pasid_table_fmt	fmt;
> > +	dma_addr_t		base;
> > +	union {
> > +		struct arm_smmu_cfg_info	cfg;
>
> For the union to be extensible, that field should be called "arm" or
> something like that.

Sure. Will amend this.

Thanks,
Vivek

> Thanks,
> Jean
>
> > +	} vendor;
> > +};
> > +
> > +struct iommu_vendor_psdtable_ops;
> > +
> > +/**
> > + * struct iommu_pasid_table - describes a set of PASID tables
> > + *
> > + * @cookie: An opaque token provided by the IOMMU driver and passed back to any
> > + * callback routine.
> > + * @cfg: A copy of the PASID table configuration
> > + * @ops: The PASID table operations in use for this set of page tables
> > + */
> > +struct iommu_pasid_table {
> > +	void					*cookie;
> > +	struct iommu_vendor_psdtable_cfg	cfg;
> > +	struct iommu_vendor_psdtable_ops	*ops;
> > +};
> > +
> > +#define pasid_table_cfg_to_table(pst_cfg) \
> > +	container_of((pst_cfg), struct iommu_pasid_table, cfg)
> > +
> > +struct iommu_vendor_psdtable_ops {
> > +	int (*alloc)(struct iommu_vendor_psdtable_cfg *cfg);
> > +	void (*free)(struct iommu_vendor_psdtable_cfg *cfg);
> > +	void (*prepare)(struct iommu_vendor_psdtable_cfg *cfg,
> > +			struct io_pgtable_cfg *pgtbl_cfg, u32 asid);
> > +	int (*write)(struct iommu_vendor_psdtable_cfg *cfg, int ssid,
> > +		     void *cookie);
> > +	void (*sync)(void *cookie, int ssid, bool leaf);
> > +};
> > +
> > +static inline int iommu_psdtable_alloc(struct iommu_pasid_table *tbl,
> > +				       struct iommu_vendor_psdtable_cfg *cfg)
> > +{
> > +	if (!tbl->ops->alloc)
> > +		return -ENOSYS;
> > +
> > +	return tbl->ops->alloc(cfg);
> > +}
> > +
> > +static inline void iommu_psdtable_free(struct iommu_pasid_table *tbl,
> > +				       struct iommu_vendor_psdtable_cfg *cfg)
> > +{
> > +	if (!tbl->ops->free)
> > +		return;
> > +
> > +	tbl->ops->free(cfg);
> > +}
> > +
> > +static inline int iommu_psdtable_prepare(struct iommu_pasid_table *tbl,
> > +					 struct iommu_vendor_psdtable_cfg *cfg,
> > +					 struct io_pgtable_cfg *pgtbl_cfg,
> > +					 u32 asid)
> > +{
> > +	if (!tbl->ops->prepare)
> > +		return -ENOSYS;
> > +
> > +	tbl->ops->prepare(cfg, pgtbl_cfg, asid);
> > +	return 0;
> > +}
> > +
> > +static inline int iommu_psdtable_write(struct iommu_pasid_table *tbl,
> > +				       struct iommu_vendor_psdtable_cfg *cfg,
> > +				       int ssid, void *cookie)
> > +{
> > +	if (!tbl->ops->write)
> > +		return -ENOSYS;
> > +
> > +	return tbl->ops->write(cfg, ssid, cookie);
> > +}
> > +
> > +static inline int iommu_psdtable_sync(struct iommu_pasid_table *tbl,
> > +				      void *cookie, int ssid, bool leaf)
> > +{
> > +	if (!tbl->ops->sync)
> > +		return -ENOSYS;
> > +
> > +	tbl->ops->sync(cookie, ssid, leaf);
> > +	return 0;
> > +}
> > +
> > +/* A placeholder to register vendor specific pasid layer */
> > +static inline struct iommu_pasid_table *
> > +iommu_register_pasid_table(enum pasid_table_fmt fmt,
> > +			   struct device *dev, void *cookie)
> > +{
> > +	return NULL;
> > +}
> > +
> > +#endif /* __IOMMU_PASID_TABLE_H */
> > --
> > 2.17.1