[PATCH] xen/vpci: allow unaligned accesses by the hardware domain

[PATCH] xen/vpci: allow unaligned accesses by the hardware domain

[Roger Pau Monne]

It's possible for domains to generate unaligned PCI config space accesses
when using ECAM, and hence vPCI should support those at least for the
hardware domain.  Such unaligned accesses to the PCI config space have been
reported to come from ACPI logic.

Relax the checking in vpci_access_allowed() to allow such accesses for the
hardware domain, and fix the handling in pci_conf_{read,write}{16,32}() to
fulfill them using MMCFG.

MMCFG regions are identity exposed to the hardware domain, and hence such
unaligned accesses can only come as a result of the host having MMCFG in the
first place, as otherwise MMCFG won't be exposed to the hardware domain
either.

Reported-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Roger Pau Monné <roger@xenproject.org>
---
 tools/include/xen-tools/common-macros.h | 2 ++
 xen/arch/x86/x86_64/pci.c               | 8 ++++----
 xen/drivers/vpci/vpci.c                 | 4 +++-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
index 88b4a0e5a693..1f9146b23b0e 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -68,6 +68,8 @@
     })
 #endif
 
+#define IS_ALIGNED(val, align) (!((val) & ((align) - 1)))
+
 #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
 #define ROUNDDOWN(x, a) ((x) & ~((a) - 1))
 
