On 21-02-02 20:43:38, Jonathan Cameron wrote: > On Tue, 2 Feb 2021 11:45:05 -0800 > Ben Widawsky <b...@bwidawsk.net> wrote: > > > On 21-02-02 19:21:35, Jonathan Cameron wrote: > > > On Mon, 1 Feb 2021 16:59:34 -0800 > > > Ben Widawsky <ben.widaw...@intel.com> wrote: > > > > > > > CXL host bridges themselves may have MMIO. Since host bridges don't have > > > > a BAR they are treated as special for MMIO. > > > > > > > > Signed-off-by: Ben Widawsky <ben.widaw...@intel.com> > > > > > > > > -- > > > > > > > > It's arbitrarily chosen here to pick 0xD0000000 as the base for the host > > > > bridge MMIO. I'm not sure what the right way to find free space for > > > > platform hardcoded things like this is. > > > > > > Seems like this needs to come from the machine definition. > > > This is fairly easy for arm/virt, where there is a clearly laid out > > > memory map. > > > For hw/i386 I'm less sure on how to do it. > > > > I think this is how to do it :D > > It may well be, but they you'll need to find a suitable region and document > it and ensure no one else ever tramples on it. Easy to do on a physical > system, > bit trickier in emulation. >
Maybe? x86 is full of magic physical address holes. As long as it's conveyed to EDK via _CRS, I think it's pretty safe. If something else tries to use the same address, you should get a fairly obvious error. Document somehow, yes please. > > > > > > > > Having said that, for this particular magic device, we do have a PCI EP > > > associated with it. How about putting all the host bridge MMIO into a > > > BAR of that rather than having it separate. > > > That has the added advantage of making it discoverable from firmware. > > > > > > Any normal system is going to have this is impdef for discovery anyway. > > > > > > > This is not how it's expected to work for Intel at least. If the device was > > discoverable you wouldn't need CEDT/CHBS. The magic host bridges are only > > advertised via the CEDT. > > I agree on a normal system (i.e. a real one) this doesn't work, > but then a normal system doesn't involve a magic PCI RCiEP that also happens > to instantiate an extra host bridge. This is what pxb_pcie is doing and > what your pxb_cxl is almost doing. > > > > > When I build and run QEMU for x86_64, I do not see the host bridge in the > > pci > > topology, do you (it's meant to not be there)? > > > > 00:00.0 Host bridge: Intel Corporation 82G33/G31/P35/P31 Express DRAM > > Controller > > ... > > 34:00.0 PCI bridge: Intel Corporation Device 7075 > > 35:00.0 Memory controller [0502]: Intel Corporation Device 0d93 (rev 01) > > > > That's Q35, Root Port, and Type 3 device respectively. > > You don't see the host bridge, for pxb_cxl, but for pxb_pcie you do. > 00:06.0 Host bridge: Red Hat, Inc QEMU PCIe Expander bridge. > If you have another device after your pxb-cxl you'll also notice that there > is a hole punched in the list where you'd expect pxb-cxl to be (device number > skipped). (that had me confused earlier). > > This seems to be because no VID etc (unlike pxb-pcie). > Right. This was in an earlier version of the series and you made me realize if I got rid of them that it disappears. I really like that this more accurately represents the hardware. I agree it can be implemented more simply, but why do it if you can accurately model it? > I gave vendor and device IDs (and a bar to test that could be done) and it > then appears just like pxb_pcie does. Hence handy place to hang our > magic memory off so that EDK2 or similar can work with it and indeed > construct he CHBS as needed so we can get to this via the same paths as > a normal system. It's a bit convoluted but in some ways more elegant. > What are you looking to get out of EDK2 or similar? Anything you want to convey should work with _CRS, I think. That was the path I was going down. > Jonathan > > > > > > That would then let you drop the separate definition of CXLHost structure > > > though it needs a bit of figuring out what to do with the memory window > > > setup etc. > > > > > > I tried hacking it together, but not gotten it working yet. > > > > > > > --- > > > > hw/pci-bridge/pci_expander_bridge.c | 53 ++++++++++++++++++++++++++++- > > > > include/hw/cxl/cxl.h | 2 ++ > > > > 2 files changed, 54 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/pci-bridge/pci_expander_bridge.c > > > > b/hw/pci-bridge/pci_expander_bridge.c > > > > index 5021b60435..226a8a5fff 100644 > > > > --- a/hw/pci-bridge/pci_expander_bridge.c > > > > +++ b/hw/pci-bridge/pci_expander_bridge.c > > > > @@ -17,6 +17,7 @@ > > > > #include "hw/pci/pci_host.h" > > > > #include "hw/qdev-properties.h" > > > > #include "hw/pci/pci_bridge.h" > > > > +#include "hw/cxl/cxl.h" > > > > #include "qemu/range.h" > > > > #include "qemu/error-report.h" > > > > #include "qemu/module.h" > > > > @@ -70,6 +71,12 @@ struct PXBDev { > > > > int32_t uid; > > > > }; > > > > > > > > +typedef struct CXLHost { > > > > + PCIHostState parent_obj; > > > > + > > > > + CXLComponentState cxl_cstate; > > > > +} CXLHost; > > > > + > > > > static PXBDev *convert_to_pxb(PCIDevice *dev) > > > > { > > > > /* A CXL PXB's parent bus is PCIe, so the normal check won't work > > > > */ > > > > @@ -85,6 +92,9 @@ static GList *pxb_dev_list; > > > > > > > > #define TYPE_PXB_HOST "pxb-host" > > > > > > > > +#define TYPE_PXB_CXL_HOST "pxb-cxl-host" > > > > +#define PXB_CXL_HOST(obj) OBJECT_CHECK(CXLHost, (obj), > > > > TYPE_PXB_CXL_HOST) > > > > + > > > > static int pxb_bus_num(PCIBus *bus) > > > > { > > > > PXBDev *pxb = convert_to_pxb(bus->parent_dev); > > > > @@ -198,6 +208,46 @@ static const TypeInfo pxb_host_info = { > > > > .class_init = pxb_host_class_init, > > > > }; > > > > > > > > +static void pxb_cxl_realize(DeviceState *dev, Error **errp) > > > > +{ > > > > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > > > > + PCIHostState *phb = PCI_HOST_BRIDGE(dev); > > > > + CXLHost *cxl = PXB_CXL_HOST(dev); > > > > + CXLComponentState *cxl_cstate = &cxl->cxl_cstate; > > > > + struct MemoryRegion *mr = &cxl_cstate->crb.component_registers; > > > > + > > > > + cxl_component_register_block_init(OBJECT(dev), cxl_cstate, > > > > + TYPE_PXB_CXL_HOST); > > > > + sysbus_init_mmio(sbd, mr); > > > > + > > > > + /* FIXME: support multiple host bridges. */ > > > > + sysbus_mmio_map(sbd, 0, CXL_HOST_BASE + > > > > + memory_region_size(mr) * > > > > pci_bus_uid(phb->bus)); > > > > +} > > > > + > > > > +static void pxb_cxl_host_class_init(ObjectClass *class, void *data) > > > > +{ > > > > + DeviceClass *dc = DEVICE_CLASS(class); > > > > + PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class); > > > > + > > > > + hc->root_bus_path = pxb_host_root_bus_path; > > > > + dc->fw_name = "cxl"; > > > > + dc->realize = pxb_cxl_realize; > > > > + /* Reason: Internal part of the pxb/pxb-pcie device, not usable by > > > > itself */ > > > > + dc->user_creatable = false; > > > > +} > > > > + > > > > +/* > > > > + * This is a device to handle the MMIO for a CXL host bridge. It does > > > > nothing > > > > + * else. > > > > + */ > > > > +static const TypeInfo cxl_host_info = { > > > > + .name = TYPE_PXB_CXL_HOST, > > > > + .parent = TYPE_PCI_HOST_BRIDGE, > > > > + .instance_size = sizeof(CXLHost), > > > > + .class_init = pxb_cxl_host_class_init, > > > > +}; > > > > + > > > > /* > > > > * Registers the PXB bus as a child of pci host root bus. > > > > */ > > > > @@ -272,7 +322,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, > > > > enum BusType type, > > > > dev_name = dev->qdev.id; > > > > } > > > > > > > > - ds = qdev_new(TYPE_PXB_HOST); > > > > + ds = qdev_new(type == CXL ? TYPE_PXB_CXL_HOST : TYPE_PXB_HOST); > > > > if (type == PCIE) { > > > > bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0, > > > > TYPE_PXB_PCIE_BUS); > > > > } else if (type == CXL) { > > > > @@ -466,6 +516,7 @@ static void pxb_register_types(void) > > > > type_register_static(&pxb_pcie_bus_info); > > > > type_register_static(&pxb_cxl_bus_info); > > > > type_register_static(&pxb_host_info); > > > > + type_register_static(&cxl_host_info); > > > > type_register_static(&pxb_dev_info); > > > > type_register_static(&pxb_pcie_dev_info); > > > > type_register_static(&pxb_cxl_dev_info); > > > > diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h > > > > index 362cda40de..6bc344f205 100644 > > > > --- a/include/hw/cxl/cxl.h > > > > +++ b/include/hw/cxl/cxl.h > > > > @@ -17,5 +17,7 @@ > > > > #define COMPONENT_REG_BAR_IDX 0 > > > > #define DEVICE_REG_BAR_IDX 2 > > > > > > > > +#define CXL_HOST_BASE 0xD0000000 > > > > + > > > > #endif > > > > > > > > > > > > >