Re: [Xen-devel] [PATCH V6 4/5] xen/mm: Clean up pfec handling in gva_to_gfn

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.01.16 at 08:30, <huaitong.han@xxxxxxxxx> wrote:
> At the moment, the pfec argument to gva_to_gfn has two functions:
> 
> * To inform guest_walk what kind of access is happenind
> 
> * As a value to pass back into the guest in the event of a fault.
> 
> Unfortunately this is not quite treated consistently: the hvm_fetch_*
> function will "pre-clear" the PFEC_insn_fetch flag before calling
> gva_to_gfn; meaning guest_walk doesn't actually know whether a given
> access is an instruction fetch or not.  This works now, but will cause
> issues when pkeys are introduced, since guest_walk will need to know
> whether an access is an instruction fetch even if it doesn't return
> PFEC_insn_fetch.
> 
> Fix this by making a clean separation for in and out functionalities
> of the pfec argument:
> 
> 1. Always pass in the access type to gva_to_gfn
> 
> 2. Filter out inappropriate access flags before returning from gva_to_gfn.
> 
> (The PFEC_insn_fetch flag should only be passed to the guest if either NX or
> SMEP is enabled.  See Intel 64 Developer's Manual, Volume 3, Section 4.7.)

As mentioned in various other contexts not so long ago - no SDM
section number please (as they tend to change); use section titles
instead (also below in code comments).

> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Signed-off-by: Huaitong Han <huaitong.han@xxxxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c           |  8 ++------
>  xen/arch/x86/mm/hap/guest_walk.c | 10 +++++++++-
>  xen/arch/x86/mm/shadow/multi.c   |  6 ++++++

You failed to Cc the shadow code maintainer (now added).

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4432,11 +4432,9 @@ enum hvm_copy_result hvm_copy_from_guest_virt(
>  enum hvm_copy_result hvm_fetch_from_guest_virt(
>      void *buf, unsigned long vaddr, int size, uint32_t pfec)
>  {
> -    if ( hvm_nx_enabled(current) || hvm_smep_enabled(current) )
> -        pfec |= PFEC_insn_fetch;
>      return __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_from_guest | HVMCOPY_fault | HVMCOPY_virt,
> -                      PFEC_page_present | pfec);
> +                      PFEC_page_present | PFEC_insn_fetch | pfec);
>  }
>  
>  enum hvm_copy_result hvm_copy_to_guest_virt_nofault(
> @@ -4458,11 +4456,9 @@ enum hvm_copy_result hvm_copy_from_guest_virt_nofault(
>  enum hvm_copy_result hvm_fetch_from_guest_virt_nofault(
>      void *buf, unsigned long vaddr, int size, uint32_t pfec)
>  {
> -    if ( hvm_nx_enabled(current) || hvm_smep_enabled(current) )
> -        pfec |= PFEC_insn_fetch;
>      return __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_from_guest | HVMCOPY_no_fault | HVMCOPY_virt,
> -                      PFEC_page_present | pfec);
> +                      PFEC_page_present | PFEC_insn_fetch | pfec);
>  }
>  
>  unsigned long copy_to_user_hvm(void *to, const void *from, unsigned int len)

This part
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

> --- a/xen/arch/x86/mm/hap/guest_walk.c
> +++ b/xen/arch/x86/mm/hap/guest_walk.c
> @@ -82,7 +82,7 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PAGING_LEVELS)(
>      if ( !top_page )
>      {
>          pfec[0] &= ~PFEC_page_present;
> -        return INVALID_GFN;
> +        goto out_tweak_pfec;
>      }
>      top_mfn = _mfn(page_to_mfn(top_page));
>  
> @@ -139,6 +139,14 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PAGING_LEVELS)(
>      if ( missing & _PAGE_SHARED )
>          pfec[0] = PFEC_page_shared;
>  
> +out_tweak_pfec:

To avoid corrupting the context in patches, labels should be indented
by at least one space please.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel