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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/11/16 14:53, Jan Beulich wrote:
>>>> On 23.11.16 at 16:38, <andrew.cooper3@xxxxxxxxxx> 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.

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

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

> Or alternatively, wouldn't it be better to have x86_emulate() return 
> EXCEPTION also
> for successful swint emulation (albeit that would likely require other
> not very nice adjustments)?

That would indeed be nasty.  If we were to go down that route, it would
be better to swap X86EMUL_EXCEPTION for X86EMUL_EVENT which explicitly
also includes traps where a register writeback has been completed.

~Andrew

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