Make these functions work with hypervisor-owned pointer rather than
guest handles, so the function parameters don't have to live in guest memory.
No functional changes intended.
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
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)
{
- 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;
-
if ( op.vector < 0x10 )
return -EINVAL;
@@ -4434,26 +4429,21 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
return rc;
}
-static int hvmop_set_param(
- XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
+static int hvmop_set_param(struct xen_hvm_param op)
{
- struct xen_hvm_param a;
struct domain *d;
int rc;
- if ( copy_from_guest(&a, arg, 1) )
- return -EFAULT;
-
- if ( a.index >= HVM_NR_PARAMS )
+ if ( op.index >= HVM_NR_PARAMS )
return -EINVAL;
- d = rcu_lock_domain_by_any_id(a.domid);
+ d = rcu_lock_domain_by_any_id(op.domid);
if ( d == NULL )
return -ESRCH;
rc = -EINVAL;
if ( is_hvm_domain(d) )
- rc = hvm_set_param(d, a.index, a.value);
+ rc = hvm_set_param(d, op.index, op.value);
rcu_unlock_domain(d);
return rc;
@@ -4544,31 +4534,21 @@ int hvm_get_param(struct domain *d, uint32_t index, uint64_t *value)
return 0;
};
-static int hvmop_get_param(
- XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
+static int hvmop_get_param(struct xen_hvm_param *op)
{
- struct xen_hvm_param a;
struct domain *d;
int rc;
- if ( copy_from_guest(&a, arg, 1) )
- return -EFAULT;
-
- if ( a.index >= HVM_NR_PARAMS )
+ if ( op->index >= HVM_NR_PARAMS )
return -EINVAL;
- d = rcu_lock_domain_by_any_id(a.domid);
+ d = rcu_lock_domain_by_any_id(op->domid);
if ( d == NULL )
return -ESRCH;
rc = -EINVAL;
- if ( is_hvm_domain(d) && !(rc = hvm_get_param(d, a.index, &a.value)) )
- {
- rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
-
- HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64,
- a.index, a.value);
- }
+ if ( is_hvm_domain(d) && !(rc = hvm_get_param(d, op->index, &op->value)) )
+ HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64, a.index, a.value);
rcu_unlock_domain(d);
return rc;
@@ -5046,18 +5026,13 @@ static int compat_altp2m_op(
return rc;
}
-static int hvmop_get_mem_type(
- XEN_GUEST_HANDLE_PARAM(xen_hvm_get_mem_type_t) arg)
+static int hvmop_get_mem_type(struct xen_hvm_get_mem_type *op)
{
- struct xen_hvm_get_mem_type a;
struct domain *d;
p2m_type_t t;
int rc;
- if ( copy_from_guest(&a, arg, 1) )
- return -EFAULT;
-
- d = rcu_lock_domain_by_any_id(a.domid);
+ d = rcu_lock_domain_by_any_id(op->domid);
if ( d == NULL )
return -ESRCH;
@@ -5074,25 +5049,22 @@ static int hvmop_get_mem_type(
* type, not in allocating or unsharing. That'll happen
* on access.
*/
- get_gfn_query_unlocked(d, a.pfn, &t);
+ get_gfn_query_unlocked(d, op->pfn, &t);
if ( p2m_is_mmio(t) )
- a.mem_type = HVMMEM_mmio_dm;
+ op->mem_type = HVMMEM_mmio_dm;
else if ( t == p2m_ioreq_server )
- a.mem_type = HVMMEM_ioreq_server;
+ op->mem_type = HVMMEM_ioreq_server;
else if ( p2m_is_readonly(t) )
- a.mem_type = HVMMEM_ram_ro;
+ op->mem_type = HVMMEM_ram_ro;
else if ( p2m_is_ram(t) )
- a.mem_type = HVMMEM_ram_rw;
+ op->mem_type = HVMMEM_ram_rw;
else if ( p2m_is_pod(t) )
- a.mem_type = HVMMEM_ram_rw;
+ op->mem_type = HVMMEM_ram_rw;
else if ( p2m_is_grant(t) )
- a.mem_type = HVMMEM_ram_rw;
+ op->mem_type = HVMMEM_ram_rw;
else
- a.mem_type = HVMMEM_mmio_dm;
+ op->mem_type = HVMMEM_mmio_dm;
- rc = -EFAULT;
- if ( __copy_to_guest(arg, &a, 1) )
- goto out;
rc = 0;
out:
@@ -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) )
+ rc = -EFAULT;
break;
+ }
case HVMOP_flush_tlbs:
rc = guest_handle_is_null(arg) ? hvmop_flush_tlb_all() : -EINVAL;
break;
case HVMOP_get_mem_type:
- rc = hvmop_get_mem_type(
- guest_handle_cast(arg, xen_hvm_get_mem_type_t));
+ {
+ struct xen_hvm_get_mem_type op;
+
+ if ( copy_from_guest(&op, arg, 1) )
+ {
+ rc = -EFAULT;
+ break;
+ }
+
+ rc = hvmop_get_mem_type(&op);
+
+ if ( !rc && copy_to_guest(arg, &op, 1) )
+ rc = -EFAULT;
break;
+ }
case HVMOP_pagetable_dying:
{
--
2.50.1
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.techOn 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@vates.tech>
> ---
> 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
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@vates.tech>
>> ---
>> 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
© 2016 - 2025 Red Hat, Inc.