[PATCH v3 1/6] KVM: fix missing check for memslot flags

[PATCH v3 1/6] KVM: fix missing check for memslot flags

[Xiao Guangrong]

Check flags when memslot is registered from userspace as Avi's suggestion

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 virt/kvm/kvm_main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 02cb440..6a7756c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -686,6 +686,14 @@ void update_memslots(struct kvm_memslots *slots, struct kvm_memory_slot *new)
 	slots->generation++;
 }

+static int check_memory_region_flags(struct kvm_userspace_memory_region *mem)
+{
+	if (mem->flags & ~KVM_MEM_LOG_DIRTY_PAGES)
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * Allocate some memory and give it an address in the guest physical address
  * space.
@@ -706,6 +714,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	struct kvm_memory_slot old, new;
 	struct kvm_memslots *slots, *old_memslots;

+	r = check_memory_region_flags(mem);
+	if (r)
+		goto out;
+
 	r = -EINVAL;
 	/* General sanity checks */
 	if (mem->memory_size & (PAGE_SIZE - 1))
-- 
1.7.7.6

[PATCH v3 2/6] KVM: hide KVM_MEMSLOT_INVALID from userspace

[Xiao Guangrong]

Quote Avi's comment:
| KVM_MEMSLOT_INVALID is actually an internal symbol, not used by
| userspace.  Please move it to kvm_host.h.

Also, move KVM_MEMSLOT_INVALID to the highest bit

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 include/linux/kvm.h      |    1 -
 include/linux/kvm_host.h |    2 ++
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 2ce09aa..dc3aa2a 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -103,7 +103,6 @@ struct kvm_userspace_memory_region {

 /* for kvm_memory_region::flags */
 #define KVM_MEM_LOG_DIRTY_PAGES  1UL
-#define KVM_MEMSLOT_INVALID      (1UL << 1)

 /* for KVM_IRQ_LINE */
 struct kvm_irq_level {
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 27ac8a4..cf6bb58 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -34,6 +34,8 @@
 #define KVM_MMIO_SIZE 8
 #endif

+#define KVM_MEMSLOT_INVALID	(1UL << 31)
+
 /*
  * If we support unaligned MMIO, at most one fragment will be split into two:
  */
-- 
1.7.7.6

[PATCH v3 3/6] KVM: introduce gfn_to_pfn_memslot_atomic

[Xiao Guangrong]

It can instead of hva_to_pfn_atomic

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c       |    5 +----
 include/linux/kvm_host.h |    3 ++-
 virt/kvm/kvm_main.c      |   15 +++++++++------
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index bfdcaa0..9bd9d0a 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2444,7 +2444,6 @@ static pfn_t pte_prefetch_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn,
 				     bool no_dirty_log)
 {
 	struct kvm_memory_slot *slot;
-	unsigned long hva;

 	slot = gfn_to_memslot_dirty_bitmap(vcpu, gfn, no_dirty_log);
 	if (!slot) {
@@ -2452,9 +2451,7 @@ static pfn_t pte_prefetch_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn,
 		return page_to_pfn(fault_page);
 	}

-	hva = gfn_to_hva_memslot(slot, gfn);
-
-	return hva_to_pfn_atomic(vcpu->kvm, hva);
+	return gfn_to_pfn_memslot_atomic(vcpu->kvm, slot, gfn);
 }

 static int direct_pte_prefetch_many(struct kvm_vcpu *vcpu,
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index cf6bb58..62830cc 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -429,7 +429,6 @@ void kvm_release_page_dirty(struct page *page);
 void kvm_set_page_dirty(struct page *page);
 void kvm_set_page_accessed(struct page *page);

-pfn_t hva_to_pfn_atomic(struct kvm *kvm, unsigned long addr);
 pfn_t gfn_to_pfn_atomic(struct kvm *kvm, gfn_t gfn);
 pfn_t gfn_to_pfn_async(struct kvm *kvm, gfn_t gfn, bool *async,
 		       bool write_fault, bool *writable);
@@ -438,6 +437,8 @@ pfn_t gfn_to_pfn_prot(struct kvm *kvm, gfn_t gfn, bool write_fault,
 		      bool *writable);
 pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
 			 struct kvm_memory_slot *slot, gfn_t gfn);
+pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
+				struct kvm_memory_slot *slot, gfn_t gfn);
 void kvm_release_pfn_dirty(pfn_t);
 void kvm_release_pfn_clean(pfn_t pfn);
 void kvm_set_pfn_dirty(pfn_t pfn);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6a7756c..c59fad6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1158,12 +1158,6 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 	return pfn;
 }

-pfn_t hva_to_pfn_atomic(struct kvm *kvm, unsigned long addr)
-{
-	return hva_to_pfn(kvm, addr, true, NULL, true, NULL);
-}
-EXPORT_SYMBOL_GPL(hva_to_pfn_atomic);
-
 static pfn_t __gfn_to_pfn(struct kvm *kvm, gfn_t gfn, bool atomic, bool *async,
 			  bool write_fault, bool *writable)
 {
@@ -1214,6 +1208,15 @@ pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
 	return hva_to_pfn(kvm, addr, false, NULL, true, NULL);
 }

+pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
+				struct kvm_memory_slot *slot, gfn_t gfn)
+{
+	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
+
+	return hva_to_pfn(kvm, addr, true, NULL, true, NULL);
+}
+EXPORT_SYMBOL_GPL(gfn_to_pfn_memslot_atomic);
+
 int gfn_to_page_many_atomic(struct kvm *kvm, gfn_t gfn, struct page **pages,
 								  int nr_pages)
 {
-- 
1.7.7.6

[PATCH v3 4/6] KVM: pass slot to hva_to_pfn

[Xiao Guangrong]

This parameter will be used in the later patch

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 virt/kvm/kvm_main.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c59fad6..811c3ee 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1077,8 +1077,9 @@ static inline int check_user_page_hwpoison(unsigned long addr)
 	return rc == -EHWPOISON;
 }

-static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
-			bool *async, bool write_fault, bool *writable)
+static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
+			unsigned long addr, bool atomic, bool *async,
+			bool write_fault, bool *writable)
 {
 	struct page *page[1];
 	int npages = 0;
@@ -1161,18 +1162,22 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 static pfn_t __gfn_to_pfn(struct kvm *kvm, gfn_t gfn, bool atomic, bool *async,
 			  bool write_fault, bool *writable)
 {
+	struct kvm_memory_slot *slot;
 	unsigned long addr;

 	if (async)
 		*async = false;

-	addr = gfn_to_hva(kvm, gfn);
+	slot = gfn_to_memslot(kvm, gfn);
+	addr = gfn_to_hva_many(slot, gfn, NULL);
+
 	if (kvm_is_error_hva(addr)) {
 		get_page(bad_page);
 		return page_to_pfn(bad_page);
 	}

-	return hva_to_pfn(kvm, addr, atomic, async, write_fault, writable);
+	return hva_to_pfn(kvm, slot, addr, atomic, async, write_fault,
+			  writable);
 }

 pfn_t gfn_to_pfn_atomic(struct kvm *kvm, gfn_t gfn)
@@ -1205,7 +1210,7 @@ pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
 			 struct kvm_memory_slot *slot, gfn_t gfn)
 {
 	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
-	return hva_to_pfn(kvm, addr, false, NULL, true, NULL);
+	return hva_to_pfn(kvm, slot, addr, false, NULL, true, NULL);
 }

 pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
@@ -1213,7 +1218,7 @@ pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
 {
 	unsigned long addr = gfn_to_hva_memslot(slot, gfn);

-	return hva_to_pfn(kvm, addr, true, NULL, true, NULL);
+	return hva_to_pfn(kvm, slot, addr, true, NULL, true, NULL);
 }
 EXPORT_SYMBOL_GPL(gfn_to_pfn_memslot_atomic);

-- 
1.7.7.6

[PATCH v3 5/6] KVM: introduce gfn_to_hva_read/kvm_read_hva/kvm_read_hva_atomic

[Xiao Guangrong]

This set of functions is only used to read data from host space, read is
a special case in the later patch

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 virt/kvm/kvm_main.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 811c3ee..4b96bc2 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1051,6 +1051,27 @@ unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
 }
 EXPORT_SYMBOL_GPL(gfn_to_hva);

+/*
+ * The hva returned by this function is only allowed to be read.
+ * It should pair with kvm_read_hva() or kvm_read_hva_atomic().
+ */
+static unsigned long gfn_to_hva_read(struct kvm *kvm, gfn_t gfn)
+{
+	return gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL);
+}
+
+static int kvm_read_hva(void *data, void *hva, int len)
+{
+	return __copy_from_user(data, (void __user *)hva, len);
+
+}
+
+static int kvm_read_hva_atomic(void *data, void *hva, int len)
+{
+	return __copy_from_user_inatomic(data, (void __user *)hva, len);
+
+}
+
 static pfn_t get_fault_pfn(void)
 {
 	get_page(fault_page);
@@ -1325,10 +1346,10 @@ int kvm_read_guest_page(struct kvm *kvm, gfn_t gfn, void *data, int offset,
 	int r;
 	unsigned long addr;

-	addr = gfn_to_hva(kvm, gfn);
+	addr = gfn_to_hva_read(kvm, gfn);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
-	r = __copy_from_user(data, (void __user *)addr + offset, len);
+	r = kvm_read_hva(data, (void __user *)addr + offset, len);
 	if (r)
 		return -EFAULT;
 	return 0;
@@ -1363,11 +1384,11 @@ int kvm_read_guest_atomic(struct kvm *kvm, gpa_t gpa, void *data,
 	gfn_t gfn = gpa >> PAGE_SHIFT;
 	int offset = offset_in_page(gpa);

-	addr = gfn_to_hva(kvm, gfn);
+	addr = gfn_to_hva_read(kvm, gfn);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
 	pagefault_disable();
-	r = __copy_from_user_inatomic(data, (void __user *)addr + offset, len);
+	r = kvm_read_hva_atomic(data, (void __user *)addr + offset, len);
 	pagefault_enable();
 	if (r)
 		return -EFAULT;
-- 
1.7.7.6

[PATCH v3 6/6] KVM: introduce readonly memslot

[Xiao Guangrong]

In current code, if we map a readonly memory space from host to guest
and the page is not currently mapped in the host, we will get a fault-pfn
and async is not allowed, then the vm will crash

Address Avi's idea, we introduce readonly memory region to map ROM/ROMD
to the guest

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 Documentation/virtual/kvm/api.txt |    9 ++++--
 arch/x86/kvm/x86.c                |    1 +
 include/linux/kvm.h               |    4 ++-
 virt/kvm/kvm_main.c               |   61 ++++++++++++++++++++++++++++--------
 4 files changed, 57 insertions(+), 18 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 310fe50..a97ee90 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -857,7 +857,8 @@ struct kvm_userspace_memory_region {
 };

 /* for kvm_memory_region::flags */
-#define KVM_MEM_LOG_DIRTY_PAGES  1UL
+#define KVM_MEM_LOG_DIRTY_PAGES	(1UL << 0)
+#define KVM_MEM_READONLY	(1UL << 1)

 This ioctl allows the user to create or modify a guest physical memory
 slot.  When changing an existing slot, it may be moved in the guest
@@ -873,9 +874,11 @@ It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr
 be identical.  This allows large pages in the guest to be backed by large
 pages in the host.

-The flags field supports just one flag, KVM_MEM_LOG_DIRTY_PAGES, which
+The flags field supports two flag, KVM_MEM_LOG_DIRTY_PAGES, which
 instructs kvm to keep track of writes to memory within the slot.  See
-the KVM_GET_DIRTY_LOG ioctl.
+the KVM_GET_DIRTY_LOG ioctl. Another flag is KVM_MEM_READONLY, which
++indicates the guest memory is read-only, that means, guest is only allowed
++to read it. Writes will be posted to userspace as KVM_EXIT_MMIO exits.

 When the KVM_CAP_SYNC_MMU capability, changes in the backing of the memory
 region are automatically reflected into the guest.  For example, an mmap()
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a01a424..994f47b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2148,6 +2148,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_GET_TSC_KHZ:
 	case KVM_CAP_PCI_2_3:
 	case KVM_CAP_KVMCLOCK_CTRL:
+	case KVM_CAP_READONLY_MEM:
 		r = 1;
 		break;
 	case KVM_CAP_COALESCED_MMIO:
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index dc3aa2a..892d673 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -102,7 +102,8 @@ struct kvm_userspace_memory_region {
 };

 /* for kvm_memory_region::flags */
-#define KVM_MEM_LOG_DIRTY_PAGES  1UL
+#define KVM_MEM_LOG_DIRTY_PAGES	(1UL << 0)
+#define KVM_MEM_READONLY	(1UL << 1)

 /* for KVM_IRQ_LINE */
 struct kvm_irq_level {
@@ -617,6 +618,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_PPC_GET_SMMU_INFO 78
 #define KVM_CAP_S390_COW 79
 #define KVM_CAP_PPC_ALLOC_HTAB 80
+#define KVM_CAP_READONLY_MEM 81

 #ifdef KVM_CAP_IRQ_ROUTING

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4b96bc2..b551db1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -688,7 +688,7 @@ void update_memslots(struct kvm_memslots *slots, struct kvm_memory_slot *new)

 static int check_memory_region_flags(struct kvm_userspace_memory_region *mem)
 {
-	if (mem->flags & ~KVM_MEM_LOG_DIRTY_PAGES)
+	if (mem->flags & ~(KVM_MEM_LOG_DIRTY_PAGES | KVM_MEM_READONLY))
 		return -EINVAL;

 	return 0;
@@ -1033,10 +1033,11 @@ out:
 	return size;
 }

-static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
-				     gfn_t *nr_pages)
+static unsigned long __gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
+				     gfn_t *nr_pages, bool write)
 {
-	if (!slot || slot->flags & KVM_MEMSLOT_INVALID)
+	if (!slot || slot->flags & KVM_MEMSLOT_INVALID ||
+	      ((slot->flags & KVM_MEM_READONLY) && write))
 		return bad_hva();

 	if (nr_pages)
@@ -1045,6 +1046,12 @@ static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
 	return gfn_to_hva_memslot(slot, gfn);
 }

+static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
+				     gfn_t *nr_pages)
+{
+	return __gfn_to_hva_many(slot, gfn, nr_pages, true);
+}
+
 unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
 {
 	return gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL);
@@ -1057,7 +1064,7 @@ EXPORT_SYMBOL_GPL(gfn_to_hva);
  */
 static unsigned long gfn_to_hva_read(struct kvm *kvm, gfn_t gfn)
 {
-	return gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL);
+	return __gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL, false);
 }

 static int kvm_read_hva(void *data, void *hva, int len)
@@ -1098,6 +1105,34 @@ static inline int check_user_page_hwpoison(unsigned long addr)
 	return rc == -EHWPOISON;
 }

+static bool vma_is_valid(struct vm_area_struct *vma, bool write_fault)
+{
+	if (unlikely(!(vma->vm_flags & VM_READ)))
+		return false;
+
+	if (write_fault && (unlikely(!(vma->vm_flags & VM_WRITE))))
+		return false;
+
+	return true;
+}
+
+static int hva_to_pfn_fast(unsigned long addr, bool atomic, bool *async,
+			bool slot_writable, bool *writable, struct page **page)
+{
+	int npages = 0;
+
+	if (!slot_writable)
+		return 0;
+
+	if (writable)
+		*writable = true;
+
+	if (atomic || async)
+		npages = __get_user_pages_fast(addr, 1, 1, page);
+
+	return npages;
+}
+
 static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 			unsigned long addr, bool atomic, bool *async,
 			bool write_fault, bool *writable)
@@ -1105,18 +1140,16 @@ static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 	struct page *page[1];
 	int npages = 0;
 	pfn_t pfn;
+	bool slot_writable = !(slot->flags & KVM_MEM_READONLY);

 	/* we can do it either atomically or asynchronously, not both */
 	BUG_ON(atomic && async);

 	BUG_ON(!write_fault && !writable);
+	BUG_ON(write_fault && !slot_writable);

-	if (writable)
-		*writable = true;
-
-	if (atomic || async)
-		npages = __get_user_pages_fast(addr, 1, 1, page);
-
+	npages = hva_to_pfn_fast(addr, atomic, async, slot_writable,
+				 writable, page);
 	if (unlikely(npages != 1) && !atomic) {
 		might_sleep();

@@ -1133,7 +1166,7 @@ static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 						     page);

 		/* map read fault as writable if possible */
-		if (unlikely(!write_fault) && npages == 1) {
+		if (unlikely(!write_fault) && npages == 1 && slot_writable) {
 			struct page *wpage[1];

 			npages = __get_user_pages_fast(addr, 1, 1, wpage);
@@ -1169,7 +1202,7 @@ static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 				vma->vm_pgoff;
 			BUG_ON(!kvm_is_mmio_pfn(pfn));
 		} else {
-			if (async && (vma->vm_flags & VM_WRITE))
+			if (async && vma_is_valid(vma, write_fault))
 				*async = true;
 			pfn = get_fault_pfn();
 		}
@@ -1190,7 +1223,7 @@ static pfn_t __gfn_to_pfn(struct kvm *kvm, gfn_t gfn, bool atomic, bool *async,
 		*async = false;

 	slot = gfn_to_memslot(kvm, gfn);
-	addr = gfn_to_hva_many(slot, gfn, NULL);
+	addr = __gfn_to_hva_many(slot, gfn, NULL, write_fault);

 	if (kvm_is_error_hva(addr)) {
 		get_page(bad_page);
-- 
1.7.7.6

Re: [PATCH v3 6/6] KVM: introduce readonly memslot

[Marcelo Tosatti]

On Tue, Jun 12, 2012 at 10:49:31AM +0800, Xiao Guangrong wrote:
> In current code, if we map a readonly memory space from host to guest
> and the page is not currently mapped in the host, we will get a fault-pfn
> and async is not allowed, then the vm will crash
> 
> Address Avi's idea, we introduce readonly memory region to map ROM/ROMD
> to the guest

Please detail what is the idea in the changelog (a commit message should 
contain information or precise references to discussions).

> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  Documentation/virtual/kvm/api.txt |    9 ++++--
>  arch/x86/kvm/x86.c                |    1 +
>  include/linux/kvm.h               |    4 ++-
>  virt/kvm/kvm_main.c               |   61 ++++++++++++++++++++++++++++--------
>  4 files changed, 57 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 310fe50..a97ee90 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -857,7 +857,8 @@ struct kvm_userspace_memory_region {
>  };
> 
>  /* for kvm_memory_region::flags */
> -#define KVM_MEM_LOG_DIRTY_PAGES  1UL
> +#define KVM_MEM_LOG_DIRTY_PAGES	(1UL << 0)
> +#define KVM_MEM_READONLY	(1UL << 1)
> 
>  This ioctl allows the user to create or modify a guest physical memory
>  slot.  When changing an existing slot, it may be moved in the guest
> @@ -873,9 +874,11 @@ It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr
>  be identical.  This allows large pages in the guest to be backed by large
>  pages in the host.
> 
> -The flags field supports just one flag, KVM_MEM_LOG_DIRTY_PAGES, which
> +The flags field supports two flag, KVM_MEM_LOG_DIRTY_PAGES, which
>  instructs kvm to keep track of writes to memory within the slot.  See
> -the KVM_GET_DIRTY_LOG ioctl.
> +the KVM_GET_DIRTY_LOG ioctl. Another flag is KVM_MEM_READONLY, which
> ++indicates the guest memory is read-only, that means, guest is only allowed
> ++to read it. Writes will be posted to userspace as KVM_EXIT_MMIO exits.

Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with
information about the fault?

Then perform this exit only if userspace allows it by explicit enable, 
and by default have the exit_read_fault handler jump to the mmio
handler. 

> 
> +static bool vma_is_valid(struct vm_area_struct *vma, bool write_fault)
> +{
> +	if (unlikely(!(vma->vm_flags & VM_READ)))
> +		return false;
> +
> +	if (write_fault && (unlikely(!(vma->vm_flags & VM_WRITE))))
> +		return false;
> +
> +	return true;
> +}
> +
> +static int hva_to_pfn_fast(unsigned long addr, bool atomic, bool *async,
> +			bool slot_writable, bool *writable, struct page **page)
> +{
> +	int npages = 0;
> +
> +	if (!slot_writable)
> +		return 0;
> +
> +	if (writable)
> +		*writable = true;
> +
> +	if (atomic || async)
> +		npages = __get_user_pages_fast(addr, 1, 1, page);
> +
> +	return npages;
> +}
> +
>  static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>  			unsigned long addr, bool atomic, bool *async,
>  			bool write_fault, bool *writable)
> @@ -1105,18 +1140,16 @@ static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	struct page *page[1];
>  	int npages = 0;
>  	pfn_t pfn;
> +	bool slot_writable = !(slot->flags & KVM_MEM_READONLY);
> 
>  	/* we can do it either atomically or asynchronously, not both */
>  	BUG_ON(atomic && async);
> 
>  	BUG_ON(!write_fault && !writable);
> +	BUG_ON(write_fault && !slot_writable);

Why BUG_ON on this condition?

Re: [PATCH v3 6/6] KVM: introduce readonly memslot

[Xiao Guangrong]

On 06/16/2012 10:11 AM, Marcelo Tosatti wrote:

> On Tue, Jun 12, 2012 at 10:49:31AM +0800, Xiao Guangrong wrote:
>> In current code, if we map a readonly memory space from host to guest
>> and the page is not currently mapped in the host, we will get a fault-pfn
>> and async is not allowed, then the vm will crash
>>
>> Address Avi's idea, we introduce readonly memory region to map ROM/ROMD
>> to the guest
> 
> Please detail what is the idea in the changelog (a commit message should 
> contain information or precise references to discussions).
> 


Okay, sorry for my laze.


>> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>> ---
>>  Documentation/virtual/kvm/api.txt |    9 ++++--
>>  arch/x86/kvm/x86.c                |    1 +
>>  include/linux/kvm.h               |    4 ++-
>>  virt/kvm/kvm_main.c               |   61 ++++++++++++++++++++++++++++--------
>>  4 files changed, 57 insertions(+), 18 deletions(-)
>>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index 310fe50..a97ee90 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -857,7 +857,8 @@ struct kvm_userspace_memory_region {
>>  };
>>
>>  /* for kvm_memory_region::flags */
>> -#define KVM_MEM_LOG_DIRTY_PAGES  1UL
>> +#define KVM_MEM_LOG_DIRTY_PAGES	(1UL << 0)
>> +#define KVM_MEM_READONLY	(1UL << 1)
>>
>>  This ioctl allows the user to create or modify a guest physical memory
>>  slot.  When changing an existing slot, it may be moved in the guest
>> @@ -873,9 +874,11 @@ It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr
>>  be identical.  This allows large pages in the guest to be backed by large
>>  pages in the host.
>>
>> -The flags field supports just one flag, KVM_MEM_LOG_DIRTY_PAGES, which
>> +The flags field supports two flag, KVM_MEM_LOG_DIRTY_PAGES, which
>>  instructs kvm to keep track of writes to memory within the slot.  See
>> -the KVM_GET_DIRTY_LOG ioctl.
>> +the KVM_GET_DIRTY_LOG ioctl. Another flag is KVM_MEM_READONLY, which
>> ++indicates the guest memory is read-only, that means, guest is only allowed
>> ++to read it. Writes will be posted to userspace as KVM_EXIT_MMIO exits.
> 
> Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with
> information about the fault?
> 


KVM_EXIT_READ_FAULT should be KVM_EXIT_WRITE_FAULT :)

The exit info is mostly the same as mmio-exit except this exit does not need
is_write.

> Then perform this exit only if userspace allows it by explicit enable, 
> and by default have the exit_read_fault handler jump to the mmio
> handler. 
> 


No object to do this,  but do we need do it in kernel? The userspace can recognise
this fault: if it is a write-mmio-exit and the memslot is readonly.

>> +static bool vma_is_valid(struct vm_area_struct *vma, bool write_fault)
>> +{
>> +	if (unlikely(!(vma->vm_flags & VM_READ)))
>> +		return false;
>> +
>> +	if (write_fault && (unlikely(!(vma->vm_flags & VM_WRITE))))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static int hva_to_pfn_fast(unsigned long addr, bool atomic, bool *async,
>> +			bool slot_writable, bool *writable, struct page **page)
>> +{
>> +	int npages = 0;
>> +
>> +	if (!slot_writable)
>> +		return 0;
>> +
>> +	if (writable)
>> +		*writable = true;
>> +
>> +	if (atomic || async)
>> +		npages = __get_user_pages_fast(addr, 1, 1, page);
>> +
>> +	return npages;
>> +}
>> +
>>  static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>>  			unsigned long addr, bool atomic, bool *async,
>>  			bool write_fault, bool *writable)
>> @@ -1105,18 +1140,16 @@ static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>>  	struct page *page[1];
>>  	int npages = 0;
>>  	pfn_t pfn;
>> +	bool slot_writable = !(slot->flags & KVM_MEM_READONLY);
>>
>>  	/* we can do it either atomically or asynchronously, not both */
>>  	BUG_ON(atomic && async);
>>
>>  	BUG_ON(!write_fault && !writable);
>> +	BUG_ON(write_fault && !slot_writable);
> 
> Why BUG_ON on this condition?
> 


The 'slot' has already been checked in gfn_to_hva_*() functions:

+	if (!slot || slot->flags & KVM_MEMSLOT_INVALID ||
+	      ((slot->flags & KVM_MEM_READONLY) && write))
 		return bad_hva();

So, it is a bug if we get a writable pfn from a readonly slot which
is got from __gfn_to_hva_many(..., write=false).

 

Re: [PATCH v3 6/6] KVM: introduce readonly memslot

[Avi Kivity]

On 06/16/2012 05:11 AM, Marcelo Tosatti wrote:
> 
> Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with
> information about the fault?

I think you mean WRITE_FAULT.  But what's wrong with the normal mmio exit?

> 
> Then perform this exit only if userspace allows it by explicit enable, 
> and by default have the exit_read_fault handler jump to the mmio
> handler. 


I don't get this.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v3 6/6] KVM: introduce readonly memslot

[Avi Kivity]

On 06/12/2012 05:49 AM, Xiao Guangrong wrote:
> In current code, if we map a readonly memory space from host to guest
> and the page is not currently mapped in the host, we will get a fault-pfn
> and async is not allowed, then the vm will crash
> 
> Address Avi's idea, we introduce readonly memory region to map ROM/ROMD
> to the guest
> 
> index 4b96bc2..b551db1 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -688,7 +688,7 @@ void update_memslots(struct kvm_memslots *slots, struct kvm_memory_slot *new)
> 
>  static int check_memory_region_flags(struct kvm_userspace_memory_region *mem)
>  {
> -	if (mem->flags & ~KVM_MEM_LOG_DIRTY_PAGES)
> +	if (mem->flags & ~(KVM_MEM_LOG_DIRTY_PAGES | KVM_MEM_READONLY))
>  		return -EINVAL;

Only x86 supports readonly so far.

> 
> -static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
> -				     gfn_t *nr_pages)
> +static unsigned long __gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
> +				     gfn_t *nr_pages, bool write)
>  {
> -	if (!slot || slot->flags & KVM_MEMSLOT_INVALID)
> +	if (!slot || slot->flags & KVM_MEMSLOT_INVALID ||
> +	      ((slot->flags & KVM_MEM_READONLY) && write))
>  		return bad_hva();
> 
>  	if (nr_pages)
> @@ -1045,6 +1046,12 @@ static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
>  	return gfn_to_hva_memslot(slot, gfn);
>  }
> 
> +static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
> +				     gfn_t *nr_pages)
> +{
> +	return __gfn_to_hva_many(slot, gfn, nr_pages, true);
> +}

