On Mon, Nov 10, 2014 at 12:05 PM, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote: > On 10 November 2014 11:53, Christoffer Dall <christoffer.dall@xxxxxxxxxx> 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@xxxxxxxxxx> >>> --- >>> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html