Linux CXL
 help / color / mirror / Atom feed
* Questions about the qemu DCD support in cxl-2023-09-13
@ 2023-09-21 22:24 ` Ira Weiny
  2023-09-21 22:51   ` Fan Ni
  2023-10-23 19:48   ` fan
  0 siblings, 2 replies; 11+ messages in thread
From: Ira Weiny @ 2023-09-21 22:24 UTC (permalink / raw)
  To: Fan Ni; +Cc: Jonathan Cameron, Singh, Naveen, linux-cxl

Fan,

I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]

I've been testing various things and so far I have a couple of questions.

1) If the qmp command is used to add extents which overlap other extents
   shouldn't that throw an error?  I don't see any validation of this and
   I would think a real device would reject such a request from the FM.

2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
   extents prior to driver load does not seem to work.  And I think this
   is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().

Ira

[1] https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Questions about the qemu DCD support in cxl-2023-09-13
  2023-09-21 22:24 ` Questions about the qemu DCD support in cxl-2023-09-13 Ira Weiny
@ 2023-09-21 22:51   ` Fan Ni
  2023-10-23 19:48   ` fan
  1 sibling, 0 replies; 11+ messages in thread
From: Fan Ni @ 2023-09-21 22:51 UTC (permalink / raw)
  To: Ira Weiny; +Cc: Jonathan Cameron, Singh, Naveen, linux-cxl@vger.kernel.org

On Thu, Sep 21, 2023 at 03:24:26PM -0700, Ira Weiny wrote:

Hi Ira,

Thanks for reaching out.
For DCD, I think Jonathan's branch does not change much compared to the patch
series sent out except some obvious issues that blocks the test.

My initial plan was to update the patches after I have tested the
current version with more stable kernel DCD driver.

See below.


> Fan,
> 
> I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]
> 
> I've been testing various things and so far I have a couple of questions.
> 
> 1) If the qmp command is used to add extents which overlap other extents
>    shouldn't that throw an error?  I don't see any validation of this and
>    I would think a real device would reject such a request from the FM.
> 

Yes. We do not have any validation for the overlapped extents here
in current code.

> 2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
>    extents prior to driver load does not seem to work.  And I think this
>    is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().

I am aware of the issue, yet have not got a chance to fix it. 

My understanding is whenever we complete adding/releasing
extents, we update it.

I will fix it and other issues mentioned in the comments in the next version.

Fan
> 
> Ira
> 
> [1] https://urldefense.com/v3/__https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13__;!!EwVzqGoTKBqv-0DWAJBm!WrndoKsXHVaUoln-Jsq7MFRemLlcFYg788t_tDaFt1lS7WFhE2s_5hsI4-WuNzlgLOc0e0DgdcepR368SHU$ 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Questions about the qemu DCD support in cxl-2023-09-13
  2023-09-21 22:24 ` Questions about the qemu DCD support in cxl-2023-09-13 Ira Weiny
  2023-09-21 22:51   ` Fan Ni
@ 2023-10-23 19:48   ` fan
  2023-10-24 18:56     ` fan
  1 sibling, 1 reply; 11+ messages in thread
From: fan @ 2023-10-23 19:48 UTC (permalink / raw)
  To: Ira Weiny
  Cc: Fan Ni, Jonathan Cameron, Singh, Naveen, linux-cxl, a.manzanares,
	dave, nmtadam.samsung

On Thu, Sep 21, 2023 at 03:24:26PM -0700, Ira Weiny wrote:
> Fan,
> 
> I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]
> 
> I've been testing various things and so far I have a couple of questions.
> 
> 1) If the qmp command is used to add extents which overlap other extents
>    shouldn't that throw an error?  I don't see any validation of this and
>    I would think a real device would reject such a request from the FM.
> 
> 2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
>    extents prior to driver load does not seem to work.  And I think this
>    is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().
> 
> Ira
> 
> [1] https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13

Hi Ira,
FYI. I have updated the DCD emulation patch series based on feedbacks on
the previous version.

The new version is here:
https://github.com/moking/qemu-jic-clone/tree/dcd-dev

The code is based on Jonathan's branch cxl-2023-09-26.

The main changes include,

1. Update cxl_find_dc_region to detect the case the range of
the extent cross multiple DC regions.
2. Add comments to explain the checks performed in function
cxl_detect_malformed_extent_list. (Jonathan)
3. Minimize the checks in cmd_dcd_add_dyn_cap_rsp.(Jonathan)
4. Update total_extent_count in add/release dynamic capacity response function.
(Ira and Jorgen Hansen).
5. Fix the logic issue in test_bits and renamed it to
test_any_bits_set to clear its function.
6. Add pending extent list for add/release extent event.
7. When add/release extent response is received, use the pending list to
verify the extents are valid.
8. Add test_any_bits_set and cxl_insert_extent_to_extent_list declaration to
cxl_device.h so it can be used in different files.
9. Updated ct3d_qmp_cxl_event_log_enc to include dynamic capacity event log type.
10. Extract the functionality to delete extent from extent list to a helper
function.
11. Move the update of the bitmap which reflects which blocks are backed with
dc extents from the moment when a dc extent is offered to the moment when it
is accepted from the host.

I was able to test the DCD code you sent previously, let me know if you
find any issues.

Fan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Questions about the qemu DCD support in cxl-2023-09-13
  2023-10-23 19:48   ` fan
@ 2023-10-24 18:56     ` fan
  2023-10-25  5:25       ` fan
  0 siblings, 1 reply; 11+ messages in thread
From: fan @ 2023-10-24 18:56 UTC (permalink / raw)
  To: fan
  Cc: Ira Weiny, Fan Ni, Jonathan Cameron, Singh, Naveen, linux-cxl,
	a.manzanares, dave, nmtadam.samsung

On Mon, Oct 23, 2023 at 12:48:02PM -0700, fan wrote:
> On Thu, Sep 21, 2023 at 03:24:26PM -0700, Ira Weiny wrote:
> > Fan,
> > 
> > I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]
> > 
> > I've been testing various things and so far I have a couple of questions.
> > 
> > 1) If the qmp command is used to add extents which overlap other extents
> >    shouldn't that throw an error?  I don't see any validation of this and
> >    I would think a real device would reject such a request from the FM.
> > 
> > 2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
> >    extents prior to driver load does not seem to work.  And I think this
> >    is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().
> > 
> > Ira
> > 
> > [1] https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13
> 
> Hi Ira,
> FYI. I have updated the DCD emulation patch series based on feedbacks on
> the previous version.
> 
> The new version is here:
> https://github.com/moking/qemu-jic-clone/tree/dcd-dev
> 
> The code is based on Jonathan's branch cxl-2023-09-26.
> 
> The main changes include,
> 
> 1. Update cxl_find_dc_region to detect the case the range of
> the extent cross multiple DC regions.
> 2. Add comments to explain the checks performed in function
> cxl_detect_malformed_extent_list. (Jonathan)
> 3. Minimize the checks in cmd_dcd_add_dyn_cap_rsp.(Jonathan)
> 4. Update total_extent_count in add/release dynamic capacity response function.
> (Ira and Jorgen Hansen).
> 5. Fix the logic issue in test_bits and renamed it to
> test_any_bits_set to clear its function.
> 6. Add pending extent list for add/release extent event.
> 7. When add/release extent response is received, use the pending list to
> verify the extents are valid.
> 8. Add test_any_bits_set and cxl_insert_extent_to_extent_list declaration to
> cxl_device.h so it can be used in different files.
> 9. Updated ct3d_qmp_cxl_event_log_enc to include dynamic capacity event log type.
> 10. Extract the functionality to delete extent from extent list to a helper
> function.
> 11. Move the update of the bitmap which reflects which blocks are backed with
> dc extents from the moment when a dc extent is offered to the moment when it
> is accepted from the host.
> 
> I was able to test the DCD code you sent previously, let me know if you
> find any issues.
> 
> Fan

FYI. 

Updated the last two commits to drop the extents for pending to
release as the host can proactively release dc extents so there can be
no pending-to-release extent list on the device side.

Fan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Questions about the qemu DCD support in cxl-2023-09-13
  2023-10-24 18:56     ` fan
@ 2023-10-25  5:25       ` fan
  2023-10-26  9:01         ` Jonathan Cameron
  0 siblings, 1 reply; 11+ messages in thread
From: fan @ 2023-10-25  5:25 UTC (permalink / raw)
  To: fan
  Cc: Ira Weiny, Fan Ni, Jonathan Cameron, Singh, Naveen, linux-cxl,
	a.manzanares, dave, nmtadam.samsung

On Tue, Oct 24, 2023 at 11:56:02AM -0700, fan wrote:
> On Mon, Oct 23, 2023 at 12:48:02PM -0700, fan wrote:
> > On Thu, Sep 21, 2023 at 03:24:26PM -0700, Ira Weiny wrote:
> > > Fan,
> > > 
> > > I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]
> > > 
> > > I've been testing various things and so far I have a couple of questions.
> > > 
> > > 1) If the qmp command is used to add extents which overlap other extents
> > >    shouldn't that throw an error?  I don't see any validation of this and
> > >    I would think a real device would reject such a request from the FM.
> > > 
> > > 2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
> > >    extents prior to driver load does not seem to work.  And I think this
> > >    is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().
> > > 
> > > Ira
> > > 
> > > [1] https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13
> > 
> > Hi Ira,
> > FYI. I have updated the DCD emulation patch series based on feedbacks on
> > the previous version.
> > 
> > The new version is here:
> > https://github.com/moking/qemu-jic-clone/tree/dcd-dev
> > 
> > The code is based on Jonathan's branch cxl-2023-09-26.
> > 
> > The main changes include,
> > 
> > 1. Update cxl_find_dc_region to detect the case the range of
> > the extent cross multiple DC regions.
> > 2. Add comments to explain the checks performed in function
> > cxl_detect_malformed_extent_list. (Jonathan)
> > 3. Minimize the checks in cmd_dcd_add_dyn_cap_rsp.(Jonathan)
> > 4. Update total_extent_count in add/release dynamic capacity response function.
> > (Ira and Jorgen Hansen).
> > 5. Fix the logic issue in test_bits and renamed it to
> > test_any_bits_set to clear its function.
> > 6. Add pending extent list for add/release extent event.
> > 7. When add/release extent response is received, use the pending list to
> > verify the extents are valid.
> > 8. Add test_any_bits_set and cxl_insert_extent_to_extent_list declaration to
> > cxl_device.h so it can be used in different files.
> > 9. Updated ct3d_qmp_cxl_event_log_enc to include dynamic capacity event log type.
> > 10. Extract the functionality to delete extent from extent list to a helper
> > function.
> > 11. Move the update of the bitmap which reflects which blocks are backed with
> > dc extents from the moment when a dc extent is offered to the moment when it
> > is accepted from the host.
> > 
> > I was able to test the DCD code you sent previously, let me know if you
> > find any issues.
> > 
> > Fan
> 
> FYI. 
> 
> Updated the last two commits to drop the extents for pending to
> release as the host can proactively release dc extents so there can be
> no pending-to-release extent list on the device side.
> 
> Fan

Ira located a bug when testing dc extent adding with his latest DCD kernel
code.
The dpa passed in the qmp command is offset relative to the base of the
region where the extent is offered, we need to convert it to address relative
to the base address of the base of the region0 so the bit in the bitmap can
be correctly mapped.

Update the commit (commit 0ad5136: hw/cxl/events: Add qmp interfaces to
add/release dynamic capacity ext…) with the following changes to fix the issue.


diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index ea88b53f41..d51f2ef9f5 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1978,7 +1978,7 @@ static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
		 extents[i].shared_seq = 0;

		 /* No duplicate or overlapped extents are allowed */