We have dozens of translation functions: read/write guest virtual, guest
physical (nested and non nested), host virtual, host physical,
atomic/nonatomic, sync/async, with/without slot lookup, and probably a
few more I forgot.

I think we should refactor this into a series of on-step translations:

   /*
    * Translate gva/len write access to a number of tlb entries
    * (due to cross-page splits) or a fault
    */
   gva_to_tlb(gva, len, ACCESS_WRITE, &translation);
   /*
    * Translate tlb entries to callbacks that do I/O (either directly
    * or through KVM_EXIT_MMIO, provided there is no exception pending
    */
   tlb_to_io(&translation, &iolist, IO_ATOMIC);
   /*
    * Initiate I/O (if no exception)
    */
   run_iolist(&iolist, data);

   struct gpa_scatterlist {
       unsigned nr_entries;
       struct {
           gpa_t gpa;
           unsigned len;
       } entry[2];
       struct x86_exception exception;
   };

   struct kvm_iolist {
       unsigned nr_entries;
       struct kvm_ioentry {
           struct kvm_memslot *slot;  /* NULL for mmio */
           struct something *kernel_iodevice;
           gfn_t page_in_slot;
           unsigned offset_in_page;
           unsigned len;
           void (*iofunc)(struct kvm_ioentry *entry, void *data);
       } entry[2];
       struct x86_exception execption;
   };

This is of course outside the scope of this patchset, just something to
think about (and write opinions on).



-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v3 4/6] KVM: pass slot to hva_to_pfn

[Avi Kivity]

On 06/12/2012 05:48 AM, Xiao Guangrong wrote:
> This parameter will be used in the later patch
> 
> 
> -	return hva_to_pfn(kvm, addr, atomic, async, write_fault, writable);
> +	return hva_to_pfn(kvm, slot, addr, atomic, async, write_fault,
> +			  writable);
>  }
> 
>  pfn_t gfn_to_pfn_atomic(struct kvm *kvm, gfn_t gfn)
> @@ -1205,7 +1210,7 @@ pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
>  			 struct kvm_memory_slot *slot, gfn_t gfn)
>  {
>  	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
> -	return hva_to_pfn(kvm, addr, false, NULL, true, NULL);
> +	return hva_to_pfn(kvm, slot, addr, false, NULL, true, NULL);
>  }
> 
>  pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
> @@ -1213,7 +1218,7 @@ pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
>  {
>  	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
> 
> -	return hva_to_pfn(kvm, addr, true, NULL, true, NULL);
> +	return hva_to_pfn(kvm, slot, addr, true, NULL, true, NULL);
>  }

It was unreadable before, but now it's even more unreadable.



-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v3 5/6] KVM: introduce gfn_to_hva_read/kvm_read_hva/kvm_read_hva_atomic

[Avi Kivity]

On 06/12/2012 05:48 AM, Xiao Guangrong wrote:
> This set of functions is only used to read data from host space, read is
> a special case in the later patch
> 
> 
> +/*
> + * The hva returned by this function is only allowed to be read.
> + * It should pair with kvm_read_hva() or kvm_read_hva_atomic().
> + */
> +static unsigned long gfn_to_hva_read(struct kvm *kvm, gfn_t gfn)
> +{
> +	return gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL);
> +}
> +
> +static int kvm_read_hva(void *data, void *hva, int len)
> +{
> +	return __copy_from_user(data, (void __user *)hva, len);
> +
> +}
> +
> +static int kvm_read_hva_atomic(void *data, void *hva, int len)
> +{
> +	return __copy_from_user_inatomic(data, (void __user *)hva, len);
> +
> +}

