KVM: introduce readonly memslot
Subsystems (4)
v6 cover KVM: introduce readonly memslot
Changelog:
- fix endless retrying for unhandleable instruction which accesses on readonly host memory
- divide slot->flags by 16:16, the lower part is visible for userspace, the reset is internally used in kvm, and document this in the code
- check slot->flags for gfn_to_hva_memslot
The test case can be found at:
http://lkml.indiana.edu/hypermail/linux/kernel/1207.2/00819/migrate-perf.tar.bz2
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.
As Avi's suggestion, We introduce readonly memory region to map ROM/ROMD to the guest, read access is happy for readonly memslot, write access on readonly memslot will cause KVM_EXIT_MMIO exit.
PATCH v6 1/12 KVM: x86: fix possible infinite loop caused by reexecute_instruction
Currently, we reexecute all unhandleable instructions if they do not access on the mmio, however, it can not work if host map the readonly memory to guest. If the instruction try to write this kind of memory, it will fault again when guest retry it, then we will goto a infinite
loop: retry instruction -> write #PF -> emulation fail -> retry instruction -> ...
Fix it by retrying the instruction only when it faults on the writable memory
--- arch/x86/kvm/x86.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +11 -1
@@ -4473,6 +4473,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)static bool reexecute_instruction (struct kvm_vcpu *vcpu,gva_t gva) {gpa_t gpa; +pfn_t pfn;if (tdp_enabled)return false;
@@ -4490,8 +4491,17 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva)if (gpa ==UNMAPPED_GVA )return true;/* let cpu generate fault */ -if (!kvm_is_error_hva (gfn_to_hva (vcpu->kvm, gpa >>PAGE_SHIFT ))) +/* +* Do not retry the unhandleable instruction if it faults on the +* readonly host memory, otherwise it will goto a infinite loop: +* retry instruction -> write #PF -> emulation fail -> retry +* instruction -> ... +*/ + pfn =gfn_to_pfn (vcpu->kvm,gpa_to_gfn (gpa)); +if (!is_error_pfn (pfn)) { +kvm_release_pfn_clean (pfn);return true; + }return false; }
Signature · 1 line
1.7.7.6
Re: [PATCH v6 01/12] KVM: x86: fix possible infinite loop caused by reexecute_instruction
Quoted 37 lines · Xiao Guangrong
Currently, we reexecute all unhandleable instructions if they do not access on the mmio, however, it can not work if host map the readonly memory to guest. If the instruction try to write this kind of memory, it will fault again when guest retry it, then we will goto a infinite
loop: retry instruction -> write #PF -> emulation fail -> retry instruction -> ...Fix it by retrying the instruction only when it faults on the writable memory
Signed-off-by Xiao Guangrong <[email protected]>--- arch/x86/kvm/x86.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)arch/x86/kvm/x86.c +5
-1@@ -4473,6 +4473,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)static bool reexecute_instruction (struct kvm_vcpu *vcpu,gva_t gva) {gpa_t gpa; +pfn_t pfn;if (tdp_enabled)return false;@@ -4490,8 +4491,17 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva)if (gpa ==UNMAPPED_GVA )return true;/* let cpu generate fault */ -if (!kvm_is_error_hva (gfn_to_hva (vcpu->kvm, gpa >>PAGE_SHIFT ))) +/* +* Do not retry the unhandleable instruction if it faults on the +* readonly host memory, otherwise it will goto a infinite loop: +* retry instruction -> write #PF -> emulation fail -> retry
arch/x86/kvm/x86.c static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva) · line 4507
Quoted 10 lines
arch/x86/kvm/x86.c +6
-0+* instruction -> ... +*/ + pfn =gfn_to_pfn (vcpu->kvm,gpa_to_gfn (gpa)); +if (!is_error_pfn (pfn)) { +kvm_release_pfn_clean (pfn);return true; + }return false; }
Good catch. Did this actually happen or did you find it by code inspection?
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH v6 01/12] KVM: x86: fix possible infinite loop caused by reexecute_instruction
Quoted 45 lines · Avi Kivity
Quoted 44 lines · Xiao Guangrong
Currently, we reexecute all unhandleable instructions if they do not access on the mmio, however, it can not work if host map the readonly memory to guest. If the instruction try to write this kind of memory, it will fault again when guest retry it, then we will goto a infinite
loop: retry instruction -> write #PF -> emulation fail -> retry instruction -> ...Fix it by retrying the instruction only when it faults on the writable memory
Signed-off-by Xiao Guangrong <[email protected]>--- arch/x86/kvm/x86.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)arch/x86/kvm/x86.c +11
-1@@ -4473,6 +4473,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)static bool reexecute_instruction (struct kvm_vcpu *vcpu,gva_t gva) {gpa_t gpa; +pfn_t pfn;if (tdp_enabled)return false;@@ -4490,8 +4491,17 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva)if (gpa ==UNMAPPED_GVA )return true;/* let cpu generate fault */ -if (!kvm_is_error_hva (gfn_to_hva (vcpu->kvm, gpa >>PAGE_SHIFT ))) +/* +* Do not retry the unhandleable instruction if it faults on the +* readonly host memory, otherwise it will goto a infinite loop: +* retry instruction -> write #PF -> emulation fail -> retry +* instruction -> ... +*/ + pfn =gfn_to_pfn (vcpu->kvm,gpa_to_gfn (gpa)); +if (!is_error_pfn (pfn)) { +kvm_release_pfn_clean (pfn);return true; + }
Quoted 6 lines
arch/x86/kvm/x86.c static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva) · line 4507
Quoted 3 lines
arch/x86/kvm/x86.c +0
-0return false; }Good catch. Did this actually happen or did you find it by code inspection?
It did not happen in my test. Actually, it is reported by Marcelo.
PATCH v6 2/12 KVM: fix missing check for memslot flags
In reply to Xiao GuangrongCheck flags when memslot is registered from userspace as Avi's suggestion
--- virt/kvm/kvm_main.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
virt/kvm/kvm_main.c +12 -0
@@ -678,6 +678,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.
@@ -698,6 +706,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 ))
Signature · 1 line
1.7.7.6
PATCH v6 3/12 KVM: hide KVM_MEMSLOT_INVALID from userspace
In reply to Xiao GuangrongQuote Avi's comment:
| KVM_MEMSLOT_INVALID is actually an internal symbol, not used by | userspace. Please move it to kvm_host.h.
Also, we divide the memlsot->flags into two parts, the lower 16 bits are visible for userspace, the higher 16 bits are internally used in kvm
--- include/linux/kvm.h | 7 +++++-- include/linux/kvm_host.h | 7 +++++++ 2 files changed, 12 insertions(+), 2 deletions(-)
include/linux/kvm.h +5 -2
@@ -101,9 +101,12 @@ struct kvm_userspace_memory_region {__u64 userspace_addr;/* start of the userspace allocated memory */ }; -/* for kvm_memory_region::flags */ +/* +* The bit 0 ~ bit 15 of kvm_memory_region::flags are visible for userspace, +* other bits are reserved for kvm internal use which are defined in +* include/linux/kvm_host.h. +*/ #define KVM_MEM_LOG_DIRTY_PAGES 1UL -#define KVM_MEMSLOT_INVALID (1UL << 1)/* for KVM_IRQ_LINE */ struct kvm_irq_level {
include/linux/kvm_host.h +7 -0
@@ -36,6 +36,13 @@#endif /* +* The bit 16 ~ bit 31 of kvm_memory_region::flags are internally used +* in kvm, other bits are visible for userspace which are defined in +* include/linux/kvm_h. +*/ +#define KVM_MEMSLOT_INVALID (1UL << 16) + +/* * If we support unaligned MMIO, at most one fragment will be split into two: */ #ifdef KVM_UNALIGNED_MMIO
Signature · 1 line
1.7.7.6
PATCH v6 4/12 KVM: introduce gfn_to_pfn_memslot_atomic
In reply to Xiao GuangrongIt can instead of hva_to_pfn_atomic
--- arch/x86/kvm/mmu.c | 5 +---- include/linux/kvm_host.h | 3 ++- virt/kvm/kvm_main.c | 14 ++++++++------ 3 files changed, 11 insertions(+), 11 deletions(-)
arch/x86/kvm/mmu.c +1 -4
@@ -2510,15 +2510,12 @@ 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)return KVM_PFN_ERR_FAULT ; - hva =gfn_to_hva_memslot (slot, gfn); - -return hva_to_pfn_atomic (hva); +return gfn_to_pfn_memslot_atomic (slot, gfn); } static int direct_pte_prefetch_many(struct kvm_vcpu *vcpu,
include/linux/kvm_host.h +2 -1
@@ -462,7 +462,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 (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);
@@ -470,6 +469,8 @@ pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn);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_memory_slot *slot,gfn_t gfn); +pfn_t gfn_to_pfn_memslot_atomic (struct kvm_memory_slot *slot,gfn_t gfn); +void kvm_release_pfn_dirty (pfn_t pfn);void kvm_release_pfn_clean (pfn_t pfn);void kvm_set_pfn_dirty (pfn_t pfn);
virt/kvm/kvm_main.c +8 -6
@@ -1102,12 +1102,6 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,return pfn; } -pfn_t hva_to_pfn_atomic (unsigned long addr) -{ -return hva_to_pfn (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) {
@@ -1155,6 +1149,14 @@ pfn_t gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn)return hva_to_pfn (addr, false,NULL , true,NULL ); } +pfn_t gfn_to_pfn_memslot_atomic (struct kvm_memory_slot *slot,gfn_t gfn) +{ +unsigned long addr =gfn_to_hva_memslot (slot, gfn); + +return hva_to_pfn (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) {
Signature · 1 line
1.7.7.6
PATCH v6 5/12 KVM: introduce gfn_to_hva_read/kvm_read_hva/kvm_read_hva_atomic
In reply to Xiao GuangrongThis set of functions is only used to read data from host space, in the later patch, we will only get a readonly hva in gfn_to_hva_read, and the function name is a good hint to let gfn_to_hva_read to pair with kvm_read_hva()/kvm_read_hva_atomic()
--- virt/kvm/kvm_main.c | 27 +++++++++++++++++++++++---- 1 files changed, 23 insertions(+), 4 deletions(-)
virt/kvm/kvm_main.c +23 -4
@@ -1002,6 +1002,25 @@ 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 __user *hva,int len) +{ +return __copy_from_user (data, hva, len); +} + +static int kvm_read_hva_atomic (void *data,void __user *hva,int len) +{ +return __copy_from_user_inatomic (data, hva, len); +} +int get_user_page_nowait (struct task_struct *tsk,struct mm_struct *mm,unsigned long start,int write,struct page **page) {
@@ -1274,10 +1293,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 ;
@@ -1312,11 +1331,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 ;
Signature · 1 line
1.7.7.6
PATCH v6 6/12 KVM: reorganize hva_to_pfn
In reply to Xiao GuangrongWe do too many things in hva_to_pfn, this patch reorganize the code, let it be better readable
--- virt/kvm/kvm_main.c | 159 +++++++++++++++++++++++++++++++-------------------- 1 files changed, 97 insertions(+), 62 deletions(-)
virt/kvm/kvm_main.c +97 -62
@@ -1041,83 +1041,118 @@ static inline int check_user_page_hwpoison(unsigned long addr)return rc == -EHWPOISON ; } -static pfn_t hva_to_pfn (unsigned long addr,bool atomic,bool *async, -bool write_fault,bool *writable) +/* +* The atomic path to get the writable pfn which will be stored in @pfn, +* true indicates success, otherwise false is returned. +*/ +static bool hva_to_pfn_fast (unsigned long addr,bool atomic,bool *async, +bool write_fault,bool *writable,pfn_t *pfn) {struct page *page[1 ]; -int npages =0 ; -pfn_t pfn; +int npages; -/* we can do it either atomically or asynchronously, not both */ -BUG_ON (atomic && async); +if (!(async || atomic)) +return false; -BUG_ON (!write_fault && !writable); + npages =__get_user_pages_fast (addr,1 ,1 , page); +if (npages ==1 ) { + *pfn =page_to_pfn (page[0 ]); -if (writable) - *writable = true; +if (writable) + *writable = true; +return true; + } + +return false; +} -if (atomic || async) - npages =__get_user_pages_fast (addr,1 ,1 , page); +/* +* The slow path to get the pfn of the specified host virtual address, +* 1 indicates success, -errno is returned if error is detected. +*/ +static int hva_to_pfn_slow (unsigned long addr,bool *async,bool write_fault, +bool *writable,pfn_t *pfn) +{ +struct page *page[1 ]; +int npages =0 ; -if (unlikely (npages !=1 ) && !atomic) { -might_sleep (); +might_sleep (); -if (writable) - *writable = write_fault; - -if (async) { -down_read (¤t->mm->mmap_sem); - npages =get_user_page_nowait (current, current->mm, - addr, write_fault, page); -up_read (¤t->mm->mmap_sem); - }else - npages =get_user_pages_fast (addr,1 , write_fault, - page); - -/* map read fault as writable if possible */ -if (unlikely (!write_fault) && npages ==1 ) { -struct page *wpage[1 ]; - - npages =__get_user_pages_fast (addr,1 ,1 , wpage); -if (npages ==1 ) { - *writable = true; -put_page (page[0 ]); - page[0 ] = wpage[0 ]; - } - npages =1 ; +if (writable) + *writable = write_fault; + +if (async) { +down_read (¤t->mm->mmap_sem); + npages =get_user_page_nowait (current, current->mm, + addr, write_fault, page); +up_read (¤t->mm->mmap_sem); + }else + npages =get_user_pages_fast (addr,1 , write_fault, + page); +if (npages !=1 ) +return npages; + +/* map read fault as writable if possible */ +if (unlikely (!write_fault)) { +struct page *wpage[1 ]; + + npages =__get_user_pages_fast (addr,1 ,1 , wpage); +if (npages ==1 ) { + *writable = true; +put_page (page[0 ]); + page[0 ] = wpage[0 ]; } + + npages =1 ; } + *pfn =page_to_pfn (page[0 ]); +return npages; +} + +static pfn_t hva_to_pfn (unsigned long addr,bool atomic,bool *async, +bool write_fault,bool *writable) +{ +struct vm_area_struct *vma; +pfn_t pfn =0 ; +int npages; -if (unlikely (npages !=1 )) { -struct vm_area_struct *vma; +/* we can do it either atomically or asynchronously, not both */ +BUG_ON (atomic && async); -if (atomic) -return KVM_PFN_ERR_FAULT ; +BUG_ON (!write_fault && !writable); -down_read (¤t->mm->mmap_sem); -if (npages == -EHWPOISON || - (!async &&check_user_page_hwpoison (addr))) { -up_read (¤t->mm->mmap_sem); -return KVM_PFN_ERR_HWPOISON ; - } +if (hva_to_pfn_fast (addr, atomic, async, write_fault, writable, &pfn)) +return pfn; - vma =find_vma_intersection (current->mm, addr, addr+1 ); - -if (vma ==NULL ) - pfn =KVM_PFN_ERR_FAULT ; -else if ((vma->vm_flags &VM_PFNMAP )) { - pfn = ((addr - vma->vm_start) >>PAGE_SHIFT ) + - vma->vm_pgoff; -BUG_ON (!kvm_is_mmio_pfn (pfn)); - }else { -if (async && (vma->vm_flags &VM_WRITE )) - *async = true; - pfn =KVM_PFN_ERR_FAULT ; - } -up_read (¤t->mm->mmap_sem); - }else - pfn =page_to_pfn (page[0 ]); +if (atomic) +return KVM_PFN_ERR_FAULT ; + npages =hva_to_pfn_slow (addr, async, write_fault, writable, &pfn); +if (npages ==1 ) +return pfn; + +down_read (¤t->mm->mmap_sem); +if (npages == -EHWPOISON || + (!async &&check_user_page_hwpoison (addr))) { + pfn =KVM_PFN_ERR_HWPOISON ; + goto exit; + } + + vma =find_vma_intersection (current->mm, addr, addr +1 ); + +if (vma ==NULL ) + pfn =KVM_PFN_ERR_FAULT ; +else if ((vma->vm_flags &VM_PFNMAP )) { + pfn = ((addr - vma->vm_start) >>PAGE_SHIFT ) + + vma->vm_pgoff; +BUG_ON (!kvm_is_mmio_pfn (pfn)); + }else { +if (async && (vma->vm_flags &VM_WRITE )) + *async = true; + pfn =KVM_PFN_ERR_FAULT ; + } +exit: +up_read (¤t->mm->mmap_sem);return pfn; }
Signature · 1 line
1.7.7.6
PATCH v6 7/12 KVM: use 'writable' as a hint to map writable pfn
In reply to Xiao GuangrongIn current code, we always map writable pfn for the read fault, in order to support readonly memslot, we map writable pfn only if 'writable' is not NULL
--- virt/kvm/kvm_main.c | 24 +++++++++++++++++++++++- 1 files changed, 23 insertions(+), 1 deletions(-)
virt/kvm/kvm_main.c +23 -1
@@ -1054,6 +1054,14 @@ static bool hva_to_pfn_fast(unsigned long addr, bool atomic, bool *async,if (!(async || atomic))return false; +/* +* Fast pin a writable pfn only if it is a write fault request +* or the caller allows to map a writable pfn for a read fault +* request. +*/ +if (!(write_fault || writable)) +return false; + npages =__get_user_pages_fast (addr,1 ,1 , page);if (npages ==1 ) { *pfn =page_to_pfn (page[0 ]);
@@ -1093,7 +1101,7 @@ static int hva_to_pfn_slow(unsigned long addr, bool *async, bool write_fault,return npages;/* map read fault as writable if possible */ -if (unlikely (!write_fault)) { +if (unlikely (!write_fault) && writable) {struct page *wpage[1 ]; npages =__get_user_pages_fast (addr,1 ,1 , wpage);
@@ -1109,6 +1117,20 @@ static int hva_to_pfn_slow(unsigned long addr, bool *async, bool write_fault,return npages; } +/* +* Pin guest page in memory and return its pfn. +* @addr: host virtual address which maps memory to the guest +* @atomic: whether this function can sleep +* @async: whether this function need to wait IO complete if the +* host page is not in the memory +* @write_fault: whether we should get a writable host page +* @writable: whether it allows to map a writable host page for !@write_fault +* +* The function will map a writable host page for these two cases: +* 1): @write_fault = true +* 2): @write_fault = false && @writable, @writable will tell the caller +* whether the mapping is writable. +*/ static pfn_t hva_to_pfn (unsigned long addr,bool atomic,bool *async,bool write_fault,bool *writable) {
Signature · 1 line
1.7.7.6
PATCH v6 8/12 KVM: introduce KVM_PFN_ERR_RO_FAULT
In reply to Xiao GuangrongIn the later patch, it indicates failure when we try to get a writable pfn from the readonly memslot
--- include/linux/kvm_host.h | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
include/linux/kvm_host.h +1 -0
@@ -65,6 +65,7 @@#define KVM_PFN_ERR_FAULT (KVM_PFN_ERR_MASK)#define KVM_PFN_ERR_HWPOISON (KVM_PFN_ERR_MASK + 1)#define KVM_PFN_ERR_BAD (KVM_PFN_ERR_MASK + 2) +#define KVM_PFN_ERR_RO_FAULT (KVM_PFN_ERR_MASK + 3)static inline bool is_error_pfn (pfn_t pfn) {
Signature · 1 line
1.7.7.6
PATCH v6 9/12 KVM: introduce KVM_HVA_ERR_BAD
In reply to Xiao GuangrongThen, remove bad_hva and inline kvm_is_error_hva
--- include/linux/kvm_host.h | 8 +++++++- virt/kvm/kvm_main.c | 13 +------------ 2 files changed, 8 insertions(+), 13 deletions(-)
include/linux/kvm_host.h +7 -1
@@ -82,6 +82,13 @@ static inline bool is_invalid_pfn(pfn_t pfn)return !is_noslot_pfn (pfn) &&is_error_pfn (pfn); } +#define KVM_HVA_ERR_BAD (PAGE_OFFSET) + +static inline bool kvm_is_error_hva (unsigned long addr) +{ +return addr ==PAGE_OFFSET ; +} +#define KVM_ERR_PTR_BAD_PAGE (ERR_PTR(-ENOENT))static inline bool is_error_page (struct page *page)
@@ -430,7 +437,6 @@ id_to_memslot(struct kvm_memslots *slots, int id)return slot; } -int kvm_is_error_hva (unsigned long addr);int kvm_set_memory_region (struct kvm *kvm,struct kvm_userspace_memory_region *mem,int user_alloc);
virt/kvm/kvm_main.c +1 -12
@@ -931,17 +931,6 @@ void kvm_disable_largepages(void) }EXPORT_SYMBOL_GPL (kvm_disable_largepages); -static inline unsigned long bad_hva (void ) -{ -return PAGE_OFFSET ; -} - -int kvm_is_error_hva (unsigned long addr) -{ -return addr ==bad_hva (); -} -EXPORT_SYMBOL_GPL (kvm_is_error_hva); -struct kvm_memory_slot *gfn_to_memslot (struct kvm *kvm,gfn_t gfn) {return __gfn_to_memslot (kvm_memslots (kvm), gfn);
@@ -988,7 +977,7 @@ static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn, gfn_t *nr_pages) {if (!slot || slot->flags &KVM_MEMSLOT_INVALID ) -return bad_hva (); +return KVM_HVA_ERR_BAD ;if (nr_pages) *nr_pages = slot->npages - (gfn - slot->base_gfn);
Signature · 1 line
1.7.7.6
PATCH v6 10/12 KVM: introduce KVM_HVA_ERR_RO_BAD
In reply to Xiao GuangrongIn the later patch, it indicates failure when we try to get a writable hva from the readonly memslot
--- include/linux/kvm_host.h | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
include/linux/kvm_host.h +3 -2
@@ -82,11 +82,12 @@ static inline bool is_invalid_pfn(pfn_t pfn)return !is_noslot_pfn (pfn) &&is_error_pfn (pfn); } -#define KVM_HVA_ERR_BAD (PAGE_OFFSET) +#define KVM_HVA_ERR_BAD (PAGE_OFFSET) +#define KVM_HVA_ERR_RO_BAD (PAGE_OFFSET + PAGE_SIZE)static inline bool kvm_is_error_hva (unsigned long addr) { -return addr ==PAGE_OFFSET ; +return addr >=PAGE_OFFSET ; }#define KVM_ERR_PTR_BAD_PAGE (ERR_PTR(-ENOENT))
Signature · 1 line
1.7.7.6
PATCH v6 11/12 KVM: introduce readonly memslot
In reply to Xiao GuangrongIn 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
We introduce readonly memory region to map ROM/ROMD to the guest, read access is happy for readonly memslot, write access on readonly memslot will cause KVM_EXIT_MMIO exit
--- Documentation/virtual/kvm/api.txt | 10 +++- arch/x86/include/asm/kvm.h | 1 + arch/x86/kvm/mmu.c | 9 ++++ arch/x86/kvm/x86.c | 1 + include/linux/kvm.h | 6 ++- include/linux/kvm_host.h | 7 +-- virt/kvm/kvm_main.c | 96 ++++++++++++++++++++++++++++++------- 7 files changed, 102 insertions(+), 28 deletions(-)
Documentation/virtual/kvm/api.txt +7 -3
@@ -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,12 @@ 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 when the +KVM_CAP_READONLY_MEM capability, it 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()
arch/x86/include/asm/kvm.h +1 -0
@@ -25,6 +25,7 @@#define __KVM_HAVE_DEBUGREGS#define __KVM_HAVE_XSAVE#define __KVM_HAVE_XCRS +#define __KVM_HAVE_READONLY_MEM/* Architectural interrupt line count. */ #define KVM_NR_INTERRUPTS 256
arch/x86/kvm/mmu.c +9 -0
@@ -2647,6 +2647,15 @@ static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct *static int kvm_handle_bad_page (struct kvm_vcpu *vcpu,gfn_t gfn,pfn_t pfn) { +/* +* Do not cache the mmio info caused by writing the readonly gfn +* into the spte otherwise read access on readonly gfn also can +* caused mmio page fault and treat it as mmio access. +* Return 1 to tell kvm to emulate it. +*/ +if (pfn ==KVM_PFN_ERR_RO_FAULT ) +return 1 ; +if (pfn ==KVM_PFN_ERR_HWPOISON ) {kvm_send_hwpoison_signal (gfn_to_hva (vcpu->kvm, gfn), current);return 0 ;
arch/x86/kvm/x86.c +1 -0
@@ -2175,6 +2175,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 :
include/linux/kvm.h +5 -1
@@ -106,7 +106,8 @@ struct kvm_userspace_memory_region {* other bits are reserved for kvm internal use which are defined in * include/linux/kvm_host.h. */ -#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 {
@@ -621,6 +622,9 @@ 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 +#ifdef __KVM_HAVE_READONLY_MEM +#define KVM_CAP_READONLY_MEM 81 +#endif #ifdef KVM_CAP_IRQ_ROUTING
include/linux/kvm_host.h +1 -6
@@ -465,6 +465,7 @@ int gfn_to_page_many_atomic(struct kvm *kvm, gfn_t gfn, struct page **pages,struct page *gfn_to_page (struct kvm *kvm,gfn_t gfn);unsigned long gfn_to_hva (struct kvm *kvm,gfn_t gfn); +unsigned long gfn_to_hva_memslot (struct kvm_memory_slot *slot,gfn_t gfn);void kvm_release_page_clean (struct page *page);void kvm_release_page_dirty (struct page *page);void kvm_set_page_dirty (struct page *page);
@@ -792,12 +793,6 @@ hva_to_gfn_memslot(unsigned long hva, struct kvm_memory_slot *slot)return slot->base_gfn + gfn_offset; } -static inline unsigned long gfn_to_hva_memslot (struct kvm_memory_slot *slot, -gfn_t gfn) -{ -return slot->userspace_addr + (gfn - slot->base_gfn) *PAGE_SIZE ; -} -static inline gpa_t gfn_to_gpa (gfn_t gfn) {return (gpa_t )gfn <<PAGE_SHIFT ;
virt/kvm/kvm_main.c +78 -18
165 lines not on this page · raw message
Signature · 1 line
1.7.7.6
Quoted 34 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
We introduce readonly memory region to map ROM/ROMD to the guest, read access is happy for readonly memslot, write access on readonly memslot will cause
KVM_EXIT_MMIOexitSigned-off-by Xiao Guangrong <[email protected]>--- Documentation/virtual/kvm/api.txt | 10 +++- arch/x86/include/asm/kvm.h | 1 + arch/x86/kvm/mmu.c | 9 ++++ arch/x86/kvm/x86.c | 1 + include/linux/kvm.h | 6 ++- include/linux/kvm_host.h | 7 +-- virt/kvm/kvm_main.c | 96 ++++++++++++++++++++++++++++++------- 7 files changed, 102 insertions(+), 28 deletions(-)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 << 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
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,12 @@ 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 when the +KVM_CAP_READONLY_MEM capability, it indicates the guest memory is read-only, +that means, guest is only allowed to read it.
The last sentence requires some refactoring. :) How about: "The KVM_CAP_READONLY_MEM capability indicates the availability of the KVM_MEM_READONLY flag. When this flag is set for a memory region, KVM only allows read accesses."
Quoted 64 lines
Writes will be posted to
Documentation/virtual/kvm/api.txt +1
-0+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()arch/x86/include/asm/kvm.h +1
-0@@ -25,6 +25,7 @@#define __KVM_HAVE_DEBUGREGS#define __KVM_HAVE_XSAVE#define __KVM_HAVE_XCRS +#define __KVM_HAVE_READONLY_MEM/* Architectural interrupt line count. */ #define KVM_NR_INTERRUPTS 256arch/x86/kvm/mmu.c +9
-0@@ -2647,6 +2647,15 @@ static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct *static int kvm_handle_bad_page (struct kvm_vcpu *vcpu,gfn_t gfn,pfn_t pfn) { +/* +* Do not cache the mmio info caused by writing the readonly gfn +* into the spte otherwise read access on readonly gfn also can +* caused mmio page fault and treat it as mmio access. +* Return 1 to tell kvm to emulate it. +*/ +if (pfn ==KVM_PFN_ERR_RO_FAULT ) +return 1 ; +if (pfn ==KVM_PFN_ERR_HWPOISON ) {kvm_send_hwpoison_signal (gfn_to_hva (vcpu->kvm, gfn), current);return 0 ;arch/x86/kvm/x86.c +1
-0@@ -2175,6 +2175,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 :include/linux/kvm.h +2
-1@@ -106,7 +106,8 @@ struct kvm_userspace_memory_region {* other bits are reserved for kvm internal use which are defined in * include/linux/kvm_host.h. */ -#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 {
include/linux/kvm.h struct kvm_ppc_smmu_info { · line 627
Quoted 6 lines
include/linux/kvm.h +3
-0@@ -621,6 +622,9 @@ 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 +#ifdef __KVM_HAVE_READONLY_MEM +#define KVM_CAP_READONLY_MEM 81 +#endif
See the discussion on the userspace patches: The CAP, as userspace API, should really be defined unconditionally, kernel code should depend on __KVM_HAVE_READONLY_MEM or a to-be-introduced CONFIG_KVM_HAVE_xxx switch. This allows for cleaner userspace code.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Quoted 38 lines · Jan Kiszka
Quoted 37 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
We introduce readonly memory region to map ROM/ROMD to the guest, read access is happy for readonly memslot, write access on readonly memslot will cause
KVM_EXIT_MMIOexitSigned-off-by Xiao Guangrong <[email protected]>--- Documentation/virtual/kvm/api.txt | 10 +++- arch/x86/include/asm/kvm.h | 1 + arch/x86/kvm/mmu.c | 9 ++++ arch/x86/kvm/x86.c | 1 + include/linux/kvm.h | 6 ++- include/linux/kvm_host.h | 7 +-- virt/kvm/kvm_main.c | 96 ++++++++++++++++++++++++++++++------- 7 files changed, 102 insertions(+), 28 deletions(-)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 << 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,12 @@ 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 12 lines
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 +4
-2-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 when the +KVM_CAP_READONLY_MEM capability, it indicates the guest memory is read-only, +that means, guest is only allowed to read it.The last sentence requires some refactoring. :) How about: "The
KVM_CAP_READONLY_MEMcapability indicates the availability of theKVM_MEM_READONLYflag. When this flag is set for a memory region, KVM only allows read accesses."
Undoubtedly, your sentence is far better than mine. :)
By the way, this patchset has been applied on kvm -next branch, would you mind to post a patch to do these works.
Quoted 62 lines
Quoted 62 lines
Writes will be posted to
Documentation/virtual/kvm/api.txt +1
-0+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()arch/x86/include/asm/kvm.h +1
-0@@ -25,6 +25,7 @@#define __KVM_HAVE_DEBUGREGS#define __KVM_HAVE_XSAVE#define __KVM_HAVE_XCRS +#define __KVM_HAVE_READONLY_MEM/* Architectural interrupt line count. */ #define KVM_NR_INTERRUPTS 256arch/x86/kvm/mmu.c +9
-0@@ -2647,6 +2647,15 @@ static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct *static int kvm_handle_bad_page (struct kvm_vcpu *vcpu,gfn_t gfn,pfn_t pfn) { +/* +* Do not cache the mmio info caused by writing the readonly gfn +* into the spte otherwise read access on readonly gfn also can +* caused mmio page fault and treat it as mmio access. +* Return 1 to tell kvm to emulate it. +*/ +if (pfn ==KVM_PFN_ERR_RO_FAULT ) +return 1 ; +if (pfn ==KVM_PFN_ERR_HWPOISON ) {kvm_send_hwpoison_signal (gfn_to_hva (vcpu->kvm, gfn), current);return 0 ;arch/x86/kvm/x86.c +1
-0@@ -2175,6 +2175,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 :include/linux/kvm.h +2
-1@@ -106,7 +106,8 @@ struct kvm_userspace_memory_region {* other bits are reserved for kvm internal use which are defined in * include/linux/kvm_host.h. */ -#define KVM_MEM_LOG_DIRTY_PAGES 1UL +#define KVM_MEM_LOG_DIRTY_PAGES (1UL << 0) +#define KVM_MEM_READONLY (1UL << 1)
Quoted 14 lines
include/linux/kvm.h struct kvm_ppc_smmu_info { · line 627
Quoted 9 lines
include/linux/kvm.h +3
-0/* for KVM_IRQ_LINE */ struct kvm_irq_level {@@ -621,6 +622,9 @@ 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 +#ifdef __KVM_HAVE_READONLY_MEM +#define KVM_CAP_READONLY_MEM 81 +#endif See the discussion on the userspace patches: The CAP, as userspace API, should really be defined unconditionally, kernel code should depend on
__KVM_HAVE_READONLY_MEMor a to-be-introducedCONFIG_KVM_HAVE_xxxswitch. This allows for cleaner userspace code.
I see that using __KVM_HAVE_ around CAP in kvm.h is very popular:
$ grep __KVM_HAVE include/linux/kvm.h | wc -l 20
As your suggestion, userspace will always use the CAP even if the CAP is not really supported. We do not need care the overload on other arches?
Quoted 52 lines · Xiao Guangrong
Quoted 38 lines · Jan Kiszka
Quoted 37 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
We introduce readonly memory region to map ROM/ROMD to the guest, read access is happy for readonly memslot, write access on readonly memslot will cause
KVM_EXIT_MMIOexitSigned-off-by Xiao Guangrong <[email protected]>--- Documentation/virtual/kvm/api.txt | 10 +++- arch/x86/include/asm/kvm.h | 1 + arch/x86/kvm/mmu.c | 9 ++++ arch/x86/kvm/x86.c | 1 + include/linux/kvm.h | 6 ++- include/linux/kvm_host.h | 7 +-- virt/kvm/kvm_main.c | 96 ++++++++++++++++++++++++++++++------- 7 files changed, 102 insertions(+), 28 deletions(-)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 << 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,12 @@ 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 12 lines
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 +4
-2-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 when the +KVM_CAP_READONLY_MEM capability, it indicates the guest memory is read-only, +that means, guest is only allowed to read it.The last sentence requires some refactoring. :) How about: "The
KVM_CAP_READONLY_MEMcapability indicates the availability of theKVM_MEM_READONLYflag. When this flag is set for a memory region, KVM only allows read accesses."
Quoted 4 lines
Undoubtedly, your sentence is far better than mine. :)
By the way, this patchset has been applied on kvm -next branch, would you mind to post a patch to do these works.
Can do, found some more improvable parts in this IOCTL anyway.
Quoted 78 lines
Quoted 63 lines
Quoted 63 lines
Writes will be posted to
Documentation/virtual/kvm/api.txt +1
-0+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()arch/x86/include/asm/kvm.h +1
-0@@ -25,6 +25,7 @@#define __KVM_HAVE_DEBUGREGS#define __KVM_HAVE_XSAVE#define __KVM_HAVE_XCRS +#define __KVM_HAVE_READONLY_MEM/* Architectural interrupt line count. */ #define KVM_NR_INTERRUPTS 256arch/x86/kvm/mmu.c +9
-0@@ -2647,6 +2647,15 @@ static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct *static int kvm_handle_bad_page (struct kvm_vcpu *vcpu,gfn_t gfn,pfn_t pfn) { +/* +* Do not cache the mmio info caused by writing the readonly gfn +* into the spte otherwise read access on readonly gfn also can +* caused mmio page fault and treat it as mmio access. +* Return 1 to tell kvm to emulate it. +*/ +if (pfn ==KVM_PFN_ERR_RO_FAULT ) +return 1 ; +if (pfn ==KVM_PFN_ERR_HWPOISON ) {kvm_send_hwpoison_signal (gfn_to_hva (vcpu->kvm, gfn), current);return 0 ;arch/x86/kvm/x86.c +1
-0@@ -2175,6 +2175,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 :include/linux/kvm.h +2
-1@@ -106,7 +106,8 @@ struct kvm_userspace_memory_region {* other bits are reserved for kvm internal use which are defined in * include/linux/kvm_host.h. */ -#define KVM_MEM_LOG_DIRTY_PAGES 1UL +#define KVM_MEM_LOG_DIRTY_PAGES (1UL << 0) +#define KVM_MEM_READONLY (1UL << 1)Quoted 14 lines
include/linux/kvm.h struct kvm_ppc_smmu_info { · line 627
Quoted 9 lines
include/linux/kvm.h +3
-0/* for KVM_IRQ_LINE */ struct kvm_irq_level {@@ -621,6 +622,9 @@ 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 +#ifdef __KVM_HAVE_READONLY_MEM +#define KVM_CAP_READONLY_MEM 81 +#endif See the discussion on the userspace patches: The CAP, as userspace API, should really be defined unconditionally, kernel code should depend on
__KVM_HAVE_READONLY_MEMor a to-be-introducedCONFIG_KVM_HAVE_xxxswitch. This allows for cleaner userspace code.
Quoted 4 lines
I see that using
__KVM_HAVE_around CAP inkvm.his very popular:$ grep __KVM_HAVE include/linux/kvm.h | wc -l 20
Yes, mistakes of the past.
Quoted 3 lines
As your suggestion, userspace will always use the CAP even if the CAP is not really supported. We do not need care the overload on other arches?
Generally, userspace can only find out if a CAP is supported by testing during runtime. Sometimes, the CAP may also be used to switch features that are only available on certain archs. But this particular feature should surely become a generic one soon, and the code you could skip in userspace is truly minimal.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Quoted 7 lines · Jan Kiszka
Quoted 3 lines
$ grep __KVM_HAVE include/linux/kvm.h | wc -l 20Yes, mistakes of the past.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
As your suggestion, userspace will always use the CAP even if the CAP is not really supported. We do not need care the overload on other arches?
Generally, userspace can only find out if a CAP is supported by testing during runtime. Sometimes, the CAP may also be used to switch features that are only available on certain archs. But this particular feature should surely become a generic one soon, and the code you could skip in userspace is truly minimal.
You also need them for build time, to see if the ioctl names and structures are defined. qemu chose not to do this ifdeffery but we must not make that choice for other userspace.
In this case the feature is indeed generic and the names introduced are globally available, so the conditional is unneeded. But for other cases it may be needed.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 17 lines · Avi Kivity
Quoted 7 lines · Jan Kiszka
Quoted 3 lines
$ grep __KVM_HAVE include/linux/kvm.h | wc -l 20Yes, mistakes of the past.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
As your suggestion, userspace will always use the CAP even if the CAP is not really supported. We do not need care the overload on other arches?
Generally, userspace can only find out if a CAP is supported by testing during runtime. Sometimes, the CAP may also be used to switch features that are only available on certain archs. But this particular feature should surely become a generic one soon, and the code you could skip in userspace is truly minimal.
Quoted 3 lines
You also need them for build time, to see if the ioctl names and structures are defined. qemu chose not to do this ifdeffery but we must not make that choice for other userspace.
I disagree. We switched QEMU to the pattern the kernel officially
recommends: carry your own headers if you cannot rely on a recent distribution. The only valid use case for #ifdef KVM_CAP is to allow disabling of generic code that only some archs can make use of.
I'm not arguing for cleaning up legacy CAPs but for applying this rule on newly introduced ones.
Jan
PATCH v6 12/12 KVM: indicate readonly access fault
In reply to Xiao GuangrongIntroduce write_readonly_mem in mmio-exit-info to indicate this exit is caused by write access on readonly memslot
--- arch/x86/kvm/x86.c | 23 ++++++++++++++--------- include/linux/kvm.h | 3 +++ include/linux/kvm_host.h | 1 + virt/kvm/kvm_main.c | 3 +++ 4 files changed, 21 insertions(+), 9 deletions(-)
arch/x86/kvm/x86.c +14 -9
@@ -3710,9 +3710,10 @@ int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa, ret =kvm_write_guest (vcpu->kvm, gpa, val, bytes);if (ret <0 ) -return 0 ; +return ret; +kvm_mmu_pte_write (vcpu, gpa, val, bytes); -return 1 ; +return 0 ; }struct read_write_emulator_ops {
@@ -3742,7 +3743,7 @@ static int read_prepare(struct kvm_vcpu *vcpu, void *val, int bytes)static int read_emulate (struct kvm_vcpu *vcpu,gpa_t gpa,void *val,int bytes) { -return !kvm_read_guest (vcpu->kvm, gpa, val, bytes); +return kvm_read_guest (vcpu->kvm, gpa, val, bytes); } static int write_emulate(struct kvm_vcpu *vcpu, gpa_t gpa,
@@ -3807,7 +3808,8 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,if (ret) goto mmio; -if (ops->read_write_emulate (vcpu, gpa, val, bytes)) + ret = ops->read_write_emulate (vcpu, gpa, val, bytes); +if (!ret)return X86EMUL_CONTINUE ; mmio:
@@ -3829,6 +3831,7 @@ mmio: frag->gpa = gpa; frag->data = val; frag->len = now; + frag->write_readonly_mem = (ret == -EPERM ); gpa += now; val += now;
@@ -3842,8 +3845,8 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,struct x86_exception *exception, struct read_write_emulator_ops *ops) { + struct kvm_mmio_fragment *frag;struct kvm_vcpu *vcpu =emul_to_vcpu (ctxt); -gpa_t gpa;int rc; if (ops->read_write_prepare &&
@@ -3875,17 +3878,18 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,if (!vcpu->mmio_nr_fragments)return rc; - gpa = vcpu->mmio_fragments[0 ].gpa; + frag = &vcpu->mmio_fragments[0 ]; vcpu->mmio_needed =1 ; vcpu->mmio_cur_fragment =0 ; - vcpu->run->mmio.len = vcpu->mmio_fragments[0 ].len; + vcpu->run->mmio.len = frag->len; vcpu->run->mmio.is_write = vcpu->mmio_is_write = ops->write; vcpu->run->exit_reason =KVM_EXIT_MMIO ; - vcpu->run->mmio.phys_addr = gpa; + vcpu->run->mmio.phys_addr = frag->gpa; + vcpu->run->mmio.write_readonly_mem = frag->write_readonly_mem; -return ops->read_write_exit_mmio (vcpu, gpa, val, bytes); +return ops->read_write_exit_mmio (vcpu, frag->gpa, val, bytes); } static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
@@ -5525,6 +5529,7 @@ static int complete_mmio(struct kvm_vcpu *vcpu) ++frag; run->exit_reason =KVM_EXIT_MMIO ; run->mmio.phys_addr = frag->gpa; + vcpu->run->mmio.write_readonly_mem = frag->write_readonly_mem;if (vcpu->mmio_is_write)memcpy (run->mmio.data, frag->data, frag->len); run->mmio.len = frag->len;
include/linux/kvm.h +3 -0
@@ -226,6 +226,9 @@ struct kvm_run {__u8 data[8 ];__u32 len;__u8 is_write; +#ifdef __KVM_HAVE_READONLY_MEM +__u8 write_readonly_mem; +#endif } mmio;/* KVM_EXIT_HYPERCALL */ struct {
include/linux/kvm_host.h +1 -0
@@ -189,6 +189,7 @@ struct kvm_mmio_fragment {gpa_t gpa;void *data;unsigned len; +bool write_readonly_mem; };struct kvm_vcpu {
virt/kvm/kvm_main.c +3 -0
@@ -1456,6 +1456,9 @@ int kvm_write_guest_page(struct kvm *kvm, gfn_t gfn, const void *data,unsigned long addr; addr =gfn_to_hva (kvm, gfn); +if (addr ==KVM_HVA_ERR_RO_BAD ) +return -EPERM ; +if (kvm_is_error_hva (addr))return -EFAULT ; r =__copy_to_user ((void __user *)addr + offset, data, len);
Signature · 1 line
1.7.7.6
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 2 lines · Xiao Guangrong
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslot
Please document this in chapter 5 of apic.txt.
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 6 lines · Avi Kivity
Quoted 2 lines · Xiao Guangrong
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslotPlease document this in chapter 5 of apic.txt.
Okay, please review this one.
Subject: [PATCH v6 12/12] KVM: indicate readonly access fault
Introduce write_readonly_mem in mmio-exit-info to indicate this exit is caused by write access on readonly memslot
--- Documentation/virtual/kvm/api.txt | 8 +++++++- arch/x86/kvm/x86.c | 23 ++++++++++++++--------- include/linux/kvm.h | 3 +++ include/linux/kvm_host.h | 1 + virt/kvm/kvm_main.c | 3 +++ 5 files changed, 28 insertions(+), 10 deletions(-)
Documentation/virtual/kvm/api.txt +7 -1
@@ -2089,12 +2089,18 @@ Unused. __u8 data[8]; __u32 len; __u8 is_write; +#ifdef __KVM_HAVE_READONLY_MEM + __u8 write_readonly_mem; +#endif } mmio; If exit_reason is KVM_EXIT_MMIO, then the vcpu has executed a memory-mapped I/O instruction which could not be satisfied by kvm. The 'data' member contains the written data if 'is_write' is -true, and should be filled by application code otherwise. +true, and should be filled by application code otherwise. When the +KVM_CAP_READONLY_MEM capability, the 'write_readonly_mem' member indicates +whether the exit is caused by write access on the readonly memslot +(see KVM_MEM_READONLY in 4.35 KVM_SET_USER_MEMORY_REGION). NOTE: For KVM_EXIT_IO, KVM_EXIT_MMIO and KVM_EXIT_OSI, the corresponding operations are complete (and guest state is consistent) only after userspace
arch/x86/kvm/x86.c +14 -9
@@ -3710,9 +3710,10 @@ int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa, ret =kvm_write_guest (vcpu->kvm, gpa, val, bytes);if (ret <0 ) -return 0 ; +return ret; +kvm_mmu_pte_write (vcpu, gpa, val, bytes); -return 1 ; +return 0 ; }struct read_write_emulator_ops {
@@ -3742,7 +3743,7 @@ static int read_prepare(struct kvm_vcpu *vcpu, void *val, int bytes)static int read_emulate (struct kvm_vcpu *vcpu,gpa_t gpa,void *val,int bytes) { -return !kvm_read_guest (vcpu->kvm, gpa, val, bytes); +return kvm_read_guest (vcpu->kvm, gpa, val, bytes); } static int write_emulate(struct kvm_vcpu *vcpu, gpa_t gpa,
@@ -3807,7 +3808,8 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,if (ret) goto mmio; -if (ops->read_write_emulate (vcpu, gpa, val, bytes)) + ret = ops->read_write_emulate (vcpu, gpa, val, bytes); +if (!ret)return X86EMUL_CONTINUE ; mmio:
@@ -3829,6 +3831,7 @@ mmio: frag->gpa = gpa; frag->data = val; frag->len = now; + frag->write_readonly_mem = (ret == -EPERM ); gpa += now; val += now;
@@ -3842,8 +3845,8 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,struct x86_exception *exception, struct read_write_emulator_ops *ops) { + struct kvm_mmio_fragment *frag;struct kvm_vcpu *vcpu =emul_to_vcpu (ctxt); -gpa_t gpa;int rc; if (ops->read_write_prepare &&
@@ -3875,17 +3878,18 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,if (!vcpu->mmio_nr_fragments)return rc; - gpa = vcpu->mmio_fragments[0 ].gpa; + frag = &vcpu->mmio_fragments[0 ]; vcpu->mmio_needed =1 ; vcpu->mmio_cur_fragment =0 ; - vcpu->run->mmio.len = vcpu->mmio_fragments[0 ].len; + vcpu->run->mmio.len = frag->len; vcpu->run->mmio.is_write = vcpu->mmio_is_write = ops->write; vcpu->run->exit_reason =KVM_EXIT_MMIO ; - vcpu->run->mmio.phys_addr = gpa; + vcpu->run->mmio.phys_addr = frag->gpa; + vcpu->run->mmio.write_readonly_mem = frag->write_readonly_mem; -return ops->read_write_exit_mmio (vcpu, gpa, val, bytes); +return ops->read_write_exit_mmio (vcpu, frag->gpa, val, bytes); } static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
@@ -5525,6 +5529,7 @@ static int complete_mmio(struct kvm_vcpu *vcpu) ++frag; run->exit_reason =KVM_EXIT_MMIO ; run->mmio.phys_addr = frag->gpa; + vcpu->run->mmio.write_readonly_mem = frag->write_readonly_mem;if (vcpu->mmio_is_write)memcpy (run->mmio.data, frag->data, frag->len); run->mmio.len = frag->len;
include/linux/kvm.h +3 -0
@@ -226,6 +226,9 @@ struct kvm_run {__u8 data[8 ];__u32 len;__u8 is_write; +#ifdef __KVM_HAVE_READONLY_MEM +__u8 write_readonly_mem; +#endif } mmio;/* KVM_EXIT_HYPERCALL */ struct {
include/linux/kvm_host.h +1 -0
@@ -189,6 +189,7 @@ struct kvm_mmio_fragment {gpa_t gpa;void *data;unsigned len; +bool write_readonly_mem; };struct kvm_vcpu {
virt/kvm/kvm_main.c +3 -0
@@ -1456,6 +1456,9 @@ int kvm_write_guest_page(struct kvm *kvm, gfn_t gfn, const void *data,unsigned long addr; addr =gfn_to_hva (kvm, gfn); +if (addr ==KVM_HVA_ERR_RO_BAD ) +return -EPERM ; +if (kvm_is_error_hva (addr))return -EFAULT ; r =__copy_to_user ((void __user *)addr + offset, data, len);
Signature · 1 line
1.7.7.6
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 8 lines · Xiao Guangrong
Quoted 6 lines · Avi Kivity
Quoted 2 lines · Xiao Guangrong
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslotPlease document this in chapter 5 of apic.txt.
Quoted 7 lines
Okay, please review this one.
Subject: [PATCH v6 12/12] KVM: indicate readonly access fault
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslot
I'm not sure whether this indication can be trusted by userspace. By the time userspace gets to process this, the slot may no longer exist, or it may be writable.
(in the same way an mmio exit might actually hit RAM)
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 13 lines · Avi Kivity
Quoted 12 lines · Xiao Guangrong
Quoted 6 lines · Avi Kivity
Quoted 2 lines · Xiao Guangrong
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslotPlease document this in chapter 5 of apic.txt.
Okay, please review this one.
Subject: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 7 lines
Quoted 3 lines
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslotI'm not sure whether this indication can be trusted by userspace. By the time userspace gets to process this, the slot may no longer exist, or it may be writable.
The case of deleting memslot is ok, because userspace just skips this fault if no readonly mem or no fault handler can be found.
Switching memslot from readonly to writable sounds strange, i agree with you that this flag is untrusty under this case.
Marcelo, any comments?
Quoted 2 lines
(in the same way an mmio exit might actually hit RAM)
So, in the userspace, for the safe reason, we should walk all memslots not just walking mmio handlers?
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 22 lines · Xiao Guangrong
Quoted 13 lines · Avi Kivity
Quoted 12 lines · Xiao Guangrong
Quoted 6 lines · Avi Kivity
Quoted 2 lines · Xiao Guangrong
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslotPlease document this in chapter 5 of apic.txt.
Okay, please review this one.
Subject: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 7 lines
Quoted 3 lines
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslotI'm not sure whether this indication can be trusted by userspace. By the time userspace gets to process this, the slot may no longer exist, or it may be writable.
Quoted 5 lines
The case of deleting memslot is ok, because userspace just skips this fault if no readonly mem or no fault handler can be found.
Switching memslot from readonly to writable sounds strange, i agree with you that this flag is untrusty under this case.
It's not so strange, see the PAM registers in the 440fx chipset.
The strategy for shadowing the BIOS is to set PAM for (read: pci, write:
RAM) (=a readonly slot in kvm), memcpy the BIOS onto itself, then switch the PAM to (read: RAM, write: RAM) (=rw slot in kvm) or (read: RAM,
write: PCI) (=different readonly slot in kvm).
Of course that usually happens with just one cpu running so there would be no confusion. But in general the indication is racy.
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
In reply to Xiao GuangrongQuoted 22 lines · Xiao Guangrong
Quoted 13 lines · Avi Kivity
Quoted 12 lines · Xiao Guangrong
Quoted 6 lines · Avi Kivity
Quoted 2 lines · Xiao Guangrong
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslotPlease document this in chapter 5 of apic.txt.
Okay, please review this one.
Subject: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 7 lines
Quoted 3 lines
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslotI'm not sure whether this indication can be trusted by userspace. By the time userspace gets to process this, the slot may no longer exist, or it may be writable.
Quoted 7 lines
The case of deleting memslot is ok, because userspace just skips this fault if no readonly mem or no fault handler can be found.
Switching memslot from readonly to writable sounds strange, i agree with you that this flag is untrusty under this case.
Marcelo, any comments?
The same can happen with slot deletion, for example.
Userspace (which performed the modification which can result in faults to non-existant/read-only/.../new-tag memslot), must handle the faults properly or avoid the possibility for reference to memslot information from the past.
I think its worthwhile to add a note about this in the API
documentation: "The user of this interface is responsible for handling references to stale memslot information, either by handling exit notifications which reference stale memslot information or not allowing these notifications to exist by stopping all vcpus in userspace before performing modifications to the memslots map".
Quoted 9 lines
Quoted 1 line
(in the same way an mmio exit might actually hit RAM)
So, in the userspace, for the safe reason, we should walk all memslots not just walking mmio handlers?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 38 lines · Marcelo Tosatti
Quoted 22 lines · Xiao Guangrong
Quoted 13 lines · Avi Kivity
Quoted 12 lines · Xiao Guangrong
Quoted 6 lines · Avi Kivity
Quoted 2 lines · Xiao Guangrong
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslotPlease document this in chapter 5 of apic.txt.
Okay, please review this one.
Subject: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 7 lines
Quoted 3 lines
Introduce
write_readonly_memin mmio-exit-info to indicate this exit is caused by write access on readonly memslotI'm not sure whether this indication can be trusted by userspace. By the time userspace gets to process this, the slot may no longer exist, or it may be writable.
Quoted 7 lines
The case of deleting memslot is ok, because userspace just skips this fault if no readonly mem or no fault handler can be found.
Switching memslot from readonly to writable sounds strange, i agree with you that this flag is untrusty under this case.
Marcelo, any comments?
The same can happen with slot deletion, for example.
Userspace (which performed the modification which can result in faults to non-existant/read-only/.../new-tag memslot), must handle the faults properly or avoid the possibility for reference to memslot information from the past.
Quoted 6 lines
I think its worthwhile to add a note about this in the API
documentation: "The user of this interface is responsible for handling references to stale memslot information, either by handling exit notifications which reference stale memslot information or not allowing these notifications to exist by stopping all vcpus in userspace before performing modifications to the memslots map".
Or we can drop the new interface and rely on userspace to perform the lookup under its own locking rules.
It's slow, but writes to ROM or ROM/device are rare anyway.
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 14 lines · Avi Kivity
Quoted 7 lines
The same can happen with slot deletion, for example.
Userspace (which performed the modification which can result in faults to non-existant/read-only/.../new-tag memslot), must handle the faults properly or avoid the possibility for reference to memslot information from the past.
Quoted 6 lines
I think its worthwhile to add a note about this in the API
documentation: "The user of this interface is responsible for handling references to stale memslot information, either by handling exit notifications which reference stale memslot information or not allowing these notifications to exist by stopping all vcpus in userspace before performing modifications to the memslots map".
Quoted 4 lines
Or we can drop the new interface and rely on userspace to perform the lookup under its own locking rules.
It's slow, but writes to ROM or ROM/device are rare anyway.
Lookup what information?
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 15 lines · Marcelo Tosatti
Quoted 14 lines · Avi Kivity
Quoted 13 lines
The same can happen with slot deletion, for example.
Userspace (which performed the modification which can result in faults to non-existant/read-only/.../new-tag memslot), must handle the faults properly or avoid the possibility for reference to memslot information from the past.
I think its worthwhile to add a note about this in the API
documentation: "The user of this interface is responsible for handling references to stale memslot information, either by handling exit notifications which reference stale memslot information or not allowing these notifications to exist by stopping all vcpus in userspace before performing modifications to the memslots map".
Quoted 6 lines
Quoted 4 lines
Or we can drop the new interface and rely on userspace to perform the lookup under its own locking rules.
It's slow, but writes to ROM or ROM/device are rare anyway.
Lookup what information?
Ping?
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
In reply to Marcelo TosattiQuoted 15 lines · Marcelo Tosatti
Quoted 14 lines · Avi Kivity
Quoted 13 lines
The same can happen with slot deletion, for example.
Userspace (which performed the modification which can result in faults to non-existant/read-only/.../new-tag memslot), must handle the faults properly or avoid the possibility for reference to memslot information from the past.
I think its worthwhile to add a note about this in the API
documentation: "The user of this interface is responsible for handling references to stale memslot information, either by handling exit notifications which reference stale memslot information or not allowing these notifications to exist by stopping all vcpus in userspace before performing modifications to the memslots map".
Quoted 6 lines
Quoted 4 lines
Or we can drop the new interface and rely on userspace to perform the lookup under its own locking rules.
It's slow, but writes to ROM or ROM/device are rare anyway.
Lookup what information?
Where to dispatch the write.
In fact userspace has to do that anyway if it's a ROM/device. There's no way userspace can guess that unless we pass in the slot number (which isn't synchronized with anything).
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 21 lines · Avi Kivity
Quoted 20 lines · Marcelo Tosatti
Quoted 18 lines · Avi Kivity
Quoted 7 lines
The same can happen with slot deletion, for example.
Userspace (which performed the modification which can result in faults to non-existant/read-only/.../new-tag memslot), must handle the faults properly or avoid the possibility for reference to memslot information from the past.
Quoted 6 lines
I think its worthwhile to add a note about this in the API
documentation: "The user of this interface is responsible for handling references to stale memslot information, either by handling exit notifications which reference stale memslot information or not allowing these notifications to exist by stopping all vcpus in userspace before performing modifications to the memslots map".Or we can drop the new interface and rely on userspace to perform the lookup under its own locking rules.
It's slow, but writes to ROM or ROM/device are rare anyway.
Quoted 7 lines
Quoted 1 line
Lookup what information?
Where to dispatch the write.
In fact userspace has to do that anyway if it's a ROM/device. There's no way userspace can guess that unless we pass in the slot number (which isn't synchronized with anything).
Alright, do you prefer the details of this exit to be worked out later, when necessary, then?
That is, not merge this particular patch of the series?
Re: [PATCH v6 12/12] KVM: indicate readonly access fault
Quoted 26 lines · Marcelo Tosatti
Quoted 25 lines · Avi Kivity
Quoted 15 lines · Marcelo Tosatti
Quoted 14 lines · Avi Kivity
Quoted 13 lines
The same can happen with slot deletion, for example.
Userspace (which performed the modification which can result in faults to non-existant/read-only/.../new-tag memslot), must handle the faults properly or avoid the possibility for reference to memslot information from the past.
I think its worthwhile to add a note about this in the API
documentation: "The user of this interface is responsible for handling references to stale memslot information, either by handling exit notifications which reference stale memslot information or not allowing these notifications to exist by stopping all vcpus in userspace before performing modifications to the memslots map".Quoted 6 lines
Quoted 4 lines
Or we can drop the new interface and rely on userspace to perform the lookup under its own locking rules.
It's slow, but writes to ROM or ROM/device are rare anyway.
Lookup what information?
Where to dispatch the write.
Quoted 9 lines
Quoted 3 lines
In fact userspace has to do that anyway if it's a ROM/device. There's no way userspace can guess that unless we pass in the slot number (which isn't synchronized with anything).
Alright, do you prefer the details of this exit to be worked out later, when necessary, then?
That is, not merge this particular patch of the series?
Right. I think it is unneeded.
Signature · 1 line
error compiling committee.c: too many arguments to function
Avi Kivity
AppliedQuoted 10 lines · Xiao Guangrong
Changelog:
- fix endless retrying for unhandleable instruction which accesses on readonly host memory
- divideslot->flagsby 16:16, the lower part is visible for userspace, the reset is internally used in kvm, and document this in the code
- checkslot->flagsforgfn_to_hva_memslotThe test case can be found at:
http://lkml.indiana.edu/hypermail/linux/kernel/1207.2/00819/migrate-perf.tar.bz2
Quoted 7 lines
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.
As Avi's suggestion, We introduce readonly memory region to map ROM/ROMD to the guest, read access is happy for readonly memslot, write access on readonly memslot will cause
KVM_EXIT_MMIOexit.
Thanks, applied 1-11.
Signature · 1 line
error compiling committee.c: too many arguments to function