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

[oleksii.kurochko@xxxxxxxxx]

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