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

["Teddy Astie" <teddy.astie@xxxxxxxxxx>]

Le 28/08/2025 à 14:16, Jan Beulich a écrit :
> 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.
>

I agree, I should reword that so that it's less confusing.

>> 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?
>

We could, but we would end up having to modify more code to go there i.e
replacing all op.* with op->* which I tried to avoid here.

> 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.
>

A part of the goal here is to split the ABI part from the hypercall
logic; such as it gets possible to have ABI that don't need to refer to
guest addresses (either virtual or physical); and could help dealing
with current 32-bits vs 64-bits ABIs. All without duplicating the main
hypercall logic.

>> @@ -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()?
>

That doesn't need to.

> Jan

Teddy


Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech