[PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Fan Ni]

Add COMMIT field check aside with existing COMMITTED field check during
hdm decoder initialization to avoid a system crash during module removal
after destroying a region which leaves the COMMIT field being reset while
the COMMITTED field still being set.

In current kernel implementation, when destroying a region (cxl
destroy-region),the decoders associated to the region will be reset
as that in cxl_decoder_reset, where the COMMIT field will be reset.
However, resetting COMMIT field will not automatically reset the
COMMITTED field, causing a situation where COMMIT is reset (0) while
COMMITTED is set (1) after the region is destroyed. Later, when
init_hdm_decoder is called (during modprobe), current code only check
the COMMITTED to decide whether the decoder is enabled or not. Since
the COMMITTED will be 1 and the code treats the decoder as enabled,
which will cause unexpected behaviour.

Before the fix, a system crash was observed when performing following
steps:
1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
3. cxl destroy-region region0 -f
4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
"no CXL window for range 0x0:0xffffffffffffffff" error message)
6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
crash at cxl_dpa_release due to dpa_res has been freed when destroying
the region).

The patch fixed the above issue, and is tested based on follow patch series:

[PATCH 00/18] CXL RAM and the 'Soft Reserved' => 'System RAM' default
Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com

Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
 drivers/cxl/core/hdm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 80eccae6ba9e..6cf854c949f0 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 	struct cxl_endpoint_decoder *cxled = NULL;
 	u64 size, base, skip, dpa_size;
 	bool committed;
+	bool should_commit;
 	u32 remainder;
 	int i, rc;
 	u32 ctrl;
@@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
 	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
 	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
+	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
 	cxld->commit = cxl_decoder_commit;
 	cxld->reset = cxl_decoder_reset;
 
-	if (!committed)
+	if (!should_commit || !committed)
 		size = 0;
 	if (base == U64_MAX || size == U64_MAX) {
 		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
@@ -727,7 +729,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 	};
 
 	/* decoders are enabled if committed */
-	if (committed) {
+	if (should_commit && committed) {
 		cxld->flags |= CXL_DECODER_F_ENABLE;
 		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
 			cxld->flags |= CXL_DECODER_F_LOCK;
@@ -772,7 +774,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		return 0;
 	}
 
-	if (!committed)
+	if (!should_commit || !committed)
 		return 0;
 
 	dpa_size = div_u64_rem(size, cxld->interleave_ways, &remainder);
-- 
2.25.1

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Dave Jiang]

On 2/28/23 3:40 PM, Fan Ni wrote:
> Add COMMIT field check aside with existing COMMITTED field check during
> hdm decoder initialization to avoid a system crash during module removal
> after destroying a region which leaves the COMMIT field being reset while
> the COMMITTED field still being set.

Hi Fan. Are you seeing this issue on qemu emulation or hardware? The 
situation does not make sense to me. If we clear the COMMIT bit, then 
the COMMITTED bit should be cleared by the hardware shortly after right? 
Otherwise, how would one reprogram the decoder if the decoder is 
indicating to be active?

DJ

> 
> In current kernel implementation, when destroying a region (cxl
> destroy-region),the decoders associated to the region will be reset
> as that in cxl_decoder_reset, where the COMMIT field will be reset.
> However, resetting COMMIT field will not automatically reset the
> COMMITTED field, causing a situation where COMMIT is reset (0) while
> COMMITTED is set (1) after the region is destroyed. Later, when
> init_hdm_decoder is called (during modprobe), current code only check
> the COMMITTED to decide whether the decoder is enabled or not. Since
> the COMMITTED will be 1 and the code treats the decoder as enabled,
> which will cause unexpected behaviour.
> 
> Before the fix, a system crash was observed when performing following
> steps:
> 1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
> 2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
> 3. cxl destroy-region region0 -f
> 4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
> 5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
> "no CXL window for range 0x0:0xffffffffffffffff" error message)
> 6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
> crash at cxl_dpa_release due to dpa_res has been freed when destroying
> the region).
> 
> The patch fixed the above issue, and is tested based on follow patch series:
> 
> [PATCH 00/18] CXL RAM and the 'Soft Reserved' => 'System RAM' default
> Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com
> 
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> ---
>   drivers/cxl/core/hdm.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 80eccae6ba9e..6cf854c949f0 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   	struct cxl_endpoint_decoder *cxled = NULL;
>   	u64 size, base, skip, dpa_size;
>   	bool committed;
> +	bool should_commit;
>   	u32 remainder;
>   	int i, rc;
>   	u32 ctrl;
> @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
>   	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
>   	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
>   	cxld->commit = cxl_decoder_commit;
>   	cxld->reset = cxl_decoder_reset;
>   
> -	if (!committed)
> +	if (!should_commit || !committed)
>   		size = 0;
>   	if (base == U64_MAX || size == U64_MAX) {
>   		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
> @@ -727,7 +729,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   	};
>   
>   	/* decoders are enabled if committed */
> -	if (committed) {
> +	if (should_commit && committed) {
>   		cxld->flags |= CXL_DECODER_F_ENABLE;
>   		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
>   			cxld->flags |= CXL_DECODER_F_LOCK;
> @@ -772,7 +774,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   		return 0;
>   	}
>   
> -	if (!committed)
> +	if (!should_commit || !committed)
>   		return 0;
>   
>   	dpa_size = div_u64_rem(size, cxld->interleave_ways, &remainder);

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Fan Ni]

On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:
> 
Hi Dave,
Thanks for looking into this.
> 
> On 2/28/23 3:40 PM, Fan Ni wrote:
> > Add COMMIT field check aside with existing COMMITTED field check during
> > hdm decoder initialization to avoid a system crash during module removal
> > after destroying a region which leaves the COMMIT field being reset while
> > the COMMITTED field still being set.
> 
> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The
I run into the issue with qemu emulation.
> situation does not make sense to me. If we clear the COMMIT bit, then the
> COMMITTED bit should be cleared by the hardware shortly after right?

From the spec, I cannot find any statement saying clearing the COMMIT bit
will automatically clear the COMMITTED. If I have not missed the statement in
the spec, I assume we should not make the assumption that it will be
cleared automatically for real hardware. But you may be right, leaving the
COMMITTED bit set can potentially cause some issue? Need to check more.

Fan

> Otherwise, how would one reprogram the decoder if the decoder is indicating
> to be active?
> 
> DJ
> 
> > 
> > In current kernel implementation, when destroying a region (cxl
> > destroy-region),the decoders associated to the region will be reset
> > as that in cxl_decoder_reset, where the COMMIT field will be reset.
> > However, resetting COMMIT field will not automatically reset the
> > COMMITTED field, causing a situation where COMMIT is reset (0) while
> > COMMITTED is set (1) after the region is destroyed. Later, when
> > init_hdm_decoder is called (during modprobe), current code only check
> > the COMMITTED to decide whether the decoder is enabled or not. Since
> > the COMMITTED will be 1 and the code treats the decoder as enabled,
> > which will cause unexpected behaviour.
> > 
> > Before the fix, a system crash was observed when performing following
> > steps:
> > 1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
> > 2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
> > 3. cxl destroy-region region0 -f
> > 4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
> > 5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
> > "no CXL window for range 0x0:0xffffffffffffffff" error message)
> > 6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
> > crash at cxl_dpa_release due to dpa_res has been freed when destroying
> > the region).
> > 
> > The patch fixed the above issue, and is tested based on follow patch series:
> > 
> > [PATCH 00/18] CXL RAM and the 'Soft Reserved' => 'System RAM' default
> > Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> >   drivers/cxl/core/hdm.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 80eccae6ba9e..6cf854c949f0 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >   	struct cxl_endpoint_decoder *cxled = NULL;
> >   	u64 size, base, skip, dpa_size;
> >   	bool committed;
> > +	bool should_commit;
> >   	u32 remainder;
> >   	int i, rc;
> >   	u32 ctrl;
> > @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >   	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> >   	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> >   	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> > +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
> >   	cxld->commit = cxl_decoder_commit;
> >   	cxld->reset = cxl_decoder_reset;
> > -	if (!committed)
> > +	if (!should_commit || !committed)
> >   		size = 0;
> >   	if (base == U64_MAX || size == U64_MAX) {
> >   		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
> > @@ -727,7 +729,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >   	};
> >   	/* decoders are enabled if committed */
> > -	if (committed) {
> > +	if (should_commit && committed) {
> >   		cxld->flags |= CXL_DECODER_F_ENABLE;
> >   		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> >   			cxld->flags |= CXL_DECODER_F_LOCK;
> > @@ -772,7 +774,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >   		return 0;
> >   	}
> > -	if (!committed)
> > +	if (!should_commit || !committed)
> >   		return 0;
> >   	dpa_size = div_u64_rem(size, cxld->interleave_ways, &remainder);

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Dave Jiang]

On 3/1/23 11:23 PM, Fan Ni wrote:
> On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:
>>
> Hi Dave,
> Thanks for looking into this.
>>
>> On 2/28/23 3:40 PM, Fan Ni wrote:
>>> Add COMMIT field check aside with existing COMMITTED field check during
>>> hdm decoder initialization to avoid a system crash during module removal
>>> after destroying a region which leaves the COMMIT field being reset while
>>> the COMMITTED field still being set.
>>
>> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The
> I run into the issue with qemu emulation.
>> situation does not make sense to me. If we clear the COMMIT bit, then the
>> COMMITTED bit should be cleared by the hardware shortly after right?
> 
>  From the spec, I cannot find any statement saying clearing the COMMIT bit
> will automatically clear the COMMITTED. If I have not missed the statement in
> the spec, I assume we should not make the assumption that it will be
> cleared automatically for real hardware. But you may be right, leaving the
> COMMITTED bit set can potentially cause some issue? Need to check more.

I have not been able to find direct verbiage that indicates this either. 
However, logically it would make sense. Otherwise, the COMMITTED field 
never clears and prevents reprogramming of the HDM decoders. The current 
QEMU implementation is creating a situation where the HDM decoder is 
always active after COMMIT bit is set the first time, regardless whether 
COMMIT field has been cleared later on during a teardown. It does sound 
like a bug with QEMU emulation currently.

DJ

> 
> Fan
> 
>> Otherwise, how would one reprogram the decoder if the decoder is indicating
>> to be active?
>>
>> DJ
>>
>>>
>>> In current kernel implementation, when destroying a region (cxl
>>> destroy-region),the decoders associated to the region will be reset
>>> as that in cxl_decoder_reset, where the COMMIT field will be reset.
>>> However, resetting COMMIT field will not automatically reset the
>>> COMMITTED field, causing a situation where COMMIT is reset (0) while
>>> COMMITTED is set (1) after the region is destroyed. Later, when
>>> init_hdm_decoder is called (during modprobe), current code only check
>>> the COMMITTED to decide whether the decoder is enabled or not. Since
>>> the COMMITTED will be 1 and the code treats the decoder as enabled,
>>> which will cause unexpected behaviour.
>>>
>>> Before the fix, a system crash was observed when performing following
>>> steps:
>>> 1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
>>> 2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
>>> 3. cxl destroy-region region0 -f
>>> 4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
>>> 5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
>>> "no CXL window for range 0x0:0xffffffffffffffff" error message)
>>> 6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
>>> crash at cxl_dpa_release due to dpa_res has been freed when destroying
>>> the region).
>>>
>>> The patch fixed the above issue, and is tested based on follow patch series:
>>>
>>> [PATCH 00/18] CXL RAM and the 'Soft Reserved' => 'System RAM' default
>>> Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com
>>>
>>> Signed-off-by: Fan Ni <fan.ni@samsung.com>
>>> ---
>>>    drivers/cxl/core/hdm.c | 8 +++++---
>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>> index 80eccae6ba9e..6cf854c949f0 100644
>>> --- a/drivers/cxl/core/hdm.c
>>> +++ b/drivers/cxl/core/hdm.c
>>> @@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>>    	struct cxl_endpoint_decoder *cxled = NULL;
>>>    	u64 size, base, skip, dpa_size;
>>>    	bool committed;
>>> +	bool should_commit;
>>>    	u32 remainder;
>>>    	int i, rc;
>>>    	u32 ctrl;
>>> @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>>    	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
>>>    	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
>>>    	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
>>> +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
>>>    	cxld->commit = cxl_decoder_commit;
>>>    	cxld->reset = cxl_decoder_reset;
>>> -	if (!committed)
>>> +	if (!should_commit || !committed)
>>>    		size = 0;
>>>    	if (base == U64_MAX || size == U64_MAX) {
>>>    		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
>>> @@ -727,7 +729,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>>    	};
>>>    	/* decoders are enabled if committed */
>>> -	if (committed) {
>>> +	if (should_commit && committed) {
>>>    		cxld->flags |= CXL_DECODER_F_ENABLE;
>>>    		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
>>>    			cxld->flags |= CXL_DECODER_F_LOCK;
>>> @@ -772,7 +774,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>>    		return 0;
>>>    	}
>>> -	if (!committed)
>>> +	if (!should_commit || !committed)
>>>    		return 0;
>>>    	dpa_size = div_u64_rem(size, cxld->interleave_ways, &remainder);

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Davidlohr Bueso]

On Thu, 02 Mar 2023, Dave Jiang wrote:

> It does sound like a bug with QEMU emulation currently.

Agreed, but still, crashing the kernel is always a no no - like with
the passthrough decoder situation we had.

Thanks,
Davidlohr

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Dave Jiang]

On 3/2/23 9:28 AM, Davidlohr Bueso wrote:
> On Thu, 02 Mar 2023, Dave Jiang wrote:
> 
>> It does sound like a bug with QEMU emulation currently.
> 
> Agreed, but still, crashing the kernel is always a no no - like with
> the passthrough decoder situation we had.

I wonder if we are missing a check in cxl_decoder_commit() to exit if 
COMMITTED bit is set in register. The current code just blindly start 
programming the registers.

> 
> Thanks,
> Davidlohr

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Jonathan Cameron]

On Thu, 2 Mar 2023 08:36:59 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 3/1/23 11:23 PM, Fan Ni wrote:
> > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:  
> >>  
> > Hi Dave,
> > Thanks for looking into this.  
> >>
> >> On 2/28/23 3:40 PM, Fan Ni wrote:  
> >>> Add COMMIT field check aside with existing COMMITTED field check during
> >>> hdm decoder initialization to avoid a system crash during module removal
> >>> after destroying a region which leaves the COMMIT field being reset while
> >>> the COMMITTED field still being set.  
> >>
> >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The  
> > I run into the issue with qemu emulation.  
> >> situation does not make sense to me. If we clear the COMMIT bit, then the
> >> COMMITTED bit should be cleared by the hardware shortly after right?  
> > 
> >  From the spec, I cannot find any statement saying clearing the COMMIT bit
> > will automatically clear the COMMITTED. If I have not missed the statement in
> > the spec, I assume we should not make the assumption that it will be
> > cleared automatically for real hardware. But you may be right, leaving the
> > COMMITTED bit set can potentially cause some issue? Need to check more.  
> 
> I have not been able to find direct verbiage that indicates this either. 
> However, logically it would make sense. Otherwise, the COMMITTED field 
> never clears and prevents reprogramming of the HDM decoders. The current 
> QEMU implementation is creating a situation where the HDM decoder is 
> always active after COMMIT bit is set the first time, regardless whether 
> COMMIT field has been cleared later on during a teardown. It does sound 
> like a bug with QEMU emulation currently.

I agree that one sane interpretation is that unsetting commit should result in
the decoder being deactivated and hence the commit bit dropping.  However
I'm not sure that's the only sane interpretation.

There is no verbage that I'm aware of that says the committed bit being
set means that the current register values are in use.  It simply says that
when the commit bit was set, the HDM decoder was successfully committed
(using registers as set at that time).  There is a specific statement about
not changing the registers whilst checks are in progress, but those checks
are only required if lock on commit is set, so it doesn't cover this case.

Wonderfully there isn't actually anything says what a commit transition to 0
means.  Does that result in the decoder become uncommitted, or does that only
happen when the next 0 to 1 transition happens?

The only stuff we have is what happens when lock on commit = 1, which isn't
the case here.

So is there another valid implementation? I think yes.
In some implementations, there will be a complex state machine that is
triggered when commit is set.  That will then write some entirely invisible
internal state for decode logic based on the contents of the registers.
As such, once it's set committed, it typically won't look at the registers
again until another commit 0->1 transition happens.  At that point the
committed bit drops and raised again once the commit state machine finishes
(given QEMU doesn't emulate that delay the upshot is if you set commit then
check committed it will be set ;)

In that implementation the commit 1->0 transition is an irrelevance and
it won't change the committed bit state.

So whilst the QEMU code is doing the less obvious implementation, I think
the spec still allows it.  I don't mind QEMU changing to the more obvious
one though if someone wants to send a patch.

Jonathan

>
> DJ
> 
> > 
> > Fan
> >   
> >> Otherwise, how would one reprogram the decoder if the decoder is indicating
> >> to be active?
> >>
> >> DJ
> >>  
> >>>
> >>> In current kernel implementation, when destroying a region (cxl
> >>> destroy-region),the decoders associated to the region will be reset
> >>> as that in cxl_decoder_reset, where the COMMIT field will be reset.
> >>> However, resetting COMMIT field will not automatically reset the
> >>> COMMITTED field, causing a situation where COMMIT is reset (0) while
> >>> COMMITTED is set (1) after the region is destroyed. Later, when
> >>> init_hdm_decoder is called (during modprobe), current code only check
> >>> the COMMITTED to decide whether the decoder is enabled or not. Since
> >>> the COMMITTED will be 1 and the code treats the decoder as enabled,
> >>> which will cause unexpected behaviour.
> >>>
> >>> Before the fix, a system crash was observed when performing following
> >>> steps:
> >>> 1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
> >>> 2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
> >>> 3. cxl destroy-region region0 -f
> >>> 4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
> >>> 5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
> >>> "no CXL window for range 0x0:0xffffffffffffffff" error message)
> >>> 6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
> >>> crash at cxl_dpa_release due to dpa_res has been freed when destroying
> >>> the region).
> >>>
> >>> The patch fixed the above issue, and is tested based on follow patch series:
> >>>
> >>> [PATCH 00/18] CXL RAM and the 'Soft Reserved' => 'System RAM' default
> >>> Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com
> >>>
> >>> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> >>> ---
> >>>    drivers/cxl/core/hdm.c | 8 +++++---
> >>>    1 file changed, 5 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> >>> index 80eccae6ba9e..6cf854c949f0 100644
> >>> --- a/drivers/cxl/core/hdm.c
> >>> +++ b/drivers/cxl/core/hdm.c
> >>> @@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >>>    	struct cxl_endpoint_decoder *cxled = NULL;
> >>>    	u64 size, base, skip, dpa_size;
> >>>    	bool committed;
> >>> +	bool should_commit;
> >>>    	u32 remainder;
> >>>    	int i, rc;
> >>>    	u32 ctrl;
> >>> @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >>>    	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> >>>    	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> >>>    	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> >>> +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
> >>>    	cxld->commit = cxl_decoder_commit;
> >>>    	cxld->reset = cxl_decoder_reset;
> >>> -	if (!committed)
> >>> +	if (!should_commit || !committed)
> >>>    		size = 0;
> >>>    	if (base == U64_MAX || size == U64_MAX) {
> >>>    		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
> >>> @@ -727,7 +729,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >>>    	};
> >>>    	/* decoders are enabled if committed */
> >>> -	if (committed) {
> >>> +	if (should_commit && committed) {
> >>>    		cxld->flags |= CXL_DECODER_F_ENABLE;
> >>>    		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> >>>    			cxld->flags |= CXL_DECODER_F_LOCK;
> >>> @@ -772,7 +774,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >>>    		return 0;
> >>>    	}
> >>> -	if (!committed)
> >>> +	if (!should_commit || !committed)
> >>>    		return 0;
> >>>    	dpa_size = div_u64_rem(size, cxld->interleave_ways, &remainder);  

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Ira Weiny]

Jonathan Cameron wrote:
> On Thu, 2 Mar 2023 08:36:59 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
> > On 3/1/23 11:23 PM, Fan Ni wrote:
> > > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:  
> > >>  
> > > Hi Dave,
> > > Thanks for looking into this.  
> > >>
> > >> On 2/28/23 3:40 PM, Fan Ni wrote:  
> > >>> Add COMMIT field check aside with existing COMMITTED field check during
> > >>> hdm decoder initialization to avoid a system crash during module removal
> > >>> after destroying a region which leaves the COMMIT field being reset while
> > >>> the COMMITTED field still being set.  
> > >>
> > >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The  
> > > I run into the issue with qemu emulation.  
> > >> situation does not make sense to me. If we clear the COMMIT bit, then the
> > >> COMMITTED bit should be cleared by the hardware shortly after right?  
> > > 
> > >  From the spec, I cannot find any statement saying clearing the COMMIT bit
> > > will automatically clear the COMMITTED. If I have not missed the statement in
> > > the spec, I assume we should not make the assumption that it will be
> > > cleared automatically for real hardware. But you may be right, leaving the
> > > COMMITTED bit set can potentially cause some issue? Need to check more.  
> > 
> > I have not been able to find direct verbiage that indicates this either. 
> > However, logically it would make sense. Otherwise, the COMMITTED field 
> > never clears and prevents reprogramming of the HDM decoders. The current 
> > QEMU implementation is creating a situation where the HDM decoder is 
> > always active after COMMIT bit is set the first time, regardless whether 
> > COMMIT field has been cleared later on during a teardown. It does sound 
> > like a bug with QEMU emulation currently.
> 
> I agree that one sane interpretation is that unsetting commit should result in
> the decoder being deactivated and hence the commit bit dropping.  However
> I'm not sure that's the only sane interpretation.
> 
> There is no verbage that I'm aware of that says the committed bit being
> set means that the current register values are in use.  It simply says that
> when the commit bit was set, the HDM decoder was successfully committed
> (using registers as set at that time).  There is a specific statement about
> not changing the registers whilst checks are in progress, but those checks
> are only required if lock on commit is set, so it doesn't cover this case.
> 
> Wonderfully there isn't actually anything says what a commit transition to 0
> means.  Does that result in the decoder become uncommitted, or does that only
> happen when the next 0 to 1 transition happens?
> 
> The only stuff we have is what happens when lock on commit = 1, which isn't
> the case here.
> 
> So is there another valid implementation? I think yes.
> In some implementations, there will be a complex state machine that is
> triggered when commit is set.  That will then write some entirely invisible
> internal state for decode logic based on the contents of the registers.
> As such, once it's set committed, it typically won't look at the registers
> again until another commit 0->1 transition happens.
> At that point the
> committed bit drops and raised again once the commit state machine finishes
> (given QEMU doesn't emulate that delay the upshot is if you set commit then
> check committed it will be set ;)

I'm only barely following along so I wanted to make sure I understand...

Are you saying that at the instant commit 0->1 happens hardware will clear
commited to 0 so that software can later check for commited vs error not
commited?

Ira

> 
> In that implementation the commit 1->0 transition is an irrelevance and
> it won't change the committed bit state.
> 
> So whilst the QEMU code is doing the less obvious implementation, I think
> the spec still allows it.  I don't mind QEMU changing to the more obvious
> one though if someone wants to send a patch.
> 
> Jonathan
> 

[...]

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Fan Ni]

On Fri, Mar 03, 2023 at 02:36:05PM +0000, Jonathan Cameron wrote:

> On Thu, 2 Mar 2023 08:36:59 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
> > On 3/1/23 11:23 PM, Fan Ni wrote:
> > > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:  
> > >>  
> > > Hi Dave,
> > > Thanks for looking into this.  
> > >>
> > >> On 2/28/23 3:40 PM, Fan Ni wrote:  
> > >>> Add COMMIT field check aside with existing COMMITTED field check during
> > >>> hdm decoder initialization to avoid a system crash during module removal
> > >>> after destroying a region which leaves the COMMIT field being reset while
> > >>> the COMMITTED field still being set.  
> > >>
> > >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The  
> > > I run into the issue with qemu emulation.  
> > >> situation does not make sense to me. If we clear the COMMIT bit, then the
> > >> COMMITTED bit should be cleared by the hardware shortly after right?  
> > > 
> > >  From the spec, I cannot find any statement saying clearing the COMMIT bit
> > > will automatically clear the COMMITTED. If I have not missed the statement in
> > > the spec, I assume we should not make the assumption that it will be
> > > cleared automatically for real hardware. But you may be right, leaving the
> > > COMMITTED bit set can potentially cause some issue? Need to check more.  
> > 
> > I have not been able to find direct verbiage that indicates this either. 
> > However, logically it would make sense. Otherwise, the COMMITTED field 
> > never clears and prevents reprogramming of the HDM decoders. The current 
> > QEMU implementation is creating a situation where the HDM decoder is 
> > always active after COMMIT bit is set the first time, regardless whether 
> > COMMIT field has been cleared later on during a teardown. It does sound 
> > like a bug with QEMU emulation currently.
> 
> I agree that one sane interpretation is that unsetting commit should result in
> the decoder being deactivated and hence the commit bit dropping.  However
> I'm not sure that's the only sane interpretation.
> 
> There is no verbage that I'm aware of that says the committed bit being
> set means that the current register values are in use.  It simply says that
> when the commit bit was set, the HDM decoder was successfully committed
> (using registers as set at that time).  There is a specific statement about
> not changing the registers whilst checks are in progress, but those checks
> are only required if lock on commit is set, so it doesn't cover this case.
> 
> Wonderfully there isn't actually anything says what a commit transition to 0
> means.  Does that result in the decoder become uncommitted, or does that only
> happen when the next 0 to 1 transition happens?
> 
> The only stuff we have is what happens when lock on commit = 1, which isn't
> the case here.
> 
> So is there another valid implementation? I think yes.
> In some implementations, there will be a complex state machine that is
> triggered when commit is set.  That will then write some entirely invisible
> internal state for decode logic based on the contents of the registers.
> As such, once it's set committed, it typically won't look at the registers
> again until another commit 0->1 transition happens.  At that point the
> committed bit drops and raised again once the commit state machine finishes
> (given QEMU doesn't emulate that delay the upshot is if you set commit then
> check committed it will be set ;)
> 
> In that implementation the commit 1->0 transition is an irrelevance and
> it won't change the committed bit state.
> 
> So whilst the QEMU code is doing the less obvious implementation, I think
> the spec still allows it.  I don't mind QEMU changing to the more obvious
> one though if someone wants to send a patch.
> 
> Jonathan
> 

In current qemu emulation, when COMMITTED bit is set when the decoder is
committed and at the same time the COMMIT field will be cleared. Does
the following fix make sense?
1. At qemu side, when the commit completes, just set the COMMITTED bit,
but leave the COMMIT bit as set, also check LOCK ON COMMIT bit,
if it is set, clear it, which will allow further reset of COMMIT bit.
2. for the kernel side, if it needs to reprogram the decoder, it needs to
check the COMMITTED bit, if it is set, then OS need to reset COMMIT bit
first, which will also clear COMMITTED bit automatically at qemu side.
3. when the OS needs to reset the decoder, it does similar thing as 2 to
reset COMMIT bit and qemu will clear COMMITTED bit.

Fan

> >
> > DJ
> > 
> > > 
> > > Fan
> > >   
> > >> Otherwise, how would one reprogram the decoder if the decoder is indicating
> > >> to be active?
> > >>
> > >> DJ
> > >>  
> > >>>
> > >>> In current kernel implementation, when destroying a region (cxl
> > >>> destroy-region),the decoders associated to the region will be reset
> > >>> as that in cxl_decoder_reset, where the COMMIT field will be reset.
> > >>> However, resetting COMMIT field will not automatically reset the
> > >>> COMMITTED field, causing a situation where COMMIT is reset (0) while
> > >>> COMMITTED is set (1) after the region is destroyed. Later, when
> > >>> init_hdm_decoder is called (during modprobe), current code only check
> > >>> the COMMITTED to decide whether the decoder is enabled or not. Since
> > >>> the COMMITTED will be 1 and the code treats the decoder as enabled,
> > >>> which will cause unexpected behaviour.
> > >>>
> > >>> Before the fix, a system crash was observed when performing following
> > >>> steps:
> > >>> 1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
> > >>> 2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
> > >>> 3. cxl destroy-region region0 -f
> > >>> 4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
> > >>> 5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
> > >>> "no CXL window for range 0x0:0xffffffffffffffff" error message)
> > >>> 6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
> > >>> crash at cxl_dpa_release due to dpa_res has been freed when destroying
> > >>> the region).
> > >>>
> > >>> The patch fixed the above issue, and is tested based on follow patch series:
> > >>>
> > >>> [PATCH 00/18] CXL RAM and the 'Soft Reserved' => 'System RAM' default
> > >>> Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com
> > >>>
> > >>> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > >>> ---
> > >>>    drivers/cxl/core/hdm.c | 8 +++++---
> > >>>    1 file changed, 5 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > >>> index 80eccae6ba9e..6cf854c949f0 100644
> > >>> --- a/drivers/cxl/core/hdm.c
> > >>> +++ b/drivers/cxl/core/hdm.c
> > >>> @@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > >>>    	struct cxl_endpoint_decoder *cxled = NULL;
> > >>>    	u64 size, base, skip, dpa_size;
> > >>>    	bool committed;
> > >>> +	bool should_commit;
> > >>>    	u32 remainder;
> > >>>    	int i, rc;
> > >>>    	u32 ctrl;
> > >>> @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > >>>    	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> > >>>    	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> > >>>    	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> > >>> +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
> > >>>    	cxld->commit = cxl_decoder_commit;
> > >>>    	cxld->reset = cxl_decoder_reset;
> > >>> -	if (!committed)
> > >>> +	if (!should_commit || !committed)
> > >>>    		size = 0;
> > >>>    	if (base == U64_MAX || size == U64_MAX) {
> > >>>    		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
> > >>> @@ -727,7 +729,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > >>>    	};
> > >>>    	/* decoders are enabled if committed */
> > >>> -	if (committed) {
> > >>> +	if (should_commit && committed) {
> > >>>    		cxld->flags |= CXL_DECODER_F_ENABLE;
> > >>>    		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> > >>>    			cxld->flags |= CXL_DECODER_F_LOCK;
> > >>> @@ -772,7 +774,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > >>>    		return 0;
> > >>>    	}
> > >>> -	if (!committed)
> > >>> +	if (!should_commit || !committed)
> > >>>    		return 0;
> > >>>    	dpa_size = div_u64_rem(size, cxld->interleave_ways, &remainder);  
> 

RE: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Dan Williams]

Fan Ni wrote:
> Add COMMIT field check aside with existing COMMITTED field check during
> hdm decoder initialization to avoid a system crash during module removal
> after destroying a region which leaves the COMMIT field being reset while
> the COMMITTED field still being set.
> 
> In current kernel implementation, when destroying a region (cxl
> destroy-region),the decoders associated to the region will be reset
> as that in cxl_decoder_reset, where the COMMIT field will be reset.
> However, resetting COMMIT field will not automatically reset the
> COMMITTED field, causing a situation where COMMIT is reset (0) while
> COMMITTED is set (1) after the region is destroyed. Later, when
> init_hdm_decoder is called (during modprobe), current code only check
> the COMMITTED to decide whether the decoder is enabled or not. Since
> the COMMITTED will be 1 and the code treats the decoder as enabled,
> which will cause unexpected behaviour.
> 
> Before the fix, a system crash was observed when performing following
> steps:
> 1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
> 2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
> 3. cxl destroy-region region0 -f
> 4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
> 5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
> "no CXL window for range 0x0:0xffffffffffffffff" error message)
> 6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
> crash at cxl_dpa_release due to dpa_res has been freed when destroying
> the region).

I think a separate fix for that crash is needed, can you send the
backtrace? I.e. I worry that crash can be triggered by other means.

> 
> The patch fixed the above issue, and is tested based on follow patch series:
> 
> [PATCH 00/18] CXL RAM and the 'Soft Reserved' => 'System RAM' default
> Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com
> 
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> ---
>  drivers/cxl/core/hdm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 80eccae6ba9e..6cf854c949f0 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	struct cxl_endpoint_decoder *cxled = NULL;
>  	u64 size, base, skip, dpa_size;
>  	bool committed;
> +	bool should_commit;
>  	u32 remainder;
>  	int i, rc;
>  	u32 ctrl;
> @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
>  	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
>  	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);

This change looks like a good idea in general given the ambiguity of
'committed'. However just combine the two checks into the @committed
variable with something like this:

commit_mask = CXL_HDM_DECODER0_CTRL_COMMITTED|CXL_HDM_DECODER0_CTRL_COMMIT;
committed = (ctrl & commit_mask) == commit_mask;

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Fan Ni]

On Fri, Mar 03, 2023 at 12:58:10PM -0800, Dan Williams wrote:

> Fan Ni wrote:
> > Add COMMIT field check aside with existing COMMITTED field check during
> > hdm decoder initialization to avoid a system crash during module removal
> > after destroying a region which leaves the COMMIT field being reset while
> > the COMMITTED field still being set.
> > 
> > In current kernel implementation, when destroying a region (cxl
> > destroy-region),the decoders associated to the region will be reset
> > as that in cxl_decoder_reset, where the COMMIT field will be reset.
> > However, resetting COMMIT field will not automatically reset the
> > COMMITTED field, causing a situation where COMMIT is reset (0) while
> > COMMITTED is set (1) after the region is destroyed. Later, when
> > init_hdm_decoder is called (during modprobe), current code only check
> > the COMMITTED to decide whether the decoder is enabled or not. Since
> > the COMMITTED will be 1 and the code treats the decoder as enabled,
> > which will cause unexpected behaviour.
> > 
> > Before the fix, a system crash was observed when performing following
> > steps:
> > 1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
> > 2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
> > 3. cxl destroy-region region0 -f
> > 4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
> > 5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
> > "no CXL window for range 0x0:0xffffffffffffffff" error message)
> > 6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
> > crash at cxl_dpa_release due to dpa_res has been freed when destroying
> > the region).
> 
> I think a separate fix for that crash is needed, can you send the
> backtrace? I.e. I worry that crash can be triggered by other means.
Hi Dan,
See backtrace below.

[  130.299394] BUG: kernel NULL pointer dereference, address: 0000000000000008
[  130.299907] #PF: supervisor read access in kernel mode
[  130.299907] #PF: error_code(0x0000) - not-present page
[  130.299907] PGD 0 P4D 0 
[  130.299907] Oops: 0000 [#1] PREEMPT SMP NOPTI
[  130.299907] CPU: 13 PID: 467 Comm: rmmod Not tainted 6.2.0-rc6-00024-g3ea761ec9dd5 #58
[  130.299907] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014
[  130.299907] RIP: 0010:__cxl_dpa_release+0x3c/0xb0 [cxl_core]
[  130.299907] Code: ff ff 48 8b 7d 40 4c 8b a8 d8 02 00 00 e8 5c a6 ff ff 4c 8b a5 28 03 00 00 48 89 c3 48 8b 85 20 03 00 00 4d 8b ad 40 03 00 00 <48> 8b 50 08 4c 8b 30 49 81 c5 90 00 00 00 4c 89 ef 48 83 c2 01 4c
[  130.299907] RSP: 0018:ffffc9000075fae0 EFLAGS: 00000246
[  130.299907] RAX: 0000000000000000 RBX: ffff88810250cc00 RCX: 0000000000000000
[  130.299907] RDX: 0000000000000001 RSI: ffff8881008d25e8 RDI: ffff88810250cc00
[  130.299907] RBP: ffff88810250d000 R08: 0000000000000001 R09: ffffffff8182b400
[  130.299907] R10: ffff888101fd7238 R11: ffff888201c1f406 R12: 0000000000000000
[  130.299907] R13: 0000000000000000 R14: ffff88810250ce90 R15: ffff88810250ce8c
[  130.299907] FS:  00007f53b3884c40(0000) GS:ffff888277d40000(0000) knlGS:0000000000000000
[  130.299907] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  130.299907] CR2: 0000000000000008 CR3: 000000010285c000 CR4: 00000000000006e0
[  130.299907] Call Trace:
[  130.299907]  <TASK>
[  130.299907]  cxl_dpa_release+0x18/0x30 [cxl_core]
[  130.299907]  release_nodes+0x40/0x70
[  130.299907]  devres_release_all+0x86/0xc0
[  130.299907]  device_unbind_cleanup+0x9/0x70
[  130.299907]  device_release_driver_internal+0xe9/0x160
[  130.299907]  bus_remove_device+0xd3/0x140
[  130.299907]  device_del+0x186/0x3d0
[  130.299907]  ? _raw_spin_unlock_irqrestore+0x16/0x30
[  130.299907]  ? devres_remove+0xcb/0xf0
[  130.299907]  device_unregister+0xe/0x60
[  130.299907]  ? __pfx_devm_action_release+0x10/0x10
[  130.299907]  devres_release+0x22/0x50
[  130.299907]  devm_release_action+0x33/0x60
[  130.299907]  ? __pfx_unregister_port+0x10/0x10 [cxl_core]
[  130.299907]  delete_endpoint+0x7a/0x80 [cxl_core]
[  130.299907]  release_nodes+0x40/0x70
[  130.299907]  devres_release_all+0x86/0xc0
[  130.299907]  device_unbind_cleanup+0x9/0x70
[  130.299907]  device_release_driver_internal+0xe9/0x160
[  130.299907]  bus_remove_device+0xd3/0x140
[  130.299907]  device_del+0x186/0x3d0
[  130.299907]  cdev_device_del+0x10/0x30
[  130.299907]  cxl_memdev_unregister+0x36/0x40 [cxl_core]
[  130.299907]  release_nodes+0x40/0x70
[  130.299907]  devres_release_all+0x86/0xc0
[  130.299907]  device_unbind_cleanup+0x9/0x70
[  130.299907]  device_release_driver_internal+0xe9/0x160
[  130.299907]  driver_detach+0x3f/0x80
[  130.299907]  bus_remove_driver+0x50/0xd0
[  130.299907]  pci_unregister_driver+0x36/0x80
[  130.299907]  __x64_sys_delete_module+0x191/0x270
[  130.299907]  ? fpregs_assert_state_consistent+0x1d/0x50
[  130.299907]  ? exit_to_user_mode_prepare+0x36/0x120
[  130.299907]  do_syscall_64+0x3b/0x90
[  130.299907]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
[  130.299907] RIP: 0033:0x7f53b3126c9b
[  130.299907] Code: 73 01 c3 48 8b 0d 95 21 0f 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 65 21 0f 00 f7 d8 64 89 01 48
[  130.299907] RSP: 002b:00007fff5a72c558 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
[  130.299907] RAX: ffffffffffffffda RBX: 000056037a16e790 RCX: 00007f53b3126c9b
[  130.299907] RDX: 000000000000000a RSI: 0000000000000800 RDI: 000056037a16e7f8
[  130.299907] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[  130.299907] R10: 00007f53b31beac0 R11: 0000000000000206 R12: 00007fff5a72c7b0
[  130.299907] R13: 000056037a16d2a0 R14: 00007fff5a72cdb7 R15: 000056037a16e790
[  130.299907]  </TASK>
[  130.299907] Modules linked in: cxl_mem cxl_pmem cxl_port cxl_pci(-) cxl_acpi cxl_core dax_pmem nd_pmem nd_btt [last unloaded: cxl_core]
[  130.299907] CR2: 0000000000000008
[  130.357813] ---[ end trace 0000000000000000 ]---
[  130.358811] RIP: 0010:__cxl_dpa_release+0x3c/0xb0 [cxl_core]
[  130.360039] Code: ff ff 48 8b 7d 40 4c 8b a8 d8 02 00 00 e8 5c a6 ff ff 4c 8b a5 28 03 00 00 48 89 c3 48 8b 85 20 03 00 00 4d 8b ad 40 03 00 00 <48> 8b 50 08 4c 8b 30 49 81 c5 90 00 00 00 4c 89 ef 48 83 c2 01 4c
[  130.363227] RSP: 0018:ffffc9000075fae0 EFLAGS: 00000246
[  130.364292] RAX: 0000000000000000 RBX: ffff88810250cc00 RCX: 0000000000000000
[  130.365400] RDX: 0000000000000001 RSI: ffff8881008d25e8 RDI: ffff88810250cc00
[  130.366645] RBP: ffff88810250d000 R08: 0000000000000001 R09: ffffffff8182b400
[  130.368025] R10: ffff888101fd7238 R11: ffff888201c1f406 R12: 0000000000000000
[  130.369337] R13: 0000000000000000 R14: ffff88810250ce90 R15: ffff88810250ce8c
[  130.370531] FS:  00007f53b3884c40(0000) GS:ffff888277d40000(0000) knlGS:0000000000000000
[  130.372515] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  130.373567] CR2: 0000000000000008 CR3: 000000010285c000 CR4: 00000000000006e0

> 
> > 
> > The patch fixed the above issue, and is tested based on follow patch series:
> > 
> > [PATCH 00/18] CXL RAM and the 'Soft Reserved' => 'System RAM' default
> > Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> >  drivers/cxl/core/hdm.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 80eccae6ba9e..6cf854c949f0 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >  	struct cxl_endpoint_decoder *cxled = NULL;
> >  	u64 size, base, skip, dpa_size;
> >  	bool committed;
> > +	bool should_commit;
> >  	u32 remainder;
> >  	int i, rc;
> >  	u32 ctrl;
> > @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >  	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> >  	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> >  	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> > +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
> 
> This change looks like a good idea in general given the ambiguity of
> 'committed'. However just combine the two checks into the @committed
> variable with something like this:
> 
> commit_mask = CXL_HDM_DECODER0_CTRL_COMMITTED|CXL_HDM_DECODER0_CTRL_COMMIT;
> committed = (ctrl & commit_mask) == commit_mask;

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Dan Williams]

Fan Ni wrote:
[..]
> > I think a separate fix for that crash is needed, can you send the
> > backtrace? I.e. I worry that crash can be triggered by other means.
> Hi Dan,
> See backtrace below.

Thanks, I'll take a look. 

[..]
> > > @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > >  	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> > >  	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> > >  	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> > > +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
> > 
> > This change looks like a good idea in general given the ambiguity of
> > 'committed'. However just combine the two checks into the @committed
> > variable with something like this:
> > 
> > commit_mask = CXL_HDM_DECODER0_CTRL_COMMITTED|CXL_HDM_DECODER0_CTRL_COMMIT;
> > committed = (ctrl & commit_mask) == commit_mask;

Did you also notice this ^^^ request for a fixed up version of the
current patch?

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Jonathan Cameron]

On Fri, 3 Mar 2023 07:57:22 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Thu, 2 Mar 2023 08:36:59 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> > > On 3/1/23 11:23 PM, Fan Ni wrote:  
> > > > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:    
> > > >>    
> > > > Hi Dave,
> > > > Thanks for looking into this.    
> > > >>
> > > >> On 2/28/23 3:40 PM, Fan Ni wrote:    
> > > >>> Add COMMIT field check aside with existing COMMITTED field check during
> > > >>> hdm decoder initialization to avoid a system crash during module removal
> > > >>> after destroying a region which leaves the COMMIT field being reset while
> > > >>> the COMMITTED field still being set.    
> > > >>
> > > >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The    
> > > > I run into the issue with qemu emulation.    
> > > >> situation does not make sense to me. If we clear the COMMIT bit, then the
> > > >> COMMITTED bit should be cleared by the hardware shortly after right?    
> > > > 
> > > >  From the spec, I cannot find any statement saying clearing the COMMIT bit
> > > > will automatically clear the COMMITTED. If I have not missed the statement in
> > > > the spec, I assume we should not make the assumption that it will be
> > > > cleared automatically for real hardware. But you may be right, leaving the
> > > > COMMITTED bit set can potentially cause some issue? Need to check more.    
> > > 
> > > I have not been able to find direct verbiage that indicates this either. 
> > > However, logically it would make sense. Otherwise, the COMMITTED field 
> > > never clears and prevents reprogramming of the HDM decoders. The current 
> > > QEMU implementation is creating a situation where the HDM decoder is 
> > > always active after COMMIT bit is set the first time, regardless whether 
> > > COMMIT field has been cleared later on during a teardown. It does sound 
> > > like a bug with QEMU emulation currently.  
> > 
> > I agree that one sane interpretation is that unsetting commit should result in
> > the decoder being deactivated and hence the commit bit dropping.  However
> > I'm not sure that's the only sane interpretation.
> > 
> > There is no verbage that I'm aware of that says the committed bit being
> > set means that the current register values are in use.  It simply says that
> > when the commit bit was set, the HDM decoder was successfully committed
> > (using registers as set at that time).  There is a specific statement about
> > not changing the registers whilst checks are in progress, but those checks
> > are only required if lock on commit is set, so it doesn't cover this case.
> > 
> > Wonderfully there isn't actually anything says what a commit transition to 0
> > means.  Does that result in the decoder become uncommitted, or does that only
> > happen when the next 0 to 1 transition happens?
> > 
> > The only stuff we have is what happens when lock on commit = 1, which isn't
> > the case here.
> > 
> > So is there another valid implementation? I think yes.
> > In some implementations, there will be a complex state machine that is
> > triggered when commit is set.  That will then write some entirely invisible
> > internal state for decode logic based on the contents of the registers.
> > As such, once it's set committed, it typically won't look at the registers
> > again until another commit 0->1 transition happens.
> > At that point the
> > committed bit drops and raised again once the commit state machine finishes
> > (given QEMU doesn't emulate that delay the upshot is if you set commit then
> > check committed it will be set ;)  
> 
> I'm only barely following along so I wanted to make sure I understand...
> 
> Are you saying that at the instant commit 0->1 happens hardware will clear
> commited to 0 so that software can later check for commited vs error not
> commited?

yup.  That's what you'd see in such an implementation.

> 
> Ira
> 
> > 
> > In that implementation the commit 1->0 transition is an irrelevance and
> > it won't change the committed bit state.
> > 
> > So whilst the QEMU code is doing the less obvious implementation, I think
> > the spec still allows it.  I don't mind QEMU changing to the more obvious
> > one though if someone wants to send a patch.
> > 
> > Jonathan
> >   
> 
> [...]
> 

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Jonathan Cameron]

On Fri, 3 Mar 2023 17:21:13 +0000
Fan Ni <fan.ni@samsung.com> wrote:

