Re: [PULL 70/73] hw/pxb-cxl: Support passthrough HDM Decoders unless overridden

[Jonathan Cameron]

On Mon, 17 Apr 2023 15:04:41 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 17/04/2023 13.29, Michael S. Tsirkin wrote:
> > On Mon, Apr 17, 2023 at 01:22:51PM +0200, Thomas Huth wrote:  
> >> On 11/04/2023 12.26, Peter Maydell wrote:  
> >>> On Wed, 8 Mar 2023 at 01:14, Michael S. Tsirkin <mst@redhat.com> wrote:  
> >>>>
> >>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>>>
> >>>> The CXL r3.0 specification allows for there to be no HDM decoders on CXL
> >>>> Host Bridges if they have only a single root port. Instead, all accesses
> >>>> directed to the host bridge (as specified in CXL Fixed Memory Windows)
> >>>> are assumed to be routed to the single root port.  
> >>>
> >>> Hi; in issue https://gitlab.com/qemu-project/qemu/-/issues/1586
> >>> it's been pointed out that this commit causes an assertion
> >>> failure during 'make check' if you configure with
> >>> --enable-qom-cast-debug. You can repro by doing that and running:
> >>>
> >>>    qemu-system-i386 -display none -machine q35,cxl=on -device 
> >>> pxb-cxl,bus=pcie.0  
> ...
> >>> The problem is here:
> >>>  
> >>>> -static void pxb_dev_reset(DeviceState *dev)
> >>>> +static void pxb_cxl_dev_reset(DeviceState *dev)  
> >>>
> >>> This function is called from  pxb_cxl_dev_realize(),
> >>> which is the realize function for TYPE_PXB_CXL_DEVICE.
> >>> That type's parent is TYPE_PCI_DEVICE.
> >>>  
> >>>>    {
> >>>>        CXLHost *cxl = PXB_CXL_DEV(dev)->cxl.cxl_host_bridge;
> >>>>        CXLComponentState *cxl_cstate = &cxl->cxl_cstate;
> >>>> +    PCIHostState *hb = PCI_HOST_BRIDGE(cxl);
> >>>>        uint32_t *reg_state = cxl_cstate->crb.cache_mem_registers;
> >>>>        uint32_t *write_msk = cxl_cstate->crb.cache_mem_regs_write_mask;
> >>>> +    int dsp_count = 0;
> >>>>
> >>>>        cxl_component_register_init_common(reg_state, write_msk, 
> >>>> CXL2_ROOT_PORT);
> >>>> -    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, 
> >>>> TARGET_COUNT, 8);
> >>>> +    /*
> >>>> +     * The CXL specification allows for host bridges with no HDM 
> >>>> decoders
> >>>> +     * if they only have a single root port.
> >>>> +     */
> >>>> +    if (!PXB_DEV(dev)->hdm_for_passthrough) {  
> >>>
> >>> However, here we try to cast the device pointer to PXB_DEV.
> >>> That is not permitted because dev is not of type TYPE_PXB_DEVICE
> >>> (either directly or as a parent class). So if you have the QOM
> >>> debugging enabled then the attempt to cast causes an assertion
> >>> failure.
> >>>  
> >>>> +        dsp_count = pcie_count_ds_ports(hb->bus);
> >>>> +    }
> >>>> +    /* Initial reset will have 0 dsp so wait until > 0 */
> >>>> +    if (dsp_count == 1) {
> >>>> +        cxl->passthrough = true;
> >>>> +        /* Set Capability ID in header to NONE */
> >>>> +        ARRAY_FIELD_DP32(reg_state, CXL_HDM_CAPABILITY_HEADER, ID, 0);
> >>>> +    } else {
> >>>> +        ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, 
> >>>> TARGET_COUNT,
> >>>> +                         8);
> >>>> +    }
> >>>>    }  
> >>>
> >>> What was the intention here with the type hierarchy?
> >>> Should TYPE_PXB_CXL_DEVICE be a subclass of TYPE_PXB_DEVICE,
> >>> or should the cxl-related functions not be trying to treat
> >>> it as a PXB device ?  
> >>
> >> Maybe we should simply revert the commit for the time being (once the 8.1
> >> cycle begins), 'til this has properly been solved, so we can enable
> >> qom-debug by default again?  
> > 
> > Can you post a revert?  
> 
> Sure, done here now:
> 20230417130037.236747-2-thuth@redhat.com/">https://lore.kernel.org/qemu-devel/20230417130037.236747-2-thuth@redhat.com/
> 

Sorry - I missed this until today.  Indeed revert makes sense. I'll chase this 
down
in the new cycle.

Jonathan

>   Thomas
>