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

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

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
>