Re: [PATCH v2 0/8] hw/cxl: Move CXL emulation options and state to machines.

[Jonathan Cameron]

On Mon, 6 Jun 2022 10:33:38 -0700
Ben Widawsky <bwidawsk@kernel.org> wrote:

> On 22-06-01 17:42:27, Jonathan Cameron wrote:
> > Changes since v1 (thanks to Paolo Bonzini)
> > * Update 'description' of cxl-fmw as suggested to mention it's an array.
> > * Add a wrapper cxl_hook_up_pxb_registers() to cxl-host.c as it'll be common
> >   for all machines using CXL with PXB.
> > 
> > Run through the CI at:
> > https://gitlab.com/jic23/qemu/-/pipelines/553257456
> >  
> > V1 Cover letter:
> > 
> > Currently only machine with CXL support upstream is i386/pc but arm/virt
> > patches have been posted and once this is merged an updated series will
> > follow. Switch support is queued behind this as well because they both
> > include documentation updates.
> > 
> > Paolo Bonzini highlighted a couple of issues with the current CXL
> > emulation code.
> > 
> > * Top level parameter rather than machine for fixed memory windows
> > 
> >   The --cxl-fixed-memory-window top level command line parameters won't play
> >   well with efforts to make it possible to instantiate entire machines via
> >   RPC. Better to move these to be machine configuration.  This change is
> >   relatively straight forward, but does result in very long command lines
> >   (cannot break fixed window setup into multiple -M entries).
> > 
> > * Move all CXL stuff to machine specific code and helpers
> > 
> >   To simplify the various interactions between machine setup and host
> >   bridges etc, currently various CXL steps are called from the generic
> >   core/machine.c and softmmu/vl.c + there are CXL elements in MachineState.
> > 
> >   Much of this is straight forward to do with one exception:
> >   The CXL pci_expander_bridge host bridges require MMIO register space.
> >   This series does this by walking the bus and filling the register space
> >   in via the machine_done callback. This is similar to the walk done for
> >   identifying host bridges in the ACPI building code but it is rather ugly
> >   and postpones rejection of PXB_CXL instances where cxl=off (default).
> > 
> > All comments welcome, but the first patch at least changes the command-line
> > so to avoid have to add backwards compatibility code, it would be great
> > to merge that before 7.1 is released.
> >   
> 
> LGTM overall. I'm not thrilled with introducing another [sub]scronym "fmw", 
> but
> otherwise, no complaints.

Agreed, it's a less than ideal compromise :( 

Spelling it out now seems too long as it has to be repeated a lot in a given 
commandline.
I think cfmw is too vague for a QEMU parameter and for CXL only the ACPI 
"structure"
(hence the s) has defined acronym of CFMWS.  The underlying actual routing 
hardware
is impdef so never given an acronym (there is only one direct reference to the
window itself in the CXL spec and that's spelled out as "fixed-memory window").

Jonathan



> Series is:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> > Thanks,
> > 
> > Jonathan
> > 
> > Jonathan Cameron (8):
> >   hw/cxl: Make the CXL fixed memory window setup a machine parameter.
> >   hw/acpi/cxl: Pass in the CXLState directly rather than MachineState
> >   hw/cxl: Push linking of CXL targets into i386/pc rather than in
> >     machine.c
> >   tests/acpi: Allow modification of q35 CXL CEDT table.
> >   pci/pci_expander_bridge: For CXL HB delay the HB register memory
> >     region setup.
> >   tests/acpi: Update q35/CEDT.cxl for new memory addresses.
> >   hw/cxl: Move the CXLState from MachineState to machine type specific
> >     state.
> >   hw/machine: Drop cxl_supported flag as no longer useful
> > 
> >  docs/system/devices/cxl.rst                 |   4 +-
> >  hw/acpi/cxl.c                               |   9 +-
> >  hw/core/machine.c                           |  28 ------
> >  hw/cxl/cxl-host-stubs.c                     |   9 +-
> >  hw/cxl/cxl-host.c                           | 100 ++++++++++++++++++--
> >  hw/i386/acpi-build.c                        |   8 +-
> >  hw/i386/pc.c                                |  31 +++---
> >  hw/pci-bridge/meson.build                   |   5 +-
> >  hw/pci-bridge/pci_expander_bridge.c         |  32 ++++---
> >  hw/pci-bridge/pci_expander_bridge_stubs.c   |  14 +++
> >  include/hw/acpi/cxl.h                       |   5 +-
> >  include/hw/boards.h                         |   3 +-
> >  include/hw/cxl/cxl.h                        |   9 +-
> >  include/hw/cxl/cxl_host.h                   |  23 +++++
> >  include/hw/i386/pc.h                        |   2 +
> >  include/hw/pci-bridge/pci_expander_bridge.h |  12 +++
> >  qapi/machine.json                           |  13 +++
> >  softmmu/vl.c                                |  46 ---------
> >  tests/data/acpi/q35/CEDT.cxl                | Bin 184 -> 184 bytes
> >  tests/qtest/bios-tables-test.c              |   4 +-
> >  tests/qtest/cxl-test.c                      |   4 +-
> >  21 files changed, 222 insertions(+), 139 deletions(-)
> >  create mode 100644 hw/pci-bridge/pci_expander_bridge_stubs.c
> >  create mode 100644 include/hw/cxl/cxl_host.h
> >  create mode 100644 include/hw/pci-bridge/pci_expander_bridge.h
> > 
> > -- 
> > 2.32.0
> >   
>