Re: [PATCH 2/4] VMX: move various uses of UD2 out of fast paths

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/08/2013 09:50, Jan Beulich wrote:
>>>> On 24.08.13 at 00:06, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 23/08/2013 15:02, Jan Beulich wrote:
>>> ... at once making conditional forward jumps, which are statically
>>> predicted to be not taken, only used for the unlikely (error) cases.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> I presume it is intentional to create many different symbols for
>> different ud2 instructions right next to each other?  It isn't clear
>> from the commit message, but would certainly be the logical choice for
>> debugging an issue arising from such a bug.
> These are labels that don't make it into the object file's symbol
> table anyway, so their names don't matter (but need to all be
> distinct to avoid duplicate names at assembly time).
>
> The addresses were chosen to be all different (rather than
> having a single global UD2 that all of the failure paths jump
> to) in order to be able to re-associate an eventual crash with
> its origin. As that isn't different from how the code behaved
> before, I don't see a need for mentioning this in the description.
>
>> As each of these ud2 instructions will never result in continued normal
>> execution, is it necessary to have the 'likely' tag and jump back?  I
>> cant see any non-buggy case where they would be executed, at which point
>> they are superfluous.
> Which "likely" tag are you talking about? They all use
> UNLIKELY_END_SECTION rather than UNLIKELY_END, so there's
> no extra code being generated afaict.

That would be me misreading the code.  Sorry for the noise.

~Andrew

>
>>> @@ -377,7 +387,9 @@ static inline void __invvpid(int type, u
>>>      /* Fix up #UD exceptions which occur when TLBs are flushed before VMXON. */
>>>      asm volatile ( "1: " INVVPID_OPCODE MODRM_EAX_08
>>>                     /* CF==1 or ZF==1 --> crash (ud2) */
>>> -                   "ja 2f ; ud2 ; 2:\n"
>>> +                   "2: " UNLIKELY_START(be, invvpid)
>>> +                   "\tud2\n"
>>> +                   UNLIKELY_END_SECTION "\n"
>>>                     _ASM_EXTABLE(1b, 2b)
>> Is this logic still correct?
>>
>> To my reading, the logic used to say "if there was an exception while
>> executing INVVPID, then jump to 2: to fix up." where 2: happens to be
>> the end of the inline assembly, bypassing the ud2.
>>
>> After the change, the jbe (formed from UNLIKELY_START) is part of the
>> fixup section, which possibly includes the ud2.  As the state of the
>> flags while executing the INVVPID as unknown, this leaves an unknown
>> chance of "fixing up" to a ud2 instruction.
> Indeed, I screwed this up. Hence there'll be a v2 of this patch
> shortly. Thanks for spotting!
>
> Jan
>