[04/10] KVM: Introduce a new guest mapping API
diff mbox

Message ID 1519235241-6500-5-git-send-email-karahmed@amazon.de
State New
Headers show

Commit Message

KarimAllah Ahmed Feb. 21, 2018, 5:47 p.m. UTC
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(+)

Comments

kernel test robot Feb. 23, 2018, 1:27 a.m. UTC | #1
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
kernel test robot Feb. 23, 2018, 1:37 a.m. UTC | #2
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
KarimAllah Ahmed Feb. 23, 2018, 11:48 p.m. UTC | #3
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
Paolo Bonzini April 12, 2018, 2:33 p.m. UTC | #4
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

> +}
> +

Patch
diff mbox

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;