Re: [PATCH 2/3] PCI: vmd: Expose VMD details from BIOS

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Mon, Nov 04, 2019 at 06:07:00PM +0000, Lorenzo Pieralisi wrote:
> On Fri, Nov 01, 2019 at 10:16:39PM +0000, Derrick, Jonathan wrote:
> > Hi Bjorn,
> > 
> > On Fri, 2019-11-01 at 16:53 -0500, Bjorn Helgaas wrote:
> > > [+cc Andrew]
> > > 
> > > On Wed, Oct 16, 2019 at 11:04:47AM -0600, Jon Derrick wrote:
> > > > When some VMDs are enabled and others are not, it's difficult to
> > > > determine which IIO stack corresponds to the enabled VMD.
> > > > 
> > > > To assist userspace with management tasks, VMD BIOS will write the VMD
> > > > instance number and socket number into the first enabled root port's IO
> > > > Base/Limit registers prior to OS handoff. VMD driver can capture this
> > > > information and expose it to userspace.
> > > 
> > > Hmmm, I'm not sure I understand this, but it sounds possibly fragile.
> > > Are these Root Ports visible to the generic PCI core device
> > > enumeration?  If so, it will find them and read these I/O window
> > > registers.  Maybe today the PCI core doesn't change them, but I'm not
> > > sure we should rely on them always being preserved until the vmd
> > > driver can claim the device.
> > > 
> > 
> > The Root Ports are on the VMD PCI domain, and this IO BASE/LIMIT
> > parsing occurs before this PCI domain is exposed to the generic PCI
> > scancode with pci_scan_child_bus(). Until that point the VMD PCI domain
> > is invisible to the kernel outside of /dev/mem or resource0.
> 
> That's because the VMD controller is a PCI device itself and its
> BARs values are used to configure the VMD host controller.
> 
> Interesting.
> 
> To add to Bjorn's question, this reasoning assumes that whatever
> code enumerates the PCI device representing the VMD host controller
> does not overwrite its BARs upon bus enumeration otherwise the VMD
> controller configuration would be lost. Am I reading the current
> code correctly ?

Sorry, I just went through the code again, I think the VMD controller
PCI device BARs can and are allowed to be reassigned by the PCI
enumeration code - I misread the code, so I raised a non-existent issue
here, they are like any other PCI device MEM/IO BARs in this respect.

Lorenzo

> I assume there is not anything you can do to add firmware bindings to
> the VMD host controller PCI device to describe these properties you are
> exporting, so config space is the only available conduit to report them
> to an OS.
> 
> Lorenzo
> 
> > However, yes, it is somewhat fragile in that a third-party driver could
> > attach to the VMD endpoint prior to the VMD driver and modify the
> > values. A /dev/mem or resource0 user could also do this on an
> > unattached VMD endpoint.
> > 
> > I'm wondering if this would also be better suited for a special reset
> > in quirks.c, but it would need to expose a bit of VMD config accessing
> > in quirks.c to do that.
> > 
> > > But I guess you're using a special config accessor (vmd_cfg_read()),
> > > so these are probably invisible to the generic enumeration?
> > > 
> > 
> > Yes the VMD domain is invisible to generic PCI until the domain is
> > enumerated late in vmd_enable_domain().
> > 
> > > > + * for_each_vmd_root_port - iterate over all enabled VMD Root Ports
> > > > + * @vmd: &struct vmd_dev VMD device descriptor
> > > > + * @rp: int iterator cursor
> > > > + * @temp: u32 temporary value for config read
> > > > + *
> > > > + * VMD Root Ports are located in the VMD PCIe Domain at 00:[0-3].0, and config
> > > > + * space can be determinately accessed through the VMD Config BAR. Because VMD
> > > 
> > > I'm not sure how to parse "determinately accessed".  Maybe this config
> > > space can *only* be accessed via the VMD Config BAR?
> > 
> > Perhaps it should instead say determinately addressed, as each Root
> > Port config space is addressable at some offset of N * 0x8000 from the
> > base of the VMD endpoint config bar. I can see the comment may not be
> > helpful as that detail is abstracted using the vmd_cfg_read() helper.
> > 
> > 
> > > 
> > > > + * Root Ports can be individually disabled, it's important to iterate for the
> > > > + * first enabled Root Port as determined by reading the Vendor/Device register.
> > > > + */
> > > > +#define for_each_vmd_root_port(vmd, rp, temp)				\
> > > > +	for (rp = 0; rp < 4; rp++)					\
> > > > +		if (vmd_cfg_read(vmd, 0, PCI_DEVFN(root_port, 0),	\
> > > > +				 PCI_VENDOR_ID, 4, &temp) ||		\
> > > > +		    temp == 0xffffffff) {} else