Re: [PATCH v3 04/23] xen/arm: vIOMMU: add generic vIOMMU framework

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Milan,

> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 94b9858ad2..241f87386b 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -28,6 +28,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"
> @@ -550,6 +551,14 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>         return -EINVAL;
>     }
> 
> +    /* Check config structure padding */
> +    if ( config->arch.pad )
> +    {
> +        dprintk(XENLOG_INFO,
> +            "Invalid input config, padding must be zero\n");
> +        return -EINVAL;
> +    }
> +

This feels ok but unrelated to the patch, but also the text maybe should be 
something like “Invalid domain configuration during domain creation\n”.

>     /* Check feature flags */
>     if ( sve_vl_bits > 0 )
>     {
> @@ -626,6 +635,21 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>         return -EINVAL;
>     }
> 
> +    if ( !(config->flags & XEN_DOMCTL_CDF_iommu) &&
> +         config->arch.viommu_type != XEN_DOMCTL_CONFIG_VIOMMU_NONE )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "vIOMMU requested while iommu not enabled for domain\n");
> +        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");
> +        return -EINVAL;
> +    }
> +
>     return sci_domain_sanitise_config(config);
> }
> 
> @@ -721,6 +745,9 @@ int arch_domain_create(struct domain *d,
>     if ( (rc = sci_domain_init(d, config)) != 0 )
>         goto fail;
> 
> +    if ( (rc = domain_viommu_init(d, config->arch.viommu_type)) != 0 )
> +        goto fail;
> +
>     return 0;
> 
> fail:
> @@ -965,6 +992,7 @@ enum {
>     PROG_pci = 1,
>     PROG_sci,
>     PROG_tee,
> +    PROG_viommu,
>     PROG_xen,
>     PROG_page,
>     PROG_mapping,
> @@ -1021,6 +1049,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 e8795745dd..a51563ee3d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -35,6 +35,7 @@
> #include <asm/arm64/sve.h>
> #include <asm/cpufeature.h>
> #include <asm/domain_build.h>
> +#include <asm/viommu.h>

NIT: In my local branch I’ve rebased this on top of new staging

> 
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index cd563cf706..d4953d40fd 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -330,6 +330,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> #define XEN_DOMCTL_CONFIG_ARM_SCI_NONE      0
> #define XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC  1
> 
> +#define XEN_DOMCTL_CONFIG_VIOMMU_NONE   0
> +
> struct xen_arch_domainconfig {
>     /* IN/OUT */
>     uint8_t gic_version;
> @@ -355,6 +357,9 @@ struct xen_arch_domainconfig {
>     uint32_t clock_frequency;
>     /* IN */
>     uint8_t arm_sci_type;
> +    /* IN */
> +    uint8_t viommu_type;
> +    uint16_t pad;

Having the padding explicit feels ok to me, but I would rely on maintainer
choice.

> };
> #endif /* __XEN__ || __XEN_TOOLS__ */
> 
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 8f6708c0a7..23124547f3 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -30,9 +30,9 @@
>  * fields) don't require a change of the version.
>  * Stable ops are NOT covered by XEN_DOMCTL_INTERFACE_VERSION!
>  *
> - * Last version bump: Xen 4.19
> + * Last version bump: Xen 4.22
>  */
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000018

I don’t think the changes in this patch are breaking the ABI, so this should 
not be bumped;
said so, I would rely on @Andrew or another maintainer for this

Cheers,
Luca