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(-)
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)
--
2.45.2
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
© 2016 - 2024 Red Hat, Inc.