[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 06/19] x86/pv: Implement pv_inject_{event, page_fault, hw_exception}()
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? > >> 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. > >> + } >> + 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. Yes. I chose not to unpick that swamp right now, but if reserved_bit_page_fault() gets moved out, this bit can be simplified to this single gprintk(). ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |