Re: [Xen-devel] [PATCH v4 04/17] x86emul: support {, V}{, U}COMIS{S, D}

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/03/17 14:26, Jan Beulich wrote:

>>> +        opc = init_prefixes(stub);
>>> +        opc[0] = b;
>>> +        opc[1] = modrm;
>>> +        if ( ea.type == OP_MEM )
>>> +        {
>>> +            rc = ops->read(ea.mem.seg, ea.mem.off, mmvalp, vex.pfx ? 8 : 4,
>>> +                           ctxt);
>>> +            if ( rc != X86EMUL_OKAY )
>>> +                goto done;
>>> +
>>> +            /* Convert memory operand to (%rAX). */
>>> +            rex_prefix &= ~REX_B;
>>> +            vex.b = 1;
>>> +            opc[1] &= 0x38;
>>> +        }
>>> +        fic.insn_bytes = PFX_BYTES + 2;
>>> +        opc[2] = 0xc3;
>>> +
>>> +        copy_REX_VEX(opc, rex_prefix, vex);
>>> +        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>>> +                    _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>>> +                    [eflags] "+g" (_regs._eflags),
>>> +                    [tmp] "=&r" (cr4 /* dummy */), "+m" (*mmvalp),
>> This is latently dangerous.  It would be better to have an explicit
>> "unsigned long dummy;", which the compiler will perfectly easily elide
>> during register scheduling.
> The thing I want to avoid as much as possible are these ugly,
> improperly indented extra scopes following case labels. If
> putting a dummy variable into the whole switch() scope is okay
> with you, I could live with that. But then again I don't see the
> danger here - there's no imaginable use for cr4 in this piece of
> code.

Whole switch() scope is fine.  I see you have similar dummy examples in
later patches as well.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel