|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 2/2] vpci/msix: fix PBA accesses
I think there is an issue in the spin_lock handling of patch 2 for the
"msix_write" function as it results in the lock being taken a second time while
held (hangs).
The lock taken before checking "VMSIX_ADDR_IN_RANGE" isn't unlocked for the non-
PBA case and a second lock is attempted just before the call to get_entry()
later in the same function. It looks like either the added lock should either
be moved inside the PBA case or the lock before get_entry() should be removed.
On my server, upon loading the ioatdma driver, it now successfully attempts an
PBA write (which now doesn't crash the system), but I'm not sure I have a way to
fully exercise it...
I also see a different (related) issue in which modify_bars is called on a
virtual function seemingly before the BAR addresses are initialized/known and
will start a different thread for that topic.
Regards,
-Alex
On Fri, 2022-02-25 at 16:39 +0100, Roger Pau Monne wrote:
> Map the PBA in order to access it from the MSI-X read and write
> handlers. Note that previously the handlers would pass the physical
> host address into the {read,write}{l,q} handlers, which is wrong as
> those expect a linear address.
>
> Map the PBA using ioremap when the first access is performed. Note
> that 32bit arches might want to abstract the call to ioremap into a
> vPCI arch handler, so they can use a fixmap range to map the PBA.
>
> Reported-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Cc: Alex Olson <this.is.a0lson@xxxxxxxxx>
> ---
> Changes since v1:
> - Also handle writes.
>
> I don't seem to have a box with a driver that will try to access the
> PBA, so I would consider this specific code path only build tested. At
> least it doesn't seem to regress the current state of vPCI.
> ---
> xen/drivers/vpci/msix.c | 55 +++++++++++++++++++++++++++++++++++++----
> xen/drivers/vpci/vpci.c | 2 ++
> xen/include/xen/vpci.h | 2 ++
> 3 files changed, 54 insertions(+), 5 deletions(-)
>
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index a1fa7a5f13..9fbc111ecc 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -198,8 +198,13 @@ static int cf_check msix_read(
> if ( !access_allowed(msix->pdev, addr, len) )
> return X86EMUL_OKAY;
>
> + spin_lock(&msix->pdev->vpci->lock);
> if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
> {
> + struct vpci *vpci = msix->pdev->vpci;
> + paddr_t base = vmsix_table_addr(vpci, VPCI_MSIX_PBA);
> + unsigned int idx = addr - base;
> +
> /*
> * Access to PBA.
> *
> @@ -207,25 +212,43 @@ static int cf_check msix_read(
> * guest address space. If this changes the address will need to be
> * translated.
> */
> +
> + if ( !msix->pba )
> + {
> + msix->pba = ioremap(base, vmsix_table_size(vpci, VPCI_MSIX_PBA));
> + if ( !msix->pba )
> + {
> + /*
> + * If unable to map the PBA return all 1s (all pending): it's
> + * likely better to trigger spurious events than drop them.
> + */
> + spin_unlock(&vpci->lock);
> + gprintk(XENLOG_WARNING,
> + "%pp: unable to map MSI-X PBA, report all pending\n",
> + msix->pdev);
> + return X86EMUL_OKAY;
> + }
> + }
> +
> switch ( len )
> {
> case 4:
> - *data = readl(addr);
> + *data = readl(msix->pba + idx);
> break;
>
> case 8:
> - *data = readq(addr);
> + *data = readq(msix->pba + idx);
> break;
>
> default:
> ASSERT_UNREACHABLE();
> break;
> }
> + spin_unlock(&vpci->lock);
>
> return X86EMUL_OKAY;
> }
>
> - spin_lock(&msix->pdev->vpci->lock);
> entry = get_entry(msix, addr);
> offset = addr & (PCI_MSIX_ENTRY_SIZE - 1);
>
> @@ -273,27 +296,49 @@ static int cf_check msix_write(
> if ( !access_allowed(msix->pdev, addr, len) )
> return X86EMUL_OKAY;
>
> + spin_lock(&msix->pdev->vpci->lock);
> if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
> {
> + struct vpci *vpci = msix->pdev->vpci;
> + paddr_t base = vmsix_table_addr(vpci, VPCI_MSIX_PBA);
> + unsigned int idx = addr - base;
>
> if ( !is_hardware_domain(d) )
> + {
> /* Ignore writes to PBA for DomUs, it's behavior is undefined. */
> + spin_unlock(&vpci->lock);
> return X86EMUL_OKAY;
> + }
> +
> + if ( !msix->pba )
> + {
> + msix->pba = ioremap(base, vmsix_table_size(vpci, VPCI_MSIX_PBA));
> + if ( !msix->pba )
> + {
> + /* Unable to map the PBA, ignore write. */
> + spin_unlock(&vpci->lock);
> + gprintk(XENLOG_WARNING,
> + "%pp: unable to map MSI-X PBA, write ignored\n",
> + msix->pdev);
> + return X86EMUL_OKAY;
> + }
> + }
>
> switch ( len )
> {
> case 4:
> - writel(data, addr);
> + writel(data, msix->pba + idx);
> break;
>
> case 8:
> - writeq(data, addr);
> + writeq(data, msix->pba + idx);
> break;
>
> default:
> ASSERT_UNREACHABLE();
> break;
> }
> + spin_unlock(&vpci->lock);
>
> return X86EMUL_OKAY;
> }
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index f3b32d66cb..9fb3c05b2b 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -51,6 +51,8 @@ void vpci_remove_device(struct pci_dev *pdev)
> xfree(r);
> }
> spin_unlock(&pdev->vpci->lock);
> + if ( pdev->vpci->msix && pdev->vpci->msix->pba )
> + iounmap(pdev->vpci->msix->pba);
> xfree(pdev->vpci->msix);
> xfree(pdev->vpci->msi);
> xfree(pdev->vpci);
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index bcad1516ae..c399b101ee 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -127,6 +127,8 @@ struct vpci {
> bool enabled : 1;
> /* Masked? */
> bool masked : 1;
> + /* PBA map */
> + void *pba;
> /* Entries. */
> struct vpci_msix_entry {
> uint64_t addr;
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |