On Wed, Mar 06, 2024 at 05:28:27PM +0000, Jonathan Cameron wrote:
> On Mon, 4 Mar 2024 11:34:03 -0800
> nifan....@gmail.com wrote:
>
> > From: Fan Ni <fan...@samsung.com>
> >
> > Per CXL spec 3.1, two mailbox commands are implemented:
> > Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> > Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
> >
> > Signed-off-by: Fan Ni <fan...@samsung.com>
>
> Hmm. So I had a thought which would work for what you
> have here. See include/qemu/range.h
> I like the region merging stuff that is also in the list operators
> but we shouldn't use that because we have other reasons not to
> fuse ranges (sequence numbering etc)
>
> We could make an extent a wrapper around a struct Range though
> so that we can use the comparison stuff directly.
> + we can use the list manipulation in there as the basis for a future
> extent merging infrastructure that is tag and sequence number (if
> provided - so shared capacity or pmem) aware.
>
> Jonathan
>
>
> > ---
> > +
> > +/*
> > + * CXL r3.1 Table 8-168: Add Dynamic Capacity Response Input Payload
> > + * CXL r3.1 Table 8-170: Release Dynamic Capacity Input Payload
> > + */
> > +typedef struct CXLUpdateDCExtentListInPl {
> > + uint32_t num_entries_updated;
> > + uint8_t flags;
> > + uint8_t rsvd[3];
> > + /* CXL r3.1 Table 8-169: Updated Extent */
> > + struct {
> > + uint64_t start_dpa;
> > + uint64_t len;
> > + uint8_t rsvd[8];
> > + } QEMU_PACKED updated_entries[];
> > +} QEMU_PACKED CXLUpdateDCExtentListInPl;
> > +
> > +/*
> > + * For the extents in the extent list to operate, check whether they are
> > valid
> > + * 1. The extent should be in the range of a valid DC region;
> > + * 2. The extent should not cross multiple regions;
> > + * 3. The start DPA and the length of the extent should align with the
> > block
> > + * size of the region;
> > + * 4. The address range of multiple extents in the list should not overlap.
>
> Hmm. Interesting. I was thinking a given add / remove command rather than
> just the extents can't overlap a region. However I can't find text on that
> so I believe your interpretation is correct. It is only specified for the
> event records, but that is good enough I think. We might want to propose
> tightening the spec on this to allow devices to say no to such complex
> extent lists. Maybe a nice friendly Memory vendor should query this one if
> it's a potential problem for real devices. Might not be!
>
> > + */
> > +static CXLRetCode cxl_detect_malformed_extent_list(CXLType3Dev *ct3d,
> > + const CXLUpdateDCExtentListInPl *in)
> > +{
> > + uint64_t min_block_size = UINT64_MAX;
> > + CXLDCRegion *region = &ct3d->dc.regions[0];
> > + CXLDCRegion *lastregion = &ct3d->dc.regions[ct3d->dc.num_regions - 1];
> > + g_autofree unsigned long *blk_bitmap = NULL;
> > + uint64_t dpa, len;
> > + uint32_t i;
> > +
> > + for (i = 0; i < ct3d->dc.num_regions; i++) {
> > + region = &ct3d->dc.regions[i];
> > + min_block_size = MIN(min_block_size, region->block_size);
> > + }
> > +
> > + blk_bitmap = bitmap_new((lastregion->base + lastregion->len -
> > + ct3d->dc.regions[0].base) / min_block_size);
> > +
> > + for (i = 0; i < in->num_entries_updated; i++) {
> > + dpa = in->updated_entries[i].start_dpa;
> > + len = in->updated_entries[i].len;
> > +
> > + region = cxl_find_dc_region(ct3d, dpa, len);
> > + if (!region) {
> > + return CXL_MBOX_INVALID_PA;
> > + }
> > +
> > + dpa -= ct3d->dc.regions[0].base;
> > + if (dpa % region->block_size || len % region->block_size) {
> > + return CXL_MBOX_INVALID_EXTENT_LIST;
> > + }
> > + /* the dpa range already covered by some other extents in the list
> > */
> > + if (test_any_bits_set(blk_bitmap, dpa / min_block_size,
> > + len / min_block_size)) {
> > + return CXL_MBOX_INVALID_EXTENT_LIST;
> > + }
> > + bitmap_set(blk_bitmap, dpa / min_block_size, len / min_block_size);
> > + }
> > +
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
> > +/*
> > + * CXL r3.1 section 8.2.9.9.9.3: Add Dynamic Capacity Response (Opcode
> > 4802h)
> > + * An extent is added to the extent list and becomes usable only after the
> > + * response is processed successfully
> > + */
> > +static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
> > + uint8_t *payload_in,
> > + size_t len_in,
> > + uint8_t *payload_out,
> > + size_t *len_out,
> > + CXLCCI *cci)
> > +{
> > + CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> > + CXLDCExtent *ent;
> > + uint32_t i;
> > + uint64_t dpa, len;
> > + CXLRetCode ret;
> > +
> > + if (in->num_entries_updated == 0) {
> > + return CXL_MBOX_SUCCESS;
> > + }
> > +
> > + /* Adding extents causes exceeding device's extent tracking ability. */
> > + if (in->num_entries_updated + ct3d->dc.total_extent_count >
> > + CXL_NUM_EXTENTS_SUPPORTED) {
> > + return CXL_MBOX_RESOURCES_EXHAUSTED;
> > + }
> > +
> > + ret = cxl_detect_malformed_extent_list(ct3d, in);
> > + if (ret != CXL_MBOX_SUCCESS) {
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < in->num_entries_updated; i++) {
> > + dpa = in->updated_entries[i].start_dpa;
> > + len = in->updated_entries[i].len;
> > +
> > + /*
> > + * Check if the DPA range of the to-be-added extent overlaps with
> > + * existing extent list maintained by the device.
> > + */
> > + QTAILQ_FOREACH(ent, extent_list, node) {
>
> There are too many checks in here for an overlapping test.
>
> Conditions are
>
> | Extent tested against |
> | Overlap entirely |
> | overlap left edge |
> | overlap right edge |
> Think of it in the inverse condition and it is easier to reason about.
>
> | Extent tested against |
> | to left |--- ---| to right |
>
> which I think is something like.
>
> if (!((dpa + len <= ent->start_dpa) || (dpa >= ent->start_dpa +
> ent->len)) {
> return CXL_MBOX_INVALID_PA;
> }
>
> Hmm. For internal tracking (not the exposed values) we should probably use
> struct range from include/qemu/range.h.
> Felt like there had to be something better than doing this ourselves so I went
> looking. Note it uses inclusive upper bound so be careful with that!
>
> Advantage is we get this checks for free.
> https://elixir.bootlin.com/qemu/latest/source/include/qemu/range.h#L152
> range_overlaps_range()
>
> There are functions to set them up nicely for us and by base and size
> as well which should tidy that part up.
>
>
>
> > + if (ent->start_dpa <= dpa &&
> > + dpa + len <= ent->start_dpa + ent->len) {
> > + return CXL_MBOX_INVALID_PA;
> > + /* Overlapping one end of the other */
> > + } else if ((dpa < ent->start_dpa + ent->len &&
> > + dpa + len > ent->start_dpa + ent->len) ||
> > + (dpa < ent->start_dpa && dpa + len >
> > ent->start_dpa)) {
> > + return CXL_MBOX_INVALID_PA;
> > + }
> > + }
> > +
> > + /*
> > + * TODO: we will add a pending extent list based on event log
> > record
> > + * and verify the input response; also, the "More" flag is not
> > + * considered at the moment.
> > + */
> > +
> > + cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
> > + ct3d->dc.total_extent_count += 1;
> > + }
> > +
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
> > +/*
> > + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (Opcode 4803h)
> > + */
> > +static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> > + uint8_t *payload_in,
> > + size_t len_in,
> > + uint8_t *payload_out,
> > + size_t *len_out,
> > + CXLCCI *cci)
> > +{
> > + CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> > + CXLDCExtent *ent;
> > + uint32_t i;
> > + uint64_t dpa, len;
> > + CXLRetCode ret;
> > +
> > + if (in->num_entries_updated == 0) {
> > + return CXL_MBOX_INVALID_INPUT;
> > + }
> > +
> > + ret = cxl_detect_malformed_extent_list(ct3d, in);
> > + if (ret != CXL_MBOX_SUCCESS) {
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < in->num_entries_updated; i++) {
> > + bool found = false;
> > +
> > + dpa = in->updated_entries[i].start_dpa;
> > + len = in->updated_entries[i].len;
> > +
> > + QTAILQ_FOREACH(ent, extent_list, node) {
> > + /* Found the extent overlapping with */
> > + if (ent->start_dpa <= dpa && dpa < ent->start_dpa + ent->len) {
> > + if (dpa + len <= ent->start_dpa + ent->len) {
> > + /*
> > + * The incoming extent covers a portion of an extent
> > + * in the device extent list, remove only the
> > overlapping
> > + * portion, meaning
> > + * 1. the portions that are not covered by the incoming
> > + * extent at both end of the original extent will
> > become
> > + * new extents and inserted to the extent list; and
> > + * 2. the original extent is removed from the extent
> > list;
> > + * 3. DC extent count is updated accordingly.
> > + */
> > + uint64_t ent_start_dpa = ent->start_dpa;
> > + uint64_t ent_len = ent->len;
> > + uint64_t len1 = dpa - ent_start_dpa;
> > + uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
> > +
> > + /*
> > + * TODO: checking for possible extent overflow, will be
> > + * moved into a dedicated function of detecting extent
> > + * overflow.
> > + */
> > + if (len1 && len2 && ct3d->dc.total_extent_count ==
> > + CXL_NUM_EXTENTS_SUPPORTED) {
> > + return CXL_MBOX_RESOURCES_EXHAUSTED;
> > + }
> > +
> > + found = true;
> > + cxl_remove_extent_from_extent_list(extent_list, ent);
> > + ct3d->dc.total_extent_count -= 1;
> > +
> > + if (len1) {
> > + cxl_insert_extent_to_extent_list(extent_list,
> > + ent_start_dpa,
> > len1,
> > + NULL, 0);
> > + ct3d->dc.total_extent_count += 1;
> > + }
> > + if (len2) {
> > + cxl_insert_extent_to_extent_list(extent_list, dpa
> > + len,
> > + len2, NULL, 0);
> > + ct3d->dc.total_extent_count += 1;
> > + }
> > + break;
> Maybe this makes sense after the support below is added, but at this
> point in the series
> return CXL_MBOX_SUCCESS;
> then found isn't relevant so can drop that. Looks like you drop it later in
> the
> series anyway.
>
> > + } else {
> > + /*
> > + * TODO: we reject the attempt to remove an extent that
> > + * overlaps with multiple extents in the device for
> > now,
> > + * once the bitmap indicating whether a DPA range is
> > + * covered by valid extents is introduced, will allow
> > it.
> > + */
> > + return CXL_MBOX_INVALID_PA;
> > + }
> > + }
> > + }
> > +
> > + if (!found) {
> > + /* Try to remove a non-existing extent. */
> > + return CXL_MBOX_INVALID_PA;
> > + }
> > + }
> > +
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
>
> > static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 102fa8151e..dccfaaad3a 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -678,6 +678,16 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d,
> > Error **errp)
> > return true;
> > }
> >
> > +static void cxl_destroy_dc_regions(CXLType3Dev *ct3d)
> > +{
> > + CXLDCExtent *ent;
> > +
> > + while (!QTAILQ_EMPTY(&ct3d->dc.extents)) {
> > + ent = QTAILQ_FIRST(&ct3d->dc.extents);
> > + cxl_remove_extent_from_extent_list(&ct3d->dc.extents, ent);
>
> Isn't this same a something like.
> QTAILQ_FOREACH_SAFE(ent, &ct3d->dc.extents, node)) {
> cxl_remove_extent_from_extent_list(&ct3d->dc.extents, ent);
> //This wrapper is small enough I'd be tempted to just have the
> //code inline at the places it's called.
>
We will have more to release after we introduce pending list as well as
bitmap. Keep it?
Fan
> }
> > + }
> > +}
>
