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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 3 Dec 2022, at 9:54 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> On 01/12/2022 16: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");
>> +        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;
> 
> I would have expected us to relinquish the vIOMMU resources *before* we 
> detach the devices. So can you explain the ordering?

I think first we need to detach the device that will set the S1 and S2 stage 
translation to bypass/abort then we
need to remove the vIOMMU. Do you have anything that you feel if we relinquish 
the vIOMMU after detach is not right.

> 
>> +
>>      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(),
> 
> I don't think the vIOMMU should be enabled to dom0less domU by default.

I am not enabling the vIOMMU by default. If vIOMMU is disabled via the command 
line or config option
then viommu_get_type() will return XEN_DOMCTL_CONFIG_VIOMMU_NONE and in that 
case
domain_viommu_init() will return 0 without doing anything. 

> 
>>              .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();
> 
> Same here.
> 
>>        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.
> 
> Did you mean ID rather than type?

I mean here type of physical IOMMU on the host available SMMUv3, SMMUv1 or 
IPMMU.
If you think ID is the right name here I will change it.

> 
>> +     * Should be one of XEN_DOMCTL_CONFIG_VIOMMU_xxx
>> +     */
>> +    uint16_t viommu_type;
> 
> The domctl is uint8_t.

Ack. I will fix that.
 
Regards,
Rahul