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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.08.13 at 00:06, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxx>
> 
> 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.

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


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