Re: [Xen-devel] [PATCH v3 09/13] pci: switch pci_conf_read32 to use pci_sbdf_t

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>, WeiLiu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Brian Woods <brian.woods@amd.com>
Subject: Re: [Xen-devel] [PATCH v3 09/13] pci: switch pci_conf_read32 to use pci_sbdf_t
Date: Fri, 14 Jun 2019 11:06:15 +0200	[thread overview]
Message-ID: <20190614090615.ywfm45lm3qvngo64@Air-de-Roger> (raw)
In-Reply-To: <5D025F630200007800237F41@prv1-mh.provo.novell.com>

On Thu, Jun 13, 2019 at 08:36:19AM -0600, Jan Beulich wrote:
> >>> On 07.06.19 at 11:22, <roger.pau@citrix.com> wrote:
> > @@ -817,7 +811,7 @@ static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8 func, u8 bir, int vf)
> >          if ( ++bir >= limit )
> >              return 0;
> >          return addr + disp +
> > -               ((u64)pci_conf_read32(seg, bus, slot, func,
> > +               ((u64)pci_conf_read32(PCI_SBDF(seg, bus, slot, func),
> >                                       base + bir * 4) << 32);
> 
> Not taking the opportunity to switch to uint64_t here, like you do
> elsewhere?
> 
> > @@ -750,7 +747,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> >              for ( i = 0; i < PCI_SRIOV_NUM_BARS; )
> >              {
> >                  unsigned int idx = pos + PCI_SRIOV_BAR + i * 4;
> > -                u32 bar = pci_conf_read32(seg, bus, slot, func, idx);
> > +                u32 bar = pci_conf_read32(pdev->sbdf, idx);
> 
> Similarly here.

Sorry, I tried to fix the surrounding issues when touching the code,
but some of those slipped. Will fix in next version.

> > --- a/xen/drivers/passthrough/vtd/quirks.c
> > +++ b/xen/drivers/passthrough/vtd/quirks.c
> > @@ -128,9 +128,11 @@ static void __init map_igd_reg(void)
> >      if ( igd_reg_va )
> >          return;
> >  
> > -    igd_mmio   = pci_conf_read32(0, 0, IGD_DEV, 0, PCI_BASE_ADDRESS_1);
> > +    igd_mmio   = pci_conf_read32(PCI_SBDF(0, 0, IGD_DEV, 0),
> 
> Afaict at this point all uses of IGD_DEV are in constructs like this one.
> As previously say, I think IGD_DEV itself would now better become an
> invocation of PCI_SBDF(). Same for IOH_DEV then.

Is it fine to do this as a followup patch?

Or else I would also have to modify the pci_conf_read{8,16} calls that
use IGD_DEV or IOH_DEV in this patch.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel