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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

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

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

> +        printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret);

Nit: Style again.

> +        goto out;

No other error handling (cleanup)?

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

Jan