| Message ID | 20200323101724.15655-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/ucode: Cleanup and fixes - Part 3/n (Intel) | expand |
On 23.03.2020 11:17, Andrew Cooper wrote: > Every caller actually passes a struct microcode_header_intel. Implement the > helpers with proper types, and leave a comment explaining the Pentium Pro/II > behaviour with empty {data,total}size fields. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with... > --- a/xen/arch/x86/cpu/microcode/intel.c > +++ b/xen/arch/x86/cpu/microcode/intel.c > @@ -46,9 +46,16 @@ struct microcode_header_intel { > unsigned int sig; > unsigned int cksum; > unsigned int ldrver; > + > + /* > + * Microcode for the Pentium Pro and II had all further fields in the > + * header reserved, had a fixed datasize of 2000 and totalsize of 2048, > + * and didn't use platform flags despite the availability of the MSR. > + */ > + > unsigned int pf; > - unsigned int datasize; > - unsigned int totalsize; > + unsigned int _datasize; > + unsigned int _totalsize; ... the underscores here dropped again. Or else - why did you add them? This (to me at least) doesn't e.g. make any more clear that the fields may be zero on old hardware. Furthermore - do we really need this PPro/PentiumII logic seeing that these aren't 64-bit capable CPUs? Jan
On 25/03/2020 13:41, Jan Beulich wrote: > On 23.03.2020 11:17, Andrew Cooper wrote: >> Every caller actually passes a struct microcode_header_intel. Implement the >> helpers with proper types, and leave a comment explaining the Pentium Pro/II >> behaviour with empty {data,total}size fields. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with... > >> --- a/xen/arch/x86/cpu/microcode/intel.c >> +++ b/xen/arch/x86/cpu/microcode/intel.c >> @@ -46,9 +46,16 @@ struct microcode_header_intel { >> unsigned int sig; >> unsigned int cksum; >> unsigned int ldrver; >> + >> + /* >> + * Microcode for the Pentium Pro and II had all further fields in the >> + * header reserved, had a fixed datasize of 2000 and totalsize of 2048, >> + * and didn't use platform flags despite the availability of the MSR. >> + */ >> + >> unsigned int pf; >> - unsigned int datasize; >> - unsigned int totalsize; >> + unsigned int _datasize; >> + unsigned int _totalsize; > ... the underscores here dropped again. Or else - why did you add > them? This (to me at least) doesn't e.g. make any more clear that > the fields may be zero on old hardware. No, but it is our normal hint that you shouldn't be using the field directly, and should be using the accessors instead. > Furthermore - do we really need this PPro/PentiumII logic seeing > that these aren't 64-bit capable CPUs? I did actually drop support in one version of my series, but put it back in. These old microcode blobs are still around, including in some versions of microcode.dat. By dropping the ability to recognise them as legitimate, we'd break the logic to search through a container of multiple blobs to find the one which matches. ~Andrew
On 26.03.2020 15:35, Andrew Cooper wrote: > On 25/03/2020 13:41, Jan Beulich wrote: >> On 23.03.2020 11:17, Andrew Cooper wrote: >>> --- a/xen/arch/x86/cpu/microcode/intel.c >>> +++ b/xen/arch/x86/cpu/microcode/intel.c >>> @@ -46,9 +46,16 @@ struct microcode_header_intel { >>> unsigned int sig; >>> unsigned int cksum; >>> unsigned int ldrver; >>> + >>> + /* >>> + * Microcode for the Pentium Pro and II had all further fields in the >>> + * header reserved, had a fixed datasize of 2000 and totalsize of 2048, >>> + * and didn't use platform flags despite the availability of the MSR. >>> + */ >>> + >>> unsigned int pf; >>> - unsigned int datasize; >>> - unsigned int totalsize; >>> + unsigned int _datasize; >>> + unsigned int _totalsize; >> ... the underscores here dropped again. Or else - why did you add >> them? This (to me at least) doesn't e.g. make any more clear that >> the fields may be zero on old hardware. > > No, but it is our normal hint that you shouldn't be using the field > directly, and should be using the accessors instead. Yet in patch 5 you do. Perhaps for an understandable reason, but that way you at least partly invalidate what you say above. >> Furthermore - do we really need this PPro/PentiumII logic seeing >> that these aren't 64-bit capable CPUs? > > I did actually drop support in one version of my series, but put it back in. > > These old microcode blobs are still around, including in some versions > of microcode.dat. By dropping the ability to recognise them as > legitimate, we'd break the logic to search through a container of > multiple blobs to find the one which matches. Oh, good point. Jan
On 26/03/2020 14:56, Jan Beulich wrote: > On 26.03.2020 15:35, Andrew Cooper wrote: >> On 25/03/2020 13:41, Jan Beulich wrote: >>> On 23.03.2020 11:17, Andrew Cooper wrote: >>>> --- a/xen/arch/x86/cpu/microcode/intel.c >>>> +++ b/xen/arch/x86/cpu/microcode/intel.c >>>> @@ -46,9 +46,16 @@ struct microcode_header_intel { >>>> unsigned int sig; >>>> unsigned int cksum; >>>> unsigned int ldrver; >>>> + >>>> + /* >>>> + * Microcode for the Pentium Pro and II had all further fields in the >>>> + * header reserved, had a fixed datasize of 2000 and totalsize of 2048, >>>> + * and didn't use platform flags despite the availability of the MSR. >>>> + */ >>>> + >>>> unsigned int pf; >>>> - unsigned int datasize; >>>> - unsigned int totalsize; >>>> + unsigned int _datasize; >>>> + unsigned int _totalsize; >>> ... the underscores here dropped again. Or else - why did you add >>> them? This (to me at least) doesn't e.g. make any more clear that >>> the fields may be zero on old hardware. >> No, but it is our normal hint that you shouldn't be using the field >> directly, and should be using the accessors instead. > Yet in patch 5 you do. Perhaps for an understandable reason, but > that way you at least partly invalidate what you say above. The net result of of patch 5 is three adjacent helpers, which are the only code which use the fields themselves. I can drop if you really insist. We're only talking about 400 lines or code, or thereabouts. >>> Furthermore - do we really need this PPro/PentiumII logic seeing >>> that these aren't 64-bit capable CPUs? >> I did actually drop support in one version of my series, but put it back in. >> >> These old microcode blobs are still around, including in some versions >> of microcode.dat. By dropping the ability to recognise them as >> legitimate, we'd break the logic to search through a container of >> multiple blobs to find the one which matches. > Oh, good point. Shame I only came to that realisation after having reworked the series to take it out... I'm constructing companion document to https://xenbits.xen.org/docs/sphinx-unstable/admin-guide/microcode-loading.html which will live in hypervisor-guide section, and cover a whole range of topics like this. ~Andrew
On 26.03.2020 16:09, Andrew Cooper wrote: > On 26/03/2020 14:56, Jan Beulich wrote: >> On 26.03.2020 15:35, Andrew Cooper wrote: >>> On 25/03/2020 13:41, Jan Beulich wrote: >>>> On 23.03.2020 11:17, Andrew Cooper wrote: >>>>> --- a/xen/arch/x86/cpu/microcode/intel.c >>>>> +++ b/xen/arch/x86/cpu/microcode/intel.c >>>>> @@ -46,9 +46,16 @@ struct microcode_header_intel { >>>>> unsigned int sig; >>>>> unsigned int cksum; >>>>> unsigned int ldrver; >>>>> + >>>>> + /* >>>>> + * Microcode for the Pentium Pro and II had all further fields in the >>>>> + * header reserved, had a fixed datasize of 2000 and totalsize of 2048, >>>>> + * and didn't use platform flags despite the availability of the MSR. >>>>> + */ >>>>> + >>>>> unsigned int pf; >>>>> - unsigned int datasize; >>>>> - unsigned int totalsize; >>>>> + unsigned int _datasize; >>>>> + unsigned int _totalsize; >>>> ... the underscores here dropped again. Or else - why did you add >>>> them? This (to me at least) doesn't e.g. make any more clear that >>>> the fields may be zero on old hardware. >>> No, but it is our normal hint that you shouldn't be using the field >>> directly, and should be using the accessors instead. >> Yet in patch 5 you do. Perhaps for an understandable reason, but >> that way you at least partly invalidate what you say above. > > The net result of of patch 5 is three adjacent helpers, which are the > only code which use the fields themselves. > > I can drop if you really insist. We're only talking about 400 lines or > code, or thereabouts. Well, I find it odd this way, but no, if you're convinced it's better, I'm not going to insist. Jan
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c index 0cceac6255..dfe44679be 100644 --- a/xen/arch/x86/cpu/microcode/intel.c +++ b/xen/arch/x86/cpu/microcode/intel.c @@ -46,9 +46,16 @@ struct microcode_header_intel { unsigned int sig; unsigned int cksum; unsigned int ldrver; + + /* + * Microcode for the Pentium Pro and II had all further fields in the + * header reserved, had a fixed datasize of 2000 and totalsize of 2048, + * and didn't use platform flags despite the availability of the MSR. + */ + unsigned int pf; - unsigned int datasize; - unsigned int totalsize; + unsigned int _datasize; + unsigned int _totalsize; unsigned int reserved[3]; }; @@ -75,20 +82,21 @@ struct microcode_patch { struct microcode_intel *mc_intel; }; -#define DEFAULT_UCODE_DATASIZE (2000) +#define PPRO_UCODE_DATASIZE 2000 #define MC_HEADER_SIZE (sizeof(struct microcode_header_intel)) -#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE) #define EXT_HEADER_SIZE (sizeof(struct extended_sigtable)) #define EXT_SIGNATURE_SIZE (sizeof(struct extended_signature)) #define DWSIZE (sizeof(u32)) -#define get_totalsize(mc) \ - (((struct microcode_intel *)mc)->hdr.totalsize ? \ - ((struct microcode_intel *)mc)->hdr.totalsize : \ - DEFAULT_UCODE_TOTALSIZE) - -#define get_datasize(mc) \ - (((struct microcode_intel *)mc)->hdr.datasize ? \ - ((struct microcode_intel *)mc)->hdr.datasize : DEFAULT_UCODE_DATASIZE) + +static uint32_t get_datasize(const struct microcode_header_intel *hdr) +{ + return hdr->_datasize ?: PPRO_UCODE_DATASIZE; +} + +static uint32_t get_totalsize(const struct microcode_header_intel *hdr) +{ + return hdr->_totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE; +} #define sigmatch(s1, s2, p1, p2) \ (((s1) == (s2)) && (((p1) & (p2)) || (((p1) == 0) && ((p2) == 0))))
Every caller actually passes a struct microcode_header_intel. Implement the helpers with proper types, and leave a comment explaining the Pentium Pro/II behaviour with empty {data,total}size fields. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/cpu/microcode/intel.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)