Re: [PATCH v1 1/3] x86: x86_emulate: address violations of MISRA C Rule 19.1

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.lira@xxxxxxx wrote:
> >>> From: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> >>>
> >>> 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.


diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c 
b/xen/arch/x86/x86_emulate/x86_emulate.c
index 8e14ebb35b..bee0332bdf 100644
--- 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 */
         case 8: _regs.r(ax) = (int32_t)_regs.r(ax); break; /* cdqe */
         }
         break;