Re: [PATCH v3 14/17] xen/arm: Enable the existing x86 virtual PCI support for ARM.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2021 13:44, Rahul Singh wrote:
>> On 30 Sep 2021, at 4:19 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 28.09.2021 20:18, Rahul Singh wrote:
>>> The existing VPCI support available for X86 is adapted for Arm.
>>> When the device is added to XEN via the hyper call
>>> “PHYSDEVOP_pci_device_add”, VPCI handler for the config space
>>> access is added to the Xen to emulate the PCI devices config space.
>>>
>>> A MMIO trap handler for the PCI ECAM space is registered in XEN
>>> so that when guest is trying to access the PCI config space,XEN
>>> will trap the access and emulate read/write using the VPCI and
>>> not the real PCI hardware.
>>
>> All of this is just for Dom0, I understand? Could you say so, perhaps
>> already in the title?
>>
> 
> DOMU guest will also use the same VPCI handler. When we assign the PCI 
> devices to DOMU guests
> XEN will deregister the VPCI handler from DOM0 and register it for DOMU 
> guests. 

But that logic is not fit for DomU use, is it? Aiui some of the other
series on the list are aiming to work towards making it usable.

>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -662,6 +662,12 @@ int arch_sanitise_domain_config(struct 
>>> xen_domctl_createdomain *config)
>>>         return -EINVAL;
>>>     }
>>>
>>> +    if ( config->flags & XEN_DOMCTL_CDF_vpci )
>>> +    {
>>> +        dprintk(XENLOG_INFO, "VPCI not supported\n");
>>
>> This is a misleading message, at least if for some reason it was to
>> trigger for Dom0. But down the road perhaps also for DomU, as I could
>> imagine vPCI to get enabled alongside passthrough rather than via a
>> separate control.
> 
> Can I silently reject the flag or do you have any suggestion for the commit 
> message.

Whether to issue a message can be judged by all the other code in this
function. Which makes the answer "no, you can't silently reject the
flag". As to the message itself, maybe "vPCI cannot be enabled this way"
or "vPCI cannot be enabled yet"?

>>> --- a/xen/include/asm-arm/domain.h
>>> +++ b/xen/include/asm-arm/domain.h
>>> @@ -262,7 +263,12 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
>>>
>>> #define arch_vm_assist_valid_mask(d) (1UL << 
>>> VMASST_TYPE_runstate_update_flag)
>>>
>>> -#define has_vpci(d)    ({ (void)(d); false; })
>>> +/*
>>> + * For X86 VPCI is enabled and tested for PVH DOM0 only but
>>> + * for ARM we enable support VPCI for guest domain also.
>>> + */
>>> +#define has_vpci(d) ((void)(d), \
>>> +        evaluate_nospec(d->options & XEN_DOMCTL_CDF_vpci))
>>
>> Why the (void)(d)? Instead you want to parenthesize the other use of d.
> 
> I will modify as below:
> #define has_vpci(d) (evaluate_nopsec((d)->options & XEN_DOMCTL_CDF_vpci))

Please also omit the outer pair of parentheses, unless the Arm maintainers
insist on having them despite being redundant.

Jan