Re: [Xen-devel] [PATCH] x86/altp2m: clean up p2m_{get/set}_suppress_ve()
On 09/24/2018 04:45 PM, Razvan Cojocaru wrote:
> 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
>>
>> 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 = >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?
Sorry, yes, that's what I intended, although I certainly wasn't clear.
What I meant was, the pt version of get_entry() would succeed and return
something sensible even on non-#VE-capable hardware; why should the ept
version not do the same thing?
So yes, I think just removing the checks and letting the actual value
from the p2m entry be passed back is the right thing to do.
>
>>> +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.
Great, thanks.
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/altp2m: clean up p2m_{get/set}_suppress_ve()
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
>
> 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 = >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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/altp2m: clean up p2m_{get/set}_suppress_ve()
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
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 = >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?
> +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()?
Everything else looks good, thanks!
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/altp2m: clean up p2m_{get/set}_suppress_ve()
On Sun, Sep 23, 2018 at 11:05 AM 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
Acked-by: Tamas K Lengyel
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
