Re: [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Apr 13, 2022 at 01:48:14PM +0000, Bertrand Marquis wrote:
> Hi Jan,
> 
> > On 11 Apr 2022, at 10:40, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > 
> > There's no good reason to use these when we already have a pci_sbdf_t
> > type object available. This extends to the use of PCI_BUS() in
> > pci_ecam_map_bus() as well.
> > 
> > No change to generated code (with gcc11 at least, and I have to admit
> > that I didn't expect compilers to necessarily be able to spot the
> > optimization potential on the original code).
> > 
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > ---
> > Note that the Arm changes are "blind": I haven't been able to spot a way
> > to at least compile test the changes there; the code looks to be
> > entirely dead.
> > 
> > --- a/xen/arch/arm/pci/ecam.c
> > +++ b/xen/arch/arm/pci/ecam.c
> > @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc
> >         container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
> >     unsigned int devfn_shift = ops->bus_shift - 8;
> >     void __iomem *base;
> > -
> > -    unsigned int busn = PCI_BUS(sbdf.bdf);
> > +    unsigned int busn = sbdf.bus;
> > 
> >     if ( busn < cfg->busn_start || busn > cfg->busn_end )
> >         return NULL;
> > @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc
> >     busn -= cfg->busn_start;
> >     base = cfg->win + (busn << ops->bus_shift);
> > 
> > -    return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where;
> > +    return base + (sbdf.df << devfn_shift) + where;
> 
> I think this should be sbdf.bdf instead (typo you removed the b).

I don't think so, notice PCI_DEVFN2(sbdf.bdf) which is extracting the
devfn from sbdf.bdf. That's not needed, as you can just get the devfn
directly from sbdf.df.

Or else the original code is wrong, and the PCI_DEVFN2 shouldn't be
there.

Thanks, Roger.