[PATCH] mem/cxl_type3: Fix overlapping region validation error

[PATCH] mem/cxl_type3: Fix overlapping region validation error

[Yao Xingtao via]

When injecting a new poisoned region through qmp_cxl_inject_poison(),
the newly injected region should not overlap with existing poisoned
regions.

The current validation method does not consider the following
overlapping region:
┌───┬───────┬───┐
│a  │  b(a) │a  │
└───┴───────┴───┘
(a is a newly added region, b is an existing region, and b is a
 subregion of a)

Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
---
 hw/mem/cxl_type3.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 35ac59883a5b..8e32de327908 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1331,9 +1331,7 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
     ct3d = CXL_TYPE3(obj);
 
     QLIST_FOREACH(p, &ct3d->poison_list, node) {
-        if (((start >= p->start) && (start < p->start + p->length)) ||
-            ((start + length > p->start) &&
-             (start + length <= p->start + p->length))) {
+        if ((start < p->start + p->length) && (start + length > p->start)) {
             error_setg(errp,
                        "Overlap with existing poisoned region not supported");
             return;
-- 
2.37.3

Re: [PATCH] mem/cxl_type3: Fix overlapping region validation error

[Jonathan Cameron via]

On Thu, 18 Jul 2024 05:07:53 -0400
Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote:

> When injecting a new poisoned region through qmp_cxl_inject_poison(),
> the newly injected region should not overlap with existing poisoned
> regions.
> 
> The current validation method does not consider the following
> overlapping region:
> ┌───┬───────┬───┐
> │a  │  b(a) │a  │
> └───┴───────┴───┘
> (a is a newly added region, b is an existing region, and b is a
>  subregion of a)
> 
> Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
Looks correct to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>
I've queued it on my local branch.
I need to put together an updated public one.

No huge rush to queue this up though I think as the effects
are minor.

Jonathan

> ---
>  hw/mem/cxl_type3.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 35ac59883a5b..8e32de327908 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1331,9 +1331,7 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
>      ct3d = CXL_TYPE3(obj);
>  
>      QLIST_FOREACH(p, &ct3d->poison_list, node) {
> -        if (((start >= p->start) && (start < p->start + p->length)) ||
> -            ((start + length > p->start) &&
> -             (start + length <= p->start + p->length))) {
> +        if ((start < p->start + p->length) && (start + length > p->start)) {
>              error_setg(errp,
>                         "Overlap with existing poisoned region not supported");
>              return;

Re: [PATCH] mem/cxl_type3: Fix overlapping region validation error

[Peter Maydell]

On Thu, 18 Jul 2024 at 17:37, Jonathan Cameron via
<qemu-devel@nongnu.org> wrote:
>
> On Thu, 18 Jul 2024 05:07:53 -0400
> Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote:
>
> > When injecting a new poisoned region through qmp_cxl_inject_poison(),
> > the newly injected region should not overlap with existing poisoned
> > regions.
> >
> > The current validation method does not consider the following
> > overlapping region:
> > ┌───┬───────┬───┐
> > │a  │  b(a) │a  │
> > └───┴───────┴───┘
> > (a is a newly added region, b is an existing region, and b is a
> >  subregion of a)
> >
> > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> Looks correct to me.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>
> I've queued it on my local branch.
> I need to put together an updated public one.
>
> No huge rush to queue this up though I think as the effects
> are minor.

I think you can probably write this as
   ranges_overlap(start, len, p->start, p->length)
using the utility function in include/qemu/ranges.h, which is
a bit more readable than open-coding the overlap test.

(There's another couple of open-coded overlap tests in
cxl-mailbox-utils.c.)

thanks
-- PMM

Re: [PATCH] mem/cxl_type3: Fix overlapping region validation error

[Fan Ni]

On Thu, Jul 18, 2024 at 05:07:53AM -0400, Yao Xingtao wrote:
> <!DOCTYPE html><!-- BaNnErBlUrFlE-BoDy-start --><!-- Preheader Text : BEGIN --><meta http-equiv="Content-Type" content="text/html; charset=utf-8"><div style="display:none !important;display:none;visibility:hidden;mso-hide:all;font-size:1px;color:#ffffff;line-height:1px;height:0px;max-height:0px;opacity:0;overflow:hidden;">
>  When injecting a new poisoned region through qmp_cxl_inject_poison(), the newly injected region should not overlap with existing poisoned regions. The current validation method does not consider the following overlapping region: ┌───┬───────┬───┐
> </div>
> <!-- Preheader Text : END -->
> 
> <!-- Email Banner : BEGIN -->
> <div style="display:none !important;display:none;visibility:hidden;mso-hide:all;font-size:1px;color:#ffffff;line-height:1px;height:0px;max-height:0px;opacity:0;overflow:hidden;">ZjQcmQRYFpfptBannerStart</div>
> 
> <!--[if ((ie)|(mso))]>
>   <table border="0" cellspacing="0" cellpadding="0" width="100%" style="padding: 16px 0px 16px 0px; direction: ltr" ><tr><td>
>     <table border="0" cellspacing="0" cellpadding="0" style="padding: 0px 10px 5px 6px; width: 100%; border-radius:4px; border-top:4px solid #f32323;background-color:#F07B7B;"><tr><td valign="top">
>       <table align="left" border="0" cellspacing="0" cellpadding="0" style="padding: 4px 8px 4px 8px">
> 	<tr><td style="color:#000000; font-family: 'Arial', sans-serif; font-weight:bold; font-size:14px; direction: ltr">
> 	  This Message Is From an External Sender
> 	</td></tr>
> 	<tr><td style="color:#000000; font-weight:normal; font-family: 'Arial', sans-serif; font-size:12px; direction: ltr">
>           Use caution opening files, clicking links or responding to requests.
> 	</td></tr>
> 
>       </table>
> 
>     </td></tr></table>
>   </td></tr></table>
> <![endif]-->
> 
> <![if !((ie)|(mso))]>
>   <div dir="ltr" id="pfptBannergkdcmiu" style="all: revert !important; display:block !important; text-align: left !important; margin:16px 0px 16px 0px !important; padding:8px 16px 8px 16px !important; border-radius: 4px !important; min-width: 200px !important; background-color: #F07B7B !important; background-color: #F07B7B; border-top: 4px solid #f32323 !important; border-top: 4px solid #f32323;">
>     <div id="pfptBannergkdcmiu" style="all: unset !important; float:left !important; display:block !important; margin: 0px 0px 1px 0px !important; max-width: 600px !important;">
>       <div id="pfptBannergkdcmiu" style="all: unset !important; display:block !important; visibility: visible !important; background-color: #F07B7B !important; color:#000000 !important; color:#000000; font-family: 'Arial', sans-serif !important; font-family: 'Arial', sans-serif; font-weight:bold !important; font-weight:bold; font-size:14px !important; line-height:18px !important; line-height:18px">
> 	This Message Is From an External Sender
>       </div>
>       <div id="pfptBannergkdcmiu" style="all: unset !important; display:block !important; visibility: visible !important; background-color: #F07B7B !important; color:#000000 !important; color:#000000; font-weight:normal; font-family: 'Arial', sans-serif !important; font-family: 'Arial', sans-serif; font-size:12px !important; line-height:18px !important; line-height:18px; margin-top:2px !important;">
> Use caution opening files, clicking links or responding to requests.
>       </div>
> 
>     </div>
> 
>     <div style="clear: both !important; display: block !important; visibility: hidden !important; line-height: 0 !important; font-size: 0.01px !important; height: 0px">&nbsp;</div>
>   </div>
> <![endif]>
> 
> <div style="display:none !important;display:none;visibility:hidden;mso-hide:all;font-size:1px;color:#ffffff;line-height:1px;height:0px;max-height:0px;opacity:0;overflow:hidden;">ZjQcmQRYFpfptBannerEnd</div>
> <!-- Email Banner : END -->
> 
> <!-- BaNnErBlUrFlE-BoDy-end -->
> <html>
> <head><!-- BaNnErBlUrFlE-HeAdEr-start -->
> <style>
>   #pfptBannergkdcmiu { all: revert !important; display: block !important; 
>     visibility: visible !important; opacity: 1 !important; 
>     background-color: #F07B7B !important; 
>     max-width: none !important; max-height: none !important }
>   .pfptPrimaryButtongkdcmiu:hover, .pfptPrimaryButtongkdcmiu:focus {
>     background-color: #ef5b5b !important; }
>   .pfptPrimaryButtongkdcmiu:active {
>     background-color: #f32323 !important; }
> </style>
> 
> <!-- BaNnErBlUrFlE-HeAdEr-end -->
> </head><body><pre style="font-family: sans-serif; font-size: 100%; white-space: pre-wrap; word-wrap: break-word">When injecting a new poisoned region through qmp_cxl_inject_poison(),
> the newly injected region should not overlap with existing poisoned
> regions.
> 
> The current validation method does not consider the following
> overlapping region:
> ┌───┬───────┬───┐
> │a  │  b(a) │a  │
> └───┴───────┴───┘
> (a is a newly added region, b is an existing region, and b is a
>  subregion of a)
> 
> Signed-off-by: Yao Xingtao &lt;yaoxt.fnst@fujitsu.com&gt;
> ---
>  hw/mem/cxl_type3.c | 4 &#43;---
>  1 file changed, 1 insertion(&#43;), 3 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 35ac59883a5b..8e32de327908 100644
> --- a/hw/mem/cxl_type3.c
> &#43;&#43;&#43; b/hw/mem/cxl_type3.c
> @@ -1331,9 &#43;1331,7 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
>      ct3d = CXL_TYPE3(obj);
>  
>      QLIST_FOREACH(p, &amp;ct3d-&gt;poison_list, node) {
> -        if (((start &gt;= p-&gt;start) &amp;&amp; (start &lt; p-&gt;start &#43; p-&gt;length)) ||
> -            ((start &#43; length &gt; p-&gt;start) &amp;&amp;
> -             (start &#43; length &lt;= p-&gt;start &#43; p-&gt;length))) {
> &#43;        if ((start &lt; p-&gt;start &#43; p-&gt;length) &amp;&amp; (start &#43; length &gt; p-&gt;start)) {
>              error_setg(errp,
>                         &quot;Overlap with existing poisoned region not supported&quot;);
>              return;

As mentioned by Peter, we can use ranges_overlap() to improve the
code readability. Other than that, looks good t me.

btw, not sure only me or not, but the message does not display
correctly in mutt, seems not a plain text message, but looks fine in
outlook.

Fan
> -- 
> 2.37.3
> 
> </pre></body></html>

RE: [PATCH] mem/cxl_type3: Fix overlapping region validation error

[Xingtao Yao (Fujitsu) via]

> 
> 
> As mentioned by Peter, we can use ranges_overlap() to improve the
> code readability. Other than that, looks good t me.
> 
> btw, not sure only me or not, but the message does not display
> correctly in mutt, seems not a plain text message, but looks fine in
> outlook.
I am not sure as well, but it shows properly on the mailing list. 
> 
> Fan

RE: [PATCH] mem/cxl_type3: Fix overlapping region validation error

[Xingtao Yao (Fujitsu) via]

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Friday, July 19, 2024 1:12 AM
> To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; fan.ni@samsung.com;
> qemu-devel@nongnu.org
> Subject: Re: [PATCH] mem/cxl_type3: Fix overlapping region validation error
> 
> On Thu, 18 Jul 2024 at 17:37, Jonathan Cameron via
> <qemu-devel@nongnu.org> wrote:
> >
> > On Thu, 18 Jul 2024 05:07:53 -0400
> > Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote:
> >
> > > When injecting a new poisoned region through qmp_cxl_inject_poison(),
> > > the newly injected region should not overlap with existing poisoned
> > > regions.
> > >
> > > The current validation method does not consider the following
> > > overlapping region:
> > > ┌───┬───────┬───┐
> > > │a  │  b(a) │a  │
> > > └───┴───────┴───┘
> > > (a is a newly added region, b is an existing region, and b is a
> > >  subregion of a)
> > >
> > > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> > Looks correct to me.
> >
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>
> > I've queued it on my local branch.
> > I need to put together an updated public one.
> >
> > No huge rush to queue this up though I think as the effects
> > are minor.
> 
> I think you can probably write this as
>    ranges_overlap(start, len, p->start, p->length)
> using the utility function in include/qemu/ranges.h, which is
> a bit more readable than open-coding the overlap test.
Great! I will fix it in the next revision.

> 
> (There's another couple of open-coded overlap tests in
> cxl-mailbox-utils.c.)
I will collect these issues and fix them in separate patches.

> 
> thanks
> -- PMM