* [PATCH v4] vpci/msix: fix PBA accesses
@ 2022-03-07 16:37 Roger Pau Monne
2022-03-08 8:31 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Roger Pau Monne @ 2022-03-07 16:37 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Alex Olson
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@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Alex Olson <this.is.a0lson@gmail.com>
---
Changes since v3:
- Use {read,write}_atomic for accessing msix pba field.
- Shrink locked section.
- Constify pba.
Changes since v2:
- Use helper function to map PBA.
- Mark memory as iomem.
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 | 64 ++++++++++++++++++++++++++++++++++++++---
xen/drivers/vpci/vpci.c | 2 ++
xen/include/xen/vpci.h | 2 ++
3 files changed, 64 insertions(+), 4 deletions(-)
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index a1fa7a5f13..63f162cf5a 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -182,6 +182,38 @@ 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;
+ /*
+ * PBA will only be unmapped when the device is deassigned, so access it
+ * without holding the vpci lock.
+ */
+ void __iomem *pba = read_atomic(&msix->pba);
+
+ if ( likely(pba) )
+ return pba;
+
+ pba = ioremap(vmsix_table_addr(vpci, VPCI_MSIX_PBA),
+ vmsix_table_size(vpci, VPCI_MSIX_PBA));
+ if ( !pba )
+ return read_atomic(&msix->pba);
+
+ spin_lock(&vpci->lock);
+ if ( !msix->pba )
+ {
+ write_atomic(&msix->pba, pba);
+ spin_unlock(&vpci->lock);
+ }
+ else
+ {
+ spin_unlock(&vpci->lock);
+ iounmap(pba);
+ }
+
+ return read_atomic(&msix->pba);
+}
+
static int cf_check msix_read(
struct vcpu *v, unsigned long addr, unsigned int len, unsigned long *data)
{
@@ -200,6 +232,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);
+ const void __iomem *pba = get_pba(vpci);
+
/*
* Access to PBA.
*
@@ -207,14 +243,22 @@ static int cf_check msix_read(
* guest address space. If this changes the address will need to be
* translated.
*/
+ if ( !pba )
+ {
+ 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(pba + idx);
break;
case 8:
- *data = readq(addr);
+ *data = readq(pba + idx);
break;
default:
@@ -275,19 +319,31 @@ static int cf_check msix_write(
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);
+ const void __iomem *pba = get_pba(vpci);
if ( !is_hardware_domain(d) )
/* Ignore writes to PBA for DomUs, it's behavior is undefined. */
return X86EMUL_OKAY;
+ if ( !pba )
+ {
+ /* Unable to map the PBA, ignore write. */
+ 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, pba + idx);
break;
case 8:
- writeq(data, addr);
+ writeq(data, pba + idx);
break;
default:
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..67c9a0c631 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 __iomem *pba;
/* Entries. */
struct vpci_msix_entry {
uint64_t addr;
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] vpci/msix: fix PBA accesses
2022-03-07 16:37 [PATCH v4] vpci/msix: fix PBA accesses Roger Pau Monne
@ 2022-03-08 8:31 ` Jan Beulich
2022-03-08 9:05 ` Roger Pau Monné
2022-03-08 18:19 ` Alex Olson
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2022-03-08 8:31 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Alex Olson, xen-devel
On 07.03.2022 17:37, 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@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Cc: Alex Olson <this.is.a0lson@gmail.com>
I'll wait a little with committing, in the hope for Alex to re-provide
a Tested-by.
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -182,6 +182,38 @@ 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;
> + /*
> + * PBA will only be unmapped when the device is deassigned, so access it
> + * without holding the vpci lock.
> + */
> + void __iomem *pba = read_atomic(&msix->pba);
> +
> + if ( likely(pba) )
> + return pba;
> +
> + pba = ioremap(vmsix_table_addr(vpci, VPCI_MSIX_PBA),
> + vmsix_table_size(vpci, VPCI_MSIX_PBA));
> + if ( !pba )
> + return read_atomic(&msix->pba);
> +
> + spin_lock(&vpci->lock);
> + if ( !msix->pba )
> + {
> + write_atomic(&msix->pba, pba);
> + spin_unlock(&vpci->lock);
> + }
> + else
> + {
> + spin_unlock(&vpci->lock);
> + iounmap(pba);
> + }
TBH I had been hoping for just a single spin_unlock(), but you're
the maintainer of this code ...
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] vpci/msix: fix PBA accesses
2022-03-08 8:31 ` Jan Beulich
@ 2022-03-08 9:05 ` Roger Pau Monné
2022-03-08 10:46 ` Jan Beulich
2022-03-08 18:19 ` Alex Olson
1 sibling, 1 reply; 6+ messages in thread
From: Roger Pau Monné @ 2022-03-08 9:05 UTC (permalink / raw)
To: Jan Beulich; +Cc: Alex Olson, xen-devel
On Tue, Mar 08, 2022 at 09:31:34AM +0100, Jan Beulich wrote:
> On 07.03.2022 17:37, 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@suse.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> > Cc: Alex Olson <this.is.a0lson@gmail.com>
>
> I'll wait a little with committing, in the hope for Alex to re-provide
> a Tested-by.
>
> > --- a/xen/drivers/vpci/msix.c
> > +++ b/xen/drivers/vpci/msix.c
> > @@ -182,6 +182,38 @@ 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;
> > + /*
> > + * PBA will only be unmapped when the device is deassigned, so access it
> > + * without holding the vpci lock.
> > + */
> > + void __iomem *pba = read_atomic(&msix->pba);
> > +
> > + if ( likely(pba) )
> > + return pba;
> > +
> > + pba = ioremap(vmsix_table_addr(vpci, VPCI_MSIX_PBA),
> > + vmsix_table_size(vpci, VPCI_MSIX_PBA));
> > + if ( !pba )
> > + return read_atomic(&msix->pba);
> > +
> > + spin_lock(&vpci->lock);
> > + if ( !msix->pba )
> > + {
> > + write_atomic(&msix->pba, pba);
> > + spin_unlock(&vpci->lock);
> > + }
> > + else
> > + {
> > + spin_unlock(&vpci->lock);
> > + iounmap(pba);
> > + }
>
> TBH I had been hoping for just a single spin_unlock(), but you're
> the maintainer of this code ...
Would you prefer something like:
spin_lock(&vpci->lock);
if ( !msix->pba )
write_atomic(&msix->pba, pba);
spin_unlock(&vpci->lock);
if ( read_atomic(&msix->pba) != pba )
iounmap(pba);
?
Thanks, Roger.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] vpci/msix: fix PBA accesses
2022-03-08 9:05 ` Roger Pau Monné
@ 2022-03-08 10:46 ` Jan Beulich
2022-03-08 12:37 ` Roger Pau Monné
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2022-03-08 10:46 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Alex Olson, xen-devel
On 08.03.2022 10:05, Roger Pau Monné wrote:
> On Tue, Mar 08, 2022 at 09:31:34AM +0100, Jan Beulich wrote:
>> On 07.03.2022 17:37, 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@suse.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>>> Cc: Alex Olson <this.is.a0lson@gmail.com>
>>
>> I'll wait a little with committing, in the hope for Alex to re-provide
>> a Tested-by.
>>
>>> --- a/xen/drivers/vpci/msix.c
>>> +++ b/xen/drivers/vpci/msix.c
>>> @@ -182,6 +182,38 @@ 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;
>>> + /*
>>> + * PBA will only be unmapped when the device is deassigned, so access it
>>> + * without holding the vpci lock.
>>> + */
>>> + void __iomem *pba = read_atomic(&msix->pba);
>>> +
>>> + if ( likely(pba) )
>>> + return pba;
>>> +
>>> + pba = ioremap(vmsix_table_addr(vpci, VPCI_MSIX_PBA),
>>> + vmsix_table_size(vpci, VPCI_MSIX_PBA));
>>> + if ( !pba )
>>> + return read_atomic(&msix->pba);
>>> +
>>> + spin_lock(&vpci->lock);
>>> + if ( !msix->pba )
>>> + {
>>> + write_atomic(&msix->pba, pba);
>>> + spin_unlock(&vpci->lock);
>>> + }
>>> + else
>>> + {
>>> + spin_unlock(&vpci->lock);
>>> + iounmap(pba);
>>> + }
>>
>> TBH I had been hoping for just a single spin_unlock(), but you're
>> the maintainer of this code ...
>
> Would you prefer something like:
>
> spin_lock(&vpci->lock);
> if ( !msix->pba )
> write_atomic(&msix->pba, pba);
> spin_unlock(&vpci->lock);
>
> if ( read_atomic(&msix->pba) != pba )
> iounmap(pba);
This or (to avoid the re-read)
spin_lock(&vpci->lock);
if ( !msix->pba )
{
write_atomic(&msix->pba, pba);
pba = NULL;
}
spin_unlock(&vpci->lock);
if ( pba )
iounmap(pba);
Iirc we have similar constructs elsewhere in a few places.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] vpci/msix: fix PBA accesses
2022-03-08 10:46 ` Jan Beulich
@ 2022-03-08 12:37 ` Roger Pau Monné
0 siblings, 0 replies; 6+ messages in thread
From: Roger Pau Monné @ 2022-03-08 12:37 UTC (permalink / raw)
To: Jan Beulich; +Cc: Alex Olson, xen-devel
On Tue, Mar 08, 2022 at 11:46:20AM +0100, Jan Beulich wrote:
> On 08.03.2022 10:05, Roger Pau Monné wrote:
> > On Tue, Mar 08, 2022 at 09:31:34AM +0100, Jan Beulich wrote:
> >> On 07.03.2022 17:37, 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@suse.com>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>
> >>> Cc: Alex Olson <this.is.a0lson@gmail.com>
> >>
> >> I'll wait a little with committing, in the hope for Alex to re-provide
> >> a Tested-by.
> >>
> >>> --- a/xen/drivers/vpci/msix.c
> >>> +++ b/xen/drivers/vpci/msix.c
> >>> @@ -182,6 +182,38 @@ 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;
> >>> + /*
> >>> + * PBA will only be unmapped when the device is deassigned, so access it
> >>> + * without holding the vpci lock.
> >>> + */
> >>> + void __iomem *pba = read_atomic(&msix->pba);
> >>> +
> >>> + if ( likely(pba) )
> >>> + return pba;
> >>> +
> >>> + pba = ioremap(vmsix_table_addr(vpci, VPCI_MSIX_PBA),
> >>> + vmsix_table_size(vpci, VPCI_MSIX_PBA));
> >>> + if ( !pba )
> >>> + return read_atomic(&msix->pba);
> >>> +
> >>> + spin_lock(&vpci->lock);
> >>> + if ( !msix->pba )
> >>> + {
> >>> + write_atomic(&msix->pba, pba);
> >>> + spin_unlock(&vpci->lock);
> >>> + }
> >>> + else
> >>> + {
> >>> + spin_unlock(&vpci->lock);
> >>> + iounmap(pba);
> >>> + }
> >>
> >> TBH I had been hoping for just a single spin_unlock(), but you're
> >> the maintainer of this code ...
> >
> > Would you prefer something like:
> >
> > spin_lock(&vpci->lock);
> > if ( !msix->pba )
> > write_atomic(&msix->pba, pba);
> > spin_unlock(&vpci->lock);
> >
> > if ( read_atomic(&msix->pba) != pba )
> > iounmap(pba);
>
> This or (to avoid the re-read)
>
> spin_lock(&vpci->lock);
> if ( !msix->pba )
> {
> write_atomic(&msix->pba, pba);
> pba = NULL;
> }
> spin_unlock(&vpci->lock);
>
> if ( pba )
> iounmap(pba);
>
> Iirc we have similar constructs elsewhere in a few places.
I don't really have a strong opinion. I agree the duplicated
spin_unlock() call is not nice, but I think the logic is easier to
follow by using a single if ... else ... section.
Feel free to adjust at commit, or else I can resend if you prefer.
Thanks, Roger.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] vpci/msix: fix PBA accesses
2022-03-08 8:31 ` Jan Beulich
2022-03-08 9:05 ` Roger Pau Monné
@ 2022-03-08 18:19 ` Alex Olson
1 sibling, 0 replies; 6+ messages in thread
From: Alex Olson @ 2022-03-08 18:19 UTC (permalink / raw)
To: Jan Beulich, Roger Pau Monne; +Cc: xen-devel
On Tue, 2022-03-08 at 09:31 +0100, Jan Beulich wrote:
> On 07.03.2022 17:37, 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@suse.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> > Cc: Alex Olson <this.is.a0lson@gmail.com>
>
> I'll wait a little with committing, in the hope for Alex to re-provide
> a Tested-by.
It works fine for me, you can add "Tested-by: Alex.Olson@starlab.io" to the
commit.
>
> > --- a/xen/drivers/vpci/msix.c
> > +++ b/xen/drivers/vpci/msix.c
> > @@ -182,6 +182,38 @@ 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;
> > + /*
> > + * PBA will only be unmapped when the device is deassigned, so access
> > it
> > + * without holding the vpci lock.
> > + */
> > + void __iomem *pba = read_atomic(&msix->pba);
> > +
> > + if ( likely(pba) )
> > + return pba;
> > +
> > + pba = ioremap(vmsix_table_addr(vpci, VPCI_MSIX_PBA),
> > + vmsix_table_size(vpci, VPCI_MSIX_PBA));
> > + if ( !pba )
> > + return read_atomic(&msix->pba);
> > +
> > + spin_lock(&vpci->lock);
> > + if ( !msix->pba )
> > + {
> > + write_atomic(&msix->pba, pba);
> > + spin_unlock(&vpci->lock);
> > + }
> > + else
> > + {
> > + spin_unlock(&vpci->lock);
> > + iounmap(pba);
> > + }
>
> TBH I had been hoping for just a single spin_unlock(), but you're
> the maintainer of this code ...
>
> Jan
>
Thanks
-Alex
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-08 18:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-07 16:37 [PATCH v4] vpci/msix: fix PBA accesses Roger Pau Monne
2022-03-08 8:31 ` Jan Beulich
2022-03-08 9:05 ` Roger Pau Monné
2022-03-08 10:46 ` Jan Beulich
2022-03-08 12:37 ` Roger Pau Monné
2022-03-08 18:19 ` Alex Olson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.