Re: [Xen-devel] [PATCH 1/2 for-4.12] xen: introduce VCPUOP_register_runstate_phys_memory_area hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.03.19 at 14:14, <andrii.anisov@xxxxxxxxx> wrote:
> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
> 
> The hypercall employs the same vcpu_register_runstate_memory_area
> structure for the interface, but requires registered area to not
> cross a page boundary.
> 
> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>

While first of all you'll need to settle the dispute with Julien, a
couple of comments on the actual changes you do in case the
approach is to be pursued.

> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -277,29 +277,33 @@ static void ctxt_switch_to(struct vcpu *n)
>  /* Update per-VCPU guest runstate shared memory area (if registered). */
>  static void update_runstate_area(struct vcpu *v)
>  {
> -    void __user *guest_handle = NULL;
> -
>      if ( guest_handle_is_null(runstate_guest(v)) )
>          return;

I think (seeing also patch 2) re-using the handle representation
is a bad idea. If there's a Xen-internal mapping use a plain
pointer. (All comments on Arm code respectively apply to the
x86 version, also [if any] in patch 2.)

> -    if ( VM_ASSIST(v->domain, runstate_update_flag) )
> +    if ( v->runstate_guest_type == RUNSTATE_VADDR )

Patch 2 shows that this wants to be switch().

>      {
> -        guest_handle = &v->runstate_guest.p->state_entry_time + 1;
> -        guest_handle--;
> -        v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
> -        __raw_copy_to_guest(guest_handle,
> -                            (void *)(&v->runstate.state_entry_time + 1) - 1, 
> 1);
> -        smp_wmb();
> -    }
> +        void __user *guest_handle = NULL;
> +        if ( VM_ASSIST(v->domain, runstate_update_flag) )
> +        {
> +            guest_handle = &v->runstate_guest.p->state_entry_time + 1;
> +            guest_handle--;
> +            v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
> +            __raw_copy_to_guest(guest_handle,
> +                                (void *)(&v->runstate.state_entry_time + 1) 
> - 1,
> +                                1);
> +            smp_wmb();
> +        }
>  
> -    __copy_to_guest(runstate_guest(v), &v->runstate, 1);
> +        __copy_to_guest(runstate_guest(v), &v->runstate, 1);
>  
> -    if ( guest_handle )
> -    {
> -        v->runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
> -        smp_wmb();
> -        __raw_copy_to_guest(guest_handle,
> -                            (void *)(&v->runstate.state_entry_time + 1) - 1, 
> 1);
> +        if ( guest_handle )
> +        {
> +            v->runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
> +            smp_wmb();
> +            __raw_copy_to_guest(guest_handle,
> +                                (void *)(&v->runstate.state_entry_time + 1) 
> - 1,
> +                                1);
> +        }
>      }

There looks to be an "else" missing here (or "default:" once changed
to switch()), which you have on the x86 side. I'm unconvinced it
should be "return true" there though (in which case having nothing
here would indeed have been correct) - ASSERT_UNREACHABLE()
would much rather seem appropriate.

> @@ -1535,6 +1536,12 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case VCPUOP_register_runstate_phys_memory_area:
> +    {
> +        rc = -ENOSYS;
> +        break;
> +    }

Stray braces and bad use of -ENOSYS (should be e.g. -EOPNOTSUPP).

> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -235,6 +235,22 @@ struct vcpu_register_time_memory_area {
>  typedef struct vcpu_register_time_memory_area 
> vcpu_register_time_memory_area_t;
>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>  
> +/*
> + * Register a shared memory area from which the guest may obtain its own
> + * runstate information without needing to execute a hypercall.
> + * Notes:
> + *  1. The registered address must be guest's physical address.
> + *  2. The registered runstate area should not cross page boundary.
> + *  3. Only one shared area may be registered per VCPU. The shared area is
> + *     updated by the hypervisor each time the VCPU is scheduled. Thus
> + *     runstate.state will always be RUNSTATE_running and
> + *     runstate.state_entry_time will indicate the system time at which the
> + *     VCPU was last scheduled to run.
> + * @extra_arg == pointer to vcpu_register_runstate_memory_area structure.

I don't think re-use of that structure is appropriate to represent
a physical address.

> + */
> +#define VCPUOP_register_runstate_phys_memory_area 14
> +
> +
>  #endif /* __XEN_PUBLIC_VCPU_H__ */

Stray double blank lines.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -163,6 +163,13 @@ struct vcpu
>      void            *sched_priv;    /* scheduler-specific data */
>  
>      struct vcpu_runstate_info runstate;
> +
> +    enum {
> +        RUNSTATE_NONE = 0,
> +        RUNSTATE_PADDR = 1,
> +        RUNSTATE_VADDR = 2,
> +    } runstate_guest_type;

I can see that putting the new field here nicely groups with the
other related fields. But this should be weighed against the
introduction of a new padding hole, as opposed to fitting the
field into a existing padding space. An abbreviated version of
the reasoning should be put in the description.

Jan


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