On Thu, Jan 08, 2015 at 08:41:15AM -0800, Mario Smarduch wrote: [...] > >>> > >>> I'm just thinking here, why do we need to check if we get a valid pud > >>> back here, but we don't need the equivalent check in dissolve_pmd from > >>> patch 7? > >> > >> kvm_pud_huge() doesn't check bit 0 for invalid entry, but > >> pud_none() is not the right way to check either, maybe pud_bad() > >> first. Nothing is done in patch 7 since the pmd is retrieved from > >> stage2_get_pmd(). > >> > > > > hmmm, but stage2_get_pmd() can return a NULL pointer if you have the > > IOMAP flag set... > > > >>> > >>> I think the rationale is that it should never happen because we never > >>> call these functions with the logging and iomap flags at the same > >>> time... > >> > >> I'm little lost here, not sure how it's related to above. > >> But I think a VFIO device will have a memslot and > >> it would be possible to enable logging. But to what > >> end I'm not sure. > >> > > > > As I said above, if you call the set_s2pte function with the IOMAP and > > LOGGING flags set, then you'll end up in a situation where you can get a > > NULL pointer back from stage2_get_pmd() but you're never checking > > against that. > > I see what you're saying now. > > > > Now, this raises an interesting point, we have now added code that > > prevents faults from ever happening on device maps, but introducing a > > path here where the user can set logging on a memslot with device memory > > regions, which introduces write faults on such regions. My gut feeling > > is that we should avoid that from ever happening, and not allow this > > function to be called with both flags set. > > Maybe kvm_arch_prepare_memory_region() can check if > KVM_MEM_LOG_DIRTY_PAGES is being enabled for an IO region > and don't allow it. > Yeah, I think we need to add a check for that somewhere as part of this series (patch 7 perhaps?). -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
