[PATCH] vpci/msix: restore PBA access length and alignment restrictions

[PATCH] vpci/msix: restore PBA access length and alignment restrictions

[Roger Pau Monne]

Accesses to the PBA array have the same length and alignment
limitations as accesses to the MSI-X table:

"For all accesses to MSI-X Table and MSI-X PBA fields, software must
use aligned full DWORD or aligned full QWORD transactions; otherwise,
the result is undefined."

Introduce such length and alignment checks into the handling of PBA
accesses for vPCI.  This was a mistake of mine for not reading the
specification correctly.

Note that accesses must now be aligned, and hence there's no longer a
need to check that the end of the access falls into the PBA region as
both the access and the region addresses must be aligned.

Fixes: b177892d2d ('vpci/msix: handle accesses adjacent to the MSI-X table')
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/vpci/msix.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 99dd249c15..25bde77586 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -278,6 +278,11 @@ static int adjacent_read(const struct domain *d, const struct vpci_msix *msix,
     if ( !adjacent_handle(msix, addr + len - 1) )
         return X86EMUL_OKAY;
 
+    if ( VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) &&
+         !access_allowed(msix->pdev, addr, len) )
+        /* PBA accesses must be aligned and 4 or 8 bytes in size. */
+        return X86EMUL_OKAY;
+
     slot = get_slot(vpci, addr);
     if ( slot >= ARRAY_SIZE(msix->table) )
         return X86EMUL_OKAY;
@@ -419,9 +424,8 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix,
      * assumed to be equal or bigger (8 bytes) than the length of any access
      * handled here.
      */
-    if ( (VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) ||
-          VMSIX_ADDR_IN_RANGE(addr + len - 1, vpci, VPCI_MSIX_PBA)) &&
-         !is_hardware_domain(d) )
+    if ( VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) &&
+         (!access_allowed(msix->pdev, addr, len) || !is_hardware_domain(d)) )
         /* Ignore writes to PBA for DomUs, it's undefined behavior. */
         return X86EMUL_OKAY;
 
-- 
2.40.0

Re: [PATCH] vpci/msix: restore PBA access length and alignment restrictions

[Jan Beulich]

On 29.03.2023 12:18, Roger Pau Monne wrote:
> Accesses to the PBA array have the same length and alignment
> limitations as accesses to the MSI-X table:
> 
> "For all accesses to MSI-X Table and MSI-X PBA fields, software must
> use aligned full DWORD or aligned full QWORD transactions; otherwise,
> the result is undefined."
> 
> Introduce such length and alignment checks into the handling of PBA
> accesses for vPCI.  This was a mistake of mine for not reading the
> specification correctly.
> 
> Note that accesses must now be aligned, and hence there's no longer a
> need to check that the end of the access falls into the PBA region as
> both the access and the region addresses must be aligned.
> 
> Fixes: b177892d2d ('vpci/msix: handle accesses adjacent to the MSI-X table')
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

And thanks for taking care of this so quickly.

Jan

Re: [PATCH] vpci/msix: restore PBA access length and alignment restrictions

[Jan Beulich]

On 29.03.2023 12:18, Roger Pau Monne wrote:
> @@ -419,9 +424,8 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix,
>       * assumed to be equal or bigger (8 bytes) than the length of any access
>       * handled here.
>       */
> -    if ( (VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) ||
> -          VMSIX_ADDR_IN_RANGE(addr + len - 1, vpci, VPCI_MSIX_PBA)) &&
> -         !is_hardware_domain(d) )
> +    if ( VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) &&
> +         (!access_allowed(msix->pdev, addr, len) || !is_hardware_domain(d)) )
>          /* Ignore writes to PBA for DomUs, it's undefined behavior. */
>          return X86EMUL_OKAY;

While preparing the backport, where I'm folding this into the earlier
patch, I've noticed that this change has now left the comment stale
(the problematic part if just out of context). Not sure though whether
that's worth yet another fixup patch.

Jan

Re: [PATCH] vpci/msix: restore PBA access length and alignment restrictions

[Roger Pau Monné]

On Wed, Mar 29, 2023 at 03:22:34PM +0200, Jan Beulich wrote:
> On 29.03.2023 12:18, Roger Pau Monne wrote:
> > @@ -419,9 +424,8 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix,
> >       * assumed to be equal or bigger (8 bytes) than the length of any access
> >       * handled here.
> >       */
> > -    if ( (VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) ||
> > -          VMSIX_ADDR_IN_RANGE(addr + len - 1, vpci, VPCI_MSIX_PBA)) &&
> > -         !is_hardware_domain(d) )
> > +    if ( VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) &&
> > +         (!access_allowed(msix->pdev, addr, len) || !is_hardware_domain(d)) )
> >          /* Ignore writes to PBA for DomUs, it's undefined behavior. */
> >          return X86EMUL_OKAY;
> 
> While preparing the backport, where I'm folding this into the earlier
> patch, I've noticed that this change has now left the comment stale
> (the problematic part if just out of context). Not sure though whether
> that's worth yet another fixup patch.

I see, thanks for noticing.  I think I can likely adjust in some further change,
or even just drop it, not sure the comment is that relevant anymore if both the
PBA and the access must be aligned now.

Roger.

Re: [PATCH] vpci/msix: restore PBA access length and alignment restrictions

[Jan Beulich]

On 29.03.2023 16:20, Roger Pau Monné wrote:
> On Wed, Mar 29, 2023 at 03:22:34PM +0200, Jan Beulich wrote:
>> On 29.03.2023 12:18, Roger Pau Monne wrote:
>>> @@ -419,9 +424,8 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix,
>>>       * assumed to be equal or bigger (8 bytes) than the length of any access
>>>       * handled here.
>>>       */
>>> -    if ( (VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) ||
>>> -          VMSIX_ADDR_IN_RANGE(addr + len - 1, vpci, VPCI_MSIX_PBA)) &&
>>> -         !is_hardware_domain(d) )
>>> +    if ( VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) &&
>>> +         (!access_allowed(msix->pdev, addr, len) || !is_hardware_domain(d)) )
>>>          /* Ignore writes to PBA for DomUs, it's undefined behavior. */
>>>          return X86EMUL_OKAY;
>>
>> While preparing the backport, where I'm folding this into the earlier
>> patch, I've noticed that this change has now left the comment stale
>> (the problematic part if just out of context). Not sure though whether
>> that's worth yet another fixup patch.
> 
> I see, thanks for noticing.  I think I can likely adjust in some further change,
> or even just drop it, not sure the comment is that relevant anymore if both the
> PBA and the access must be aligned now.

Yeah, dropping would be fine with me. Plus you're the maintainer anyway ...

Jan