Re: [PATCH 00/10] KVM/X86: Handle guest memory that does not have a struct page

Re: [PATCH 00/10] KVM/X86: Handle guest memory that does not have a struct page

[Raslan, KarimAllah]

Jim/Paolo/Radim,

Any complains about the current API? (introduced in 4/10)

I have more patches on top and I would like to ensure that this is 
agreed upon at least before sending more revisions/patches.

Also 1, 2, and 3 should be a bit straight forward and does not use 
this API.

Thanks.

On Wed, 2018-02-21 at 18:47 +0100, KarimAllah Ahmed wrote:
> For the most part, KVM can handle guest memory that does not have a struct
> page (i.e. not directly managed by the kernel). However, There are a few places
> in the code, specially in the nested code, that does not support that.
> 
> Patch 1, 2, and 3 avoid the mapping and unmapping all together and just
> directly use kvm_guest_read and kvm_guest_write.
> 
> Patch 4 introduces a new guest mapping interface that encapsulate all the
> bioler plate code that is needed to map and unmap guest memory. It also
> supports guest memory without "struct page".
> 
> Patch 5, 6, 7, 8, 9, and 10 switch most of the offending code in VMX and hyperv
> to use the new guest mapping API.
> 
> This patch series is the first set of fixes. Handling SVM and APIC-access page
> will be handled in a different patch series.
> 
> KarimAllah Ahmed (10):
>   X86/nVMX: handle_vmon: Read 4 bytes from guest memory instead of
>     map->read->unmap sequence
>   X86/nVMX: handle_vmptrld: Copy the VMCS12 directly from guest memory
>     instead of map->copy->unmap sequence.
>   X86/nVMX: Update the PML table without mapping and unmapping the page
>   KVM: Introduce a new guest mapping API
>   KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap
>   KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page
>   KVM/nVMX: Use kvm_vcpu_map when mapping the posted interrupt
>     descriptor table
>   KVM/X86: Use kvm_vcpu_map in emulator_cmpxchg_emulated
>   KVM/X86: hyperv: Use kvm_vcpu_map in synic_clear_sint_msg_pending
>   KVM/X86: hyperv: Use kvm_vcpu_map in synic_deliver_msg
> 
>  arch/x86/kvm/hyperv.c    |  28 ++++-----
>  arch/x86/kvm/vmx.c       | 144 +++++++++++++++--------------------------------
>  arch/x86/kvm/x86.c       |  13 ++---
>  include/linux/kvm_host.h |  15 +++++
>  virt/kvm/kvm_main.c      |  50 ++++++++++++++++
>  5 files changed, 129 insertions(+), 121 deletions(-)
> 
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 00/10] KVM/X86: Handle guest memory that does not have a struct page

[Jim Mattson]

No complaints here!

On Thu, Mar 1, 2018 at 7:24 AM, Raslan, KarimAllah <karahmed@amazon.de> wrote:
> Jim/Paolo/Radim,
>
> Any complains about the current API? (introduced in 4/10)
>
> I have more patches on top and I would like to ensure that this is
> agreed upon at least before sending more revisions/patches.
>
> Also 1, 2, and 3 should be a bit straight forward and does not use
> this API.
>
> Thanks.
>
> On Wed, 2018-02-21 at 18:47 +0100, KarimAllah Ahmed wrote:
>> For the most part, KVM can handle guest memory that does not have a struct
>> page (i.e. not directly managed by the kernel). However, There are a few places
>> in the code, specially in the nested code, that does not support that.
>>
>> Patch 1, 2, and 3 avoid the mapping and unmapping all together and just
>> directly use kvm_guest_read and kvm_guest_write.
>>
>> Patch 4 introduces a new guest mapping interface that encapsulate all the
>> bioler plate code that is needed to map and unmap guest memory. It also
>> supports guest memory without "struct page".
>>
>> Patch 5, 6, 7, 8, 9, and 10 switch most of the offending code in VMX and hyperv
>> to use the new guest mapping API.
>>
>> This patch series is the first set of fixes. Handling SVM and APIC-access page
>> will be handled in a different patch series.
>>
>> KarimAllah Ahmed (10):
>>   X86/nVMX: handle_vmon: Read 4 bytes from guest memory instead of
>>     map->read->unmap sequence
>>   X86/nVMX: handle_vmptrld: Copy the VMCS12 directly from guest memory
>>     instead of map->copy->unmap sequence.
>>   X86/nVMX: Update the PML table without mapping and unmapping the page
>>   KVM: Introduce a new guest mapping API
>>   KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap
>>   KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page
>>   KVM/nVMX: Use kvm_vcpu_map when mapping the posted interrupt
>>     descriptor table
>>   KVM/X86: Use kvm_vcpu_map in emulator_cmpxchg_emulated
>>   KVM/X86: hyperv: Use kvm_vcpu_map in synic_clear_sint_msg_pending
>>   KVM/X86: hyperv: Use kvm_vcpu_map in synic_deliver_msg
>>
>>  arch/x86/kvm/hyperv.c    |  28 ++++-----
>>  arch/x86/kvm/vmx.c       | 144 +++++++++++++++--------------------------------
>>  arch/x86/kvm/x86.c       |  13 ++---
>>  include/linux/kvm_host.h |  15 +++++
>>  virt/kvm/kvm_main.c      |  50 ++++++++++++++++
>>  5 files changed, 129 insertions(+), 121 deletions(-)
>>
> 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 00/10] KVM/X86: Handle guest memory that does not have a struct page

[Paolo Bonzini]

On 01/03/2018 18:51, Jim Mattson wrote:
> No complaints here!

It seems sane here too, I'll review the individual patches as soon as
possible.

Thanks,

Paolo

> On Thu, Mar 1, 2018 at 7:24 AM, Raslan, KarimAllah <karahmed@amazon.de> wrote:
>> Jim/Paolo/Radim,
>>
>> Any complains about the current API? (introduced in 4/10)
>>
>> I have more patches on top and I would like to ensure that this is
>> agreed upon at least before sending more revisions/patches.
>>
>> Also 1, 2, and 3 should be a bit straight forward and does not use
>> this API.
>>
>> Thanks.
>>
>> On Wed, 2018-02-21 at 18:47 +0100, KarimAllah Ahmed wrote:
>>> For the most part, KVM can handle guest memory that does not have a struct
>>> page (i.e. not directly managed by the kernel). However, There are a few places
>>> in the code, specially in the nested code, that does not support that.
>>>
>>> Patch 1, 2, and 3 avoid the mapping and unmapping all together and just
>>> directly use kvm_guest_read and kvm_guest_write.
>>>
>>> Patch 4 introduces a new guest mapping interface that encapsulate all the
>>> bioler plate code that is needed to map and unmap guest memory. It also
>>> supports guest memory without "struct page".
>>>
>>> Patch 5, 6, 7, 8, 9, and 10 switch most of the offending code in VMX and hyperv
>>> to use the new guest mapping API.
>>>
>>> This patch series is the first set of fixes. Handling SVM and APIC-access page
>>> will be handled in a different patch series.
>>>
>>> KarimAllah Ahmed (10):
>>>   X86/nVMX: handle_vmon: Read 4 bytes from guest memory instead of
>>>     map->read->unmap sequence
>>>   X86/nVMX: handle_vmptrld: Copy the VMCS12 directly from guest memory
>>>     instead of map->copy->unmap sequence.
>>>   X86/nVMX: Update the PML table without mapping and unmapping the page
>>>   KVM: Introduce a new guest mapping API
>>>   KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap
>>>   KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page
>>>   KVM/nVMX: Use kvm_vcpu_map when mapping the posted interrupt
>>>     descriptor table
>>>   KVM/X86: Use kvm_vcpu_map in emulator_cmpxchg_emulated
>>>   KVM/X86: hyperv: Use kvm_vcpu_map in synic_clear_sint_msg_pending
>>>   KVM/X86: hyperv: Use kvm_vcpu_map in synic_deliver_msg
>>>
>>>  arch/x86/kvm/hyperv.c    |  28 ++++-----
>>>  arch/x86/kvm/vmx.c       | 144 +++++++++++++++--------------------------------
>>>  arch/x86/kvm/x86.c       |  13 ++---
>>>  include/linux/kvm_host.h |  15 +++++
>>>  virt/kvm/kvm_main.c      |  50 ++++++++++++++++
>>>  5 files changed, 129 insertions(+), 121 deletions(-)
>>>
>> 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

