| Message ID | 20240718090753.59163-1-yaoxt.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mem/cxl_type3: Fix overlapping region validation error | expand |
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;
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
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"> </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 <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; 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>
> > > 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
> -----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
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;
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(-)