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

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

On Fri, May 08, 2020 at 05:04:02PM +0200, Jan Beulich wrote:
> On 08.05.2020 15:37, Roger Pau Monné wrote:
> > On Tue, May 05, 2020 at 10:16:20AM +0200, Jan Beulich wrote:
> >> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> >> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> >> @@ -11648,6 +11651,89 @@ int x86_emul_blk(
> >>  
> >>  #ifndef X86EMUL_NO_FPU
> >>  
> >> +    case blk_fld:
> >> +        ASSERT(!data);
> >> +
> >> +        /* state->rex_prefix carries CR0.PE && !EFLAGS.VM setting */
> >> +        switch ( bytes )
> >> +        {
> >> +        case sizeof(fpstate.env):
> >> +        case sizeof(fpstate):
> >> +            memcpy(&fpstate.env, ptr, sizeof(fpstate.env));
> >> +            if ( !state->rex_prefix )
> >> +            {
> >> +                unsigned int fip = fpstate.env.mode.real.fip_lo +
> >> +                                   (fpstate.env.mode.real.fip_hi << 16);
> >> +                unsigned int fdp = fpstate.env.mode.real.fdp_lo +
> >> +                                   (fpstate.env.mode.real.fdp_hi << 16);
> >> +                unsigned int fop = fpstate.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;
> > 
> > I've found the layouts in the SDM vol. 1, but I haven't been able to
> > found the translation mechanism from real to protected. Could you
> > maybe add a reference here?
> 
> A reference to some piece of documentation? I don't think this
> is spelled out anywhere. It's also only one of various possible
> ways of doing the translation, but among them the most flexible
> one for possible consumers of the data (because of using the
> smallest possible offsets into the segments).

Having this written down as a comment would help, but maybe that's
just because I'm not familiar at all with all this stuff.

Again, likely a very stupid question, but I would expect:

fpstate.env.mode.prot.fip = fip;

Without the mask.

> >> +            }
> >> +
> >> +            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? 

I think a macro would be fine IMO.

Thanks, Roger.