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

[Jan Beulich <jbeulich@xxxxxxxx>]

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
>> @@ -2442,6 +2442,27 @@ int main(int argc, char **argv)
>>      else
>>          printf("skipped\n");
>>  
>> +    printf("%-40s", "Testing fldenv 8(%edx)...");
> 
> Likely a stupid question, but why the added 8? edx will contain the
> memory address used to save the sate by fnstenv, so I would expect
> fldenv to just load from there?

The 8 is just to vary ModR/M encodings acrosss the various
tests - it's an arbitrary choice.

>> +    if ( stack_exec && cpu_has_fpu )
>> +    {
>> +        asm volatile ( "fnstenv %0\n\t"
>> +                       "fninit"
>> +                       : "=m" (res[2]) :: "memory" );
> 
> Why do you need the memory clobber here? I assume it's because res is
> of type unsigned int and hence doesn't have the right size that
> fnstenv will actually write to?

Correct.

>> @@ -4948,21 +4949,14 @@ x86_emulate(
>>                  dst.bytes = 4;
>>                  emulate_fpu_insn_memdst(b, modrm_reg & 7, dst.val);
>>                  break;
>> -            case 4: /* fldenv - TODO */
>> -                state->fpu_ctrl = true;
>> -                goto unimplemented_insn;
>> -            case 5: /* fldcw m2byte */
>> -                state->fpu_ctrl = true;
>> -            fpu_memsrc16:
>> -                if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val,
>> -                                     2, ctxt)) != X86EMUL_OKAY )
>> -                    goto done;
>> -                emulate_fpu_insn_memsrc(b, modrm_reg & 7, src.val);
>> -                break;
>> +            case 4: /* fldenv */
>> +                /* Raise #MF now if there are pending unmasked exceptions. 
>> */
>> +                emulate_fpu_insn_stub(0xd9, 0xd0 /* fnop */);
> 
> Maybe it would make sense to have a wrapper for fnop?

I'm not convinced that would be worth it.

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

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

Jan