Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2021 18:51, Bertrand Marquis wrote:
> --- /dev/null
> +++ b/xen/arch/arm/vpci.c
> @@ -0,0 +1,77 @@
> +/*
> + * xen/arch/arm/vpci.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <xen/sched.h>
> +#include <xen/vpci.h>
> +
> +#include <asm/mmio.h>
> +
> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> +                          register_t *r, void *p)
> +{
> +    pci_sbdf_t sbdf;
> +    /* data is needed to prevent a pointer cast on 32bit */
> +    unsigned long data;
> +
> +    /* We ignore segment part and always handle segment 0 */
> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
> +
> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
> +                        1U << info->dabt.size, &data) )
> +    {

Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
the virtual one. The function then calls vpci_read(), which in turn
will call vpci_read_hw() in a number of situations (first and foremost
whenever pci_get_pdev_by_domain() returns NULL). That function as well
as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
physical one; I'm unable to spot any translation. Yet I do recall
seeing assignment of a virtual device and function number somewhere
(perhaps another of the related series), so the model also doesn't
look to assume 1:1 mapping of SBDF.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      if ( !pdev->domain )
>      {
>          pdev->domain = hardware_domain;
> +#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 )
> +        {
> +            printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
> +            pdev->domain = NULL;
> +            goto out;
> +        }
> +#endif
>          ret = iommu_add_device(pdev);
>          if ( ret )
>          {

Upon failure, vpci_add_handlers() will itself call vpci_remove_handlers().
What about iommu_add_device() failure? The device will have ->domain
zapped, but all vPCI handlers still in place. This aspect of insufficient
error cleanup was pointed out already in review of v1.

Furthermore already in v1 I questioned why this would be Arm-specific: On
x86 this code path is going to be taken for all devices Xen wasn't able
to discover at boot (anything on segments we wouldn't consider config
space access safe on without reassurance by Dom0 plus SR-IOV VFs, at the
very least). Hence it is my understanding that something along these
lines is actually also needed for PVH Dom0. I've just checked, and
according to my mailbox that comment was actually left unresponded to.

Roger, am I missing anything here as to PVH Dom0 getting handlers put in
place?

Of course as soon as the CONFIG_ARM conditionals were dropped, the
__hwdom_init issue would become an "active" one.

Jan