Re: [PATCH v2 3/4] vpci: use pcidevs locking to protect MMIO handlers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2022 23:15, Volodymyr Babchuk wrote:
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -891,10 +891,16 @@ void vpci_msix_arch_init_entry(struct vpci_msix_entry 
> *entry)
>      entry->arch.pirq = INVALID_PIRQ;
>  }
>  
> -int vpci_msix_arch_print(const struct vpci_msix *msix)
> +int vpci_msix_arch_print(const struct domain *d, const struct vpci_msix 
> *msix)

I don't think the extra parameter is needed:

> @@ -911,11 +917,23 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>          if ( i && !(i % 64) )
>          {
>              struct pci_dev *pdev = msix->pdev;

You get hold of pdev here, and hence you can take the domain from pdev.

> +            pci_sbdf_t sbdf = pdev->sbdf;
>  
>              spin_unlock(&msix->pdev->vpci->lock);
> +            pcidevs_read_unlock();
> +
> +            /* NB: we still hold rcu_read_lock(&domlist_read_lock); here. */
>              process_pending_softirqs();
> -            /* NB: we assume that pdev cannot go away for an alive domain. */

I think this comment wants retaining, as the new one you add is about
a different aspect.

> -            if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
> +
> +            if ( !pcidevs_read_trylock() )
> +                return -EBUSY;
> +            pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
> +            /*
> +             * FIXME: we may find a re-allocated pdev's copy here.
> +             * Even occupying the same address as before. Do our best.
> +             */
> +            if ( !pdev || (pdev != msix->pdev) || !pdev->vpci ||

Despite the comment: What guarantees that msix isn't a dangling pointer
at this point? At the very least I think you need to check !pdev->vpci
first. And I'm afraid I don't view "do our best" as good enough here
(considering the patch doesn't carry an RFC tag). And no, I don't have
any good suggestion other than "our PCI device locking needs a complete
overhaul". Quite likely what we need is a refcounter per device, which
- as long as non-zero - prevents removal.

> +                 !spin_trylock(&pdev->vpci->lock) )
>                  return -EBUSY;

Don't you need to drop the pcidevs lock on this error path?

> @@ -450,10 +465,15 @@ static int cf_check init_bars(struct pci_dev *pdev)
>      uint16_t cmd;
>      uint64_t addr, size;
>      unsigned int i, num_bars, rom_reg;
> -    struct vpci_header *header = &pdev->vpci->header;
> -    struct vpci_bar *bars = header->bars;
> +    struct vpci_header *header;
> +    struct vpci_bar *bars;
>      int rc;
>  
> +    ASSERT(pcidevs_write_locked());
> +
> +    header = &pdev->vpci->header;
> +    bars = header->bars;

I'm not convinced the code movement here does us any good. (Same
apparently elsewhere below.)

> @@ -277,6 +282,9 @@ void vpci_dump_msi(void)
>  
>          printk("vPCI MSI/MSI-X d%d\n", d->domain_id);
>  
> +        if ( !pcidevs_read_trylock() )
> +            continue;

Note how this lives ahead of ...

>          for_each_pdev ( d, pdev )
>          {

... the loop, while ...

> @@ -310,7 +318,7 @@ void vpci_dump_msi(void)
>                  printk("  entries: %u maskall: %d enabled: %d\n",
>                         msix->max_entries, msix->masked, msix->enabled);
>  
> -                rc = vpci_msix_arch_print(msix);
> +                rc = vpci_msix_arch_print(d, msix);
>                  if ( rc )
>                  {
>                      /*
> @@ -318,12 +326,13 @@ void vpci_dump_msi(void)
>                       * holding the lock.
>                       */
>                      printk("unable to print all MSI-X entries: %d\n", rc);
> -                    process_pending_softirqs();
> -                    continue;
> +                    goto pdev_done;
>                  }
>              }
>  
>              spin_unlock(&pdev->vpci->lock);
> + pdev_done:
> +            pcidevs_read_unlock();

... this is still inside the loop body.

> @@ -332,10 +334,14 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size)
>          return data;
>      }
>  
> +    pcidevs_read_lock();
>      /* Find the PCI dev matching the address. */
>      pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
> -    if ( !pdev )
> +    if ( !pdev || (pdev && !pdev->vpci) )

Simpler

    if ( !pdev || !pdev->vpci )

?

> @@ -381,6 +387,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size)
>          ASSERT(data_offset < size);
>      }
>      spin_unlock(&pdev->vpci->lock);
> +    pcidevs_read_unlock();

I guess this is too early and wants to come after ...

>      if ( data_offset < size )
>      {

... this if, which - even if it doesn't use pdev - still accesses the
device.

Both comments equally apply to vpci_write().

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -161,6 +161,7 @@ void pcidevs_unlock(void);
>  bool __must_check pcidevs_locked(void);
>  
>  void pcidevs_read_lock(void);
> +int pcidevs_read_trylock(void);

This declaration wants adding alongside the introduction of the
function or, if the series was structured that way, at the time of the
dropping of "static" from the function (which from a Misra perspective
would likely be better).

Jan