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/[email protected]/
Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
Cc: Huai-Cheng Kuo <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
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
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/[email protected]/
>
> Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> Cc: Huai-Cheng Kuo <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
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;
>
>
Jonathan Cameron wrote:
> On Wed, 29 Nov 2023 17:33:04 -0800
> Ira Weiny <[email protected]> wrote:
>
[snip]
> > [1] https://lore.kernel.org/all/[email protected]/
> >
> > Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> > Cc: Huai-Cheng Kuo <[email protected]>
> > Signed-off-by: Ira Weiny <[email protected]>
>
> 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
Ira Weiny wrote:
> Jonathan Cameron wrote:
> > On Wed, 29 Nov 2023 17:33:04 -0800
> > Ira Weiny <[email protected]> wrote:
> >
>
> [snip]
>
> > > [1] https://lore.kernel.org/all/[email protected]/
> > >
> > > Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> > > Cc: Huai-Cheng Kuo <[email protected]>
> > > Signed-off-by: Ira Weiny <[email protected]>
> >
> > 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).
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/[email protected]/
>
> Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> Cc: Huai-Cheng Kuo <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
>
> ---
> 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 <[email protected]>
> --
> 2.42.0
>
Jonathan Cameron wrote:
> On Wed, 29 Nov 2023 17:33:04 -0800
> Ira Weiny <[email protected]> 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/[email protected]/
> >
> > Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> > Cc: Huai-Cheng Kuo <[email protected]>
> > Signed-off-by: Ira Weiny <[email protected]>
> >
> > ---
> > 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
Jonathan Cameron wrote:
> On Mon, 18 Dec 2023 15:30:14 -0800
> Dan Williams <[email protected]> wrote:
>
> > Ira Weiny wrote:
> > > Jonathan Cameron wrote:
> > > > On Wed, 29 Nov 2023 17:33:04 -0800
> > > > Ira Weiny <[email protected]> wrote:
> > > >
> > >
> > > [snip]
> > >
> > > > > [1] https://lore.kernel.org/all/[email protected]/
> > > > >
> > > > > Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
> > > > > Cc: Huai-Cheng Kuo <[email protected]>
> > > > > Signed-off-by: Ira Weiny <[email protected]>
> > > >
> > > > 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.