Re: [Xen-devel] [PATCH v2] x86emul: add "unblock NMI" retire flag

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.04.17 at 10:06, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 12/04/2017 08:43, Jan Beulich wrote:
>>>>> On 11.04.17 at 18:09, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 11/04/17 16:36, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e
>>>>          memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, 
>>>> vio->mmio_insn_bytes);
>>>>      }
>>>>  
>>>> -    if ( rc != X86EMUL_OKAY )
>>>> -        return rc;
>>>> +    new_intr_shadow = hvmemul_ctxt->intr_shadow;
>>>>  
>>>> -    if ( hvmemul_ctxt->ctxt.retire.singlestep )
>>>> -        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
>>>> +    /*
>>>> +     * IRET, if valid in the given context, clears NMI blocking
>>>> +     * (irrespective of rc).
>>>> +     */
>>>> +    if ( hvmemul_ctxt->ctxt.retire.unblock_nmi )
>>>> +        new_intr_shadow &= ~HVM_INTR_SHADOW_NMI;
>>>>  
>>>> -    new_intr_shadow = hvmemul_ctxt->intr_shadow;
>>>> +    if ( rc == X86EMUL_OKAY )
>>>> +    {
>>> On further thought, given the assertion, you don't need to introduce
>>> this check, and can avoid the block indentation.  It should make the
>>> patch rather smaller.
>> Hmm, I did consider this, but I feel uneasy doing so, as it leaves
>> production builds in potentially bad shape (in case we have a path
>> we don't ever execute in any of the routine testing done, but
>> which someone nevertheless ends up coming down in a released
>> product). Paul, you're the maintainer of the code, do you have an
>> opinion either way?
> 
> Actually, on yet further thought, the reset nature of these conditions
> are actually necessary on the != OKAY path.
> 
> Consider "mov %eax, %ss; ud2a".  Even on raising an exception for the
> ud2a, the SS shadow needs dropping.

Hmm, indeed (for EXCEPTION at least, not so sure about the others;
quite certainly not for RETRY). Along the lines of the above this would
then call for an "else" to the newly added "if ( rc == X86EMUL_OKAY )"
though.

Jan


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