Re: [PATCH 3/3] x86/treewide: Drop the TRAP_* legacy names

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/02/2023 1:10 pm, Jan Beulich wrote:
> On 20.02.2023 12:59, Andrew Cooper wrote:
>> We have two naming schemes for exceptions - X86_EXC_?? which use the
>> archtiectural abbreviations, and TRAP_* which is a mix of terminology and
>> nonstandard abbrevations.  Switch to X86_EXC_* uniformly.
>>
>> No funcational change, confirmed by diffing the disassembly.  Only 7 binary
>> changes, and they're all __LINE__ being passed into printk().
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

Given that this is proving a pain to rebase, I've pulled it out on its
own and committed it.  I'll rebase patches 1 and 2 over this at some
other point.

>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -2745,9 +2745,9 @@ static int cf_check sh_page_fault(
>>           * stream under Xen's feet.
>>           */
>>          if ( emul_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
>> -             ((emul_ctxt.ctxt.event.vector == TRAP_page_fault) ||
>> -              (((emul_ctxt.ctxt.event.vector == TRAP_gp_fault) ||
>> -                (emul_ctxt.ctxt.event.vector == TRAP_stack_error)) &&
>> +             ((emul_ctxt.ctxt.event.vector == X86_EXC_PF) ||
>> +              (((emul_ctxt.ctxt.event.vector == X86_EXC_GP) ||
>> +                (emul_ctxt.ctxt.event.vector == X86_EXC_SS)) &&
>>                 emul_ctxt.ctxt.event.error_code == 0)) )
>>              hvm_inject_event(&emul_ctxt.ctxt.event);
>>          else
> Entirely unrelated to your change, but seeing that this piece of code is
> being touched: Aren't we too lax here with #PF? Shouldn't we refuse
> propagation also for e.g. reserved bit faults and insn fetch ones? Maybe
> even for anything that isn't a supervisor write?

Imagine a guest does a CMPXCHG16B which straddles a page boundary, with
the lower part hitting a shadow page table, and the higher half hitting
a mapping that the guest has mapped with RSVD bits in the PTE.

In this scenario, there would be a VMExit type PF (write to a page which
is actually read-only because it's a shadowed pagetable), and during the
course of emulation we'd permit the lower half, then encounter PF[Rsvd]
for the second half.

And the correct action here is genuinely to pass PF[Rsvd] back to the
guest.  So no, filtering on Rsvd is not the appropriate course of
action.  Similarly for a fetch fault, the guest could genuinely have
paged out the frame we're interested in in the time we've been spending
in the sh_fault() handler.


Whether the else path, choosing UNHANDLEABLE and unshadowing the frame
is really the right course of action, is a different question...  I
think the current behaviour is safe, and its not as if it's a codepath
worth optimising these days.

~Andrew