Re: [PATCH v3 2/5] xen/arm: pci: plumb xen_arch_domainconfig with pci info

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 10/20/23 13:25, Julien Grall wrote:
> On 09/10/2023 20:57, Stewart Hildebrand wrote:
>> Add a flag to struct xen_arch_domainconfig to allow specifying at domain
>> creation time whether the domain uses vPCI.
>>
>> Add a corresponding flag to struct arch_domain to indicate vPCI and set it
>> accordingly.
> 
> The new boolean you are adding doesn't seem very arch specific. So could
> we use a bit in xen_domctl_createdomain.flags?

Right. Yes. Since x86 already has a vpci flag in 
xen_domctl_createdomain.arch.emulation_flags, I think the way forward is to 
move the x86 vpci flag to the common xen_domctl_createdomain.flags.

> 
>>
>> Bump XEN_DOMCTL_INTERFACE_VERSION since we're modifying struct
>> xen_arch_domainconfig.
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>> ---
>> v2->v3:
>> * new patch
>> ---
>>   xen/arch/arm/domain.c             | 10 ++++++++++
>>   xen/arch/arm/include/asm/domain.h |  2 ++
>>   xen/include/public/arch-arm.h     |  4 ++++
>>   xen/include/public/domctl.h       |  2 +-
>>   4 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 28e3aaa5e482..9470c28595da 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -687,6 +687,13 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>           return -EINVAL;
>>       }
>>
>> +    if ( (config->arch.pci_flags & XEN_DOMCTL_CONFIG_PCI_VPCI) &&
>> +         !IS_ENABLED(CONFIG_HAS_VPCI) )
>> +    {
>> +        dprintk(XENLOG_INFO, "vPCI support not enabled\n");
>> +        return -EINVAL;
>> +    }
>> +
>>       return 0;
>>   }
>>
>> @@ -737,6 +744,9 @@ int arch_domain_create(struct domain *d,
>>           BUG();
>>       }
>>
>> +    if ( config->arch.pci_flags & XEN_DOMCTL_CONFIG_PCI_VPCI )
>> +        d->arch.has_vpci = true;
>> +
>>       if ( (rc = domain_vgic_register(d, &count)) != 0 )
>>           goto fail;
>>
>> diff --git a/xen/arch/arm/include/asm/domain.h 
>> b/xen/arch/arm/include/asm/domain.h
>> index 99e798ffff68..0a66ed09a617 100644
>> --- a/xen/arch/arm/include/asm/domain.h
>> +++ b/xen/arch/arm/include/asm/domain.h
>> @@ -119,6 +119,8 @@ struct arch_domain
>>       void *tee;
>>   #endif
>>
>> +    bool has_vpci;
>> +
>>   }  __cacheline_aligned;
>>
>>   struct arch_vcpu
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index 6a4467e8f5d1..5c8424341aad 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -300,6 +300,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>>   #define XEN_DOMCTL_CONFIG_TEE_OPTEE     1
>>   #define XEN_DOMCTL_CONFIG_TEE_FFA       2
>>
>> +#define XEN_DOMCTL_CONFIG_PCI_VPCI      (1U << 0)
>> +
>>   struct xen_arch_domainconfig {
>>       /* IN/OUT */
>>       uint8_t gic_version;
>> @@ -323,6 +325,8 @@ struct xen_arch_domainconfig {
>>        *
>>        */
>>       uint32_t clock_frequency;
>> +    /* IN */
>> +    uint8_t pci_flags;
> 
> Regardless what I wrote above. Do you have any plan for adding more PCI
> specific flags?

No

> If not, then if you want to keep the flag Arm specific,
> then I think this should be named pci_flags.
> 
> As you add a new field, I believe, you also want to modoify the various
> language specific bindings (e.g. go, ocaml).

OK

> 
>>   };
>>   #endif /* __XEN__ || __XEN_TOOLS__ */
>>
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index a33f9ec32b08..dcd42b3d603d 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -21,7 +21,7 @@
>>   #include "hvm/save.h"
>>   #include "memory.h"
>>
>> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016
>> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017
>>
>>   /*
>>    * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> 
> Cheers,
> 
> -- 
> Julien Grall