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

[fan <nifan.cxl@gmail.com>]

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