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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

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@xxxxxxxxx>
>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> 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