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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Rahul,

On 05/12/2022 14:53, Rahul Singh wrote:
> 
> 
> 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.
Also I think that you need to bump XEN_DOMCTL_INTERFACE_VERSION due to the 
change
in struct xen_arch_domainconfig.

> 
> Regards,
> Rahul
> 
> 
~Michal