Re: [Xen-devel] [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On Thu, May 11, 2017 at 2:42 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Oleksandr,
Hi Julien

>
>
> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> This flag is intended to let Xen know that the guest has devices
>> which will most likely be used for passthrough and as the result
>> the use of IOMMU is expected for this domain.
>> The primary aim of this knowledge is to help the IOMMUs that don't
>> share page tables with the CPU on ARM be ready before P2M code starts
>> updating IOMMU mapping.
>> So, if this flag is set the non-shared IOMMUs will populate
>> their page tables at the domain creation time and thereby will be able
>> to handle IOMMU mapping updates from *the very beginning*.
>>
>> In order to retain the current behavior for x86 still call
>> iommu_domain_init() with use_iommu flag being forced to false.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxx>
>> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>>
>> ---
>>    Changes in V1:
>>       - Treat use_iommu flag as the ARM decision only. Don't use
>>         common domain creation flag for it, use ARM config instead.
>>       - Clarify patch subject/description.
>> ---
>>  tools/libxl/libxl_arm.c       | 10 ++++++++++
>>  xen/arch/arm/domain.c         |  2 +-
>>  xen/include/public/arch-arm.h |  5 +++++
>>  3 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index d842d88..9c4705e 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>          return ERROR_FAIL;
>>      }
>>
>> +    /* TODO Are these assumptions enough to make decision about using
>> IOMMU? */
>> +    if ((d_config->num_dtdevs && d_config->dtdevs) ||
>> +        (d_config->num_pcidevs && d_config->pcidevs))
>
>
> Checking num_dtdevs and num_pcidevs is enough. It would be a bug if dtdevs
> and pcidevs are not null.
ok

>
>> +        xc_config->use_iommu = 1;
>> +    else
>> +        xc_config->use_iommu = 0;
>> +
>> +    LOG(DEBUG, "The use of IOMMU %s expected for this domain",
>> +        xc_config->use_iommu ? "is" : "isn't");
>> +
>>      return 0;
>>  }
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index ec19310..81c4b90 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int
>> domcr_flags,
>>      ASSERT(config != NULL);
>>
>>      /* p2m_init relies on some value initialized by the IOMMU subsystem
>> */
>> -    if ( (rc = iommu_domain_init(d, false)) != 0 )
>> +    if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) !=
>> 0 )
>
>
> !!config->use_iommu is enough.
ok

>
>>          goto fail;
>>
>>      if ( (rc = p2m_init(d)) != 0 )
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index bd974fb..cb33f75 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -322,6 +322,11 @@ struct xen_arch_domainconfig {
>>       *
>>       */
>>      uint32_t clock_frequency;
>> +    /*
>> +     * IN
>> +     * Inform the hypervisor that the use of IOMMU is expected for this
>> domain.
>
>
> I would simplify to : "IOMMU is expected to be used for this domain".
ok

>
>> +     */
>> +    uint8_t use_iommu;
>>  };
>>  #endif /* __XEN__ || __XEN_TOOLS__ */
>>
>>
>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel