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

[Ben Widawsky]

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.
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
>