Re: [Xen-devel] [PATCH v5] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Aug 16, 2017 at 6:43 AM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 16.08.2017 15:32, Tamas K Lengyel wrote:
>>
>> On Wed, Aug 16, 2017 at 12:07 AM, Razvan Cojocaru
>> <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>
>>> On 08/16/2017 02:16 AM, Tamas K Lengyel wrote:
>>>>
>>>> On Tue, Aug 15, 2017 at 2:06 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>>>
>>>>>>>> On 14.08.17 at 17:53, <tamas@xxxxxxxxxxxxx> wrote:
>>>>>>
>>>>>> On Tue, Aug 8, 2017 at 2:27 AM, Alexandru Isaila
>>>>>> <aisaila@xxxxxxxxxxxxxxx> wrote:
>>>>>>>
>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>>>> @@ -155,6 +155,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>>>>>>>           /* Fallthrough to permission check. */
>>>>>>>       case 4:
>>>>>>>       case 2:
>>>>>>> +        if ( currd->arch.monitor.guest_request_userspace_enabled &&
>>>>>>> +            eax == __HYPERVISOR_hvm_op &&
>>>>>>> +            (mode == 8 ? regs->rdi : regs->ebx) ==
>>>>>>> HVMOP_guest_request_vm_event )
>>>>>>> +            break;
>>>>>>> +
>>>>>>
>>>>>>
>>>>>> So the CPL check happens after the monitor check, which means this
>>>>>> will trigger regardless if the hypercall is coming from userspace or
>>>>>> kernelspace. Since the monitor option specifically says userspace,
>>>>>> this should probably get moved into the block where CPL was checked.
>>>>>
>>>>>
>>>>> What difference would this make? For CPL0 the hypercall is
>>>>> permitted anyway, and for CPL > 0 we specifically want to bypass
>>>>> the CPL check. Or are you saying you want to restrict the new
>>>>> check to just CPL3?
>>>>>
>>>>
>>>> Yes, according to the name of this monitor option this should only
>>>> trigger a vm_event when the hypercall is coming from CPL3. However,
>>>> the way it is implemented right now I see that this monitor option
>>>> actually requires the other one to be enabled too. By itself this
>>>> monitor option will not work. So I would also like to ask that the
>>>> check in xen/common/monitor.c, if ( d->monitor.guest_request_enabled
>>>> ), to be extended to be: if ( d->monitor.guest_request_enabled ||
>>>> d->monitor.guest_request_userspace_enabled )
>>>
>>>
>>> The option does not trigger anything. Its job is to allow guest requests
>>> coming from userspace (via VMCALLs). And not to _only_ allow these for
>>> userspace, but to allow them coming from userspace _as_well_.
>>>
>>> The current version of the patch, if I've not missed something, does not
>>> require d->monitor.guest_request_enabled to be true to work (the options
>>> can be toggled independently).
>>>
>>> The new function is meant to be called at any time, independent of
>>> enabling / disabling the guest request vm_event (i.e. it only controls
>>> its behaviour once it's enabled). So guest_request_userspace_enabled
>>> should not be used as synonym for guest_request_enabled.
>>>
>>
>> Hi Razvan,
>> so while monitor.guest_request_enabled can indeed be toggled
>> independently, if it is not set, how would the vm_event actually be
>> sent for monitor.guest_request_userspace_enabled? AFAICT it wouldn't
>> because the code responsible for sending the vm_event is gated only on
>> monitor.guest_request_enabled.
>
>
> Hello, indeed it wouldn't.
>
> This new option only control what happens _if_ monitor.guest_request_enabled
> is true. While monitor.guest_request_enabled is false, it has no effect. As
> soon as guest_request_enabled gets toggled on again, the behaviour will be
> the one that has been previously set by using
> guest_request_userspace_enabled.
>
>> And as for this new option being an extended version of
>> guest_request_enabled in the sense that it would allow both userspace
>> _and_ kernelspace, we can do that, but then the name of it is
>> misleading.
>
>
> The naming of the function did get shuffled around a bit. :)
>
> Would xc_monitor_allow_guest_userspace_event() be more appropriate?
>
> Also, if having a separate function feels counter-intuitive, we could also
> simply add a bool parameter to xc_monitor_guest_request(), for example:
>
> int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
>                              bool enable, bool sync,
>                              bool allow_userspace);
>
> and use that to toggle guest_request_userspace_enabled.

In light of the conversion here I have to say I like this option the most.

>
> Thanks,
> Razvan
>
> P.S. Is replying only to me (as opposed to a "reply all") intentional?

Oops, no, I've re-added xen-devel :)

Tamas

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