[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 3/4] VMX: use proper instruction mnemonics if assembler supports them

>>> On 26.08.13 at 11:25, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 26/08/2013 10:06, Jan Beulich wrote:
>> The constraints aren't the problem - the use of sizeof(value) inside
>> the macro is. You can't pass both pointers to 32-bit and 64-bit data
>> items into the function, and then have the function guess what the
>> type is to be.
> Ok (wrt the explanation), but I am not sure the result is correct.
> VMREAD has a single encoding, 0f 78, and the size of the reg/mem operand
> depends on long mode or not (where compat mode forces a #UD).
> From SDM Vol 3 30.3 "VMREAD":
> "If the VMCS field specified by the source operand is shorter than this
> effective operand size, the high bits of the destination operand are
> cleared to 0. If the VMCS field is longer, then the high bits of the
> field are not read."
> So passing a pointer to a 32bit value into this function/macro in long
> mode will result in the adjacent 32 bits being clobbered, which should
> best be avoided.

Well spotted (and hence the __vmread_safe() construct is buggy
altogether). Since I know I had looked at the instruction page to
see whether it has 32- and 64-bit variants, I can only blame this
at their inconsistent documentation: Vol 2, where all the other
instructions live anyway, never uses the description field to outline
the modes in which a certain encoding is valid - they have a
separate column for this purpose. And I (mis-)interpreted the
absence of that column...

Anyway - v3 will need to be cut, and then I'll of course see to
make this a function instead of a macro. Luckily the patch in its
entirety is not incorrect, as the two users of __vmread_safe()
use 64-bit values. But as soon as the concept was extended to
__vmread() the bug would have become obvious (and likely even
at compile time already).


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.