Re: [PATCH RFC v1 00/15] iommu/virtio: Nested stage support with Arm

Re: [PATCH RFC v1 00/15] iommu/virtio: Nested stage support with Arm

[Auger Eric]

Hi Vivek,

On 1/15/21 1:13 PM, Vivek Gautam wrote:
> This patch-series aims at enabling Nested stage translation in guests
> using virtio-iommu as the paravirtualized iommu. The backend is supported
> with Arm SMMU-v3 that provides nested stage-1 and stage-2 translation.
> 
> This series derives its purpose from various efforts happening to add
> support for Shared Virtual Addressing (SVA) in host and guest. On Arm,
> most of the support for SVA has already landed. The support for nested
> stage translation and fault reporting to guest has been proposed [1].
> The related changes required in VFIO [2] framework have also been put
> forward.
> 
> This series proposes changes in virtio-iommu to program PASID tables
> and related stage-1 page tables. A simple iommu-pasid-table library
> is added for this purpose that interacts with vendor drivers to
> allocate and populate PASID tables.
> In Arm SMMUv3 we propose to pull the Context Descriptor (CD) management
> code out of the arm-smmu-v3 driver and add that as a glue vendor layer
> to support allocating CD tables, and populating them with right values.
> These CD tables are essentially the PASID tables and contain stage-1
> page table configurations too.
> A request to setup these CD tables come from virtio-iommu driver using
> the iommu-pasid-table library when running on Arm. The virtio-iommu
> then pass these PASID tables to the host using the right virtio backend
> and support in VMM.
> 
> For testing we have added necessary support in kvmtool. The changes in
> kvmtool are based on virtio-iommu development branch by Jean-Philippe
> Brucker [3].
> 
> The tested kernel branch contains following in the order bottom to top
> on the git hash -
> a) v5.11-rc3
> b) arm-smmu-v3 [1] and vfio [2] changes from Eric to add nested page
>    table support for Arm.
> c) Smmu test engine patches from Jean-Philippe's branch [4]
> d) This series
> e) Domain nesting info patches [5][6][7].
> f) Changes to add arm-smmu-v3 specific nesting info (to be sent to
>    the list).
> 
> This kernel is tested on Neoverse reference software stack with
> Fixed virtual platform. Public version of the software stack and
> FVP is available here[8][9].
> 
> A big thanks to Jean-Philippe for his contributions towards this work
> and for his valuable guidance.
> 
> [1] https://lore.kernel.org/linux-iommu/20201118112151.25412-1-eric.auger@redhat.com/T/
> [2] https://lore.kernel.org/kvmarm/20201116110030.32335-12-eric.auger@redhat.com/T/
> [3] https://jpbrucker.net/git/kvmtool/log/?h=virtio-iommu/devel
> [4] https://jpbrucker.net/git/linux/log/?h=sva/smmute
> [5] https://lore.kernel.org/kvm/1599734733-6431-2-git-send-email-yi.l.liu@intel.com/
> [6] https://lore.kernel.org/kvm/1599734733-6431-3-git-send-email-yi.l.liu@intel.com/
> [7] https://lore.kernel.org/kvm/1599734733-6431-4-git-send-email-yi.l.liu@intel.com/
> [8] https://developer.arm.com/tools-and-software/open-source-software/arm-platforms-software/arm-ecosystem-fvps
> [9] https://git.linaro.org/landing-teams/working/arm/arm-reference-platforms.git/about/docs/rdn1edge/user-guide.rst

Could you share a public branch where we could find all the kernel pieces.

Thank you in advance

Best Regards

Eric
> 
> Jean-Philippe Brucker (6):
>   iommu/virtio: Add headers for table format probing
>   iommu/virtio: Add table format probing
>   iommu/virtio: Add headers for binding pasid table in iommu
>   iommu/virtio: Add support for INVALIDATE request
>   iommu/virtio: Attach Arm PASID tables when available
>   iommu/virtio: Add support for Arm LPAE page table format
> 
> Vivek Gautam (9):
>   iommu/arm-smmu-v3: Create a Context Descriptor library
>   iommu: Add a simple PASID table library
>   iommu/arm-smmu-v3: Update drivers to work with iommu-pasid-table
>   iommu/arm-smmu-v3: Update CD base address info for user-space
>   iommu/arm-smmu-v3: Set sync op from consumer driver of cd-lib
>   iommu: Add asid_bits to arm smmu-v3 stage1 table info
>   iommu/virtio: Update table format probing header
>   iommu/virtio: Prepare to add attach pasid table infrastructure
>   iommu/virtio: Update fault type and reason info for viommu fault
> 
>  drivers/iommu/arm/arm-smmu-v3/Makefile        |   2 +-
>  .../arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c      | 283 +++++++
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  16 +-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 268 +------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   4 +-
>  drivers/iommu/iommu-pasid-table.h             | 140 ++++
>  drivers/iommu/virtio-iommu.c                  | 692 +++++++++++++++++-
>  include/uapi/linux/iommu.h                    |   2 +-
>  include/uapi/linux/virtio_iommu.h             | 158 +++-
>  9 files changed, 1303 insertions(+), 262 deletions(-)
>  create mode 100644 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
>  create mode 100644 drivers/iommu/iommu-pasid-table.h
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v1 00/15] iommu/virtio: Nested stage support with Arm

[Auger Eric]

Hi Vivek,

On 1/21/21 6:34 PM, Vivek Kumar Gautam wrote:
> Hi Eric,
> 
> 
> On 1/19/21 2:33 PM, Auger Eric wrote:
>> Hi Vivek,
>>
>> On 1/15/21 1:13 PM, Vivek Gautam wrote:
>>> This patch-series aims at enabling Nested stage translation in guests
>>> using virtio-iommu as the paravirtualized iommu. The backend is
>>> supported
>>> with Arm SMMU-v3 that provides nested stage-1 and stage-2 translation.
>>>
>>> This series derives its purpose from various efforts happening to add
>>> support for Shared Virtual Addressing (SVA) in host and guest. On Arm,
>>> most of the support for SVA has already landed. The support for nested
>>> stage translation and fault reporting to guest has been proposed [1].
>>> The related changes required in VFIO [2] framework have also been put
>>> forward.
>>>
>>> This series proposes changes in virtio-iommu to program PASID tables
>>> and related stage-1 page tables. A simple iommu-pasid-table library
>>> is added for this purpose that interacts with vendor drivers to
>>> allocate and populate PASID tables.
>>> In Arm SMMUv3 we propose to pull the Context Descriptor (CD) management
>>> code out of the arm-smmu-v3 driver and add that as a glue vendor layer
>>> to support allocating CD tables, and populating them with right values.
>>> These CD tables are essentially the PASID tables and contain stage-1
>>> page table configurations too.
>>> A request to setup these CD tables come from virtio-iommu driver using
>>> the iommu-pasid-table library when running on Arm. The virtio-iommu
>>> then pass these PASID tables to the host using the right virtio backend
>>> and support in VMM.
>>>
>>> For testing we have added necessary support in kvmtool. The changes in
>>> kvmtool are based on virtio-iommu development branch by Jean-Philippe
>>> Brucker [3].
>>>
>>> The tested kernel branch contains following in the order bottom to top
>>> on the git hash -
>>> a) v5.11-rc3
>>> b) arm-smmu-v3 [1] and vfio [2] changes from Eric to add nested page
>>>     table support for Arm.
>>> c) Smmu test engine patches from Jean-Philippe's branch [4]
>>> d) This series
>>> e) Domain nesting info patches [5][6][7].
>>> f) Changes to add arm-smmu-v3 specific nesting info (to be sent to
>>>     the list).
>>>
>>> This kernel is tested on Neoverse reference software stack with
>>> Fixed virtual platform. Public version of the software stack and
>>> FVP is available here[8][9].
>>>
>>> A big thanks to Jean-Philippe for his contributions towards this work
>>> and for his valuable guidance.
>>>
>>> [1]
>>> https://lore.kernel.org/linux-iommu/20201118112151.25412-1-eric.auger@redhat.com/T/
>>>
>>> [2]
>>> https://lore.kernel.org/kvmarm/20201116110030.32335-12-eric.auger@redhat.com/T/
>>>
>>> [3] https://jpbrucker.net/git/kvmtool/log/?h=virtio-iommu/devel
>>> [4] https://jpbrucker.net/git/linux/log/?h=sva/smmute
>>> [5]
>>> https://lore.kernel.org/kvm/1599734733-6431-2-git-send-email-yi.l.liu@intel.com/
>>>
>>> [6]
>>> https://lore.kernel.org/kvm/1599734733-6431-3-git-send-email-yi.l.liu@intel.com/
>>>
>>> [7]
>>> https://lore.kernel.org/kvm/1599734733-6431-4-git-send-email-yi.l.liu@intel.com/
>>>
>>> [8]
>>> https://developer.arm.com/tools-and-software/open-source-software/arm-platforms-software/arm-ecosystem-fvps
>>>
>>> [9]
>>> https://git.linaro.org/landing-teams/working/arm/arm-reference-platforms.git/about/docs/rdn1edge/user-guide.rst
>>>
>>
>> Could you share a public branch where we could find all the kernel
>> pieces.
>>
>> Thank you in advance
> 
> Apologies for the delay. It took a bit of time to sort things out for a
> public branch.
> The branch is available in my github now. Please have a look.
> 
> https://github.com/vivek-arm/linux/tree/5.11-rc3-nested-pgtbl-arm-smmuv3-virtio-iommu

no problem. Thank you for the link.

Best Regards

Eric
> 
> 
> 
> Thanks and regards
> Vivek
> 
>>
>> Best Regards
>>
>> Eric
>>>
>>> Jean-Philippe Brucker (6):
>>>    iommu/virtio: Add headers for table format probing
>>>    iommu/virtio: Add table format probing
>>>    iommu/virtio: Add headers for binding pasid table in iommu
>>>    iommu/virtio: Add support for INVALIDATE request
>>>    iommu/virtio: Attach Arm PASID tables when available
>>>    iommu/virtio: Add support for Arm LPAE page table format
>>>
>>> Vivek Gautam (9):
>>>    iommu/arm-smmu-v3: Create a Context Descriptor library
>>>    iommu: Add a simple PASID table library
>>>    iommu/arm-smmu-v3: Update drivers to work with iommu-pasid-table
>>>    iommu/arm-smmu-v3: Update CD base address info for user-space
>>>    iommu/arm-smmu-v3: Set sync op from consumer driver of cd-lib
>>>    iommu: Add asid_bits to arm smmu-v3 stage1 table info
>>>    iommu/virtio: Update table format probing header
>>>    iommu/virtio: Prepare to add attach pasid table infrastructure
>>>    iommu/virtio: Update fault type and reason info for viommu fault
>>>
>>>   drivers/iommu/arm/arm-smmu-v3/Makefile        |   2 +-
>>>   .../arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c      | 283 +++++++
>>>   .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  16 +-
>>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 268 +------
>>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   4 +-
>>>   drivers/iommu/iommu-pasid-table.h             | 140 ++++
>>>   drivers/iommu/virtio-iommu.c                  | 692 +++++++++++++++++-
>>>   include/uapi/linux/iommu.h                    |   2 +-
>>>   include/uapi/linux/virtio_iommu.h             | 158 +++-
>>>   9 files changed, 1303 insertions(+), 262 deletions(-)
>>>   create mode 100644 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
>>>   create mode 100644 drivers/iommu/iommu-pasid-table.h
>>>
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Jean-Philippe Brucker]

Hi Vivek,

Thanks again for working on this. I have a few comments but it looks
sensible overall.

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.

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.

> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> ---
>  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.

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
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v1 04/15] iommu/arm-smmu-v3: Update CD base address info for user-space

[Jean-Philippe Brucker]

On Fri, Jan 15, 2021 at 05:43:31PM +0530, Vivek Gautam wrote:
> Update base address information in vendor pasid table info to pass that
> to user-space for stage1 table management.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
> index 8a7187534706..ec37476c8d09 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
> @@ -55,6 +55,9 @@ static __le64 *arm_smmu_get_cd_ptr(struct iommu_vendor_psdtable_cfg *pst_cfg,
>  		if (arm_smmu_alloc_cd_leaf_table(dev, l1_desc))
>  			return NULL;
>  
> +		if (s1cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
> +			pst_cfg->base = l1_desc->l2ptr_dma;
> +

This isn't the right place, because this path allocates second-level
tables for two-level tables. I don't think we need pst_cfg->base at all,
because for both linear and two-level tables, the base pointer is in
cdcfg->cdtab_dma, which can be read directly.

Thanks,
Jean

>  		l1ptr = cdcfg->cdtab + idx * CTXDESC_L1_DESC_DWORDS;
>  		arm_smmu_write_cd_l1_desc(l1ptr, l1_desc);
>  		/* An invalid L1CD can be cached */
> @@ -211,6 +214,9 @@ static int arm_smmu_alloc_cd_tables(struct iommu_vendor_psdtable_cfg *pst_cfg)
>  		goto err_free_l1;
>  	}
>  
> +	if (s1cfg->s1fmt == STRTAB_STE_0_S1FMT_64K_L2)
> +		pst_cfg->base = cdcfg->cdtab_dma;
> +
>  	return 0;
>  
>  err_free_l1:
> -- 
> 2.17.1
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v1 05/15] iommu/arm-smmu-v3: Set sync op from consumer driver of cd-lib

