Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Michal,

> On 5 Dec 2022, at 8:26 am, Michal Orzel <michal.orzel@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> On 01/12/2022 17:02, Rahul Singh wrote:
>> 
>> 
>> This patch adds basic framework for vIOMMU.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> ---
>> xen/arch/arm/domain.c                | 17 +++++++
>> xen/arch/arm/domain_build.c          |  3 ++
>> xen/arch/arm/include/asm/viommu.h    | 70 ++++++++++++++++++++++++++++
>> xen/drivers/passthrough/Kconfig      |  6 +++
>> xen/drivers/passthrough/arm/Makefile |  1 +
>> xen/drivers/passthrough/arm/viommu.c | 48 +++++++++++++++++++
>> xen/include/public/arch-arm.h        |  4 ++
>> 7 files changed, 149 insertions(+)
>> create mode 100644 xen/arch/arm/include/asm/viommu.h
>> create mode 100644 xen/drivers/passthrough/arm/viommu.c
>> 
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 38e22f12af..2a85209736 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -37,6 +37,7 @@
>> #include <asm/tee/tee.h>
>> #include <asm/vfp.h>
>> #include <asm/vgic.h>
>> +#include <asm/viommu.h>
>> #include <asm/vtimer.h>
>> 
>> #include "vpci.h"
>> @@ -691,6 +692,13 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>         return -EINVAL;
>>     }
>> 
>> +    if ( config->arch.viommu_type != XEN_DOMCTL_CONFIG_VIOMMU_NONE )
>> +    {
>> +        dprintk(XENLOG_INFO,
>> +                "vIOMMU type requested not supported by the platform or 
>> Xen\n");
> Maybe a simpler message like for TEE would be better: "Unsupported vIOMMU 
> type"

Ack. 
> 
>> +        return -EINVAL;
>> +    }
>> +
>>     return 0;
>> }
>> 
>> @@ -783,6 +791,9 @@ int arch_domain_create(struct domain *d,
>>     if ( (rc = domain_vpci_init(d)) != 0 )
>>         goto fail;
>> 
>> +    if ( (rc = domain_viommu_init(d, config->arch.viommu_type)) != 0 )
>> +        goto fail;
>> +
>>     return 0;
>> 
>> fail:
>> @@ -998,6 +1009,7 @@ static int relinquish_memory(struct domain *d, struct 
>> page_list_head *list)
>> enum {
>>     PROG_pci = 1,
>>     PROG_tee,
>> +    PROG_viommu,
>>     PROG_xen,
>>     PROG_page,
>>     PROG_mapping,
>> @@ -1048,6 +1060,11 @@ int domain_relinquish_resources(struct domain *d)
>>         if (ret )
>>             return ret;
>> 
>> +    PROGRESS(viommu):
>> +        ret = viommu_relinquish_resources(d);
>> +        if (ret )
>> +            return ret;
>> +
>>     PROGRESS(xen):
>>         ret = relinquish_memory(d, &d->xenpage_list);
>>         if ( ret )
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index bd30d3798c..abbaf37a2e 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -27,6 +27,7 @@
>> #include <asm/setup.h>
>> #include <asm/cpufeature.h>
>> #include <asm/domain_build.h>
>> +#include <asm/viommu.h>
>> #include <xen/event.h>
>> 
>> #include <xen/irq.h>
>> @@ -3858,6 +3859,7 @@ void __init create_domUs(void)
>>         struct domain *d;
>>         struct xen_domctl_createdomain d_cfg = {
>>             .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
>> +            .arch.viommu_type = viommu_get_type(),
>>             .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>>             /*
>>              * The default of 1023 should be sufficient for guests because
>> @@ -4052,6 +4054,7 @@ void __init create_dom0(void)
>>         printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>>     dom0_cfg.arch.tee_type = tee_get_type();
>>     dom0_cfg.max_vcpus = dom0_max_vcpus();
>> +    dom0_cfg.arch.viommu_type = viommu_get_type();
>> 
>>     if ( iommu_enabled )
>>         dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>> diff --git a/xen/arch/arm/include/asm/viommu.h 
>> b/xen/arch/arm/include/asm/viommu.h
>> new file mode 100644
>> index 0000000000..7cd3818a12
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/viommu.h
>> @@ -0,0 +1,70 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
>> +#ifndef __ARCH_ARM_VIOMMU_H__
>> +#define __ARCH_ARM_VIOMMU_H__
>> +
>> +#ifdef CONFIG_VIRTUAL_IOMMU
>> +
>> +#include <xen/lib.h>
>> +#include <xen/types.h>
>> +#include <public/xen.h>
>> +
>> +struct viommu_ops {
>> +    /*
>> +     * Called during domain construction if toolstack requests to enable
>> +     * vIOMMU support.
>> +     */
>> +    int (*domain_init)(struct domain *d);
>> +
>> +    /*
>> +     * Called during domain destruction to free resources used by vIOMMU.
>> +     */
>> +    int (*relinquish_resources)(struct domain *d);
>> +};
>> +
>> +struct viommu_desc {
>> +    /* vIOMMU domains init/free operations described above. */
>> +    const struct viommu_ops *ops;
>> +
>> +    /*
>> +     * ID of vIOMMU. Corresponds to xen_arch_domainconfig.viommu_type.
>> +     * Should be one of XEN_DOMCTL_CONFIG_VIOMMU_xxx
>> +     */
>> +    uint16_t viommu_type;
> Here the viommu_type is uint16_t ...
> 
>> +};
>> +
>> +int domain_viommu_init(struct domain *d, uint16_t viommu_type);
>> +int viommu_relinquish_resources(struct domain *d);
>> +uint16_t viommu_get_type(void);
>> +
>> +#else
>> +
>> +static inline uint8_t viommu_get_type(void)
> Here you are returning uint8_t ...
> 
>> +{
>> +    return XEN_DOMCTL_CONFIG_VIOMMU_NONE;
>> +}
>> +
>> +static inline int domain_viommu_init(struct domain *d, uint16_t viommu_type)
> Here you are taking uint16_t. So it looks like that ...
> 
>> +{
>> +    if ( likely(viommu_type == XEN_DOMCTL_CONFIG_VIOMMU_NONE) )
>> +        return 0;
>> +
>> +    return -ENODEV;
>> +}
>> +
>> +static inline int viommu_relinquish_resources(struct domain *d)
>> +{
>> +    return 0;
>> +}
>> +
>> +#endif /* CONFIG_VIRTUAL_IOMMU */
>> +
>> +#endif /* __ARCH_ARM_VIOMMU_H__ */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/drivers/passthrough/Kconfig 
>> b/xen/drivers/passthrough/Kconfig
>> index 479d7de57a..19924fa2de 100644
>> --- a/xen/drivers/passthrough/Kconfig
>> +++ b/xen/drivers/passthrough/Kconfig
>> @@ -35,6 +35,12 @@ config IPMMU_VMSA
>>          (H3 ES3.0, M3-W+, etc) or Gen4 SoCs which IPMMU hardware supports 
>> stage 2
>>          translation table format and is able to use CPU's P2M table as is.
>> 
>> +config VIRTUAL_IOMMU
>> +       bool "Virtual IOMMU Support (UNSUPPORTED)" if UNSUPPORTED
>> +       default n
>> +       help
>> +        Support virtual IOMMU infrastructure to implement vIOMMU.
>> +
>> endif
>> 
>> config IOMMU_FORCE_PT_SHARE
>> diff --git a/xen/drivers/passthrough/arm/Makefile 
>> b/xen/drivers/passthrough/arm/Makefile
>> index c5fb3b58a5..4cc54f3f4d 100644
>> --- a/xen/drivers/passthrough/arm/Makefile
>> +++ b/xen/drivers/passthrough/arm/Makefile
>> @@ -2,3 +2,4 @@ obj-y += iommu.o iommu_helpers.o iommu_fwspec.o
>> 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
>> diff --git a/xen/drivers/passthrough/arm/viommu.c 
>> b/xen/drivers/passthrough/arm/viommu.c
>> new file mode 100644
>> index 0000000000..7ab6061e34
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/arm/viommu.c
>> @@ -0,0 +1,48 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
>> +
>> +#include <xen/errno.h>
>> +#include <xen/init.h>
>> +#include <xen/types.h>
>> +
>> +#include <asm/viommu.h>
>> +
>> +const struct viommu_desc __read_mostly *cur_viommu;
>> +
>> +int domain_viommu_init(struct domain *d, uint16_t viommu_type)
>> +{
>> +    if ( viommu_type == XEN_DOMCTL_CONFIG_VIOMMU_NONE )
>> +        return 0;
>> +
>> +    if ( !cur_viommu )
>> +        return -ENODEV;
>> +
>> +    if ( cur_viommu->viommu_type != viommu_type )
>> +        return -EINVAL;
>> +
>> +    return cur_viommu->ops->domain_init(d);
>> +}
>> +
>> +int viommu_relinquish_resources(struct domain *d)
>> +{
>> +    if ( !cur_viommu )
>> +        return 0;
>> +
>> +    return cur_viommu->ops->relinquish_resources(d);
>> +}
>> +
>> +uint16_t viommu_get_type(void)
>> +{
>> +    if ( !cur_viommu )
>> +        return XEN_DOMCTL_CONFIG_VIOMMU_NONE;
>> +
>> +    return cur_viommu->viommu_type;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index 1528ced509..33d32835e7 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -297,10 +297,14 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>> #define XEN_DOMCTL_CONFIG_TEE_NONE      0
>> #define XEN_DOMCTL_CONFIG_TEE_OPTEE     1
>> 
>> +#define XEN_DOMCTL_CONFIG_VIOMMU_NONE   0
>> +
>> struct xen_arch_domainconfig {
>>     /* IN/OUT */
>>     uint8_t gic_version;
>>     /* IN */
>> +    uint8_t viommu_type;
> this should be uint16_t and not uint8_t 

I will modify the in viommu_type to uint8_t in "arch/arm/include/asm/viommu.h" 
and will
also fix  everywhere the viommu_type to uint8_t. 

Regards,
Rahul