diff --git a/xen/arch/x86/x86_64/pci.c b/xen/arch/x86/x86_64/pci.c
index 8d33429103b9..6298141c3ca7 100644
--- a/xen/arch/x86/x86_64/pci.c
+++ b/xen/arch/x86/x86_64/pci.c
@@ -26,7 +26,7 @@ uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg)
 
 uint16_t pci_conf_read16(pci_sbdf_t sbdf, unsigned int reg)
 {
-    if ( sbdf.seg || reg > 255 )
+    if ( sbdf.seg || reg > 255 || !IS_ALIGNED(reg, 2) )
     {
         uint32_t value;
 
@@ -39,7 +39,7 @@ uint16_t pci_conf_read16(pci_sbdf_t sbdf, unsigned int reg)
 
 uint32_t pci_conf_read32(pci_sbdf_t sbdf, unsigned int reg)
 {
-    if ( sbdf.seg || reg > 255 )
+    if ( sbdf.seg || reg > 255 || !IS_ALIGNED(reg, 4) )
     {
         uint32_t value;
 
@@ -60,7 +60,7 @@ void pci_conf_write8(pci_sbdf_t sbdf, unsigned int reg, uint8_t data)
 
 void pci_conf_write16(pci_sbdf_t sbdf, unsigned int reg, uint16_t data)
 {
-    if ( sbdf.seg || reg > 255 )
+    if ( sbdf.seg || reg > 255 || !IS_ALIGNED(reg, 2) )
         pci_mmcfg_write(sbdf.seg, sbdf.bus, sbdf.devfn, reg, 2, data);
     else
         pci_conf_write(PCI_CONF_ADDRESS(sbdf, reg), reg & 2, 2, data);
@@ -68,7 +68,7 @@ void pci_conf_write16(pci_sbdf_t sbdf, unsigned int reg, uint16_t data)
 
 void pci_conf_write32(pci_sbdf_t sbdf, unsigned int reg, uint32_t data)
 {
-    if ( sbdf.seg || reg > 255 )
+    if ( sbdf.seg || reg > 255 || !IS_ALIGNED(reg, 4) )
         pci_mmcfg_write(sbdf.seg, sbdf.bus, sbdf.devfn, reg, 4, data);
     else
         pci_conf_write(PCI_CONF_ADDRESS(sbdf, reg), 0, 4, data);
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 0ac9ec8b0475..b4e053bb4946 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -685,6 +685,8 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
 /* Helper function to check an access size and alignment on vpci space. */
 bool vpci_access_allowed(unsigned int reg, unsigned int len)
 {
+    const struct domain *currd = current->domain;
+
     /* Check access size. */
     if ( len != 1 && len != 2 && len != 4 && len != 8 )
         return false;
@@ -696,7 +698,7 @@ bool vpci_access_allowed(unsigned int reg, unsigned int len)
 #endif
 
     /* Check that access is size aligned. */
-    if ( (reg & (len - 1)) )
+    if ( !is_hardware_domain(currd) && !IS_ALIGNED(reg, len) )
         return false;
 
     return true;
-- 
2.53.0

Re: [PATCH] xen/vpci: allow unaligned accesses by the hardware domain

[Andrew Cooper]

On 06/08/2026 12:04 pm, Roger Pau Monne wrote:
> It's possible for domains to generate unaligned PCI config space accesses
> when using ECAM, and hence vPCI should support those at least for the
> hardware domain.  Such unaligned accesses to the PCI config space have been
> reported to come from ACPI logic.

By this, I presume you mean AML from the DSDT/SSDT ?

Misaligned ECAM accesses have undefined behaviour.  On a particular
platform this probably means implementation defined, and for an access
wholly within a single devices CFG window it might even work.  But,
misaligning allows you to have one access hitting two devices, and this
really can't be a good thing.

I presume "fix your BIOS" isn't on the cards, even if it would be for
the greater good?

>
> Relax the checking in vpci_access_allowed() to allow such accesses for the
> hardware domain, and fix the handling in pci_conf_{read,write}{16,32}() to
> fulfill them using MMCFG.
>
> MMCFG regions are identity exposed to the hardware domain, and hence such
> unaligned accesses can only come as a result of the host having MMCFG in the
> first place, as otherwise MMCFG won't be exposed to the hardware domain
> either.
>
> Reported-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> ---
>  tools/include/xen-tools/common-macros.h | 2 ++
>  xen/arch/x86/x86_64/pci.c               | 8 ++++----
>  xen/drivers/vpci/vpci.c                 | 4 +++-
>  3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/x86_64/pci.c b/xen/arch/x86/x86_64/pci.c
> index 8d33429103b9..6298141c3ca7 100644
> --- a/xen/arch/x86/x86_64/pci.c
> +++ b/xen/arch/x86/x86_64/pci.c
> @@ -26,7 +26,7 @@ uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg)
>  
>  uint16_t pci_conf_read16(pci_sbdf_t sbdf, unsigned int reg)
>  {
> -    if ( sbdf.seg || reg > 255 )
> +    if ( sbdf.seg || reg > 255 || !IS_ALIGNED(reg, 2) )
>      {
>          uint32_t value;
>  

Personally, I think this is making a bad situation worse.

Baring quirks (i.e. K10 era), there is never a case where we want to use
the IO Ports when we've got ECAM.  Furthermore, on AMD systems when
we're lacking ECAM we can still access Extended Config Space; something
which Xen currently gets wrong in several ways.

The IO ports require a global spinlock in Xen, and then a global
resource in hardware just to be able to translate the IO access back
into the ECAM access we passed on originally.  i.e. from a safety
non-interference point of view, you want to veto any use of the IO ports
by Xen.

Xen needs to use ECAM, and only fall back to IO Ports if we think there
isn't ECAM covering the target sbdf.  This will cause (mis)alignment to
get fixed automatically.  It will also be a substantial perf boost in
the general case; all the MSI/MSI-X editing we do (far too frequently)
is in Legacy Config Space just uses IO Ports.

~Andrew

Re: [PATCH] xen/vpci: allow unaligned accesses by the hardware domain

[Jan Beulich]

On 06.08.2026 13:48, Andrew Cooper wrote:
> On 06/08/2026 12:04 pm, Roger Pau Monne wrote:
>> --- a/xen/arch/x86/x86_64/pci.c
>> +++ b/xen/arch/x86/x86_64/pci.c
>> @@ -26,7 +26,7 @@ uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg)
>>  
>>  uint16_t pci_conf_read16(pci_sbdf_t sbdf, unsigned int reg)
>>  {
>> -    if ( sbdf.seg || reg > 255 )
>> +    if ( sbdf.seg || reg > 255 || !IS_ALIGNED(reg, 2) )
>>      {
>>          uint32_t value;
>>  
> 
> Personally, I think this is making a bad situation worse.
> 
> Baring quirks (i.e. K10 era), there is never a case where we want to use
> the IO Ports when we've got ECAM.  Furthermore, on AMD systems when
> we're lacking ECAM we can still access Extended Config Space; something
> which Xen currently gets wrong in several ways.
> 
> The IO ports require a global spinlock in Xen, and then a global
> resource in hardware just to be able to translate the IO access back
> into the ECAM access we passed on originally.  i.e. from a safety
> non-interference point of view, you want to veto any use of the IO ports
> by Xen.
> 
> Xen needs to use ECAM, and only fall back to IO Ports if we think there
> isn't ECAM covering the target sbdf.  This will cause (mis)alignment to
> get fixed automatically.  It will also be a substantial perf boost in
> the general case; all the MSI/MSI-X editing we do (far too frequently)
> is in Legacy Config Space just uses IO Ports.

While I agree, that's a bigger change which likely is going to be unsuitable
for (immediate) backporting. Nevertheless the cross-device access that the
changes as presented could cause needs preventing, by altering the checks at
the start of pci_mmcfg_{read,write}().

Jan

Re: [PATCH] xen/vpci: allow unaligned accesses by the hardware domain

[Roger Pau Monné]

On Thu, Aug 06, 2026 at 12:48:12PM +0100, Andrew Cooper wrote:
> On 06/08/2026 12:04 pm, Roger Pau Monne wrote:
> > It's possible for domains to generate unaligned PCI config space accesses
> > when using ECAM, and hence vPCI should support those at least for the
> > hardware domain.  Such unaligned accesses to the PCI config space have been
> > reported to come from ACPI logic.
> 
> By this, I presume you mean AML from the DSDT/SSDT ?

That's my understanding yes, I can't confirm whether it's from the
DSDST/SSDT or maybe one of the OEM tables.

> Misaligned ECAM accesses have undefined behaviour.  On a particular
> platform this probably means implementation defined, and for an access
> wholly within a single devices CFG window it might even work.  But,
> misaligning allows you to have one access hitting two devices, and this
> really can't be a good thing.

Well, for the hardware domain I merely wanted to replay the access,
but it's true that for access control (that we still do in the
hardware domain case) an access targeting two different devices would
require extra checking that we don't do.

I think it's possibly best to not relax the check in
vpci_access_allowed() as much as I did, and refuse cross-device
accesses.

> I presume "fix your BIOS" isn't on the cards, even if it would be for
> the greater good?

I don't know really, maybe it's possible to get this one fixed.  But
it's likely Xen would hit this again, and most people would attribute
the issue to Xen, because it works with plain Linux, but doesn't when
running as a hardware domain (as the unaligned access is ignored or
reads all 1s).

We have always said that for the hardware domain we would attempt to
get as less as possible in the way of it interacting with devices. I
think this is just one more instance of such avoidance of interference
from hardware domain accesses.

> >
> > Relax the checking in vpci_access_allowed() to allow such accesses for the
> > hardware domain, and fix the handling in pci_conf_{read,write}{16,32}() to
> > fulfill them using MMCFG.
> >
> > MMCFG regions are identity exposed to the hardware domain, and hence such
> > unaligned accesses can only come as a result of the host having MMCFG in the
> > first place, as otherwise MMCFG won't be exposed to the hardware domain
> > either.
> >
> > Reported-by: Jason Andryuk <jason.andryuk@amd.com>
> > Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> > ---
> >  tools/include/xen-tools/common-macros.h | 2 ++
> >  xen/arch/x86/x86_64/pci.c               | 8 ++++----
> >  xen/drivers/vpci/vpci.c                 | 4 +++-
> >  3 files changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/x86/x86_64/pci.c b/xen/arch/x86/x86_64/pci.c
> > index 8d33429103b9..6298141c3ca7 100644
> > --- a/xen/arch/x86/x86_64/pci.c
> > +++ b/xen/arch/x86/x86_64/pci.c
> > @@ -26,7 +26,7 @@ uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg)
> >  
> >  uint16_t pci_conf_read16(pci_sbdf_t sbdf, unsigned int reg)
> >  {
> > -    if ( sbdf.seg || reg > 255 )
> > +    if ( sbdf.seg || reg > 255 || !IS_ALIGNED(reg, 2) )
> >      {
> >          uint32_t value;
> >  
> 
> Personally, I think this is making a bad situation worse.
> 
> Baring quirks (i.e. K10 era), there is never a case where we want to use
> the IO Ports when we've got ECAM.  Furthermore, on AMD systems when
> we're lacking ECAM we can still access Extended Config Space; something
> which Xen currently gets wrong in several ways.
> 
> The IO ports require a global spinlock in Xen, and then a global
> resource in hardware just to be able to translate the IO access back
> into the ECAM access we passed on originally.  i.e. from a safety
> non-interference point of view, you want to veto any use of the IO ports
> by Xen.
> 
> Xen needs to use ECAM, and only fall back to IO Ports if we think there
> isn't ECAM covering the target sbdf.  This will cause (mis)alignment to
> get fixed automatically.  It will also be a substantial perf boost in
> the general case; all the MSI/MSI-X editing we do (far too frequently)
> is in Legacy Config Space just uses IO Ports.

I've wondered the same myself for some time, but I was also a bit
puzzled by Linux using the same approach, and preferring the IO ports
for accesses to the non-extended space.

Overall I think we would widen the hardware domain accesses _before_
ahead of switching PCI accesses to prefer ECAM over IO ports.  So that
the former can be backported without depending on the latter.

Thanks, Roger.

Re: [PATCH] xen/vpci: allow unaligned accesses by the hardware domain

[Roger Pau Monné]

On Thu, Aug 06, 2026 at 02:15:02PM +0200, Jan Beulich wrote:
> On 06.08.2026 13:48, Andrew Cooper wrote:
> > On 06/08/2026 12:04 pm, Roger Pau Monne wrote:
> >> --- a/xen/arch/x86/x86_64/pci.c
> >> +++ b/xen/arch/x86/x86_64/pci.c
> >> @@ -26,7 +26,7 @@ uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg)
> >>  
> >>  uint16_t pci_conf_read16(pci_sbdf_t sbdf, unsigned int reg)
> >>  {
> >> -    if ( sbdf.seg || reg > 255 )
> >> +    if ( sbdf.seg || reg > 255 || !IS_ALIGNED(reg, 2) )
> >>      {
> >>          uint32_t value;
> >>  
> > 
> > Personally, I think this is making a bad situation worse.
> > 
> > Baring quirks (i.e. K10 era), there is never a case where we want to use
> > the IO Ports when we've got ECAM.  Furthermore, on AMD systems when
> > we're lacking ECAM we can still access Extended Config Space; something
> > which Xen currently gets wrong in several ways.
> > 
> > The IO ports require a global spinlock in Xen, and then a global
> > resource in hardware just to be able to translate the IO access back
> > into the ECAM access we passed on originally.  i.e. from a safety
> > non-interference point of view, you want to veto any use of the IO ports
> > by Xen.
> > 
> > Xen needs to use ECAM, and only fall back to IO Ports if we think there
> > isn't ECAM covering the target sbdf.  This will cause (mis)alignment to
> > get fixed automatically.  It will also be a substantial perf boost in
> > the general case; all the MSI/MSI-X editing we do (far too frequently)
> > is in Legacy Config Space just uses IO Ports.
> 
> While I agree, that's a bigger change which likely is going to be unsuitable
> for (immediate) backporting. Nevertheless the cross-device access that the
> changes as presented could cause needs preventing, by altering the checks at
> the start of pci_mmcfg_{read,write}().

Hm, OK. I was planning to be a bit mire strict in
vpci_access_allowed(), but I can also adjust pci_mmcfg_{read,write}().

Thanks, Roger.