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

Re: [Xen-devel] [PATCH] x86/emul: only emulate possibly operand sizes for POPA


  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: Keir Fraser <keir.xen@xxxxxxxxx>
  • Date: Thu, 08 Nov 2012 07:48:23 +0000
  • Cc: xen-devel <xen-devel@xxxxxxxxxxxxx>
  • Delivery-date: Thu, 08 Nov 2012 07:48:52 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xen.org>
  • Thread-index: Ac29hW2O/l9HTu14qkKyDgnFBX+aiQ==
  • Thread-topic: [Xen-devel] [PATCH] x86/emul: only emulate possibly operand sizes for POPA

On 08/11/2012 07:34, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:

>> Would prefer:
>>  if ( op_bytes == 2 )
>>      *(uint16_t *)regs[i] = (uint16_t)dst.val;
>>  else
>>      *regs[i] = dst.val;
>> 
>> Handles the exceptional case immediately after its predicate.
> 
> I had it that way first, but compilers tend to prefer (in terms of
> static branch prediction) the if() body over the else one. Doesn't
> matter that much here of course, but I'm generally trying to
> follow such guidelines even in non performance critical paths so
> that in case code gets cloned elsewhere it doesn't require extra
> reviewing or adjustment.

Should follow such guidelines where the optimisation matters. I think
shaping code to follow such guidelines all the time, is misguided. I'd
rather have the fractionally more readable version than the
possibly-fractionally faster version.

>> And the cast
>> from uint32_t, and 64b-related comment, are pointless and in fact misleading
>> in the default case, since as you say the instruction is invalid in 64-bit
>> mode.
> 
> And I considered that aspect too: Even if invalid in 64-bit mode, it
> is valid in compatibility mode, and in that case the zero-extension
> makes sense (as does the comment).

I did wonder. The top halves of 64b registers are not used in compatibility
mode. Are their contents at all guaranteed to be
maintained/updated/preserved in any meaningful way across transitions into
and out of compatibility mode? I wasn't aware they were, and in that case
the cast and comment are indeed pointless.

 -- Keir



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


 


Rackspace

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