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

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

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@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.
>
> ~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