On Thu, 12 Dec 2024 05:04:21 -0800
Zhi Wang <z...@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 <z...@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);
>