Why cast to __user?  Make it __user in the first place.

Also these are just simple wrappers, why to them at all?



-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v3 6/6] KVM: introduce readonly memslot

[Marcelo Tosatti]

On Mon, Jun 18, 2012 at 12:50:10PM +0300, Avi Kivity wrote:
> On 06/16/2012 05:11 AM, Marcelo Tosatti wrote:
> > 
> > Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with
> > information about the fault?
> 
> I think you mean WRITE_FAULT.  

Yes.

> But what's wrong with the normal mmio exit?

It is necessary to perform an address->mmio region lookup, to verify
whether the mmio exit is due to an actual mmio (no memory slot) or from
a write access to a write protected slot. That information is readily
available in the kernel but is lost if the mmio exit is used to transmit 
the information.

Moreover, i'd argue the uses are different: one is an mmio emulation
exit, the other is more like handling a pagefault in qemu.

> > Then perform this exit only if userspace allows it by explicit enable, 
> > and by default have the exit_read_fault handler jump to the mmio
> > handler. 
> 
> 
> I don't get this.


CAN USERSPACE HANDLE WRITE FAULT EXITS?
YES: WRITE FAULT EXIT.
NO: MMIO EXIT.

But then again userspace won't set read-only slots if it does not know
about them. So it is not necessary.

Re: [PATCH v3 6/6] KVM: introduce readonly memslot

[Xiao Guangrong]

On 06/18/2012 06:11 PM, Avi Kivity wrote:


>>  static int check_memory_region_flags(struct kvm_userspace_memory_region *mem)
>>  {
>> -	if (mem->flags & ~KVM_MEM_LOG_DIRTY_PAGES)
>> +	if (mem->flags & ~(KVM_MEM_LOG_DIRTY_PAGES | KVM_MEM_READONLY))
>>  		return -EINVAL;
> 
> Only x86 supports readonly so far.
> 


Right, will fix.

>>
>> -static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
>> -				     gfn_t *nr_pages)
>> +static unsigned long __gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
>> +				     gfn_t *nr_pages, bool write)
>>  {
>> -	if (!slot || slot->flags & KVM_MEMSLOT_INVALID)
>> +	if (!slot || slot->flags & KVM_MEMSLOT_INVALID ||
>> +	      ((slot->flags & KVM_MEM_READONLY) && write))
>>  		return bad_hva();
>>
>>  	if (nr_pages)
>> @@ -1045,6 +1046,12 @@ static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
>>  	return gfn_to_hva_memslot(slot, gfn);
>>  }
>>
>> +static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
>> +				     gfn_t *nr_pages)
>> +{
>> +	return __gfn_to_hva_many(slot, gfn, nr_pages, true);
>> +}
> 
> We have dozens of translation functions: read/write guest virtual, guest
> physical (nested and non nested), host virtual, host physical,
> atomic/nonatomic, sync/async, with/without slot lookup, and probably a
> few more I forgot.
> 


Yes, they make me confused me too.

> I think we should refactor this into a series of on-step translations:
> 
>    /*
>     * Translate gva/len write access to a number of tlb entries
>     * (due to cross-page splits) or a fault
>     */
>    gva_to_tlb(gva, len, ACCESS_WRITE, &translation);
>    /*
>     * Translate tlb entries to callbacks that do I/O (either directly
>     * or through KVM_EXIT_MMIO, provided there is no exception pending
>     */
>    tlb_to_io(&translation, &iolist, IO_ATOMIC);
>    /*
>     * Initiate I/O (if no exception)
>     */
>    run_iolist(&iolist, data);
> 
>    struct gpa_scatterlist {
>        unsigned nr_entries;
>        struct {
>            gpa_t gpa;
>            unsigned len;
>        } entry[2];
>        struct x86_exception exception;
>    };
> 
>    struct kvm_iolist {
>        unsigned nr_entries;
>        struct kvm_ioentry {
>            struct kvm_memslot *slot;  /* NULL for mmio */
>            struct something *kernel_iodevice;
>            gfn_t page_in_slot;
>            unsigned offset_in_page;
>            unsigned len;
>            void (*iofunc)(struct kvm_ioentry *entry, void *data);
>        } entry[2];
>        struct x86_exception execption;
>    };
> 
> This is of course outside the scope of this patchset, just something to
> think about (and write opinions on).


I will think it more and try to do it based on you idea after this patchset,
thank you, Avi!

Re: [PATCH v3 4/6] KVM: pass slot to hva_to_pfn

[Xiao Guangrong]

On 06/18/2012 06:15 PM, Avi Kivity wrote:

> On 06/12/2012 05:48 AM, Xiao Guangrong wrote:
>> This parameter will be used in the later patch
>>
>>
>> -	return hva_to_pfn(kvm, addr, atomic, async, write_fault, writable);
>> +	return hva_to_pfn(kvm, slot, addr, atomic, async, write_fault,
>> +			  writable);
>>  }
>>
>>  pfn_t gfn_to_pfn_atomic(struct kvm *kvm, gfn_t gfn)
>> @@ -1205,7 +1210,7 @@ pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
>>  			 struct kvm_memory_slot *slot, gfn_t gfn)
>>  {
>>  	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
>> -	return hva_to_pfn(kvm, addr, false, NULL, true, NULL);
>> +	return hva_to_pfn(kvm, slot, addr, false, NULL, true, NULL);
>>  }
>>
>>  pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
>> @@ -1213,7 +1218,7 @@ pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
>>  {
>>  	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
>>
>> -	return hva_to_pfn(kvm, addr, true, NULL, true, NULL);
>> +	return hva_to_pfn(kvm, slot, addr, true, NULL, true, NULL);
>>  }
> 
> It was unreadable before, but now it's even more unreadable.