[Jean-Philippe Brucker]

On Fri, Jan 15, 2021 at 05:43:32PM +0530, Vivek Gautam wrote:
> Te change allows different consumers of arm-smmu-v3-cd-lib to set
> their respective sync op for pasid entries.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c | 1 -
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c        | 7 +++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
> index ec37476c8d09..acaa09acecdd 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
> @@ -265,7 +265,6 @@ struct iommu_vendor_psdtable_ops arm_cd_table_ops = {
>  	.free	 = arm_smmu_free_cd_tables,
>  	.prepare = arm_smmu_prepare_cd,
>  	.write	 = arm_smmu_write_ctx_desc,
> -	.sync	 = arm_smmu_sync_cd,
>  };
>  
>  struct iommu_pasid_table *arm_smmu_register_cd_table(struct device *dev,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 2f86c6ac42b6..0c644be22b4b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1869,6 +1869,13 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
>  	if (ret)
>  		goto out_free_cd_tables;
>  
> +	/*
> +	 * Strange to setup an op here?
> +	 * cd-lib is the actual user of sync op, and therefore the platform
> +	 * drivers should assign this sync/maintenance ops as per need.
> +	 */
> +	tbl->ops->sync = arm_smmu_sync_cd;
> +

Modifying a static struct from here doesn't feel right. I think the
interface should be roughly similar to io-pgtable since the principle is
the same. So the sync() op should be separate from arm_cd_table_ops since
it's a callback into the driver. Maybe pass it to
iommu_register_pasid_table().

Thanks,
Jean
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v1 06/15] iommu/virtio: Add headers for table format probing

[Jean-Philippe Brucker]

On Fri, Jan 15, 2021 at 05:43:33PM +0530, Vivek Gautam wrote:
> From: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> 
> Add required UAPI defines for probing table format for underlying
> iommu hardware. The device may provide information about hardware
> tables and additional capabilities for each device.
> This allows guest to correctly fabricate stage-1 page tables.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> [Vivek: Use a single "struct virtio_iommu_probe_table_format" rather
>         than separate structures for page table and pasid table format.

Makes sense. I've integrated that into the spec draft, added more precise
documentation and modified some of the definitions.

The current draft is here:
https://jpbrucker.net/virtio-iommu/spec/virtio-iommu-v0.13.pdf
Posted on the list here
https://lists.oasis-open.org/archives/virtio-dev/202102/msg00012.html

> 	Also update commit message.]
> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> ---
>  include/uapi/linux/virtio_iommu.h | 44 ++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> index 237e36a280cb..43821e33e7af 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -2,7 +2,7 @@
>  /*
>   * Virtio-iommu definition v0.12
>   *
> - * Copyright (C) 2019 Arm Ltd.
> + * Copyright (C) 2019-2021 Arm Ltd.

Not strictly necessary. But if you're modifying this comment please also
remove the "v0.12" above

>   */
>  #ifndef _UAPI_LINUX_VIRTIO_IOMMU_H
>  #define _UAPI_LINUX_VIRTIO_IOMMU_H
> @@ -111,6 +111,12 @@ struct virtio_iommu_req_unmap {
>  
>  #define VIRTIO_IOMMU_PROBE_T_NONE		0
>  #define VIRTIO_IOMMU_PROBE_T_RESV_MEM		1
> +#define VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK	2
> +#define VIRTIO_IOMMU_PROBE_T_INPUT_RANGE	3
> +#define VIRTIO_IOMMU_PROBE_T_OUTPUT_SIZE	4
> +#define VIRTIO_IOMMU_PROBE_T_PASID_SIZE		5
> +#define VIRTIO_IOMMU_PROBE_T_PAGE_TABLE_FMT	6
> +#define VIRTIO_IOMMU_PROBE_T_PASID_TABLE_FMT	7

Since there is a single struct we can have a single
VIRTIO_IOMMU_PROBE_T_TABLE_FORMAT.

>  
>  #define VIRTIO_IOMMU_PROBE_T_MASK		0xfff
>  
> @@ -130,6 +136,42 @@ struct virtio_iommu_probe_resv_mem {
>  	__le64					end;
>  };
>  
> +struct virtio_iommu_probe_page_size_mask {
> +	struct virtio_iommu_probe_property	head;
> +	__u8					reserved[4];
> +	__le64					mask;
> +};
> +
> +struct virtio_iommu_probe_input_range {
> +	struct virtio_iommu_probe_property	head;
> +	__u8					reserved[4];
> +	__le64					start;
> +	__le64					end;
> +};
> +
> +struct virtio_iommu_probe_output_size {
> +	struct virtio_iommu_probe_property	head;
> +	__u8					bits;
> +	__u8					reserved[3];
> +};
> +
> +struct virtio_iommu_probe_pasid_size {
> +	struct virtio_iommu_probe_property	head;
> +	__u8					bits;
> +	__u8					reserved[3];
> +};
> +
> +/* Arm LPAE page table format */
> +#define VIRTIO_IOMMU_FOMRAT_PGTF_ARM_LPAE	1

s/FOMRAT/FORMAT

> +/* Arm smmu-v3 type PASID table format */
> +#define VIRTIO_IOMMU_FORMAT_PSTF_ARM_SMMU_V3	2

These should be with the Arm-specific definitions patches 11 and 14

Thanks,
Jean

> +
> +struct virtio_iommu_probe_table_format {
> +	struct virtio_iommu_probe_property	head;
> +	__le16					format;
> +	__u8					reserved[2];
> +};
> +
>  struct virtio_iommu_req_probe {
>  	struct virtio_iommu_req_head		head;
>  	__le32					endpoint;
> -- 
> 2.17.1
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v1 08/15] iommu: Add asid_bits to arm smmu-v3 stage1 table info

[Jean-Philippe Brucker]

On Fri, Jan 15, 2021 at 05:43:35PM +0530, Vivek Gautam wrote:
> aisd_bits data is required to prepare stage-1 tables for arm-smmu-v3.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> ---
>  include/uapi/linux/iommu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index 082d758dd016..96abbfc7c643 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -357,7 +357,7 @@ struct iommu_pasid_smmuv3 {
>  	__u32	version;
>  	__u8	s1fmt;
>  	__u8	s1dss;
> -	__u8	padding[2];
> +	__u16	asid_bits;

Is this used anywhere?  This struct is passed from host userspace to host
kernel to attach the PASID table, so I don't think it needs an asid_bits
field.

Thanks,
Jean

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v1 09/15] iommu/virtio: Update table format probing header

[Jean-Philippe Brucker]

On Fri, Jan 15, 2021 at 05:43:36PM +0530, Vivek Gautam wrote:
> Add info about asid_bits and additional flags to table format
> probing header.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> ---
>  include/uapi/linux/virtio_iommu.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> index 43821e33e7af..8a0624bab4b2 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -169,7 +169,10 @@ struct virtio_iommu_probe_pasid_size {
>  struct virtio_iommu_probe_table_format {
>  	struct virtio_iommu_probe_property	head;
>  	__le16					format;
> -	__u8					reserved[2];
> +	__le16					asid_bits;
> +
> +	__le32					flags;

This struct should only contain the head and format fields. asid and flags
should go in a specialized structure - virtio_iommu_probe_pgt_arm64 in the
latest spec draft, where I dropped the asid_bits field in favor of an
"ASID16" flag.

Thanks,
Jean

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v1 13/15] iommu/virtio: Attach Arm PASID tables when available

[Jean-Philippe Brucker]

On Fri, Jan 15, 2021 at 05:43:40PM +0530, Vivek Gautam wrote:
[...]
> +static int viommu_setup_pgtable(struct viommu_endpoint *vdev,
> +				struct viommu_domain *vdomain)
> +{
> +	int ret, id;
> +	u32 asid;
> +	enum io_pgtable_fmt fmt;
> +	struct io_pgtable_ops *ops = NULL;
> +	struct viommu_dev *viommu = vdev->viommu;
> +	struct virtio_iommu_probe_table_format *desc = vdev->pgtf;
> +	struct iommu_pasid_table *tbl = vdomain->pasid_tbl;
> +	struct iommu_vendor_psdtable_cfg *pst_cfg;
> +	struct arm_smmu_cfg_info *cfgi;
> +	struct io_pgtable_cfg cfg = {
> +		.iommu_dev	= viommu->dev->parent,
> +		.tlb		= &viommu_flush_ops,
> +		.pgsize_bitmap	= vdev->pgsize_mask ? vdev->pgsize_mask :
> +				  vdomain->domain.pgsize_bitmap,
> +		.ias		= (vdev->input_end ? ilog2(vdev->input_end) :
> +				   ilog2(vdomain->domain.geometry.aperture_end)) + 1,
> +		.oas		= vdev->output_bits,
> +	};
> +
> +	if (!desc)
> +		return -EINVAL;
> +
> +	if (!vdev->output_bits)
> +		return -ENODEV;
> +
> +	switch (le16_to_cpu(desc->format)) {
> +	case VIRTIO_IOMMU_FOMRAT_PGTF_ARM_LPAE:
> +		fmt = ARM_64_LPAE_S1;
> +		break;
> +	default:
> +		dev_err(vdev->dev, "unsupported page table format 0x%x\n",
> +			le16_to_cpu(desc->format));
> +		return -EINVAL;
> +	}
> +
> +	if (vdomain->mm.ops) {
> +		/*
> +		 * TODO: attach additional endpoint to the domain. Check that
> +		 * the config is sane.
> +		 */
> +		return -EEXIST;
> +	}
> +
> +	vdomain->mm.domain = vdomain;
> +	ops = alloc_io_pgtable_ops(fmt, &cfg, &vdomain->mm);
> +	if (!ops)
> +		return -ENOMEM;
> +
> +	pst_cfg = &tbl->cfg;
> +	cfgi = &pst_cfg->vendor.cfg;
> +	id = ida_simple_get(&asid_ida, 1, 1 << desc->asid_bits, GFP_KERNEL);
> +	if (id < 0) {
> +		ret = id;
> +		goto err_free_pgtable;
> +	}
> +
> +	asid = id;
> +	ret = iommu_psdtable_prepare(tbl, pst_cfg, &cfg, asid);
> +	if (ret)
> +		goto err_free_asid;
> +
> +	/*
> +	 * Strange to setup an op here?
> +	 * cd-lib is the actual user of sync op, and therefore the platform
> +	 * drivers should assign this sync/maintenance ops as per need.
> +	 */
> +	tbl->ops->sync = viommu_flush_pasid;

But this function deals with page tables, not pasid tables. As said on an
earlier patch, the TLB flush ops should probably be passed during table
registration - those ops are global so should really be const.

> +
> +	/* Right now only PASID 0 supported ?? */
> +	ret = iommu_psdtable_write(tbl, pst_cfg, 0, &cfgi->s1_cfg->cd);
> +	if (ret)
> +		goto err_free_asid;
> +
> +	vdomain->mm.ops = ops;
> +	dev_dbg(vdev->dev, "using page table format 0x%x\n", fmt);
> +
> +	return 0;
> +
> +err_free_asid:
> +	ida_simple_remove(&asid_ida, asid);
> +err_free_pgtable:
> +	free_io_pgtable_ops(ops);
> +	return ret;
> +}
> +
> +static int viommu_config_arm_pst(struct iommu_vendor_psdtable_cfg *pst_cfg,
> +				 struct virtio_iommu_req_attach_pst_arm *req)
> +{
> +	struct arm_smmu_s1_cfg *s1_cfg = pst_cfg->vendor.cfg.s1_cfg;
> +
> +	if (!s1_cfg)
> +		return -ENODEV;
> +
> +	req->format	= cpu_to_le16(VIRTIO_IOMMU_FORMAT_PSTF_ARM_SMMU_V3);
> +	req->s1fmt	= s1_cfg->s1fmt;
> +	req->s1dss	= VIRTIO_IOMMU_PSTF_ARM_SMMU_V3_DSS_0;
> +	req->s1contextptr = cpu_to_le64(pst_cfg->base);
> +	req->s1cdmax	= cpu_to_le32(s1_cfg->s1cdmax);
> +
> +	return 0;
> +}
> +
> +static int viommu_config_pst(struct iommu_vendor_psdtable_cfg *pst_cfg,
> +			     void *req, enum pasid_table_fmt fmt)
> +{
> +	int ret;
> +
> +	switch (fmt) {
> +	case PASID_TABLE_ARM_SMMU_V3:
> +		ret = viommu_config_arm_pst(pst_cfg, req);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		WARN_ON(1);
> +	}
> +
> +	return ret;
> +}
> +
> +static int viommu_prepare_arm_pst(struct viommu_endpoint *vdev,
> +				  struct iommu_vendor_psdtable_cfg *pst_cfg)
> +{
> +	struct virtio_iommu_probe_table_format *pgtf = vdev->pgtf;
> +	struct arm_smmu_cfg_info *cfgi = &pst_cfg->vendor.cfg;
> +	struct arm_smmu_s1_cfg *cfg;
> +
> +	/* Some sanity checks */
> +	if (pgtf->asid_bits != 8 && pgtf->asid_bits != 16)
> +		return -EINVAL;

No need for this, next patch cheks asid size in viommu_config_arm_pgt()

> +
> +	cfg = devm_kzalloc(pst_cfg->iommu_dev, sizeof(cfg), GFP_KERNEL);
> +	if (!cfg)
> +		return -ENOMEM;
> +
> +	cfgi->s1_cfg = cfg;
> +	cfg->s1cdmax = vdev->pasid_bits;
> +	cfg->cd.asid = pgtf->asid_bits;

That doesn't look right, cfg->cd.asid takes the ASID value of context 0
but here we're writing a limit. viommu_setup_pgtable() probably needs to
set this field to the allocated ASID, since viommu_teardown_pgtable() uses
it.

> +
> +	pst_cfg->fmt = PASID_TABLE_ARM_SMMU_V3;

Parent function can set this

> +	/* XXX HACK: set feature bit ARM_SMMU_FEAT_2_LVL_CDTAB */
> +	pst_cfg->vendor.cfg.feat_flag |= (1 << 1);

Oh right, this flag is missing. I'll add

  #define VIRTIO_IOMMU_PST_ARM_SMMU3_F_CD2L (1ULL << 1)

to the spec.

> +
> +	return 0;
> +}
> +
> +static int viommu_prepare_pst(struct viommu_endpoint *vdev,
> +			      struct iommu_vendor_psdtable_cfg *pst_cfg,
> +			      enum pasid_table_fmt fmt)
> +{
> +	int ret;
> +
> +	switch (fmt) {
> +	case PASID_TABLE_ARM_SMMU_V3:
> +		ret = viommu_prepare_arm_pst(vdev, pst_cfg);
> +		break;
> +	default:
> +		dev_err(vdev->dev, "unsupported PASID table format 0x%x\n", fmt);
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int viommu_attach_pasid_table(struct viommu_endpoint *vdev,
> +				     struct viommu_domain *vdomain)
> +{
> +	int ret;
> +	int i, eid;
> +	enum pasid_table_fmt fmt = -1;
> +	struct virtio_iommu_probe_table_format *desc = vdev->pstf;
> +	struct virtio_iommu_req_attach_table req = {
> +		.head.type	= VIRTIO_IOMMU_T_ATTACH_TABLE,
> +		.domain		= cpu_to_le32(vdomain->id),
> +	};
> +	struct viommu_dev *viommu = vdev->viommu;
> +	struct iommu_pasid_table *tbl;
> +	struct iommu_vendor_psdtable_cfg *pst_cfg;
> +
> +	if (!viommu->has_table)
> +		return 0;
> +
> +	if (!desc)
> +		return -ENODEV;
> +
> +	/* Prepare PASID tables configuration */
> +	switch (le16_to_cpu(desc->format)) {
> +	case VIRTIO_IOMMU_FORMAT_PSTF_ARM_SMMU_V3:
> +		fmt = PASID_TABLE_ARM_SMMU_V3;
> +		break;
> +	default:
> +		dev_err(vdev->dev, "unsupported PASID table format 0x%x\n",
> +			le16_to_cpu(desc->format));
> +		return 0;
> +	}
> +
> +	if (!tbl) {
> +		tbl = iommu_register_pasid_table(fmt, viommu->dev->parent, vdomain);
> +		if (!tbl)
> +			return -ENOMEM;
> +
> +		vdomain->pasid_tbl = tbl;
> +		pst_cfg = &tbl->cfg;
> +
> +		pst_cfg->iommu_dev = viommu->dev->parent;
> +
> +		/* Prepare PASID tables info to allocate a new table */
> +		ret = viommu_prepare_pst(vdev, pst_cfg, fmt);
> +		if (ret)
> +			return ret;
> +
> +		ret = iommu_psdtable_alloc(tbl, pst_cfg);
> +		if (ret)
> +			return ret;
> +
> +		pst_cfg->iommu_dev = viommu->dev->parent;

Already set by iommu_register_pasid_table() (and needed for DMA
allocations in iommu_psdtable_alloc())

> +		pst_cfg->fmt = PASID_TABLE_ARM_SMMU_V3;

Already set above

> +
> +		ret = viommu_setup_pgtable(vdev, vdomain);
> +		if (ret) {
> +			dev_err(vdev->dev, "could not install page tables\n");
> +			goto err_free_psdtable;
> +		}
> +
> +		/* Add arch-specific configuration */
> +		ret = viommu_config_pst(pst_cfg, (void *)&req, fmt);
> +		if (ret)
> +			goto err_free_ops;
> +
> +		vdev_for_each_id(i, eid, vdev) {
> +			req.endpoint = cpu_to_le32(eid);
> +			ret = viommu_send_req_sync(viommu, &req, sizeof(req));
> +			if (ret)
> +				goto err_free_ops;
> +		}
> +	} else {
> +		/* TODO: otherwise, check for compatibility with vdev. */
> +		return -ENOSYS;
> +	}
> +
> +	dev_dbg(vdev->dev, "uses PASID table format 0x%x\n", fmt);
> +
> +	return 0;
> +
> +err_free_ops:
> +	if (vdomain->mm.ops)
> +		viommu_teardown_pgtable(vdomain);
> +err_free_psdtable:
> +	iommu_psdtable_free(tbl, &tbl->cfg);
> +
> +	return ret;
> +}
> +
>  static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  {
>  	int ret = 0;
> @@ -928,6 +1213,17 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	if (vdev->vdomain)
>  		vdev->vdomain->nr_endpoints--;
>  
> +	ret = viommu_attach_pasid_table(vdev, vdomain);
> +	if (ret) {
> +		/*
> +		 * No PASID support, too bad. Perhaps we can bind a single set
> +		 * of page tables?
> +		 */
> +		ret = viommu_setup_pgtable(vdev, vdomain);

This cannot work at the moment because viommu_setup_pgtable() writes to
the non-existing pasid table. Probably best to leave this call for next
patch.

Thanks,
Jean

> +		if (ret)
> +			dev_err(vdev->dev, "could not install tables\n");
> +	}
> +
>  	if (!vdomain->mm.ops) {
>  		/* If we couldn't bind any table, use the mapping tree */
>  		ret = viommu_simple_attach(vdomain, vdev);
> @@ -948,6 +1244,10 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
>  	u32 flags;
>  	struct virtio_iommu_req_map map;
>  	struct viommu_domain *vdomain = to_viommu_domain(domain);
> +	struct io_pgtable_ops *ops = vdomain->mm.ops;
> +
> +	if (ops)
> +		return ops->map(ops, iova, paddr, size, prot, gfp);
>  
>  	flags = (prot & IOMMU_READ ? VIRTIO_IOMMU_MAP_F_READ : 0) |
>  		(prot & IOMMU_WRITE ? VIRTIO_IOMMU_MAP_F_WRITE : 0) |
> @@ -986,6 +1286,10 @@ static size_t viommu_unmap(struct iommu_domain *domain, unsigned long iova,
>  	size_t unmapped;
>  	struct virtio_iommu_req_unmap unmap;
>  	struct viommu_domain *vdomain = to_viommu_domain(domain);
> +	struct io_pgtable_ops *ops = vdomain->mm.ops;
> +
> +	if (ops)
> +		return ops->unmap(ops, iova, size, gather);
>  
>  	unmapped = viommu_del_mappings(vdomain, iova, size);
>  	if (unmapped < size)
> @@ -1014,6 +1318,10 @@ static phys_addr_t viommu_iova_to_phys(struct iommu_domain *domain,
>  	struct viommu_mapping *mapping;
>  	struct interval_tree_node *node;
>  	struct viommu_domain *vdomain = to_viommu_domain(domain);
> +	struct io_pgtable_ops *ops = vdomain->mm.ops;
> +
> +	if (ops)
> +		return ops->iova_to_phys(ops, iova);
>  
>  	spin_lock_irqsave(&vdomain->mappings_lock, flags);
>  	node = interval_tree_iter_first(&vdomain->mappings, iova, iova);
> @@ -1264,7 +1572,12 @@ static int viommu_probe(struct virtio_device *vdev)
>  				struct virtio_iommu_config, probe_size,
>  				&viommu->probe_size);
>  
> +	viommu->has_table = virtio_has_feature(vdev, VIRTIO_IOMMU_F_ATTACH_TABLE);
>  	viommu->has_map = virtio_has_feature(vdev, VIRTIO_IOMMU_F_MAP_UNMAP);
> +	if (!viommu->has_table && !viommu->has_map) {
> +		ret = -EINVAL;
> +		goto err_free_vqs;
> +	}
>  
>  	viommu->geometry = (struct iommu_domain_geometry) {
>  		.aperture_start	= input_start,
> @@ -1356,6 +1669,7 @@ static unsigned int features[] = {
>  	VIRTIO_IOMMU_F_DOMAIN_RANGE,
>  	VIRTIO_IOMMU_F_PROBE,
>  	VIRTIO_IOMMU_F_MMIO,
> +	VIRTIO_IOMMU_F_ATTACH_TABLE,
>  };
>  
>  static struct virtio_device_id id_table[] = {
> -- 
> 2.17.1
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v1 15/15] iommu/virtio: Update fault type and reason info for viommu fault

[Jean-Philippe Brucker]

On Fri, Jan 15, 2021 at 05:43:42PM +0530, Vivek Gautam wrote:
> Fault type information can tell about a page request fault or
> an unreceoverable fault, and further additions to fault reasons
> and the related PASID information can help in handling faults
> efficiently.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/iommu/virtio-iommu.c      | 27 +++++++++++++++++++++++++--
>  include/uapi/linux/virtio_iommu.h | 13 ++++++++++++-
>  2 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 9cc3d35125e9..10ef9e98214a 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -652,9 +652,16 @@ static int viommu_fault_handler(struct viommu_dev *viommu,
>  	char *reason_str;
>  
>  	u8 reason	= fault->reason;
> +	u16 type	= fault->flt_type;
>  	u32 flags	= le32_to_cpu(fault->flags);
>  	u32 endpoint	= le32_to_cpu(fault->endpoint);
>  	u64 address	= le64_to_cpu(fault->address);
> +	u32 pasid	= le32_to_cpu(fault->pasid);
> +
> +	if (type == VIRTIO_IOMMU_FAULT_F_PAGE_REQ) {
> +		dev_info(viommu->dev, "Page request fault - unhandled\n");
> +		return 0;
> +	}
>  
>  	switch (reason) {
>  	case VIRTIO_IOMMU_FAULT_R_DOMAIN:
> @@ -663,6 +670,21 @@ static int viommu_fault_handler(struct viommu_dev *viommu,
>  	case VIRTIO_IOMMU_FAULT_R_MAPPING:
>  		reason_str = "page";
>  		break;
> +	case VIRTIO_IOMMU_FAULT_R_WALK_EABT:
> +		reason_str = "page walk external abort";
> +		break;
> +	case VIRTIO_IOMMU_FAULT_R_PTE_FETCH:
> +		reason_str = "pte fetch";
> +		break;
> +	case VIRTIO_IOMMU_FAULT_R_PERMISSION:
> +		reason_str = "permission";
> +		break;
> +	case VIRTIO_IOMMU_FAULT_R_ACCESS:
> +		reason_str = "access";
> +		break;
> +	case VIRTIO_IOMMU_FAULT_R_OOR_ADDRESS:
> +		reason_str = "output address";
> +		break;
>  	case VIRTIO_IOMMU_FAULT_R_UNKNOWN:
>  	default:
>  		reason_str = "unknown";
> @@ -671,8 +693,9 @@ static int viommu_fault_handler(struct viommu_dev *viommu,
>  
>  	/* TODO: find EP by ID and report_iommu_fault */
>  	if (flags & VIRTIO_IOMMU_FAULT_F_ADDRESS)
> -		dev_err_ratelimited(viommu->dev, "%s fault from EP %u at %#llx [%s%s%s]\n",
> -				    reason_str, endpoint, address,
> +		dev_err_ratelimited(viommu->dev,
> +				    "%s fault from EP %u PASID %u at %#llx [%s%s%s]\n",
> +				    reason_str, endpoint, pasid, address,
>  				    flags & VIRTIO_IOMMU_FAULT_F_READ ? "R" : "",
>  				    flags & VIRTIO_IOMMU_FAULT_F_WRITE ? "W" : "",
>  				    flags & VIRTIO_IOMMU_FAULT_F_EXEC ? "X" : "");
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> index 608c8d642e1f..a537d82777f7 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -290,19 +290,30 @@ struct virtio_iommu_req_invalidate {
>  #define VIRTIO_IOMMU_FAULT_R_UNKNOWN		0
>  #define VIRTIO_IOMMU_FAULT_R_DOMAIN		1
>  #define VIRTIO_IOMMU_FAULT_R_MAPPING		2
> +#define VIRTIO_IOMMU_FAULT_R_WALK_EABT		3
> +#define VIRTIO_IOMMU_FAULT_R_PTE_FETCH		4
> +#define VIRTIO_IOMMU_FAULT_R_PERMISSION		5
> +#define VIRTIO_IOMMU_FAULT_R_ACCESS		6
> +#define VIRTIO_IOMMU_FAULT_R_OOR_ADDRESS	7
>  
>  #define VIRTIO_IOMMU_FAULT_F_READ		(1 << 0)
>  #define VIRTIO_IOMMU_FAULT_F_WRITE		(1 << 1)
>  #define VIRTIO_IOMMU_FAULT_F_EXEC		(1 << 2)
>  #define VIRTIO_IOMMU_FAULT_F_ADDRESS		(1 << 8)
>  
> +#define VIRTIO_IOMMU_FAULT_F_DMA_UNRECOV	1
> +#define VIRTIO_IOMMU_FAULT_F_PAGE_REQ		2

Currently all reported faults are unrecoverable, so to be consistent
DMA_UNRECOV should be 0. But I'd prefer having just a new "page request"
flag in the flags field, instead of the flt_type field.

For page requests we'll also need a 16-bit fault ID field to store the PRI
"page request group index" or the stall "stag". "last" and "privileged"
flags as well, to match the PRI page request. And a new command to
complete a page fault.

> +
>  struct virtio_iommu_fault {
>  	__u8					reason;
> -	__u8					reserved[3];
> +	__le16					flt_type;
> +	__u8					reserved;
>  	__le32					flags;
>  	__le32					endpoint;
>  	__u8					reserved2[4];

Why not replace reserved2 with the pasid?  It fits perfectly :)

Thanks,
Jean

>  	__le64					address;
> +	__le32					pasid;
> +	__u8					reserved3[4];
>  };
>  
>  #endif
> -- 
> 2.17.1
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH RFC v1 12/15] iommu/virtio: Add support for INVALIDATE request

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Thursday, March 4, 2021 2:29 AM
> 
> Hi Vivek,
> 
> On Fri, 15 Jan 2021 17:43:39 +0530, Vivek Gautam <vivek.gautam@arm.com>
> wrote:
> 
> > From: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> >
> > Add support for tlb invalidation ops that can send invalidation
> > requests to back-end virtio-iommu when stage-1 page tables are
> > supported.
> >
> Just curious if it possible to reuse the iommu uapi for invalidation and others.
> When we started out designing the iommu uapi, the intention was to support
> both emulated and virtio iommu.

IIUC this patch is about the protocol between virtio-iommu frontend and backend.
After the virtio-iommu backend receives invalidation ops, it then needs to
forward the request to the host IOMMU driver through the existing iommu
uapi that you referred to, as a emulated VT-d or SMMU would do.

Thanks
Kevin

> 
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> > [Vivek: Refactoring the iommu_flush_ops, and adding only one pasid sync
> >         op that's needed with current iommu-pasid-table infrastructure.
> > 	Also updating uapi defines as required by latest changes]
> > Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> > Cc: Joerg Roedel <joro@8bytes.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Cc: Eric Auger <eric.auger@redhat.com>
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Cc: Liu Yi L <yi.l.liu@intel.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  drivers/iommu/virtio-iommu.c | 95
> ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> >
> > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > index ae5dfd3f8269..004ea94e3731 100644
> > --- a/drivers/iommu/virtio-iommu.c
> > +++ b/drivers/iommu/virtio-iommu.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/freezer.h>
> >  #include <linux/interval_tree.h>
> >  #include <linux/iommu.h>
> > +#include <linux/io-pgtable.h>
> >  #include <linux/module.h>
> >  #include <linux/of_iommu.h>
> >  #include <linux/of_platform.h>
> > @@ -63,6 +64,8 @@ struct viommu_mapping {
> >  };
> >
> >  struct viommu_mm {
> > +	int				pasid;
> > +	u64				archid;
> >  	struct io_pgtable_ops		*ops;
> >  	struct viommu_domain		*domain;
> >  };
> > @@ -692,6 +695,98 @@ static void viommu_event_handler(struct
> virtqueue
> > *vq) virtqueue_kick(vq);
> >  }
> >
> > +/* PASID and pgtable APIs */
> > +
> > +static void __viommu_flush_pasid_tlb_all(struct viommu_domain
> *vdomain,
> > +					 int pasid, u64 arch_id, int
> > type) +{
> > +	struct virtio_iommu_req_invalidate req = {
> > +		.head.type	= VIRTIO_IOMMU_T_INVALIDATE,
> > +		.inv_gran	=
> > cpu_to_le32(VIRTIO_IOMMU_INVAL_G_PASID),
> > +		.flags		=
> > cpu_to_le32(VIRTIO_IOMMU_INVAL_F_PASID),
> > +		.inv_type	= cpu_to_le32(type),
> > +
> > +		.domain		= cpu_to_le32(vdomain->id),
> > +		.pasid		= cpu_to_le32(pasid),
> > +		.archid		= cpu_to_le64(arch_id),
> > +	};
> > +
> > +	if (viommu_send_req_sync(vdomain->viommu, &req, sizeof(req)))
> > +		pr_debug("could not send invalidate request\n");
> > +}
> > +
> > +static void viommu_flush_tlb_add(struct iommu_iotlb_gather *gather,
> > +				 unsigned long iova, size_t granule,
> > +				 void *cookie)
> > +{
> > +	struct viommu_mm *viommu_mm = cookie;
> > +	struct viommu_domain *vdomain = viommu_mm->domain;
> > +	struct iommu_domain *domain = &vdomain->domain;
> > +
> > +	iommu_iotlb_gather_add_page(domain, gather, iova, granule);
> > +}
> > +
> > +static void viommu_flush_tlb_walk(unsigned long iova, size_t size,
> > +				  size_t granule, void *cookie)
> > +{
> > +	struct viommu_mm *viommu_mm = cookie;
> > +	struct viommu_domain *vdomain = viommu_mm->domain;
> > +	struct virtio_iommu_req_invalidate req = {
> > +		.head.type	= VIRTIO_IOMMU_T_INVALIDATE,
> > +		.inv_gran	= cpu_to_le32(VIRTIO_IOMMU_INVAL_G_VA),
> > +		.inv_type	=
> cpu_to_le32(VIRTIO_IOMMU_INV_T_IOTLB),
> > +		.flags		=
> > cpu_to_le32(VIRTIO_IOMMU_INVAL_F_ARCHID), +
> > +		.domain		= cpu_to_le32(vdomain->id),
> > +		.pasid		= cpu_to_le32(viommu_mm->pasid),
> > +		.archid		= cpu_to_le64(viommu_mm->archid),
> > +		.virt_start	= cpu_to_le64(iova),
> > +		.nr_pages	= cpu_to_le64(size / granule),
> > +		.granule	= ilog2(granule),
> > +	};
> > +
> > +	if (viommu_add_req(vdomain->viommu, &req, sizeof(req)))
> > +		pr_debug("could not add invalidate request\n");
> > +}
> > +
> > +static void viommu_flush_tlb_all(void *cookie)
> > +{
> > +	struct viommu_mm *viommu_mm = cookie;
> > +
> > +	if (!viommu_mm->archid)
> > +		return;
> > +
> > +	__viommu_flush_pasid_tlb_all(viommu_mm->domain,
> viommu_mm->pasid,
> > +				     viommu_mm->archid,
> > +				     VIRTIO_IOMMU_INV_T_IOTLB);
> > +}
> > +
> > +static struct iommu_flush_ops viommu_flush_ops = {
> > +	.tlb_flush_all		= viommu_flush_tlb_all,
> > +	.tlb_flush_walk		= viommu_flush_tlb_walk,
> > +	.tlb_add_page		= viommu_flush_tlb_add,
> > +};
> > +
> > +static void viommu_flush_pasid(void *cookie, int pasid, bool leaf)
> > +{
> > +	struct viommu_domain *vdomain = cookie;
> > +	struct virtio_iommu_req_invalidate req = {
> > +		.head.type	= VIRTIO_IOMMU_T_INVALIDATE,
> > +		.inv_gran	=
> > cpu_to_le32(VIRTIO_IOMMU_INVAL_G_PASID),
> > +		.inv_type	=
> cpu_to_le32(VIRTIO_IOMMU_INV_T_PASID),
> > +		.flags		=
> > cpu_to_le32(VIRTIO_IOMMU_INVAL_F_PASID), +
> > +		.domain		= cpu_to_le32(vdomain->id),
> > +		.pasid		= cpu_to_le32(pasid),
> > +	};
> > +
> > +	if (leaf)
> > +		req.flags	|=
> > cpu_to_le32(VIRTIO_IOMMU_INVAL_F_LEAF); +
> > +	if (viommu_send_req_sync(vdomain->viommu, &req, sizeof(req)))
> > +		pr_debug("could not send invalidate request\n");
> > +}
> > +
> >  /* IOMMU API */
> >
> >  static struct iommu_domain *viommu_domain_alloc(unsigned type)
> 
> 
> Thanks,
> 
> Jacob
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v1 13/15] iommu/virtio: Attach Arm PASID tables when available

[Jean-Philippe Brucker]

On Fri, Mar 12, 2021 at 06:59:17PM +0530, Vivek Kumar Gautam wrote:
> > > +	/* XXX HACK: set feature bit ARM_SMMU_FEAT_2_LVL_CDTAB */
> > > +	pst_cfg->vendor.cfg.feat_flag |= (1 << 1);
> > 
> > Oh right, this flag is missing. I'll add
> > 
> >    #define VIRTIO_IOMMU_PST_ARM_SMMU3_F_CD2L (1ULL << 1)
> > 
> > to the spec.
> 
> Regarding this Eric pointed out [1] in my other patch about the scalability
> of the approach where we keep adding flags in 'iommu_nesting_info'
> corresponding to the arm-smmu-v3 capabilities. I guess the same goes to
> these flags in virtio.
> May be the 'iommu_nesting_info' can have a bitmap with the caps for vendor
> specific features, and here we can add the related flags?

Something like that, but I'd keep separate arch-specific structs. Vt-d
reports the capability registers directly through iommu_nesting_info [2].
We could do the same for Arm, copy sanitized values of IDR0..5 into
struct iommu_nesting_info_arm_smmuv3.

I've avoided doing that for virtio-iommu because every field needs a
description in the spec. So where possible I used generic properties that
apply to any architecture, such as page, PASID and address size. What's
left is the minimum arch-specific information to get nested translation
going, leaving out a lot of properties such as big-endian and 32-bit,
which can be added later if needed. The Arm specific properties are split
into page table and pasid table information. Page table info should work
for both SMMUv2 and v3 (where they correspond to an SMMU_IDRx field that
constrains a context descriptor field.) I should move BTM in there since
it's supported by SMMUv2.

Thanks,
Jean

[2] https://lore.kernel.org/linux-iommu/20210302203545.436623-11-yi.l.liu@intel.com/
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v1 15/15] iommu/virtio: Update fault type and reason info for viommu fault

[Jean-Philippe Brucker]

On Fri, Mar 12, 2021 at 06:39:05PM +0530, Vivek Kumar Gautam wrote:
> To complete the page request we would also need to send the response back to
> the host from virtio backend when handling page request. So the virtio
> command should also be accompanied with a vfio api to send the page request
> response back to the host. Isn't it?
> This is where the host smmuv3 can send PRI_RESP command to the device to
> complete the page fault.

It looks like Eric already has this in the VFIO series:
https://lore.kernel.org/linux-iommu/20210223210625.604517-14-eric.auger@redhat.com/

Thanks,
Jean
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Jean-Philippe Brucker]

On Fri, Mar 12, 2021 at 06:17:55PM +0530, Vivek Kumar Gautam wrote:
> > 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?

Yes. And we can support nested translation with SMMUv2 that way. But with
SMMUv3 the guest has to manage the whole PASID 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?

Makes sense

Thanks,
Jean
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization