KVM: introduce readonly memory region

PATCH In review Xiao Guangrong 3 participants 7 messages May 24, 2012 – May 28, 2012

Subsystems (2)

Documentation KVM

lore

Share & tools

lore

Xiao Guangrong

author

2012-05-24 09:24 UTC #0 lore raw

PATCH v2 1/1 KVM: introduce readonly memory region

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 <[email protected]>

---
 Documentation/virtual/kvm/api.txt |    9 +++++--
 include/linux/kvm.h               |    5 ++-
 virt/kvm/kvm_main.c               |   43 ++++++++++++++++++++++++++++++-------
 3 files changed, 44 insertions(+), 13 deletions(-)

Documentation/virtual/kvm/api.txt +6 -3

index 9301266..e2a82c3 100644
@@ -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
+#define KVM_MEM_READ_ONLY		(1UL << 2)
 
 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 flags, 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_READ_ONLY, which
+indicates the guest memory is read-only, that means, guest is only allowed
+to read it.
 
 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()

include/linux/kvm.h +3 -2

index 09f2b3a..d178e3d 100644
@@ -102,8 +102,9 @@ struct kvm_userspace_memory_region {
 };
 
 /* for kvm_memory_region::flags */
-#define KVM_MEM_LOG_DIRTY_PAGES  1UL
-#define KVM_MEMSLOT_INVALID      (1UL << 1)
+#define KVM_MEM_LOG_DIRTY_PAGES		1UL
+#define KVM_MEMSLOT_INVALID		(1UL << 1)
+#define KVM_MEM_READ_ONLY		(1UL << 2)
 
 /* for KVM_IRQ_LINE */
 struct kvm_irq_level {

virt/kvm/kvm_main.c +35 -8

93 lines not on this page · raw message

Signature · 1 line

1.7.7.6

Gleb Natapov

2012-05-24 09:59 UTC #1 lore raw

Quoted 7 lines · 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

As far as I can tell this implements only ROMD. i.e write access to read only slot will generate IO exit.

Quoted 164 lines

Signed-off-by Xiao Guangrong <[email protected]>

---
 Documentation/virtual/kvm/api.txt |    9 +++++--
 include/linux/kvm.h               |    5 ++-
 virt/kvm/kvm_main.c               |   43 ++++++++++++++++++++++++++++++-------
 3 files changed, 44 insertions(+), 13 deletions(-)

Documentation/virtual/kvm/api.txt +6 -3

index 9301266..e2a82c3 100644
@@ -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
+#define KVM_MEM_READ_ONLY		(1UL << 2)
 
 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 flags, 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_READ_ONLY, which
+indicates the guest memory is read-only, that means, guest is only allowed
+to read it.
 
 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()

include/linux/kvm.h +3 -2

index 09f2b3a..d178e3d 100644
@@ -102,8 +102,9 @@ struct kvm_userspace_memory_region {
 };
 
 /* for kvm_memory_region::flags */
-#define KVM_MEM_LOG_DIRTY_PAGES  1UL
-#define KVM_MEMSLOT_INVALID      (1UL << 1)
+#define KVM_MEM_LOG_DIRTY_PAGES		1UL
+#define KVM_MEMSLOT_INVALID		(1UL << 1)
+#define KVM_MEM_READ_ONLY		(1UL << 2)
 
 /* for KVM_IRQ_LINE */
 struct kvm_irq_level {

virt/kvm/kvm_main.c +35 -8

index 7e14068..27283e4 100644
@@ -1009,10 +1009,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_READ_ONLY) && write))
 		return bad_hva();
 
 	if (nr_pages)

@@ -1021,6 +1022,17 @@ 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_prot(struct kvm *kvm, gfn_t gfn, bool write)
+{
+	return __gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL, write);
+}
+
 unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
 {
 	return gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL);

@@ -1053,6 +1065,21 @@ static inline int check_user_page_hwpoison(unsigned long addr)
 	return rc == -EHWPOISON;
 }
 
+static bool vma_is_avalid(struct vm_area_struct *vma, bool write_fault)
+{
+	if (write_fault) {
+		if (unlikely(!(vma->vm_flags & VM_WRITE)))
+			return false;
+
+		return true;
+	}
+
+	if (unlikely(!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE))))
+		return false;
+
+	return true;
+}
+
 static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 			bool *async, bool write_fault, bool *writable)
 {

@@ -1076,7 +1103,6 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 
 		if (writable)
 			*writable = write_fault;
-
 		if (async) {
 			down_read(&current->mm->mmap_sem);
 			npages = get_user_page_nowait(current, current->mm,

@@ -1123,8 +1149,9 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 				vma->vm_pgoff;
 			BUG_ON(!kvm_is_mmio_pfn(pfn));
 		} else {
-			if (async && (vma->vm_flags & VM_WRITE))
+			if (async && vma_is_avalid(vma, write_fault))
 				*async = true;
+
 			pfn = get_fault_pfn();
 		}
 		up_read(&current->mm->mmap_sem);

@@ -1148,7 +1175,7 @@ static pfn_t __gfn_to_pfn(struct kvm *kvm, gfn_t gfn, bool atomic, bool *async,
 	if (async)
 		*async = false;
 
-	addr = gfn_to_hva(kvm, gfn);
+	addr = gfn_to_hva_prot(kvm, gfn, write_fault);
 	if (kvm_is_error_hva(addr)) {
 		get_page(bad_page);
 		return page_to_pfn(bad_page);

@@ -1293,7 +1320,7 @@ 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_prot(kvm, gfn, false);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
 	r = __copy_from_user(data, (void __user *)addr + offset, len);

@@ -1331,7 +1358,7 @@ 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_prot(kvm, gfn, false);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
 	pagefault_disable();

Signature · 7 lines

1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

--
Gleb.

Avi Kivity

2012-05-24 10:33 UTC #2 lore raw

Quoted 10 lines · Gleb Natapov

Quoted 7 lines · 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
As far as I can tell this implements only ROMD. i.e write access to read only slot will generate IO exit.

Which userspace can then ignore. The question is whether writes to ROM are frequent, and whether the performance in that case matters.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2012-05-24 12:10 UTC #3 lore raw

In reply to Xiao Guangrong

Quoted 19 lines · 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 <[email protected]>
---
 Documentation/virtual/kvm/api.txt |    9 +++++--
 include/linux/kvm.h               |    5 ++-
 virt/kvm/kvm_main.c               |   43 ++++++++++++++++++++++++++++++-------
 3 files changed, 44 insertions(+), 13 deletions(-)
Documentation/virtual/kvm/api.txt +0 -0
index 9301266..e2a82c3 100644

Documentation/virtual/kvm/api.txt struct kvm_userspace_memory_region { · line 861

Quoted 6 lines

Documentation/virtual/kvm/api.txt +2 -1

@@ -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
+#define KVM_MEM_READ_ONLY		(1UL << 2)

Bit 1 should be fine too, see below.

Quoted 4 lines

Documentation/virtual/kvm/api.txt +0 -0

 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

Documentation/virtual/kvm/api.txt It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr · line 881

Quoted 10 lines

Documentation/virtual/kvm/api.txt +4 -2

@@ -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 flags, 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_READ_ONLY, 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.

Documentation/virtual/kvm/api.txt It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr

Quoted 7 lines

Documentation/virtual/kvm/api.txt +3 -2

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

KVM_MEMSLOT_INVALID is actually an internal symbol, not used by userspace. Please move it to kvm_host.h.

I see that we don't check flags for validity. Please add a check that we don't use undefined flags and return -EINVAL. Should be a separate patch since we may want to backport it.

We need a KVM_CAP_ so userspace knows it can use the feature. Only x86 should respond to it now, until (or if) other archs are updated.

Quoted 2 lines

+static bool vma_is_avalid(struct vm_area_struct *vma, bool write_fault)

s/avalid/valid/.

Documentation/virtual/kvm/api.txt

Quoted 11 lines

Documentation/virtual/kvm/api.txt +11 -0

+{
+	if (write_fault) {
+		if (unlikely(!(vma->vm_flags & VM_WRITE)))
+			return false;
+
+		return true;
+	}
+
+	if (unlikely(!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE))))
+		return false;
+

Strange check. VM_EXEC doesn't concern us at all. Maybe we should check for VM_READ always, and VM_WRITE for write faults.

Quoted 15 lines

Documentation/virtual/kvm/api.txt +3 -1

+	return true;
+}
+
 static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 			bool *async, bool write_fault, bool *writable)
 {

@@ -1076,7 +1103,6 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 
 		if (writable)
 			*writable = write_fault;
-
 		if (async) {
 			down_read(&current->mm->mmap_sem);
 			npages = get_user_page_nowait(current, current->mm,

Documentation/virtual/kvm/api.txt static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic, · line 1154

Quoted 7 lines

Documentation/virtual/kvm/api.txt +2 -1

@@ -1123,8 +1149,9 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 				vma->vm_pgoff;
 			BUG_ON(!kvm_is_mmio_pfn(pfn));
 		} else {
-			if (async && (vma->vm_flags & VM_WRITE))
+			if (async && vma_is_avalid(vma, write_fault))
 				*async = true;
+

This checks based on the fault type, not memslot type. So we have the risk of the pfn later used for writes?

Quoted 22 lines

Documentation/virtual/kvm/api.txt +2 -2

 			pfn = get_fault_pfn();
 		}
 		up_read(&current->mm->mmap_sem);

@@ -1148,7 +1175,7 @@ static pfn_t __gfn_to_pfn(struct kvm *kvm, gfn_t gfn, bool atomic, bool *async,
 	if (async)
 		*async = false;
 
-	addr = gfn_to_hva(kvm, gfn);
+	addr = gfn_to_hva_prot(kvm, gfn, write_fault);
 	if (kvm_is_error_hva(addr)) {
 		get_page(bad_page);
 		return page_to_pfn(bad_page);

@@ -1293,7 +1320,7 @@ 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_prot(kvm, gfn, false);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
 	r = __copy_from_user(data, (void __user *)addr + offset, len);

Documentation/virtual/kvm/api.txt int kvm_read_guest_atomic(struct kvm *kvm, gpa_t gpa, void *data, · line 1364

Quoted 8 lines

Documentation/virtual/kvm/api.txt +1 -1

@@ -1331,7 +1358,7 @@ 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_prot(kvm, gfn, false);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
 	pagefault_disable();

Surprised only those places.

How do we make sure a pfn obtained with write = false isn't later used for writing?

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2012-05-25 08:47 UTC #4 lore raw

Quoted 6 lines · Avi Kivity

Documentation/virtual/kvm/api.txt struct kvm_userspace_memory_region { · line 861
Quoted 4 lines
/* for kvm_memory_region::flags */
Documentation/virtual/kvm/api.txt +2 -1
-#define KVM_MEM_LOG_DIRTY_PAGES  1UL
+#define KVM_MEM_LOG_DIRTY_PAGES		1UL
+#define KVM_MEM_READ_ONLY		(1UL << 2)
Bit 1 should be fine too, see below.

Okay.

Quoted 8 lines

Quoted 8 lines
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
Documentation/virtual/kvm/api.txt +0 -0
@@ -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.
 

Quoted 9 lines

Documentation/virtual/kvm/api.txt It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr · line 881
Quoted 7 lines
Documentation/virtual/kvm/api.txt +4 -2
-The flags field supports just one flag, KVM_MEM_LOG_DIRTY_PAGES, which
+The flags field supports two flags, 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_READ_ONLY, 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.

Okay.

Quoted 12 lines

include/linux/kvm.h struct kvm_userspace_memory_region { · line 107
Quoted 8 lines
include/linux/kvm.h +3 -2
 /* for kvm_memory_region::flags */
-#define KVM_MEM_LOG_DIRTY_PAGES  1UL
-#define KVM_MEMSLOT_INVALID      (1UL << 1)
+#define KVM_MEM_LOG_DIRTY_PAGES		1UL
+#define KVM_MEMSLOT_INVALID		(1UL << 1)
+#define KVM_MEM_READ_ONLY		(1UL << 2)
KVM_MEMSLOT_INVALID is actually an internal symbol, not used by userspace. Please move it to kvm_host.h.

Quoted 4 lines

I see that we don't check flags for validity. Please add a check that we don't use undefined flags and return -EINVAL. Should be a separate patch since we may want to backport it.

Okay, will do.

Quoted 3 lines

We need a KVM_CAP_ so userspace knows it can use the feature. Only x86 should respond to it now, until (or if) other archs are updated.

Right.

Quoted 4 lines

Quoted 2 lines
+static bool vma_is_avalid(struct vm_area_struct *vma, bool write_fault)
s/avalid/valid/.

Oops, thanks for you pointing it out.

Quoted 16 lines

virt/kvm/kvm_main.c static inline int check_user_page_hwpoison(unsigned long addr) · line 1079
Quoted 12 lines
virt/kvm/kvm_main.c +11 -0
+{
+	if (write_fault) {
+		if (unlikely(!(vma->vm_flags & VM_WRITE)))
+			return false;
+
+		return true;
+	}
+
+	if (unlikely(!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE))))
+		return false;
+
Strange check. VM_EXEC doesn't concern us at all. Maybe we should check for VM_READ always, and VM_WRITE for write faults.

I do not know if some process's vma only has VM_EXTC that hopes to protect the text/stack section, and we want to map the text section to guest for writing test case.

But i do not have strong opinion about it, since checking VM_READ works fine for my test case.

I will remove the VM_EXEC in the next version.

Quoted 18 lines

virt/kvm/kvm_main.c +3 -1

+	return true;
+}
+
 static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 			bool *async, bool write_fault, bool *writable)
 {

@@ -1076,7 +1103,6 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 
 		if (writable)
 			*writable = write_fault;
-
 		if (async) {
 			down_read(&current->mm->mmap_sem);
 			npages = get_user_page_nowait(current, current->mm,

@@ -1123,8 +1149,9 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 				vma->vm_pgoff;
 			BUG_ON(!kvm_is_mmio_pfn(pfn));
 		} else {

Quoted 9 lines

virt/kvm/kvm_main.c static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic, · line 1154
Quoted 4 lines
virt/kvm/kvm_main.c +2 -1
-			if (async && (vma->vm_flags & VM_WRITE))
+			if (async && vma_is_avalid(vma, write_fault))
 				*async = true;
+
This checks based on the fault type, not memslot type. So we have the risk of the pfn later used for writes?

Yes, but we can not export hva_to_pfn which is only allowed to be used in kvm_main.c. (it is only the help function for gfn_to_pfn_*().)

Quoted 33 lines

Quoted 22 lines

virt/kvm/kvm_main.c +2 -2

 			pfn = get_fault_pfn();
 		}
 		up_read(&current->mm->mmap_sem);

@@ -1148,7 +1175,7 @@ static pfn_t __gfn_to_pfn(struct kvm *kvm, gfn_t gfn, bool atomic, bool *async,
 	if (async)
 		*async = false;
 
-	addr = gfn_to_hva(kvm, gfn);
+	addr = gfn_to_hva_prot(kvm, gfn, write_fault);
 	if (kvm_is_error_hva(addr)) {
 		get_page(bad_page);
 		return page_to_pfn(bad_page);

@@ -1293,7 +1320,7 @@ 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_prot(kvm, gfn, false);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
 	r = __copy_from_user(data, (void __user *)addr + offset, len);

virt/kvm/kvm_main.c int kvm_read_guest_atomic(struct kvm *kvm, gpa_t gpa, void *data, · line 1364

Quoted 8 lines

virt/kvm/kvm_main.c +1 -1

@@ -1331,7 +1358,7 @@ 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_prot(kvm, gfn, false);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
 	pagefault_disable();

Surprised only those places.

Quoted 2 lines

How do we make sure a pfn obtained with write = false isn't later used for writing?

Ah, i think it is hard to ensure it.

May be we can introduce two APIs:
- gfn_to_pfn_read(), kvm_read_gfn()
- gfn_to_pfn_write(), kvm_write_pfn()

They should be paired together by the developer.

By the way, a foolish question, what is ROMD? i did not find any explanation on google.

Gleb Natapov

2012-05-28 07:21 UTC #5 lore raw

Quoted 2 lines · Xiao Guangrong

By the way, a foolish question, what is ROMD? i did not find any explanation on google.

This is memory region that behaves like ROM on read and like a device on write. IIRC some flash chips are like that.

--
Gleb.

Xiao Guangrong

author

2012-05-28 13:01 UTC #6 lore raw

Quoted 6 lines · Gleb Natapov

Quoted 2 lines · Xiao Guangrong
By the way, a foolish question, what is ROMD? i did not find any explanation on google.
This is memory region that behaves like ROM on read and like a device on write. IIRC some flash chips are like that.

Got it, thank you, Gleb! :)