-        dpa -= dcd->dc.regions[0].base;
+        dpa = dpa + dcd->dc.regions[rid].base - dcd->dc.regions[0].base;
		 if (test_any_bits_set(blk_bitmap, dpa / min_block_size,
			 len / min_block_size)) {
			 error_setg(errp, "duplicate or overlapped extents are detected");


Fan

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: Questions about the qemu DCD support in cxl-2023-09-13
  2023-10-25  5:25       ` fan
@ 2023-10-26  9:01         ` Jonathan Cameron
  2023-10-26 16:58           ` Jonathan Cameron
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2023-10-26  9:01 UTC (permalink / raw)
  To: fan, linux-cxl, a.manzanares
  Cc: Ira Weiny, Fan Ni, Singh, Naveen, dave, nmtadam.samsung

On Tue, 24 Oct 2023 22:25:53 -0700
fan <nifan.cxl@gmail.com> wrote:

> On Tue, Oct 24, 2023 at 11:56:02AM -0700, fan wrote:
> > On Mon, Oct 23, 2023 at 12:48:02PM -0700, fan wrote:  
> > > On Thu, Sep 21, 2023 at 03:24:26PM -0700, Ira Weiny wrote:  
> > > > Fan,
> > > > 
> > > > I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]
> > > > 
> > > > I've been testing various things and so far I have a couple of questions.
> > > > 
> > > > 1) If the qmp command is used to add extents which overlap other extents
> > > >    shouldn't that throw an error?  I don't see any validation of this and
> > > >    I would think a real device would reject such a request from the FM.
> > > > 
> > > > 2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
> > > >    extents prior to driver load does not seem to work.  And I think this
> > > >    is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().
> > > > 
> > > > Ira
> > > > 
> > > > [1] https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13  
> > > 
> > > Hi Ira,
> > > FYI. I have updated the DCD emulation patch series based on feedbacks on
> > > the previous version.
> > > 
> > > The new version is here:
> > > https://github.com/moking/qemu-jic-clone/tree/dcd-dev
> > > 
> > > The code is based on Jonathan's branch cxl-2023-09-26.
> > > 
> > > The main changes include,
> > > 
> > > 1. Update cxl_find_dc_region to detect the case the range of
> > > the extent cross multiple DC regions.
> > > 2. Add comments to explain the checks performed in function
> > > cxl_detect_malformed_extent_list. (Jonathan)
> > > 3. Minimize the checks in cmd_dcd_add_dyn_cap_rsp.(Jonathan)
> > > 4. Update total_extent_count in add/release dynamic capacity response function.
> > > (Ira and Jorgen Hansen).
> > > 5. Fix the logic issue in test_bits and renamed it to
> > > test_any_bits_set to clear its function.
> > > 6. Add pending extent list for add/release extent event.
> > > 7. When add/release extent response is received, use the pending list to
> > > verify the extents are valid.
> > > 8. Add test_any_bits_set and cxl_insert_extent_to_extent_list declaration to
> > > cxl_device.h so it can be used in different files.
> > > 9. Updated ct3d_qmp_cxl_event_log_enc to include dynamic capacity event log type.
> > > 10. Extract the functionality to delete extent from extent list to a helper
> > > function.
> > > 11. Move the update of the bitmap which reflects which blocks are backed with
> > > dc extents from the moment when a dc extent is offered to the moment when it
> > > is accepted from the host.
> > > 
> > > I was able to test the DCD code you sent previously, let me know if you
> > > find any issues.
> > > 
> > > Fan  
> > 
> > FYI. 
> > 
> > Updated the last two commits to drop the extents for pending to
> > release as the host can proactively release dc extents so there can be
> > no pending-to-release extent list on the device side.
> > 
> > Fan  
> 
> Ira located a bug when testing dc extent adding with his latest DCD kernel
> code.
> The dpa passed in the qmp command is offset relative to the base of the
> region where the extent is offered, we need to convert it to address relative
> to the base address of the base of the region0 so the bit in the bitmap can
> be correctly mapped.
> 
> Update the commit (commit 0ad5136: hw/cxl/events: Add qmp interfaces to
> add/release dynamic capacity ext…) with the following changes to fix the issue.

Hi Fan,

I hit this whilst reusing some of your code for the fmapi initiate add DC
command.   Try adding an empty extent list..

"extents": []

boom.

Definitely need to reject that input :)

Jonathan


> 
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index ea88b53f41..d51f2ef9f5 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1978,7 +1978,7 @@ static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
> 		 extents[i].shared_seq = 0;
> 
> 		 /* No duplicate or overlapped extents are allowed */
> -        dpa -= dcd->dc.regions[0].base;
> +        dpa = dpa + dcd->dc.regions[rid].base - dcd->dc.regions[0].base;
> 		 if (test_any_bits_set(blk_bitmap, dpa / min_block_size,
> 			 len / min_block_size)) {
> 			 error_setg(errp, "duplicate or overlapped extents are detected");
> 
> 
> Fan
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Questions about the qemu DCD support in cxl-2023-09-13
  2023-10-26 16:58           ` Jonathan Cameron
@ 2023-10-26 16:58             ` Jonathan Cameron
  2023-10-27 16:34             ` fan
  1 sibling, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2023-10-26 16:58 UTC (permalink / raw)
  To: fan, linux-cxl, a.manzanares
  Cc: Ira Weiny, Fan Ni, Singh, Naveen, dave, nmtadam.samsung

On Thu, 26 Oct 2023 10:01:21 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Tue, 24 Oct 2023 22:25:53 -0700
> fan <nifan.cxl@gmail.com> wrote:
> 
> > On Tue, Oct 24, 2023 at 11:56:02AM -0700, fan wrote:  
> > > On Mon, Oct 23, 2023 at 12:48:02PM -0700, fan wrote:    
> > > > On Thu, Sep 21, 2023 at 03:24:26PM -0700, Ira Weiny wrote:    
> > > > > Fan,
> > > > > 
> > > > > I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]
> > > > > 
> > > > > I've been testing various things and so far I have a couple of questions.
> > > > > 
> > > > > 1) If the qmp command is used to add extents which overlap other extents
> > > > >    shouldn't that throw an error?  I don't see any validation of this and
> > > > >    I would think a real device would reject such a request from the FM.
> > > > > 
> > > > > 2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
> > > > >    extents prior to driver load does not seem to work.  And I think this
> > > > >    is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().
> > > > > 
> > > > > Ira
> > > > > 
> > > > > [1] https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13    
> > > > 
> > > > Hi Ira,
> > > > FYI. I have updated the DCD emulation patch series based on feedbacks on
> > > > the previous version.
> > > > 
> > > > The new version is here:
> > > > https://github.com/moking/qemu-jic-clone/tree/dcd-dev
> > > > 
> > > > The code is based on Jonathan's branch cxl-2023-09-26.
> > > > 
> > > > The main changes include,
> > > > 
> > > > 1. Update cxl_find_dc_region to detect the case the range of
> > > > the extent cross multiple DC regions.
> > > > 2. Add comments to explain the checks performed in function
> > > > cxl_detect_malformed_extent_list. (Jonathan)
> > > > 3. Minimize the checks in cmd_dcd_add_dyn_cap_rsp.(Jonathan)
> > > > 4. Update total_extent_count in add/release dynamic capacity response function.
> > > > (Ira and Jorgen Hansen).
> > > > 5. Fix the logic issue in test_bits and renamed it to
> > > > test_any_bits_set to clear its function.
> > > > 6. Add pending extent list for add/release extent event.
> > > > 7. When add/release extent response is received, use the pending list to
> > > > verify the extents are valid.
> > > > 8. Add test_any_bits_set and cxl_insert_extent_to_extent_list declaration to
> > > > cxl_device.h so it can be used in different files.
> > > > 9. Updated ct3d_qmp_cxl_event_log_enc to include dynamic capacity event log type.
> > > > 10. Extract the functionality to delete extent from extent list to a helper
> > > > function.
> > > > 11. Move the update of the bitmap which reflects which blocks are backed with
> > > > dc extents from the moment when a dc extent is offered to the moment when it
> > > > is accepted from the host.
> > > > 
> > > > I was able to test the DCD code you sent previously, let me know if you
> > > > find any issues.
> > > > 
> > > > Fan    
> > > 
> > > FYI. 
> > > 
> > > Updated the last two commits to drop the extents for pending to
> > > release as the host can proactively release dc extents so there can be
> > > no pending-to-release extent list on the device side.
> > > 
> > > Fan    
> > 
> > Ira located a bug when testing dc extent adding with his latest DCD kernel
> > code.
> > The dpa passed in the qmp command is offset relative to the base of the
> > region where the extent is offered, we need to convert it to address relative
> > to the base address of the base of the region0 so the bit in the bitmap can
> > be correctly mapped.
> > 
> > Update the commit (commit 0ad5136: hw/cxl/events: Add qmp interfaces to
> > add/release dynamic capacity ext…) with the following changes to fix the issue.  
> 
> Hi Fan,
> 
> I hit this whilst reusing some of your code for the fmapi initiate add DC
> command.   Try adding an empty extent list..
> 
> "extents": []
> 
> boom.
> 
> Definitely need to reject that input :)

Fan,

Another thing I noticed...

I would not provide a more flexible QMP interface than the FMAPI interfaces.
The key thing I've noticed so far is no need to provide option for a list
of extents in different regions.  Just provide them as separate commands if that
is what is wanted.   That is drop the region-id in QMP out of the extent and
put alongside path in the top level arguements.

{ "execute": "cxl-add-dynamic-capcity",
  "arguements": {
	"path": "cxl-mem1",
	"region-id": 3,
	"extents": [ { "dpa": 0, "len": 6 } ] }}
  }



> 
> Jonathan
> 
> 
> > 
> > 
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index ea88b53f41..d51f2ef9f5 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -1978,7 +1978,7 @@ static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
> > 		 extents[i].shared_seq = 0;
> > 
> > 		 /* No duplicate or overlapped extents are allowed */
> > -        dpa -= dcd->dc.regions[0].base;
> > +        dpa = dpa + dcd->dc.regions[rid].base - dcd->dc.regions[0].base;
> > 		 if (test_any_bits_set(blk_bitmap, dpa / min_block_size,
> > 			 len / min_block_size)) {
> > 			 error_setg(errp, "duplicate or overlapped extents are detected");
> > 
> > 
> > Fan
> >   
> 
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Questions about the qemu DCD support in cxl-2023-09-13
  2023-10-26  9:01         ` Jonathan Cameron
@ 2023-10-26 16:58           ` Jonathan Cameron
  2023-10-26 16:58             ` Jonathan Cameron
  2023-10-27 16:34             ` fan
  0 siblings, 2 replies; 11+ messages in thread
From: Jonathan Cameron @ 2023-10-26 16:58 UTC (permalink / raw)
  To: fan, linux-cxl, a.manzanares
  Cc: Ira Weiny, Fan Ni, Singh, Naveen, dave, nmtadam.samsung

On Thu, 26 Oct 2023 10:01:21 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Tue, 24 Oct 2023 22:25:53 -0700
> fan <nifan.cxl@gmail.com> wrote:
> 
> > On Tue, Oct 24, 2023 at 11:56:02AM -0700, fan wrote:  
> > > On Mon, Oct 23, 2023 at 12:48:02PM -0700, fan wrote:    
> > > > On Thu, Sep 21, 2023 at 03:24:26PM -0700, Ira Weiny wrote:    
> > > > > Fan,
> > > > > 
> > > > > I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]
> > > > > 
> > > > > I've been testing various things and so far I have a couple of questions.
> > > > > 
> > > > > 1) If the qmp command is used to add extents which overlap other extents
> > > > >    shouldn't that throw an error?  I don't see any validation of this and
> > > > >    I would think a real device would reject such a request from the FM.
> > > > > 
> > > > > 2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
> > > > >    extents prior to driver load does not seem to work.  And I think this
> > > > >    is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().
> > > > > 
> > > > > Ira
> > > > > 
> > > > > [1] https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13    
> > > > 
> > > > Hi Ira,
> > > > FYI. I have updated the DCD emulation patch series based on feedbacks on
> > > > the previous version.
> > > > 
> > > > The new version is here:
> > > > https://github.com/moking/qemu-jic-clone/tree/dcd-dev
> > > > 
> > > > The code is based on Jonathan's branch cxl-2023-09-26.
> > > > 
> > > > The main changes include,
> > > > 
> > > > 1. Update cxl_find_dc_region to detect the case the range of
> > > > the extent cross multiple DC regions.
> > > > 2. Add comments to explain the checks performed in function
> > > > cxl_detect_malformed_extent_list. (Jonathan)
> > > > 3. Minimize the checks in cmd_dcd_add_dyn_cap_rsp.(Jonathan)
> > > > 4. Update total_extent_count in add/release dynamic capacity response function.
> > > > (Ira and Jorgen Hansen).
> > > > 5. Fix the logic issue in test_bits and renamed it to
> > > > test_any_bits_set to clear its function.
> > > > 6. Add pending extent list for add/release extent event.
> > > > 7. When add/release extent response is received, use the pending list to
> > > > verify the extents are valid.
> > > > 8. Add test_any_bits_set and cxl_insert_extent_to_extent_list declaration to
> > > > cxl_device.h so it can be used in different files.
> > > > 9. Updated ct3d_qmp_cxl_event_log_enc to include dynamic capacity event log type.
> > > > 10. Extract the functionality to delete extent from extent list to a helper
> > > > function.
> > > > 11. Move the update of the bitmap which reflects which blocks are backed with
> > > > dc extents from the moment when a dc extent is offered to the moment when it
> > > > is accepted from the host.
> > > > 
> > > > I was able to test the DCD code you sent previously, let me know if you
> > > > find any issues.
> > > > 
> > > > Fan    
> > > 
> > > FYI. 
> > > 
> > > Updated the last two commits to drop the extents for pending to
> > > release as the host can proactively release dc extents so there can be
> > > no pending-to-release extent list on the device side.
> > > 
> > > Fan    
> > 
> > Ira located a bug when testing dc extent adding with his latest DCD kernel
> > code.
> > The dpa passed in the qmp command is offset relative to the base of the
> > region where the extent is offered, we need to convert it to address relative
> > to the base address of the base of the region0 so the bit in the bitmap can
> > be correctly mapped.
> > 
> > Update the commit (commit 0ad5136: hw/cxl/events: Add qmp interfaces to
> > add/release dynamic capacity ext…) with the following changes to fix the issue.  
> 
> Hi Fan,
> 
> I hit this whilst reusing some of your code for the fmapi initiate add DC
> command.   Try adding an empty extent list..
> 
> "extents": []
> 
> boom.
> 
> Definitely need to reject that input :)

