| Message ID | 20240913123954.1907305-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/ucode: Improve commentary for parsing AMD containers | expand |
On 13.09.2024 14:39, Andrew Cooper wrote: > Despite writing this code, it's not the easiest logic to follow. > > Shorten the UCODE_EQUIV_TYPE name, and provide more of an explanation of > what's going on. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> I'm okay with this as is, so Acked-by: Jan Beulich <jbeulich@suse.com> yet ... > @@ -335,10 +335,10 @@ static struct microcode_patch *cf_check cpu_request_microcode( > buf += 4; > size -= 4; > > - if ( size < sizeof(*et) || > - (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE || > - size - sizeof(*et) < et->len || > - et->len % sizeof(et->eq[0]) ) > + if ( size < sizeof(*et) || /* Insufficient space for header? */ > + (et = buf)->type != UCODE_EQUIV_TYPE || /* Not an Equivalence Table? */ > + size - sizeof(*et) < et->len || /* Insufficient space for table? */ > + et->len % sizeof(et->eq[0]) ) /* Not a multiple of equiv_cpu_entry? */ ... this of course goes quite a bit beyond 80 cols (yet worse for the other block further down). How about if ( /* Insufficient space for header? */ size < sizeof(*et) || /* Not an Equivalence Table? */ (et = buf)->type != UCODE_EQUIV_TYPE || /* Insufficient space for table? */ size - sizeof(*et) < et->len || /* Not a multiple of equiv_cpu_entry? */ et->len % sizeof(et->eq[0]) ) ? Jan
On 13/09/2024 6:56 pm, Jan Beulich wrote: > On 13.09.2024 14:39, Andrew Cooper wrote: >> Despite writing this code, it's not the easiest logic to follow. >> >> Shorten the UCODE_EQUIV_TYPE name, and provide more of an explanation of >> what's going on. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > I'm okay with this as is, so > Acked-by: Jan Beulich <jbeulich@suse.com> Thanks. > yet ... > >> @@ -335,10 +335,10 @@ static struct microcode_patch *cf_check cpu_request_microcode( >> buf += 4; >> size -= 4; >> >> - if ( size < sizeof(*et) || >> - (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE || >> - size - sizeof(*et) < et->len || >> - et->len % sizeof(et->eq[0]) ) >> + if ( size < sizeof(*et) || /* Insufficient space for header? */ >> + (et = buf)->type != UCODE_EQUIV_TYPE || /* Not an Equivalence Table? */ >> + size - sizeof(*et) < et->len || /* Insufficient space for table? */ >> + et->len % sizeof(et->eq[0]) ) /* Not a multiple of equiv_cpu_entry? */ > ... this of course goes quite a bit beyond 80 cols (yet worse for the > other block further down). How about > > > if ( /* Insufficient space for header? */ > size < sizeof(*et) || > /* Not an Equivalence Table? */ > (et = buf)->type != UCODE_EQUIV_TYPE || > /* Insufficient space for table? */ > size - sizeof(*et) < et->len || > /* Not a multiple of equiv_cpu_entry? */ > et->len % sizeof(et->eq[0]) ) > > ? That really interferes with legibility. We explicitly tolerate longer printk lines for legibility and grepability, and this is the same. I've managed to reduce the line length a bit with some rewording, but I'm going put it in with this basic form. ~Andrew
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 32490c8b7d2a..35bcec643c04 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -52,11 +52,11 @@ struct microcode_patch { }; #define UCODE_MAGIC 0x00414d44 -#define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000 +#define UCODE_EQUIV_TYPE 0x00000000 #define UCODE_UCODE_TYPE 0x00000001 struct container_equiv_table { - uint32_t type; /* UCODE_EQUIV_CPU_TABLE_TYPE */ + uint32_t type; /* UCODE_EQUIV_TYPE */ uint32_t len; struct equiv_cpu_entry eq[]; }; @@ -335,10 +335,10 @@ static struct microcode_patch *cf_check cpu_request_microcode( buf += 4; size -= 4; - if ( size < sizeof(*et) || - (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE || - size - sizeof(*et) < et->len || - et->len % sizeof(et->eq[0]) ) + if ( size < sizeof(*et) || /* Insufficient space for header? */ + (et = buf)->type != UCODE_EQUIV_TYPE || /* Not an Equivalence Table? */ + size - sizeof(*et) < et->len || /* Insufficient space for table? */ + et->len % sizeof(et->eq[0]) ) /* Not a multiple of equiv_cpu_entry? */ { printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n"); error = -EINVAL; @@ -351,7 +351,12 @@ static struct microcode_patch *cf_check cpu_request_microcode( error = scan_equiv_cpu_table(et); - /* -ESRCH means no applicable microcode in this container. */ + /* + * -ESRCH means no applicable microcode in this container. But, there + * might be subsequent containers in the blob. Skipping to the end of + * this container still requires us to follow the UCODE_UCODE_TYPE/len + * metadata because there's no overall container length given. + */ if ( error && error != -ESRCH ) break; skip_ucode = error; @@ -361,10 +366,10 @@ static struct microcode_patch *cf_check cpu_request_microcode( { const struct container_microcode *mc; - if ( size < sizeof(*mc) || - (mc = buf)->type != UCODE_UCODE_TYPE || - size - sizeof(*mc) < mc->len || - mc->len < sizeof(struct microcode_patch) ) + if ( size < sizeof(*mc) || /* Insufficient space for container header? */ + (mc = buf)->type != UCODE_UCODE_TYPE || /* Not an ucode blob? */ + size - sizeof(*mc) < mc->len || /* Insufficient space for blob? */ + mc->len < sizeof(struct microcode_patch) ) /* Insufficient space for patch header? */ { printk(XENLOG_ERR "microcode: Bad microcode data\n"); error = -EINVAL;
Despite writing this code, it's not the easiest logic to follow. Shorten the UCODE_EQUIV_TYPE name, and provide more of an explanation of what's going on. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/cpu/microcode/amd.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)