Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1698174 > unrolled thread

Re: [PATCH v2 1/3] kvm: svm: Add support for additional SVM NPF error codes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/3] kvm: svm: Add support for additional SVM NPF error  codes Paolo Bonzini <pbonzini@redhat.com> - 2017-07-27 18:40 +0200
    Re: [PATCH v2 1/3] kvm: svm: Add support for additional SVM NPF error  codes Paolo Bonzini <pbonzini@redhat.com> - 2017-07-31 17:50 +0200
      Re: [PATCH v2 1/3] kvm: svm: Add support for additional SVM NPF  error codes Paolo Bonzini <pbonzini@redhat.com> - 2017-07-31 22:10 +0200
        Re: [PATCH v2 1/3] kvm: svm: Add support for additional SVM NPF error  codes Paolo Bonzini <pbonzini@redhat.com> - 2017-08-02 12:50 +0200
          Re: [PATCH v2 1/3] kvm: svm: Add support for additional SVM NPF error  codes Paolo Bonzini <pbonzini@redhat.com> - 2017-08-04 16:10 +0200

#1698174 — Re: [PATCH v2 1/3] kvm: svm: Add support for additional SVM NPF error codes

On 23/11/2016 18:01, Brijesh Singh wrote:
>  
> +	/*
> +	 * Before emulating the instruction, check if the error code
> +	 * was due to a RO violation while translating the guest page.
> +	 * This can occur when using nested virtualization with nested
> +	 * paging in both guests. If true, we simply unprotect the page
> +	 * and resume the guest.
> +	 *
> +	 * Note: AMD only (since it supports the PFERR_GUEST_PAGE_MASK used
> +	 *       in PFERR_NEXT_GUEST_PAGE)
> +	 */
> +	if (error_code == PFERR_NESTED_GUEST_PAGE) {
> +		kvm_mmu_unprotect_page(vcpu->kvm, gpa_to_gfn(cr2));
> +		return 1;
> +	}


What happens if L1 is mapping some memory that is read only in L0?  That
is, the L1 nested page tables make it read-write, but the L0 shadow
nested page tables make it read-only.

Accessing it would cause an NPF, and then my guess is that the L1 guest
would loop on the failing instruction instead of just dropping the write.

Paolo

[toc] | [next] | [standalone]

#1700140

On 31/07/2017 15:30, Brijesh Singh wrote:
> Hi Paolo,
> 
> On 07/27/2017 11:27 AM, Paolo Bonzini wrote:
>> On 23/11/2016 18:01, Brijesh Singh wrote:
>>>   +    /*
>>> +     * Before emulating the instruction, check if the error code
>>> +     * was due to a RO violation while translating the guest page.
>>> +     * This can occur when using nested virtualization with nested
>>> +     * paging in both guests. If true, we simply unprotect the page
>>> +     * and resume the guest.
>>> +     *
>>> +     * Note: AMD only (since it supports the PFERR_GUEST_PAGE_MASK used
>>> +     *       in PFERR_NEXT_GUEST_PAGE)
>>> +     */
>>> +    if (error_code == PFERR_NESTED_GUEST_PAGE) {
>>> +        kvm_mmu_unprotect_page(vcpu->kvm, gpa_to_gfn(cr2));
>>> +        return 1;
>>> +    }
>>
>>
>> What happens if L1 is mapping some memory that is read only in L0?  That
>> is, the L1 nested page tables make it read-write, but the L0 shadow
>> nested page tables make it read-only.
>>
>> Accessing it would cause an NPF, and then my guess is that the L1 guest
>> would loop on the failing instruction instead of just dropping the write.
>>
> 
> 
> Not sure if I am able to follow your use case. Could you please explain me
> in bit detail.
> 
> The purpose of the code above was really for when we resume from the L2 guest
> back to the L1 guest. The L1 page tables are marked RO when in the L2 guest
> (for shadow paging) as I recall, so when we come back to the L1 guest, it can
> get a fault since its page tables are not marked writeable at L0 as they
> need to be.

There can be different cases where an L0->L2 shadow nested page table is
marked read only, in particular when a page is read only in L1's nested
page tables.  If such a page is accessed by L2 while walking page tables
it will cause a nested page fault (page table walks are write accesses).
 However, after kvm_mmu_unprotect_page you will get another page fault,
and again in an endless stream.

Instead, emulation would have caused a nested page fault vmexit, I think.

Paolo

[toc] | [prev] | [next] | [standalone]

#1700363 — Re: [PATCH v2 1/3] kvm: svm: Add support for additional SVM NPF error codes

> > There can be different cases where an L0->L2 shadow nested page table is
> > marked read only, in particular when a page is read only in L1's nested
> > page tables.  If such a page is accessed by L2 while walking page tables
> > it will cause a nested page fault (page table walks are write accesses).
> >   However, after kvm_mmu_unprotect_page you will get another page fault,
> > and again in an endless stream.
> > 
> > Instead, emulation would have caused a nested page fault vmexit, I think.
> 
> If possible could you please give me some pointer on how to create this use
> case so that we can get definitive answer.
> 
> Looking at the code path is giving me indication that the new code
> (the kvm_mmu_unprotect_page call) only happens if vcpu->arch.mmu_page_fault()
> returns an indication that the instruction should be emulated. I would not
> expect that to be the case scenario you described above since L1 making a page
> read-only (this is a page table for L2) is an error and should result in #NPF
> being injected into L1.

The flow is:

  hardware walks page table; L2 page table points to read only memory
  -> pf_interception (code = 
  -> kvm_handle_page_fault (need_unprotect = false)
  -> kvm_mmu_page_fault
  -> paging64_page_fault (for example)
     -> try_async_pf
        map_writable set to false
     -> paging64_fetch(write_fault = true, map_writable = false, prefault = false)
        -> mmu_set_spte(speculative = false, host_writable = false, write_fault = true)
           -> set_spte
              mmu_need_write_protect returns true
              return true
           write_fault == true -> set emulate = true
           return true
        return true
     return true
  emulate

Without this patch, emulation would have called

  ..._gva_to_gpa_nested
  -> translate_nested_gpa
  -> paging64_gva_to_gpa
  -> paging64_walk_addr
  -> paging64_walk_addr_generic
     set fault (nested_page_fault=true)

and then:

   kvm_propagate_fault
   -> nested_svm_inject_npf_exit

> It's bit hard for me to visualize the code flow and
> figure out exactly how that would happen, but I just tried booting nested
> virtualization and it seem to be working okay.

I don't expect the above to happen when booting a normal guest (usual L1
guests hardly have readonly mappings).

> Is there a kvm-unit-test which I can run to trigger this scenario ? thanks

No, there isn't.

Paolo

> -Brijesh
> 

[toc] | [prev] | [next] | [standalone]

#1701995

