[PATCH 1/3] kvm: add a memslot flag for incoherent memory regions

[PATCH 1/3] kvm: add a memslot flag for incoherent memory regions

[Ard Biesheuvel]

Memory regions may be incoherent with the caches, typically when the
guest has mapped a host system RAM backed memory region as uncached.
Add a flag KVM_MEMSLOT_INCOHERENT so that we can tag these memslots
and handle them appropriately when mapping them.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 include/linux/kvm_host.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index a6059bdf7b03..e4d8f705fecd 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -43,6 +43,7 @@
  * include/linux/kvm_h.
  */
 #define KVM_MEMSLOT_INVALID	(1UL << 16)
+#define KVM_MEMSLOT_INCOHERENT	(1UL << 17)
 
 /* Two fragments for cross MMIO pages. */
 #define KVM_MAX_MMIO_FRAGMENTS	2
-- 
1.8.3.2

[PATCH 2/3] arm, arm64: KVM: allow forced dcache flush on page faults

[Ard Biesheuvel]

From: Laszlo Ersek <lersek@redhat.com>

To allow handling of incoherent memslots in a subsequent patch, this
patch adds a paramater 'ipa_uncached' to cache_coherent_guest_page()
so that we can instruct it to flush the page's contents to DRAM even
if the guest has caching globally enabled.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/kvm_mmu.h   | 5 +++--
 arch/arm/kvm/mmu.c               | 9 +++++++--
 arch/arm64/include/asm/kvm_mmu.h | 5 +++--
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index acb0d5712716..f867060035ec 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -161,9 +161,10 @@ static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
 }
 
 static inline void coherent_cache_guest_page(struct kvm_vcpu *vcpu, hva_t hva,
-					     unsigned long size)
+					     unsigned long size,
+					     bool ipa_uncached)
 {
-	if (!vcpu_has_cache_enabled(vcpu))
+	if (!vcpu_has_cache_enabled(vcpu) || ipa_uncached)
 		kvm_flush_dcache_to_poc((void *)hva, size);
 	
 	/*
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index b007438242e2..cb924c6d56a6 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -852,6 +852,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	struct vm_area_struct *vma;
 	pfn_t pfn;
 	pgprot_t mem_type = PAGE_S2;
+	bool fault_ipa_uncached;
 
 	write_fault = kvm_is_write_fault(vcpu);
 	if (fault_status == FSC_PERM && !write_fault) {
@@ -918,6 +919,8 @@ 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;
+
 	if (hugetlb) {
 		pmd_t new_pmd = pfn_pmd(pfn, mem_type);
 		new_pmd = pmd_mkhuge(new_pmd);
@@ -925,7 +928,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 			kvm_set_s2pmd_writable(&new_pmd);
 			kvm_set_pfn_dirty(pfn);
 		}
-		coherent_cache_guest_page(vcpu, hva & PMD_MASK, PMD_SIZE);
+		coherent_cache_guest_page(vcpu, hva & PMD_MASK, PMD_SIZE,
+					  fault_ipa_uncached);
 		ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
 	} else {
 		pte_t new_pte = pfn_pte(pfn, mem_type);
@@ -933,7 +937,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 			kvm_set_s2pte_writable(&new_pte);
 			kvm_set_pfn_dirty(pfn);
 		}
-		coherent_cache_guest_page(vcpu, hva, PAGE_SIZE);
+		coherent_cache_guest_page(vcpu, hva, PAGE_SIZE,
+					  fault_ipa_uncached);
 		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte,
 			pgprot_val(mem_type) == pgprot_val(PAGE_S2_DEVICE));
 	}
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 0caf7a59f6a1..123b521a9908 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -243,9 +243,10 @@ static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
 }
 
 static inline void coherent_cache_guest_page(struct kvm_vcpu *vcpu, hva_t hva,
-					     unsigned long size)
+					     unsigned long size,
+					     bool ipa_uncached)
 {
-	if (!vcpu_has_cache_enabled(vcpu))
+	if (!vcpu_has_cache_enabled(vcpu) || ipa_uncached)
 		kvm_flush_dcache_to_poc((void *)hva, size);
 
 	if (!icache_is_aliasing()) {		/* PIPT */
-- 
1.8.3.2

[PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Ard Biesheuvel]

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(-)

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

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Paolo Bonzini]

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

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Laszlo Ersek]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

On 11/17/14 16: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?

Combining occurs on ARMv8 too. The Stage1 (guest) mapping is very strict
(Device non-Gathering, non-Reordering, no Early Write Acknowledgement --
for EFI_MEMORY_UC), which basically "overrides" the Stage2 (very lax
host) memory attributes.

When qemu writes, as part of emulating the flash programming commands,
to the RAMBlock that *otherwise* backs the flash range (as a r/o
memslot), those writes (from host userspace) tend to end up in dcache.

But, when the guest flips back the flash to romd mode, and tries to read
back the values from the flash as plain ROM, the dcache is completely
bypassed due to the strict stage1 mapping, and the guest goes directly
to DRAM.

Where qemu's earlier writes are not yet / necessarily visible.

Please see my original patch (which was incomplete) in the attachment,
it has a very verbose commit message.

Anyway, I'll let others explain; they can word it better than I can :)

FWIW,

Series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I ported this series to a 3.17.0+ based kernel, and tested it. It works
fine. The ROM-like view of the NOR flash now reflects the previously
programmed contents.

Series
Tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

[-- Attachment #2: 0001-arm-arm64-KVM-clean-cache-on-page-fault-also-when-IP.upstream.patch --]
[-- Type: text/x-patch, Size: 6063 bytes --]

>From a2b4da9b03f03ccdb8b0988a5cc64d1967f00398 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Sun, 16 Nov 2014 01:43:11 +0100
Subject: [PATCH] arm, arm64: KVM: clean cache on page fault also when IPA is
 uncached (WIP)

This patch builds on Marc Zyngier's commit
2d58b733c87689d3d5144e4ac94ea861cc729145.

(1) The guest bypasses the cache *not only* when the VCPU's dcache is
    disabled (see bit 0 and bit 2 in SCTLR_EL1, "MMU enable" and "Cache
    enable", respectively -- vcpu_has_cache_enabled()).

    The guest bypasses the cache *also* when the Stage 1 memory attributes
    say "device memory" about the Intermediate Page Address in question,
    independently of the Stage 2 memory attributes. Refer to:

      Table D5-38
      Combining the stage 1 and stage 2 memory type assignments

    in the ARM ARM. (This is likely similar to MTRRs on x86.)

(2) In edk2 (EFI Development Kit II), the ARM NOR flash driver,

      ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c

    uses the AddMemorySpace() and SetMemorySpaceAttributes() Global
    Coherency Domain Services of DXE (Driver eXecution Environment) to
    *justifiedly* set the attributes of the guest memory covering the
    flash chip to EFI_MEMORY_UC ("uncached").

    According to the AArch64 bindings for UEFI (see "2.3.6.1 Memory types"
    in the UEFI-2.4A specification), EFI_MEMORY_UC is mapped to:

                       ARM Memory Type:
                       MAIR attribute encoding    ARM Memory Type:
    EFI Memory Type    Attr<n> [7:4] [3:0]        Meaning
    ---------------    -----------------------    ------------------------
    EFI_MEMORY_UC      0000 0000                  Device-nGnRnE (Device
    (Not cacheable)                               non-Gathering,
                                                  non-Reordering, no Early
                                                  Write Acknowledgement)

    This is correctly implemented in edk2, in the ArmConfigureMmu()
    function, via the ArmSetMAIR() call and the MAIR_ATTR() macro:

    The TT_ATTR_INDX_DEVICE_MEMORY (== 0) memory attribute index, which is
    used for EFI_MEMORY_UC memory, is associated with the
    MAIR_ATTR_DEVICE_MEMORY (== 0x00, see above) memory attribute value,
    in the MAIR_ELx register.

As a consequence of (1) and (2), when edk2 code running in the guest
accesses an IPA falling in the flash range, it will completely bypass
the cache.

Therefore, when such a page is faulted in in user_mem_abort(), we must
flush the data cache; otherwise the guest will see stale data in the flash
chip.

This patch is not complete because I have no clue how to calculate the
memory attribute for "fault_ipa" in user_mem_abort(). Right now I set
"fault_ipa_uncached" to constant true, which might incur some performance
penalty for data faults, but it certainly improves correctness -- the
ArmVirtualizationPkg platform build of edk2 actually boots as a KVM guest
on APM Mustang.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 arch/arm/include/asm/kvm_mmu.h   |  5 +++--
 arch/arm64/include/asm/kvm_mmu.h |  5 +++--
 arch/arm/kvm/mmu.c               | 10 ++++++++--
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index acb0d57..f867060 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -161,9 +161,10 @@ static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
 }
 
 static inline void coherent_cache_guest_page(struct kvm_vcpu *vcpu, hva_t hva,
-					     unsigned long size)
+					     unsigned long size,
+					     bool ipa_uncached)
 {
-	if (!vcpu_has_cache_enabled(vcpu))
+	if (!vcpu_has_cache_enabled(vcpu) || ipa_uncached)
 		kvm_flush_dcache_to_poc((void *)hva, size);
 	
 	/*
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 0caf7a5..123b521 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -243,9 +243,10 @@ static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
 }
 
 static inline void coherent_cache_guest_page(struct kvm_vcpu *vcpu, hva_t hva,
-					     unsigned long size)
+					     unsigned long size,
+					     bool ipa_uncached)
 {
-	if (!vcpu_has_cache_enabled(vcpu))
+	if (!vcpu_has_cache_enabled(vcpu) || ipa_uncached)
 		kvm_flush_dcache_to_poc((void *)hva, size);
 
 	if (!icache_is_aliasing()) {		/* PIPT */
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 57a403a..e2323b7 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -847,6 +847,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	struct vm_area_struct *vma;
 	pfn_t pfn;
 	pgprot_t mem_type = PAGE_S2;
+	bool fault_ipa_uncached;
 
 	write_fault = kvm_is_write_fault(vcpu);
 	if (fault_status == FSC_PERM && !write_fault) {
@@ -913,6 +914,9 @@ 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);
 
+	/* TBD */
+	fault_ipa_uncached = true;
+
 	if (hugetlb) {
 		pmd_t new_pmd = pfn_pmd(pfn, mem_type);
 		new_pmd = pmd_mkhuge(new_pmd);
@@ -920,7 +924,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 			kvm_set_s2pmd_writable(&new_pmd);
 			kvm_set_pfn_dirty(pfn);
 		}
-		coherent_cache_guest_page(vcpu, hva & PMD_MASK, PMD_SIZE);
+		coherent_cache_guest_page(vcpu, hva & PMD_MASK, PMD_SIZE,
+					  fault_ipa_uncached);
 		ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
 	} else {
 		pte_t new_pte = pfn_pte(pfn, mem_type);
@@ -928,7 +933,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 			kvm_set_s2pte_writable(&new_pte);
 			kvm_set_pfn_dirty(pfn);
 		}
-		coherent_cache_guest_page(vcpu, hva, PAGE_SIZE);
+		coherent_cache_guest_page(vcpu, hva, PAGE_SIZE,
+					  fault_ipa_uncached);
 		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte,
 			pgprot_val(mem_type) == pgprot_val(PAGE_S2_DEVICE));
 	}
-- 
1.8.3.1

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Paolo Bonzini]

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.

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Ard Biesheuvel]

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
>

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Paolo Bonzini]

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
> 

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Paolo Bonzini]

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
> 

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Mario Smarduch]

Hi Laszlo,

couple observations.

     I'm wondering if access from qemu and guest won't
result in mixed memory attributes and if that's acceptable
to the CPU.

Also is if you update memory from qemu you may break
dirty page logging/migration. Unless there is some other way
you keep track. Of course it may not be applicable in your
case (i.e. flash unused after boot).

- Mario

On 11/17/2014 07:49 AM, Laszlo Ersek wrote:
> On 11/17/14 16: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?
> 
> Combining occurs on ARMv8 too. The Stage1 (guest) mapping is very strict
> (Device non-Gathering, non-Reordering, no Early Write Acknowledgement --
> for EFI_MEMORY_UC), which basically "overrides" the Stage2 (very lax
> host) memory attributes.
> 
> When qemu writes, as part of emulating the flash programming commands,
> to the RAMBlock that *otherwise* backs the flash range (as a r/o
> memslot), those writes (from host userspace) tend to end up in dcache.
> 
> But, when the guest flips back the flash to romd mode, and tries to read
> back the values from the flash as plain ROM, the dcache is completely
> bypassed due to the strict stage1 mapping, and the guest goes directly
> to DRAM.
> 
> Where qemu's earlier writes are not yet / necessarily visible.
> 
> Please see my original patch (which was incomplete) in the attachment,
> it has a very verbose commit message.
> 
> Anyway, I'll let others explain; they can word it better than I can :)
> 
> FWIW,
> 
> Series
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> I ported this series to a 3.17.0+ based kernel, and tested it. It works
> fine. The ROM-like view of the NOR flash now reflects the previously
> programmed contents.
> 
> Series
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks!
> Laszlo
> 
> 
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Laszlo Ersek]

On 11/20/14 00:32, Mario Smarduch wrote:
> Hi Laszlo,
> 
> couple observations.
> 
>      I'm wondering if access from qemu and guest won't
> result in mixed memory attributes and if that's acceptable
> to the CPU.

Normally this would be a problem I think (Jon raised the topic of live
migration). However, for flash programming specifically, I think the
guest's access pattern ensures that we'll see things OK.

When the guest issues the first write access, the memslot is deleted,
and everything is forwarded to qemu, both reads and writes. In response
qemu modifies the array that *otherwise* backs the flash. These
modifications by qemu end up in the dcache mostly. When the guest is
done "programming", it writes a special command (read array mode) at
which point the memslot is recreated (as read-only) and flushed / set up
for flushing during demand paging.

So from the emulated flash POV, the memslot either doesn't exist at all
(and then qemu serves all accesses just fine), or it exists r/o, at
which point qemu (host userspace) will have stopped writing to it, and
will have set it up for flushing before and during guest read accesses.

> Also is if you update memory from qemu you may break
> dirty page logging/migration.

Very probably. Jon said the same thing.

> Unless there is some other way
> you keep track. Of course it may not be applicable in your
> case (i.e. flash unused after boot).

The flash *is* used after boot, because the UEFI runtime variable
services *are* exercised by the guest kernel. However those use the same
access pattern (it's the same set of UEFI services just called by a
different "client").

*Uncoordinated* access from guest and host in parallel will be a big
problem; but we're not that far yet, and we need to get the flash
problem sorted, so that we can at least boot and work on the basic
stuff. The flash programming dance happens to provide coordination; the
flash mode changes (which are equivalent to the teardown and the
recreation of the memslot) can be considered barriers.

I hope this is acceptable for the time being...

Thanks
Laszlo

> 
> - Mario
> 
> On 11/17/2014 07:49 AM, Laszlo Ersek wrote:
>> On 11/17/14 16: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?
>>
>> Combining occurs on ARMv8 too. The Stage1 (guest) mapping is very strict
>> (Device non-Gathering, non-Reordering, no Early Write Acknowledgement --
>> for EFI_MEMORY_UC), which basically "overrides" the Stage2 (very lax
>> host) memory attributes.
>>
>> When qemu writes, as part of emulating the flash programming commands,
>> to the RAMBlock that *otherwise* backs the flash range (as a r/o
>> memslot), those writes (from host userspace) tend to end up in dcache.
>>
>> But, when the guest flips back the flash to romd mode, and tries to read
>> back the values from the flash as plain ROM, the dcache is completely
>> bypassed due to the strict stage1 mapping, and the guest goes directly
>> to DRAM.
>>
>> Where qemu's earlier writes are not yet / necessarily visible.
>>
>> Please see my original patch (which was incomplete) in the attachment,
>> it has a very verbose commit message.
>>
>> Anyway, I'll let others explain; they can word it better than I can :)
>>
>> FWIW,
>>
>> Series
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> I ported this series to a 3.17.0+ based kernel, and tested it. It works
>> fine. The ROM-like view of the NOR flash now reflects the previously
>> programmed contents.
>>
>> Series
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks!
>> Laszlo
>>
>>
>>
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>
> 

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Mario Smarduch]

On 11/20/2014 12:08 AM, Laszlo Ersek wrote:
> On 11/20/14 00:32, Mario Smarduch wrote:
>> Hi Laszlo,
>>
>> couple observations.
>>
>>      I'm wondering if access from qemu and guest won't
>> result in mixed memory attributes and if that's acceptable
>> to the CPU.
> 
> Normally this would be a problem I think (Jon raised the topic of live
> migration). However, for flash programming specifically, I think the
> guest's access pattern ensures that we'll see things OK.
> 
> When the guest issues the first write access, the memslot is deleted,
> and everything is forwarded to qemu, both reads and writes. In response
> qemu modifies the array that *otherwise* backs the flash. These
> modifications by qemu end up in the dcache mostly. When the guest is
> done "programming", it writes a special command (read array mode) at
> which point the memslot is recreated (as read-only) and flushed / set up
> for flushing during demand paging.
> 
> So from the emulated flash POV, the memslot either doesn't exist at all
> (and then qemu serves all accesses just fine), or it exists r/o, at
> which point qemu (host userspace) will have stopped writing to it, and
> will have set it up for flushing before and during guest read accesses.

I think beyond consistency, there should be no double mappings with
conflicting attributes at any time or CPU state is undefined. At least
that's what I recall for cases where identity mapping was cacheble and user
mmapp'ed regions uncacheable. Side effects like CPU hardstop or
victim invalidate of dirty cache line. With virtualization
extensions maybe behavior is different. I guess if you're not seeing
lock ups or crashes then it appears to work :) Probably more senior
folks in ARM community are in better position to address this,
but I thought I raise a flag.

> 
>> Also is if you update memory from qemu you may break
>> dirty page logging/migration.
> 
> Very probably. Jon said the same thing.
> 
>> Unless there is some other way
>> you keep track. Of course it may not be applicable in your
>> case (i.e. flash unused after boot).
> 
> The flash *is* used after boot, because the UEFI runtime variable
> services *are* exercised by the guest kernel. However those use the same
> access pattern (it's the same set of UEFI services just called by a
> different "client").
> 
> *Uncoordinated* access from guest and host in parallel will be a big
> problem; but we're not that far yet, and we need to get the flash
> problem sorted, so that we can at least boot and work on the basic
> stuff. The flash programming dance happens to provide coordination; the
> flash mode changes (which are equivalent to the teardown and the
> recreation of the memslot) can be considered barriers.
> 
> I hope this is acceptable for the time being...

Yeah I understand you have a more imediatte requirement to support,
migration
isssue is more fyi. Thanks for the details helps to understand the context.

- Mario
> 
> Thanks
> Laszlo
> 
>>
>> - Mario
>>
>> On 11/17/2014 07:49 AM, Laszlo Ersek wrote:
>>> On 11/17/14 16: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?
>>>
>>> Combining occurs on ARMv8 too. The Stage1 (guest) mapping is very strict
>>> (Device non-Gathering, non-Reordering, no Early Write Acknowledgement --
>>> for EFI_MEMORY_UC), which basically "overrides" the Stage2 (very lax
>>> host) memory attributes.
>>>
>>> When qemu writes, as part of emulating the flash programming commands,
>>> to the RAMBlock that *otherwise* backs the flash range (as a r/o
>>> memslot), those writes (from host userspace) tend to end up in dcache.
>>>
>>> But, when the guest flips back the flash to romd mode, and tries to read
>>> back the values from the flash as plain ROM, the dcache is completely
>>> bypassed due to the strict stage1 mapping, and the guest goes directly
>>> to DRAM.
>>>
>>> Where qemu's earlier writes are not yet / necessarily visible.
>>>
>>> Please see my original patch (which was incomplete) in the attachment,
>>> it has a very verbose commit message.
>>>
>>> Anyway, I'll let others explain; they can word it better than I can :)
>>>
>>> FWIW,
>>>
>>> Series
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> I ported this series to a 3.17.0+ based kernel, and tested it. It works
>>> fine. The ROM-like view of the NOR flash now reflects the previously
>>> programmed contents.
>>>
>>> Series
>>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>
>>>
>>> _______________________________________________
>>> kvmarm mailing list
>>> kvmarm@lists.cs.columbia.edu
>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>
>>
> 

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Peter Maydell]

On 20 November 2014 18:35, Mario Smarduch <m.smarduch@samsung.com> wrote:
> I think beyond consistency, there should be no double mappings with
> conflicting attributes at any time or CPU state is undefined.

The situation is not so bleak as this. See section B2.9 "Mismatched
memory attributes" in the ARMv8 ARM ARM (DDI0487A.d), which lays
out in some detail what the results of mismatched attributes are
(generally, you lose ordering or coherency guarantees you might
have hoped to have). They're not pretty, but it's not as bad
as completely UNPREDICTABLE behaviour.

thanks
-- PMM

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Mario Smarduch]

On 11/20/2014 10:40 AM, Peter Maydell wrote:
> On 20 November 2014 18:35, Mario Smarduch <m.smarduch@samsung.com> wrote:
>> I think beyond consistency, there should be no double mappings with
>> conflicting attributes at any time or CPU state is undefined.
> 
> The situation is not so bleak as this. See section B2.9 "Mismatched
> memory attributes" in the ARMv8 ARM ARM (DDI0487A.d), which lays
> out in some detail what the results of mismatched attributes are
> (generally, you lose ordering or coherency guarantees you might
> have hoped to have). They're not pretty, but it's not as bad
> as completely UNPREDICTABLE behaviour.
> 
> thanks
> -- PMM
> 
Hi Peter,
  thanks for digging that up, quite a list to navigate
but it does provide detailed guidance.

- Mario

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Jon Masters]

On 11/20/2014 01:40 PM, Peter Maydell wrote:
> On 20 November 2014 18:35, Mario Smarduch <m.smarduch@samsung.com> wrote:
>> I think beyond consistency, there should be no double mappings with
>> conflicting attributes at any time or CPU state is undefined.
> 
> The situation is not so bleak as this. See section B2.9 "Mismatched
> memory attributes" in the ARMv8 ARM ARM (DDI0487A.d), which lays
> out in some detail what the results of mismatched attributes are
> (generally, you lose ordering or coherency guarantees you might
> have hoped to have). They're not pretty, but it's not as bad
> as completely UNPREDICTABLE behaviour.

Quick side note that I did raise exactly this issue with the ARM
Architecture team several years ago (that of missmatched memory
attributes between a guest and hypervisor) and it is a known concern.
I'm personally concerned about a couple of things that I won't go into
here but will followup on what the longer term plan might be.

Jon.

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Peter Maydell]

On 20 November 2014 19:49, Jon Masters <jcm@redhat.com> wrote:
> On 11/20/2014 01:40 PM, Peter Maydell wrote:
>> The situation is not so bleak as this. See section B2.9 "Mismatched
>> memory attributes" in the ARMv8 ARM ARM (DDI0487A.d), which lays
>> out in some detail what the results of mismatched attributes are
>> (generally, you lose ordering or coherency guarantees you might
>> have hoped to have). They're not pretty, but it's not as bad
>> as completely UNPREDICTABLE behaviour.
>
> Quick side note that I did raise exactly this issue with the ARM
> Architecture team several years ago (that of missmatched memory
> attributes between a guest and hypervisor) and it is a known concern.

I think in practice for a well-behaved guest we can arrange
that everything is fine (roughly, the guest has to treat
DMA-capable devices as doing coherent-dma, which we can tell
them to do via DT bindings or ACPI[*], plus the special
case handling we already have for bootup), and naughty guests
will only confuse themselves. But I need to think a bit more
about it (and we should probably write down how it works
somewhere :-)).

[*] We should be able to emulate non-coherent-DMA devices but
would need an extra API from KVM so userspace can do "clean
dcache to point of coherency". And in practice I'm not sure
we need to emulate those devices...

-- PMM

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Laszlo Ersek]

