Re: [RFC PATCH 05/21] xen/arm: vsmmuv3: Add dummy support for virtual SMMUv3 for guests

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Michal,

> On 5 Dec 2022, at 8:33 am, Michal Orzel <michal.orzel@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> On 01/12/2022 17:02, Rahul Singh wrote:
>> 
>> 
>> domain_viommu_init() will be called during domain creation and will add
>> the dummy trap handler for virtual IOMMUs for guests.
>> 
>> A host IOMMU list will be created when host IOMMU devices are probed
>> and this list will be used to create the IOMMU device tree node for
>> dom0. For dom0, 1-1 mapping will be established between vIOMMU in dom0
>> and physical IOMMU.
>> 
>> For domUs, the 1-N mapping will be established between domU and physical
>> IOMMUs. A new area has been reserved in the arm guest physical map at
>> which the emulated vIOMMU node is created in the device tree.
>> 
>> Also set the vIOMMU type to vSMMUv3 to enable vIOMMU framework to call
>> vSMMUv3 domain creation/destroy functions.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> ---
>> xen/arch/arm/domain.c                  |   3 +-
>> xen/arch/arm/include/asm/domain.h      |   4 +
>> xen/arch/arm/include/asm/viommu.h      |  20 ++++
>> xen/drivers/passthrough/Kconfig        |   8 ++
>> xen/drivers/passthrough/arm/Makefile   |   1 +
>> xen/drivers/passthrough/arm/smmu-v3.c  |   7 ++
>> xen/drivers/passthrough/arm/viommu.c   |  30 ++++++
>> xen/drivers/passthrough/arm/vsmmu-v3.c | 124 +++++++++++++++++++++++++
>> xen/drivers/passthrough/arm/vsmmu-v3.h |  20 ++++
>> xen/include/public/arch-arm.h          |   7 +-
>> 10 files changed, 222 insertions(+), 2 deletions(-)
>> create mode 100644 xen/drivers/passthrough/arm/vsmmu-v3.c
>> create mode 100644 xen/drivers/passthrough/arm/vsmmu-v3.h
>> 
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 2a85209736..9a2b613500 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -692,7 +692,8 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>         return -EINVAL;
>>     }
>> 
>> -    if ( config->arch.viommu_type != XEN_DOMCTL_CONFIG_VIOMMU_NONE )
>> +    if ( config->arch.viommu_type != XEN_DOMCTL_CONFIG_VIOMMU_NONE &&
>> +         config->arch.viommu_type != viommu_get_type() )
>>     {
>>         dprintk(XENLOG_INFO,
>>                 "vIOMMU type requested not supported by the platform or 
>> Xen\n");
>> diff --git a/xen/arch/arm/include/asm/domain.h 
>> b/xen/arch/arm/include/asm/domain.h
>> index 2ce6764322..8eb4eb5fd6 100644
>> --- a/xen/arch/arm/include/asm/domain.h
>> +++ b/xen/arch/arm/include/asm/domain.h
>> @@ -114,6 +114,10 @@ struct arch_domain
>>     void *tee;
>> #endif
>> 
>> +#ifdef CONFIG_VIRTUAL_IOMMU
>> +    struct list_head viommu_list;     /* List of virtual IOMMUs */
>> +#endif
>> +
>> }  __cacheline_aligned;
>> 
>> struct arch_vcpu
>> diff --git a/xen/arch/arm/include/asm/viommu.h 
>> b/xen/arch/arm/include/asm/viommu.h
>> index 7cd3818a12..4785877e2a 100644
>> --- a/xen/arch/arm/include/asm/viommu.h
>> +++ b/xen/arch/arm/include/asm/viommu.h
>> @@ -5,9 +5,21 @@
>> #ifdef CONFIG_VIRTUAL_IOMMU
>> 
>> #include <xen/lib.h>
>> +#include <xen/list.h>
>> #include <xen/types.h>
>> #include <public/xen.h>
>> 
>> +extern struct list_head host_iommu_list;
>> +
>> +/* data structure for each hardware IOMMU */
>> +struct host_iommu {
>> +    struct list_head entry;
>> +    const struct dt_device_node *dt_node;
>> +    paddr_t addr;
>> +    paddr_t size;
>> +    uint32_t irq;
> You want this to be int and not unsigned. The reason is ...
> 
>> +};
>> +
>> struct viommu_ops {
>>     /*
>>      * Called during domain construction if toolstack requests to enable
>> @@ -35,6 +47,8 @@ struct viommu_desc {
>> int domain_viommu_init(struct domain *d, uint16_t viommu_type);
>> int viommu_relinquish_resources(struct domain *d);
>> uint16_t viommu_get_type(void);
>> +void add_to_host_iommu_list(paddr_t addr, paddr_t size,
>> +                            const struct dt_device_node *node);
>> 
>> #else
>> 
>> @@ -56,6 +70,12 @@ static inline int viommu_relinquish_resources(struct 
>> domain *d)
>>     return 0;
>> }
>> 
>> +static inline void add_to_host_iommu_list(paddr_t addr, paddr_t size,
>> +                                          const struct dt_device_node *node)
>> +{
>> +    return;
>> +}
>> +
>> #endif /* CONFIG_VIRTUAL_IOMMU */
>> 
>> #endif /* __ARCH_ARM_VIOMMU_H__ */
>> diff --git a/xen/drivers/passthrough/Kconfig 
>> b/xen/drivers/passthrough/Kconfig
>> index 19924fa2de..4c725f5f67 100644
>> --- a/xen/drivers/passthrough/Kconfig
>> +++ b/xen/drivers/passthrough/Kconfig
>> @@ -41,6 +41,14 @@ config VIRTUAL_IOMMU
>>        help
>>         Support virtual IOMMU infrastructure to implement vIOMMU.
>> 
>> +config VIRTUAL_ARM_SMMU_V3
>> +       bool "ARM Ltd. Virtual SMMUv3 Support (UNSUPPORTED)" if UNSUPPORTED
>> +       depends on ARM_SMMU_V3 && VIRTUAL_IOMMU
>> +       help
>> +        Support for implementations of the virtual ARM System MMU 
>> architecture
>> +        version 3. Virtual SMMUv3 is unsupported feature and should not be 
>> used
>> +        in production.
>> +
>> endif
>> 
>> config IOMMU_FORCE_PT_SHARE
>> diff --git a/xen/drivers/passthrough/arm/Makefile 
>> b/xen/drivers/passthrough/arm/Makefile
>> index 4cc54f3f4d..e758a9d6aa 100644
>> --- a/xen/drivers/passthrough/arm/Makefile
>> +++ b/xen/drivers/passthrough/arm/Makefile
>> @@ -3,3 +3,4 @@ obj-$(CONFIG_ARM_SMMU) += smmu.o
>> obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o
>> obj-$(CONFIG_ARM_SMMU_V3) += smmu-v3.o
>> obj-$(CONFIG_VIRTUAL_IOMMU) += viommu.o
>> +obj-$(CONFIG_VIRTUAL_ARM_SMMU_V3) += vsmmu-v3.o
>> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
>> b/xen/drivers/passthrough/arm/smmu-v3.c
>> index 9174d2dedd..4f96fdb92f 100644
>> --- a/xen/drivers/passthrough/arm/smmu-v3.c
>> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
>> @@ -91,6 +91,7 @@
>> #include <asm/platform.h>
>> 
>> #include "smmu-v3.h"
>> +#include "vsmmu-v3.h"
>> 
>> #define ARM_SMMU_VTCR_SH_IS            3
>> #define ARM_SMMU_VTCR_RGN_WBWA         1
>> @@ -2680,6 +2681,9 @@ static int arm_smmu_device_probe(struct 
>> platform_device *pdev)
>>        list_add(&smmu->devices, &arm_smmu_devices);
>>        spin_unlock(&arm_smmu_devices_lock);
>> 
>> +    /* Add to host IOMMU list to initialize vIOMMU for dom0 */
>> +       add_to_host_iommu_list(ioaddr, iosize, dev_to_dt(pdev));
>> +
>>        return 0;
>> 
>> 
>> @@ -2936,6 +2940,9 @@ static __init int arm_smmu_dt_init(struct 
>> dt_device_node *dev,
>> 
>>        iommu_set_ops(&arm_smmu_iommu_ops);
>> 
>> +       /* Set vIOMMU type to SMMUv3 */
>> +       vsmmuv3_set_type();
>> +
>>        return 0;
>> }
>> 
>> diff --git a/xen/drivers/passthrough/arm/viommu.c 
>> b/xen/drivers/passthrough/arm/viommu.c
>> index 7ab6061e34..53ae46349a 100644
>> --- a/xen/drivers/passthrough/arm/viommu.c
>> +++ b/xen/drivers/passthrough/arm/viommu.c
>> @@ -2,12 +2,42 @@
>> 
>> #include <xen/errno.h>
>> #include <xen/init.h>
>> +#include <xen/irq.h>
>> #include <xen/types.h>
>> 
>> #include <asm/viommu.h>
>> 
>> +/* List of all host IOMMUs */
>> +LIST_HEAD(host_iommu_list);
>> +
>> const struct viommu_desc __read_mostly *cur_viommu;
>> 
>> +/* Common function for adding to host_iommu_list */
>> +void add_to_host_iommu_list(paddr_t addr, paddr_t size,
>> +                            const struct dt_device_node *node)
>> +{
>> +    struct host_iommu *iommu_data;
>> +
>> +    iommu_data = xzalloc(struct host_iommu);
>> +    if ( !iommu_data )
>> +        panic("vIOMMU: Cannot allocate memory for host IOMMU data\n");
>> +
>> +    iommu_data->addr = addr;
>> +    iommu_data->size = size;
>> +    iommu_data->dt_node = node;
>> +    iommu_data->irq = platform_get_irq(node, 0);
> you are assigning platform_get_irq to it which can return a negative number 
> in case of failure.
> In your current implementation, the following check does not make sense as 
> unsigned can never be negative.

Ack. I will fix that in next version.
 
Regards,
Rahul