Re: [Xen-devel] [PATCH] x86/altp2m: clean up p2m_{get/set}_suppress_ve()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 9/24/18 6:25 PM, George Dunlap wrote:
> On 09/23/2018 06:04 PM, Razvan Cojocaru wrote:
>> Move p2m_{get/set}_suppress_ve() to p2m.c, replace incorrect
>> ASSERT() in p2m-pt.c (since a guest can run in shadow mode even on
>> a system with virt exceptions, which would trigger the ASSERT()),
>> and move the VMX-isms (cpu_has_vmx_virt_exceptions checks) to
>> p2m_ept_{get/set}_entry().
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> 
> Thanks for the clean up.  Two realtively minor comments...
> 
>> @@ -931,6 +942,16 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
>>      mfn_t mfn = INVALID_MFN;
>>      struct ept_data *ept = &p2m->ept;
>>  
>> +    if ( sve )
>> +    {
>> +        if ( !cpu_has_vmx_virt_exceptions )
>> +            return INVALID_MFN;
>> +
>> +        /* #VE should be enabled for this vcpu. */
>> +        if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
>> +            return INVALID_MFN;
>> +    }
> 
> Is there a good reason to return error her rather than just putting '1'
> in the sve location, like the p2m_pt.c version of this function does?

First, thanks for the review!

The p2m_pt.c version can only return 1 because that's the only value
that bit can have on #VE-incapable hardware. For the
cpu_has_vmx_virt_exceptions, that assumption does hold, however in a
scenario where:

1. we enable #VE and set that bit to 0;
2. we disable #VE (so gfn_eq(vcpu_altp2m(current).veinfo_gfn,
INVALID_GFN) == true);
3. we call ept_get_entry();

setting it to 1 would be misleading, since it's value is now really 0.

I do agree that returning INVALID_MFN is no necessarily more informative.

Alternatively, I could simply remove the checks here altogether. If
!cpu_has_vmx_virt_exceptions then ept_get_entry() should fail anyway, so
the bit will just remain 1 and thus the following code:

 999     if ( is_epte_valid(ept_entry) )
1000     {
1001         *t = p2m_recalc_type(recalc || ept_entry->recalc,
1002                              ept_entry->sa_p2mt, p2m, gfn);
1003         *a = ept_entry->access;
1004         if ( sve )
1005             *sve = ept_entry->suppress_ve;

should automatically do the right thing. And if, in the above scenario,
the bit became 0, we return that value properly as well.

Would that be better?

>> +int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
>> +                        unsigned int altp2m_idx)
>> +{
>> +    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;
>> +
>> +    /* #VE should be enabled for this vcpu. */
>> +    if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
>> +        return -ENXIO;
> 
> What's the purpose of checking for this here, if we don't check for this
> in p2m_set_suppress_ve()?

Sorry, I seem to have accidentally left that in p2m_get_suppress_ve() -
I'll delete it from here and leave it only in ept_set_entry(). It's
pointless to have it duplicated here.


Thanks,
Razvan

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