Re: [PATCH 06/15] x86/emul: Rework emulator event injection

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Tim Deegan <tim@xen.org>
Cc: JunNakajima <jun.nakajima@intel.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Jan Beulich <JBeulich@suse.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>,
	Paul Durrant <paul.durrant@citrix.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH 06/15] x86/emul: Rework emulator event injection
Date: Thu, 24 Nov 2016 17:37:51 +0000	[thread overview]
Message-ID: <9c2f8672-8870-fb42-715f-657832af0db8@citrix.com> (raw)
In-Reply-To: <20161124173024.GA65611@deinos.phlegethon.org>

On 24/11/16 17:30, Tim Deegan wrote:
> At 17:19 +0000 on 24 Nov (1480007992), Andrew Cooper wrote:
>> On 24/11/16 17:08, Jan Beulich wrote:
>>>>>> On 24.11.16 at 18:00, <andrew.cooper3@citrix.com> wrote:
>>>> On 24/11/16 14:53, Jan Beulich wrote:
>>>>>>>> On 23.11.16 at 16:38, <andrew.cooper3@citrix.com> wrote:
>>>>>> --- a/xen/arch/x86/mm.c
>>>>>> +++ b/xen/arch/x86/mm.c
>>>>>> @@ -5377,7 +5377,7 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr,
>>>>>>      page_unlock(page);
>>>>>>      put_page(page);
>>>>>>  
>>>>>> -    if ( rc == X86EMUL_UNHANDLEABLE )
>>>>>> +    if ( rc == X86EMUL_UNHANDLEABLE || ptwr_ctxt.ctxt.event_pending )
>>>>>>          goto bail;
>>>>>>  
>>>>>>      perfc_incr(ptwr_emulations);
>>>>>> @@ -5501,7 +5501,8 @@ int mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr,
>>>>>>      else
>>>>>>          rc = x86_emulate(&ctxt, &mmio_ro_emulate_ops);
>>>>>>  
>>>>>> -    return rc != X86EMUL_UNHANDLEABLE ? EXCRET_fault_fixed : 0;
>>>>>> +    return ((rc != X86EMUL_UNHANDLEABLE && !ctxt.event_pending)
>>>>>> +            ? EXCRET_fault_fixed : 0);
>>>>>>  }
>>>>> Wouldn't these two better be adjusted to check for OKAY and RETRY,
>>>>> the more that iirc we had settled on it not (yet) being guaranteed to
>>>>> see event_pending set whenever getting back EXCEPTION?
>>>> In this patch, the key point I am guarding against is that, without the
>>>> ->inject_*() hooks, some actions which previously took a fail_if() path
>>>> now succeed and latch an event.
>>>>
>>>> From that point of view, it doesn't matter how the event became pending,
>>>> but the fact that one is means that it was a codepath which would
>>>> previously have returned UNHANDLEABLE.
>>>>
>>>>
>>>> Later patches, which stop raising faults behind the back of emulator,
>>>> are the ones where new consideration is needed towards the handling of
>>>> EXCEPTION/event_pending.  Following Tim's feedback, I have more work to
>>>> do in patch 9, as propagate_page_fault() raises #PF behind the back of
>>>> the emulator for PV guests.
>>>>
>>>> In other words, I think this patch wants to stay like this, and a later
>>>> one change to be better accommodating.
>>> Okay.
>>>
>>>>>> @@ -3433,7 +3433,7 @@ static int sh_page_fault(struct vcpu *v,
>>>>>>              shadow_continue_emulation(&emul_ctxt, regs);
>>>>>>              v->arch.paging.last_write_was_pt = 0;
>>>>>>              r = x86_emulate(&emul_ctxt.ctxt, emul_ops);
>>>>>> -            if ( r == X86EMUL_OKAY )
>>>>>> +            if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.event_pending )
>>>>> Aiui you need this for the swint case.
>>>> Why?  software interrupts were never previously tolerated in shadow
>>>> emulation.
>>> Then why would you expect OKAY together with event_pending set?
>>> I'm not saying swint handling needs to succeed here, but I can't see
>>> anything else to cause that particular state to occur.
>> Before this patch, a VM playing race conditions with the emulator could
>> cause this case to emulate 0xcc, which would fail because of the lack of
>> ->inject_sw_interrupt() hook, and return X86_UNHANDLEABLE.
>>
>> The changes in this patch now mean that the same case would properly
>> latch #BP, returning OKAY because its a trap not an exception.
>>
>> By not explicitly failing the OKAY case with an event pending, we are
>> suddenly opening up rather more functionality than previously existed.
>>
>>>>> But wouldn't you then need to add similar checks in OKAY paths elsewhere?
>>>> I don't see why I would.  Does my explanation above resolve your concern?
>>> I'm afraid not: On the same basis as above, code not expecting to
>>> handle swint may now see OKAY together with event_pending set,
>>> and would need to indicate failure to their callers just like you do in
>>> sh_page_fault().
>> That is my intent with the current code.  I have double checked it, and
>> it still looks correct.
> So is that not the case I was worried about, where the emulator
> updates register state but we then drop the expected event on the
> floor?

Oh right, yes.  Sorry for being dense.

As an interim between now and getting a proper audit hook, would a bool
permit_traps in x86_emulate_ctxt suffice?

~Andrew

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