| Message ID | 20231117-fix-cdat-cs-v2-2-715399976d4d@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl/cdat: Fixes for CXL CDAT processing | expand |
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; > >
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; > >
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; > >
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
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).
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
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 >
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
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.
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
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;
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(-)