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

[Jan Beulich <jbeulich@xxxxxxxx>]

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".

> +/* 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?

> --- 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?

Jan