Re: [Xen-devel] [PATCH 02/11] VMX: implement suppress #VE.

[Ed White <edmund.h.white@xxxxxxxxx>]

On 01/12/2015 09:45 AM, Ed White wrote:
> On 01/12/2015 08:43 AM, Andrew Cooper wrote:
>> On 09/01/15 21:26, Ed White wrote:
>>> In preparation for selectively enabling hardware #VE in a later patch,
>>> set suppress #VE on all EPTE's on #VE-capable hardware.
>>>
>>> Suppress #VE should always be the default condition for two reasons:
>>> it is generally not safe to deliver #VE into a guest unless that guest
>>> has been modified to receive it; and even then for most EPT violations only
>>> the hypervisor is able to handle the violation.
>>>
>>> Signed-off-by: Ed White <edmund.h.white@xxxxxxxxx>
>>> ---
>>>  xen/arch/x86/mm/p2m-ept.c         | 34 +++++++++++++++++++++++++++++++++-
>>>  xen/include/asm-x86/hvm/vmx/vmx.h |  1 +
>>>  2 files changed, 34 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>>> index eb8b5f9..2b9f07c 100644
>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>> @@ -41,7 +41,7 @@
>>>  #define is_epte_superpage(ept_entry)    ((ept_entry)->sp)
>>>  static inline bool_t is_epte_valid(ept_entry_t *e)
>>>  {
>>> -    return (e->epte != 0 && e->sa_p2mt != p2m_invalid);
>>> +    return (e->valid != 0 && e->sa_p2mt != p2m_invalid);
>>>  }
>>>  
>>>  /* returns : 0 for success, -errno otherwise */
>>> @@ -194,6 +194,19 @@ static int ept_set_middle_entry(struct p2m_domain 
>>> *p2m, ept_entry_t *ept_entry)
>>>  
>>>      ept_entry->r = ept_entry->w = ept_entry->x = 1;
>>>  
>>> +    /* Disable #VE on all entries */ 
>>> +    if ( cpu_has_vmx_virt_exceptions )
>>> +    {
>>> +        ept_entry_t *table = __map_domain_page(pg);
>>> +
>>> +        for ( int i = 0; i < EPT_PAGETABLE_ENTRIES; i++ )
>>
>> Style - please declare i in the upper scope, and it should be unsigned.
>>
>>> +            table[i].suppress_ve = 1;
>>> +
>>> +        unmap_domain_page(table);
>>> +
>>> +        ept_entry->suppress_ve = 1;
>>> +    }
>>> +
>>>      return 1;
>>>  }
>>>  
>>> @@ -243,6 +256,10 @@ static int ept_split_super_page(struct p2m_domain 
>>> *p2m, ept_entry_t *ept_entry,
>>>          epte->sp = (level > 1);
>>>          epte->mfn += i * trunk;
>>>          epte->snp = (iommu_enabled && iommu_snoop);
>>> +
>>> +        if ( cpu_has_vmx_virt_exceptions )
>>> +            epte->suppress_ve = 1;
>>> +
>>>          ASSERT(!epte->rsvd1);
>>>  
>>>          ept_p2m_type_to_flags(epte, epte->sa_p2mt, epte->access);
>>> @@ -753,6 +770,9 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long 
>>> gfn, mfn_t mfn,
>>>          ept_p2m_type_to_flags(&new_entry, p2mt, p2ma);
>>>      }
>>>  
>>> +    if ( cpu_has_vmx_virt_exceptions )
>>> +        new_entry.suppress_ve = 1;
>>> +
>>>      rc = atomic_write_ept_entry(ept_entry, new_entry, target);
>>>      if ( unlikely(rc) )
>>>          old_entry.epte = 0;
>>> @@ -1069,6 +1089,18 @@ int ept_p2m_init(struct p2m_domain *p2m)
>>>      /* set EPT page-walk length, now it's actual walk length - 1, i.e. 3 */
>>>      ept->ept_wl = 3;
>>>  
>>> +    /* Disable #VE on all entries */
>>> +    if ( cpu_has_vmx_virt_exceptions )
>>> +    {
>>> +        ept_entry_t *table =
>>> +            map_domain_page(pagetable_get_pfn(p2m_get_pagetable(p2m)));
>>> +
>>> +        for ( int i = 0; i < EPT_PAGETABLE_ENTRIES; i++ )
>>> +            table[i].suppress_ve = 1;
>>
>> Is it safe setting SVE on an entry which is not known to be a superpage
>> or not present?  The manual states that the bit is ignored in this case,
>> but I am concerned that, as with SVE, this bit will suddenly gain
>> meaning in the future.
>>
> 
> It is safe to do this. Never say never, but I am aware of no plans to
> overload this bit, and I would know. Unless you feel strongly about it,
> I would prefer to leave this as-is, since changing it would make the code
> more complex.
> 

One point that I should have clarified yesterday: the SDM says the bit is
ignored for a non-terminal present entry; the bit is not ignored for
non-present entries, which is why I have to set all the SVE bits in a new
page -- my lazy EPTE copying algorithm wouldn't work otherwise because all
the zero entries would generate #VE.

Ed

>>> +
>>> +        unmap_domain_page(table);
>>> +    }
>>> +
>>>      if ( !zalloc_cpumask_var(&ept->synced_mask) )
>>>          return -ENOMEM;
>>>  
>>> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h 
>>> b/xen/include/asm-x86/hvm/vmx/vmx.h
>>> index 8bae195..70fee74 100644
>>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>>> @@ -49,6 +49,7 @@ typedef union {
>>>          suppress_ve :   1;  /* bit 63 - suppress #VE */
>>>      };
>>>      u64 epte;
>>> +    u64 valid       :   63; /* entire EPTE except suppress #VE bit */
>>
>> I am not sure 'valid' is a sensible name here.  As it is only used in
>> is_epte_valid(), might it be better to just use ->epte and a bitmask for
>> everything other than the #VE bit?
>>
> 
> This seemed more in the style of the code I was changing, but I can do it
> as you suggest.
> 
> Ed
> 
>>>  } ept_entry_t;
>>>  
>>>  typedef struct {
>>
>>

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