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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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