Hmm, i will simplify hva_to_pfn() in the next version.

Re: [PATCH v3 5/6] KVM: introduce gfn_to_hva_read/kvm_read_hva/kvm_read_hva_atomic

[Xiao Guangrong]

On 06/18/2012 06:16 PM, Avi Kivity wrote:

> On 06/12/2012 05:48 AM, Xiao Guangrong wrote:
>> This set of functions is only used to read data from host space, read is
>> a special case in the later patch
>>
>>
>> +/*
>> + * The hva returned by this function is only allowed to be read.
>> + * It should pair with kvm_read_hva() or kvm_read_hva_atomic().
>> + */
>> +static unsigned long gfn_to_hva_read(struct kvm *kvm, gfn_t gfn)
>> +{
>> +	return gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL);
>> +}
>> +
>> +static int kvm_read_hva(void *data, void *hva, int len)
>> +{
>> +	return __copy_from_user(data, (void __user *)hva, len);
>> +
>> +}
>> +
>> +static int kvm_read_hva_atomic(void *data, void *hva, int len)
>> +{
>> +	return __copy_from_user_inatomic(data, (void __user *)hva, len);
>> +
>> +}
> 
> Why cast to __user?  Make it __user in the first place.
> 


Okay.

> Also these are just simple wrappers, why to them at all?


For gfn_to_hva_read(), they will call gfn_to_hva_many(...write = false)
in the later patch.

For kvm_read_hva() and kvm_read_hva_atomic(), they are pure wrappers,
i think its name is a good hint to let gfn_to_hva_read to pair with
kvm_read_hva()/kvm_read_hva_atomic().

Re: [PATCH v3 6/6] KVM: introduce readonly memslot

[Gleb Natapov]

On Mon, Jun 18, 2012 at 05:25:05PM -0300, Marcelo Tosatti wrote:
> On Mon, Jun 18, 2012 at 12:50:10PM +0300, Avi Kivity wrote:
> > On 06/16/2012 05:11 AM, Marcelo Tosatti wrote:
> > > 
> > > Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with
> > > information about the fault?
> > 
> > I think you mean WRITE_FAULT.  
> 
> Yes.
> 
> > But what's wrong with the normal mmio exit?
> 
> It is necessary to perform an address->mmio region lookup, to verify
> whether the mmio exit is due to an actual mmio (no memory slot) or from
> a write access to a write protected slot. That information is readily
> available in the kernel but is lost if the mmio exit is used to transmit 
> the information.
> 
Why is it necessary though? Write access to a write protected slot is
MMIO by (our) definition.

> Moreover, i'd argue the uses are different: one is an mmio emulation
> exit, the other is more like handling a pagefault in qemu.
> 
What do you mean by "handling a pagefault in qemu"?

--
			Gleb.

Re: [PATCH v3 6/6] KVM: introduce readonly memslot

[Avi Kivity]

On 06/18/2012 11:25 PM, Marcelo Tosatti wrote:
> On Mon, Jun 18, 2012 at 12:50:10PM +0300, Avi Kivity wrote:
>> On 06/16/2012 05:11 AM, Marcelo Tosatti wrote:
>> > 
>> > Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with
>> > information about the fault?
>> 
>> I think you mean WRITE_FAULT.  
> 
> Yes.
> 
>> But what's wrong with the normal mmio exit?
> 
> It is necessary to perform an address->mmio region lookup, to verify
> whether the mmio exit is due to an actual mmio (no memory slot) or from
> a write access to a write protected slot. That information is readily
> available in the kernel but is lost if the mmio exit is used to transmit 
> the information.

For qemu it doesn't matter, but other userspaces might need it.  Can we
add it to the mmio exit reason as extra data?  Only present if the CAP
is available.

> Moreover, i'd argue the uses are different: one is an mmio emulation
> exit, the other is more like handling a pagefault in qemu.

It is not.  A pagefault is handled by fixing and retrying (fault).  MMIO
emulation is done by userspace instead of the kernel or guest (trap).
Here we're not fixing anything (say by marking the page writeable; the
slot is readonly, not the page (as is the case with ksm).

Qemu's ROM and ROMD behaviour follow this (discard access for ROM,
emualte for ROMD).

There might be value in implementing user visible write protection, for
example to implement distributed shared memory or postcopy migration.
But as long as per page protection requires separate VMAs it is not
practical (and is unlikely to perform well anyway).

> 
>> > Then perform this exit only if userspace allows it by explicit enable, 
>> > and by default have the exit_read_fault handler jump to the mmio
>> > handler. 
>> 
>> 
>> I don't get this.
> 
> 
> CAN USERSPACE HANDLE WRITE FAULT EXITS?
> YES: WRITE FAULT EXIT.
> NO: MMIO EXIT.
> 
> But then again userspace won't set read-only slots if it does not know
> about them. So it is not necessary.
> 

Okay, okay, don't shout.

-- 
error compiling committee.c: too many arguments to function