Fan,

Another thing I noticed...

I would not provide a more flexible QMP interface than the FMAPI interfaces.
The key thing I've noticed so far is no need to provide option for a list
of extents in different regions.  Just provide them as separate commands if that
is what is wanted.   That is drop the region-id in QMP out of the extent and
put alongside path in the top level arguements.

{ "execute": "cxl-add-dynamic-capcity",
  "arguements": {
	"path": "cxl-mem1",
	"region-id": 3,
	"extents": [ { "dpa": 0, "len": 6 } ] }}
  }



> 
> Jonathan
> 
> 
> > 
> > 
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index ea88b53f41..d51f2ef9f5 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -1978,7 +1978,7 @@ static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
> > 		 extents[i].shared_seq = 0;
> > 
> > 		 /* No duplicate or overlapped extents are allowed */
> > -        dpa -= dcd->dc.regions[0].base;
> > +        dpa = dpa + dcd->dc.regions[rid].base - dcd->dc.regions[0].base;
> > 		 if (test_any_bits_set(blk_bitmap, dpa / min_block_size,
> > 			 len / min_block_size)) {
> > 			 error_setg(errp, "duplicate or overlapped extents are detected");
> > 
> > 
> > Fan
> >   
> 
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Questions about the qemu DCD support in cxl-2023-09-13
  2023-10-26 16:58           ` Jonathan Cameron
  2023-10-26 16:58             ` Jonathan Cameron
@ 2023-10-27 16:34             ` fan
  2023-10-31 17:18               ` Jonathan Cameron
  1 sibling, 1 reply; 11+ messages in thread
From: fan @ 2023-10-27 16:34 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: fan, linux-cxl, a.manzanares, Ira Weiny, Fan Ni, Singh, Naveen,
	dave, nmtadam.samsung

On Thu, Oct 26, 2023 at 05:58:02PM +0100, Jonathan Cameron wrote:
> On Thu, 26 Oct 2023 10:01:21 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Tue, 24 Oct 2023 22:25:53 -0700
> > fan <nifan.cxl@gmail.com> wrote:
> > 
> > > On Tue, Oct 24, 2023 at 11:56:02AM -0700, fan wrote:  
> > > > On Mon, Oct 23, 2023 at 12:48:02PM -0700, fan wrote:    
> > > > > On Thu, Sep 21, 2023 at 03:24:26PM -0700, Ira Weiny wrote:    
> > > > > > Fan,
> > > > > > 
> > > > > > I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]
> > > > > > 
> > > > > > I've been testing various things and so far I have a couple of questions.
> > > > > > 
> > > > > > 1) If the qmp command is used to add extents which overlap other extents
> > > > > >    shouldn't that throw an error?  I don't see any validation of this and
> > > > > >    I would think a real device would reject such a request from the FM.
> > > > > > 
> > > > > > 2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
> > > > > >    extents prior to driver load does not seem to work.  And I think this
> > > > > >    is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().
> > > > > > 
> > > > > > Ira
> > > > > > 
> > > > > > [1] https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13    
> > > > > 
> > > > > Hi Ira,
> > > > > FYI. I have updated the DCD emulation patch series based on feedbacks on
> > > > > the previous version.
> > > > > 
> > > > > The new version is here:
> > > > > https://github.com/moking/qemu-jic-clone/tree/dcd-dev
> > > > > 
> > > > > The code is based on Jonathan's branch cxl-2023-09-26.
> > > > > 
> > > > > The main changes include,
> > > > > 
> > > > > 1. Update cxl_find_dc_region to detect the case the range of
> > > > > the extent cross multiple DC regions.
> > > > > 2. Add comments to explain the checks performed in function
> > > > > cxl_detect_malformed_extent_list. (Jonathan)
> > > > > 3. Minimize the checks in cmd_dcd_add_dyn_cap_rsp.(Jonathan)
> > > > > 4. Update total_extent_count in add/release dynamic capacity response function.
> > > > > (Ira and Jorgen Hansen).
> > > > > 5. Fix the logic issue in test_bits and renamed it to
> > > > > test_any_bits_set to clear its function.
> > > > > 6. Add pending extent list for add/release extent event.
> > > > > 7. When add/release extent response is received, use the pending list to
> > > > > verify the extents are valid.
> > > > > 8. Add test_any_bits_set and cxl_insert_extent_to_extent_list declaration to
> > > > > cxl_device.h so it can be used in different files.
> > > > > 9. Updated ct3d_qmp_cxl_event_log_enc to include dynamic capacity event log type.
> > > > > 10. Extract the functionality to delete extent from extent list to a helper
> > > > > function.
> > > > > 11. Move the update of the bitmap which reflects which blocks are backed with
> > > > > dc extents from the moment when a dc extent is offered to the moment when it
> > > > > is accepted from the host.
> > > > > 
> > > > > I was able to test the DCD code you sent previously, let me know if you
> > > > > find any issues.
> > > > > 
> > > > > Fan    
> > > > 
> > > > FYI. 
> > > > 
> > > > Updated the last two commits to drop the extents for pending to
> > > > release as the host can proactively release dc extents so there can be
> > > > no pending-to-release extent list on the device side.
> > > > 
> > > > Fan    
> > > 
> > > Ira located a bug when testing dc extent adding with his latest DCD kernel
> > > code.
> > > The dpa passed in the qmp command is offset relative to the base of the
> > > region where the extent is offered, we need to convert it to address relative
> > > to the base address of the base of the region0 so the bit in the bitmap can
> > > be correctly mapped.
> > > 
> > > Update the commit (commit 0ad5136: hw/cxl/events: Add qmp interfaces to
> > > add/release dynamic capacity ext…) with the following changes to fix the issue.  
> > 
> > Hi Fan,
> > 
> > I hit this whilst reusing some of your code for the fmapi initiate add DC
> > command.   Try adding an empty extent list..
> > 
> > "extents": []
> > 
> > boom.
> > 
> > Definitely need to reject that input :)
> 
> Fan,
> 
> Another thing I noticed...
> 
> I would not provide a more flexible QMP interface than the FMAPI interfaces.
> The key thing I've noticed so far is no need to provide option for a list
> of extents in different regions.  Just provide them as separate commands if that
> is what is wanted.   That is drop the region-id in QMP out of the extent and
> put alongside path in the top level arguements.
> 
> { "execute": "cxl-add-dynamic-capcity",
>   "arguements": {
> 	"path": "cxl-mem1",
> 	"region-id": 3,
> 	"extents": [ { "dpa": 0, "len": 6 } ] }}
>   }
> 
> 
> 
> > 
> > Jonathan

Hi Jonathan,

I updated my branch with the following changes:
1. Add code to detect and reject QMP requests without any extents;
2. Add code to detect and reject QMP requests where the extent len is 0.
3. Change the QMP interface as you suggested above and moved the
region-id out of extents and now each command only takes care of extent
add/release request in a single region.
4. Change the region bitmap length from decode_len to len.

Fan

> > 
> > 
> > > 
> > > 
> > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > index ea88b53f41..d51f2ef9f5 100644
> > > --- a/hw/mem/cxl_type3.c
> > > +++ b/hw/mem/cxl_type3.c
> > > @@ -1978,7 +1978,7 @@ static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
> > > 		 extents[i].shared_seq = 0;
> > > 
> > > 		 /* No duplicate or overlapped extents are allowed */
> > > -        dpa -= dcd->dc.regions[0].base;
> > > +        dpa = dpa + dcd->dc.regions[rid].base - dcd->dc.regions[0].base;
> > > 		 if (test_any_bits_set(blk_bitmap, dpa / min_block_size,
> > > 			 len / min_block_size)) {
> > > 			 error_setg(errp, "duplicate or overlapped extents are detected");
> > > 
> > > 
> > > Fan
> > >   
> > 
> > 
> > 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Questions about the qemu DCD support in cxl-2023-09-13
  2023-10-27 16:34             ` fan
@ 2023-10-31 17:18               ` Jonathan Cameron
  2023-10-31 17:44                 ` fan
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2023-10-31 17:18 UTC (permalink / raw)
  To: fan
  Cc: linux-cxl, a.manzanares, Ira Weiny, Fan Ni, Singh, Naveen, dave,
	nmtadam.samsung

On Fri, 27 Oct 2023 09:34:02 -0700
fan <nifan.cxl@gmail.com> wrote:

> On Thu, Oct 26, 2023 at 05:58:02PM +0100, Jonathan Cameron wrote:
> > On Thu, 26 Oct 2023 10:01:21 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Tue, 24 Oct 2023 22:25:53 -0700
> > > fan <nifan.cxl@gmail.com> wrote:
> > >   
> > > > On Tue, Oct 24, 2023 at 11:56:02AM -0700, fan wrote:    
> > > > > On Mon, Oct 23, 2023 at 12:48:02PM -0700, fan wrote:      
> > > > > > On Thu, Sep 21, 2023 at 03:24:26PM -0700, Ira Weiny wrote:      
> > > > > > > Fan,
> > > > > > > 
> > > > > > > I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]
> > > > > > > 
> > > > > > > I've been testing various things and so far I have a couple of questions.
> > > > > > > 
> > > > > > > 1) If the qmp command is used to add extents which overlap other extents
> > > > > > >    shouldn't that throw an error?  I don't see any validation of this and
> > > > > > >    I would think a real device would reject such a request from the FM.
> > > > > > > 
> > > > > > > 2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
> > > > > > >    extents prior to driver load does not seem to work.  And I think this
> > > > > > >    is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().
> > > > > > > 
> > > > > > > Ira
> > > > > > > 
> > > > > > > [1] https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13      
> > > > > > 
> > > > > > Hi Ira,
> > > > > > FYI. I have updated the DCD emulation patch series based on feedbacks on
> > > > > > the previous version.
> > > > > > 
> > > > > > The new version is here:
> > > > > > https://github.com/moking/qemu-jic-clone/tree/dcd-dev
> > > > > > 
> > > > > > The code is based on Jonathan's branch cxl-2023-09-26.
> > > > > > 
> > > > > > The main changes include,
> > > > > > 
> > > > > > 1. Update cxl_find_dc_region to detect the case the range of
> > > > > > the extent cross multiple DC regions.
> > > > > > 2. Add comments to explain the checks performed in function
> > > > > > cxl_detect_malformed_extent_list. (Jonathan)
> > > > > > 3. Minimize the checks in cmd_dcd_add_dyn_cap_rsp.(Jonathan)
> > > > > > 4. Update total_extent_count in add/release dynamic capacity response function.
> > > > > > (Ira and Jorgen Hansen).
> > > > > > 5. Fix the logic issue in test_bits and renamed it to
> > > > > > test_any_bits_set to clear its function.
> > > > > > 6. Add pending extent list for add/release extent event.
> > > > > > 7. When add/release extent response is received, use the pending list to
> > > > > > verify the extents are valid.
> > > > > > 8. Add test_any_bits_set and cxl_insert_extent_to_extent_list declaration to
> > > > > > cxl_device.h so it can be used in different files.
> > > > > > 9. Updated ct3d_qmp_cxl_event_log_enc to include dynamic capacity event log type.
> > > > > > 10. Extract the functionality to delete extent from extent list to a helper
> > > > > > function.
> > > > > > 11. Move the update of the bitmap which reflects which blocks are backed with
> > > > > > dc extents from the moment when a dc extent is offered to the moment when it
> > > > > > is accepted from the host.
> > > > > > 
> > > > > > I was able to test the DCD code you sent previously, let me know if you
> > > > > > find any issues.
> > > > > > 
> > > > > > Fan      
> > > > > 
> > > > > FYI. 
> > > > > 
> > > > > Updated the last two commits to drop the extents for pending to
> > > > > release as the host can proactively release dc extents so there can be
> > > > > no pending-to-release extent list on the device side.
> > > > > 
> > > > > Fan      
> > > > 
> > > > Ira located a bug when testing dc extent adding with his latest DCD kernel
> > > > code.
> > > > The dpa passed in the qmp command is offset relative to the base of the
> > > > region where the extent is offered, we need to convert it to address relative
> > > > to the base address of the base of the region0 so the bit in the bitmap can
> > > > be correctly mapped.
> > > > 
> > > > Update the commit (commit 0ad5136: hw/cxl/events: Add qmp interfaces to
> > > > add/release dynamic capacity ext…) with the following changes to fix the issue.    
> > > 
> > > Hi Fan,
> > > 
> > > I hit this whilst reusing some of your code for the fmapi initiate add DC
> > > command.   Try adding an empty extent list..
> > > 
> > > "extents": []
> > > 
> > > boom.
> > > 
> > > Definitely need to reject that input :)  
> > 
> > Fan,
> > 
> > Another thing I noticed...
> > 
> > I would not provide a more flexible QMP interface than the FMAPI interfaces.
> > The key thing I've noticed so far is no need to provide option for a list
> > of extents in different regions.  Just provide them as separate commands if that
> > is what is wanted.   That is drop the region-id in QMP out of the extent and
> > put alongside path in the top level arguements.
> > 
> > { "execute": "cxl-add-dynamic-capcity",
> >   "arguements": {
> > 	"path": "cxl-mem1",
> > 	"region-id": 3,
> > 	"extents": [ { "dpa": 0, "len": 6 } ] }}
> >   }
> > 
> > 
> >   
> > > 
> > > Jonathan  
> 
> Hi Jonathan,
> 
> I updated my branch with the following changes:
> 1. Add code to detect and reject QMP requests without any extents;
> 2. Add code to detect and reject QMP requests where the extent len is 0.
> 3. Change the QMP interface as you suggested above and moved the
> region-id out of extents and now each command only takes care of extent
> add/release request in a single region.
> 4. Change the region bitmap length from decode_len to len.

Great. Looking at what I think is your newest implementation.

dpa is used qmp_cxl_process_dynamic_capacity() for various things that aren't
the device physical address (aren't relative to start of device).
That's confusing at times.  The FMAPI commands are all in DPA so I got a bit
lost.  I think you should do all the checks based on actual DPA (so for some
you will then need to subtract the region base).

Now you only have one region, you can do the checking of overlap in that function
in one bit per block_size - whatever the block size happens to be.

Otherwise this bit looks good to me.  My initial FMAPI stuff is going to
be a bit of a hack on top, but the aim is to illustrate where there
might be shared code rather than end up with a proper clean result.

Jonathan

> 
> Fan
> 
> > > 
> > >   
> > > > 
> > > > 
> > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > > index ea88b53f41..d51f2ef9f5 100644
> > > > --- a/hw/mem/cxl_type3.c
> > > > +++ b/hw/mem/cxl_type3.c
> > > > @@ -1978,7 +1978,7 @@ static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
> > > > 		 extents[i].shared_seq = 0;
> > > > 
> > > > 		 /* No duplicate or overlapped extents are allowed */
> > > > -        dpa -= dcd->dc.regions[0].base;
> > > > +        dpa = dpa + dcd->dc.regions[rid].base - dcd->dc.regions[0].base;
> > > > 		 if (test_any_bits_set(blk_bitmap, dpa / min_block_size,
> > > > 			 len / min_block_size)) {
> > > > 			 error_setg(errp, "duplicate or overlapped extents are detected");
> > > > 
> > > > 
> > > > Fan
> > > >     
> > > 
> > > 
> > >   
> >   
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Questions about the qemu DCD support in cxl-2023-09-13
  2023-10-31 17:18               ` Jonathan Cameron
@ 2023-10-31 17:44                 ` fan
  0 siblings, 0 replies; 11+ messages in thread
From: fan @ 2023-10-31 17:44 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: fan, linux-cxl, a.manzanares, Ira Weiny, Fan Ni, Singh, Naveen,
	dave, nmtadam.samsung

On Tue, Oct 31, 2023 at 05:18:46PM +0000, Jonathan Cameron wrote:
> On Fri, 27 Oct 2023 09:34:02 -0700
> fan <nifan.cxl@gmail.com> wrote:
> 
> > On Thu, Oct 26, 2023 at 05:58:02PM +0100, Jonathan Cameron wrote:
> > > On Thu, 26 Oct 2023 10:01:21 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >   
> > > > On Tue, 24 Oct 2023 22:25:53 -0700
> > > > fan <nifan.cxl@gmail.com> wrote:
> > > >   
> > > > > On Tue, Oct 24, 2023 at 11:56:02AM -0700, fan wrote:    
> > > > > > On Mon, Oct 23, 2023 at 12:48:02PM -0700, fan wrote:      
> > > > > > > On Thu, Sep 21, 2023 at 03:24:26PM -0700, Ira Weiny wrote:      
> > > > > > > > Fan,
> > > > > > > > 
> > > > > > > > I'm working off of Jonathan's latest CXL branch with the DCD patches.[1]
> > > > > > > > 
> > > > > > > > I've been testing various things and so far I have a couple of questions.
> > > > > > > > 
> > > > > > > > 1) If the qmp command is used to add extents which overlap other extents
> > > > > > > >    shouldn't that throw an error?  I don't see any validation of this and
> > > > > > > >    I would think a real device would reject such a request from the FM.
> > > > > > > > 
> > > > > > > > 2) Where is CXLType3Dev->dc.total_extent_count set?  Attempting to add
> > > > > > > >    extents prior to driver load does not seem to work.  And I think this
> > > > > > > >    is because total_extent_count is 0 in cmd_dcd_get_dyn_cap_ext_list().
> > > > > > > > 
> > > > > > > > Ira
> > > > > > > > 
> > > > > > > > [1] https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13      
> > > > > > > 
> > > > > > > Hi Ira,
> > > > > > > FYI. I have updated the DCD emulation patch series based on feedbacks on
> > > > > > > the previous version.
> > > > > > > 
> > > > > > > The new version is here:
> > > > > > > https://github.com/moking/qemu-jic-clone/tree/dcd-dev
> > > > > > > 
> > > > > > > The code is based on Jonathan's branch cxl-2023-09-26.
> > > > > > > 
> > > > > > > The main changes include,
> > > > > > > 
> > > > > > > 1. Update cxl_find_dc_region to detect the case the range of
> > > > > > > the extent cross multiple DC regions.
> > > > > > > 2. Add comments to explain the checks performed in function
> > > > > > > cxl_detect_malformed_extent_list. (Jonathan)
> > > > > > > 3. Minimize the checks in cmd_dcd_add_dyn_cap_rsp.(Jonathan)
> > > > > > > 4. Update total_extent_count in add/release dynamic capacity response function.
> > > > > > > (Ira and Jorgen Hansen).
> > > > > > > 5. Fix the logic issue in test_bits and renamed it to
> > > > > > > test_any_bits_set to clear its function.
> > > > > > > 6. Add pending extent list for add/release extent event.
> > > > > > > 7. When add/release extent response is received, use the pending list to
> > > > > > > verify the extents are valid.
> > > > > > > 8. Add test_any_bits_set and cxl_insert_extent_to_extent_list declaration to
> > > > > > > cxl_device.h so it can be used in different files.
> > > > > > > 9. Updated ct3d_qmp_cxl_event_log_enc to include dynamic capacity event log type.
> > > > > > > 10. Extract the functionality to delete extent from extent list to a helper
> > > > > > > function.
> > > > > > > 11. Move the update of the bitmap which reflects which blocks are backed with
> > > > > > > dc extents from the moment when a dc extent is offered to the moment when it
> > > > > > > is accepted from the host.
> > > > > > > 
> > > > > > > I was able to test the DCD code you sent previously, let me know if you
> > > > > > > find any issues.
> > > > > > > 
> > > > > > > Fan      
> > > > > > 
> > > > > > FYI. 
> > > > > > 
> > > > > > Updated the last two commits to drop the extents for pending to
> > > > > > release as the host can proactively release dc extents so there can be
> > > > > > no pending-to-release extent list on the device side.
> > > > > > 
> > > > > > Fan      
> > > > > 
> > > > > Ira located a bug when testing dc extent adding with his latest DCD kernel
> > > > > code.
> > > > > The dpa passed in the qmp command is offset relative to the base of the
> > > > > region where the extent is offered, we need to convert it to address relative
> > > > > to the base address of the base of the region0 so the bit in the bitmap can
> > > > > be correctly mapped.
> > > > > 
> > > > > Update the commit (commit 0ad5136: hw/cxl/events: Add qmp interfaces to
> > > > > add/release dynamic capacity ext…) with the following changes to fix the issue.    
> > > > 
> > > > Hi Fan,
> > > > 
> > > > I hit this whilst reusing some of your code for the fmapi initiate add DC
> > > > command.   Try adding an empty extent list..
> > > > 
> > > > "extents": []
> > > > 
> > > > boom.
> > > > 
> > > > Definitely need to reject that input :)  
> > > 
> > > Fan,
> > > 
> > > Another thing I noticed...
> > > 
> > > I would not provide a more flexible QMP interface than the FMAPI interfaces.
> > > The key thing I've noticed so far is no need to provide option for a list
> > > of extents in different regions.  Just provide them as separate commands if that
> > > is what is wanted.   That is drop the region-id in QMP out of the extent and
> > > put alongside path in the top level arguements.
> > > 
> > > { "execute": "cxl-add-dynamic-capcity",
> > >   "arguements": {
> > > 	"path": "cxl-mem1",
> > > 	"region-id": 3,
> > > 	"extents": [ { "dpa": 0, "len": 6 } ] }}
> > >   }
> > > 
> > > 
> > >   
> > > > 
> > > > Jonathan  
> > 
> > Hi Jonathan,
> > 
> > I updated my branch with the following changes:
> > 1. Add code to detect and reject QMP requests without any extents;
> > 2. Add code to detect and reject QMP requests where the extent len is 0.
> > 3. Change the QMP interface as you suggested above and moved the
> > region-id out of extents and now each command only takes care of extent
> > add/release request in a single region.
> > 4. Change the region bitmap length from decode_len to len.
> 
> Great. Looking at what I think is your newest implementation.
> 
> dpa is used qmp_cxl_process_dynamic_capacity() for various things that aren't
> the device physical address (aren't relative to start of device).
> That's confusing at times.  The FMAPI commands are all in DPA so I got a bit
> lost.  I think you should do all the checks based on actual DPA (so for some
> you will then need to subtract the region base).

The dpa is only used as the offset to the base of current region, not to the
start of the device. We can change it to offset if it causes confusion.

> 
> Now you only have one region, you can do the checking of overlap in that function
> in one bit per block_size - whatever the block size happens to be.

All the checks now are within a single region, and we use one bit per
block_size already in the code.

...

blk_bitmap = bitmap_new(dcd->dc.regions[rid].len / block_size);

https://github.com/moking/qemu-jic-clone/blob/dcd-dev/hw/mem/cxl_type3.c#L2044

One more thing, I forgot to update the interface in the stub.c file for
the new QMP interface, will update.

Fan


...

> 
> Otherwise this bit looks good to me.  My initial FMAPI stuff is going to
> be a bit of a hack on top, but the aim is to illustrate where there
> might be shared code rather than end up with a proper clean result.
> 
> Jonathan
> 
> > 
> > Fan
> > 
> > > > 
> > > >   
> > > > > 
> > > > > 
> > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > > > index ea88b53f41..d51f2ef9f5 100644
> > > > > --- a/hw/mem/cxl_type3.c
> > > > > +++ b/hw/mem/cxl_type3.c
> > > > > @@ -1978,7 +1978,7 @@ static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
> > > > > 		 extents[i].shared_seq = 0;
> > > > > 
> > > > > 		 /* No duplicate or overlapped extents are allowed */
> > > > > -        dpa -= dcd->dc.regions[0].base;
> > > > > +        dpa = dpa + dcd->dc.regions[rid].base - dcd->dc.regions[0].base;
> > > > > 		 if (test_any_bits_set(blk_bitmap, dpa / min_block_size,
> > > > > 			 len / min_block_size)) {
> > > > > 			 error_setg(errp, "duplicate or overlapped extents are detected");
> > > > > 
> > > > > 
> > > > > Fan
> > > > >     
> > > > 
> > > > 
> > > >   
> > >   
> > 
> > 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-10-31 17:44 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <CGME20230921222437uscas1p158e5ceecab50dcb39e33811f567152d8@uscas1p1.samsung.com>
2023-09-21 22:24 ` Questions about the qemu DCD support in cxl-2023-09-13 Ira Weiny
2023-09-21 22:51   ` Fan Ni
2023-10-23 19:48   ` fan
2023-10-24 18:56     ` fan
2023-10-25  5:25       ` fan
2023-10-26  9:01         ` Jonathan Cameron
2023-10-26 16:58           ` Jonathan Cameron
2023-10-26 16:58             ` Jonathan Cameron
2023-10-27 16:34             ` fan
2023-10-31 17:18               ` Jonathan Cameron
2023-10-31 17:44                 ` fan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox