[PATCH v2 0/2] cxl/cdat: Fixes for CXL CDAT processing

[PATCH v2 0/2] cxl/cdat: Fixes for CXL CDAT processing

[Ira Weiny]

These are a couple of fixes needed for DCD processing.  The first is via
code inspection and the second was found as a result of the kernel bug
found previously.[1]

[1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
Changes in v2:
- [iweiny: add fixes tags]
- [djiang: remove do {} while (0);]
- Link to v1: https://lore.kernel.org/r/20231117-fix-cdat-cs-v1-0-ffc2b116ca6c@intel.com

---
Ira Weiny (2):
      cxl/cdat: Handle cdat table build errors
      cxl/cdat: Fix header sum value in CDAT checksum

 hw/cxl/cxl-cdat.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
---
base-commit: abe893944bb3fb3ca59aaeaa8d48e52dfdc0f3db
change-id: 20231117-fix-cdat-cs-a8ed72ec2244

Best regards,
-- 
Ira Weiny <ira.weiny@intel.com>

[PATCH v2 1/2] cxl/cdat: Handle cdat table build errors

[Ira Weiny]

The callback for building CDAT tables may return negative error codes.
This was previously unhandled and will result in potentially huge
allocations later on in ct3_build_cdat()

Detect the negative error code and defer cdat building.

Fixes: f5ee7413d592 ("hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange")
Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 hw/cxl/cxl-cdat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 639a2db3e17b..24829cf2428d 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -63,7 +63,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
     cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
                                                  cdat->private);
 
