Re: [PATCH] hw/pci-bridge/pci_expander_bridge: Fix HDM passthrough condition

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 29 Apr 2025 01:11:01 +0000
"Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> wrote:

> Hi Jonathan,
> 
> 
> I apologize for the delayed response; I have just returned from vacation.
> 
> 
> On 16/04/2025 00:47, Jonathan Cameron wrote:
> > On Mon, 7 Apr 2025 02:59:20 +0000
> > "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> wrote:
> >   
> >> Ping  
> > 
> > Sorry, I wrote half a reply but then lost it before sending
> > - was still in my drafts :(
> >   
> >>
> >> Only if (dsp_count==1 && hdm_for_passthrough==true), the QEMU shouldn't implement
> >> the HDM decodder for the Host-bridge.  
> > 
> > HDM for pass through means that we do have decoders even for pass through
> > not that we do not.  The name could be better and the code flow
> > perhaps simpler.  
> 
> I got you, I did admit that I misunderstood it before.
> the word *hdm* in hdm_for_passthrough means "Host-managed Device Memory" which was missing *Decoders*
> So it though of it was an option to enable the pass through feature

Good point. That parameter name is less than ideal but we are stuck with it :(

> 
> Per CXL SPEC "8.2.4.20 CXL HDM Decoder Capability Structure",
> I read it as "zero capability structure instance is optional, so it should disable by default",
> while the QEMU implemented it as enabled by default.

It is optional whether to have the structure in this case, but not whether to program it if
it is there.  The spec talks about when you must have it (multiple downstream targets).

There is no case of disabled by default.  A host may or may not implement it in the laid
out cases.

> 
> This annoyed me when I tried to implement provisioning HDM decoders(the guest will construct
> CXL region by them) in QEMU.

Faking a firmware configured setup?

Jonathan

> 
> Thanks
> Zhijian
> 
> 
> >   
> >>
> >> But previous code didn't follow this.  
> >   
> >> Thanks
> >> Zhijian
> >>
> >> On 23/03/2025 16:04, Li Zhijian wrote:  
> >>> Reverse the logical condition for HDM passthrough support in
> >>> pci_expander_bridge. This patch ensures the HDM passthrough condition
> >>> is evaluated only when hdm_for_passthrough is set to true, aligning
> >>> behavior with intended semantics and comments.
> >>>
> >>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> >>> ---
> >>>
> >>> This change corrects what appears to be a previous mistake in logic
> >>> regarding HDM passthrough conditions.
> >>> ---
> >>>    hw/pci-bridge/pci_expander_bridge.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> >>> index 3396ab4bdd..25f8922d76 100644
> >>> --- a/hw/pci-bridge/pci_expander_bridge.c
> >>> +++ b/hw/pci-bridge/pci_expander_bridge.c
> >>> @@ -307,7 +307,7 @@ static void pxb_cxl_dev_reset(DeviceState *dev)
> >>>         * The CXL specification allows for host bridges with no HDM decoders
> >>>         * if they only have a single root port.
> >>>         */
> >>> -    if (!PXB_CXL_DEV(dev)->hdm_for_passthrough) {  
> > 
> > This makes us only check the number of ports if we are not
> > providing hdm decoders for passthrough ports.
> > 
> > If we are providing HDM Decoders anyway we don't care how many
> > ports there are so the 0 value is fine.
> >   
> >>> +    if (PXB_CXL_DEV(dev)->hdm_for_passthrough) {
> >>>            dsp_count = pcie_count_ds_ports(hb->bus);
> >>>        }
> >>>        /* Initial reset will have 0 dsp so wait until > 0 *  
> >