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.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
This is a fix for a latent bug. Should go into 4.19.

v4
  * New patch
---
 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..3ad7c4f6d6a2 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 op,
+    uint32_t count,
+    uint32_t *eax,
+    uint32_t *ebx,
+    uint32_t *ecx,
+    uint32_t *edx)
+{
+    asm volatile ( "cpuid"
+          : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
+          : "0" (op), "c" (count) );
+}
+
+/* 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.34.1

On 26/06/2024 5:28 pm, 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>
>
>
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index deb823a892ef..3ad7c4f6d6a2 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 op,
> +    uint32_t count,
> +    uint32_t *eax,
> +    uint32_t *ebx,
> +    uint32_t *ecx,
> +    uint32_t *edx)
> +{
> +    asm volatile ( "cpuid"
> +          : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
> +          : "0" (op), "c" (count) );

"a" to be consistent with "c".

Also it would be better to name the parameters as leaf and subleaf.

Both can be fixed on commit.  However, there's no use in HVMLoader
tickling this bug right now, so I'm not sure we want to rush this into
4.19 at this point.

~Andrew

On Wed, 2024-06-26 at 17:43 +0100, Andrew Cooper wrote:
> On 26/06/2024 5:28 pm, 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>
> > 
> > 
> > diff --git a/tools/firmware/hvmloader/util.h
> > b/tools/firmware/hvmloader/util.h
> > index deb823a892ef..3ad7c4f6d6a2 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 op,
> > +    uint32_t count,
> > +    uint32_t *eax,
> > +    uint32_t *ebx,
> > +    uint32_t *ecx,
> > +    uint32_t *edx)
> > +{
> > +    asm volatile ( "cpuid"
> > +          : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
> > +          : "0" (op), "c" (count) );
> 
> "a" to be consistent with "c".
> 
> Also it would be better to name the parameters as leaf and subleaf.
> 
> Both can be fixed on commit.  However, there's no use in HVMLoader
> tickling this bug right now, so I'm not sure we want to rush this
> into
> 4.19 at this point.
I agree, I think it would be better to postpone the patch until 4.20
branch.

~ Oleksii

On Wed Jun 26, 2024 at 5:43 PM BST, Andrew Cooper wrote:
> On 26/06/2024 5:28 pm, 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>
> >
> >
> > diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> > index deb823a892ef..3ad7c4f6d6a2 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 op,
> > +    uint32_t count,
> > +    uint32_t *eax,
> > +    uint32_t *ebx,
> > +    uint32_t *ecx,
> > +    uint32_t *edx)
> > +{
> > +    asm volatile ( "cpuid"
> > +          : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
> > +          : "0" (op), "c" (count) );
>
> "a" to be consistent with "c".
>
> Also it would be better to name the parameters as leaf and subleaf.
>
> Both can be fixed on commit.  However, there's no use in HVMLoader
> tickling this bug right now, so I'm not sure we want to rush this into
> 4.19 at this point.
>
> ~Andrew

All sound good to me. For the record, the static inlines are copied verbatim
from Xen so if you'd like these adjusted you probably also want to make a
postit to change Xen's too.

Cheers,
Alejandro