| Message ID | 20240808134251.29995-2-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Expose consistent topology to guests | expand |
On 08.08.2024 15:42, Alejandro Vallejo wrote: > hvmloader's cpuid() implementation deviates from Xen's in that the value > passed on ecx is unspecified. This means that when used on leaves that > implement subleaves it's unspecified which one you get; though it's more > than likely an invalid one. > > Import Xen's implementation so there are no surprises. > > Fixes: 318ac791f9f9 ("Add utilities needed for SMBIOS generation to > hvmloader") > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Minor remark: A Fixes: tag wants to go all on a single line. > --- a/tools/firmware/hvmloader/util.c > +++ b/tools/firmware/hvmloader/util.c > @@ -267,15 +267,6 @@ memcmp(const void *s1, const void *s2, unsigned n) > return 0; > } > > -void > -cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > -{ > - asm volatile ( > - "cpuid" > - : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx) > - : "0" (idx) ); Compared to the original ... > --- a/tools/firmware/hvmloader/util.h > +++ b/tools/firmware/hvmloader/util.h > @@ -184,9 +184,30 @@ int uart_exists(uint16_t uart_base); > int lpt_exists(uint16_t lpt_base); > int hpet_exists(unsigned long hpet_base); > > -/* Do cpuid instruction, with operation 'idx' */ > -void cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx, > - uint32_t *ecx, uint32_t *edx); > +/* Some CPUID calls want 'count' to be placed in ecx */ > +static inline void cpuid_count( > + uint32_t leaf, > + uint32_t subleaf, > + uint32_t *eax, > + uint32_t *ebx, > + uint32_t *ecx, > + uint32_t *edx) > +{ > + asm volatile ( "cpuid" > + : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx) > + : "a" (leaf), "c" (subleaf) ); ... you alter indentation, without it becoming clear why you do so. Imo there are only two ways of indenting this which are conforming to our style - either as it was (secondary lines indented by one more level, i.e. 4 more spaces) or asm volatile ( "cpuid" : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx) : "a" (leaf), "c" (subleaf) ); I guess I'll take the liberty and adjust while committing. Jan
On Thu Aug 8, 2024 at 3:10 PM BST, Jan Beulich wrote: > On 08.08.2024 15:42, Alejandro Vallejo wrote: > > hvmloader's cpuid() implementation deviates from Xen's in that the value > > passed on ecx is unspecified. This means that when used on leaves that > > implement subleaves it's unspecified which one you get; though it's more > > than likely an invalid one. > > > > Import Xen's implementation so there are no surprises. > > > > Fixes: 318ac791f9f9 ("Add utilities needed for SMBIOS generation to > > hvmloader") > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Minor remark: A Fixes: tag wants to go all on a single line. Noted for next time. > > > --- a/tools/firmware/hvmloader/util.c > > +++ b/tools/firmware/hvmloader/util.c > > @@ -267,15 +267,6 @@ memcmp(const void *s1, const void *s2, unsigned n) > > return 0; > > } > > > > -void > > -cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > > -{ > > - asm volatile ( > > - "cpuid" > > - : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx) > > - : "0" (idx) ); > > Compared to the original ... > > > --- a/tools/firmware/hvmloader/util.h > > +++ b/tools/firmware/hvmloader/util.h > > @@ -184,9 +184,30 @@ int uart_exists(uint16_t uart_base); > > int lpt_exists(uint16_t lpt_base); > > int hpet_exists(unsigned long hpet_base); > > > > -/* Do cpuid instruction, with operation 'idx' */ > > -void cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx, > > - uint32_t *ecx, uint32_t *edx); > > +/* Some CPUID calls want 'count' to be placed in ecx */ > > +static inline void cpuid_count( > > + uint32_t leaf, > > + uint32_t subleaf, > > + uint32_t *eax, > > + uint32_t *ebx, > > + uint32_t *ecx, > > + uint32_t *edx) > > +{ > > + asm volatile ( "cpuid" > > + : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx) > > + : "a" (leaf), "c" (subleaf) ); > > ... you alter indentation, without it becoming clear why you do so. Imo > there are only two ways of indenting this which are conforming to our > style - either as it was (secondary lines indented by one more level, > i.e. 4 more spaces) or > > asm volatile ( "cpuid" > : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx) > : "a" (leaf), "c" (subleaf) ); > > I guess I'll take the liberty and adjust while committing. > > Jan Sure, I don't mind about that. As for the indentation difference, the inline assembly is taken quasi-verbatim from arch/x86/include/asm/processor.h. That one happens to have this indentation. Cheers, Alejandro
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c index c34f077b38e3..d3b3f9038e64 100644 --- a/tools/firmware/hvmloader/util.c +++ b/tools/firmware/hvmloader/util.c @@ -267,15 +267,6 @@ memcmp(const void *s1, const void *s2, unsigned n) return 0; } -void -cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) -{ - asm volatile ( - "cpuid" - : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx) - : "0" (idx) ); -} - static const char hex_digits[] = "0123456789abcdef"; /* Write a two-character hex representation of 'byte' to digits[]. diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h index deb823a892ef..e53a36476b70 100644 --- a/tools/firmware/hvmloader/util.h +++ b/tools/firmware/hvmloader/util.h @@ -184,9 +184,30 @@ int uart_exists(uint16_t uart_base); int lpt_exists(uint16_t lpt_base); int hpet_exists(unsigned long hpet_base); -/* Do cpuid instruction, with operation 'idx' */ -void cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx, - uint32_t *ecx, uint32_t *edx); +/* Some CPUID calls want 'count' to be placed in ecx */ +static inline void cpuid_count( + uint32_t leaf, + uint32_t subleaf, + uint32_t *eax, + uint32_t *ebx, + uint32_t *ecx, + uint32_t *edx) +{ + asm volatile ( "cpuid" + : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx) + : "a" (leaf), "c" (subleaf) ); +} + +/* Generic CPUID function (subleaf 0) */ +static inline void cpuid( + uint32_t leaf, + uint32_t *eax, + uint32_t *ebx, + uint32_t *ecx, + uint32_t *edx) +{ + cpuid_count(leaf, 0, eax, ebx, ecx, edx); +} /* Read the TSC register. */ static inline uint64_t rdtsc(void)
hvmloader's cpuid() implementation deviates from Xen's in that the value passed on ecx is unspecified. This means that when used on leaves that implement subleaves it's unspecified which one you get; though it's more than likely an invalid one. Import Xen's implementation so there are no surprises. Fixes: 318ac791f9f9 ("Add utilities needed for SMBIOS generation to hvmloader") Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v5: * Added Fixes tag * Cosmetic changes to static inline, as proposed by Andrew --- tools/firmware/hvmloader/util.c | 9 --------- tools/firmware/hvmloader/util.h | 27 ++++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 12 deletions(-)