Re: [PATCH 2/3] hw/cxl: introduce cxl_component_update_dvsec()

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Thu, 12 Dec 2024 05:04:21 -0800
Zhi Wang <zhiw@nvidia.com> wrote:

> There are many DVSEC registers in the PCI configuration space that are
> configurable. E.g. DVS control. They are configured and initalized in
> cxl_component_create_dvsec(). When the virtual machine reboots, the
> reset callback in the emulation of the emulated CXL device resets the
> device states back to default states.
> 
> So far, there is no decent approach to reset the values of CXL DVSEC
> registers in the PCI configuation space one for all. Without reseting
> the values of CXL DVSEC registers, the CXL type-2 driver failing to
> claim the endpoint:
> 
> - DVS_CONTROL.MEM_ENABLE is left to be 1 across the system reboot.
> - Type-2 driver loads.
> - In the endpoint probe, the kernel CXL core sees the
>   DVS_CONTROL.MEM_ENABLE is set.
> - The kernel CXL core wrongly thinks the HDM decoder is pre-configured
>   by BIOS/UEFI.
> - The kernel CXL core uses the garbage in the HDM decoder registers and
>   fails:
> 
> [   74.586911] cxl_accel_vfio_pci 0000:0d:00.0: Range register decodes
> outside platform defined CXL ranges.
> [   74.588585] cxl_mem mem0: endpoint2 failed probe
> [   74.589478] cxl_accel_vfio_pci 0000:0d:00.0: Fail to acquire CXL
> endpoint
> [   74.591944] pcieport 0000:0c:00.0: unlocked secondary bus reset via:
> pciehp_reset_slot+0xa8/0x150
> 
> Introduce cxl_component_update_dvsec() for the emulation of CXL devices
> to reset the CXL DVSEC registers in the PCI configuration space.

We know there are issues with this reset path for the type 3 devices.
I'd be keen to see that fixed up using a mechanism like this then we can
build on top of it for type2 support.  I'm not convinced that a generic
solution like this makes sense rather than a specific reset of registers
as appropriate which allows us to carefully preserve the sticky bits.
Reset for the type 3 has proved a little tricky to fix in the past and
wasn't really a priority.  Given we have to do it for type 2 I'd like
to fix it up for both.

Jonathan

> 
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> ---
>  hw/cxl/cxl-component-utils.c   | 36 ++++++++++++++++++++++++++++------
>  include/hw/cxl/cxl_component.h |  3 +++
>  2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index aa5fb20d25..355103d165 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -365,9 +365,13 @@ void cxl_component_register_init_common(uint32_t *reg_state,
>   * Helper to creates a DVSEC header for a CXL entity. The caller is responsible
>   * for tracking the valid offset.
>   *
> - * This function will build the DVSEC header on behalf of the caller and then
> - * copy in the remaining data for the vendor specific bits.
> - * It will also set up appropriate write masks.
> + * This function will build the DVSEC header on behalf of the caller. It will
> + * also set up appropriate write masks.
> + *
> + * If required, it will copy in the remaining data for the vendor specific bits.
> + * Or the caller can also fill the remaining data later after the DVSEC header
> + * is built via cxl_component_update_dvsec().
> + *

Pet hate.  This blank line adds nothing so drop it.

>   */
>  void cxl_component_create_dvsec(CXLComponentState *cxl,
>                                  enum reg_type cxl_dev_type, uint16_t length,
> @@ -387,9 +391,12 @@ void cxl_component_create_dvsec(CXLComponentState *cxl,
>      pci_set_long(pdev->config + offset + PCIE_DVSEC_HEADER1_OFFSET,
>                   (length << 20) | (rev << 16) | CXL_VENDOR_ID);
>      pci_set_word(pdev->config + offset + PCIE_DVSEC_ID_OFFSET, type);
> -    memcpy(pdev->config + offset + sizeof(DVSECHeader),
> -           body + sizeof(DVSECHeader),
> -           length - sizeof(DVSECHeader));
> +
> +    if (body) {
> +        memcpy(pdev->config + offset + sizeof(DVSECHeader),
> +                body + sizeof(DVSECHeader),
> +                length - sizeof(DVSECHeader));
> +    }
>  
>      /* Configure write masks */
>      switch (type) {
> @@ -481,6 +488,23 @@ void cxl_component_create_dvsec(CXLComponentState *cxl,
>      cxl->dvsec_offset += length;
>  }
>  
> +void cxl_component_update_dvsec(CXLComponentState *cxl, uint16_t length,
> +                                uint16_t type, uint8_t *body)
> +{
> +    PCIDevice *pdev = cxl->pdev;
> +    struct Range *r;
> +
> +    assert(type < CXL20_MAX_DVSEC);
> +
> +    r = &cxl->dvsecs[type];
> +
> +    assert(range_size(r) == length);
> +
> +    memcpy(pdev->config + r->lob + sizeof(DVSECHeader),
> +           body + sizeof(DVSECHeader),
> +           length - sizeof(DVSECHeader));
> +}
> +
>  /* CXL r3.1 Section 8.2.4.20.7 CXL HDM Decoder n Control Register */
>  uint8_t cxl_interleave_ways_enc(int iw, Error **errp)
>  {
> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index abb2e874b2..30fe4bfa24 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -261,6 +261,9 @@ void cxl_component_create_dvsec(CXLComponentState *cxl_cstate,
>                                  enum reg_type cxl_dev_type, uint16_t length,
>                                  uint16_t type, uint8_t rev, uint8_t *body);
>  
> +void cxl_component_update_dvsec(CXLComponentState *cxl, uint16_t length,
> +                                uint16_t type, uint8_t *body);
> +
>  int cxl_decoder_count_enc(int count);
>  int cxl_decoder_count_dec(int enc_cnt);
>