On Fri, 2018-02-09 at 16:01 +0100, Paolo Bonzini wrote: > > > I don't think the else case is useful. If fact you are checking > > > VM_PFNMAP twice... > > > > yes. I am not sure what the VM_PFNMAP case does. The code comes > > from kvmppc_book3s_hv_page_fault() I suppose. > > Maybe the outer check should be for VM_IO|VM_PFNMAP? > > (Also, why is this code reinventing most of hva_to_pfn? Not asking you > to fix that of course, but perhaps there's something to improve in the > generic code too). I've been wondering the same thing, which led me to try to understand hva_to_pfn, which resulted in skull marks in the wall ;-) So hva_to_pfn does a complicated dance around 3 or 4 different variants of gup and it's really not obvious why, the "intent" doesn't appear to be documented clearly. I think I somewhat figued out it relates to the async page fault stuff but even then I don't actually know what those are and what is their purpose :-) But yeah, Paul's code more/less does its own thing with a simple gup_fast, and is missing the hva_to_pfn_remapped case for VM_IO | VM_PFNMAP. The short fix is to make hva_to_pfn_remapped exported and use it via a fixed variant of Cedric patch. The longer term fix might be to understand if/how we can use hva_to_pfn Cheers, Ben.