On 11/20/14 21:10, Peter Maydell wrote:
> On 20 November 2014 19:49, Jon Masters <jcm@redhat.com> wrote:
>> On 11/20/2014 01:40 PM, Peter Maydell wrote:
>>> The situation is not so bleak as this. See section B2.9 "Mismatched
>>> memory attributes" in the ARMv8 ARM ARM (DDI0487A.d), which lays
>>> out in some detail what the results of mismatched attributes are
>>> (generally, you lose ordering or coherency guarantees you might
>>> have hoped to have). They're not pretty, but it's not as bad
>>> as completely UNPREDICTABLE behaviour.
>>
>> Quick side note that I did raise exactly this issue with the ARM
>> Architecture team several years ago (that of missmatched memory
>> attributes between a guest and hypervisor) and it is a known concern.
> 
> I think in practice for a well-behaved guest we can arrange
> that everything is fine (roughly, the guest has to treat
> DMA-capable devices as doing coherent-dma, which we can tell
> them to do via DT bindings or ACPI[*], plus the special
> case handling we already have for bootup), and naughty guests
> will only confuse themselves. But I need to think a bit more
> about it (and we should probably write down how it works
> somewhere :-)).
> 
> [*] We should be able to emulate non-coherent-DMA devices but
> would need an extra API from KVM so userspace can do "clean
> dcache to point of coherency". And in practice I'm not sure
> we need to emulate those devices...

This basically means that virtio transfers should just use normal memory
in the guest (treating virtio transfers as "coherent DMA"), right?

We're actually doing that in the ArmVirtualizationQemu.dsc build of
edk2, and Things Work Great (TM) in guests on the Mustang.

Thanks
Laszlo

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Peter Maydell]

On 20 November 2014 21:13, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/20/14 21:10, Peter Maydell wrote:
>> I think in practice for a well-behaved guest we can arrange
>> that everything is fine (roughly, the guest has to treat
>> DMA-capable devices as doing coherent-dma, which we can tell
>> them to do via DT bindings or ACPI[*], plus the special
>> case handling we already have for bootup), and naughty guests
>> will only confuse themselves. But I need to think a bit more
>> about it (and we should probably write down how it works
>> somewhere :-)).

> This basically means that virtio transfers should just use normal memory
> in the guest (treating virtio transfers as "coherent DMA"), right?

Normal *cacheable*, but yes.

-- PMM

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Christoffer Dall]

Hi Mario,

On Wed, Nov 19, 2014 at 03:32:31PM -0800, Mario Smarduch wrote:
> Hi Laszlo,
> 
> couple observations.
> 
>      I'm wondering if access from qemu and guest won't
> result in mixed memory attributes and if that's acceptable
> to the CPU.
> 
> Also is if you update memory from qemu you may break
> dirty page logging/migration. Unless there is some other way
> you keep track. Of course it may not be applicable in your
> case (i.e. flash unused after boot).
> 
I'm not concerned about this particular case; dirty page logging exists
so KVM can inform userspace when a page may have been dirtied.  If
userspace directly dirties (is that a verb?) a page, then it already
knows that it needs to migrate that page and deal with it accordingly.

Or did I miss some more subtle point here?

-Christoffer

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Mario Smarduch]

On 11/21/2014 03:19 AM, Christoffer Dall wrote:
> Hi Mario,
> 
> On Wed, Nov 19, 2014 at 03:32:31PM -0800, Mario Smarduch wrote:
>> Hi Laszlo,
>>
>> couple observations.
>>
>>      I'm wondering if access from qemu and guest won't
>> result in mixed memory attributes and if that's acceptable
>> to the CPU.
>>
>> Also is if you update memory from qemu you may break
>> dirty page logging/migration. Unless there is some other way
>> you keep track. Of course it may not be applicable in your
>> case (i.e. flash unused after boot).
>>
> I'm not concerned about this particular case; dirty page logging exists
> so KVM can inform userspace when a page may have been dirtied.  If
> userspace directly dirties (is that a verb?) a page, 
I would think so, I rely on software too much :)
> then it already knows that it needs to migrate that page and 
> deal with it accordingly.
> 
> Or did I miss some more subtle point here

QEMU has a global migration bitmap for all regions initially set
dirty, and it's updated over iterations with KVM's dirty bitmap. Once
dirty pages are migrated bits are cleared. If QEMU updates a
memory region directly I can't see how it's reflected in  that migration
bitmap that determines what pages should be migrated as it makes
it's passes. On x86 if host updates guest memory it marks that
page dirty.

But virtio writes to guest memory directly and that appears to
work just fine. I read that code sometime back, and will need to revisit.

- Mario

> 
> -Christoffer
> 

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Christoffer Dall]

On Fri, Nov 21, 2014 at 05:50:43PM -0800, Mario Smarduch wrote:
> On 11/21/2014 03:19 AM, Christoffer Dall wrote:
> > Hi Mario,
> > 
> > On Wed, Nov 19, 2014 at 03:32:31PM -0800, Mario Smarduch wrote:
> >> Hi Laszlo,
> >>
> >> couple observations.
> >>
> >>      I'm wondering if access from qemu and guest won't
> >> result in mixed memory attributes and if that's acceptable
> >> to the CPU.
> >>
> >> Also is if you update memory from qemu you may break
> >> dirty page logging/migration. Unless there is some other way
> >> you keep track. Of course it may not be applicable in your
> >> case (i.e. flash unused after boot).
> >>
> > I'm not concerned about this particular case; dirty page logging exists
> > so KVM can inform userspace when a page may have been dirtied.  If
> > userspace directly dirties (is that a verb?) a page, 
> I would think so, I rely on software too much :)
> > then it already knows that it needs to migrate that page and 
> > deal with it accordingly.
> > 
> > Or did I miss some more subtle point here
> 
> QEMU has a global migration bitmap for all regions initially set
> dirty, and it's updated over iterations with KVM's dirty bitmap. Once
> dirty pages are migrated bits are cleared. If QEMU updates a
> memory region directly I can't see how it's reflected in  that migration
> bitmap that determines what pages should be migrated as it makes
> it's passes. On x86 if host updates guest memory it marks that
> page dirty.
> 
> But virtio writes to guest memory directly and that appears to
> work just fine. I read that code sometime back, and will need to revisit.
> 
In any case, that's a QEMU implementation issue and nothing the kernel
needs to be concerned with.

-Christoffer

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Laszlo Ersek]

On 11/22/14 11:18, Christoffer Dall wrote:
> On Fri, Nov 21, 2014 at 05:50:43PM -0800, Mario Smarduch wrote:

>> But virtio writes to guest memory directly and that appears to
>> work just fine. I read that code sometime back, and will need to revisit.
>>
> In any case, that's a QEMU implementation issue and nothing the kernel
> needs to be concerned with.

(Let's call it "qemu implementation detail" -- there's no "issue". The
reason virtio works is not by incident, it has a known cause. As
confirmed by Peter, there's no problem with the virtio buffers because
the guest doesn't mark them as uncacheable, so *all* accesses go through
the dcache.)

Thanks
Laszlo

Re: [PATCH 3/3] arm, arm64: KVM: handle potential incoherency of readonly memslots

[Peter Maydell]

On 22 November 2014 at 01:50, Mario Smarduch <m.smarduch@samsung.com> wrote:
> QEMU has a global migration bitmap for all regions initially set
> dirty, and it's updated over iterations with KVM's dirty bitmap. Once
> dirty pages are migrated bits are cleared. If QEMU updates a
> memory region directly I can't see how it's reflected in  that migration
> bitmap that determines what pages should be migrated as it makes
> it's passes. On x86 if host updates guest memory it marks that
> page dirty.
>
> But virtio writes to guest memory directly and that appears to
> work just fine. I read that code sometime back, and will need to revisit.

All devices in QEMU that write to guest memory will do it via
a function in exec.c (possibly through wrapper functions) which
eventually will call invalidate_and_set_dirty(), which is what
is responsible for updating our dirty bitmaps. In the specific
case of virtio, the virtio device ends up calling virtqueue_fill(),
which does a cpu_physical_memory_unmap(), which just calls
address_space_unmap(), which will either directly call
invalidate_and_set_dirty() or, if using a bounce buffer, will
copy the bounce buffer to guest RAM with address_space_write(),
which calls address_space_rw(), which will do the
invalidate_and_set_dirty().

There's no cache incoherency issue for migration, because the
migration code is in the QEMU process and so it will read the
most recent thing QEMU wrote whether that data is still in the
dcache or has migrated out to real (host) RAM.

-- PMM