Re: [PATCH v8 08/12] x86emul: support FLDENV and FRSTOR

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, May 11, 2020 at 09:25:54AM +0200, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> 
> On 08.05.2020 20:29, Andrew Cooper wrote:
> > On 08/05/2020 16:04, Jan Beulich wrote:
> >>>> +            }
> >>>> +
> >>>> +            if ( bytes == sizeof(fpstate.env) )
> >>>> +                ptr = NULL;
> >>>> +            else
> >>>> +                ptr += sizeof(fpstate.env);
> >>>> +            break;
> >>>> +
> >>>> +        case sizeof(struct x87_env16):
> >>>> +        case sizeof(struct x87_env16) + sizeof(fpstate.freg):
> >>>> +        {
> >>>> +            const struct x87_env16 *env = ptr;
> >>>> +
> >>>> +            fpstate.env.fcw = env->fcw;
> >>>> +            fpstate.env.fsw = env->fsw;
> >>>> +            fpstate.env.ftw = env->ftw;
> >>>> +
> >>>> +            if ( state->rex_prefix )
> >>>> +            {
> >>>> +                fpstate.env.mode.prot.fip = env->mode.prot.fip;
> >>>> +                fpstate.env.mode.prot.fcs = env->mode.prot.fcs;
> >>>> +                fpstate.env.mode.prot.fdp = env->mode.prot.fdp;
> >>>> +                fpstate.env.mode.prot.fds = env->mode.prot.fds;
> >>>> +                fpstate.env.mode.prot.fop = 0; /* unknown */
> >>>> +            }
> >>>> +            else
> >>>> +            {
> >>>> +                unsigned int fip = env->mode.real.fip_lo +
> >>>> +                                   (env->mode.real.fip_hi << 16);
> >>>> +                unsigned int fdp = env->mode.real.fdp_lo +
> >>>> +                                   (env->mode.real.fdp_hi << 16);
> >>>> +                unsigned int fop = env->mode.real.fop;
> >>>> +
> >>>> +                fpstate.env.mode.prot.fip = fip & 0xf;
> >>>> +                fpstate.env.mode.prot.fcs = fip >> 4;
> >>>> +                fpstate.env.mode.prot.fop = fop;
> >>>> +                fpstate.env.mode.prot.fdp = fdp & 0xf;
> >>>> +                fpstate.env.mode.prot.fds = fdp >> 4;
> >>> This looks mostly the same as the translation done above, so maybe
> >>> could be abstracted anyway in a macro to avoid the code repetition?
> >>> (ie: fpstate_real_to_prot(src, dst) or some such).
> >> Just the 5 assignments could be put in an inline function, but
> >> if we also wanted to abstract away the declarations with their
> >> initializers, it would need to be a macro because of the
> >> different types of fpstate.env and *env. While I'd generally
> >> prefer inline functions, the macro would have the benefit that
> >> it could be #define-d / #undef-d right inside this case block.
> >> Thoughts?
> > 
> > Code like this is large in terms of volume, but it is completely crystal
> > clear (with the requested comments in place) and easy to follow.
> > 
> > I don't see how attempting to abstract out two small portions is going
> > to be an improvement.
> 
> Okay, easier for me if I don't need to touch it. Roger, can you
> live with that?

Yes, that's fine.

Thanks.