Re: [PATCH v5 6/7] cxl/hdm: Add emulation when HDM decoders are not committed

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

On Tue, 21 Feb 2023 09:06:21 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 2/20/23 4:36 AM, Jonathan Cameron wrote:
> > On Tue, 14 Feb 2023 11:41:35 -0800
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> >> From: Dave Jiang <dave.jiang@intel.com>
> >>
> >> For the case where DVSEC range register(s) are active and HDM decoders are
> >> not committed, use RR to provide emulation. A first pass is done to note
> >> whether any decoders are committed. If there are no committed endpoint
> >> decoders, then DVSEC ranges will be used for emulation.
> >>
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > 
> > I'm confused a bit here.  I 'think' the aim should always be to use
> > HDM decoders unless they either aren't present or we know the
> > DVSEC range registers are in use.
> > 
> > The conditions in here are too broad + trip up current QEMU.
> > There is a 'bug' in QEMU as it programs the base registers in DVSEC
> > but it doesn't affect this flow (and shouldn't).
> > 
> > I'm not sure how you are detecting 'active' for the DVSEC range
> > registers as described in this patch description. As far as I can
> > tell there is no way to work that out other than if the top level
> > CXL.mem_enabled == true;  The various active bits in the range registers
> > refer to whether the memory behind them is ready to use, not anything
> > to do whether the range registers are setup correctly and 'turned on'.  
> 
> cxl.mem_enabled && range and size programmed in range register && no HDM 
> decoders programmed. Does that imply active ranges via DVSEC range 
> registers?

Range size isn't programmed. That's RO provided by the hardware so no use to
detect anything.  Base address isn't useful either as in theory you
could have a CFMWS at address 0.

If HDM decoders are enabled, then definitely don't want to use range
registers - so that check is good.

The cxl.mem_enabled check is missing / it is enabled before this
point in the driver probe. 

Jonathan


> 
> > 
> > If CXL.mem is not enabled, the memory is definitely not
> > in use. Equation 8-2
> > 
> > Memory_active AND CXL Mem_Enable=1
> > 
> > Now the snag is that CXL mem enabled has been set already in
> > cxl_hdm_decode_init() called from cxl_endpoint_port_probe()
> > 
> > So the hack I'm carrying is to stash if 'it was not enabled when
> > we would otherwise turn it on'.  If that condition is true and
> > there are HDM decoders, we should use them.  Sometimes it feels
> > like there is no right order possible for enabling all of a CXL
> > device!
> > 
> > Note the QEMU device here is heaving like any unconfigured freshly
> > reset type 3 device - or a hotplugged one.
> > 
> > 
> >  From 5c3e6c5c5c6c37dd0e614273113daf1ff2487e53 Mon Sep 17 00:00:00 2001
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Date: Mon, 20 Feb 2023 11:24:34 +0000
> > Subject: [PATCH] hack
> > 
> > ---
> >   drivers/cxl/core/hdm.c | 11 ++++++++---
> >   drivers/cxl/core/pci.c |  1 +
> >   drivers/cxl/cxl.h      |  2 ++
> >   3 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 45deda18ed32..a2b209cf9856 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -739,7 +739,8 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
> >   	return 0;
> >   }
> >   
> > -static bool should_emulate_decoders(struct cxl_port *port)
> > +static bool should_emulate_decoders(struct cxl_port *port,
> > +				    struct cxl_endpoint_dvsec_info *info)
> >   {
> >   	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> >   	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> > @@ -752,6 +753,9 @@ static bool should_emulate_decoders(struct cxl_port *port)
> >   	if (!hdm)
> >   		return true;
> >   
> > +	if (info->was_not_enabled_at_probe) {
> > +		return false;
> > +	}
> >   	/*
> >   	 * If any decoders are committed already, there should not be any
> >   	 * emulated DVSEC decoders.
> > @@ -780,7 +784,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >   		unsigned char target_id[8];
> >   	} target_list;
> >   
> > -	if (should_emulate_decoders(port))
> > +	if (should_emulate_decoders(port, info))
> >   		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
> >   
> >   	if (is_endpoint_decoder(&cxld->dev))
> > @@ -806,7 +810,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >   		.end = base + size - 1,
> >   	};
> >   
> > -	if (cxled && !committed && range_len(&info->dvsec_range[which]))
> > +	if ((!info || !info->was_not_enabled_at_probe) && cxled && !committed &&
> > +	    range_len(&info->dvsec_range[which]))
> >   		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
> >   
> >   	/* decoders are enabled if committed */
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 7328a2552411..a08c35216ad8 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -377,6 +377,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> >   	if (hdm)
> >   		global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> >   
> > +	info->was_not_enabled_at_probe = !info->mem_enabled;
> >   	/*
> >   	 * If the HDM Decoder Capability is already enabled then assume
> >   	 * that some other agent like platform firmware set it up.
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index d853a0238ad7..c32a9f9438e8 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -696,11 +696,13 @@ int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint)
> >   /**
> >    * struct cxl_endpoint_dvsec_info - Cached DVSEC info
> >    * @mem_enabled: cached value of mem_enabled in the DVSEC, PCIE_DEVICE
> > + * @was_not_enabled_at_probe: hack
> >    * @ranges: Number of active HDM ranges this device uses.
> >    * @dvsec_range: cached attributes of the ranges in the DVSEC, PCIE_DEVICE
> >    */
> >   struct cxl_endpoint_dvsec_info {
> >   	bool mem_enabled;
> > +	bool was_not_enabled_at_probe;
> >   	int ranges;
> >   	struct range dvsec_range[2];
> >   };