Re: [PATCH v6 11/12] hw/cxl/cxl-mailbox-utils: Add superset extent release mailbox support

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

On Fri, Apr 05, 2024 at 09:57:18AM +0000, Jørgen Hansen wrote:
> On 3/25/24 20:02, nifan.cxl@gmail.com wrote:
> > From: Fan Ni <fan.ni@samsung.com>
> > 
> > With the change, we extend the extent release mailbox command processing
> > to allow more flexible release. As long as the DPA range of the extent to
> > release is covered by accepted extent(s) in the device, the release can be
> > performed.
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> >   hw/cxl/cxl-mailbox-utils.c | 41 ++++++++++++++++++++++----------------
> >   1 file changed, 24 insertions(+), 17 deletions(-)
> > 
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index a0d2239176..3b7949c364 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -1674,6 +1674,12 @@ static CXLRetCode cxl_dc_extent_release_dry_run(CXLType3Dev *ct3d,
> >           dpa = in->updated_entries[i].start_dpa;
> >           len = in->updated_entries[i].len;
> > 
> > +        /* Check if the DPA range is not fully backed with valid extents */
> > +        if (!ct3_test_region_block_backed(ct3d, dpa, len)) {
> > +            ret = CXL_MBOX_INVALID_PA;
> > +            goto free_and_exit;
> > +        }
> 
> In cxl_dcd_add_dyn_cap_rsp_dry_run, the opposite check (all 0's in the 
> bitmap) could be used instead of looping through the full extent list 
> (and this also makes my previous comment about reusing the bitmap from 
> cxl_detect_malformed_extent_list irrelevant).

For adding, we need to make sure the incoming extents have no overlaps
with accepted extents, that means if any bit of the range is not 0, it
returns error. We cannot use !ct3_test_region_block_backed for the
purpose, as it return true when all 1s, not has 1s.

For the purpose, we need some function like
ct3_test_region_block_all_cleared or ct3_test_region_block_non_backed.
We do not have that in current code.
Checking bitmap is more performance efficient, but it introduces more
changes, so I will leave it as it is until there are more concerns.

Fan

> 
> > +        /* After this point, extent overflow is the only error can happen */
> >           while (len > 0) {
> >               QTAILQ_FOREACH(ent, &tmp_list, node) {
> >                   range_init_nofail(&range, ent->start_dpa, ent->len);
> > @@ -1713,25 +1719,27 @@ static CXLRetCode cxl_dc_extent_release_dry_run(CXLType3Dev *ct3d,
> >                               goto free_and_exit;
> >                           }
> >                       } else {
> > -                        /*
> > -                         * TODO: we reject the attempt to remove an extent
> > -                         * that overlaps with multiple extents in the device
> > -                         * for now, we will allow it once superset release
> > -                         * support is added.
> > -                         */
> > -                        ret = CXL_MBOX_INVALID_PA;
> > -                        goto free_and_exit;
> > +                        len1 = dpa - ent_start_dpa;
> > +                        len2 = 0;
> > +                        len_done = ent_len - len1 - len2;
> 
> You don't need len2 in the else statement.
> 
> Thanks,
> Jørgen
> 
> > +
> > +                        cxl_remove_extent_from_extent_list(&tmp_list, ent);
> > +                        cnt_delta--;
> > +                        if (len1) {
> > +                            cxl_insert_extent_to_extent_list(&tmp_list,
> > +                                                             ent_start_dpa,
> > +                                                             len1, NULL, 0);
> > +                            cnt_delta++;
> > +                        }
> >                       }
> > 
> >                       len -= len_done;
> > -                    /* len == 0 here until superset release is added */
> > +                    if (len) {
> > +                        dpa = ent_start_dpa + ent_len;
> > +                    }
> >                       break;
> >                   }
> >               }
> > -            if (len) {
> > -                ret = CXL_MBOX_INVALID_PA;
> > -                goto free_and_exit;
> > -            }
> >           }
> >       }
> >   free_and_exit:
> > @@ -1819,10 +1827,9 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> >                       }
> > 
> >                       len -= len_done;
> > -                    /*
> > -                     * len will always be 0 until superset release is add.
> > -                     * TODO: superset release will be added.
> > -                     */
> > +                    if (len > 0) {
> > +                        dpa = ent_start_dpa + ent_len;
> > +                    }
> >                       break;
> >                   }
> >               }
> > --
> > 2.43.0
> >