Re: [PATCH v4 10/11] vpci: add initial support for virtual PCI bus topology

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
> Since v3:
>  - make use of VPCI_INIT
>  - moved all new code to vpci.c which belongs to it
>  - changed open-coded 31 to PCI_SLOT(~0)
>  - revisited locking: add dedicated vdev list's lock

What is this about? I can't spot any locking in the patch. In particular ...

> @@ -125,6 +128,54 @@ int vpci_add_handlers(struct pci_dev *pdev)
>  }
>  
>  #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +int vpci_add_virtual_device(struct pci_dev *pdev)
> +{
> +    struct domain *d = pdev->domain;
> +    pci_sbdf_t sbdf;
> +    unsigned long new_dev_number;
> +
> +    /*
> +     * Each PCI bus supports 32 devices/slots at max or up to 256 when
> +     * there are multi-function ones which are not yet supported.
> +     */
> +    if ( pdev->info.is_extfn )
> +    {
> +        gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n",
> +                 &pdev->sbdf);
> +        return -EOPNOTSUPP;
> +    }
> +
> +    new_dev_number = find_first_zero_bit(&d->vpci_dev_assigned_map,
> +                                         PCI_SLOT(~0) + 1);
> +    if ( new_dev_number > PCI_SLOT(~0) )
> +        return -ENOSPC;
> +
> +    set_bit(new_dev_number, &d->vpci_dev_assigned_map);

... I wonder whether this isn't racy without any locking around it,
and without looping over test_and_set_bit(). Whereas with locking I
think you could just use __set_bit().

> +    /*
> +     * Both segment and bus number are 0:
> +     *  - we emulate a single host bridge for the guest, e.g. segment 0
> +     *  - with bus 0 the virtual devices are seen as embedded
> +     *    endpoints behind the root complex
> +     *
> +     * TODO: add support for multi-function devices.
> +     */
> +    sbdf.sbdf = 0;

I think this would be better expressed as an initializer, making it
clear to the reader that the whole object gets initialized with out
them needing to go check the type (and find that .sbdf covers the
entire object).

> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -145,6 +145,10 @@ struct vpci {
>              struct vpci_arch_msix_entry arch;
>          } entries[];
>      } *msix;
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +    /* Virtual SBDF of the device. */
> +    pci_sbdf_t guest_sbdf;

Would vsbdf perhaps be better in line with things like vpci or vcpu
(as well as with the comment here)?

Jan