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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Jan,

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


>> For Dom0less systems scan_pci_devices() would be used to discover the
>> PCI device in XEN and VPCI handler will be added during XEN boots.
> 
> So "would be" here means at some point in the future, rather than
> before or in this patch? This could do with making unambiguous.

Yes we are currently working on to implement support for Dom0less. I thought it 
is good 
to mention here how we going to add the vpci_handler(..) for Dom0less system.  
> 
>> --- 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.

>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -767,6 +767,18 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>     else
>>         iommu_enable_device(pdev);
>> 
>> +#ifdef CONFIG_ARM
>> +    /*
>> +     * On ARM PCI devices discovery will be done by Dom0. Add vpci handler 
>> when
>> +     * Dom0 inform XEN to add the PCI devices in XEN.
>> +     */
>> +    ret = vpci_add_handlers(pdev);
>> +    if ( ret ) {
> 
> Nit: Style.
Ack.
> 
>> +        printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret);
> 
> Nit: Style again.

Ack.
> 
>> +        goto out;
> 
> No other error handling (cleanup)?
> 

I will add the below error handling next version :

#define CONFIG_ARM
ret = vpci_add_handlers(pdev);
if ( ret ) 
{
        printk(XENLOG_ERR "setup of vPCI for failed: %d\n”, ret);
        iommu_remove_device(pdev);
        if ( pdev->domain )
           list_del(&pdev->domain_list);
        free_pdev(pseg, pdev);
        goto out;
}
#endif

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


> Jan
> 
 
Regards,
Rahul