On 30.04.2025 00:54, Stefano Stabellini wrote:
> On Tue, 29 Apr 2025, Jan Beulich wrote:
>> On 29.04.2025 03:27, Stefano Stabellini wrote:
>>> On Mon, 28 Apr 2025, Jan Beulich wrote:
>>>> On 26.04.2025 01:42, victorm.l...@amd.com wrote:
>>>>> From: Nicola Vetrini <nicola.vetr...@bugseng.com>
>>>>>
>>>>> Rule 19.1 states: "An object shall not be assigned or copied
>>>>> to an overlapping object". Since the "call" and "compat_call" are
>>>>
>>>> Was this taken from patch 2 without editing?
>>>>
>>>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>>>> @@ -526,9 +526,19 @@ static inline void put_loop_count(
>>>>> */ \
>>>>> if ( !amd_like(ctxt) && mode_64bit() && ad_bytes == 4 ) \
>>>>> { \
>>>>> + uint64_t tmp; \
>>>>> + \
>>>>> _regs.r(cx) = 0; \
>>>>> - if ( extend_si ) _regs.r(si) = _regs.esi; \
>>>>> - if ( extend_di ) _regs.r(di) = _regs.edi; \
>>>>> + if ( extend_si ) \
>>>>> + { \
>>>>> + tmp = _regs.esi; \
>>>>> + _regs.r(si) = tmp; \
>>>>> + } \
>>>>> + if ( extend_di ) \
>>>>> + { \
>>>>> + tmp = _regs.edi; \
>>>>> + _regs.r(di) = tmp; \
>>>>> + } \
>>>>
>>>> See commit 7225f13aef03 for how we chose to address similar issues
>>>> elsewhere
>>>> in the emulator. I think we want to be consistent there. This will then
>>>> also
>>>> eliminate ...
>>>>
>>>>> @@ -2029,7 +2039,12 @@ x86_emulate(
>>>>> switch ( op_bytes )
>>>>> {
>>>>> case 2: _regs.ax = (int8_t)_regs.ax; break; /* cbw */
>>>>> - case 4: _regs.r(ax) = (uint32_t)(int16_t)_regs.ax; break; /*
>>>>> cwde */
>>>>> + case 4:
>>>>> + {
>>>>> + uint32_t tmp = (uint32_t)(int16_t)_regs.ax;
>>>>> + _regs.r(ax) = tmp;
>>>>> + break; /* cwde */
>>>>> + }
>>>>
>>>> ... the odd brace placement here, as well as the inconsistency in the types
>>>> you used for the temporary variables (both really could have been unsigned
>>>> int; no need for a fixed-width type).
>>>
>>> Is this what you have in mind?
>>
>> No, and that's also not what the referenced commit did in a similar
>> situation.
>>
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -527,8 +527,8 @@ static inline void put_loop_count(
>>> if ( !amd_like(ctxt) && mode_64bit() && ad_bytes == 4 ) \
>>> { \
>>> _regs.r(cx) = 0; \
>>> - if ( extend_si ) _regs.r(si) = _regs.esi; \
>>> - if ( extend_di ) _regs.r(di) = _regs.edi; \
>>> + if ( extend_si ) _regs.r(si) = (uint64_t)_regs.esi; \
>>> + if ( extend_di ) _regs.r(di) = (uint64_t)_regs.edi; \
>>
>> if ( extend_si ) _regs.r(si) = (uint32_t)_regs.r(si); \
>> if ( extend_di ) _regs.r(di) = (uint32_t)_regs.r(di); \
>>
>> After all what the rule requires is that we use _the same_ field on both
>> sides.
>
> I see, thanks Jan. Yes I did try this version and worked as expected.
Except that ...
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -527,8 +527,8 @@ static inline void put_loop_count(
> if ( !amd_like(ctxt) && mode_64bit() && ad_bytes == 4 ) \
> { \
> _regs.r(cx) = 0; \
> - if ( extend_si ) _regs.r(si) = _regs.esi; \
> - if ( extend_di ) _regs.r(di) = _regs.edi; \
> + if ( extend_si ) _regs.r(si) = (uint32_t)_regs.r(si); \
> + if ( extend_di ) _regs.r(di) = (uint32_t)_regs.r(di); \
> } \
> goto complete_insn; \
> } \
> @@ -2029,7 +2029,7 @@ x86_emulate(
> switch ( op_bytes )
> {
> case 2: _regs.ax = (int8_t)_regs.ax; break; /* cbw */
> - case 4: _regs.r(ax) = (uint32_t)(int16_t)_regs.ax; break; /* cwde */
> + case 4: _regs.r(ax) = (int16_t)_regs.r(ax); break; /* cwde */
... the change in casts here renders this wrong now, afaict. We'd sign-
extend from 16 to 64 bits, rather than sign-extending to 32 bits and
then zero-extending to 64.
Jan
