Re: [PATCH v2 7/8] cxl/port: Introduce cxl_port objects

[Dan Williams]

Hi Bjorn, thanks for taking a look.


On Thu, Apr 8, 2021 at 3:42 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> [+cc Greg, Rafael, Matthew: device model questions]
>
> Hi Dan,
>
> On Thu, Apr 01, 2021 at 07:31:20AM -0700, Dan Williams wrote:
> > Once the cxl_root is established then other ports in the hierarchy can
> > be attached. The cxl_port object, unlike cxl_root that is associated
> > with host bridges, is associated with PCIE Root Ports or PCIE Switch
> > Ports. Add cxl_port instances for all PCIE Root Ports in an ACPI0016
> > host bridge.
>
> I'm not a device model expert, but I'm not sure about adding a new
> /sys/bus/cxl/devices hierarchy.  I'm under the impression that CXL
> devices will be enumerated by the PCI core as PCIe devices.

Yes, PCIe is involved, but mostly only for the CXL.io slow path
(configuration and provisioning via mailbox) when we're talking about
memory expander devices (CXL calls these Type-3). So-called "Type-3"
support is the primary driver of this infrastructure.

You might be thinking of CXL accelerator devices that will look like
plain PCIe devices that happen to participate in the CPU cache
hierarchy (CXL calls these Type-1). There will also be accelerator
devices that want to share coherent memory with the system (CXL calls
these Type-2).

The infrastructure being proposed here is primarily for the memory
expander (Type-3) device case where the PCI sysfs hierarchy is wholly
unsuited for modeling it. A single CXL memory region device may span
multiple endpoints, switches, and host bridges. It poses similar
stress to an OS device model as RAID where there is a driver for the
component contributors to an upper level device / driver that exposes
the RAID Volume (CXL memory region interleave set). The CXL memory
decode space (HDM: Host Managed Device Memory) is independent of the
PCIe MMIO BAR space.

That's where the /sys/bus/cxl hierarchy is needed, to manage the HDM
space across the CXL topology in a way that is foreign to PCIE (HDM
Decoder hierarchy).

> Doesn't
> that mean we will have one struct device in the pci_dev, and another
> one in the cxl_port?

Yes, that is the proposal.

> That seems like an issue to me.  More below.

hmm...

>
> > The cxl_port instances for PCIE Switch Ports are not
> > included here as those are to be modeled as another service device
> > registered on the pcie_port_bus_type.
>
> I'm hesitant about the idea of adding more uses of pcie_port_bus_type.
> I really dislike portdrv because it makes a parallel hierarchy:
>
>   /sys/bus/pci
>   /sys/bus/pci_express
>
> for things that really should not be different.  There's a struct
> device in pci_dev, and potentially several pcie_devices, each with
> another struct device.  We make these pcie_device things for AER, DPC,
> hotplug, etc.  E.g.,
>
>   /sys/bus/pci/devices/0000:00:1c.0
>   /sys/bus/pci_express/devices/0000:00:1c.0:pcie002  # AER
>   /sys/bus/pci_express/devices/0000:00:1c.0:pcie010  # BW notification
>
> These are all the same PCI device.  AER is a PCI capability.
> Bandwidth notification is just a feature of all Downstream Ports.  I
> think it makes zero sense to have extra struct devices for them.  From
> a device point of view (enumeration, power management, VM assignment),
> we can't manage them separately from the underlying PCI device.  For
> example, we have three separate "power/" directories, but obviously
> there's only one point of control (00:1c.0):
>
>   /sys/devices/pci0000:00/0000:00:1c.0/power/
>   /sys/devices/pci0000:00/0000:00:1c.0/0000:00:1c.0:pcie002/power/
>   /sys/devices/pci0000:00/0000:00:1c.0/0000:00:1c.0:pcie010/power/

The superfluous power/ issue can be cleaned up with
device_set_pm_not_required().

What are the other problems this poses, because in other areas this
ability to subdivide a device's functionality into sub-drivers is a
useful organization principle? So much so that several device writer
teams came together to create the auxiliary-bus for the purpose of
allowing sub-drivers to be carved off for independent functionality
similar to the portdrv organization.

That said, I'm open to CXL switch support *not* building on the
portdrv model, but I'm not yet on the same page with your concern.