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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/11/16 08:41, Jan Beulich wrote:
>>>>      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.)

Done.  This looks much cleaner.

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

hvm_inject_page_fault() has always had a signed error code, and the API
is maintained by x86_emul_* and pv_inject_* for consistency.

struct pfinfo currently has an unsigned ec parameter, because all the
existing code uses uint32_t pfec.  In reality, this isn't a problem at
the pfinfo / *_inject_page_fault() boundary.

I have a number of patches focusing on pagefault, and in particular
trying to clean up a lot of misuse of pfec.

~Andrew

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