Re: CXL type 3 which doesn't have cxl mem enabled.

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

On Tue, 26 Apr 2022 10:19:55 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Tue, Apr 26, 2022 at 10:09 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > Hi All,
> >
> > I ran into this whilst debugging why on the current QEMU code
> > we now get a probe failure for CXL mem due to the range 1 size being
> > non 0.
> >
> > The conditions for whether we have legacy ranges programmed don't
> > take into account if Mem_Enable = 1.  That is if the
> > DVSEC CXL Control Mem_Enable bit is set on the type 3 device.
> > If it's not then there is no existing user of the CXL memory
> > setup by firmware or similar so we can switch over to HDM
> > decoders and it doesn't matter what is in the range registers.
> >
> > Unfortunately the QEMU code was bringing the device up with
> > Mem_Enabled already set.  So I fixed that.  After all default
> > value of that bit should be 0.
> >
> > A few problems then showed up.
> >
> > 1. Nothing in the Linux code actually sets Mem_Enabled to 1.  

Sorry - my mistake, that should be Mem_Enable. Though that doesn't
actually clarify things much...

> 
> That's because the device is supposed to, I though, set it of its own
> accord as a result of link training. It's an RO field in the spec, so
> Linux can't set it:
> 
> 8.2.1.3.3 DVSEC Flex Bus Port Status (Offset 0Eh)
> "Mem_Enabled: When set, indicates that CXL.mem protocol operation has
> been enabled as a result of PCIe alternate protocol negotiation for
> Flex Bus."

Agreed with that statement.

Ah. Nothing like confusing register field names that are very similar....
A Mem_Enable is in DVSEC for CXL Device 8.1.3.2 and is RWL.
Just for giggles there is also a Mem_Enable in the Flex Bus Port Control
but the range registers comment isn't about that one (I hope anyway!).

The kernel currently sets the value of info->mem_enabled using
the Mem_Enable field of the DVSEC for CXL Device.
https://elixir.bootlin.com/linux/v5.18-rc4/source/drivers/cxl/pci.c#L501

So I think wrong name and wrong DVSEC for that particular condition.

Jonathan


> 
> > 2. Probing fails in mem.c as wait_for_media() checks for
> >    info->mem_enabled (cached value of this bit).
> >
> > So, dirty hack was to
> > * drop that check in wait_for_media() as media being enabled doesn't
> >   have much to do with CXL.mem protocol being enabled.  
> 
> Per above I think it does.
> 
> > * Make the check in cxl_hdm_decode_init()
> >    if (info->mem_enabled && !global_enable && info->ranges)
> > * Immediately after enabling the hdm decoder global enable,
> >   also set the Mem_Enable bit also update info->mem_enabled.
> >
> > This seems to work, but I can't help thinking I'm missing something.  
> 
> I think QEMU should just unconditionally init that value to 1.