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

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é <[email protected]>
> 
> Reviewed-by: Jan Beulich <[email protected]>
> 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.