[Paolo Bonzini]

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 05/10] KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap

[Paolo Bonzini]

On 21/02/2018 18:47, KarimAllah Ahmed wrote:
> ... since using kvm_vcpu_gpa_to_page() and kmap() will only work for guest
> memory that has a "struct page".
> 
> The life-cycle of the mapping also changes to avoid doing map and unmap on
> every single exit (which becomes very expesive once we use memremap). Now
> the memory is mapped and only unmapped when a new VMCS12 is loaded into the
> vCPU (or when the vCPU is freed!).

In this particular case SMM is not an issue because it cannot use VMX.
Therefore it's safe to ignore non-SMM address spaces.  You can then
introduce

int kvm_map_gfn(struct kvm_vcpu *vcpu gfn_t gfn,
		struct kvm_host_map *map)

	calling kvm_gfn_to_memslot + __kvm_map_gfn

which could also handle the caching aspect.  But please let's look at it
later, making the lifecycle change separate from the new API.

Paolo

> Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
> ---
>  arch/x86/kvm/vmx.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)

Re: [PATCH 06/10] KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page

[Paolo Bonzini]

On 21/02/2018 18:47, KarimAllah Ahmed wrote:
> +
> +		if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->virtual_apic_page_addr), map))
> +			vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, gfn_to_gpa(map->pfn));

This should in principle be pfn_to_hpa.  However, pfn_to_hpa is unused;
let's just remove it and do "<< PAGE_SHIFT".  Unlike gfn_to_gpa, where
in principle the shift could be different, pfn_to_hpa is *by definition*
a shift left by PAGE_SHIFT.

Paolo

Re: [PATCH 07/10] KVM/nVMX: Use kvm_vcpu_map when mapping the posted interrupt descriptor table

[Paolo Bonzini]

On 21/02/2018 18:47, KarimAllah Ahmed wrote:
> ... since using kvm_vcpu_gpa_to_page() and kmap() will only work for guest
> memory that has a "struct page".
> 
> The life-cycle of the mapping also changes to avoid doing map and unmap on
> every single exit (which becomes very expesive once we use memremap).  Now
> the memory is mapped and only unmapped when a new VMCS12 is loaded into the
> vCPU (or when the vCPU is freed!).
> 
> Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>

Same here, let's change the lifecycle separately.

Paolo

> ---
>  arch/x86/kvm/vmx.c | 45 +++++++++++++--------------------------------
>  1 file changed, 13 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index a700338..7b29419 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -461,7 +461,7 @@ struct nested_vmx {
>  	 */
>  	struct page *apic_access_page;
>  	struct kvm_host_map virtual_apic_map;
> -	struct page *pi_desc_page;
> +	struct kvm_host_map pi_desc_map;
>  	struct kvm_host_map msr_bitmap_map;
>  
>  	struct pi_desc *pi_desc;
> @@ -7666,6 +7666,7 @@ static inline void nested_release_vmcs12(struct vcpu_vmx *vmx)
>  				  vmx->nested.cached_vmcs12, 0, VMCS12_SIZE);
>  
>  	kvm_vcpu_unmap(&vmx->nested.virtual_apic_map);
> +	kvm_vcpu_unmap(&vmx->nested.pi_desc_map);
>  	kvm_vcpu_unmap(&vmx->nested.msr_bitmap_map);
>  
>  	vmx->nested.current_vmptr = -1ull;
> @@ -7698,14 +7699,9 @@ static void free_nested(struct vcpu_vmx *vmx)
>  		vmx->nested.apic_access_page = NULL;
>  	}
>  	kvm_vcpu_unmap(&vmx->nested.virtual_apic_map);
> -	if (vmx->nested.pi_desc_page) {
> -		kunmap(vmx->nested.pi_desc_page);
> -		kvm_release_page_dirty(vmx->nested.pi_desc_page);
> -		vmx->nested.pi_desc_page = NULL;
> -		vmx->nested.pi_desc = NULL;
> -	}
> -
> +	kvm_vcpu_unmap(&vmx->nested.pi_desc_map);
>  	kvm_vcpu_unmap(&vmx->nested.msr_bitmap_map);
> +	vmx->nested.pi_desc = NULL;
>  
>  	free_loaded_vmcs(&vmx->nested.vmcs02);
>  }
> @@ -10278,24 +10274,16 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu,
>  	}
>  
>  	if (nested_cpu_has_posted_intr(vmcs12)) {
> -		if (vmx->nested.pi_desc_page) { /* shouldn't happen */
> -			kunmap(vmx->nested.pi_desc_page);
> -			kvm_release_page_dirty(vmx->nested.pi_desc_page);
> -			vmx->nested.pi_desc_page = NULL;
> +		map = &vmx->nested.pi_desc_map;
> +
> +		if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->posted_intr_desc_addr), map)) {
> +			vmx->nested.pi_desc =
> +				(struct pi_desc *)(((void *)map->kaddr) +
> +				offset_in_page(vmcs12->posted_intr_desc_addr));
> +			vmcs_write64(POSTED_INTR_DESC_ADDR, pfn_to_hpa(map->pfn) +
> +							    offset_in_page(vmcs12->posted_intr_desc_addr));
>  		}
> -		page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->posted_intr_desc_addr);
> -		if (is_error_page(page))
> -			return;
> -		vmx->nested.pi_desc_page = page;
> -		vmx->nested.pi_desc = kmap(vmx->nested.pi_desc_page);
> -		vmx->nested.pi_desc =
> -			(struct pi_desc *)((void *)vmx->nested.pi_desc +
> -			(unsigned long)(vmcs12->posted_intr_desc_addr &
> -			(PAGE_SIZE - 1)));
> -		vmcs_write64(POSTED_INTR_DESC_ADDR,
> -			page_to_phys(vmx->nested.pi_desc_page) +
> -			(unsigned long)(vmcs12->posted_intr_desc_addr &
> -			(PAGE_SIZE - 1)));
> +
>  	}
>  	if (nested_vmx_prepare_msr_bitmap(vcpu, vmcs12))
>  		vmcs_set_bits(CPU_BASED_VM_EXEC_CONTROL,
> @@ -11893,13 +11881,6 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>  		kvm_release_page_dirty(vmx->nested.apic_access_page);
>  		vmx->nested.apic_access_page = NULL;
>  	}
> -	if (vmx->nested.pi_desc_page) {
> -		kunmap(vmx->nested.pi_desc_page);
> -		kvm_release_page_dirty(vmx->nested.pi_desc_page);
> -		vmx->nested.pi_desc_page = NULL;
> -		vmx->nested.pi_desc = NULL;
> -	}
> -
>  	/*
>  	 * We are now running in L2, mmu_notifier will force to reload the
>  	 * page's hpa for L2 vmcs. Need to reload it for L1 before entering L1.
> 

Re: [PATCH 00/10] KVM/X86: Handle guest memory that does not have a struct page

[Paolo Bonzini]

On 21/02/2018 18:47, KarimAllah Ahmed wrote:
> For the most part, KVM can handle guest memory that does not have a struct
> page (i.e. not directly managed by the kernel). However, There are a few places
> in the code, specially in the nested code, that does not support that.
> 
> Patch 1, 2, and 3 avoid the mapping and unmapping all together and just
> directly use kvm_guest_read and kvm_guest_write.
> 
> Patch 4 introduces a new guest mapping interface that encapsulate all the
> bioler plate code that is needed to map and unmap guest memory. It also
> supports guest memory without "struct page".
> 
> Patch 5, 6, 7, 8, 9, and 10 switch most of the offending code in VMX and hyperv
> to use the new guest mapping API.
> 
> This patch series is the first set of fixes. Handling SVM and APIC-access page
> will be handled in a different patch series.

I like the patches and the new API.  However, I'm a bit less convinced
about the caching aspect; keeping a page pinned is not the nicest thing
with respect (for example) to memory hot-unplug.

Since you're basically reinventing kmap_high, or alternatively
(depending on your background) xc_map_foreign_pages, it's not surprising
that memremap is slow.  How slow is it really (as seen e.g. with
vmexit.flat running in L1, on EC2 compared to vanilla KVM)?

Perhaps you can keep some kind of per-CPU cache of the last N remapped
pfns?  This cache would sit between memremap and __kvm_map_gfn and it
would be completely transparent to the layer below since it takes raw
pfns.  This removes the need to store the memslots generation etc.  (If
you go this way please place it in virt/kvm/pfncache.[ch], since
kvm_main.c is already way too big).

Thanks,

Paolo

> KarimAllah Ahmed (10):
>   X86/nVMX: handle_vmon: Read 4 bytes from guest memory instead of
>     map->read->unmap sequence
>   X86/nVMX: handle_vmptrld: Copy the VMCS12 directly from guest memory
>     instead of map->copy->unmap sequence.
>   X86/nVMX: Update the PML table without mapping and unmapping the page
>   KVM: Introduce a new guest mapping API
>   KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap
>   KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page
>   KVM/nVMX: Use kvm_vcpu_map when mapping the posted interrupt
>     descriptor table
>   KVM/X86: Use kvm_vcpu_map in emulator_cmpxchg_emulated
>   KVM/X86: hyperv: Use kvm_vcpu_map in synic_clear_sint_msg_pending
>   KVM/X86: hyperv: Use kvm_vcpu_map in synic_deliver_msg
> 
>  arch/x86/kvm/hyperv.c    |  28 ++++-----
>  arch/x86/kvm/vmx.c       | 144 +++++++++++++++--------------------------------
>  arch/x86/kvm/x86.c       |  13 ++---
>  include/linux/kvm_host.h |  15 +++++
>  virt/kvm/kvm_main.c      |  50 ++++++++++++++++
>  5 files changed, 129 insertions(+), 121 deletions(-)
> 

Re: [PATCH 03/10] X86/nVMX: Update the PML table without mapping and unmapping the page

[Paolo Bonzini]

On 21/02/2018 18:47, KarimAllah Ahmed wrote:
> +		dst = (gpa_t)(((u64 *)vmcs12->pml_address) + vmcs12->guest_pml_index);

This is not a pointer, since it's in the guest.  Please use

  dst = vmcs12->pml_address + sizeof(u64) * vmcs12->guest_pml_index;

(It may also make sense to use kvm_write_guest_page if you prefer).

Thanks,

Paolo

> -		page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->pml_address);
> -		if (is_error_page(page))
> +		if (kvm_write_guest(vcpu->kvm, dst, &gpa, sizeof(gpa)))
>  			return 0;

Re: [PATCH 06/10] KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page

[Sean Christopherson]

On Thu, Apr 12, 2018 at 04:38:39PM +0200, Paolo Bonzini wrote:
> On 21/02/2018 18:47, KarimAllah Ahmed wrote:
> > +
> > +		if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->virtual_apic_page_addr), map))
> > +			vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, gfn_to_gpa(map->pfn));
> 
> This should in principle be pfn_to_hpa.  However, pfn_to_hpa is unused;
> let's just remove it and do "<< PAGE_SHIFT".  Unlike gfn_to_gpa, where
> in principle the shift could be different, pfn_to_hpa is *by definition*
> a shift left by PAGE_SHIFT.

Any reason not to use PFN_PHYS instead of the handcoded shift?
 
> Paolo

Re: [PATCH 06/10] KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page

[Paolo Bonzini]

On 12/04/2018 19:57, Sean Christopherson wrote:
> On Thu, Apr 12, 2018 at 04:38:39PM +0200, Paolo Bonzini wrote:
>> On 21/02/2018 18:47, KarimAllah Ahmed wrote:
>>> +
>>> +		if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->virtual_apic_page_addr), map))
>>> +			vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, gfn_to_gpa(map->pfn));
>>
>> This should in principle be pfn_to_hpa.  However, pfn_to_hpa is unused;
>> let's just remove it and do "<< PAGE_SHIFT".  Unlike gfn_to_gpa, where
>> in principle the shift could be different, pfn_to_hpa is *by definition*
>> a shift left by PAGE_SHIFT.
> 
> Any reason not to use PFN_PHYS instead of the handcoded shift?

That works too of course.  It's just not used much (or at all?) in KVM.

Paolo

Re: [PATCH 00/10] KVM/X86: Handle guest memory that does not have a struct page

[Raslan, KarimAllah]

On Thu, 2018-04-12 at 16:59 +0200, Paolo Bonzini wrote:
> On 21/02/2018 18:47, KarimAllah Ahmed wrote:
> > 
> > For the most part, KVM can handle guest memory that does not have a struct
> > page (i.e. not directly managed by the kernel). However, There are a few places
> > in the code, specially in the nested code, that does not support that.
> > 
> > Patch 1, 2, and 3 avoid the mapping and unmapping all together and just
> > directly use kvm_guest_read and kvm_guest_write.
> > 
> > Patch 4 introduces a new guest mapping interface that encapsulate all the
> > bioler plate code that is needed to map and unmap guest memory. It also
> > supports guest memory without "struct page".
> > 
> > Patch 5, 6, 7, 8, 9, and 10 switch most of the offending code in VMX and hyperv
> > to use the new guest mapping API.
> > 
> > This patch series is the first set of fixes. Handling SVM and APIC-access page
> > will be handled in a different patch series.
> 
> I like the patches and the new API.  However, I'm a bit less convinced
> about the caching aspect; keeping a page pinned is not the nicest thing
> with respect (for example) to memory hot-unplug.
> 
> Since you're basically reinventing kmap_high, or alternatively
> (depending on your background) xc_map_foreign_pages, it's not surprising
> that memremap is slow.  How slow is it really (as seen e.g. with
> vmexit.flat running in L1, on EC2 compared to vanilla KVM)?

I have not actually compared EC2 vs vanilla but I compared between the 
version with cached vs no-cache (both in EC2 setup). The one that 
cached the mappings was an order of magnitude better. For example, 
booting an Ubuntu L2 guest with QEMU took around 10-13 seconds with 
this caching and it took over 5 minutes without the caching.

I will test with vanilla KVM and post the results.

> 
> Perhaps you can keep some kind of per-CPU cache of the last N remapped
> pfns?  This cache would sit between memremap and __kvm_map_gfn and it
> would be completely transparent to the layer below since it takes raw
> pfns.  This removes the need to store the memslots generation etc.  (If
> you go this way please place it in virt/kvm/pfncache.[ch], since
> kvm_main.c is already way too big).

Yup, that sounds like a good idea. I actually already implemented some 
sort of a per-CPU mapping pool in order to reduce the overhead when
the vCPU is over-committed. I will clean this and post as you
suggested.

> 
> Thanks,
> 
> Paolo
> 
> > 
> > KarimAllah Ahmed (10):
> >   X86/nVMX: handle_vmon: Read 4 bytes from guest memory instead of
> >     map->read->unmap sequence
> >   X86/nVMX: handle_vmptrld: Copy the VMCS12 directly from guest memory
> >     instead of map->copy->unmap sequence.
> >   X86/nVMX: Update the PML table without mapping and unmapping the page
> >   KVM: Introduce a new guest mapping API
> >   KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap
> >   KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page
> >   KVM/nVMX: Use kvm_vcpu_map when mapping the posted interrupt
> >     descriptor table
> >   KVM/X86: Use kvm_vcpu_map in emulator_cmpxchg_emulated
> >   KVM/X86: hyperv: Use kvm_vcpu_map in synic_clear_sint_msg_pending
> >   KVM/X86: hyperv: Use kvm_vcpu_map in synic_deliver_msg
> > 
> >  arch/x86/kvm/hyperv.c    |  28 ++++-----
> >  arch/x86/kvm/vmx.c       | 144 +++++++++++++++--------------------------------
> >  arch/x86/kvm/x86.c       |  13 ++---
> >  include/linux/kvm_host.h |  15 +++++
> >  virt/kvm/kvm_main.c      |  50 ++++++++++++++++
> >  5 files changed, 129 insertions(+), 121 deletions(-)
> > 
> 
> 
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