> On Fri, Mar 03, 2023 at 02:36:05PM +0000, Jonathan Cameron wrote:
> 
> > On Thu, 2 Mar 2023 08:36:59 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> > > On 3/1/23 11:23 PM, Fan Ni wrote:  
> > > > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:    
> > > >>    
> > > > Hi Dave,
> > > > Thanks for looking into this.    
> > > >>
> > > >> On 2/28/23 3:40 PM, Fan Ni wrote:    
> > > >>> Add COMMIT field check aside with existing COMMITTED field check during
> > > >>> hdm decoder initialization to avoid a system crash during module removal
> > > >>> after destroying a region which leaves the COMMIT field being reset while
> > > >>> the COMMITTED field still being set.    
> > > >>
> > > >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The    
> > > > I run into the issue with qemu emulation.    
> > > >> situation does not make sense to me. If we clear the COMMIT bit, then the
> > > >> COMMITTED bit should be cleared by the hardware shortly after right?    
> > > > 
> > > >  From the spec, I cannot find any statement saying clearing the COMMIT bit
> > > > will automatically clear the COMMITTED. If I have not missed the statement in
> > > > the spec, I assume we should not make the assumption that it will be
> > > > cleared automatically for real hardware. But you may be right, leaving the
> > > > COMMITTED bit set can potentially cause some issue? Need to check more.    
> > > 
> > > I have not been able to find direct verbiage that indicates this either. 
> > > However, logically it would make sense. Otherwise, the COMMITTED field 
> > > never clears and prevents reprogramming of the HDM decoders. The current 
> > > QEMU implementation is creating a situation where the HDM decoder is 
> > > always active after COMMIT bit is set the first time, regardless whether 
> > > COMMIT field has been cleared later on during a teardown. It does sound 
> > > like a bug with QEMU emulation currently.  
> > 
> > I agree that one sane interpretation is that unsetting commit should result in
> > the decoder being deactivated and hence the commit bit dropping.  However
> > I'm not sure that's the only sane interpretation.
> > 
> > There is no verbage that I'm aware of that says the committed bit being
> > set means that the current register values are in use.  It simply says that
> > when the commit bit was set, the HDM decoder was successfully committed
> > (using registers as set at that time).  There is a specific statement about
> > not changing the registers whilst checks are in progress, but those checks
> > are only required if lock on commit is set, so it doesn't cover this case.
> > 
> > Wonderfully there isn't actually anything says what a commit transition to 0
> > means.  Does that result in the decoder become uncommitted, or does that only
> > happen when the next 0 to 1 transition happens?
> > 
> > The only stuff we have is what happens when lock on commit = 1, which isn't
> > the case here.
> > 
> > So is there another valid implementation? I think yes.
> > In some implementations, there will be a complex state machine that is
> > triggered when commit is set.  That will then write some entirely invisible
> > internal state for decode logic based on the contents of the registers.
> > As such, once it's set committed, it typically won't look at the registers
> > again until another commit 0->1 transition happens.  At that point the
> > committed bit drops and raised again once the commit state machine finishes
> > (given QEMU doesn't emulate that delay the upshot is if you set commit then
> > check committed it will be set ;)
> > 
> > In that implementation the commit 1->0 transition is an irrelevance and
> > it won't change the committed bit state.
> > 
> > So whilst the QEMU code is doing the less obvious implementation, I think
> > the spec still allows it.  I don't mind QEMU changing to the more obvious
> > one though if someone wants to send a patch.
> > 
> > Jonathan
> >   
> 
> In current qemu emulation, when COMMITTED bit is set when the decoder is
> committed and at the same time the COMMIT field will be cleared. Does
> the following fix make sense?
> 1. At qemu side, when the commit completes, just set the COMMITTED bit,
> but leave the COMMIT bit as set, also check LOCK ON COMMIT bit,
> if it is set, clear it, which will allow further reset of COMMIT bit.

QEMU definitely can't do anything to the Commit bit, other than prevent it being
cleared if lock on commit is set.
Right now the QEMU emulation doesn't handle LOCK ON COMMIT at all.
It would be sensible to add this support, but we don't have an
open software stack that ever sets that yet so any testing is likely to be
one time only via some hacks.

> 2. for the kernel side, if it needs to reprogram the decoder, it needs to
> check the COMMITTED bit, if it is set, then OS need to reset COMMIT bit
> first, which will also clear COMMITTED bit automatically at qemu side.

Could do it that way, or simplify it by always clearing commit before setting
it to make sure the transition happens.

Looks like commit is cleared in cxl_decoder_reset() already so this may
already happen - I haven't checked the flow.

> 3. when the OS needs to reset the decoder, it does similar thing as 2 to
> reset COMMIT bit and qemu will clear COMMITTED bit.

No the point of the above argument is that the spec doesn't say anything
about when committed is cleared. 2 options.
1) Hardware clears it when commit 1->0.
2) Hardware clears it when commit 0->1

Given that spec only talks about after a commit 0->1 transition whilst commit
remains 1, the state after a commit 0->1 transition is implementation defined.

I think that closing that corner case requires a clarification to the spec.

Which leaves us with a sticky question of what to do...

> 
> Fan
> 
> > >
> > > DJ
> > >   
> > > > 
> > > > Fan
> > > >     
> > > >> Otherwise, how would one reprogram the decoder if the decoder is indicating
> > > >> to be active?
> > > >>
> > > >> DJ
> > > >>    
> > > >>>
> > > >>> In current kernel implementation, when destroying a region (cxl
> > > >>> destroy-region),the decoders associated to the region will be reset
> > > >>> as that in cxl_decoder_reset, where the COMMIT field will be reset.
> > > >>> However, resetting COMMIT field will not automatically reset the
> > > >>> COMMITTED field, causing a situation where COMMIT is reset (0) while
> > > >>> COMMITTED is set (1) after the region is destroyed. Later, when
> > > >>> init_hdm_decoder is called (during modprobe), current code only check
> > > >>> the COMMITTED to decide whether the decoder is enabled or not. Since
> > > >>> the COMMITTED will be 1 and the code treats the decoder as enabled,
> > > >>> which will cause unexpected behaviour.
> > > >>>
> > > >>> Before the fix, a system crash was observed when performing following
> > > >>> steps:
> > > >>> 1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
> > > >>> 2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
> > > >>> 3. cxl destroy-region region0 -f
> > > >>> 4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
> > > >>> 5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
> > > >>> "no CXL window for range 0x0:0xffffffffffffffff" error message)
> > > >>> 6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
> > > >>> crash at cxl_dpa_release due to dpa_res has been freed when destroying
> > > >>> the region).
> > > >>>
> > > >>> The patch fixed the above issue, and is tested based on follow patch series:
> > > >>>
> > > >>> [PATCH 00/18] CXL RAM and the 'Soft Reserved' => 'System RAM' default
> > > >>> Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com
> > > >>>
> > > >>> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > > >>> ---
> > > >>>    drivers/cxl/core/hdm.c | 8 +++++---
> > > >>>    1 file changed, 5 insertions(+), 3 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > >>> index 80eccae6ba9e..6cf854c949f0 100644
> > > >>> --- a/drivers/cxl/core/hdm.c
> > > >>> +++ b/drivers/cxl/core/hdm.c
> > > >>> @@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > > >>>    	struct cxl_endpoint_decoder *cxled = NULL;
> > > >>>    	u64 size, base, skip, dpa_size;
> > > >>>    	bool committed;
> > > >>> +	bool should_commit;
> > > >>>    	u32 remainder;
> > > >>>    	int i, rc;
> > > >>>    	u32 ctrl;
> > > >>> @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > > >>>    	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> > > >>>    	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> > > >>>    	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> > > >>> +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
> > > >>>    	cxld->commit = cxl_decoder_commit;
> > > >>>    	cxld->reset = cxl_decoder_reset;
> > > >>> -	if (!committed)
> > > >>> +	if (!should_commit || !committed)
> > > >>>    		size = 0;
> > > >>>    	if (base == U64_MAX || size == U64_MAX) {
> > > >>>    		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
> > > >>> @@ -727,7 +729,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > > >>>    	};
> > > >>>    	/* decoders are enabled if committed */
> > > >>> -	if (committed) {
> > > >>> +	if (should_commit && committed) {
> > > >>>    		cxld->flags |= CXL_DECODER_F_ENABLE;
> > > >>>    		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> > > >>>    			cxld->flags |= CXL_DECODER_F_LOCK;
> > > >>> @@ -772,7 +774,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > > >>>    		return 0;
> > > >>>    	}
> > > >>> -	if (!committed)
> > > >>> +	if (!should_commit || !committed)
> > > >>>    		return 0;
> > > >>>    	dpa_size = div_u64_rem(size, cxld->interleave_ways, &remainder);    
> >  

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Jonathan Cameron]

On Mon, 6 Mar 2023 16:04:22 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 3 Mar 2023 17:21:13 +0000
> Fan Ni <fan.ni@samsung.com> wrote:
> 
> > On Fri, Mar 03, 2023 at 02:36:05PM +0000, Jonathan Cameron wrote:
> >   
> > > On Thu, 2 Mar 2023 08:36:59 -0700
> > > Dave Jiang <dave.jiang@intel.com> wrote:
> > >     
> > > > On 3/1/23 11:23 PM, Fan Ni wrote:    
> > > > > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:      
> > > > >>      
> > > > > Hi Dave,
> > > > > Thanks for looking into this.      
> > > > >>
> > > > >> On 2/28/23 3:40 PM, Fan Ni wrote:      
> > > > >>> Add COMMIT field check aside with existing COMMITTED field check during
> > > > >>> hdm decoder initialization to avoid a system crash during module removal
> > > > >>> after destroying a region which leaves the COMMIT field being reset while
> > > > >>> the COMMITTED field still being set.      
> > > > >>
> > > > >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The      
> > > > > I run into the issue with qemu emulation.      
> > > > >> situation does not make sense to me. If we clear the COMMIT bit, then the
> > > > >> COMMITTED bit should be cleared by the hardware shortly after right?      
> > > > > 
> > > > >  From the spec, I cannot find any statement saying clearing the COMMIT bit
> > > > > will automatically clear the COMMITTED. If I have not missed the statement in
> > > > > the spec, I assume we should not make the assumption that it will be
> > > > > cleared automatically for real hardware. But you may be right, leaving the
> > > > > COMMITTED bit set can potentially cause some issue? Need to check more.      
> > > > 
> > > > I have not been able to find direct verbiage that indicates this either. 
> > > > However, logically it would make sense. Otherwise, the COMMITTED field 
> > > > never clears and prevents reprogramming of the HDM decoders. The current 
> > > > QEMU implementation is creating a situation where the HDM decoder is 
> > > > always active after COMMIT bit is set the first time, regardless whether 
> > > > COMMIT field has been cleared later on during a teardown. It does sound 
> > > > like a bug with QEMU emulation currently.    
> > > 
> > > I agree that one sane interpretation is that unsetting commit should result in
> > > the decoder being deactivated and hence the commit bit dropping.  However
> > > I'm not sure that's the only sane interpretation.
> > > 
> > > There is no verbage that I'm aware of that says the committed bit being
> > > set means that the current register values are in use.  It simply says that
> > > when the commit bit was set, the HDM decoder was successfully committed
> > > (using registers as set at that time).  There is a specific statement about
> > > not changing the registers whilst checks are in progress, but those checks
> > > are only required if lock on commit is set, so it doesn't cover this case.
> > > 
> > > Wonderfully there isn't actually anything says what a commit transition to 0
> > > means.  Does that result in the decoder become uncommitted, or does that only
> > > happen when the next 0 to 1 transition happens?
> > > 
> > > The only stuff we have is what happens when lock on commit = 1, which isn't
> > > the case here.
> > > 
> > > So is there another valid implementation? I think yes.
> > > In some implementations, there will be a complex state machine that is
> > > triggered when commit is set.  That will then write some entirely invisible
> > > internal state for decode logic based on the contents of the registers.
> > > As such, once it's set committed, it typically won't look at the registers
> > > again until another commit 0->1 transition happens.  At that point the
> > > committed bit drops and raised again once the commit state machine finishes
> > > (given QEMU doesn't emulate that delay the upshot is if you set commit then
> > > check committed it will be set ;)
> > > 
> > > In that implementation the commit 1->0 transition is an irrelevance and
> > > it won't change the committed bit state.
> > > 
> > > So whilst the QEMU code is doing the less obvious implementation, I think
> > > the spec still allows it.  I don't mind QEMU changing to the more obvious
> > > one though if someone wants to send a patch.
> > > 
> > > Jonathan
> > >     
> > 
> > In current qemu emulation, when COMMITTED bit is set when the decoder is
> > committed and at the same time the COMMIT field will be cleared. Does
> > the following fix make sense?
> > 1. At qemu side, when the commit completes, just set the COMMITTED bit,
> > but leave the COMMIT bit as set, also check LOCK ON COMMIT bit,
> > if it is set, clear it, which will allow further reset of COMMIT bit.  
> 
> QEMU definitely can't do anything to the Commit bit, other than prevent it being
> cleared if lock on commit is set.
> Right now the QEMU emulation doesn't handle LOCK ON COMMIT at all.
> It would be sensible to add this support, but we don't have an
> open software stack that ever sets that yet so any testing is likely to be
> one time only via some hacks.
> 
> > 2. for the kernel side, if it needs to reprogram the decoder, it needs to
> > check the COMMITTED bit, if it is set, then OS need to reset COMMIT bit
> > first, which will also clear COMMITTED bit automatically at qemu side.  
> 
> Could do it that way, or simplify it by always clearing commit before setting
> it to make sure the transition happens.
> 
> Looks like commit is cleared in cxl_decoder_reset() already so this may
> already happen - I haven't checked the flow.
> 
> > 3. when the OS needs to reset the decoder, it does similar thing as 2 to
> > reset COMMIT bit and qemu will clear COMMITTED bit.  
> 
> No the point of the above argument is that the spec doesn't say anything
> about when committed is cleared. 2 options.
> 1) Hardware clears it when commit 1->0.
> 2) Hardware clears it when commit 0->1
> 
> Given that spec only talks about after a commit 0->1 transition whilst commit
> remains 1, the state after a commit 0->1 transition is implementation defined.
> 
> I think that closing that corner case requires a clarification to the spec.
> 
> Which leaves us with a sticky question of what to do...

