Re: [Xen-devel] [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 9/20/18 2:34 PM, George Dunlap wrote:
> On 09/03/2018 04:48 PM, Adrian Pop wrote:
>> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
>> domain to change the value of the #VE suppress bit for a page.
>>
>> Add a libxc wrapper for invoking this hvmop.
>>
>> Signed-off-by: Adrian Pop <apop@xxxxxxxxxxxxxxx>
>> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>> Acked-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> 
> Sorry I haven't had a chance to weigh in on this one yet.  A couple fo
> comments...
> 
> 
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index e1522a0b75..4d49025cbe 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -495,6 +495,61 @@ void arch_p2m_set_access_required(struct domain *d, 
>> bool access_required)
>>      }
>>  }
>>  
>> +/*
>> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
>> + */
>> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>> +                        unsigned int altp2m_idx)
> 
> This should clearly be in p2m.c, and...
> 
>> +{
>> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>> +    struct p2m_domain *ap2m = NULL;
>> +    struct p2m_domain *p2m;
>> +    mfn_t mfn;
>> +    p2m_access_t a;
>> +    p2m_type_t t;
>> +    int rc;
>> +
>> +    if ( !cpu_has_vmx_virt_exceptions )
>> +        return -EOPNOTSUPP;
> 
> We should avoid Intel-specific checks in common code.
> 
> In fact, this is wrong, because you can choose to run a guest in shadow
> mode even on a system with virt exceptions -- in which case you'd
> trigger the ASSERT() in p2m-pt.c:p2m_pt_set_entry().
> 
> Probably what should happen is that we should move the vmx check into
> p2m-ept.c:p2m_ept_set_entry(), and replace the ASSERT(sve = 0) in
> p2m-pt.c:p2m_pt_set_entry() with "if ( sve != 0 ) return -ENOTSUPP;".
> 
> Although what should probably *really* happen is that
> `HVMOP_altp2m_vcpu_enable_notify` should fail with -EOPNOTSUPP instead
> of silently succeeding.
> 
> Everything else looks good.
> 
> I realize this has been checked in already; no need to revert if you can
> send a follow-up patch in the next couple of (business) days.

Thanks for the review! I'll submit a follow up patch as soon as possible.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel