Re: [PATCH v5 4/5] [FUTURE] xen/arm: enable vPCI for domUs

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 11/6/23 04:26, Jan Beulich wrote:
> On 02.11.2023 20:59, Stewart Hildebrand wrote:
>> --- a/xen/arch/x86/include/asm/domain.h
>> +++ b/xen/arch/x86/include/asm/domain.h
>> @@ -503,6 +503,15 @@ struct arch_domain
>>  #define has_vpit(d)        (!!((d)->arch.emulation_flags & X86_EMU_PIT))
>>  #define has_pirq(d)        (!!((d)->arch.emulation_flags & 
>> X86_EMU_USE_PIRQ))
>>  
>> +#define is_pvh_domain(d) ({                                  \
>> +    unsigned int _emflags = (d)->arch.emulation_flags;       \
>> +    IS_ENABLED(CONFIG_HVM) &&                                \
>> +        ((_emflags == X86_EMU_LAPIC) ||                      \
>> +         (_emflags == (X86_EMU_LAPIC | X86_EMU_IOAPIC))); })
> 
> I'm not convinced we want to re-introduce such a predicate; it'll be at
> risk of going stale when the boundary between HVM and PVH becomes more
> "fuzzy".

OK, I'll drop it

> 
>> +/* PCI passthrough may be backed by qemu for non-PVH domains */
>> +#define arch_needs_vpci(d) is_pvh_domain(d)
> 
> Wouldn't we want to check for exactly what the comment alludes to then,
> i.e. whether the domain has any (specific?) device model attached?

This patch is primarily dealing with Arm, so I'm considering simply making it 
return false for now:

#define arch_needs_vpci(d) ({ (void)(d); false; })

If it needs to be changed in the future when we enable vPCI for PVH domUs, we 
can deal with it at that time.

> 
>> --- a/xen/include/xen/domain.h
>> +++ b/xen/include/xen/domain.h
>> @@ -51,8 +51,17 @@ void arch_get_domain_info(const struct domain *d,
>>  
>>  #define is_domain_using_staticmem(d) ((d)->cdf & CDF_staticmem)
>>  
>> -#define has_vpci(d) (((d)->options & XEN_DOMCTL_CDF_vpci) && \
>> -                     IS_ENABLED(CONFIG_HAS_VPCI))
>> +#define has_vpci(d) ({                                                      
>>   \
>> +    const struct domain *_d = (d);                                          
>>   \
>> +    bool _has_vpci = false;                                                 
>>   \
>> +    if ( (_d->options & XEN_DOMCTL_CDF_vpci) && IS_ENABLED(CONFIG_HAS_VPCI) 
>> ) \
>> +    {                                                                       
>>   \
>> +        if ( is_hardware_domain(_d) )                                       
>>   \
>> +            _has_vpci = true;                                               
>>   \
>> +        else if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )               
>>   \
>> +            _has_vpci = true;                                               
>>   \
>> +    }                                                                       
>>   \
>> +    _has_vpci; })
> 
> This is a commonly executed check, and as such wants to remain as simple as
> possible. Wouldn't it be better anyway to prevent XEN_DOMCTL_CDF_vpci getting
> set for a domain which cannot possibly have vPCI?

Yes, agreed, I'll leave has_vpci alone

> 
> Jan