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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 9/23/18 7:33 PM, George Dunlap wrote:
> Sorry, looks like this may not have gone through.
>  -G
> On Thu, Sep 20, 2018 at 5:08 PM George Dunlap <george.dunlap@xxxxxxxxxx> 
> wrote:
>>
>> On Thu, Sep 20, 2018 at 4:53 PM Razvan Cojocaru
>> <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>
>>> On 9/20/18 2:34 PM, George Dunlap wrote:
>>>>> +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.
>>>
>>> Do you mean HVMOP_altp2m_set_suppress_ve here, or am I misunderstanding
>>> your comment? I'm happy to do the exact modifications you've requested
>>> above but I'm afraid I don't follow the HVMOP_altp2m_vcpu_enable_notify
>>> comment.
>>
>> At the moment, it appears that you can call
>> HVMOP_altp2m_vcpu_enable_notify to set a vcpu's veinfo_gfn even if the
>> hardware or the p2m mode doesn't support #VE.  It would be better if
>> callers got the -EOPNOTSUPP error at that point, rather than having
>> vcpu_enable_notify succeed and then having set_suppress_ve fail.
>>
>> To be clear, I'm not requiring that as a clean-up, I'm just saying
>> that such a change would be an improvement (which you may consider
>> doing if you wish).

Undestood, thank you for the clarification! We'll follow up with a patch
on that.

I have in the meantime sent out a patch with the requested altp2m
cleanup (code moved to p2m.c and checks moved to p2m-ept.c. I haven't
seen a reply to my question about whether this is what you meant
(perhaps it got lost as well?), so I hope I've understood your request
correctly.


Thanks,
Razvan

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