-    if (!cdat->built_buf_len) {
+    if (cdat->built_buf_len <= 0) {
         /* Build later as not all data available yet */
         cdat->to_update = true;
         return;

-- 
2.42.0

Re: [PATCH v2 1/2] cxl/cdat: Handle cdat table build errors

[fan]

On Wed, Nov 29, 2023 at 05:33:03PM -0800, Ira Weiny wrote:
> The callback for building CDAT tables may return negative error codes.
> This was previously unhandled and will result in potentially huge
> allocations later on in ct3_build_cdat()
> 
> Detect the negative error code and defer cdat building.
> 
> Fixes: f5ee7413d592 ("hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange")
> Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  hw/cxl/cxl-cdat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 639a2db3e17b..24829cf2428d 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -63,7 +63,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>      cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
>                                                   cdat->private);
>  
> -    if (!cdat->built_buf_len) {
> +    if (cdat->built_buf_len <= 0) {
>          /* Build later as not all data available yet */
>          cdat->to_update = true;
>          return;
> 

The fix looks good to me. Just curious how to really build cdat table
again when an error occurs, for example, the memory allocation fails.

Fan
> -- 
> 2.42.0
> 

Re: [PATCH v2 1/2] cxl/cdat: Handle cdat table build errors

[Ira Weiny]

fan wrote:
> On Wed, Nov 29, 2023 at 05:33:03PM -0800, Ira Weiny wrote:
> > The callback for building CDAT tables may return negative error codes.
> > This was previously unhandled and will result in potentially huge
> > allocations later on in ct3_build_cdat()
> > 
> > Detect the negative error code and defer cdat building.
> > 
> > Fixes: f5ee7413d592 ("hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange")
> > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  hw/cxl/cxl-cdat.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > index 639a2db3e17b..24829cf2428d 100644
> > --- a/hw/cxl/cxl-cdat.c
> > +++ b/hw/cxl/cxl-cdat.c
> > @@ -63,7 +63,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> >      cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
> >                                                   cdat->private);
> >  
> > -    if (!cdat->built_buf_len) {
> > +    if (cdat->built_buf_len <= 0) {
> >          /* Build later as not all data available yet */
> >          cdat->to_update = true;
> >          return;
> > 
> 
> The fix looks good to me. Just curious how to really build cdat table
> again when an error occurs, for example, the memory allocation fails.

I did not go that far as I am unsure as well.

Ira

Re: [PATCH v2 1/2] cxl/cdat: Handle cdat table build errors

[Jonathan Cameron]

On Wed, 20 Dec 2023 11:55:33 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> fan wrote:
> > On Wed, Nov 29, 2023 at 05:33:03PM -0800, Ira Weiny wrote:  
> > > The callback for building CDAT tables may return negative error codes.
> > > This was previously unhandled and will result in potentially huge
> > > allocations later on in ct3_build_cdat()
> > > 
> > > Detect the negative error code and defer cdat building.
> > > 
> > > Fixes: f5ee7413d592 ("hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange")
> > > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > ---
> > >  hw/cxl/cxl-cdat.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > > index 639a2db3e17b..24829cf2428d 100644
> > > --- a/hw/cxl/cxl-cdat.c
> > > +++ b/hw/cxl/cxl-cdat.c
> > > @@ -63,7 +63,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> > >      cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
> > >                                                   cdat->private);
> > >  
> > > -    if (!cdat->built_buf_len) {
> > > +    if (cdat->built_buf_len <= 0) {
> > >          /* Build later as not all data available yet */
> > >          cdat->to_update = true;
> > >          return;
> > >   
> > 
> > The fix looks good to me. Just curious how to really build cdat table
> > again when an error occurs, for example, the memory allocation fails.  
> 
> I did not go that far as I am unsure as well.
Memory allocations in qemu don't fail (well if they do it crashes)
Side effect of using glib which makes for simpler cases.
https://docs.gtk.org/glib/func.malloc.html

There shouldn't even be any checks :(  I'll fix that up at somepoint
across all the CXL emulation.  Sometimes reviewers noticed and
we dropped it at earlier stages, but clearly didn't catch them all.

Which come to think of it is why this error condition is in practice
not actually buggy as the code won't ever manage to return -ENOMEM and
I don't think there are other error codes.

Jonathan

> 
> Ira
> 

Re: [PATCH v2 1/2] cxl/cdat: Handle cdat table build errors

[Ira Weiny]

Jonathan Cameron wrote:
> On Wed, 20 Dec 2023 11:55:33 -0800
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > fan wrote:
> > > On Wed, Nov 29, 2023 at 05:33:03PM -0800, Ira Weiny wrote:  
> > > > The callback for building CDAT tables may return negative error codes.
> > > > This was previously unhandled and will result in potentially huge
> > > > allocations later on in ct3_build_cdat()
> > > > 
> > > > Detect the negative error code and defer cdat building.
> > > > 
> > > > Fixes: f5ee7413d592 ("hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange")
> > > > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > > ---
> > > >  hw/cxl/cxl-cdat.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > > > index 639a2db3e17b..24829cf2428d 100644
> > > > --- a/hw/cxl/cxl-cdat.c
> > > > +++ b/hw/cxl/cxl-cdat.c
> > > > @@ -63,7 +63,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> > > >      cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
> > > >                                                   cdat->private);
> > > >  
> > > > -    if (!cdat->built_buf_len) {
> > > > +    if (cdat->built_buf_len <= 0) {
> > > >          /* Build later as not all data available yet */
> > > >          cdat->to_update = true;
> > > >          return;
> > > >   
> > > 
> > > The fix looks good to me. Just curious how to really build cdat table
> > > again when an error occurs, for example, the memory allocation fails.  
> > 
> > I did not go that far as I am unsure as well.
> Memory allocations in qemu don't fail (well if they do it crashes)
> Side effect of using glib which makes for simpler cases.
> https://docs.gtk.org/glib/func.malloc.html
> 
> There shouldn't even be any checks :(  I'll fix that up at somepoint
> across all the CXL emulation.  Sometimes reviewers noticed and
> we dropped it at earlier stages, but clearly didn't catch them all.
> 
> Which come to think of it is why this error condition is in practice
> not actually buggy as the code won't ever manage to return -ENOMEM and
> I don't think there are other error codes.

Ah.  Ok but in that case I would say that build_cdat_table() should never
return < 0 to be clear at this level what can happen.

Would you like a patch for that?  (/me assumes you dropped this patch)

Ira

Re: [PATCH v2 1/2] cxl/cdat: Handle cdat table build errors

[Jonathan Cameron]

On Mon, 8 Jan 2024 08:06:32 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Wed, 20 Dec 2023 11:55:33 -0800
> > Ira Weiny <ira.weiny@intel.com> wrote:
> >   
> > > fan wrote:  
> > > > On Wed, Nov 29, 2023 at 05:33:03PM -0800, Ira Weiny wrote:    
> > > > > The callback for building CDAT tables may return negative error codes.
> > > > > This was previously unhandled and will result in potentially huge
> > > > > allocations later on in ct3_build_cdat()
> > > > > 
> > > > > Detect the negative error code and defer cdat building.
> > > > > 
> > > > > Fixes: f5ee7413d592 ("hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange")
> > > > > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > > > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > > > ---
> > > > >  hw/cxl/cxl-cdat.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > > > > index 639a2db3e17b..24829cf2428d 100644
> > > > > --- a/hw/cxl/cxl-cdat.c
> > > > > +++ b/hw/cxl/cxl-cdat.c
> > > > > @@ -63,7 +63,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> > > > >      cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
> > > > >                                                   cdat->private);
> > > > >  
> > > > > -    if (!cdat->built_buf_len) {
> > > > > +    if (cdat->built_buf_len <= 0) {
> > > > >          /* Build later as not all data available yet */
> > > > >          cdat->to_update = true;
> > > > >          return;
> > > > >     
> > > > 
> > > > The fix looks good to me. Just curious how to really build cdat table
> > > > again when an error occurs, for example, the memory allocation fails.    
> > > 
> > > I did not go that far as I am unsure as well.  
> > Memory allocations in qemu don't fail (well if they do it crashes)
> > Side effect of using glib which makes for simpler cases.
> > https://docs.gtk.org/glib/func.malloc.html
> > 
> > There shouldn't even be any checks :(  I'll fix that up at somepoint
> > across all the CXL emulation.  Sometimes reviewers noticed and
> > we dropped it at earlier stages, but clearly didn't catch them all.
> > 
> > Which come to think of it is why this error condition is in practice
> > not actually buggy as the code won't ever manage to return -ENOMEM and
> > I don't think there are other error codes.  
> 
> Ah.  Ok but in that case I would say that build_cdat_table() should never
> return < 0 to be clear at this level what can happen.
> 
> Would you like a patch for that?  (/me assumes you dropped this patch)

Probably needs to first rip out all the -ENOMEM returns that got into
the CXL code in general, then tidy up the return type to be unsigned.

If you want to do that it would be welcome!

Jonathan


> 
> Ira
> 

Re: [PATCH v2 1/2] cxl/cdat: Handle cdat table build errors

[Jonathan Cameron]

On Mon, 8 Jan 2024 18:00:42 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon, 8 Jan 2024 08:06:32 -0800
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > Jonathan Cameron wrote:  
> > > On Wed, 20 Dec 2023 11:55:33 -0800
> > > Ira Weiny <ira.weiny@intel.com> wrote:
> > >     
> > > > fan wrote:    
> > > > > On Wed, Nov 29, 2023 at 05:33:03PM -0800, Ira Weiny wrote:      
> > > > > > The callback for building CDAT tables may return negative error codes.
> > > > > > This was previously unhandled and will result in potentially huge
> > > > > > allocations later on in ct3_build_cdat()
> > > > > > 
> > > > > > Detect the negative error code and defer cdat building.
> > > > > > 
> > > > > > Fixes: f5ee7413d592 ("hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange")
> > > > > > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > > > > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > > > > ---
> > > > > >  hw/cxl/cxl-cdat.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > > > > > index 639a2db3e17b..24829cf2428d 100644
> > > > > > --- a/hw/cxl/cxl-cdat.c
> > > > > > +++ b/hw/cxl/cxl-cdat.c
> > > > > > @@ -63,7 +63,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> > > > > >      cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
> > > > > >                                                   cdat->private);
> > > > > >  
> > > > > > -    if (!cdat->built_buf_len) {
> > > > > > +    if (cdat->built_buf_len <= 0) {
> > > > > >          /* Build later as not all data available yet */
> > > > > >          cdat->to_update = true;
> > > > > >          return;
> > > > > >       
> > > > > 
> > > > > The fix looks good to me. Just curious how to really build cdat table
> > > > > again when an error occurs, for example, the memory allocation fails.      
> > > > 
> > > > I did not go that far as I am unsure as well.    
> > > Memory allocations in qemu don't fail (well if they do it crashes)
> > > Side effect of using glib which makes for simpler cases.
> > > https://docs.gtk.org/glib/func.malloc.html
> > > 
> > > There shouldn't even be any checks :(  I'll fix that up at somepoint
> > > across all the CXL emulation.  Sometimes reviewers noticed and
> > > we dropped it at earlier stages, but clearly didn't catch them all.
> > > 
> > > Which come to think of it is why this error condition is in practice
> > > not actually buggy as the code won't ever manage to return -ENOMEM and
> > > I don't think there are other error codes.    
> > 
> > Ah.  Ok but in that case I would say that build_cdat_table() should never
> > return < 0 to be clear at this level what can happen.
> > 
> > Would you like a patch for that?  (/me assumes you dropped this patch)  
> 
> Probably needs to first rip out all the -ENOMEM returns that got into
> the CXL code in general, then tidy up the return type to be unsigned.
> 
> If you want to do that it would be welcome!
Actually.  Build_cdat_table() can return errors just not for this reason.

host_memory_backend_get_memory() can fail for example.  So original patch is good
as is, just that the discussion of memory allocation failure threw me
off and should be cleaned up separately.

Jonathan

> 
> Jonathan
> 
> 
> > 
> > Ira
> >   
> 

Re: [PATCH v2 1/2] cxl/cdat: Handle cdat table build errors

[Ira Weiny]

Jonathan Cameron wrote:

[snip]

> > > > > 
> > > > > I did not go that far as I am unsure as well.    
> > > > Memory allocations in qemu don't fail (well if they do it crashes)
> > > > Side effect of using glib which makes for simpler cases.
> > > > https://docs.gtk.org/glib/func.malloc.html
> > > > 
> > > > There shouldn't even be any checks :(  I'll fix that up at somepoint
> > > > across all the CXL emulation.  Sometimes reviewers noticed and
> > > > we dropped it at earlier stages, but clearly didn't catch them all.
> > > > 
> > > > Which come to think of it is why this error condition is in practice
> > > > not actually buggy as the code won't ever manage to return -ENOMEM and
> > > > I don't think there are other error codes.    
> > > 
> > > Ah.  Ok but in that case I would say that build_cdat_table() should never
> > > return < 0 to be clear at this level what can happen.
> > > 
> > > Would you like a patch for that?  (/me assumes you dropped this patch)  
> > 
> > Probably needs to first rip out all the -ENOMEM returns that got into
> > the CXL code in general, then tidy up the return type to be unsigned.
> > 
> > If you want to do that it would be welcome!
> Actually.  Build_cdat_table() can return errors just not for this reason.
> 
> host_memory_backend_get_memory() can fail for example.

I must be on a different version because I don't see that.

>
> So original patch is good
> as is, just that the discussion of memory allocation failure threw me
> off and should be cleaned up separately.
> 

I did this testing on Fan's DCD version...  :-/  ... probably very out of
date.

Fan do you have a newer version than your 2023-11-16 branch?

Ira

Re: [PATCH v2 1/2] cxl/cdat: Handle cdat table build errors

[Jonathan Cameron]

On Mon, 8 Jan 2024 18:48:48 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> Jonathan Cameron wrote:
> 
> [snip]
> 
> > > > > > 
> > > > > > I did not go that far as I am unsure as well.      
> > > > > Memory allocations in qemu don't fail (well if they do it crashes)
> > > > > Side effect of using glib which makes for simpler cases.
> > > > > https://docs.gtk.org/glib/func.malloc.html
> > > > > 
> > > > > There shouldn't even be any checks :(  I'll fix that up at somepoint
> > > > > across all the CXL emulation.  Sometimes reviewers noticed and
> > > > > we dropped it at earlier stages, but clearly didn't catch them all.
> > > > > 
> > > > > Which come to think of it is why this error condition is in practice
> > > > > not actually buggy as the code won't ever manage to return -ENOMEM and
> > > > > I don't think there are other error codes.      
> > > > 
> > > > Ah.  Ok but in that case I would say that build_cdat_table() should never
> > > > return < 0 to be clear at this level what can happen.
> > > > 
> > > > Would you like a patch for that?  (/me assumes you dropped this patch)    
> > > 
> > > Probably needs to first rip out all the -ENOMEM returns that got into
> > > the CXL code in general, then tidy up the return type to be unsigned.
> > > 
> > > If you want to do that it would be welcome!  
> > Actually.  Build_cdat_table() can return errors just not for this reason.
> > 
> > host_memory_backend_get_memory() can fail for example.  
> 
> I must be on a different version because I don't see that.
> 
> >
> > So original patch is good
> > as is, just that the discussion of memory allocation failure threw me
> > off and should be cleaned up separately.
> >   
> 
> I did this testing on Fan's DCD version...  :-/  ... probably very out of
> date.

https://elixir.bootlin.com/qemu/latest/source/hw/mem/cxl_type3.c#L183
https://elixir.bootlin.com/qemu/v8.1.0/source/hw/mem/cxl_type3.c#L171
been there a while, but meh, too many branches floating around :)

> 
> Fan do you have a newer version than your 2023-11-16 branch?
> 


> Ira
> 

[PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[Ira Weiny]

The addition of the DCD support for CXL type-3 devices extended the CDAT
table large enough that the checksum being returned was incorrect.[1]

This was because the checksum value was using the header length field
rather than each of the 4 bytes of the length field.  This was
previously not seen because the length of the CDAT data was less than
256 thus resulting in an equivalent checksum value.

Properly calculate the checksum for the CDAT header.

[1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/

Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes from V1:
[djiang: Remove do {} while (0);]
---
 hw/cxl/cxl-cdat.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 24829cf2428d..67e44a4f992a 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -49,6 +49,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
     g_autofree CDATTableHeader *cdat_header = NULL;
     g_autofree CDATEntry *cdat_st = NULL;
     uint8_t sum = 0;
+    uint8_t *buf;
     int ent, i;
 
     /* Use default table if fopen == NULL */
@@ -95,8 +96,12 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
     /* For now, no runtime updates */
     cdat_header->sequence = 0;
     cdat_header->length += sizeof(CDATTableHeader);
-    sum += cdat_header->revision + cdat_header->sequence +
-        cdat_header->length;
+
+    buf = (uint8_t *)cdat_header;
+    for (i = 0; i < sizeof(*cdat_header); i++) {
+        sum += buf[i];
+    }
+
     /* Sum of all bytes including checksum must be 0 */
     cdat_header->checksum = ~sum + 1;
 

-- 
2.42.0

Re: [PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[Dave Jiang]

On 11/29/23 18:33, Ira Weiny wrote:
> The addition of the DCD support for CXL type-3 devices extended the CDAT
> table large enough that the checksum being returned was incorrect.[1]
> 
> This was because the checksum value was using the header length field
> rather than each of the 4 bytes of the length field.  This was
> previously not seen because the length of the CDAT data was less than
> 256 thus resulting in an equivalent checksum value.
> 
> Properly calculate the checksum for the CDAT header.
> 
> [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
> 
> Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

Def more future proof if they introduce new fields in later versions of the table.

> 
> ---
> Changes from V1:
> [djiang: Remove do {} while (0);]
> ---
>  hw/cxl/cxl-cdat.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 24829cf2428d..67e44a4f992a 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -49,6 +49,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>      g_autofree CDATTableHeader *cdat_header = NULL;
>      g_autofree CDATEntry *cdat_st = NULL;
>      uint8_t sum = 0;
> +    uint8_t *buf;
>      int ent, i;
>  
>      /* Use default table if fopen == NULL */
> @@ -95,8 +96,12 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>      /* For now, no runtime updates */
>      cdat_header->sequence = 0;
>      cdat_header->length += sizeof(CDATTableHeader);
> -    sum += cdat_header->revision + cdat_header->sequence +
> -        cdat_header->length;
> +
> +    buf = (uint8_t *)cdat_header;
> +    for (i = 0; i < sizeof(*cdat_header); i++) {
> +        sum += buf[i];
> +    }
> +
>      /* Sum of all bytes including checksum must be 0 */
>      cdat_header->checksum = ~sum + 1;
>  
> 

Re: [PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[Jonathan Cameron]

On Wed, 29 Nov 2023 17:33:04 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> The addition of the DCD support for CXL type-3 devices extended the CDAT
> table large enough that the checksum being returned was incorrect.[1]
> 
> This was because the checksum value was using the header length field
> rather than each of the 4 bytes of the length field.  This was
> previously not seen because the length of the CDAT data was less than
> 256 thus resulting in an equivalent checksum value.
> 
> Properly calculate the checksum for the CDAT header.
> 
> [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
> 
> Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

This only becomes a problem with the addition of DCDs so I'm not going to rush it in.
Btw cc qemu-devel on qemu patches!


I'll add it to my tree for now.

Jonathan


> 
> ---
> Changes from V1:
> [djiang: Remove do {} while (0);]
> ---
>  hw/cxl/cxl-cdat.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 24829cf2428d..67e44a4f992a 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -49,6 +49,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>      g_autofree CDATTableHeader *cdat_header = NULL;
>      g_autofree CDATEntry *cdat_st = NULL;
>      uint8_t sum = 0;
> +    uint8_t *buf;
>      int ent, i;
>  
>      /* Use default table if fopen == NULL */
> @@ -95,8 +96,12 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>      /* For now, no runtime updates */
>      cdat_header->sequence = 0;
>      cdat_header->length += sizeof(CDATTableHeader);
> -    sum += cdat_header->revision + cdat_header->sequence +
> -        cdat_header->length;
> +
> +    buf = (uint8_t *)cdat_header;
> +    for (i = 0; i < sizeof(*cdat_header); i++) {
> +        sum += buf[i];
> +    }
> +
>      /* Sum of all bytes including checksum must be 0 */
>      cdat_header->checksum = ~sum + 1;
>  
> 

Re: [PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[Ira Weiny]

Jonathan Cameron wrote:
> On Wed, 29 Nov 2023 17:33:04 -0800
> Ira Weiny <ira.weiny@intel.com> wrote:
> 

[snip]

> > [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
> > 
> > Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> This only becomes a problem with the addition of DCDs so I'm not going to rush it in.

That makes sense.

> Btw cc qemu-devel on qemu patches!
> 

Ah...  yea my bad.

> 
> I'll add it to my tree for now.

Thanks!
Ira

Re: [PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[Dan Williams]

Ira Weiny wrote:
> Jonathan Cameron wrote:
> > On Wed, 29 Nov 2023 17:33:04 -0800
> > Ira Weiny <ira.weiny@intel.com> wrote:
> > 
> 
> [snip]
> 
> > > [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
> > > 
> > > Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> > > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > This only becomes a problem with the addition of DCDs so I'm not going to rush it in.
> 
> That makes sense.
> 
> > Btw cc qemu-devel on qemu patches!
> > 
> 
> Ah...  yea my bad.

Might I also ask for a more prominent way to quickly identify kernel vs
qemu patches, like a "[QEMU PATCH]" prefix? I tend to look for "hw/" in
the diff path names, but the kernel vs qemu question is ambiguous when
looking at the linux-cxl Patchwork queue.

@Jonathan, what do you think of having the kernel patchwork-bot watch
your tree for updating patch state (if it is not happening already).

Re: [PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[Jonathan Cameron]

On Mon, 18 Dec 2023 15:30:14 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Ira Weiny wrote:
> > Jonathan Cameron wrote:  
> > > On Wed, 29 Nov 2023 17:33:04 -0800
> > > Ira Weiny <ira.weiny@intel.com> wrote:
> > >   
> > 
> > [snip]
> >   
> > > > [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
> > > > 
> > > > Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> > > > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>  
> > > 
> > > This only becomes a problem with the addition of DCDs so I'm not going to rush it in.  
> > 
> > That makes sense.
> >   
> > > Btw cc qemu-devel on qemu patches!
> > >   
> > 
> > Ah...  yea my bad.  
> 
> Might I also ask for a more prominent way to quickly identify kernel vs
> qemu patches, like a "[QEMU PATCH]" prefix? I tend to look for "hw/" in
> the diff path names, but the kernel vs qemu question is ambiguous when
> looking at the linux-cxl Patchwork queue.
I'm not sure if the QEMU maintainers would be that keen on a tag there.
Maybe just stick qemu/cxl: in the cover letter naming as a prefix?
[PATCH 0/4] qemu/cxl: Whatever the change is

> 
> @Jonathan, what do you think of having the kernel patchwork-bot watch
> your tree for updating patch state (if it is not happening already).
My QEMU tree is a bit intermittent and frequently rebased as I'm juggling
too many patches. Not sure we'd get a good match.  Mind you I've
never tried the bot so not even sure how to configure it.

Jonathan

Re: [PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[Dan Williams]

Jonathan Cameron wrote:
> On Mon, 18 Dec 2023 15:30:14 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Ira Weiny wrote:
> > > Jonathan Cameron wrote:  
> > > > On Wed, 29 Nov 2023 17:33:04 -0800
> > > > Ira Weiny <ira.weiny@intel.com> wrote:
> > > >   
> > > 
> > > [snip]
> > >   
> > > > > [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
> > > > > 
> > > > > Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> > > > > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>  
> > > > 
> > > > This only becomes a problem with the addition of DCDs so I'm not going to rush it in.  
> > > 
> > > That makes sense.
> > >   
> > > > Btw cc qemu-devel on qemu patches!
> > > >   
> > > 
> > > Ah...  yea my bad.  
> > 
> > Might I also ask for a more prominent way to quickly identify kernel vs
> > qemu patches, like a "[QEMU PATCH]" prefix? I tend to look for "hw/" in
> > the diff path names, but the kernel vs qemu question is ambiguous when
> > looking at the linux-cxl Patchwork queue.
> I'm not sure if the QEMU maintainers would be that keen on a tag there.
> Maybe just stick qemu/cxl: in the cover letter naming as a prefix?
> [PATCH 0/4] qemu/cxl: Whatever the change is

+1 from me.

> > @Jonathan, what do you think of having the kernel patchwork-bot watch
> > your tree for updating patch state (if it is not happening already).
> My QEMU tree is a bit intermittent and frequently rebased as I'm juggling
> too many patches. Not sure we'd get a good match.  Mind you I've
> never tried the bot so not even sure how to configure it.

Here's the documentation:
https://korg.docs.kernel.org/patchwork/index.html

The basics are you just point the bot at kernel tree and whenever that
tree is updated it checks if any of the new commits match patchwork
patches by git-patch-id (or equivalent). Rebases are ok as it will just
"re-accept" the patch with new commit id. The main benefit it has is
transitioning patches to the Accepted state, or Mainline state depending
on what branch you tell it represents those states.

It does require a git.kernel.org tree to monitor, but we might already
get benefit from just pointing it to:

https://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git/

...to automatically mark patches as "Accepted". The "Superseded" state
comes for free with the existing patchwork-bot monitoring of the
linux-cxl@ list.

Re: [PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[Jonathan Cameron]

On Wed, 29 Nov 2023 17:33:04 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> The addition of the DCD support for CXL type-3 devices extended the CDAT
> table large enough that the checksum being returned was incorrect.[1]
> 
> This was because the checksum value was using the header length field
> rather than each of the 4 bytes of the length field.  This was
> previously not seen because the length of the CDAT data was less than
> 256 thus resulting in an equivalent checksum value.
> 
> Properly calculate the checksum for the CDAT header.
> 
> [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
> 
> Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes from V1:
> [djiang: Remove do {} while (0);]
> ---
>  hw/cxl/cxl-cdat.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 24829cf2428d..67e44a4f992a 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -49,6 +49,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>      g_autofree CDATTableHeader *cdat_header = NULL;
>      g_autofree CDATEntry *cdat_st = NULL;
>      uint8_t sum = 0;
> +    uint8_t *buf;
This results in a shadowing variable warning. I'll rename it to hdr_buf or something
like that.
>      int ent, i;
>  
>      /* Use default table if fopen == NULL */
> @@ -95,8 +96,12 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>      /* For now, no runtime updates */
>      cdat_header->sequence = 0;
>      cdat_header->length += sizeof(CDATTableHeader);
> -    sum += cdat_header->revision + cdat_header->sequence +
> -        cdat_header->length;
> +
> +    buf = (uint8_t *)cdat_header;
> +    for (i = 0; i < sizeof(*cdat_header); i++) {
> +        sum += buf[i];
> +    }
> +
>      /* Sum of all bytes including checksum must be 0 */
>      cdat_header->checksum = ~sum + 1;
>  
> 

Re: [PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[Ira Weiny]

Jonathan Cameron wrote:
> On Wed, 29 Nov 2023 17:33:04 -0800
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > The addition of the DCD support for CXL type-3 devices extended the CDAT
> > table large enough that the checksum being returned was incorrect.[1]
> > 
> > This was because the checksum value was using the header length field
> > rather than each of the 4 bytes of the length field.  This was
> > previously not seen because the length of the CDAT data was less than
> > 256 thus resulting in an equivalent checksum value.
> > 
> > Properly calculate the checksum for the CDAT header.
> > 
> > [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
> > 
> > Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > Changes from V1:
> > [djiang: Remove do {} while (0);]
> > ---
> >  hw/cxl/cxl-cdat.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > index 24829cf2428d..67e44a4f992a 100644
> > --- a/hw/cxl/cxl-cdat.c
> > +++ b/hw/cxl/cxl-cdat.c
> > @@ -49,6 +49,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> >      g_autofree CDATTableHeader *cdat_header = NULL;
> >      g_autofree CDATEntry *cdat_st = NULL;
> >      uint8_t sum = 0;
> > +    uint8_t *buf;
> This results in a shadowing variable warning. I'll rename it to hdr_buf or something
> like that.

<sigh>  sorry again...

With all the discussion did you want me to re-roll the set?

AFAICS this is the only actual issue.  So if you want to do it that would
be great.

Thanks,
Ira

Re: [PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[Jonathan Cameron]

On Tue, 19 Dec 2023 15:32:18 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Wed, 29 Nov 2023 17:33:04 -0800
> > Ira Weiny <ira.weiny@intel.com> wrote:
> >   
> > > The addition of the DCD support for CXL type-3 devices extended the CDAT
> > > table large enough that the checksum being returned was incorrect.[1]
> > > 
> > > This was because the checksum value was using the header length field
> > > rather than each of the 4 bytes of the length field.  This was
> > > previously not seen because the length of the CDAT data was less than
> > > 256 thus resulting in an equivalent checksum value.
> > > 
> > > Properly calculate the checksum for the CDAT header.
> > > 
> > > [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
> > > 
> > > Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> > > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > ---
> > > Changes from V1:
> > > [djiang: Remove do {} while (0);]
> > > ---
> > >  hw/cxl/cxl-cdat.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > > index 24829cf2428d..67e44a4f992a 100644
> > > --- a/hw/cxl/cxl-cdat.c
> > > +++ b/hw/cxl/cxl-cdat.c
> > > @@ -49,6 +49,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> > >      g_autofree CDATTableHeader *cdat_header = NULL;
> > >      g_autofree CDATEntry *cdat_st = NULL;
> > >      uint8_t sum = 0;
> > > +    uint8_t *buf;  
> > This results in a shadowing variable warning. I'll rename it to hdr_buf or something
> > like that.  
> 
> <sigh>  sorry again...
> 
> With all the discussion did you want me to re-roll the set?
> 
> AFAICS this is the only actual issue.  So if you want to do it that would
> be great.
> 
I've done it locally - just not dealt with some other stuff on that tree yet hence
not pushed out.  Will get to that sometime this week hopefully.

Jonathan

> Thanks,
> Ira

Re: [PATCH v2 2/2] cxl/cdat: Fix header sum value in CDAT checksum

[fan]

On Wed, Nov 29, 2023 at 05:33:04PM -0800, Ira Weiny wrote:
> The addition of the DCD support for CXL type-3 devices extended the CDAT
> table large enough that the checksum being returned was incorrect.[1]
> 
> This was because the checksum value was using the header length field
> rather than each of the 4 bytes of the length field.  This was
> previously not seen because the length of the CDAT data was less than
> 256 thus resulting in an equivalent checksum value.
> 
> Properly calculate the checksum for the CDAT header.
> 
> [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
> 
> Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes from V1:
> [djiang: Remove do {} while (0);]
> ---
>  hw/cxl/cxl-cdat.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 24829cf2428d..67e44a4f992a 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -49,6 +49,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>      g_autofree CDATTableHeader *cdat_header = NULL;
>      g_autofree CDATEntry *cdat_st = NULL;
>      uint8_t sum = 0;
> +    uint8_t *buf;
>      int ent, i;
>  
>      /* Use default table if fopen == NULL */
> @@ -95,8 +96,12 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>      /* For now, no runtime updates */
>      cdat_header->sequence = 0;
>      cdat_header->length += sizeof(CDATTableHeader);
> -    sum += cdat_header->revision + cdat_header->sequence +
> -        cdat_header->length;
> +
> +    buf = (uint8_t *)cdat_header;
> +    for (i = 0; i < sizeof(*cdat_header); i++) {
> +        sum += buf[i];
> +    }
> +
>      /* Sum of all bytes including checksum must be 0 */
>      cdat_header->checksum = ~sum + 1;
>  
> 

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> -- 
> 2.42.0
>