Re: [PATCH 04/10] KVM: Introduce a new guest mapping API
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
> +}
> +
Re: [PATCH 04/10] KVM: Introduce a new guest mapping API
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
> 1635bool kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct
> kvm_host_map *map)
> 1636{
> 1637kvm_pfn_t pfn;
> 1638void *kaddr = NULL;
> 1639struct page *page = NULL;
> 1640
> 1641if (map->kaddr && map->gfn == gfn)
> 1642/* If the mapping is valid and guest memory is
> already mapped */
> 1643return true;
> 1644else if (map->kaddr)
> 1645/* If the mapping is valid but trying to map a
> different guest pfn */
> 1646kvm_vcpu_unmap(map);
> 1647
> 1648pfn = kvm_vcpu_gfn_to_pfn(vcpu, gfn);
> 1649if (is_error_pfn(pfn))
> 1650return false;
> 1651
> 1652if (pfn_valid(pfn)) {
> 1653page = pfn_to_page(pfn);
> 1654kaddr = vmap(&page, 1, VM_MAP, PAGE_KERNEL);
> 1655} else {
> 1656kaddr = memremap(pfn_to_hpa(pfn), PAGE_SIZE,
> MEMREMAP_WB);
> 1657}
> 1658
> 1659if (!kaddr)
> 1660return false;
> 1661
> 1662map->page = page;
> 1663map->kaddr = kaddr;
> 1664map->pfn = pfn;
> 1665map->gfn = gfn;
> 1666
> 1667return true;
> 1668}
> >
> > 1669EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_kaddr);
> 1670
>
> ---
> 0-DAY kernel test infrastructureOpen 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
Re: [PATCH 04/10] KVM: Introduce a new guest mapping API
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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH 04/10] KVM: Introduce a new guest mapping API
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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
[PATCH 04/10] KVM: Introduce a new guest mapping API
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
---
include/linux/kvm_host.h | 15 +++
virt/kvm/kvm_main.c | 50
2 files changed, 65 insertions(+)
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;
--
2.7.4
