| Message ID | 1415608436-5127-2-git-send-email-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ard, On Mon, Nov 10, 2014 at 09:33:56AM +0100, Ard Biesheuvel wrote: > This reverts commit 85c8555ff0 ("KVM: check for !is_zero_pfn() in > kvm_is_mmio_pfn()") and renames the function to kvm_is_reserved_pfn. > > The problem being addressed by the patch above was that some ARM code > based the memory mapping attributes of a pfn on the return value of > kvm_is_mmio_pfn(), whose name indeed suggests that such pfns should > be mapped as device memory. > > However, kvm_is_mmio_pfn() doesn't do quite what it says on the tin, > and the existing non-ARM users were already using it in a way which > suggests that its name should probably have been 'kvm_is_reserved_pfn' > from the beginning, e.g., whether or not to call get_page/put_page on > it etc. This means that returning false for the zero page is a mistake > and the patch above should be reverted. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/ia64/kvm/kvm-ia64.c | 2 +- > arch/x86/kvm/mmu.c | 6 +++--- > include/linux/kvm_host.h | 2 +- > virt/kvm/kvm_main.c | 16 ++++++++-------- > 4 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > index ec6b9acb6bea..dbe46f43884d 100644 > --- a/arch/ia64/kvm/kvm-ia64.c > +++ b/arch/ia64/kvm/kvm-ia64.c > @@ -1563,7 +1563,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > > for (i = 0; i < npages; i++) { > pfn = gfn_to_pfn(kvm, base_gfn + i); > - if (!kvm_is_mmio_pfn(pfn)) { > + if (!kvm_is_reserved_pfn(pfn)) { > kvm_set_pmt_entry(kvm, base_gfn + i, > pfn << PAGE_SHIFT, > _PAGE_AR_RWX | _PAGE_MA_WB); > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index ac1c4de3a484..978f402006ee 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -630,7 +630,7 @@ static int mmu_spte_clear_track_bits(u64 *sptep) > * kvm mmu, before reclaiming the page, we should > * unmap it from mmu first. > */ > - WARN_ON(!kvm_is_mmio_pfn(pfn) && !page_count(pfn_to_page(pfn))); > + WARN_ON(!kvm_is_reserved_pfn(pfn) && !page_count(pfn_to_page(pfn))); > > if (!shadow_accessed_mask || old_spte & shadow_accessed_mask) > kvm_set_pfn_accessed(pfn); > @@ -2461,7 +2461,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > spte |= PT_PAGE_SIZE_MASK; > if (tdp_enabled) > spte |= kvm_x86_ops->get_mt_mask(vcpu, gfn, > - kvm_is_mmio_pfn(pfn)); > + kvm_is_reserved_pfn(pfn)); > > if (host_writable) > spte |= SPTE_HOST_WRITEABLE; > @@ -2737,7 +2737,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, > * PT_PAGE_TABLE_LEVEL and there would be no adjustment done > * here. > */ > - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn) && > + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn) && > level == PT_PAGE_TABLE_LEVEL && > PageTransCompound(pfn_to_page(pfn)) && > !has_wrprotected_page(vcpu->kvm, gfn, PT_DIRECTORY_LEVEL)) { > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index ea53b04993f2..a6059bdf7b03 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -703,7 +703,7 @@ void kvm_arch_sync_events(struct kvm *kvm); > int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu); > void kvm_vcpu_kick(struct kvm_vcpu *vcpu); > > -bool kvm_is_mmio_pfn(pfn_t pfn); > +bool kvm_is_reserved_pfn(pfn_t pfn); > > struct kvm_irq_ack_notifier { > struct hlist_node link; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 25ffac9e947d..3cee7b167052 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -107,10 +107,10 @@ EXPORT_SYMBOL_GPL(kvm_rebooting); > > static bool largepages_enabled = true; > > -bool kvm_is_mmio_pfn(pfn_t pfn) > +bool kvm_is_reserved_pfn(pfn_t pfn) > { > if (pfn_valid(pfn)) > - return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn)); > + return PageReserved(pfn_to_page(pfn)); so we return true for !pfn_valid(pfn), is this still semantically correct with the rename? > > return true; > } > @@ -1321,7 +1321,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, > else if ((vma->vm_flags & VM_PFNMAP)) { > pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) + > vma->vm_pgoff; > - BUG_ON(!kvm_is_mmio_pfn(pfn)); > + BUG_ON(!kvm_is_reserved_pfn(pfn)); > } else { > if (async && vma_is_valid(vma, write_fault)) > *async = true; > @@ -1427,7 +1427,7 @@ static struct page *kvm_pfn_to_page(pfn_t pfn) > if (is_error_noslot_pfn(pfn)) > return KVM_ERR_PTR_BAD_PAGE; > > - if (kvm_is_mmio_pfn(pfn)) { > + if (kvm_is_reserved_pfn(pfn)) { > WARN_ON(1); > return KVM_ERR_PTR_BAD_PAGE; > } > @@ -1456,7 +1456,7 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean); > > void kvm_release_pfn_clean(pfn_t pfn) > { > - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn)) > + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) > put_page(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_release_pfn_clean); > @@ -1477,7 +1477,7 @@ static void kvm_release_pfn_dirty(pfn_t pfn) > > void kvm_set_pfn_dirty(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) { > + if (!kvm_is_reserved_pfn(pfn)) { > struct page *page = pfn_to_page(pfn); > if (!PageReserved(page)) > SetPageDirty(page); this looks rather redundant now then? Or is it catering specifically to the situation where !pfn_valid(pfn) ? > @@ -1487,14 +1487,14 @@ EXPORT_SYMBOL_GPL(kvm_set_pfn_dirty); > > void kvm_set_pfn_accessed(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) > + if (!kvm_is_reserved_pfn(pfn)) > mark_page_accessed(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_set_pfn_accessed); > > void kvm_get_pfn(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) > + if (!kvm_is_reserved_pfn(pfn)) > get_page(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_get_pfn); > -- > 1.8.3.2 > Thanks for looking at this! -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 November 2014 11:53, Christoffer Dall <christoffer.dall@linaro.org> wrote: > Hi Ard, > > On Mon, Nov 10, 2014 at 09:33:56AM +0100, Ard Biesheuvel wrote: >> This reverts commit 85c8555ff0 ("KVM: check for !is_zero_pfn() in >> kvm_is_mmio_pfn()") and renames the function to kvm_is_reserved_pfn. >> >> The problem being addressed by the patch above was that some ARM code >> based the memory mapping attributes of a pfn on the return value of >> kvm_is_mmio_pfn(), whose name indeed suggests that such pfns should >> be mapped as device memory. >> >> However, kvm_is_mmio_pfn() doesn't do quite what it says on the tin, >> and the existing non-ARM users were already using it in a way which >> suggests that its name should probably have been 'kvm_is_reserved_pfn' >> from the beginning, e.g., whether or not to call get_page/put_page on >> it etc. This means that returning false for the zero page is a mistake >> and the patch above should be reverted. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> arch/ia64/kvm/kvm-ia64.c | 2 +- >> arch/x86/kvm/mmu.c | 6 +++--- >> include/linux/kvm_host.h | 2 +- >> virt/kvm/kvm_main.c | 16 ++++++++-------- >> 4 files changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c >> index ec6b9acb6bea..dbe46f43884d 100644 >> --- a/arch/ia64/kvm/kvm-ia64.c >> +++ b/arch/ia64/kvm/kvm-ia64.c >> @@ -1563,7 +1563,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, >> >> for (i = 0; i < npages; i++) { >> pfn = gfn_to_pfn(kvm, base_gfn + i); >> - if (!kvm_is_mmio_pfn(pfn)) { >> + if (!kvm_is_reserved_pfn(pfn)) { >> kvm_set_pmt_entry(kvm, base_gfn + i, >> pfn << PAGE_SHIFT, >> _PAGE_AR_RWX | _PAGE_MA_WB); >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c >> index ac1c4de3a484..978f402006ee 100644 >> --- a/arch/x86/kvm/mmu.c >> +++ b/arch/x86/kvm/mmu.c >> @@ -630,7 +630,7 @@ static int mmu_spte_clear_track_bits(u64 *sptep) >> * kvm mmu, before reclaiming the page, we should >> * unmap it from mmu first. >> */ >> - WARN_ON(!kvm_is_mmio_pfn(pfn) && !page_count(pfn_to_page(pfn))); >> + WARN_ON(!kvm_is_reserved_pfn(pfn) && !page_count(pfn_to_page(pfn))); >> >> if (!shadow_accessed_mask || old_spte & shadow_accessed_mask) >> kvm_set_pfn_accessed(pfn); >> @@ -2461,7 +2461,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, >> spte |= PT_PAGE_SIZE_MASK; >> if (tdp_enabled) >> spte |= kvm_x86_ops->get_mt_mask(vcpu, gfn, >> - kvm_is_mmio_pfn(pfn)); >> + kvm_is_reserved_pfn(pfn)); >> >> if (host_writable) >> spte |= SPTE_HOST_WRITEABLE; >> @@ -2737,7 +2737,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, >> * PT_PAGE_TABLE_LEVEL and there would be no adjustment done >> * here. >> */ >> - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn) && >> + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn) && >> level == PT_PAGE_TABLE_LEVEL && >> PageTransCompound(pfn_to_page(pfn)) && >> !has_wrprotected_page(vcpu->kvm, gfn, PT_DIRECTORY_LEVEL)) { >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index ea53b04993f2..a6059bdf7b03 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -703,7 +703,7 @@ void kvm_arch_sync_events(struct kvm *kvm); >> int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu); >> void kvm_vcpu_kick(struct kvm_vcpu *vcpu); >> >> -bool kvm_is_mmio_pfn(pfn_t pfn); >> +bool kvm_is_reserved_pfn(pfn_t pfn); >> >> struct kvm_irq_ack_notifier { >> struct hlist_node link; >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 25ffac9e947d..3cee7b167052 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -107,10 +107,10 @@ EXPORT_SYMBOL_GPL(kvm_rebooting); >> >> static bool largepages_enabled = true; >> >> -bool kvm_is_mmio_pfn(pfn_t pfn) >> +bool kvm_is_reserved_pfn(pfn_t pfn) >> { >> if (pfn_valid(pfn)) >> - return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn)); >> + return PageReserved(pfn_to_page(pfn)); > > so we return true for !pfn_valid(pfn), is this still semantically > correct with the rename? > I guess it is still debatable, but is arguably more correct than 'kvm_is_mmio_pfn' I was reluctant to choose something like 'kvm_is_special_pfn' because 'special' is not very discriminating here, and at least 'reserved' has a very clear meaning wrt pages, and treating non-struct page backed pfn's as reserved implicitly is not counter-intuitive imo. >> >> return true; >> } >> @@ -1321,7 +1321,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, >> else if ((vma->vm_flags & VM_PFNMAP)) { >> pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) + >> vma->vm_pgoff; >> - BUG_ON(!kvm_is_mmio_pfn(pfn)); >> + BUG_ON(!kvm_is_reserved_pfn(pfn)); >> } else { >> if (async && vma_is_valid(vma, write_fault)) >> *async = true; >> @@ -1427,7 +1427,7 @@ static struct page *kvm_pfn_to_page(pfn_t pfn) >> if (is_error_noslot_pfn(pfn)) >> return KVM_ERR_PTR_BAD_PAGE; >> >> - if (kvm_is_mmio_pfn(pfn)) { >> + if (kvm_is_reserved_pfn(pfn)) { >> WARN_ON(1); >> return KVM_ERR_PTR_BAD_PAGE; >> } >> @@ -1456,7 +1456,7 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean); >> >> void kvm_release_pfn_clean(pfn_t pfn) >> { >> - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn)) >> + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) >> put_page(pfn_to_page(pfn)); >> } >> EXPORT_SYMBOL_GPL(kvm_release_pfn_clean); >> @@ -1477,7 +1477,7 @@ static void kvm_release_pfn_dirty(pfn_t pfn) >> >> void kvm_set_pfn_dirty(pfn_t pfn) >> { >> - if (!kvm_is_mmio_pfn(pfn)) { >> + if (!kvm_is_reserved_pfn(pfn)) { >> struct page *page = pfn_to_page(pfn); >> if (!PageReserved(page)) >> SetPageDirty(page); > > this looks rather redundant now then? Or is it catering specifically to > the situation where !pfn_valid(pfn) ? > I hadn't spotted this myself, to be honest, but that second test was redundant to begin with, i.e., we never enter the block for a reserved page. I can remove it here, or in a followup patch, as you [plural] prefer. >> @@ -1487,14 +1487,14 @@ EXPORT_SYMBOL_GPL(kvm_set_pfn_dirty); >> >> void kvm_set_pfn_accessed(pfn_t pfn) >> { >> - if (!kvm_is_mmio_pfn(pfn)) >> + if (!kvm_is_reserved_pfn(pfn)) >> mark_page_accessed(pfn_to_page(pfn)); >> } >> EXPORT_SYMBOL_GPL(kvm_set_pfn_accessed); >> >> void kvm_get_pfn(pfn_t pfn) >> { >> - if (!kvm_is_mmio_pfn(pfn)) >> + if (!kvm_is_reserved_pfn(pfn)) >> get_page(pfn_to_page(pfn)); >> } >> EXPORT_SYMBOL_GPL(kvm_get_pfn); >> -- >> 1.8.3.2 >> > > Thanks for looking at this! > -Christoffer > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 10, 2014 at 12:05 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 10 November 2014 11:53, Christoffer Dall <christoffer.dall@linaro.org> wrote: >> Hi Ard, >> >> On Mon, Nov 10, 2014 at 09:33:56AM +0100, Ard Biesheuvel wrote: >>> This reverts commit 85c8555ff0 ("KVM: check for !is_zero_pfn() in >>> kvm_is_mmio_pfn()") and renames the function to kvm_is_reserved_pfn. >>> >>> The problem being addressed by the patch above was that some ARM code >>> based the memory mapping attributes of a pfn on the return value of >>> kvm_is_mmio_pfn(), whose name indeed suggests that such pfns should >>> be mapped as device memory. >>> >>> However, kvm_is_mmio_pfn() doesn't do quite what it says on the tin, >>> and the existing non-ARM users were already using it in a way which >>> suggests that its name should probably have been 'kvm_is_reserved_pfn' >>> from the beginning, e.g., whether or not to call get_page/put_page on >>> it etc. This means that returning false for the zero page is a mistake >>> and the patch above should be reverted. >>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> arch/ia64/kvm/kvm-ia64.c | 2 +- >>> arch/x86/kvm/mmu.c | 6 +++--- >>> include/linux/kvm_host.h | 2 +- >>> virt/kvm/kvm_main.c | 16 ++++++++-------- >>> 4 files changed, 13 insertions(+), 13 deletions(-) >>> >>> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c >>> index ec6b9acb6bea..dbe46f43884d 100644 >>> --- a/arch/ia64/kvm/kvm-ia64.c >>> +++ b/arch/ia64/kvm/kvm-ia64.c >>> @@ -1563,7 +1563,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, >>> >>> for (i = 0; i < npages; i++) { >>> pfn = gfn_to_pfn(kvm, base_gfn + i); >>> - if (!kvm_is_mmio_pfn(pfn)) { >>> + if (!kvm_is_reserved_pfn(pfn)) { >>> kvm_set_pmt_entry(kvm, base_gfn + i, >>> pfn << PAGE_SHIFT, >>> _PAGE_AR_RWX | _PAGE_MA_WB); >>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c >>> index ac1c4de3a484..978f402006ee 100644 >>> --- a/arch/x86/kvm/mmu.c >>> +++ b/arch/x86/kvm/mmu.c >>> @@ -630,7 +630,7 @@ static int mmu_spte_clear_track_bits(u64 *sptep) >>> * kvm mmu, before reclaiming the page, we should >>> * unmap it from mmu first. >>> */ >>> - WARN_ON(!kvm_is_mmio_pfn(pfn) && !page_count(pfn_to_page(pfn))); >>> + WARN_ON(!kvm_is_reserved_pfn(pfn) && !page_count(pfn_to_page(pfn))); >>> >>> if (!shadow_accessed_mask || old_spte & shadow_accessed_mask) >>> kvm_set_pfn_accessed(pfn); >>> @@ -2461,7 +2461,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, >>> spte |= PT_PAGE_SIZE_MASK; >>> if (tdp_enabled) >>> spte |= kvm_x86_ops->get_mt_mask(vcpu, gfn, >>> - kvm_is_mmio_pfn(pfn)); >>> + kvm_is_reserved_pfn(pfn)); >>> >>> if (host_writable) >>> spte |= SPTE_HOST_WRITEABLE; >>> @@ -2737,7 +2737,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, >>> * PT_PAGE_TABLE_LEVEL and there would be no adjustment done >>> * here. >>> */ >>> - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn) && >>> + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn) && >>> level == PT_PAGE_TABLE_LEVEL && >>> PageTransCompound(pfn_to_page(pfn)) && >>> !has_wrprotected_page(vcpu->kvm, gfn, PT_DIRECTORY_LEVEL)) { >>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>> index ea53b04993f2..a6059bdf7b03 100644 >>> --- a/include/linux/kvm_host.h >>> +++ b/include/linux/kvm_host.h >>> @@ -703,7 +703,7 @@ void kvm_arch_sync_events(struct kvm *kvm); >>> int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu); >>> void kvm_vcpu_kick(struct kvm_vcpu *vcpu); >>> >>> -bool kvm_is_mmio_pfn(pfn_t pfn); >>> +bool kvm_is_reserved_pfn(pfn_t pfn); >>> >>> struct kvm_irq_ack_notifier { >>> struct hlist_node link; >>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >>> index 25ffac9e947d..3cee7b167052 100644 >>> --- a/virt/kvm/kvm_main.c >>> +++ b/virt/kvm/kvm_main.c >>> @@ -107,10 +107,10 @@ EXPORT_SYMBOL_GPL(kvm_rebooting); >>> >>> static bool largepages_enabled = true; >>> >>> -bool kvm_is_mmio_pfn(pfn_t pfn) >>> +bool kvm_is_reserved_pfn(pfn_t pfn) >>> { >>> if (pfn_valid(pfn)) >>> - return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn)); >>> + return PageReserved(pfn_to_page(pfn)); >> >> so we return true for !pfn_valid(pfn), is this still semantically >> correct with the rename? >> > > I guess it is still debatable, but is arguably more correct than > 'kvm_is_mmio_pfn' > agreed > I was reluctant to choose something like 'kvm_is_special_pfn' because > 'special' is not very discriminating here, and at least 'reserved' has > a very clear meaning wrt pages, and treating non-struct page backed > pfn's as reserved implicitly is not counter-intuitive imo. > just wanted to make sure we thought everything through, and it sounds like we did. >>> >>> return true; >>> } >>> @@ -1321,7 +1321,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, >>> else if ((vma->vm_flags & VM_PFNMAP)) { >>> pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) + >>> vma->vm_pgoff; >>> - BUG_ON(!kvm_is_mmio_pfn(pfn)); >>> + BUG_ON(!kvm_is_reserved_pfn(pfn)); >>> } else { >>> if (async && vma_is_valid(vma, write_fault)) >>> *async = true; >>> @@ -1427,7 +1427,7 @@ static struct page *kvm_pfn_to_page(pfn_t pfn) >>> if (is_error_noslot_pfn(pfn)) >>> return KVM_ERR_PTR_BAD_PAGE; >>> >>> - if (kvm_is_mmio_pfn(pfn)) { >>> + if (kvm_is_reserved_pfn(pfn)) { >>> WARN_ON(1); >>> return KVM_ERR_PTR_BAD_PAGE; >>> } >>> @@ -1456,7 +1456,7 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean); >>> >>> void kvm_release_pfn_clean(pfn_t pfn) >>> { >>> - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn)) >>> + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) >>> put_page(pfn_to_page(pfn)); >>> } >>> EXPORT_SYMBOL_GPL(kvm_release_pfn_clean); >>> @@ -1477,7 +1477,7 @@ static void kvm_release_pfn_dirty(pfn_t pfn) >>> >>> void kvm_set_pfn_dirty(pfn_t pfn) >>> { >>> - if (!kvm_is_mmio_pfn(pfn)) { >>> + if (!kvm_is_reserved_pfn(pfn)) { >>> struct page *page = pfn_to_page(pfn); >>> if (!PageReserved(page)) >>> SetPageDirty(page); >> >> this looks rather redundant now then? Or is it catering specifically to >> the situation where !pfn_valid(pfn) ? >> > > I hadn't spotted this myself, to be honest, but that second test was > redundant to begin with, i.e., we never enter the block for a reserved > page. > I can remove it here, or in a followup patch, as you [plural] prefer. > I have no preference either way, just happened to spot it purely from the diff context. Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 November 2014 09:33, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > This reverts commit 85c8555ff0 ("KVM: check for !is_zero_pfn() in > kvm_is_mmio_pfn()") and renames the function to kvm_is_reserved_pfn. > > The problem being addressed by the patch above was that some ARM code > based the memory mapping attributes of a pfn on the return value of > kvm_is_mmio_pfn(), whose name indeed suggests that such pfns should > be mapped as device memory. > > However, kvm_is_mmio_pfn() doesn't do quite what it says on the tin, > and the existing non-ARM users were already using it in a way which > suggests that its name should probably have been 'kvm_is_reserved_pfn' > from the beginning, e.g., whether or not to call get_page/put_page on > it etc. This means that returning false for the zero page is a mistake > and the patch above should be reverted. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Ping? > --- > arch/ia64/kvm/kvm-ia64.c | 2 +- > arch/x86/kvm/mmu.c | 6 +++--- > include/linux/kvm_host.h | 2 +- > virt/kvm/kvm_main.c | 16 ++++++++-------- > 4 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > index ec6b9acb6bea..dbe46f43884d 100644 > --- a/arch/ia64/kvm/kvm-ia64.c > +++ b/arch/ia64/kvm/kvm-ia64.c > @@ -1563,7 +1563,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > > for (i = 0; i < npages; i++) { > pfn = gfn_to_pfn(kvm, base_gfn + i); > - if (!kvm_is_mmio_pfn(pfn)) { > + if (!kvm_is_reserved_pfn(pfn)) { > kvm_set_pmt_entry(kvm, base_gfn + i, > pfn << PAGE_SHIFT, > _PAGE_AR_RWX | _PAGE_MA_WB); > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index ac1c4de3a484..978f402006ee 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -630,7 +630,7 @@ static int mmu_spte_clear_track_bits(u64 *sptep) > * kvm mmu, before reclaiming the page, we should > * unmap it from mmu first. > */ > - WARN_ON(!kvm_is_mmio_pfn(pfn) && !page_count(pfn_to_page(pfn))); > + WARN_ON(!kvm_is_reserved_pfn(pfn) && !page_count(pfn_to_page(pfn))); > > if (!shadow_accessed_mask || old_spte & shadow_accessed_mask) > kvm_set_pfn_accessed(pfn); > @@ -2461,7 +2461,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > spte |= PT_PAGE_SIZE_MASK; > if (tdp_enabled) > spte |= kvm_x86_ops->get_mt_mask(vcpu, gfn, > - kvm_is_mmio_pfn(pfn)); > + kvm_is_reserved_pfn(pfn)); > > if (host_writable) > spte |= SPTE_HOST_WRITEABLE; > @@ -2737,7 +2737,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, > * PT_PAGE_TABLE_LEVEL and there would be no adjustment done > * here. > */ > - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn) && > + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn) && > level == PT_PAGE_TABLE_LEVEL && > PageTransCompound(pfn_to_page(pfn)) && > !has_wrprotected_page(vcpu->kvm, gfn, PT_DIRECTORY_LEVEL)) { > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index ea53b04993f2..a6059bdf7b03 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -703,7 +703,7 @@ void kvm_arch_sync_events(struct kvm *kvm); > int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu); > void kvm_vcpu_kick(struct kvm_vcpu *vcpu); > > -bool kvm_is_mmio_pfn(pfn_t pfn); > +bool kvm_is_reserved_pfn(pfn_t pfn); > > struct kvm_irq_ack_notifier { > struct hlist_node link; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 25ffac9e947d..3cee7b167052 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -107,10 +107,10 @@ EXPORT_SYMBOL_GPL(kvm_rebooting); > > static bool largepages_enabled = true; > > -bool kvm_is_mmio_pfn(pfn_t pfn) > +bool kvm_is_reserved_pfn(pfn_t pfn) > { > if (pfn_valid(pfn)) > - return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn)); > + return PageReserved(pfn_to_page(pfn)); > > return true; > } > @@ -1321,7 +1321,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, > else if ((vma->vm_flags & VM_PFNMAP)) { > pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) + > vma->vm_pgoff; > - BUG_ON(!kvm_is_mmio_pfn(pfn)); > + BUG_ON(!kvm_is_reserved_pfn(pfn)); > } else { > if (async && vma_is_valid(vma, write_fault)) > *async = true; > @@ -1427,7 +1427,7 @@ static struct page *kvm_pfn_to_page(pfn_t pfn) > if (is_error_noslot_pfn(pfn)) > return KVM_ERR_PTR_BAD_PAGE; > > - if (kvm_is_mmio_pfn(pfn)) { > + if (kvm_is_reserved_pfn(pfn)) { > WARN_ON(1); > return KVM_ERR_PTR_BAD_PAGE; > } > @@ -1456,7 +1456,7 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean); > > void kvm_release_pfn_clean(pfn_t pfn) > { > - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn)) > + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) > put_page(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_release_pfn_clean); > @@ -1477,7 +1477,7 @@ static void kvm_release_pfn_dirty(pfn_t pfn) > > void kvm_set_pfn_dirty(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) { > + if (!kvm_is_reserved_pfn(pfn)) { > struct page *page = pfn_to_page(pfn); > if (!PageReserved(page)) > SetPageDirty(page); > @@ -1487,14 +1487,14 @@ EXPORT_SYMBOL_GPL(kvm_set_pfn_dirty); > > void kvm_set_pfn_accessed(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) > + if (!kvm_is_reserved_pfn(pfn)) > mark_page_accessed(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_set_pfn_accessed); > > void kvm_get_pfn(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) > + if (!kvm_is_reserved_pfn(pfn)) > get_page(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_get_pfn); > -- > 1.8.3.2 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Paolo, I think these look good, would you mind queueing them as either a fix or for 3.19 as you see fit, assuming you agree with the content? Thanks, -Christoffer On Mon, Nov 10, 2014 at 09:33:56AM +0100, Ard Biesheuvel wrote: > This reverts commit 85c8555ff0 ("KVM: check for !is_zero_pfn() in > kvm_is_mmio_pfn()") and renames the function to kvm_is_reserved_pfn. > > The problem being addressed by the patch above was that some ARM code > based the memory mapping attributes of a pfn on the return value of > kvm_is_mmio_pfn(), whose name indeed suggests that such pfns should > be mapped as device memory. > > However, kvm_is_mmio_pfn() doesn't do quite what it says on the tin, > and the existing non-ARM users were already using it in a way which > suggests that its name should probably have been 'kvm_is_reserved_pfn' > from the beginning, e.g., whether or not to call get_page/put_page on > it etc. This means that returning false for the zero page is a mistake > and the patch above should be reverted. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/ia64/kvm/kvm-ia64.c | 2 +- > arch/x86/kvm/mmu.c | 6 +++--- > include/linux/kvm_host.h | 2 +- > virt/kvm/kvm_main.c | 16 ++++++++-------- > 4 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > index ec6b9acb6bea..dbe46f43884d 100644 > --- a/arch/ia64/kvm/kvm-ia64.c > +++ b/arch/ia64/kvm/kvm-ia64.c > @@ -1563,7 +1563,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > > for (i = 0; i < npages; i++) { > pfn = gfn_to_pfn(kvm, base_gfn + i); > - if (!kvm_is_mmio_pfn(pfn)) { > + if (!kvm_is_reserved_pfn(pfn)) { > kvm_set_pmt_entry(kvm, base_gfn + i, > pfn << PAGE_SHIFT, > _PAGE_AR_RWX | _PAGE_MA_WB); > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index ac1c4de3a484..978f402006ee 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -630,7 +630,7 @@ static int mmu_spte_clear_track_bits(u64 *sptep) > * kvm mmu, before reclaiming the page, we should > * unmap it from mmu first. > */ > - WARN_ON(!kvm_is_mmio_pfn(pfn) && !page_count(pfn_to_page(pfn))); > + WARN_ON(!kvm_is_reserved_pfn(pfn) && !page_count(pfn_to_page(pfn))); > > if (!shadow_accessed_mask || old_spte & shadow_accessed_mask) > kvm_set_pfn_accessed(pfn); > @@ -2461,7 +2461,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > spte |= PT_PAGE_SIZE_MASK; > if (tdp_enabled) > spte |= kvm_x86_ops->get_mt_mask(vcpu, gfn, > - kvm_is_mmio_pfn(pfn)); > + kvm_is_reserved_pfn(pfn)); > > if (host_writable) > spte |= SPTE_HOST_WRITEABLE; > @@ -2737,7 +2737,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, > * PT_PAGE_TABLE_LEVEL and there would be no adjustment done > * here. > */ > - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn) && > + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn) && > level == PT_PAGE_TABLE_LEVEL && > PageTransCompound(pfn_to_page(pfn)) && > !has_wrprotected_page(vcpu->kvm, gfn, PT_DIRECTORY_LEVEL)) { > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index ea53b04993f2..a6059bdf7b03 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -703,7 +703,7 @@ void kvm_arch_sync_events(struct kvm *kvm); > int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu); > void kvm_vcpu_kick(struct kvm_vcpu *vcpu); > > -bool kvm_is_mmio_pfn(pfn_t pfn); > +bool kvm_is_reserved_pfn(pfn_t pfn); > > struct kvm_irq_ack_notifier { > struct hlist_node link; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 25ffac9e947d..3cee7b167052 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -107,10 +107,10 @@ EXPORT_SYMBOL_GPL(kvm_rebooting); > > static bool largepages_enabled = true; > > -bool kvm_is_mmio_pfn(pfn_t pfn) > +bool kvm_is_reserved_pfn(pfn_t pfn) > { > if (pfn_valid(pfn)) > - return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn)); > + return PageReserved(pfn_to_page(pfn)); > > return true; > } > @@ -1321,7 +1321,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, > else if ((vma->vm_flags & VM_PFNMAP)) { > pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) + > vma->vm_pgoff; > - BUG_ON(!kvm_is_mmio_pfn(pfn)); > + BUG_ON(!kvm_is_reserved_pfn(pfn)); > } else { > if (async && vma_is_valid(vma, write_fault)) > *async = true; > @@ -1427,7 +1427,7 @@ static struct page *kvm_pfn_to_page(pfn_t pfn) > if (is_error_noslot_pfn(pfn)) > return KVM_ERR_PTR_BAD_PAGE; > > - if (kvm_is_mmio_pfn(pfn)) { > + if (kvm_is_reserved_pfn(pfn)) { > WARN_ON(1); > return KVM_ERR_PTR_BAD_PAGE; > } > @@ -1456,7 +1456,7 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean); > > void kvm_release_pfn_clean(pfn_t pfn) > { > - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn)) > + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) > put_page(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_release_pfn_clean); > @@ -1477,7 +1477,7 @@ static void kvm_release_pfn_dirty(pfn_t pfn) > > void kvm_set_pfn_dirty(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) { > + if (!kvm_is_reserved_pfn(pfn)) { > struct page *page = pfn_to_page(pfn); > if (!PageReserved(page)) > SetPageDirty(page); > @@ -1487,14 +1487,14 @@ EXPORT_SYMBOL_GPL(kvm_set_pfn_dirty); > > void kvm_set_pfn_accessed(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) > + if (!kvm_is_reserved_pfn(pfn)) > mark_page_accessed(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_set_pfn_accessed); > > void kvm_get_pfn(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) > + if (!kvm_is_reserved_pfn(pfn)) > get_page(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_get_pfn); > -- > 1.8.3.2 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/11/2014 12:46, Christoffer Dall wrote: > Hi Paolo, > > I think these look good, would you mind queueing them as either a fix or > for 3.19 as you see fit, assuming you agree with the content? Ah, I was thinking _you_ would queue them for 3.19. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 21, 2014 at 02:06:40PM +0100, Paolo Bonzini wrote: > > > On 21/11/2014 12:46, Christoffer Dall wrote: > > Hi Paolo, > > > > I think these look good, would you mind queueing them as either a fix or > > for 3.19 as you see fit, assuming you agree with the content? > > Ah, I was thinking _you_ would queue them for 3.19. > We can do that, did I miss your previous ack or reviewed-by? -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/11/2014 09:33, Ard Biesheuvel wrote: > This reverts commit 85c8555ff0 ("KVM: check for !is_zero_pfn() in > kvm_is_mmio_pfn()") and renames the function to kvm_is_reserved_pfn. > > The problem being addressed by the patch above was that some ARM code > based the memory mapping attributes of a pfn on the return value of > kvm_is_mmio_pfn(), whose name indeed suggests that such pfns should > be mapped as device memory. > > However, kvm_is_mmio_pfn() doesn't do quite what it says on the tin, > and the existing non-ARM users were already using it in a way which > suggests that its name should probably have been 'kvm_is_reserved_pfn' > from the beginning, e.g., whether or not to call get_page/put_page on > it etc. This means that returning false for the zero page is a mistake > and the patch above should be reverted. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/ia64/kvm/kvm-ia64.c | 2 +- > arch/x86/kvm/mmu.c | 6 +++--- > include/linux/kvm_host.h | 2 +- > virt/kvm/kvm_main.c | 16 ++++++++-------- > 4 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > index ec6b9acb6bea..dbe46f43884d 100644 > --- a/arch/ia64/kvm/kvm-ia64.c > +++ b/arch/ia64/kvm/kvm-ia64.c > @@ -1563,7 +1563,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > > for (i = 0; i < npages; i++) { > pfn = gfn_to_pfn(kvm, base_gfn + i); > - if (!kvm_is_mmio_pfn(pfn)) { > + if (!kvm_is_reserved_pfn(pfn)) { > kvm_set_pmt_entry(kvm, base_gfn + i, > pfn << PAGE_SHIFT, > _PAGE_AR_RWX | _PAGE_MA_WB); > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index ac1c4de3a484..978f402006ee 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -630,7 +630,7 @@ static int mmu_spte_clear_track_bits(u64 *sptep) > * kvm mmu, before reclaiming the page, we should > * unmap it from mmu first. > */ > - WARN_ON(!kvm_is_mmio_pfn(pfn) && !page_count(pfn_to_page(pfn))); > + WARN_ON(!kvm_is_reserved_pfn(pfn) && !page_count(pfn_to_page(pfn))); > > if (!shadow_accessed_mask || old_spte & shadow_accessed_mask) > kvm_set_pfn_accessed(pfn); > @@ -2461,7 +2461,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > spte |= PT_PAGE_SIZE_MASK; > if (tdp_enabled) > spte |= kvm_x86_ops->get_mt_mask(vcpu, gfn, > - kvm_is_mmio_pfn(pfn)); > + kvm_is_reserved_pfn(pfn)); > > if (host_writable) > spte |= SPTE_HOST_WRITEABLE; > @@ -2737,7 +2737,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, > * PT_PAGE_TABLE_LEVEL and there would be no adjustment done > * here. > */ > - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn) && > + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn) && > level == PT_PAGE_TABLE_LEVEL && > PageTransCompound(pfn_to_page(pfn)) && > !has_wrprotected_page(vcpu->kvm, gfn, PT_DIRECTORY_LEVEL)) { > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index ea53b04993f2..a6059bdf7b03 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -703,7 +703,7 @@ void kvm_arch_sync_events(struct kvm *kvm); > int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu); > void kvm_vcpu_kick(struct kvm_vcpu *vcpu); > > -bool kvm_is_mmio_pfn(pfn_t pfn); > +bool kvm_is_reserved_pfn(pfn_t pfn); > > struct kvm_irq_ack_notifier { > struct hlist_node link; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 25ffac9e947d..3cee7b167052 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -107,10 +107,10 @@ EXPORT_SYMBOL_GPL(kvm_rebooting); > > static bool largepages_enabled = true; > > -bool kvm_is_mmio_pfn(pfn_t pfn) > +bool kvm_is_reserved_pfn(pfn_t pfn) > { > if (pfn_valid(pfn)) > - return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn)); > + return PageReserved(pfn_to_page(pfn)); > > return true; > } > @@ -1321,7 +1321,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, > else if ((vma->vm_flags & VM_PFNMAP)) { > pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) + > vma->vm_pgoff; > - BUG_ON(!kvm_is_mmio_pfn(pfn)); > + BUG_ON(!kvm_is_reserved_pfn(pfn)); > } else { > if (async && vma_is_valid(vma, write_fault)) > *async = true; > @@ -1427,7 +1427,7 @@ static struct page *kvm_pfn_to_page(pfn_t pfn) > if (is_error_noslot_pfn(pfn)) > return KVM_ERR_PTR_BAD_PAGE; > > - if (kvm_is_mmio_pfn(pfn)) { > + if (kvm_is_reserved_pfn(pfn)) { > WARN_ON(1); > return KVM_ERR_PTR_BAD_PAGE; > } > @@ -1456,7 +1456,7 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean); > > void kvm_release_pfn_clean(pfn_t pfn) > { > - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn)) > + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) > put_page(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_release_pfn_clean); > @@ -1477,7 +1477,7 @@ static void kvm_release_pfn_dirty(pfn_t pfn) > > void kvm_set_pfn_dirty(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) { > + if (!kvm_is_reserved_pfn(pfn)) { > struct page *page = pfn_to_page(pfn); > if (!PageReserved(page)) > SetPageDirty(page); > @@ -1487,14 +1487,14 @@ EXPORT_SYMBOL_GPL(kvm_set_pfn_dirty); > > void kvm_set_pfn_accessed(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) > + if (!kvm_is_reserved_pfn(pfn)) > mark_page_accessed(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_set_pfn_accessed); > > void kvm_get_pfn(pfn_t pfn) > { > - if (!kvm_is_mmio_pfn(pfn)) > + if (!kvm_is_reserved_pfn(pfn)) > get_page(pfn_to_page(pfn)); > } > EXPORT_SYMBOL_GPL(kvm_get_pfn); > Acked-by: Paolo Bonzini <pbonzini@redhat.com> Since this is in practice ARM only, I'll apply these for 3.18. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/11/2014 14:18, Christoffer Dall wrote: > On Fri, Nov 21, 2014 at 02:06:40PM +0100, Paolo Bonzini wrote: >> >> >> On 21/11/2014 12:46, Christoffer Dall wrote: >>> Hi Paolo, >>> >>> I think these look good, would you mind queueing them as either a fix or >>> for 3.19 as you see fit, assuming you agree with the content? >> >> Ah, I was thinking _you_ would queue them for 3.19. >> > We can do that, did I miss your previous ack or reviewed-by? Since there's more stuff for 3.18 I can include these too. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 21, 2014 at 07:35:46PM +0100, Paolo Bonzini wrote: > > > On 10/11/2014 09:33, Ard Biesheuvel wrote: > > This reverts commit 85c8555ff0 ("KVM: check for !is_zero_pfn() in > > kvm_is_mmio_pfn()") and renames the function to kvm_is_reserved_pfn. > > > > The problem being addressed by the patch above was that some ARM code > > based the memory mapping attributes of a pfn on the return value of > > kvm_is_mmio_pfn(), whose name indeed suggests that such pfns should > > be mapped as device memory. > > > > However, kvm_is_mmio_pfn() doesn't do quite what it says on the tin, > > and the existing non-ARM users were already using it in a way which > > suggests that its name should probably have been 'kvm_is_reserved_pfn' > > from the beginning, e.g., whether or not to call get_page/put_page on > > it etc. This means that returning false for the zero page is a mistake > > and the patch above should be reverted. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > arch/ia64/kvm/kvm-ia64.c | 2 +- > > arch/x86/kvm/mmu.c | 6 +++--- > > include/linux/kvm_host.h | 2 +- > > virt/kvm/kvm_main.c | 16 ++++++++-------- > > 4 files changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > > index ec6b9acb6bea..dbe46f43884d 100644 > > --- a/arch/ia64/kvm/kvm-ia64.c > > +++ b/arch/ia64/kvm/kvm-ia64.c > > @@ -1563,7 +1563,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > > > > for (i = 0; i < npages; i++) { > > pfn = gfn_to_pfn(kvm, base_gfn + i); > > - if (!kvm_is_mmio_pfn(pfn)) { > > + if (!kvm_is_reserved_pfn(pfn)) { > > kvm_set_pmt_entry(kvm, base_gfn + i, > > pfn << PAGE_SHIFT, > > _PAGE_AR_RWX | _PAGE_MA_WB); > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > > index ac1c4de3a484..978f402006ee 100644 > > --- a/arch/x86/kvm/mmu.c > > +++ b/arch/x86/kvm/mmu.c > > @@ -630,7 +630,7 @@ static int mmu_spte_clear_track_bits(u64 *sptep) > > * kvm mmu, before reclaiming the page, we should > > * unmap it from mmu first. > > */ > > - WARN_ON(!kvm_is_mmio_pfn(pfn) && !page_count(pfn_to_page(pfn))); > > + WARN_ON(!kvm_is_reserved_pfn(pfn) && !page_count(pfn_to_page(pfn))); > > > > if (!shadow_accessed_mask || old_spte & shadow_accessed_mask) > > kvm_set_pfn_accessed(pfn); > > @@ -2461,7 +2461,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > > spte |= PT_PAGE_SIZE_MASK; > > if (tdp_enabled) > > spte |= kvm_x86_ops->get_mt_mask(vcpu, gfn, > > - kvm_is_mmio_pfn(pfn)); > > + kvm_is_reserved_pfn(pfn)); > > > > if (host_writable) > > spte |= SPTE_HOST_WRITEABLE; > > @@ -2737,7 +2737,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, > > * PT_PAGE_TABLE_LEVEL and there would be no adjustment done > > * here. > > */ > > - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn) && > > + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn) && > > level == PT_PAGE_TABLE_LEVEL && > > PageTransCompound(pfn_to_page(pfn)) && > > !has_wrprotected_page(vcpu->kvm, gfn, PT_DIRECTORY_LEVEL)) { > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index ea53b04993f2..a6059bdf7b03 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -703,7 +703,7 @@ void kvm_arch_sync_events(struct kvm *kvm); > > int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu); > > void kvm_vcpu_kick(struct kvm_vcpu *vcpu); > > > > -bool kvm_is_mmio_pfn(pfn_t pfn); > > +bool kvm_is_reserved_pfn(pfn_t pfn); > > > > struct kvm_irq_ack_notifier { > > struct hlist_node link; > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 25ffac9e947d..3cee7b167052 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -107,10 +107,10 @@ EXPORT_SYMBOL_GPL(kvm_rebooting); > > > > static bool largepages_enabled = true; > > > > -bool kvm_is_mmio_pfn(pfn_t pfn) > > +bool kvm_is_reserved_pfn(pfn_t pfn) > > { > > if (pfn_valid(pfn)) > > - return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn)); > > + return PageReserved(pfn_to_page(pfn)); > > > > return true; > > } > > @@ -1321,7 +1321,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, > > else if ((vma->vm_flags & VM_PFNMAP)) { > > pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) + > > vma->vm_pgoff; > > - BUG_ON(!kvm_is_mmio_pfn(pfn)); > > + BUG_ON(!kvm_is_reserved_pfn(pfn)); > > } else { > > if (async && vma_is_valid(vma, write_fault)) > > *async = true; > > @@ -1427,7 +1427,7 @@ static struct page *kvm_pfn_to_page(pfn_t pfn) > > if (is_error_noslot_pfn(pfn)) > > return KVM_ERR_PTR_BAD_PAGE; > > > > - if (kvm_is_mmio_pfn(pfn)) { > > + if (kvm_is_reserved_pfn(pfn)) { > > WARN_ON(1); > > return KVM_ERR_PTR_BAD_PAGE; > > } > > @@ -1456,7 +1456,7 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean); > > > > void kvm_release_pfn_clean(pfn_t pfn) > > { > > - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn)) > > + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) > > put_page(pfn_to_page(pfn)); > > } > > EXPORT_SYMBOL_GPL(kvm_release_pfn_clean); > > @@ -1477,7 +1477,7 @@ static void kvm_release_pfn_dirty(pfn_t pfn) > > > > void kvm_set_pfn_dirty(pfn_t pfn) > > { > > - if (!kvm_is_mmio_pfn(pfn)) { > > + if (!kvm_is_reserved_pfn(pfn)) { > > struct page *page = pfn_to_page(pfn); > > if (!PageReserved(page)) > > SetPageDirty(page); > > @@ -1487,14 +1487,14 @@ EXPORT_SYMBOL_GPL(kvm_set_pfn_dirty); > > > > void kvm_set_pfn_accessed(pfn_t pfn) > > { > > - if (!kvm_is_mmio_pfn(pfn)) > > + if (!kvm_is_reserved_pfn(pfn)) > > mark_page_accessed(pfn_to_page(pfn)); > > } > > EXPORT_SYMBOL_GPL(kvm_set_pfn_accessed); > > > > void kvm_get_pfn(pfn_t pfn) > > { > > - if (!kvm_is_mmio_pfn(pfn)) > > + if (!kvm_is_reserved_pfn(pfn)) > > get_page(pfn_to_page(pfn)); > > } > > EXPORT_SYMBOL_GPL(kvm_get_pfn); > > > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > > Since this is in practice ARM only, I'll apply these for 3.18. > Thanks Paolo! -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c index ec6b9acb6bea..dbe46f43884d 100644 --- a/arch/ia64/kvm/kvm-ia64.c +++ b/arch/ia64/kvm/kvm-ia64.c @@ -1563,7 +1563,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, for (i = 0; i < npages; i++) { pfn = gfn_to_pfn(kvm, base_gfn + i); - if (!kvm_is_mmio_pfn(pfn)) { + if (!kvm_is_reserved_pfn(pfn)) { kvm_set_pmt_entry(kvm, base_gfn + i, pfn << PAGE_SHIFT, _PAGE_AR_RWX | _PAGE_MA_WB); diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index ac1c4de3a484..978f402006ee 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -630,7 +630,7 @@ static int mmu_spte_clear_track_bits(u64 *sptep) * kvm mmu, before reclaiming the page, we should * unmap it from mmu first. */ - WARN_ON(!kvm_is_mmio_pfn(pfn) && !page_count(pfn_to_page(pfn))); + WARN_ON(!kvm_is_reserved_pfn(pfn) && !page_count(pfn_to_page(pfn))); if (!shadow_accessed_mask || old_spte & shadow_accessed_mask) kvm_set_pfn_accessed(pfn); @@ -2461,7 +2461,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, spte |= PT_PAGE_SIZE_MASK; if (tdp_enabled) spte |= kvm_x86_ops->get_mt_mask(vcpu, gfn, - kvm_is_mmio_pfn(pfn)); + kvm_is_reserved_pfn(pfn)); if (host_writable) spte |= SPTE_HOST_WRITEABLE; @@ -2737,7 +2737,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, * PT_PAGE_TABLE_LEVEL and there would be no adjustment done * here. */ - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn) && + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn) && level == PT_PAGE_TABLE_LEVEL && PageTransCompound(pfn_to_page(pfn)) && !has_wrprotected_page(vcpu->kvm, gfn, PT_DIRECTORY_LEVEL)) { diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index ea53b04993f2..a6059bdf7b03 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -703,7 +703,7 @@ void kvm_arch_sync_events(struct kvm *kvm); int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu); void kvm_vcpu_kick(struct kvm_vcpu *vcpu); -bool kvm_is_mmio_pfn(pfn_t pfn); +bool kvm_is_reserved_pfn(pfn_t pfn); struct kvm_irq_ack_notifier { struct hlist_node link; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 25ffac9e947d..3cee7b167052 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -107,10 +107,10 @@ EXPORT_SYMBOL_GPL(kvm_rebooting); static bool largepages_enabled = true; -bool kvm_is_mmio_pfn(pfn_t pfn) +bool kvm_is_reserved_pfn(pfn_t pfn) { if (pfn_valid(pfn)) - return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn)); + return PageReserved(pfn_to_page(pfn)); return true; } @@ -1321,7 +1321,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, else if ((vma->vm_flags & VM_PFNMAP)) { pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff; - BUG_ON(!kvm_is_mmio_pfn(pfn)); + BUG_ON(!kvm_is_reserved_pfn(pfn)); } else { if (async && vma_is_valid(vma, write_fault)) *async = true; @@ -1427,7 +1427,7 @@ static struct page *kvm_pfn_to_page(pfn_t pfn) if (is_error_noslot_pfn(pfn)) return KVM_ERR_PTR_BAD_PAGE; - if (kvm_is_mmio_pfn(pfn)) { + if (kvm_is_reserved_pfn(pfn)) { WARN_ON(1); return KVM_ERR_PTR_BAD_PAGE; } @@ -1456,7 +1456,7 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean); void kvm_release_pfn_clean(pfn_t pfn) { - if (!is_error_noslot_pfn(pfn) && !kvm_is_mmio_pfn(pfn)) + if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) put_page(pfn_to_page(pfn)); } EXPORT_SYMBOL_GPL(kvm_release_pfn_clean); @@ -1477,7 +1477,7 @@ static void kvm_release_pfn_dirty(pfn_t pfn) void kvm_set_pfn_dirty(pfn_t pfn) { - if (!kvm_is_mmio_pfn(pfn)) { + if (!kvm_is_reserved_pfn(pfn)) { struct page *page = pfn_to_page(pfn); if (!PageReserved(page)) SetPageDirty(page); @@ -1487,14 +1487,14 @@ EXPORT_SYMBOL_GPL(kvm_set_pfn_dirty); void kvm_set_pfn_accessed(pfn_t pfn) { - if (!kvm_is_mmio_pfn(pfn)) + if (!kvm_is_reserved_pfn(pfn)) mark_page_accessed(pfn_to_page(pfn)); } EXPORT_SYMBOL_GPL(kvm_set_pfn_accessed); void kvm_get_pfn(pfn_t pfn) { - if (!kvm_is_mmio_pfn(pfn)) + if (!kvm_is_reserved_pfn(pfn)) get_page(pfn_to_page(pfn)); } EXPORT_SYMBOL_GPL(kvm_get_pfn);
This reverts commit 85c8555ff0 ("KVM: check for !is_zero_pfn() in kvm_is_mmio_pfn()") and renames the function to kvm_is_reserved_pfn. The problem being addressed by the patch above was that some ARM code based the memory mapping attributes of a pfn on the return value of kvm_is_mmio_pfn(), whose name indeed suggests that such pfns should be mapped as device memory. However, kvm_is_mmio_pfn() doesn't do quite what it says on the tin, and the existing non-ARM users were already using it in a way which suggests that its name should probably have been 'kvm_is_reserved_pfn' from the beginning, e.g., whether or not to call get_page/put_page on it etc. This means that returning false for the zero page is a mistake and the patch above should be reverted. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/ia64/kvm/kvm-ia64.c | 2 +- arch/x86/kvm/mmu.c | 6 +++--- include/linux/kvm_host.h | 2 +- virt/kvm/kvm_main.c | 16 ++++++++-------- 4 files changed, 13 insertions(+), 13 deletions(-)