Re: [Xen-devel] [PATCH v3 1/4] xen/libxc: Allow changes to hypervisor CPUID leaf from config file

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.03.14 at 19:08, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -43,22 +45,29 @@ static int hypervisor_is_64bit(xc_interface *xch)
>  static void cpuid(const unsigned int *input, unsigned int *regs)
>  {
>      unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1];
> +    uint8_t is_hyp = IS_HYPERVISOR_LEAF(input[0]);
>  #ifdef __i386__
>      /* Use the stack to avoid reg constraint failures with some gcc flags */
>      asm (
>          "push %%ebx; push %%edx\n\t"
> -        "cpuid\n\t"
> +        "testb $0xff,%5\n\t"
> +        "jz 1f\n\t"
> +        XEN_EMULATE_PREFIX
> +        "1: cpuid\n\t"
>          "mov %%ebx,4(%4)\n\t"
>          "mov %%edx,12(%4)\n\t"
>          "pop %%edx; pop %%ebx\n\t"
>          : "=a" (regs[0]), "=c" (regs[2])
> -        : "0" (input[0]), "1" (count), "S" (regs)
> +        : "0" (input[0]), "1" (count), "S" (regs), "m" (is_hyp)

All inputs must be in registers here, since memory references might
use %esp and hence be off by 2 stack slots due to the pushes/pops
surrounding the actual operation. Since you evaluate the flag prior
to the CPUID, using "db" as constraint would seem possible here.

And I'd additionally recommend stopping the practice of using
numeric labels in inline assembly - especially when nesting things
through multiple macro layers (admittedly not the case here) this
is just too fragile, and gcc has a nice solution ("%=") available.

>          : "memory" );
>  #else
>      asm (
> -        "cpuid"
> +        "testb $0xff,%6\n\t"
> +        "jz 1f\n\t"
> +        XEN_EMULATE_PREFIX
> +        "1: cpuid"
>          : "=a" (regs[0]), "=b" (regs[1]), "=c" (regs[2]), "=d" (regs[3])
> -        : "0" (input[0]), "2" (count) );
> +        : "0" (input[0]), "2" (count), "m" (is_hyp) );

And afaict there's no reason not to use "g" here.

> @@ -555,6 +564,15 @@ static int xc_cpuid_policy(
>  {
>      xc_dominfo_t        info;
>  
> +    if ( IS_HYPERVISOR_LEAF(input[0]) )
> +    {
> +        /* Only leaf 1 can be modified */
> +        if ( input[0] == 0x40000000 )
> +            return 0;
> +        else
> +            return -EACCES;

And I'm still worried about altering this in uncontrolled ways: No
good can result from allowing ecx/edx/ebx to be modified, and
improperly modifying the eax value (namely putting in place
wrong upper bits, the more that those aren't statically
determined) won't help much either. IOW it should really only
be the low 8 bits of eax that can be overridden, and it shouldn't
be possible to clear these 8 bits.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel