Re: [Xen-devel] [PATCH v2 06/19] x86/pv: Implement pv_inject_{event, page_fault, hw_exception}()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.11.16 at 17:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 29/11/16 16:00, Jan Beulich wrote:
>>>>> On 28.11.16 at 12:13, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> The existing propagate_page_fault() is fairly similar to
>>> pv_inject_page_fault(), although it has a return value.  Only a single 
>>> caller
>>> makes use of the return value, and non-NULL is only returned if the passed 
>>> cr2
>>> is non-canonical.  Opencode this single case in
>>> handle_gdt_ldt_mapping_fault(), allowing propagate_page_fault() to become
>>> void.
>> I can only say that back then it was quite intentional to not open
>> code this in the caller, no matter that it was (and still is) just one.
> 
> Is that an objection to me making this change?

Well, no, not really. It's more like "I'd prefer it to stay as is, but I can
see why you want to change it, and not changing it would likely make
your overall modification harder".

>>>      if ( unlikely(null_trap_bounce(v, tb)) )
>>> -        gprintk(XENLOG_WARNING,
>>> -                "Unhandled %s fault/trap [#%d, ec=%04x]\n",
>>> -                trapstr(trapnr), trapnr, regs->error_code);
>>> +    {
>>> +        if ( vector == TRAP_page_fault )
>>> +        {
>>> +            printk("%pv: unhandled page fault (ec=%04X)\n", v, error_code);
>>> +            show_page_walk(event->cr2);
>>> +
>>> +            if ( unlikely(error_code & PFEC_reserved_bit) )
>>> +                reserved_bit_page_fault(event->cr2, regs);
>> I think you want to move the show_page_walk() into an else here,
>> to avoid logging two of them. But then again - why do you move
>> this behind the null_trap_bounce() check? It had been logged
>> independently in the original code, and for (imo) a good reason
>> (reserved bit faults are always a sign of a hypervisor problem
>> after all).
> 
> TBH, I found it odd that it was in propagate_page_fault() to start with.
> 
> It is the kind of thing which should be in the pagefault handler itself,
> not in the reinjection-to-pv-guests code.
> 
> Would moving it into fixup_page_fault() be ok?  It should probably go
> between the hap/shadow and memadd checks, as shadow guests can have
> reserved bits set.

That would move it ahead in the flow quite a bit, which I'm not sure
is a good idea (due to possible [hypothetical] other uses of reserved
bits). Also note that there already is a call to reserved_bit_page_fault()
in the !guest_mode() case, so if anything I would see it moved right
ahead of the pv_inject_page_fault() invocation from do_page_fault().
(This would then shrink page size too, as you wouldn't have to move
around reserved_bit_page_fault() itself.)

Btw, looking at the full patch again I notice that the error code
parameter to pv_inject_page_fault() is signed, which is contrary to
what I think I recall you saying on the HVM side of things (the
error code being non-optional here, and hence better being unsigned,
as X86_EVENT_NO_EC is not allowed).

Jan


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