On 06.04.2023 17:33, Andrew Cooper wrote:
> On 06/04/2023 3:15 pm, Jan Beulich wrote:
>> The assertion at the end of x86_emulate_wrapper() as well as the ones
>> in x86_emul_{hw_exception,pagefault}() can trigger if we ignore
>> X86EMUL_EXCEPTION coming back from certain hook functions.
>
> Looking at the comment I wrote back then, I don't think I'd considered
> this case.
>
> What I was fixing at the time was the case where
> hvm_inject_hw_exception() had raised an exception behind the back of the
> emulator, and any subsequent exception escalates to #DF.
>
> I guess the comment wants updating to discuss this problem too, where
> the hook queued an exception but we didn't squash it properly when
> deciding to ignore it.
Can do.
> As it's debugging-only anyway, it might be worth rearranging the
> expression to be
>
> if ( ctxt->event_pending )
> ASSERT(rc == X86EMUL_EXCEPTION);
> else
> ASSERT(rc != X86EMUL_EXCEPTION);
>
> because it distinguishes the two cases without an intermediate round of
> debugging.
Maybe, but I don't think this adjustment would belong here.
>> ---
>> EFER reads won't fault in any of the handlers we have, so in principle
>> the respective check could be omitted (and hence has no Fixes: tag).
>> Thoughts?
>
> We already require LMA as input state, and don't have an execution model
> where EFER is actually absent in the first place, so passing the whole
> thing wouldn't really be an issue.
>
> I have previously considered doing the same for CR0 too, but at best
> it's marginal so I haven't got around to trying it.
Well, that's more longer term plans (and not very clear as you say). I'm
afraid this doesn't answer my question, though: Do you think the respective
adjustment should stay, or be dropped?
>> --- a/xen/arch/x86/x86_emulate/0fae.c
>> +++ b/xen/arch/x86/x86_emulate/0fae.c
>> @@ -67,7 +67,10 @@ int x86emul_0fae(struct x86_emulate_stat
>> cr4 = X86_CR4_OSFXSR;
>> if ( !ops->read_msr ||
>> ops->read_msr(MSR_EFER, _val, ctxt) !=
>> X86EMUL_OKAY )
>> +{
>> +x86_emul_reset_event(ctxt);
>
> This is the only path you've introduced that doesn't restrict the reset
> to the X86EMUL_EXCEPTION case.
Right, other similar ones had to go into individual other patches I have
pending. The distinction I made was whether the call was in a helper
function (where I want to be careful, because I don't know what state we
may have accumulated) vs mainline code. IOW ...
> In principle you can get things like RETRY for introspection.
> Internally, UNHANDLEABLE is used but I hope that never escapes from
> guest_{rd,wr}msr().
... other errors are possible, yes, but those shouldn't cause an event
to be recorded. Hence it seemed reasonable to me to flush events
unconditionally here, i.e. even if none is pending in the first place.
Jan
