| Message ID | 1519235241-6500-5-git-send-email-karahmed@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi KarimAllah,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v4.16-rc2 next-20180222]
[cannot apply to kvm/linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/KarimAllah-Ahmed/KVM-X86-Handle-guest-memory-that-does-not-have-a-struct-page/20180223-064826
config: x86_64-randconfig-x012-201807 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
In file included from include/linux/linkage.h:7:0,
from include/linux/preempt.h:10,
from include/linux/hardirq.h:5,
from include/linux/kvm_host.h:10,
from arch/x86/kvm/../../../virt/kvm/kvm_main.c:21:
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:1669:19: error: 'kvm_vcpu_gfn_to_kaddr' undeclared here (not in a function); did you mean 'kvm_vcpu_gfn_to_page'?
EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr);
^
include/linux/export.h:65:16: note: in definition of macro '___EXPORT_SYMBOL'
extern typeof(sym) sym; \
^~~
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:1669:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr);
^~~~~~~~~~~~~~~~~
vim +1669 arch/x86/kvm/../../../virt/kvm/kvm_main.c
1634
1635 bool kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map)
1636 {
1637 kvm_pfn_t pfn;
1638 void *kaddr = NULL;
1639 struct page *page = NULL;
1640
1641 if (map->kaddr && map->gfn == gfn)
1642 /* If the mapping is valid and guest memory is already mapped */
1643 return true;
1644 else if (map->kaddr)
1645 /* If the mapping is valid but trying to map a different guest pfn */
1646 kvm_vcpu_unmap(map);
1647
1648 pfn = kvm_vcpu_gfn_to_pfn(vcpu, gfn);
1649 if (is_error_pfn(pfn))
1650 return false;
1651
1652 if (pfn_valid(pfn)) {
1653 page = pfn_to_page(pfn);
1654 kaddr = vmap(&page, 1, VM_MAP, PAGE_KERNEL);
1655 } else {
1656 kaddr = memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB);
1657 }
1658
1659 if (!kaddr)
1660 return false;
1661
1662 map->page = page;
1663 map->kaddr = kaddr;
1664 map->pfn = pfn;
1665 map->gfn = gfn;
1666
1667 return true;
1668 }
> 1669 EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr);
1670
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi KarimAllah,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v4.16-rc2 next-20180222]
[cannot apply to kvm/linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/KarimAllah-Ahmed/KVM-X86-Handle-guest-memory-that-does-not-have-a-struct-page/20180223-064826
config: mips-malta_kvm_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mips
All error/warnings (new ones prefixed by >>):
In file included from include/linux/linkage.h:7:0,
from include/linux/preempt.h:10,
from include/linux/hardirq.h:5,
from include/linux/kvm_host.h:10,
from arch/mips/kvm/../../../virt/kvm/kvm_main.c:21:
>> arch/mips/kvm/../../../virt/kvm/kvm_main.c:1669:19: error: 'kvm_vcpu_gfn_to_kaddr' undeclared here (not in a function); did you mean 'kvm_vcpu_gfn_to_page'?
EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr);
^
include/linux/export.h:65:16: note: in definition of macro '___EXPORT_SYMBOL'
extern typeof(sym) sym; \
^~~
>> arch/mips/kvm/../../../virt/kvm/kvm_main.c:1669:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr);
^~~~~~~~~~~~~~~~~
vim +1669 arch/mips/kvm/../../../virt/kvm/kvm_main.c
1634
1635 bool kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map)
1636 {
1637 kvm_pfn_t pfn;
1638 void *kaddr = NULL;
1639 struct page *page = NULL;
1640
1641 if (map->kaddr && map->gfn == gfn)
1642 /* If the mapping is valid and guest memory is already mapped */
1643 return true;
1644 else if (map->kaddr)
1645 /* If the mapping is valid but trying to map a different guest pfn */
1646 kvm_vcpu_unmap(map);
1647
1648 pfn = kvm_vcpu_gfn_to_pfn(vcpu, gfn);
1649 if (is_error_pfn(pfn))
1650 return false;
1651
1652 if (pfn_valid(pfn)) {
1653 page = pfn_to_page(pfn);
1654 kaddr = vmap(&page, 1, VM_MAP, PAGE_KERNEL);
1655 } else {
1656 kaddr = memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB);
1657 }
1658
1659 if (!kaddr)
1660 return false;
1661
1662 map->page = page;
1663 map->kaddr = kaddr;
1664 map->pfn = pfn;
1665 map->gfn = gfn;
1666
1667 return true;
1668 }
> 1669 EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr);
1670
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, 2018-02-23 at 09:37 +0800, kbuild test robot wrote: > Hi KarimAllah, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on tip/auto-latest] > [also build test ERROR on v4.16-rc2 next-20180222] > [cannot apply to kvm/linux-next] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/KarimAllah-Ahmed/KVM-X86-Handle-guest-memory-that-does-not-have-a-struct-page/20180223-064826 > config: mips-malta_kvm_defconfig (attached as .config) > compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=mips > > All error/warnings (new ones prefixed by >>): > > In file included from include/linux/linkage.h:7:0, > from include/linux/preempt.h:10, > from include/linux/hardirq.h:5, > from include/linux/kvm_host.h:10, > from arch/mips/kvm/../../../virt/kvm/kvm_main.c:21: > > > > > > > > arch/mips/kvm/../../../virt/kvm/kvm_main.c:1669:19: error: 'kvm_vcpu_gfn_to_kaddr' undeclared here (not in a function); did you mean 'kvm_vcpu_gfn_to_page'? > EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr); > ^ > include/linux/export.h:65:16: note: in definition of macro '___EXPORT_SYMBOL' > extern typeof(sym) sym; \ > ^~~ > > > > > > > > arch/mips/kvm/../../../virt/kvm/kvm_main.c:1669:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL' > EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr); > ^~~~~~~~~~~~~~~~~ Ooops! I will make sure I build KVM as a module as well before posting v2. I will also drop "kvm_vcpu_map_valid" since it is no longer used. > > vim +1669 arch/mips/kvm/../../../virt/kvm/kvm_main.c > > 1634 > 1635 bool kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map) > 1636 { > 1637 kvm_pfn_t pfn; > 1638 void *kaddr = NULL; > 1639 struct page *page = NULL; > 1640 > 1641 if (map->kaddr && map->gfn == gfn) > 1642 /* If the mapping is valid and guest memory is already mapped */ > 1643 return true; > 1644 else if (map->kaddr) > 1645 /* If the mapping is valid but trying to map a different guest pfn */ > 1646 kvm_vcpu_unmap(map); > 1647 > 1648 pfn = kvm_vcpu_gfn_to_pfn(vcpu, gfn); > 1649 if (is_error_pfn(pfn)) > 1650 return false; > 1651 > 1652 if (pfn_valid(pfn)) { > 1653 page = pfn_to_page(pfn); > 1654 kaddr = vmap(&page, 1, VM_MAP, PAGE_KERNEL); > 1655 } else { > 1656 kaddr = memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB); > 1657 } > 1658 > 1659 if (!kaddr) > 1660 return false; > 1661 > 1662 map->page = page; > 1663 map->kaddr = kaddr; > 1664 map->pfn = pfn; > 1665 map->gfn = gfn; > 1666 > 1667 return true; > 1668 } > > > > 1669 EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr); > 1670 > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
Coming back to this (nice) series. On 21/02/2018 18:47, KarimAllah Ahmed wrote: > +bool kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map) > +{ > + kvm_pfn_t pfn; > + void *kaddr = NULL; Can we s/kaddr/hva/ since that's the standard nomenclature? > + struct page *page = NULL; > + > + if (map->kaddr && map->gfn == gfn) > + /* If the mapping is valid and guest memory is already mapped */ > + return true; Please return 0/-EINVAL instead. More important, this optimization is problematic because: 1) the underlying memslots array could have changed. You'd also need to store the generation count (see kvm_read_guest_cached for an example) 2) worse, the memslots array could have switched between the SMM and non-SMM address spaces. This is by the way the reason why there is no kvm_vcpu_read_guest_cached API. However, all the places you're changing in patches 4-10 are doing already kvm_vcpu_gpa_to_page, so I suggest just dropping this optimization. > + else if (map->kaddr) > + /* If the mapping is valid but trying to map a different guest pfn */ > + kvm_vcpu_unmap(map); > + > + pfn = kvm_vcpu_gfn_to_pfn(vcpu, gfn); Please change the API to: static int __kvm_map_gfn(struct kvm_memslot *memslots, gfn_t gfn, struct kvm_host_map *map) calling gfn_to_pfn_memslot(memslots, gfn) int kvm_vcpu_map_gfn(struct kvm_vcpu *vcpu gfn_t gfn, struct kvm_host_map *map) calling kvm_vcpu_gfn_to_memslot + __kvm_map void kvm_unmap_gfn(struct kvm_host_map *map) > + if (is_error_pfn(pfn)) > + return false; Should this be is_error_noslot_pfn? > + if (pfn_valid(pfn)) { > + page = pfn_to_page(pfn); > + kaddr = vmap(&page, 1, VM_MAP, PAGE_KERNEL); > + } else { > + kaddr = memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB); > + } > + > + if (!kaddr) > + return false; > + > + map->page = page; > + map->kaddr = kaddr; > + map->pfn = pfn; > + map->gfn = gfn; > + > + return true; > +} > > +void kvm_vcpu_unmap(struct kvm_host_map *map) > +{ > + if (!map->kaddr) > + return; > + > + if (map->page) > + kunmap(map->page); > + else > + memunmap(map->kaddr); > + > + kvm_release_pfn_dirty(map->pfn); > + memset(map, 0, sizeof(*map)); This can clear just map->kaddr (map->hva after the above review). Thanks, Paolo > +} > +
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index ac0062b..6cc2c29 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -204,6 +204,13 @@ enum { READING_SHADOW_PAGE_TABLES, }; +struct kvm_host_map { + struct page *page; + void *kaddr; + kvm_pfn_t pfn; + kvm_pfn_t gfn; +}; + /* * Sometimes a large or cross-page mmio needs to be broken up into separate * exits for userspace servicing. @@ -700,6 +707,9 @@ struct kvm_memslots *kvm_vcpu_memslots(struct kvm_vcpu *vcpu); struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn); kvm_pfn_t kvm_vcpu_gfn_to_pfn_atomic(struct kvm_vcpu *vcpu, gfn_t gfn); kvm_pfn_t kvm_vcpu_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn); +bool kvm_vcpu_map(struct kvm_vcpu *vcpu, gpa_t gpa, + struct kvm_host_map *map); +void kvm_vcpu_unmap(struct kvm_host_map *map); struct page *kvm_vcpu_gfn_to_page(struct kvm_vcpu *vcpu, gfn_t gfn); unsigned long kvm_vcpu_gfn_to_hva(struct kvm_vcpu *vcpu, gfn_t gfn); unsigned long kvm_vcpu_gfn_to_hva_prot(struct kvm_vcpu *vcpu, gfn_t gfn, bool *writable); @@ -996,6 +1006,11 @@ static inline struct page *kvm_vcpu_gpa_to_page(struct kvm_vcpu *vcpu, return kvm_vcpu_gfn_to_page(vcpu, gpa_to_gfn(gpa)); } +static inline bool kvm_vcpu_map_valid(struct kvm_host_map *map) +{ + return map->kaddr != NULL; +} + static inline bool kvm_is_error_gpa(struct kvm *kvm, gpa_t gpa) { unsigned long hva = gfn_to_hva(kvm, gpa_to_gfn(gpa)); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4501e65..54e7329 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1632,6 +1632,56 @@ struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn) } EXPORT_SYMBOL_GPL(gfn_to_page); +bool kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map) +{ + kvm_pfn_t pfn; + void *kaddr = NULL; + struct page *page = NULL; + + if (map->kaddr && map->gfn == gfn) + /* If the mapping is valid and guest memory is already mapped */ + return true; + else if (map->kaddr) + /* If the mapping is valid but trying to map a different guest pfn */ + kvm_vcpu_unmap(map); + + pfn = kvm_vcpu_gfn_to_pfn(vcpu, gfn); + if (is_error_pfn(pfn)) + return false; + + if (pfn_valid(pfn)) { + page = pfn_to_page(pfn); + kaddr = vmap(&page, 1, VM_MAP, PAGE_KERNEL); + } else { + kaddr = memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB); + } + + if (!kaddr) + return false; + + map->page = page; + map->kaddr = kaddr; + map->pfn = pfn; + map->gfn = gfn; + + return true; +} +EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr); + +void kvm_vcpu_unmap(struct kvm_host_map *map) +{ + if (!map->kaddr) + return; + + if (map->page) + kunmap(map->page); + else + memunmap(map->kaddr); + + kvm_release_pfn_dirty(map->pfn); + memset(map, 0, sizeof(*map)); +} + struct page *kvm_vcpu_gfn_to_page(struct kvm_vcpu *vcpu, gfn_t gfn) { kvm_pfn_t pfn;
In KVM, specially for nested guests, there is a dominant pattern of: => map guest memory -> do_something -> unmap guest memory In addition to all this unnecessarily noise in the code due to boiler plate code, most of the time the mapping function does not properly handle memory that is not backed by "struct page". This new guest mapping API encapsulate most of this boiler plate code and also handles guest memory that is not backed by "struct page". Keep in mind that memremap is horribly slow, so this mapping API should not be used for high-frequency mapping operations. But rather for low-frequency mappings. Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de> --- include/linux/kvm_host.h | 15 +++++++++++++++ virt/kvm/kvm_main.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+)