Thinking a little more on this and another close look at spec.
The committed bit definition calls out "Indicates a decoder is active"
so if it is not cleared when commit 1->0 then we may have a problem with
multiple decoders and the clear only on commit 0->1 option

Let us first setup decoders as.
decoder 0 -> HPA X to X + N1 (then commit)
decoder 1 -> HPA X + N1 to X + N1 + M1 (then commit)

Now we want to change them without passing through a situation where we have
overlap so that we have N2 > N1. There is a route to doing this but it's
not very intuitive.

1. Unset commit on both decoders
2. Update decoder 1 first and commit.  Have to do it in this order as
   decoder 0 is still committed (in use) so we can't overlap with it.
3. Update decoder 0 second and commit. 

If N1 < N2 need to reverse the order.

1. Unset commit on both decoders
2. Update decoder 0 first and commit. Avoids overlap with still committed decoder 1.
3. Update decoder 1 and commit.

So I think there is a path to make it work but it's nasty.

I'll raise a query with CXL SSWG chair (off list but referring to this thread)

Jonathan 


> 
> > 
> > Fan
> >   
> > > >
> > > > DJ
> > > >     
> > > > > 
> > > > > Fan
> > > > >       
> > > > >> Otherwise, how would one reprogram the decoder if the decoder is indicating
> > > > >> to be active?
> > > > >>
> > > > >> DJ
> > > > >>      
> > > > >>>
> > > > >>> In current kernel implementation, when destroying a region (cxl
> > > > >>> destroy-region),the decoders associated to the region will be reset
> > > > >>> as that in cxl_decoder_reset, where the COMMIT field will be reset.
> > > > >>> However, resetting COMMIT field will not automatically reset the
> > > > >>> COMMITTED field, causing a situation where COMMIT is reset (0) while
> > > > >>> COMMITTED is set (1) after the region is destroyed. Later, when
> > > > >>> init_hdm_decoder is called (during modprobe), current code only check
> > > > >>> the COMMITTED to decide whether the decoder is enabled or not. Since
> > > > >>> the COMMITTED will be 1 and the code treats the decoder as enabled,
> > > > >>> which will cause unexpected behaviour.
> > > > >>>
> > > > >>> Before the fix, a system crash was observed when performing following
> > > > >>> steps:
> > > > >>> 1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
> > > > >>> 2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
> > > > >>> 3. cxl destroy-region region0 -f
> > > > >>> 4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
> > > > >>> 5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
> > > > >>> "no CXL window for range 0x0:0xffffffffffffffff" error message)
> > > > >>> 6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
> > > > >>> crash at cxl_dpa_release due to dpa_res has been freed when destroying
> > > > >>> the region).
> > > > >>>
> > > > >>> The patch fixed the above issue, and is tested based on follow patch series:
> > > > >>>
> > > > >>> [PATCH 00/18] CXL RAM and the 'Soft Reserved' => 'System RAM' default
> > > > >>> Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com
> > > > >>>
> > > > >>> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > > > >>> ---
> > > > >>>    drivers/cxl/core/hdm.c | 8 +++++---
> > > > >>>    1 file changed, 5 insertions(+), 3 deletions(-)
> > > > >>>
> > > > >>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > > >>> index 80eccae6ba9e..6cf854c949f0 100644
> > > > >>> --- a/drivers/cxl/core/hdm.c
> > > > >>> +++ b/drivers/cxl/core/hdm.c
> > > > >>> @@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > > > >>>    	struct cxl_endpoint_decoder *cxled = NULL;
> > > > >>>    	u64 size, base, skip, dpa_size;
> > > > >>>    	bool committed;
> > > > >>> +	bool should_commit;
> > > > >>>    	u32 remainder;
> > > > >>>    	int i, rc;
> > > > >>>    	u32 ctrl;
> > > > >>> @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > > > >>>    	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> > > > >>>    	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> > > > >>>    	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> > > > >>> +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
> > > > >>>    	cxld->commit = cxl_decoder_commit;
> > > > >>>    	cxld->reset = cxl_decoder_reset;
> > > > >>> -	if (!committed)
> > > > >>> +	if (!should_commit || !committed)
> > > > >>>    		size = 0;
> > > > >>>    	if (base == U64_MAX || size == U64_MAX) {
> > > > >>>    		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
> > > > >>> @@ -727,7 +729,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > > > >>>    	};
> > > > >>>    	/* decoders are enabled if committed */
> > > > >>> -	if (committed) {
> > > > >>> +	if (should_commit && committed) {
> > > > >>>    		cxld->flags |= CXL_DECODER_F_ENABLE;
> > > > >>>    		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> > > > >>>    			cxld->flags |= CXL_DECODER_F_LOCK;
> > > > >>> @@ -772,7 +774,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > > > >>>    		return 0;
> > > > >>>    	}
> > > > >>> -	if (!committed)
> > > > >>> +	if (!should_commit || !committed)
> > > > >>>    		return 0;
> > > > >>>    	dpa_size = div_u64_rem(size, cxld->interleave_ways, &remainder);      
> > >    
> 
> 

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Ira Weiny]

Jonathan Cameron wrote:
> On Mon, 6 Mar 2023 16:04:22 +0000
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Fri, 3 Mar 2023 17:21:13 +0000
> > Fan Ni <fan.ni@samsung.com> wrote:
> > 
> > > On Fri, Mar 03, 2023 at 02:36:05PM +0000, Jonathan Cameron wrote:
> > >   
> > > > On Thu, 2 Mar 2023 08:36:59 -0700
> > > > Dave Jiang <dave.jiang@intel.com> wrote:
> > > >     
> > > > > On 3/1/23 11:23 PM, Fan Ni wrote:    
> > > > > > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:      
> > > > > >>      
> > > > > > Hi Dave,
> > > > > > Thanks for looking into this.      
> > > > > >>
> > > > > >> On 2/28/23 3:40 PM, Fan Ni wrote:      
> > > > > >>> Add COMMIT field check aside with existing COMMITTED field check during
> > > > > >>> hdm decoder initialization to avoid a system crash during module removal
> > > > > >>> after destroying a region which leaves the COMMIT field being reset while
> > > > > >>> the COMMITTED field still being set.      
> > > > > >>
> > > > > >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The      
> > > > > > I run into the issue with qemu emulation.      
> > > > > >> situation does not make sense to me. If we clear the COMMIT bit, then the
> > > > > >> COMMITTED bit should be cleared by the hardware shortly after right?      
> > > > > > 
> > > > > >  From the spec, I cannot find any statement saying clearing the COMMIT bit
> > > > > > will automatically clear the COMMITTED. If I have not missed the statement in
> > > > > > the spec, I assume we should not make the assumption that it will be
> > > > > > cleared automatically for real hardware. But you may be right, leaving the
> > > > > > COMMITTED bit set can potentially cause some issue? Need to check more.      
> > > > > 
> > > > > I have not been able to find direct verbiage that indicates this either. 
> > > > > However, logically it would make sense. Otherwise, the COMMITTED field 
> > > > > never clears and prevents reprogramming of the HDM decoders. The current 
> > > > > QEMU implementation is creating a situation where the HDM decoder is 
> > > > > always active after COMMIT bit is set the first time, regardless whether 
> > > > > COMMIT field has been cleared later on during a teardown. It does sound 
> > > > > like a bug with QEMU emulation currently.    
> > > > 
> > > > I agree that one sane interpretation is that unsetting commit should result in
> > > > the decoder being deactivated and hence the commit bit dropping.  However
> > > > I'm not sure that's the only sane interpretation.
> > > > 
> > > > There is no verbage that I'm aware of that says the committed bit being
> > > > set means that the current register values are in use.  It simply says that
> > > > when the commit bit was set, the HDM decoder was successfully committed
> > > > (using registers as set at that time).  There is a specific statement about
> > > > not changing the registers whilst checks are in progress, but those checks
> > > > are only required if lock on commit is set, so it doesn't cover this case.
> > > > 
> > > > Wonderfully there isn't actually anything says what a commit transition to 0
> > > > means.  Does that result in the decoder become uncommitted, or does that only
> > > > happen when the next 0 to 1 transition happens?
> > > > 
> > > > The only stuff we have is what happens when lock on commit = 1, which isn't
> > > > the case here.
> > > > 
> > > > So is there another valid implementation? I think yes.
> > > > In some implementations, there will be a complex state machine that is
> > > > triggered when commit is set.  That will then write some entirely invisible
> > > > internal state for decode logic based on the contents of the registers.
> > > > As such, once it's set committed, it typically won't look at the registers
> > > > again until another commit 0->1 transition happens.  At that point the
> > > > committed bit drops and raised again once the commit state machine finishes
> > > > (given QEMU doesn't emulate that delay the upshot is if you set commit then
> > > > check committed it will be set ;)
> > > > 
> > > > In that implementation the commit 1->0 transition is an irrelevance and
> > > > it won't change the committed bit state.
> > > > 
> > > > So whilst the QEMU code is doing the less obvious implementation, I think
> > > > the spec still allows it.  I don't mind QEMU changing to the more obvious
> > > > one though if someone wants to send a patch.
> > > > 
> > > > Jonathan
> > > >     
> > > 
> > > In current qemu emulation, when COMMITTED bit is set when the decoder is
> > > committed and at the same time the COMMIT field will be cleared. Does
> > > the following fix make sense?
> > > 1. At qemu side, when the commit completes, just set the COMMITTED bit,
> > > but leave the COMMIT bit as set, also check LOCK ON COMMIT bit,
> > > if it is set, clear it, which will allow further reset of COMMIT bit.  
> > 
> > QEMU definitely can't do anything to the Commit bit, other than prevent it being
> > cleared if lock on commit is set.
> > Right now the QEMU emulation doesn't handle LOCK ON COMMIT at all.
> > It would be sensible to add this support, but we don't have an
> > open software stack that ever sets that yet so any testing is likely to be
> > one time only via some hacks.
> > 
> > > 2. for the kernel side, if it needs to reprogram the decoder, it needs to
> > > check the COMMITTED bit, if it is set, then OS need to reset COMMIT bit
> > > first, which will also clear COMMITTED bit automatically at qemu side.  
> > 
> > Could do it that way, or simplify it by always clearing commit before setting
> > it to make sure the transition happens.
> > 
> > Looks like commit is cleared in cxl_decoder_reset() already so this may
> > already happen - I haven't checked the flow.
> > 
> > > 3. when the OS needs to reset the decoder, it does similar thing as 2 to
> > > reset COMMIT bit and qemu will clear COMMITTED bit.  
> > 
> > No the point of the above argument is that the spec doesn't say anything
> > about when committed is cleared. 2 options.
> > 1) Hardware clears it when commit 1->0.
> > 2) Hardware clears it when commit 0->1
> > 
> > Given that spec only talks about after a commit 0->1 transition whilst commit
> > remains 1, the state after a commit 0->1 transition is implementation defined.
> > 
> > I think that closing that corner case requires a clarification to the spec.
> > 
> > Which leaves us with a sticky question of what to do...
> 
> Thinking a little more on this and another close look at spec.
> The committed bit definition calls out "Indicates a decoder is active"
> so if it is not cleared when commit 1->0 then we may have a problem with
> multiple decoders and the clear only on commit 0->1 option
> 
> Let us first setup decoders as.
> decoder 0 -> HPA X to X + N1 (then commit)
> decoder 1 -> HPA X + N1 to X + N1 + M1 (then commit)
> 
> Now we want to change them without passing through a situation where we have
> overlap so that we have N2 > N1. There is a route to doing this but it's
> not very intuitive.

