Re: PATCH: Implement Intel Transactional Synchronization Extensions

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Tue, Feb 21, 2012 at 11:52 PM, Jan Beulich <jbeulich@suse.com> wrote:
>>>> "H.J. Lu" <hjl.tools@gmail.com> 02/21/12 7:17 PM >>>
> On Tue, Feb 21, 2012 at 12:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 08.02.12 at 19:25, "H.J. Lu" <hongjiu.lu@intel.com> wrote:
>>>>+ ? ? ?if (i.prefix[HLE_PREFIX] == XACQUIRE_PREFIX_OPCODE)
>>>
>>> With HLE_PREFIX == REP_PREFIX and XACQUIRE_PREFIX_OPCODE
>>> == REPNE_PREFIX_OPCODE, how is this distinguished from the repne
>>> case? Oh, I see, the *caller* checks i.have_hle - that's pretty bad to
>>> be done this way imo.
>>
>>Any suggestions are welcome.
>
> At least move the check into the function, so that an eventual second caller
> doesn't have to ope code the check a second time.

Can you provide a patch?

>>> Wouldn't it make sense to insert missing LOCK prefixes silently
>>> (optionally verbosely) rather than complaining about their absence?
>>> (I'd also question the value of displaying both prefixes by default
>>> in the disassembly - this just clutters things rather than helping
>>> with anything.)
>>
>>In some cases, it is very useful to know/control exactly how many
>>prefixes are generated.
>
> But there's no alternative here - the code is wrong without the prefixes.
> And as written, for those who do care an optional warning could be
> implemented.

LOCK prefix is optional on some instructions.  Implicit LOCK prefix
means we can't tell how many prefix bytes are generated from instructions.
I will not go with implicit LOCK prefix.

>>> Further, while the patch deals with CMPXCHG8B, for some reason
>>> it leaves out CMPXCHG16B (perhaps because the instruction, oddly
>>> enough, is considered an SSE3 one).
>>>
>>> Finally, albeit consistent with what is documented, I'm surprised that
>>> MOV opcodes 0xA2 and 0xA3 aren't allowed with XRELEASE - is that
>>> really the case?
>>>
>>
>>Yes, they are implemented according to TSX spec.
>
> Assuming you meant "are not", did you check that this isn't just an omission
> in the spec?
>

I will let you know what I found out.

Thanks,

-- 
H.J.