On 01/08/2017 15:36, Brijesh Singh wrote:
>>
>> The flow is:
>>
>>    hardware walks page table; L2 page table points to read only memory
>>    -> pf_interception (code =
>>    -> kvm_handle_page_fault (need_unprotect = false)
>>    -> kvm_mmu_page_fault
>>    -> paging64_page_fault (for example)
>>       -> try_async_pf
>>          map_writable set to false
>>       -> paging64_fetch(write_fault = true, map_writable = false,
>> prefault = false)
>>          -> mmu_set_spte(speculative = false, host_writable = false,
>> write_fault = true)
>>             -> set_spte
>>                mmu_need_write_protect returns true
>>                return true
>>             write_fault == true -> set emulate = true
>>             return true
>>          return true
>>       return true
>>    emulate
>>
>> Without this patch, emulation would have called
>>
>>    ..._gva_to_gpa_nested
>>    -> translate_nested_gpa
>>    -> paging64_gva_to_gpa
>>    -> paging64_walk_addr
>>    -> paging64_walk_addr_generic
>>       set fault (nested_page_fault=true)
>>
>> and then:
>>
>>     kvm_propagate_fault
>>     -> nested_svm_inject_npf_exit
>>
> 
> maybe then safer thing would be to qualify the new error_code check with
> !mmu_is_nested(vcpu) or something like that. So that way it would run on
> L1 guest, and not the L2 guest. I believe that would restrict it avoid
> hitting this case. Are you okay with this change ?

Or check "vcpu->arch.mmu.direct_map"?  That would be true when not using
shadow pages.

> IIRC, the main place where this check was valuable was when L1 guest had
> a fault (when coming out of the L2 guest) and emulation was not needed.

How do I measure the effect?  I tried counting the number of emulations,
and any difference from the patch was lost in noise.

Paolo

[toc] | [prev] | [next] | [standalone]

#1704013

On 04/08/2017 02:30, Brijesh Singh wrote:
> 
> 
> On 8/2/17 5:42 AM, Paolo Bonzini wrote:
>> On 01/08/2017 15:36, Brijesh Singh wrote:
>>>> The flow is:
>>>>
>>>>    hardware walks page table; L2 page table points to read only memory
>>>>    -> pf_interception (code =
>>>>    -> kvm_handle_page_fault (need_unprotect = false)
>>>>    -> kvm_mmu_page_fault
>>>>    -> paging64_page_fault (for example)
>>>>       -> try_async_pf
>>>>          map_writable set to false
>>>>       -> paging64_fetch(write_fault = true, map_writable = false,
>>>> prefault = false)
>>>>          -> mmu_set_spte(speculative = false, host_writable = false,
>>>> write_fault = true)
>>>>             -> set_spte
>>>>                mmu_need_write_protect returns true
>>>>                return true
>>>>             write_fault == true -> set emulate = true
>>>>             return true
>>>>          return true
>>>>       return true
>>>>    emulate
>>>>
>>>> Without this patch, emulation would have called
>>>>
>>>>    ..._gva_to_gpa_nested
>>>>    -> translate_nested_gpa
>>>>    -> paging64_gva_to_gpa
>>>>    -> paging64_walk_addr
>>>>    -> paging64_walk_addr_generic
>>>>       set fault (nested_page_fault=true)
>>>>
>>>> and then:
>>>>
>>>>     kvm_propagate_fault
>>>>     -> nested_svm_inject_npf_exit
>>>>
>>> maybe then safer thing would be to qualify the new error_code check with
>>> !mmu_is_nested(vcpu) or something like that. So that way it would run on
>>> L1 guest, and not the L2 guest. I believe that would restrict it avoid
>>> hitting this case. Are you okay with this change ?
>> Or check "vcpu->arch.mmu.direct_map"?  That would be true when not using
>> shadow pages.
> 
> Yes that can be used.

Are you going to send a patch for this?

Paolo

>>> IIRC, the main place where this check was valuable was when L1 guest had
>>> a fault (when coming out of the L2 guest) and emulation was not needed.
>> How do I measure the effect?  I tried counting the number of emulations,
>> and any difference from the patch was lost in noise.
> 
> I think this patch is necessary for functional reasons (not just
> perf), because we added the other patch to look at the GPA and stop
> walking the guest page tables on a NPF.
> 
> The issue I think was that hardware has taken an NPF because the page
> table is marked RO, and it saves the GPA in the VMCB. KVM was then going
> and emulating the instruction and it saw that a GPA was available. But
> that GPA was not the GPA of the instruction it is emulating, since it
> was the GPA of the tablewalk page that had the fault. It was debugged
> that at the time and realized that emulating the instruction was
> unnecessary so we added this new code in there which fixed the
> functional issue and helps perf.
> 
> I don't have any data on how much perf, as I recall it was most
> effective when the L1 guest page tables and L2 nested page tables were
> exactly the same. In that case, it avoided emulations for code that L1
> executes which I think could be as much as one emulation per 4kb code page.
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web