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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan