Re: [PATCH v3] vpci/msix: fix PBA accesses

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.03.2022 13:53, Roger Pau Monne wrote:
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -182,6 +182,33 @@ static struct vpci_msix_entry *get_entry(struct 
> vpci_msix *msix,
>      return &msix->entries[(addr - start) / PCI_MSIX_ENTRY_SIZE];
>  }
>  
> +static void __iomem *get_pba(struct vpci *vpci)
> +{
> +    struct vpci_msix *msix = vpci->msix;
> +    void __iomem *pba;
> +
> +    /*
> +     * PBA will only be unmapped when the device is deassigned, so access it
> +     * without holding the vpci lock.
> +     */
> +    if ( likely(msix->pba) )
> +        return msix->pba;
> +
> +    pba = ioremap(vmsix_table_addr(vpci, VPCI_MSIX_PBA),
> +                  vmsix_table_size(vpci, VPCI_MSIX_PBA));
> +    if ( !pba )
> +        return msix->pba;

For this particular purpose may want to consider using ACCESS_ONCE() for
all accesses to this field.

> +    spin_lock(&vpci->lock);
> +    if ( !msix->pba )
> +        msix->pba = pba;
> +    else
> +        iounmap(pba);
> +    spin_unlock(&vpci->lock);

Whenever possible I think we should try to do things, in particular ones
involving further locks, with as few locks held as possible. IOW I'd like
to ask that you pull out the iounmap().

> @@ -200,6 +227,10 @@ static int cf_check msix_read(
>  
>      if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
>      {
> +        struct vpci *vpci = msix->pdev->vpci;
> +        unsigned int idx = addr - vmsix_table_addr(vpci, VPCI_MSIX_PBA);
> +        void __iomem *pba = get_pba(vpci);

const?

Jan