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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.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.
> 
> 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