On Wed, 30 Apr 2025, Jan Beulich wrote:
> 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.
Thanks Jan, this should be:
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
b/xen/arch/x86/x86_emulate/x86_emulate.c
index bee0332bdf..d678855238 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -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) = (uint32_t)(int16_t)_regs.r(ax); break; /* cwde */
case 8: _regs.r(ax) = (int32_t)_regs.r(ax); break; /* cdqe */
}
break;
I tested this too and passes MISRA
