Re: [PATCH 4/4] vpci: move lock outside of struct vpci

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.02.2022 17:25, Oleksandr Andrushchenko wrote:
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> 
> This way the lock can be used to check whether vpci is present, and
> removal can be performed while holding the lock, in order to make
> sure there are no accesses to the contents of the vpci struct.
> Previously removal could race with vpci_read for example, since the
> lock was dropped prior to freeing pdev->vpci.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Julien Grall <julien@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> ---
> New in v5 of this series: this is an updated version of the patch published at
> https://lore.kernel.org/xen-devel/20180717094830.54806-2-roger.pau@xxxxxxxxxx/
> 
> Changes since v5:

This is a little odd in a series implicitly tagged as v1.

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -142,12 +142,13 @@ bool vpci_process_pending(struct vcpu *v)
>          if ( rc == -ERESTART )
>              return true;
>  
> -        spin_lock(&v->vpci.pdev->vpci->lock);
> -        /* Disable memory decoding unconditionally on failure. */
> -        modify_decoding(v->vpci.pdev,
> -                        rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
> -                        !rc && v->vpci.rom_only);
> -        spin_unlock(&v->vpci.pdev->vpci->lock);
> +        spin_lock(&v->vpci.pdev->vpci_lock);
> +        if ( v->vpci.pdev->vpci )
> +            /* Disable memory decoding unconditionally on failure. */
> +            modify_decoding(v->vpci.pdev,
> +                            rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : 
> v->vpci.cmd,
> +                            !rc && v->vpci.rom_only);
> +        spin_unlock(&v->vpci.pdev->vpci_lock);

While I certainly see the point, the addition of this if() (and a
few more elsewhere) isn't covered by title or description.

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -35,12 +35,10 @@ extern vpci_register_init_t *const __start_vpci_array[];
>  extern vpci_register_init_t *const __end_vpci_array[];
>  #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>  
> -void vpci_remove_device(struct pci_dev *pdev)
> +static void vpci_remove_device_locked(struct pci_dev *pdev)
>  {
> -    if ( !has_vpci(pdev->domain) )
> -        return;
> +    ASSERT(spin_is_locked(&pdev->vpci_lock));

While, unlike here, ...

> @@ -152,8 +164,6 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
> *read_handler,
>      r->offset = offset;
>      r->private = data;
>  
> -    spin_lock(&vpci->lock);

... you did explain why you don't want to add a similar assertion
here, I think in return the function wants to have a comment added
that it's required to be called with the respective lock held. I
notice you did so for the declaration, but I think such a comment
would better be present at the definition as well. Same for
vpci_remove_register() then, obviously.

> @@ -311,7 +316,7 @@ static uint32_t merge_result(uint32_t data, uint32_t new, 
> unsigned int size,
>  uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
>  {
>      const struct domain *d = current->domain;
> -    const struct pci_dev *pdev;
> +    struct pci_dev *pdev;
>      const struct vpci_register *r;
>      unsigned int data_offset = 0;
>      uint32_t data = ~(uint32_t)0;
> @@ -327,7 +332,12 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size)
>      if ( !pdev )
>          return vpci_read_hw(sbdf, reg, size);
>  
> -    spin_lock(&pdev->vpci->lock);
> +    spin_lock(&pdev->vpci_lock);
> +    if ( !pdev->vpci )
> +    {
> +        spin_unlock(&pdev->vpci_lock);
> +        return vpci_read_hw(sbdf, reg, size);
> +    }

In this case as well as in its write counterpart it becomes even more
important to justify (in the description) the new behavior. It is not
obvious at all that the absence of a struct vpci should be taken as
an indication that the underlying device needs accessing instead.
This also cannot be inferred from the "!pdev" case visible in context.
In that case we have no record of a device at this SBDF, and hence the
fallback pretty clearly is a "just in case" one. Yet if we know of a
device, the absence of a struct vpci may mean various possible things.

Jan