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.vall...@cloud.com>
Reviewed-by: Jan Beulich <jbeul...@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
