[v5] x86: Expose consistent topology to guests

[PATCH v5 01/10] tools/hvmloader: Fix non-deterministic cpuid()

Posted by Alejandro Vallejo 1 year, 6 months ago

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

Re: [PATCH v5 01/10] tools/hvmloader: Fix non-deterministic cpuid()

Posted by Jan Beulich 1 year, 6 months ago

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

Re: [PATCH v5 01/10] tools/hvmloader: Fix non-deterministic cpuid()

Posted by Alejandro Vallejo 1 year, 6 months ago

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