Re: [RFC PATCH 1/9] x86/hvm: Use direct structures instead of guest handles

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.08.2025 17:25, Teddy Astie wrote:
> Make these functions work with hypervisor-owned pointer rather than
> guest handles, so the function parameters don't have to live in guest memory.

This is odd to read - the function parameters (arguments) didn't live in
guest memory before either.

> No functional changes intended.
> 
> Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c | 126 +++++++++++++++++++++++------------------
>  1 file changed, 70 insertions(+), 56 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 56c7de3977..8bf59c63fe 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4142,19 +4142,14 @@ static int hvmop_flush_tlb_all(void)
>      return paging_flush_tlb(NULL) ? 0 : -ERESTART;
>  }
>  
> -static int hvmop_set_evtchn_upcall_vector(
> -    XEN_GUEST_HANDLE_PARAM(xen_hvm_evtchn_upcall_vector_t) uop)
> +static int hvmop_set_evtchn_upcall_vector(xen_hvm_evtchn_upcall_vector_t op)

Please can we avoid passing structures by value?

More generally: This one-by-one adjustment is what I'd really like to avoid
with any new interface. It would be far better if ...

>  {
> -    xen_hvm_evtchn_upcall_vector_t op;
>      struct domain *d = current->domain;
>      struct vcpu *v;
>  
>      if ( !is_hvm_domain(d) )
>          return -EINVAL;
>  
> -    if ( copy_from_guest(&op, uop, 1) )
> -        return -EFAULT;

... copy_from_guest() could transparantly handle both cases (virtual and
physical addresses being used). And yes, this would exclude an "everying in
registers" approach.

> @@ -5115,28 +5087,70 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>      switch ( op )
>      {
>      case HVMOP_set_evtchn_upcall_vector:
> -        rc = hvmop_set_evtchn_upcall_vector(
> -            guest_handle_cast(arg, xen_hvm_evtchn_upcall_vector_t));
> +    {
> +        struct xen_hvm_evtchn_upcall_vector op;
> +
> +        if ( copy_from_guest(&op, arg, 1) )
> +        {
> +            rc = -EFAULT;
> +            break;
> +        }
> +
> +        rc = hvmop_set_evtchn_upcall_vector(op);
>          break;
> +    }
>      
>      case HVMOP_set_param:
> -        rc = hvmop_set_param(
> -            guest_handle_cast(arg, xen_hvm_param_t));
> +    {
> +        struct xen_hvm_param op;
> +        
> +        if ( copy_from_guest(&op, arg, 1) )
> +        {
> +            rc = -EFAULT;
> +            break;
> +        }
> +
> +        rc = hvmop_set_param(op);
>          break;
> +    }
>  
>      case HVMOP_get_param:
> -        rc = hvmop_get_param(
> -            guest_handle_cast(arg, xen_hvm_param_t));
> +    {
> +        struct xen_hvm_param op;
> +        
> +        if ( copy_from_guest(&op, arg, 1) )
> +        {
> +            rc = -EFAULT;
> +            break;
> +        }
> +
> +        rc = hvmop_get_param(&op);
> +
> +        if ( !rc && copy_to_guest(arg, &op, 1) )

Why would the original __copy_to_guest() need to change to copy_to_guest()?

Jan