Re: Questions about the qemu DCD support in cxl-2023-09-13

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

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
> >   
> 
> 
>