hw/pci-bridge/pci_expander_bridge: Fix HDM passthrough condition

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

Posted by Li Zhijian via 10 months, 3 weeks ago

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) {
+    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 */
-- 
2.41.0

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

Posted by Zhijian Li (Fujitsu) via 10 months, 1 week ago

Ping

Only if (dsp_count==1 && hdm_for_passthrough==true), the QEMU shouldn't implement
the HDM decodder for the Host-bridge.

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) {
> +    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 */

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

Posted by Jonathan Cameron via 10 months ago

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.

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

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

Posted by Zhijian Li (Fujitsu) via 9 months, 2 weeks ago

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

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.

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

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

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

Posted by Jonathan Cameron via 9 months, 2 weeks ago

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