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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

> +        }
> +        else
> +            gprintk(XENLOG_WARNING,
> +                    "Unhandled %s fault/trap [#%d, ec=%04x]\n",
> +                    trapstr(vector), vector, error_code);

Which tells us that we need to finally get our log level handling
straightened, to avoid inconsistencies like the one here comparing
with the code a few lines up.

> +static inline void do_guest_trap(unsigned int trapnr,
> +                                 const struct cpu_user_regs *regs)
> +{
> +    const struct x86_event event = {

I don't mind the const, but I don't think it's very useful here.

Jan


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