| Message ID | 20241118164434.7551-11-alejandro.lucero-palau@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | cxl: add type2 device basic support | expand |
On 11/18/24 9:44 AM, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > For a resource defined with size zero, resource_contains returns > always true. > > Add resource size check before using it. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> Should this be broken out and send ahead of the type2 series? nit below > --- > drivers/cxl/core/hdm.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 223c273c0cd1..c58d6b8f9b58 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, > cxled->dpa_res = res; > cxled->skip = skipped; > > - if (resource_contains(&cxlds->pmem_res, res)) > + if (resource_size(&cxlds->pmem_res) && > + resource_contains(&cxlds->pmem_res, res)) { > cxled->mode = CXL_DECODER_PMEM; > - else if (resource_contains(&cxlds->ram_res, res)) > + } else if (resource_size(&cxlds->ram_res) && > + resource_contains(&cxlds->ram_res, res)) { > cxled->mode = CXL_DECODER_RAM; > + } > else { } else { > dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n", > port->id, cxled->cxld.id, cxled->dpa_res);
On Mon, 18 Nov 2024 16:44:17 +0000 <alejandro.lucero-palau@amd.com> wrote: > From: Alejandro Lucero <alucerop@amd.com> > > For a resource defined with size zero, resource_contains returns > always true. > > Add resource size check before using it. > Does this trigger a bug? Looks like this should be with a Fixes: tag? Z. > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > --- > drivers/cxl/core/hdm.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 223c273c0cd1..c58d6b8f9b58 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct > cxl_endpoint_decoder *cxled, cxled->dpa_res = res; > cxled->skip = skipped; > > - if (resource_contains(&cxlds->pmem_res, res)) > + if (resource_size(&cxlds->pmem_res) && > + resource_contains(&cxlds->pmem_res, res)) { > cxled->mode = CXL_DECODER_PMEM; > - else if (resource_contains(&cxlds->ram_res, res)) > + } else if (resource_size(&cxlds->ram_res) && > + resource_contains(&cxlds->ram_res, res)) { > cxled->mode = CXL_DECODER_RAM; > + } > else { > dev_warn(dev, "decoder%d.%d: %pr mixed mode not > supported\n", port->id, cxled->cxld.id, cxled->dpa_res);
On 11/19/24 18:00, Dave Jiang wrote: > > On 11/18/24 9:44 AM, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> For a resource defined with size zero, resource_contains returns >> always true. >> >> Add resource size check before using it. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > > Should this be broken out and send ahead of the type2 series? I could. This seems not a problem though until the new code coming along. > nit below > >> --- >> drivers/cxl/core/hdm.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >> index 223c273c0cd1..c58d6b8f9b58 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c >> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, >> cxled->dpa_res = res; >> cxled->skip = skipped; >> >> - if (resource_contains(&cxlds->pmem_res, res)) >> + if (resource_size(&cxlds->pmem_res) && >> + resource_contains(&cxlds->pmem_res, res)) { >> cxled->mode = CXL_DECODER_PMEM; >> - else if (resource_contains(&cxlds->ram_res, res)) >> + } else if (resource_size(&cxlds->ram_res) && >> + resource_contains(&cxlds->ram_res, res)) { >> cxled->mode = CXL_DECODER_RAM; >> + } >> else { > } else { I'll fix it. Thanks! >> dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n", >> port->id, cxled->cxld.id, cxled->dpa_res);
On 11/19/24 19:50, Zhi Wang wrote: > On Mon, 18 Nov 2024 16:44:17 +0000 > <alejandro.lucero-palau@amd.com> wrote: > >> From: Alejandro Lucero <alucerop@amd.com> >> >> For a resource defined with size zero, resource_contains returns >> always true. >> >> Add resource size check before using it. >> > Does this trigger a bug? Looks like this should be with a Fixes: tag? It seems it does not until the new code for type2 support. > Z. > >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/cxl/core/hdm.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >> index 223c273c0cd1..c58d6b8f9b58 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c >> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct >> cxl_endpoint_decoder *cxled, cxled->dpa_res = res; >> cxled->skip = skipped; >> >> - if (resource_contains(&cxlds->pmem_res, res)) >> + if (resource_size(&cxlds->pmem_res) && >> + resource_contains(&cxlds->pmem_res, res)) { >> cxled->mode = CXL_DECODER_PMEM; >> - else if (resource_contains(&cxlds->ram_res, res)) >> + } else if (resource_size(&cxlds->ram_res) && >> + resource_contains(&cxlds->ram_res, res)) { >> cxled->mode = CXL_DECODER_RAM; >> + } >> else { >> dev_warn(dev, "decoder%d.%d: %pr mixed mode not >> supported\n", port->id, cxled->cxld.id, cxled->dpa_res);
On Mon, Nov 18, 2024 at 04:44:17PM +0000, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > For a resource defined with size zero, resource_contains returns > always true. > I'm not following the premise above - Looking at resource_contains() and the changes made below, it seems the concern is with &cxlds->ram_res or &cxlds->pmem_res being zero - because we already checked that the second param 'res' is not zero a few lines above. Looking at what happens when r1 is of size 0, I don't see how resource_contains() returns always true. In resource_contains(r1, r2), if r1 is of size 0, r1->start == r1->end. The func can only return true if r2 is also of size 0 and located at exactly r1->start. But, in this case, we are not going to get there because we never send an r2 of size 0. For any non-zero size r2 the func will always return false because the size 0 r1 cannot encompass any range. I could be misreading it all ;) --Alison > Add resource size check before using it. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > --- > drivers/cxl/core/hdm.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 223c273c0cd1..c58d6b8f9b58 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, > cxled->dpa_res = res; > cxled->skip = skipped; > > - if (resource_contains(&cxlds->pmem_res, res)) > + if (resource_size(&cxlds->pmem_res) && > + resource_contains(&cxlds->pmem_res, res)) { > cxled->mode = CXL_DECODER_PMEM; > - else if (resource_contains(&cxlds->ram_res, res)) > + } else if (resource_size(&cxlds->ram_res) && > + resource_contains(&cxlds->ram_res, res)) { > cxled->mode = CXL_DECODER_RAM; > + } > else { > dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n", > port->id, cxled->cxld.id, cxled->dpa_res); > -- > 2.17.1 > >
On Wed, 20 Nov 2024 13:45:54 +0000 Alejandro Lucero Palau <alucerop@amd.com> wrote: > > On 11/19/24 19:50, Zhi Wang wrote: > > On Mon, 18 Nov 2024 16:44:17 +0000 > > <alejandro.lucero-palau@amd.com> wrote: > > > >> From: Alejandro Lucero <alucerop@amd.com> > >> > >> For a resource defined with size zero, resource_contains returns > >> always true. > >> > >> Add resource size check before using it. > >> > > Does this trigger a bug? Looks like this should be with a Fixes: > > tag? > > > It seems it does not until the new code for type2 support. > I see. Then it is not necessary for a Fixes: tag. > > > Z. > > > >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > >> --- > >> drivers/cxl/core/hdm.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > >> index 223c273c0cd1..c58d6b8f9b58 100644 > >> --- a/drivers/cxl/core/hdm.c > >> +++ b/drivers/cxl/core/hdm.c > >> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct > >> cxl_endpoint_decoder *cxled, cxled->dpa_res = res; > >> cxled->skip = skipped; > >> > >> - if (resource_contains(&cxlds->pmem_res, res)) > >> + if (resource_size(&cxlds->pmem_res) && > >> + resource_contains(&cxlds->pmem_res, res)) { > >> cxled->mode = CXL_DECODER_PMEM; > >> - else if (resource_contains(&cxlds->ram_res, res)) > >> + } else if (resource_size(&cxlds->ram_res) && > >> + resource_contains(&cxlds->ram_res, res)) { > >> cxled->mode = CXL_DECODER_RAM; > >> + } > >> else { > >> dev_warn(dev, "decoder%d.%d: %pr mixed mode not > >> supported\n", port->id, cxled->cxld.id, cxled->dpa_res); >
On 11/21/24 02:46, Alison Schofield wrote: > On Mon, Nov 18, 2024 at 04:44:17PM +0000, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> For a resource defined with size zero, resource_contains returns >> always true. >> > I'm not following the premise above - > > Looking at resource_contains() and the changes made below, > it seems the concern is with &cxlds->ram_res or &cxlds->pmem_res > being zero - because we already checked that the second param > 'res' is not zero a few lines above. > > Looking at what happens when r1 is of size 0, I don't see how > resource_contains() returns always true. > > In resource_contains(r1, r2), if r1 is of size 0, r1->start == r1->end. > The func can only return true if r2 is also of size 0 and located at > exactly r1->start. But, in this case, we are not going to get there > because we never send an r2 of size 0. > > For any non-zero size r2 the func will always return false because > the size 0 r1 cannot encompass any range. > > I could be misreading it all ;) The key is to know how a resource with size 0 is initialized, what can be understood looking at DEFINE_RES_NAMED macro. The end field is set as size - 1. With unsigned variables, as it is the case here, it means to have a resource as big as possible ... if you do not check first the size is not 0. The pmem resource is explicitly initialized inside cxl_accel_state_create in the previous patch, so it has: pmem_res->start = 0, pmem_res.end = 0xffffffffffffffff the resource checked against is defined with, for example, a 256MB size: res.start =0, res.end = 0xfffffff if you then use resource_contains(pmem_res, res), that implies always true, whatever the res range defined. All this confused me as well when facing it initially. I hope this explanation makes sense. > > --Alison > > >> Add resource size check before using it. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/cxl/core/hdm.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >> index 223c273c0cd1..c58d6b8f9b58 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c >> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, >> cxled->dpa_res = res; >> cxled->skip = skipped; >> >> - if (resource_contains(&cxlds->pmem_res, res)) >> + if (resource_size(&cxlds->pmem_res) && >> + resource_contains(&cxlds->pmem_res, res)) { >> cxled->mode = CXL_DECODER_PMEM; >> - else if (resource_contains(&cxlds->ram_res, res)) >> + } else if (resource_size(&cxlds->ram_res) && >> + resource_contains(&cxlds->ram_res, res)) { >> cxled->mode = CXL_DECODER_RAM; >> + } >> else { >> dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n", >> port->id, cxled->cxld.id, cxled->dpa_res); >> -- >> 2.17.1 >> >>
On Thu, Nov 21, 2024 at 09:22:33AM +0000, Alejandro Lucero Palau wrote: > > On 11/21/24 02:46, Alison Schofield wrote: > > On Mon, Nov 18, 2024 at 04:44:17PM +0000, alejandro.lucero-palau@amd.com wrote: > > > From: Alejandro Lucero <alucerop@amd.com> > > > > > > For a resource defined with size zero, resource_contains returns > > > always true. > > > > > I'm not following the premise above - > > > > Looking at resource_contains() and the changes made below, > > it seems the concern is with &cxlds->ram_res or &cxlds->pmem_res > > being zero - because we already checked that the second param > > 'res' is not zero a few lines above. > > > > Looking at what happens when r1 is of size 0, I don't see how > > resource_contains() returns always true. > > > > In resource_contains(r1, r2), if r1 is of size 0, r1->start == r1->end. > > The func can only return true if r2 is also of size 0 and located at > > exactly r1->start. But, in this case, we are not going to get there > > because we never send an r2 of size 0. > > > > For any non-zero size r2 the func will always return false because > > the size 0 r1 cannot encompass any range. > > > > I could be misreading it all ;) > > > The key is to know how a resource with size 0 is initialized, what can be > understood looking at DEFINE_RES_NAMED macro. The end field is set as size > - 1. > > With unsigned variables, as it is the case here, it means to have a resource > as big as possible ... if you do not check first the size is not 0. > > The pmem resource is explicitly initialized inside cxl_accel_state_create in > the previous patch, so it has: > > pmem_res->start = 0, pmem_res.end = 0xffffffffffffffff > > the resource checked against is defined with, for example, a 256MB size: > > res.start =0, res.end = 0xfffffff > > > if you then use resource_contains(pmem_res, res), that implies always true, > whatever the res range defined. > > > All this confused me as well when facing it initially. I hope this > explanation makes sense. > Thanks for the explanation! I'm wondering if we are leaving a trap for the next developer. resource_contains() seems to have intended that a check for IORESOURCE_UNSET would take care of the zero size case: (5edb93b89f6c resource: Add resource_contains) and it would if folks used _UNSET. Some check r1->start before calling resource_contains(). One option would be to use _UNSET in this case, but that only covers us here, and doesn't remove the trap ;) How about hardening resource_contains(): ie: make resource_contains() return false if either res empty /* True iff r1 completely contains r2 */ static inline bool resource_contains(const struct resource *r1, const struct resource *r2) { + if (!resource_size(r1) || !resource_size(r2)) + return false; if (resource_type(r1) != resource_type(r2)) return false; if (r1->flags & IORESOURCE_UNSET || r2->flags & IORESOURCE_UNSET) return false; return r1->start <= r2->start && r1->end >= r2->end; } -- Alison > > > > --Alison > > > > > > > Add resource size check before using it. > > > > > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > > > --- > > > drivers/cxl/core/hdm.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > > > index 223c273c0cd1..c58d6b8f9b58 100644 > > > --- a/drivers/cxl/core/hdm.c > > > +++ b/drivers/cxl/core/hdm.c > > > @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, > > > cxled->dpa_res = res; > > > cxled->skip = skipped; > > > - if (resource_contains(&cxlds->pmem_res, res)) > > > + if (resource_size(&cxlds->pmem_res) && > > > + resource_contains(&cxlds->pmem_res, res)) { > > > cxled->mode = CXL_DECODER_PMEM; > > > - else if (resource_contains(&cxlds->ram_res, res)) > > > + } else if (resource_size(&cxlds->ram_res) && > > > + resource_contains(&cxlds->ram_res, res)) { > > > cxled->mode = CXL_DECODER_RAM; > > > + } > > > else { > > > dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n", > > > port->id, cxled->cxld.id, cxled->dpa_res); > > > -- > > > 2.17.1 > > > > > >
On 11/21/24 21:00, Alison Schofield wrote: > On Thu, Nov 21, 2024 at 09:22:33AM +0000, Alejandro Lucero Palau wrote: >> On 11/21/24 02:46, Alison Schofield wrote: >>> On Mon, Nov 18, 2024 at 04:44:17PM +0000, alejandro.lucero-palau@amd.com wrote: >>>> From: Alejandro Lucero <alucerop@amd.com> >>>> >>>> For a resource defined with size zero, resource_contains returns >>>> always true. >>>> >>> I'm not following the premise above - >>> >>> Looking at resource_contains() and the changes made below, >>> it seems the concern is with &cxlds->ram_res or &cxlds->pmem_res >>> being zero - because we already checked that the second param >>> 'res' is not zero a few lines above. >>> >>> Looking at what happens when r1 is of size 0, I don't see how >>> resource_contains() returns always true. >>> >>> In resource_contains(r1, r2), if r1 is of size 0, r1->start == r1->end. >>> The func can only return true if r2 is also of size 0 and located at >>> exactly r1->start. But, in this case, we are not going to get there >>> because we never send an r2 of size 0. >>> >>> For any non-zero size r2 the func will always return false because >>> the size 0 r1 cannot encompass any range. >>> >>> I could be misreading it all ;) >> >> The key is to know how a resource with size 0 is initialized, what can be >> understood looking at DEFINE_RES_NAMED macro. The end field is set as size >> - 1. >> >> With unsigned variables, as it is the case here, it means to have a resource >> as big as possible ... if you do not check first the size is not 0. >> >> The pmem resource is explicitly initialized inside cxl_accel_state_create in >> the previous patch, so it has: >> >> pmem_res->start = 0, pmem_res.end = 0xffffffffffffffff >> >> the resource checked against is defined with, for example, a 256MB size: >> >> res.start =0, res.end = 0xfffffff >> >> >> if you then use resource_contains(pmem_res, res), that implies always true, >> whatever the res range defined. >> >> >> All this confused me as well when facing it initially. I hope this >> explanation makes sense. >> > Thanks for the explanation! I'm wondering if we are leaving a trap for the next > developer. > > resource_contains() seems to have intended that a check for IORESOURCE_UNSET > would take care of the zero size case: > > (5edb93b89f6c resource: Add resource_contains) > > and it would if folks used _UNSET. Some check r1->start before calling > resource_contains(). > > One option would be to use _UNSET in this case, but that only covers us here, > and doesn't remove the trap ;) > > How about hardening resource_contains(): > > ie: make resource_contains() return false if either res empty > > /* True iff r1 completely contains r2 */ > static inline bool resource_contains(const struct resource *r1, const struct resource *r2) > { > + if (!resource_size(r1) || !resource_size(r2)) > + return false; > if (resource_type(r1) != resource_type(r2)) > return false; > if (r1->flags & IORESOURCE_UNSET || r2->flags & IORESOURCE_UNSET) > return false; > return r1->start <= r2->start && r1->end >= r2->end; > } I can try that. If there is a good reason for not hardening it, we will know. Thanks! > -- Alison > >>> --Alison >>> >>> >>>> Add resource size check before using it. >>>> >>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >>>> --- >>>> drivers/cxl/core/hdm.c | 7 +++++-- >>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >>>> index 223c273c0cd1..c58d6b8f9b58 100644 >>>> --- a/drivers/cxl/core/hdm.c >>>> +++ b/drivers/cxl/core/hdm.c >>>> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, >>>> cxled->dpa_res = res; >>>> cxled->skip = skipped; >>>> - if (resource_contains(&cxlds->pmem_res, res)) >>>> + if (resource_size(&cxlds->pmem_res) && >>>> + resource_contains(&cxlds->pmem_res, res)) { >>>> cxled->mode = CXL_DECODER_PMEM; >>>> - else if (resource_contains(&cxlds->ram_res, res)) >>>> + } else if (resource_size(&cxlds->ram_res) && >>>> + resource_contains(&cxlds->ram_res, res)) { >>>> cxled->mode = CXL_DECODER_RAM; >>>> + } >>>> else { >>>> dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n", >>>> port->id, cxled->cxld.id, cxled->dpa_res); >>>> -- >>>> 2.17.1 >>>> >>>>
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 223c273c0cd1..c58d6b8f9b58 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, cxled->dpa_res = res; cxled->skip = skipped; - if (resource_contains(&cxlds->pmem_res, res)) + if (resource_size(&cxlds->pmem_res) && + resource_contains(&cxlds->pmem_res, res)) { cxled->mode = CXL_DECODER_PMEM; - else if (resource_contains(&cxlds->ram_res, res)) + } else if (resource_size(&cxlds->ram_res) && + resource_contains(&cxlds->ram_res, res)) { cxled->mode = CXL_DECODER_RAM; + } else { dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n", port->id, cxled->cxld.id, cxled->dpa_res);