diff mbox series

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

Message ID 20240325190339.696686-12-nifan.cxl@gmail.com
State Superseded
Headers show
Series Enabling DCD emulation support in Qemu | expand

Commit Message

Fan Ni March 25, 2024, 7:02 p.m. UTC
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(-)

Comments

Jørgen Hansen April 5, 2024, 9:57 a.m. UTC | #1
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).

> +        /* 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
>
Jonathan Cameron April 5, 2024, 12:32 p.m. UTC | #2
On Mon, 25 Mar 2024 12:02:29 -0700
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>
Nothing to add from me.
Nice and simple which is great.
Jonathan
Fan Ni April 15, 2024, 8:17 p.m. UTC | #3
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
> >
diff mbox series

Patch

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;
+        }
+        /* 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;
+
+                        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;
                 }
             }