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

[Julien Grall <julien@xxxxxxx>]

On 10/04/2026 19:41, Luca Fancellu wrote:
> 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
In general, we want to have padding explictly define and zeroed. I 
wouldn't mind if this is kept in this patch but this would need to be 
mentioned in the commit message.
> , 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.
See above. To expand what I wrote, if we don't define the padding 
explicitly and then check against zero, then we can't re-use them 
without breaking the ABI. So...
> > };
> > #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
... this is needed because older toolstack may not zero the field.

Cheers,

--
Julien Grall