| Message ID | 1416236333-9378-3-git-send-email-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/11/2014 15:58, Ard Biesheuvel wrote: > Readonly memslots are often used to implement emulation of ROMs and > NOR flashes, in which case the guest may legally map these regions as > uncached. > To deal with the incoherency associated with uncached guest mappings, > treat all readonly memslots as incoherent, and ensure that pages that > belong to regions tagged as such are flushed to DRAM before being passed > to the guest. On x86, the processor combines the cacheability values from the two levels of page tables. Is there no way to do the same on ARM? 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
Hi Paolo, On 17/11/14 15:29, Paolo Bonzini wrote: > > > On 17/11/2014 15:58, Ard Biesheuvel wrote: >> Readonly memslots are often used to implement emulation of ROMs and >> NOR flashes, in which case the guest may legally map these regions as >> uncached. >> To deal with the incoherency associated with uncached guest mappings, >> treat all readonly memslots as incoherent, and ensure that pages that >> belong to regions tagged as such are flushed to DRAM before being passed >> to the guest. > > On x86, the processor combines the cacheability values from the two > levels of page tables. Is there no way to do the same on ARM? ARM is broadly similar, but there's a number of gotchas: - uncacheable (guest level) + cacheable (host level) -> uncacheable: the read request is going to be directly sent to RAM, bypassing the caches. - Userspace is going to use a cacheable view of the "NOR" pages, which is going to stick around in the cache (this is just memory, after all). The net result is that we need to detect those cases and make sure the guest sees the latest bit of data written by userland. We already have a similar mechanism when we fault pages in, but the guest has not enabled its caches yet. M.
On 17/11/2014 16:39, Marc Zyngier wrote: > ARM is broadly similar, but there's a number of gotchas: > - uncacheable (guest level) + cacheable (host level) -> uncacheable: the > read request is going to be directly sent to RAM, bypassing the caches. > - Userspace is going to use a cacheable view of the "NOR" pages, which > is going to stick around in the cache (this is just memory, after all). Ah, x86 also has uncacheable + cacheable -> uncacheable, but Intel also added a bit to ignore the guest-provided type. We use that bit for RAM-backed areas. Also, on x86 if the cache is disabled the processor will still snoop caches (including its own cache) and perform writeback+invalidate of the cache line before accessing main memory, if it's dirty. AMD does not have the aforementioned bit, but applies this same algorithm if the host says the page is writeback in the MTRR (memory type range register). The Intel solution is less tricky and has better performance. Paolo > The net result is that we need to detect those cases and make sure the > guest sees the latest bit of data written by userland. > > We already have a similar mechanism when we fault pages in, but the > guest has not enabled its caches yet. -- 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 17 November 2014 15:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > Readonly memslots are often used to implement emulation of ROMs and > NOR flashes, in which case the guest may legally map these regions as > uncached. > To deal with the incoherency associated with uncached guest mappings, > treat all readonly memslots as incoherent, and ensure that pages that > belong to regions tagged as such are flushed to DRAM before being passed > to the guest. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- Hello all, I have another bug report (from Canonical this time) of essentially the same issue, and it is also fixed by these patches. Are you happy with these patches? Should I respin to add Laszlo's tested-by? Cheers, Ard. > arch/arm/kvm/mmu.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c > index cb924c6d56a6..f2a9874ff5cb 100644 > --- a/arch/arm/kvm/mmu.c > +++ b/arch/arm/kvm/mmu.c > @@ -919,7 +919,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > if (!hugetlb && !force_pte) > hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa); > > - fault_ipa_uncached = false; > + fault_ipa_uncached = memslot->flags & KVM_MEMSLOT_INCOHERENT; > > if (hugetlb) { > pmd_t new_pmd = pfn_pmd(pfn, mem_type); > @@ -1298,11 +1298,12 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > hva = vm_end; > } while (hva < reg_end); > > - if (ret) { > - spin_lock(&kvm->mmu_lock); > + spin_lock(&kvm->mmu_lock); > + if (ret) > unmap_stage2_range(kvm, mem->guest_phys_addr, mem->memory_size); > - spin_unlock(&kvm->mmu_lock); > - } > + else > + stage2_flush_memslot(kvm, memslot); > + spin_unlock(&kvm->mmu_lock); > return ret; > } > > @@ -1314,6 +1315,15 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free, > int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot, > unsigned long npages) > { > + /* > + * Readonly memslots are not incoherent with the caches by definition, > + * but in practice, they are used mostly to emulate ROMs or NOR flashes > + * that the guest may consider devices and hence map as uncached. > + * To prevent incoherency issues in these cases, tag all readonly > + * regions as incoherent. > + */ > + if (slot->flags & KVM_MEM_READONLY) > + slot->flags |= KVM_MEMSLOT_INCOHERENT; > return 0; > } > > -- > 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 19/11/2014 09:51, Ard Biesheuvel wrote: > On 17 November 2014 15:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> Readonly memslots are often used to implement emulation of ROMs and >> NOR flashes, in which case the guest may legally map these regions as >> uncached. >> To deal with the incoherency associated with uncached guest mappings, >> treat all readonly memslots as incoherent, and ensure that pages that >> belong to regions tagged as such are flushed to DRAM before being passed >> to the guest. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- > > Hello all, > > I have another bug report (from Canonical this time) of essentially > the same issue, and it is also fixed by these patches. > Are you happy with these patches? Should I respin to add Laszlo's tested-by? Christoffer can add it, together with... Acked-by: Paolo Bonzini <pbonzini@redhat.com> Paolo > Cheers, > Ard. > > >> arch/arm/kvm/mmu.c | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c >> index cb924c6d56a6..f2a9874ff5cb 100644 >> --- a/arch/arm/kvm/mmu.c >> +++ b/arch/arm/kvm/mmu.c >> @@ -919,7 +919,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, >> if (!hugetlb && !force_pte) >> hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa); >> >> - fault_ipa_uncached = false; >> + fault_ipa_uncached = memslot->flags & KVM_MEMSLOT_INCOHERENT; >> >> if (hugetlb) { >> pmd_t new_pmd = pfn_pmd(pfn, mem_type); >> @@ -1298,11 +1298,12 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, >> hva = vm_end; >> } while (hva < reg_end); >> >> - if (ret) { >> - spin_lock(&kvm->mmu_lock); >> + spin_lock(&kvm->mmu_lock); >> + if (ret) >> unmap_stage2_range(kvm, mem->guest_phys_addr, mem->memory_size); >> - spin_unlock(&kvm->mmu_lock); >> - } >> + else >> + stage2_flush_memslot(kvm, memslot); >> + spin_unlock(&kvm->mmu_lock); >> return ret; >> } >> >> @@ -1314,6 +1315,15 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free, >> int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot, >> unsigned long npages) >> { >> + /* >> + * Readonly memslots are not incoherent with the caches by definition, >> + * but in practice, they are used mostly to emulate ROMs or NOR flashes >> + * that the guest may consider devices and hence map as uncached. >> + * To prevent incoherency issues in these cases, tag all readonly >> + * regions as incoherent. >> + */ >> + if (slot->flags & KVM_MEM_READONLY) >> + slot->flags |= KVM_MEMSLOT_INCOHERENT; >> return 0; >> } >> >> -- >> 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 > -- 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 19/11/2014 09:51, Ard Biesheuvel wrote: > On 17 November 2014 15:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> Readonly memslots are often used to implement emulation of ROMs and >> NOR flashes, in which case the guest may legally map these regions as >> uncached. >> To deal with the incoherency associated with uncached guest mappings, >> treat all readonly memslots as incoherent, and ensure that pages that >> belong to regions tagged as such are flushed to DRAM before being passed >> to the guest. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- > > Hello all, > > I have another bug report (from Canonical this time) of essentially > the same issue, and it is also fixed by these patches. > Are you happy with these patches? Should I respin to add Laszlo's tested-by? Christoffer can add it, together with... Acked-by: Paolo Bonzini <pbonzini@redhat.com> It will be 3.19 only, though. Paolo > Cheers, > Ard. > > >> arch/arm/kvm/mmu.c | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c >> index cb924c6d56a6..f2a9874ff5cb 100644 >> --- a/arch/arm/kvm/mmu.c >> +++ b/arch/arm/kvm/mmu.c >> @@ -919,7 +919,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, >> if (!hugetlb && !force_pte) >> hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa); >> >> - fault_ipa_uncached = false; >> + fault_ipa_uncached = memslot->flags & KVM_MEMSLOT_INCOHERENT; >> >> if (hugetlb) { >> pmd_t new_pmd = pfn_pmd(pfn, mem_type); >> @@ -1298,11 +1298,12 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, >> hva = vm_end; >> } while (hva < reg_end); >> >> - if (ret) { >> - spin_lock(&kvm->mmu_lock); >> + spin_lock(&kvm->mmu_lock); >> + if (ret) >> unmap_stage2_range(kvm, mem->guest_phys_addr, mem->memory_size); >> - spin_unlock(&kvm->mmu_lock); >> - } >> + else >> + stage2_flush_memslot(kvm, memslot); >> + spin_unlock(&kvm->mmu_lock); >> return ret; >> } >> >> @@ -1314,6 +1315,15 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free, >> int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot, >> unsigned long npages) >> { >> + /* >> + * Readonly memslots are not incoherent with the caches by definition, >> + * but in practice, they are used mostly to emulate ROMs or NOR flashes >> + * that the guest may consider devices and hence map as uncached. >> + * To prevent incoherency issues in these cases, tag all readonly >> + * regions as incoherent. >> + */ >> + if (slot->flags & KVM_MEM_READONLY) >> + slot->flags |= KVM_MEMSLOT_INCOHERENT; >> return 0; >> } >> >> -- >> 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 > -- 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/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c index cb924c6d56a6..f2a9874ff5cb 100644 --- a/arch/arm/kvm/mmu.c +++ b/arch/arm/kvm/mmu.c @@ -919,7 +919,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, if (!hugetlb && !force_pte) hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa); - fault_ipa_uncached = false; + fault_ipa_uncached = memslot->flags & KVM_MEMSLOT_INCOHERENT; if (hugetlb) { pmd_t new_pmd = pfn_pmd(pfn, mem_type); @@ -1298,11 +1298,12 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, hva = vm_end; } while (hva < reg_end); - if (ret) { - spin_lock(&kvm->mmu_lock); + spin_lock(&kvm->mmu_lock); + if (ret) unmap_stage2_range(kvm, mem->guest_phys_addr, mem->memory_size); - spin_unlock(&kvm->mmu_lock); - } + else + stage2_flush_memslot(kvm, memslot); + spin_unlock(&kvm->mmu_lock); return ret; } @@ -1314,6 +1315,15 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free, int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot, unsigned long npages) { + /* + * Readonly memslots are not incoherent with the caches by definition, + * but in practice, they are used mostly to emulate ROMs or NOR flashes + * that the guest may consider devices and hence map as uncached. + * To prevent incoherency issues in these cases, tag all readonly + * regions as incoherent. + */ + if (slot->flags & KVM_MEM_READONLY) + slot->flags |= KVM_MEMSLOT_INCOHERENT; return 0; }
Readonly memslots are often used to implement emulation of ROMs and NOR flashes, in which case the guest may legally map these regions as uncached. To deal with the incoherency associated with uncached guest mappings, treat all readonly memslots as incoherent, and ensure that pages that belong to regions tagged as such are flushed to DRAM before being passed to the guest. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm/kvm/mmu.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)