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

[Dave Jiang <dave.jiang@intel.com>]

On 2/21/23 9:45 AM, Jonathan Cameron wrote:
> 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.

Ok, then your patch looks reasonable to me.

> 
> 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];
>>>    };
>