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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew