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@lists.xen.org https://lists.xen.org/xen-devel