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

[PATCH 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]

Does the QEMU code require fixes tags like the kernel?

[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>
---
Ira Weiny (2):
      cxl/cdat: Handle cdat table build errors
      cxl/cdat: Fix header sum value in CDAT checksum

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

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

[PATCH 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.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
This likely needs to have some more robust error handling in the event
of errors.  But this at least prevents more errors down the line with
invalid allocations.
---
 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.41.0

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

[Dave Jiang]

On 11/17/23 18:14, 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.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> This likely needs to have some more robust error handling in the event
> of errors.  But this at least prevents more errors down the line with
> invalid allocations.
> ---
>  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;
> 

[PATCH 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/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 hw/cxl/cxl-cdat.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 24829cf2428d..d93e2e4e64f2 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -95,8 +95,15 @@ 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;
+
+    do {
+        uint8_t *buf = (uint8_t *)cdat_header;
+
+        for (i = 0; i < sizeof(*cdat_header); i++) {
+            sum += buf[i];
+        }
+    } while (0);
+
     /* Sum of all bytes including checksum must be 0 */
     cdat_header->checksum = ~sum + 1;
 

-- 
2.41.0

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

[Dave Jiang]

On 11/17/23 18:14, 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/
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  hw/cxl/cxl-cdat.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 24829cf2428d..d93e2e4e64f2 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -95,8 +95,15 @@ 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;
> +
> +    do {
> +        uint8_t *buf = (uint8_t *)cdat_header;
> +
> +        for (i = 0; i < sizeof(*cdat_header); i++) {
> +            sum += buf[i];
> +        }
> +    } while (0);

Why the empty do/while loop?

> +
>      /* Sum of all bytes including checksum must be 0 */
>      cdat_header->checksum = ~sum + 1;
>  
> 

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

[Ira Weiny]

Dave Jiang wrote:
> 

[snip]

> > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > index 24829cf2428d..d93e2e4e64f2 100644
> > --- a/hw/cxl/cxl-cdat.c
> > +++ b/hw/cxl/cxl-cdat.c
> > @@ -95,8 +95,15 @@ 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;
> > +
> > +    do {
> > +        uint8_t *buf = (uint8_t *)cdat_header;
> > +
> > +        for (i = 0; i < sizeof(*cdat_header); i++) {
> > +            sum += buf[i];
> > +        }
> > +    } while (0);
> 
> Why the empty do/while loop?

Because I used the loop for debugging and forgot to clean up after it was
tested.

I'll send a v2,
Ira