[PATCH v2 0/2] vpci: allow unaligned accesses by the hardware domain

[PATCH v2 0/2] vpci: allow unaligned accesses by the hardware domain

[Roger Pau Monne]

Hello,

Mostly the same as v1, plus an additional patch that refuses accesses
that cross a device boundary in pci_mmcfg_{read,write}() itself.

Thanks, Roger.

Roger Pau Monne (2):
  x86/pci: prevent cross-device accesses in pci_mmcfg_{read,write}()
  xen/vpci: allow unaligned accesses by the hardware domain

 tools/include/xen-tools/common-macros.h | 2 ++
 xen/arch/x86/x86_64/mmconfig_64.c       | 6 ++++--
 xen/arch/x86/x86_64/pci.c               | 8 ++++----
 xen/drivers/vpci/vpci.c                 | 6 ++++--
 4 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] x86/pci: prevent cross-device accesses in pci_mmcfg_{read,write}()

[Roger Pau Monne]

Introduce a specific check that prevents an accesses from spilling across
two devices.

Signed-off-by: Roger Pau Monné <roger@xenproject.org>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/x86_64/mmconfig_64.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/x86_64/mmconfig_64.c b/xen/arch/x86/x86_64/mmconfig_64.c
index 940cf6d7471b..91b1a398e646 100644
--- a/xen/arch/x86/x86_64/mmconfig_64.c
+++ b/xen/arch/x86/x86_64/mmconfig_64.c
@@ -61,7 +61,8 @@ int pci_mmcfg_read(unsigned int seg, unsigned int bus,
     char __iomem *addr;
 
     /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
-    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
+    if (unlikely((bus > 255) || (devfn > 255) ||
+                 (reg + len > PCI_CFG_SPACE_EXP_SIZE))) {
 err:        *value = -1;
         return -EINVAL;
     }
@@ -91,7 +92,8 @@ int pci_mmcfg_write(unsigned int seg, unsigned int bus,
     char __iomem *addr;
 
     /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
-    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
+    if (unlikely((bus > 255) || (devfn > 255) ||
+                 (reg + len > PCI_CFG_SPACE_EXP_SIZE)))
         return -EINVAL;
 
     addr = pci_dev_base(seg, bus, devfn);
-- 
2.53.0

Re: [PATCH v2 1/2] x86/pci: prevent cross-device accesses in pci_mmcfg_{read,write}()

[Jan Beulich]

On 06.08.2026 17:26, Roger Pau Monne wrote:
> Introduce a specific check that prevents an accesses from spilling across
> two devices.
> 
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit with a remark:

> --- a/xen/arch/x86/x86_64/mmconfig_64.c
> +++ b/xen/arch/x86/x86_64/mmconfig_64.c
> @@ -61,7 +61,8 @@ int pci_mmcfg_read(unsigned int seg, unsigned int bus,
>      char __iomem *addr;
>  
>      /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
> -    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
> +    if (unlikely((bus > 255) || (devfn > 255) ||
> +                 (reg + len > PCI_CFG_SPACE_EXP_SIZE))) {
>  err:        *value = -1;
>          return -EINVAL;
>      }
> @@ -91,7 +92,8 @@ int pci_mmcfg_write(unsigned int seg, unsigned int bus,
>      char __iomem *addr;
>  
>      /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
> -    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
> +    if (unlikely((bus > 255) || (devfn > 255) ||
> +                 (reg + len > PCI_CFG_SPACE_EXP_SIZE)))
>          return -EINVAL;
>  
>      addr = pci_dev_base(seg, bus, devfn);

In both cases the unlikely() uses won't have the intended effect, from all I
know. They would help as used here only if the compiler managed to fold all
three parts of the ||-expression into a single conditional branch, which I
don't think it would end up doing.

Jan

Re: [PATCH v2 1/2] x86/pci: prevent cross-device accesses in pci_mmcfg_{read,write}()

[Roger Pau Monné]

On Fri, Aug 07, 2026 at 08:22:44AM +0200, Jan Beulich wrote:
> On 06.08.2026 17:26, Roger Pau Monne wrote:
> > Introduce a specific check that prevents an accesses from spilling across
> > two devices.
> > 
> > Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit with a remark:
> 
> > --- a/xen/arch/x86/x86_64/mmconfig_64.c
> > +++ b/xen/arch/x86/x86_64/mmconfig_64.c
> > @@ -61,7 +61,8 @@ int pci_mmcfg_read(unsigned int seg, unsigned int bus,
> >      char __iomem *addr;
> >  
> >      /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
> > -    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
> > +    if (unlikely((bus > 255) || (devfn > 255) ||
> > +                 (reg + len > PCI_CFG_SPACE_EXP_SIZE))) {
> >  err:        *value = -1;
> >          return -EINVAL;
> >      }
> > @@ -91,7 +92,8 @@ int pci_mmcfg_write(unsigned int seg, unsigned int bus,
> >      char __iomem *addr;
> >  
> >      /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
> > -    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
> > +    if (unlikely((bus > 255) || (devfn > 255) ||
> > +                 (reg + len > PCI_CFG_SPACE_EXP_SIZE)))
> >          return -EINVAL;
> >  
> >      addr = pci_dev_base(seg, bus, devfn);
> 
> In both cases the unlikely() uses won't have the intended effect, from all I
> know. They would help as used here only if the compiler managed to fold all
> three parts of the ||-expression into a single conditional branch, which I
> don't think it would end up doing.

I don't mind dropping the unlikely() while changing the line.  I tend
to leave those alone if present, even when I'm not sure they are
actually helpful.  The comment ahead of the check is also not very
useful IMO, but I've decided to leave it alone.

Thanks, Roger.

Re: [PATCH v2 1/2] x86/pci: prevent cross-device accesses in pci_mmcfg_{read,write}()

[Jan Beulich]

On 07.08.2026 10:01, Roger Pau Monné wrote:
> On Fri, Aug 07, 2026 at 08:22:44AM +0200, Jan Beulich wrote:
>> On 06.08.2026 17:26, Roger Pau Monne wrote:
>>> Introduce a specific check that prevents an accesses from spilling across
>>> two devices.
>>>
>>> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> albeit with a remark:
>>
>>> --- a/xen/arch/x86/x86_64/mmconfig_64.c
>>> +++ b/xen/arch/x86/x86_64/mmconfig_64.c
>>> @@ -61,7 +61,8 @@ int pci_mmcfg_read(unsigned int seg, unsigned int bus,
>>>      char __iomem *addr;
>>>  
>>>      /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
>>> -    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
>>> +    if (unlikely((bus > 255) || (devfn > 255) ||
>>> +                 (reg + len > PCI_CFG_SPACE_EXP_SIZE))) {
>>>  err:        *value = -1;
>>>          return -EINVAL;
>>>      }
>>> @@ -91,7 +92,8 @@ int pci_mmcfg_write(unsigned int seg, unsigned int bus,
>>>      char __iomem *addr;
>>>  
>>>      /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
>>> -    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
>>> +    if (unlikely((bus > 255) || (devfn > 255) ||
>>> +                 (reg + len > PCI_CFG_SPACE_EXP_SIZE)))
>>>          return -EINVAL;
>>>  
>>>      addr = pci_dev_base(seg, bus, devfn);
>>
>> In both cases the unlikely() uses won't have the intended effect, from all I
>> know. They would help as used here only if the compiler managed to fold all
>> three parts of the ||-expression into a single conditional branch, which I
>> don't think it would end up doing.
> 
> I don't mind dropping the unlikely() while changing the line.  I tend
> to leave those alone if present, even when I'm not sure they are
> actually helpful.

I think unlikely() is actually reasonable to have here, yet it would need to
be four of them, not just one. Unless reachable from a DomU-accessible path,
converting to BUG_ON() (or ASSERT_UNREACHABLE()) instead would also be an
option.

>  The comment ahead of the check is also not very
> useful IMO, but I've decided to leave it alone.

Indeed, that's an odd thing to survive over 20 years (I think).

Jan

Re: [PATCH v2 1/2] x86/pci: prevent cross-device accesses in pci_mmcfg_{read,write}()

[Roger Pau Monné]

On Wed, Aug 12, 2026 at 08:28:50AM +0200, Jan Beulich wrote:
> On 07.08.2026 10:01, Roger Pau Monné wrote:
> > On Fri, Aug 07, 2026 at 08:22:44AM +0200, Jan Beulich wrote:
> >> On 06.08.2026 17:26, Roger Pau Monne wrote:
> >>> Introduce a specific check that prevents an accesses from spilling across
> >>> two devices.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> >>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >> albeit with a remark:
> >>
> >>> --- a/xen/arch/x86/x86_64/mmconfig_64.c
> >>> +++ b/xen/arch/x86/x86_64/mmconfig_64.c
> >>> @@ -61,7 +61,8 @@ int pci_mmcfg_read(unsigned int seg, unsigned int bus,
> >>>      char __iomem *addr;
> >>>  
> >>>      /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
> >>> -    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
> >>> +    if (unlikely((bus > 255) || (devfn > 255) ||
> >>> +                 (reg + len > PCI_CFG_SPACE_EXP_SIZE))) {
> >>>  err:        *value = -1;
> >>>          return -EINVAL;
> >>>      }
> >>> @@ -91,7 +92,8 @@ int pci_mmcfg_write(unsigned int seg, unsigned int bus,
> >>>      char __iomem *addr;
> >>>  
> >>>      /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
> >>> -    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
> >>> +    if (unlikely((bus > 255) || (devfn > 255) ||
> >>> +                 (reg + len > PCI_CFG_SPACE_EXP_SIZE)))
> >>>          return -EINVAL;
> >>>  
> >>>      addr = pci_dev_base(seg, bus, devfn);
> >>
> >> In both cases the unlikely() uses won't have the intended effect, from all I
> >> know. They would help as used here only if the compiler managed to fold all
> >> three parts of the ||-expression into a single conditional branch, which I
> >> don't think it would end up doing.
> > 
> > I don't mind dropping the unlikely() while changing the line.  I tend
> > to leave those alone if present, even when I'm not sure they are
> > actually helpful.
> 
> I think unlikely() is actually reasonable to have here, yet it would need to
> be four of them, not just one. Unless reachable from a DomU-accessible path,
> converting to BUG_ON() (or ASSERT_UNREACHABLE()) instead would also be an
> option.

Hm, let's do that in a separate patch, I don't want to merge it with
the work here.

> >  The comment ahead of the check is also not very
> > useful IMO, but I've decided to leave it alone.
> 
> Indeed, that's an odd thing to survive over 20 years (I think).

I can also take care of the comment there.

Thanks, Roger.

Re: [PATCH v2 1/2] x86/pci: prevent cross-device accesses in pci_mmcfg_{read,write}()

[Stewart Hildebrand]

On 8/6/26 17:26, Roger Pau Monne wrote:
> Introduce a specific check that prevents an accesses from spilling across
> two devices.
> 
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
Reviewed-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

[PATCH v2 2/2] 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.

Note that vpci_ecam_{read,write}() already refuse accesses that cross a
device boundary unconditionally.

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                 | 6 ++++--
 3 files changed, 10 insertions(+), 6 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..9e2c27e3a300 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;
@@ -695,8 +697,8 @@ bool vpci_access_allowed(unsigned int reg, unsigned int len)
         return false;
 #endif
 
-    /* Check that access is size aligned. */
-    if ( (reg & (len - 1)) )
+    /* Refuse unaligned accesses for non-hardware domains. */
+    if ( !is_hardware_domain(currd) && !IS_ALIGNED(reg, len) )
         return false;
 
     return true;
-- 
2.53.0

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

[Jan Beulich]

On 06.08.2026 17:26, 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.
> 
> 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.
> 
> Note that vpci_ecam_{read,write}() already refuse accesses that cross a
> device boundary unconditionally.
> 
> Reported-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>

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

Albeit without the intent to override possible remaining objections by Andrew.

Jan

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

[Stewart Hildebrand]

On 8/6/26 17:26, 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.
> 
> 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.
> 
> Note that vpci_ecam_{read,write}() already refuse accesses that cross a
> device boundary unconditionally.
> 
> Reported-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
Reviewed-by: Stewart Hildebrand <stewart.hildebrand@amd.com>