I'm a bit unclear on the variables here.

We have 2 ranges A and B and we want to add C?

Size of A is N1
Size of B is M1?

Then Size of C is N2?

Or is N2 a new size of N1?  So the size of A is changing?

> 
> 1. Unset commit on both decoders
> 2. Update decoder 1 first and commit.  Have to do it in this order as
>    decoder 0 is still committed (in use) so we can't overlap with it.
> 3. Update decoder 0 second and commit. 
> 
> If N1 < N2 need to reverse the order.
> 
> 1. Unset commit on both decoders
> 2. Update decoder 0 first and commit. Avoids overlap with still committed decoder 1.
> 3. Update decoder 1 and commit.

If the size of A is changing then yes I think this is required.  But I
don't think it has anything to do with the commit bit.  I think we have to
program decoders in order anyway so this was required all along.  Wasn't
it?

> 
> So I think there is a path to make it work but it's nasty.

Not nice no...  :-(

> 
> I'll raise a query with CXL SSWG chair (off list but referring to this thread)

Not a bad idea.  I'm no expert on this I'm just going off of what I have
heard/remember/read on the fly...

Ira

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Jonathan Cameron]

On Tue, 7 Mar 2023 09:27:43 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Mon, 6 Mar 2023 16:04:22 +0000
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Fri, 3 Mar 2023 17:21:13 +0000
> > > Fan Ni <fan.ni@samsung.com> wrote:
> > >   
> > > > On Fri, Mar 03, 2023 at 02:36:05PM +0000, Jonathan Cameron wrote:
> > > >     
> > > > > On Thu, 2 Mar 2023 08:36:59 -0700
> > > > > Dave Jiang <dave.jiang@intel.com> wrote:
> > > > >       
> > > > > > On 3/1/23 11:23 PM, Fan Ni wrote:      
> > > > > > > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:        
> > > > > > >>        
> > > > > > > Hi Dave,
> > > > > > > Thanks for looking into this.        
> > > > > > >>
> > > > > > >> On 2/28/23 3:40 PM, Fan Ni wrote:        
> > > > > > >>> Add COMMIT field check aside with existing COMMITTED field check during
> > > > > > >>> hdm decoder initialization to avoid a system crash during module removal
> > > > > > >>> after destroying a region which leaves the COMMIT field being reset while
> > > > > > >>> the COMMITTED field still being set.        
> > > > > > >>
> > > > > > >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The        
> > > > > > > I run into the issue with qemu emulation.        
> > > > > > >> situation does not make sense to me. If we clear the COMMIT bit, then the
> > > > > > >> COMMITTED bit should be cleared by the hardware shortly after right?        
> > > > > > > 
> > > > > > >  From the spec, I cannot find any statement saying clearing the COMMIT bit
> > > > > > > will automatically clear the COMMITTED. If I have not missed the statement in
> > > > > > > the spec, I assume we should not make the assumption that it will be
> > > > > > > cleared automatically for real hardware. But you may be right, leaving the
> > > > > > > COMMITTED bit set can potentially cause some issue? Need to check more.        
> > > > > > 
> > > > > > I have not been able to find direct verbiage that indicates this either. 
> > > > > > However, logically it would make sense. Otherwise, the COMMITTED field 
> > > > > > never clears and prevents reprogramming of the HDM decoders. The current 
> > > > > > QEMU implementation is creating a situation where the HDM decoder is 
> > > > > > always active after COMMIT bit is set the first time, regardless whether 
> > > > > > COMMIT field has been cleared later on during a teardown. It does sound 
> > > > > > like a bug with QEMU emulation currently.      
> > > > > 
> > > > > I agree that one sane interpretation is that unsetting commit should result in
> > > > > the decoder being deactivated and hence the commit bit dropping.  However
> > > > > I'm not sure that's the only sane interpretation.
> > > > > 
> > > > > There is no verbage that I'm aware of that says the committed bit being
> > > > > set means that the current register values are in use.  It simply says that
> > > > > when the commit bit was set, the HDM decoder was successfully committed
> > > > > (using registers as set at that time).  There is a specific statement about
> > > > > not changing the registers whilst checks are in progress, but those checks
> > > > > are only required if lock on commit is set, so it doesn't cover this case.
> > > > > 
> > > > > Wonderfully there isn't actually anything says what a commit transition to 0
> > > > > means.  Does that result in the decoder become uncommitted, or does that only
> > > > > happen when the next 0 to 1 transition happens?
> > > > > 
> > > > > The only stuff we have is what happens when lock on commit = 1, which isn't
> > > > > the case here.
> > > > > 
> > > > > So is there another valid implementation? I think yes.
> > > > > In some implementations, there will be a complex state machine that is
> > > > > triggered when commit is set.  That will then write some entirely invisible
> > > > > internal state for decode logic based on the contents of the registers.
> > > > > As such, once it's set committed, it typically won't look at the registers
> > > > > again until another commit 0->1 transition happens.  At that point the
> > > > > committed bit drops and raised again once the commit state machine finishes
> > > > > (given QEMU doesn't emulate that delay the upshot is if you set commit then
> > > > > check committed it will be set ;)
> > > > > 
> > > > > In that implementation the commit 1->0 transition is an irrelevance and
> > > > > it won't change the committed bit state.
> > > > > 
> > > > > So whilst the QEMU code is doing the less obvious implementation, I think
> > > > > the spec still allows it.  I don't mind QEMU changing to the more obvious
> > > > > one though if someone wants to send a patch.
> > > > > 
> > > > > Jonathan
> > > > >       
> > > > 
> > > > In current qemu emulation, when COMMITTED bit is set when the decoder is
> > > > committed and at the same time the COMMIT field will be cleared. Does
> > > > the following fix make sense?
> > > > 1. At qemu side, when the commit completes, just set the COMMITTED bit,
> > > > but leave the COMMIT bit as set, also check LOCK ON COMMIT bit,
> > > > if it is set, clear it, which will allow further reset of COMMIT bit.    
> > > 
> > > QEMU definitely can't do anything to the Commit bit, other than prevent it being
> > > cleared if lock on commit is set.
> > > Right now the QEMU emulation doesn't handle LOCK ON COMMIT at all.
> > > It would be sensible to add this support, but we don't have an
> > > open software stack that ever sets that yet so any testing is likely to be
> > > one time only via some hacks.
> > >   
> > > > 2. for the kernel side, if it needs to reprogram the decoder, it needs to
> > > > check the COMMITTED bit, if it is set, then OS need to reset COMMIT bit
> > > > first, which will also clear COMMITTED bit automatically at qemu side.    
> > > 
> > > Could do it that way, or simplify it by always clearing commit before setting
> > > it to make sure the transition happens.
> > > 
> > > Looks like commit is cleared in cxl_decoder_reset() already so this may
> > > already happen - I haven't checked the flow.
> > >   
> > > > 3. when the OS needs to reset the decoder, it does similar thing as 2 to
> > > > reset COMMIT bit and qemu will clear COMMITTED bit.    
> > > 
> > > No the point of the above argument is that the spec doesn't say anything
> > > about when committed is cleared. 2 options.
> > > 1) Hardware clears it when commit 1->0.
> > > 2) Hardware clears it when commit 0->1
> > > 
> > > Given that spec only talks about after a commit 0->1 transition whilst commit
> > > remains 1, the state after a commit 0->1 transition is implementation defined.
> > > 
> > > I think that closing that corner case requires a clarification to the spec.
> > > 
> > > Which leaves us with a sticky question of what to do...  
> > 
> > Thinking a little more on this and another close look at spec.
> > The committed bit definition calls out "Indicates a decoder is active"
> > so if it is not cleared when commit 1->0 then we may have a problem with
> > multiple decoders and the clear only on commit 0->1 option
> > 
> > Let us first setup decoders as.
> > decoder 0 -> HPA X to X + N1 (then commit)
> > decoder 1 -> HPA X + N1 to X + N1 + M1 (then commit)
> > 
> > Now we want to change them without passing through a situation where we have
> > overlap so that we have N2 > N1. There is a route to doing this but it's
> > not very intuitive.  
> 
> I'm a bit unclear on the variables here.
> 
> We have 2 ranges A and B and we want to add C?

Always 2 ranges, aim is to move the boundary between them
in one direction or the other.  It's the simplest dance
I could think of that makes things complex.


> 
> Size of A is N1? 
Yes
> Size of B is M1?
Yes
> 
> Then Size of C is N2?
> 
> Or is N2 a new size of N1?  So the size of A is changing?

This one. I indeed failed to define N2. Oops.

> 
> > 
> > 1. Unset commit on both decoders
> > 2. Update decoder 1 first and commit.  Have to do it in this order as
> >    decoder 0 is still committed (in use) so we can't overlap with it.
> > 3. Update decoder 0 second and commit. 
> > 
> > If N1 < N2 need to reverse the order.
> > 
> > 1. Unset commit on both decoders
> > 2. Update decoder 0 first and commit. Avoids overlap with still committed decoder 1.
> > 3. Update decoder 1 and commit.  
> 
> If the size of A is changing then yes I think this is required.  But I
> don't think it has anything to do with the commit bit.  I think we have to
> program decoders in order anyway so this was required all along.  Wasn't
> it?

Snag is that the decodering is based not on the commit bit, but on the state
of the committed bit.  If the spec provides no way to 'unset' that short
of committing new settings for a given decoder, the dance becomes 'exciting'.

If we can just reset it then a resize like the above just means uncommitting
the two regions then building them again as if nothing ever existed.

> 
> > 
> > So I think there is a path to make it work but it's nasty.  
> 
> Not nice no...  :-(
> 
> > 
> > I'll raise a query with CXL SSWG chair (off list but referring to this thread)  
> 
> Not a bad idea.  I'm no expert on this I'm just going off of what I have
> heard/remember/read on the fly...

In progress.

Note that I'm fine to just change QEMU to do the 'obvious' option.  We can revisit
the pathological cases if it turns out to be necessary.
Good to identify this fun corner case though ;)

Jonathan

> 
> Ira
> 

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Jonathan Cameron]

On Mon, 13 Mar 2023 10:10:13 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Tue, 7 Mar 2023 09:27:43 -0800
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > Jonathan Cameron wrote:  
> > > On Mon, 6 Mar 2023 16:04:22 +0000
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >     
> > > > On Fri, 3 Mar 2023 17:21:13 +0000
> > > > Fan Ni <fan.ni@samsung.com> wrote:
> > > >     
> > > > > On Fri, Mar 03, 2023 at 02:36:05PM +0000, Jonathan Cameron wrote:
> > > > >       
> > > > > > On Thu, 2 Mar 2023 08:36:59 -0700
> > > > > > Dave Jiang <dave.jiang@intel.com> wrote:
> > > > > >         
> > > > > > > On 3/1/23 11:23 PM, Fan Ni wrote:        
> > > > > > > > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:          
> > > > > > > >>          
> > > > > > > > Hi Dave,
> > > > > > > > Thanks for looking into this.          
> > > > > > > >>
> > > > > > > >> On 2/28/23 3:40 PM, Fan Ni wrote:          
> > > > > > > >>> Add COMMIT field check aside with existing COMMITTED field check during
> > > > > > > >>> hdm decoder initialization to avoid a system crash during module removal
> > > > > > > >>> after destroying a region which leaves the COMMIT field being reset while
> > > > > > > >>> the COMMITTED field still being set.          
> > > > > > > >>
> > > > > > > >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The          
> > > > > > > > I run into the issue with qemu emulation.          
> > > > > > > >> situation does not make sense to me. If we clear the COMMIT bit, then the
> > > > > > > >> COMMITTED bit should be cleared by the hardware shortly after right?          
> > > > > > > > 
> > > > > > > >  From the spec, I cannot find any statement saying clearing the COMMIT bit
> > > > > > > > will automatically clear the COMMITTED. If I have not missed the statement in
> > > > > > > > the spec, I assume we should not make the assumption that it will be
> > > > > > > > cleared automatically for real hardware. But you may be right, leaving the
> > > > > > > > COMMITTED bit set can potentially cause some issue? Need to check more.          
> > > > > > > 
> > > > > > > I have not been able to find direct verbiage that indicates this either. 
> > > > > > > However, logically it would make sense. Otherwise, the COMMITTED field 
> > > > > > > never clears and prevents reprogramming of the HDM decoders. The current 
> > > > > > > QEMU implementation is creating a situation where the HDM decoder is 
> > > > > > > always active after COMMIT bit is set the first time, regardless whether 
> > > > > > > COMMIT field has been cleared later on during a teardown. It does sound 
> > > > > > > like a bug with QEMU emulation currently.        
> > > > > > 
> > > > > > I agree that one sane interpretation is that unsetting commit should result in
> > > > > > the decoder being deactivated and hence the commit bit dropping.  However
> > > > > > I'm not sure that's the only sane interpretation.
> > > > > > 
> > > > > > There is no verbage that I'm aware of that says the committed bit being
> > > > > > set means that the current register values are in use.  It simply says that
> > > > > > when the commit bit was set, the HDM decoder was successfully committed
> > > > > > (using registers as set at that time).  There is a specific statement about
> > > > > > not changing the registers whilst checks are in progress, but those checks
> > > > > > are only required if lock on commit is set, so it doesn't cover this case.
> > > > > > 
> > > > > > Wonderfully there isn't actually anything says what a commit transition to 0
> > > > > > means.  Does that result in the decoder become uncommitted, or does that only
> > > > > > happen when the next 0 to 1 transition happens?
> > > > > > 
> > > > > > The only stuff we have is what happens when lock on commit = 1, which isn't
> > > > > > the case here.
> > > > > > 
> > > > > > So is there another valid implementation? I think yes.
> > > > > > In some implementations, there will be a complex state machine that is
> > > > > > triggered when commit is set.  That will then write some entirely invisible
> > > > > > internal state for decode logic based on the contents of the registers.
> > > > > > As such, once it's set committed, it typically won't look at the registers
> > > > > > again until another commit 0->1 transition happens.  At that point the
> > > > > > committed bit drops and raised again once the commit state machine finishes
> > > > > > (given QEMU doesn't emulate that delay the upshot is if you set commit then
> > > > > > check committed it will be set ;)
> > > > > > 
> > > > > > In that implementation the commit 1->0 transition is an irrelevance and
> > > > > > it won't change the committed bit state.
> > > > > > 
> > > > > > So whilst the QEMU code is doing the less obvious implementation, I think
> > > > > > the spec still allows it.  I don't mind QEMU changing to the more obvious
> > > > > > one though if someone wants to send a patch.
> > > > > > 
> > > > > > Jonathan
> > > > > >         
> > > > > 
> > > > > In current qemu emulation, when COMMITTED bit is set when the decoder is
> > > > > committed and at the same time the COMMIT field will be cleared. Does
> > > > > the following fix make sense?
> > > > > 1. At qemu side, when the commit completes, just set the COMMITTED bit,
> > > > > but leave the COMMIT bit as set, also check LOCK ON COMMIT bit,
> > > > > if it is set, clear it, which will allow further reset of COMMIT bit.      
> > > > 
> > > > QEMU definitely can't do anything to the Commit bit, other than prevent it being
> > > > cleared if lock on commit is set.
> > > > Right now the QEMU emulation doesn't handle LOCK ON COMMIT at all.
> > > > It would be sensible to add this support, but we don't have an
> > > > open software stack that ever sets that yet so any testing is likely to be
> > > > one time only via some hacks.
> > > >     
> > > > > 2. for the kernel side, if it needs to reprogram the decoder, it needs to
> > > > > check the COMMITTED bit, if it is set, then OS need to reset COMMIT bit
> > > > > first, which will also clear COMMITTED bit automatically at qemu side.      
> > > > 
> > > > Could do it that way, or simplify it by always clearing commit before setting
> > > > it to make sure the transition happens.
> > > > 
> > > > Looks like commit is cleared in cxl_decoder_reset() already so this may
> > > > already happen - I haven't checked the flow.
> > > >     
> > > > > 3. when the OS needs to reset the decoder, it does similar thing as 2 to
> > > > > reset COMMIT bit and qemu will clear COMMITTED bit.      
> > > > 
> > > > No the point of the above argument is that the spec doesn't say anything
> > > > about when committed is cleared. 2 options.
> > > > 1) Hardware clears it when commit 1->0.
> > > > 2) Hardware clears it when commit 0->1
> > > > 
> > > > Given that spec only talks about after a commit 0->1 transition whilst commit
> > > > remains 1, the state after a commit 0->1 transition is implementation defined.
> > > > 
> > > > I think that closing that corner case requires a clarification to the spec.
> > > > 
> > > > Which leaves us with a sticky question of what to do...    
> > > 
> > > Thinking a little more on this and another close look at spec.
> > > The committed bit definition calls out "Indicates a decoder is active"
> > > so if it is not cleared when commit 1->0 then we may have a problem with
> > > multiple decoders and the clear only on commit 0->1 option
> > > 
> > > Let us first setup decoders as.
> > > decoder 0 -> HPA X to X + N1 (then commit)
> > > decoder 1 -> HPA X + N1 to X + N1 + M1 (then commit)
> > > 
> > > Now we want to change them without passing through a situation where we have
> > > overlap so that we have N2 > N1. There is a route to doing this but it's
> > > not very intuitive.    
> > 
> > I'm a bit unclear on the variables here.
> > 
> > We have 2 ranges A and B and we want to add C?  
> 
> Always 2 ranges, aim is to move the boundary between them
> in one direction or the other.  It's the simplest dance
> I could think of that makes things complex.
> 
> 
> > 
> > Size of A is N1?   
> Yes
> > Size of B is M1?  
> Yes
> > 
> > Then Size of C is N2?
> > 
> > Or is N2 a new size of N1?  So the size of A is changing?  
> 
> This one. I indeed failed to define N2. Oops.
> 
> >   
> > > 
> > > 1. Unset commit on both decoders
> > > 2. Update decoder 1 first and commit.  Have to do it in this order as
> > >    decoder 0 is still committed (in use) so we can't overlap with it.
> > > 3. Update decoder 0 second and commit. 
> > > 
> > > If N1 < N2 need to reverse the order.
> > > 
> > > 1. Unset commit on both decoders
> > > 2. Update decoder 0 first and commit. Avoids overlap with still committed decoder 1.
> > > 3. Update decoder 1 and commit.    
> > 
> > If the size of A is changing then yes I think this is required.  But I
> > don't think it has anything to do with the commit bit.  I think we have to
> > program decoders in order anyway so this was required all along.  Wasn't
> > it?  
> 
> Snag is that the decodering is based not on the commit bit, but on the state
> of the committed bit.  If the spec provides no way to 'unset' that short
> of committing new settings for a given decoder, the dance becomes 'exciting'.
> 
> If we can just reset it then a resize like the above just means uncommitting
> the two regions then building them again as if nothing ever existed.
> 
> >   
> > > 
> > > So I think there is a path to make it work but it's nasty.    
> > 
> > Not nice no...  :-(
> >   
> > > 
> > > I'll raise a query with CXL SSWG chair (off list but referring to this thread)    
> > 
> > Not a bad idea.  I'm no expert on this I'm just going off of what I have
> > heard/remember/read on the fly...  
> 
> In progress.
> 
> Note that I'm fine to just change QEMU to do the 'obvious' option.  We can revisit
> the pathological cases if it turns out to be necessary.
> Good to identify this fun corner case though ;)

Hmm.. I should have read the QEMU code more closely. It is definitely buggy in this area
even if we ignore this discussion.  Currently QEMU self clears commit when it sets
committed. That's definitely wrong as register field is RWL. I'll send a fix out shortly and roll in
the committed clearing into the same patch.

Jonathan

> 
> Jonathan
> 
> > 
> > Ira
> >   
> 

Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check

[Fan Ni]

On Fri, Mar 03, 2023 at 02:36:25PM -0800, Dan Williams wrote:
> Fan Ni wrote:
> [..]
> > > I think a separate fix for that crash is needed, can you send the
> > > backtrace? I.e. I worry that crash can be triggered by other means.
> > Hi Dan,
> > See backtrace below.
> 
> Thanks, I'll take a look. 
> 
> [..]
> > > > @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > > >  	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> > > >  	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> > > >  	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> > > > +	should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
> > > 
> > > This change looks like a good idea in general given the ambiguity of
> > > 'committed'. However just combine the two checks into the @committed
> > > variable with something like this:
> > > 
> > > commit_mask = CXL_HDM_DECODER0_CTRL_COMMITTED|CXL_HDM_DECODER0_CTRL_COMMIT;
> > > committed = (ctrl & commit_mask) == commit_mask;
> 
> Did you also notice this ^^^ request for a fixed up version of the
> current patch?

Hi Dan,
Jonathan sent out a qemu patch to fix the committed field
reset as below, and the patch fixed the system crash discussed here.
https://lore.kernel.org/linux-cxl/20230322102731.4219-1-Jonathan.Cameron@huawei.com/T/#me5283349b37d53abc93904a2428910a2f6a354f6

Do you think we need a separate fix at kernel side to fix the
possible system crash when cxl_dpa_release is called and dpa_res is
null? I have noticed at some location, dpa_res is checked before
calling cxl_dpa_release for example in function cxl_dpa_free, but no guard
from other callers. If it is needed, I have a simple fix and ready
to send out.

Fan