| Message ID | 20220324140139.5899-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: drop hypercall function tables | expand |
On 24.03.22 15:01, Juergen Gross wrote: > The entry point used for the vcpu_op hypercall on Arm is different > from the one on x86 today, as some of the common sub-ops are not > supported on Arm. The Arm specific handler filters out the not > supported sub-ops and then calls the common handler. This leads to the > weird call hierarchy: > > do_arm_vcpu_op() > do_vcpu_op() > arch_do_vcpu_op() > > Clean this up by renaming do_vcpu_op() to common_vcpu_op() and > arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one > of above calls can be avoided without restricting any potential > future use of common sub-ops for Arm. > > Signed-off-by: Juergen Gross <jgross@suse.com> > Reviewed-by: Julien Grall <jgrall@amazon.com> There is still an Ack missing for x86 side. Jan already said he isn't happy to give one, but won't Nack it. Roger, Andrew, any comments for this patch? It is blocking further cleanup patches to go in (patches 2 and 4 of this series). Juergen > --- > V4: > - don't remove HYPERCALL_ARM() > V4.1: > - add missing cf_check (Andrew Cooper) > V5: > - use v instead of current (Julien Grall) > --- > xen/arch/arm/domain.c | 15 ++++++++------- > xen/arch/arm/include/asm/hypercall.h | 2 -- > xen/arch/arm/traps.c | 2 +- > xen/arch/x86/domain.c | 12 ++++++++---- > xen/arch/x86/include/asm/hypercall.h | 2 +- > xen/arch/x86/x86_64/domain.c | 18 +++++++++++++----- > xen/common/compat/domain.c | 15 ++++++--------- > xen/common/domain.c | 12 ++++-------- > xen/include/xen/hypercall.h | 2 +- > 9 files changed, 42 insertions(+), 38 deletions(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 8110c1df86..2f8eaab7b5 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -1079,23 +1079,24 @@ void arch_dump_domain_info(struct domain *d) > } > > > -long do_arm_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) > +long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) > { > + struct domain *d = current->domain; > + struct vcpu *v; > + > + if ( (v = domain_vcpu(d, vcpuid)) == NULL ) > + return -ENOENT; > + > switch ( cmd ) > { > case VCPUOP_register_vcpu_info: > case VCPUOP_register_runstate_memory_area: > - return do_vcpu_op(cmd, vcpuid, arg); > + return common_vcpu_op(cmd, v, arg); > default: > return -EINVAL; > } > } > > -long arch_do_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) > -{ > - return -ENOSYS; > -} > - > void arch_dump_vcpu_info(struct vcpu *v) > { > gic_dump_info(v); > diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h > index 39d2e7889d..fac4d60f17 100644 > --- a/xen/arch/arm/include/asm/hypercall.h > +++ b/xen/arch/arm/include/asm/hypercall.h > @@ -4,8 +4,6 @@ > #include <public/domctl.h> /* for arch_do_domctl */ > int do_arm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg); > > -long do_arm_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg); > - > long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d, > XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl); > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 43f30747cf..e906bb4a89 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -1380,7 +1380,7 @@ static arm_hypercall_t arm_hypercall_table[] = { > #endif > HYPERCALL(multicall, 2), > HYPERCALL(platform_op, 1), > - HYPERCALL_ARM(vcpu_op, 3), > + HYPERCALL(vcpu_op, 3), > HYPERCALL(vm_assist, 2), > #ifdef CONFIG_ARGO > HYPERCALL(argo_op, 5), > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index a5048ed654..d566fc82b4 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -1489,11 +1489,15 @@ int arch_vcpu_reset(struct vcpu *v) > return 0; > } > > -long > -arch_do_vcpu_op( > - int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) > +long cf_check do_vcpu_op(int cmd, unsigned int vcpuid, > + XEN_GUEST_HANDLE_PARAM(void) arg) > { > long rc = 0; > + struct domain *d = current->domain; > + struct vcpu *v; > + > + if ( (v = domain_vcpu(d, vcpuid)) == NULL ) > + return -ENOENT; > > switch ( cmd ) > { > @@ -1545,7 +1549,7 @@ arch_do_vcpu_op( > } > > default: > - rc = -ENOSYS; > + rc = common_vcpu_op(cmd, v, arg); > break; > } > > diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h > index 61bf897147..d6daa7e4cb 100644 > --- a/xen/arch/x86/include/asm/hypercall.h > +++ b/xen/arch/x86/include/asm/hypercall.h > @@ -145,7 +145,7 @@ compat_physdev_op( > XEN_GUEST_HANDLE_PARAM(void) arg); > > extern int > -arch_compat_vcpu_op( > +compat_common_vcpu_op( > int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg); > > extern int cf_check compat_mmuext_op( > diff --git a/xen/arch/x86/x86_64/domain.c b/xen/arch/x86/x86_64/domain.c > index c46dccc25a..9c559aa3ea 100644 > --- a/xen/arch/x86/x86_64/domain.c > +++ b/xen/arch/x86/x86_64/domain.c > @@ -12,11 +12,15 @@ > CHECK_vcpu_get_physid; > #undef xen_vcpu_get_physid > > -int > -arch_compat_vcpu_op( > - int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) > +int cf_check > +compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) > { > - int rc = -ENOSYS; > + int rc; > + struct domain *d = current->domain; > + struct vcpu *v; > + > + if ( (v = domain_vcpu(d, vcpuid)) == NULL ) > + return -ENOENT; > > switch ( cmd ) > { > @@ -55,7 +59,11 @@ arch_compat_vcpu_op( > } > > case VCPUOP_get_physid: > - rc = arch_do_vcpu_op(cmd, v, arg); > + rc = do_vcpu_op(cmd, vcpuid, arg); > + break; > + > + default: > + rc = compat_common_vcpu_op(cmd, v, arg); > break; > } > > diff --git a/xen/common/compat/domain.c b/xen/common/compat/domain.c > index afae27eeba..1119534679 100644 > --- a/xen/common/compat/domain.c > +++ b/xen/common/compat/domain.c > @@ -38,15 +38,12 @@ CHECK_vcpu_hvm_context; > > #endif > > -int cf_check compat_vcpu_op( > - int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) > +int compat_common_vcpu_op(int cmd, struct vcpu *v, > + XEN_GUEST_HANDLE_PARAM(void) arg) > { > - struct domain *d = current->domain; > - struct vcpu *v; > int rc = 0; > - > - if ( (v = domain_vcpu(d, vcpuid)) == NULL ) > - return -ENOENT; > + struct domain *d = current->domain; > + unsigned int vcpuid = v->vcpu_id; > > switch ( cmd ) > { > @@ -103,7 +100,7 @@ int cf_check compat_vcpu_op( > case VCPUOP_stop_singleshot_timer: > case VCPUOP_register_vcpu_info: > case VCPUOP_send_nmi: > - rc = do_vcpu_op(cmd, vcpuid, arg); > + rc = common_vcpu_op(cmd, v, arg); > break; > > case VCPUOP_get_runstate_info: > @@ -134,7 +131,7 @@ int cf_check compat_vcpu_op( > } > > default: > - rc = arch_compat_vcpu_op(cmd, v, arg); > + rc = -ENOSYS; > break; > } > > diff --git a/xen/common/domain.c b/xen/common/domain.c > index 351029f8b2..70747c02e6 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1570,15 +1570,11 @@ int default_initialise_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) > return rc; > } > > -long cf_check do_vcpu_op( > - int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) > +long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) > { > - struct domain *d = current->domain; > - struct vcpu *v; > long rc = 0; > - > - if ( (v = domain_vcpu(d, vcpuid)) == NULL ) > - return -ENOENT; > + struct domain *d = v->domain; > + unsigned int vcpuid = v->vcpu_id; > > switch ( cmd ) > { > @@ -1750,7 +1746,7 @@ long cf_check do_vcpu_op( > } > > default: > - rc = arch_do_vcpu_op(cmd, v, arg); > + rc = -ENOSYS; > break; > } > > diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h > index a1b6575976..81aae7a662 100644 > --- a/xen/include/xen/hypercall.h > +++ b/xen/include/xen/hypercall.h > @@ -110,7 +110,7 @@ do_vcpu_op( > > struct vcpu; > extern long > -arch_do_vcpu_op(int cmd, > +common_vcpu_op(int cmd, > struct vcpu *v, > XEN_GUEST_HANDLE_PARAM(void) arg); >
On Thu, Mar 24, 2022 at 03:01:31PM +0100, Juergen Gross wrote: > The entry point used for the vcpu_op hypercall on Arm is different > from the one on x86 today, as some of the common sub-ops are not > supported on Arm. The Arm specific handler filters out the not > supported sub-ops and then calls the common handler. This leads to the > weird call hierarchy: > > do_arm_vcpu_op() > do_vcpu_op() > arch_do_vcpu_op() > > Clean this up by renaming do_vcpu_op() to common_vcpu_op() and > arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one > of above calls can be avoided without restricting any potential > future use of common sub-ops for Arm. Wouldn't it be more natural to have do_vcpu_op() contain the common code (AFAICT handlers for VCPUOP_register_{vcpu_info,runstate_memory_area}) and then everything else handled by the x86 arch_do_vcpu_op() handler? I find the common prefix misleading, as not all the VCPUOP hypercalls are available to all the architectures. Thanks, Roger.
On 27.06.22 12:28, Roger Pau Monné wrote: > On Thu, Mar 24, 2022 at 03:01:31PM +0100, Juergen Gross wrote: >> The entry point used for the vcpu_op hypercall on Arm is different >> from the one on x86 today, as some of the common sub-ops are not >> supported on Arm. The Arm specific handler filters out the not >> supported sub-ops and then calls the common handler. This leads to the >> weird call hierarchy: >> >> do_arm_vcpu_op() >> do_vcpu_op() >> arch_do_vcpu_op() >> >> Clean this up by renaming do_vcpu_op() to common_vcpu_op() and >> arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one >> of above calls can be avoided without restricting any potential >> future use of common sub-ops for Arm. > > Wouldn't it be more natural to have do_vcpu_op() contain the common > code (AFAICT handlers for > VCPUOP_register_{vcpu_info,runstate_memory_area}) and then everything > else handled by the x86 arch_do_vcpu_op() handler? > > I find the common prefix misleading, as not all the VCPUOP hypercalls > are available to all the architectures. This would end up in Arm suddenly supporting the sub-ops it doesn't (want to) support today. Otherwise it would make no sense that Arm has a dedicated entry for this hypercall. The "common" just wants to express that the code is common. I'm open for a better suggestion, though. :-) Juergen
On Mon, Jun 27, 2022 at 12:40:41PM +0200, Juergen Gross wrote: > On 27.06.22 12:28, Roger Pau Monné wrote: > > On Thu, Mar 24, 2022 at 03:01:31PM +0100, Juergen Gross wrote: > > > The entry point used for the vcpu_op hypercall on Arm is different > > > from the one on x86 today, as some of the common sub-ops are not > > > supported on Arm. The Arm specific handler filters out the not > > > supported sub-ops and then calls the common handler. This leads to the > > > weird call hierarchy: > > > > > > do_arm_vcpu_op() > > > do_vcpu_op() > > > arch_do_vcpu_op() > > > > > > Clean this up by renaming do_vcpu_op() to common_vcpu_op() and > > > arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one > > > of above calls can be avoided without restricting any potential > > > future use of common sub-ops for Arm. > > > > Wouldn't it be more natural to have do_vcpu_op() contain the common > > code (AFAICT handlers for > > VCPUOP_register_{vcpu_info,runstate_memory_area}) and then everything > > else handled by the x86 arch_do_vcpu_op() handler? > > > > I find the common prefix misleading, as not all the VCPUOP hypercalls > > are available to all the architectures. > > This would end up in Arm suddenly supporting the sub-ops it doesn't > (want to) support today. Otherwise it would make no sense that Arm has > a dedicated entry for this hypercall. My preference would be for a common do_vcpu_op() that just contains handlers for VCPUOP_register_{vcpu_info,runstate_memory_area} and then an empty arch_ handler for Arm, and everything else moved to the x86 arch_ handler. That obviously implies some code churn, but results in a cleaner implementation IMO. Also has the nice benefit of removing unreachable code from the Arm build, which is also a MISRA-C rule. > The "common" just wants to express that the code is common. I'm open > for a better suggestion, though. :-) Right, it lives in common/ anyway, so there's not a much better name. Thanks, Roger.
On 27.06.22 13:02, Roger Pau Monné wrote: > On Mon, Jun 27, 2022 at 12:40:41PM +0200, Juergen Gross wrote: >> On 27.06.22 12:28, Roger Pau Monné wrote: >>> On Thu, Mar 24, 2022 at 03:01:31PM +0100, Juergen Gross wrote: >>>> The entry point used for the vcpu_op hypercall on Arm is different >>>> from the one on x86 today, as some of the common sub-ops are not >>>> supported on Arm. The Arm specific handler filters out the not >>>> supported sub-ops and then calls the common handler. This leads to the >>>> weird call hierarchy: >>>> >>>> do_arm_vcpu_op() >>>> do_vcpu_op() >>>> arch_do_vcpu_op() >>>> >>>> Clean this up by renaming do_vcpu_op() to common_vcpu_op() and >>>> arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one >>>> of above calls can be avoided without restricting any potential >>>> future use of common sub-ops for Arm. >>> >>> Wouldn't it be more natural to have do_vcpu_op() contain the common >>> code (AFAICT handlers for >>> VCPUOP_register_{vcpu_info,runstate_memory_area}) and then everything >>> else handled by the x86 arch_do_vcpu_op() handler? >>> >>> I find the common prefix misleading, as not all the VCPUOP hypercalls >>> are available to all the architectures. >> >> This would end up in Arm suddenly supporting the sub-ops it doesn't >> (want to) support today. Otherwise it would make no sense that Arm has >> a dedicated entry for this hypercall. > > My preference would be for a common do_vcpu_op() that just contains > handlers for VCPUOP_register_{vcpu_info,runstate_memory_area} and then > an empty arch_ handler for Arm, and everything else moved to the x86 > arch_ handler. That obviously implies some code churn, but results in > a cleaner implementation IMO. I'd be fine with that. I did it in V2 of the (then secret) series, and Jan replied: I'm afraid I don't agree with this movement. I could see things getting moved that are purely PV (on the assumption that no new PV ports will appear), but anything that's also usable by PVH / HVM ought to be usable in principle also by Arm or other PV-free ports. Juergen
On Mon, Jun 27, 2022 at 01:08:11PM +0200, Juergen Gross wrote: > On 27.06.22 13:02, Roger Pau Monné wrote: > > On Mon, Jun 27, 2022 at 12:40:41PM +0200, Juergen Gross wrote: > > > On 27.06.22 12:28, Roger Pau Monné wrote: > > > > On Thu, Mar 24, 2022 at 03:01:31PM +0100, Juergen Gross wrote: > > > > > The entry point used for the vcpu_op hypercall on Arm is different > > > > > from the one on x86 today, as some of the common sub-ops are not > > > > > supported on Arm. The Arm specific handler filters out the not > > > > > supported sub-ops and then calls the common handler. This leads to the > > > > > weird call hierarchy: > > > > > > > > > > do_arm_vcpu_op() > > > > > do_vcpu_op() > > > > > arch_do_vcpu_op() > > > > > > > > > > Clean this up by renaming do_vcpu_op() to common_vcpu_op() and > > > > > arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one > > > > > of above calls can be avoided without restricting any potential > > > > > future use of common sub-ops for Arm. > > > > > > > > Wouldn't it be more natural to have do_vcpu_op() contain the common > > > > code (AFAICT handlers for > > > > VCPUOP_register_{vcpu_info,runstate_memory_area}) and then everything > > > > else handled by the x86 arch_do_vcpu_op() handler? > > > > > > > > I find the common prefix misleading, as not all the VCPUOP hypercalls > > > > are available to all the architectures. > > > > > > This would end up in Arm suddenly supporting the sub-ops it doesn't > > > (want to) support today. Otherwise it would make no sense that Arm has > > > a dedicated entry for this hypercall. > > > > My preference would be for a common do_vcpu_op() that just contains > > handlers for VCPUOP_register_{vcpu_info,runstate_memory_area} and then > > an empty arch_ handler for Arm, and everything else moved to the x86 > > arch_ handler. That obviously implies some code churn, but results in > > a cleaner implementation IMO. > > I'd be fine with that. > > I did it in V2 of the (then secret) series, and Jan replied: > > I'm afraid I don't agree with this movement. I could see things getting > moved that are purely PV (on the assumption that no new PV ports will > appear), but anything that's also usable by PVH / HVM ought to be usable > in principle also by Arm or other PV-free ports. I see. My opinion is that when other ports need those functions they should be pulled out of arch code into common code, until then it just adds confusion to have them in common code. I will take a look at the current patch, as we need to make progress on this. Thanks, Roger.
On 27.06.22 13:35, Roger Pau Monné wrote: > On Mon, Jun 27, 2022 at 01:08:11PM +0200, Juergen Gross wrote: >> On 27.06.22 13:02, Roger Pau Monné wrote: >>> On Mon, Jun 27, 2022 at 12:40:41PM +0200, Juergen Gross wrote: >>>> On 27.06.22 12:28, Roger Pau Monné wrote: >>>>> On Thu, Mar 24, 2022 at 03:01:31PM +0100, Juergen Gross wrote: >>>>>> The entry point used for the vcpu_op hypercall on Arm is different >>>>>> from the one on x86 today, as some of the common sub-ops are not >>>>>> supported on Arm. The Arm specific handler filters out the not >>>>>> supported sub-ops and then calls the common handler. This leads to the >>>>>> weird call hierarchy: >>>>>> >>>>>> do_arm_vcpu_op() >>>>>> do_vcpu_op() >>>>>> arch_do_vcpu_op() >>>>>> >>>>>> Clean this up by renaming do_vcpu_op() to common_vcpu_op() and >>>>>> arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one >>>>>> of above calls can be avoided without restricting any potential >>>>>> future use of common sub-ops for Arm. >>>>> >>>>> Wouldn't it be more natural to have do_vcpu_op() contain the common >>>>> code (AFAICT handlers for >>>>> VCPUOP_register_{vcpu_info,runstate_memory_area}) and then everything >>>>> else handled by the x86 arch_do_vcpu_op() handler? >>>>> >>>>> I find the common prefix misleading, as not all the VCPUOP hypercalls >>>>> are available to all the architectures. >>>> >>>> This would end up in Arm suddenly supporting the sub-ops it doesn't >>>> (want to) support today. Otherwise it would make no sense that Arm has >>>> a dedicated entry for this hypercall. >>> >>> My preference would be for a common do_vcpu_op() that just contains >>> handlers for VCPUOP_register_{vcpu_info,runstate_memory_area} and then >>> an empty arch_ handler for Arm, and everything else moved to the x86 >>> arch_ handler. That obviously implies some code churn, but results in >>> a cleaner implementation IMO. >> >> I'd be fine with that. >> >> I did it in V2 of the (then secret) series, and Jan replied: >> >> I'm afraid I don't agree with this movement. I could see things getting >> moved that are purely PV (on the assumption that no new PV ports will >> appear), but anything that's also usable by PVH / HVM ought to be usable >> in principle also by Arm or other PV-free ports. > > I see. My opinion is that when other ports need those functions they > should be pulled out of arch code into common code, until then it just > adds confusion to have them in common code. > > I will take a look at the current patch, as we need to make progress > on this. Thanks for that. Juergen
On Thu, Mar 24, 2022 at 03:01:31PM +0100, Juergen Gross wrote: > The entry point used for the vcpu_op hypercall on Arm is different > from the one on x86 today, as some of the common sub-ops are not > supported on Arm. The Arm specific handler filters out the not > supported sub-ops and then calls the common handler. This leads to the > weird call hierarchy: > > do_arm_vcpu_op() > do_vcpu_op() > arch_do_vcpu_op() > > Clean this up by renaming do_vcpu_op() to common_vcpu_op() and > arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one > of above calls can be avoided without restricting any potential > future use of common sub-ops for Arm. > > Signed-off-by: Juergen Gross <jgross@suse.com> > Reviewed-by: Julien Grall <jgrall@amazon.com> Acked-by: Roger Pau Monné <roger.pau@cirtrix.com> From an x86 only PoV I prefer the previous arrangement (do_vcpu_op() -> arch_do_vcpu_op()), but this approach seems to be better for Arm, so that's a reasonable argument for changing it. My preference would have been to move the handling of hypercalls not used by Arm into arch_do_vcpu_op() for x86, but that's also not widely liked. > --- > V4: > - don't remove HYPERCALL_ARM() > V4.1: > - add missing cf_check (Andrew Cooper) > V5: > - use v instead of current (Julien Grall) > --- > xen/arch/arm/domain.c | 15 ++++++++------- > xen/arch/arm/include/asm/hypercall.h | 2 -- > xen/arch/arm/traps.c | 2 +- > xen/arch/x86/domain.c | 12 ++++++++---- > xen/arch/x86/include/asm/hypercall.h | 2 +- > xen/arch/x86/x86_64/domain.c | 18 +++++++++++++----- > xen/common/compat/domain.c | 15 ++++++--------- > xen/common/domain.c | 12 ++++-------- > xen/include/xen/hypercall.h | 2 +- > 9 files changed, 42 insertions(+), 38 deletions(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 8110c1df86..2f8eaab7b5 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -1079,23 +1079,24 @@ void arch_dump_domain_info(struct domain *d) > } > > > -long do_arm_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) > +long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) > { > + struct domain *d = current->domain; > + struct vcpu *v; > + > + if ( (v = domain_vcpu(d, vcpuid)) == NULL ) > + return -ENOENT; My preference (here and in x86) code would be to do the initialization at definition, and then just check for v here, ie: struct vcpu *v = domain_vcpu(d, vcpuid); if ( !v ) return -ENOENT; But that's just my taste. Thanks, Roger.
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 8110c1df86..2f8eaab7b5 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -1079,23 +1079,24 @@ void arch_dump_domain_info(struct domain *d) } -long do_arm_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) +long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) { + struct domain *d = current->domain; + struct vcpu *v; + + if ( (v = domain_vcpu(d, vcpuid)) == NULL ) + return -ENOENT; + switch ( cmd ) { case VCPUOP_register_vcpu_info: case VCPUOP_register_runstate_memory_area: - return do_vcpu_op(cmd, vcpuid, arg); + return common_vcpu_op(cmd, v, arg); default: return -EINVAL; } } -long arch_do_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) -{ - return -ENOSYS; -} - void arch_dump_vcpu_info(struct vcpu *v) { gic_dump_info(v); diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h index 39d2e7889d..fac4d60f17 100644 --- a/xen/arch/arm/include/asm/hypercall.h +++ b/xen/arch/arm/include/asm/hypercall.h @@ -4,8 +4,6 @@ #include <public/domctl.h> /* for arch_do_domctl */ int do_arm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg); -long do_arm_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg); - long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d, XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl); diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 43f30747cf..e906bb4a89 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1380,7 +1380,7 @@ static arm_hypercall_t arm_hypercall_table[] = { #endif HYPERCALL(multicall, 2), HYPERCALL(platform_op, 1), - HYPERCALL_ARM(vcpu_op, 3), + HYPERCALL(vcpu_op, 3), HYPERCALL(vm_assist, 2), #ifdef CONFIG_ARGO HYPERCALL(argo_op, 5), diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index a5048ed654..d566fc82b4 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1489,11 +1489,15 @@ int arch_vcpu_reset(struct vcpu *v) return 0; } -long -arch_do_vcpu_op( - int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) +long cf_check do_vcpu_op(int cmd, unsigned int vcpuid, + XEN_GUEST_HANDLE_PARAM(void) arg) { long rc = 0; + struct domain *d = current->domain; + struct vcpu *v; + + if ( (v = domain_vcpu(d, vcpuid)) == NULL ) + return -ENOENT; switch ( cmd ) { @@ -1545,7 +1549,7 @@ arch_do_vcpu_op( } default: - rc = -ENOSYS; + rc = common_vcpu_op(cmd, v, arg); break; } diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h index 61bf897147..d6daa7e4cb 100644 --- a/xen/arch/x86/include/asm/hypercall.h +++ b/xen/arch/x86/include/asm/hypercall.h @@ -145,7 +145,7 @@ compat_physdev_op( XEN_GUEST_HANDLE_PARAM(void) arg); extern int -arch_compat_vcpu_op( +compat_common_vcpu_op( int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg); extern int cf_check compat_mmuext_op( diff --git a/xen/arch/x86/x86_64/domain.c b/xen/arch/x86/x86_64/domain.c index c46dccc25a..9c559aa3ea 100644 --- a/xen/arch/x86/x86_64/domain.c +++ b/xen/arch/x86/x86_64/domain.c @@ -12,11 +12,15 @@ CHECK_vcpu_get_physid; #undef xen_vcpu_get_physid -int -arch_compat_vcpu_op( - int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) +int cf_check +compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) { - int rc = -ENOSYS; + int rc; + struct domain *d = current->domain; + struct vcpu *v; + + if ( (v = domain_vcpu(d, vcpuid)) == NULL ) + return -ENOENT; switch ( cmd ) { @@ -55,7 +59,11 @@ arch_compat_vcpu_op( } case VCPUOP_get_physid: - rc = arch_do_vcpu_op(cmd, v, arg); + rc = do_vcpu_op(cmd, vcpuid, arg); + break; + + default: + rc = compat_common_vcpu_op(cmd, v, arg); break; } diff --git a/xen/common/compat/domain.c b/xen/common/compat/domain.c index afae27eeba..1119534679 100644 --- a/xen/common/compat/domain.c +++ b/xen/common/compat/domain.c @@ -38,15 +38,12 @@ CHECK_vcpu_hvm_context; #endif -int cf_check compat_vcpu_op( - int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) +int compat_common_vcpu_op(int cmd, struct vcpu *v, + XEN_GUEST_HANDLE_PARAM(void) arg) { - struct domain *d = current->domain; - struct vcpu *v; int rc = 0; - - if ( (v = domain_vcpu(d, vcpuid)) == NULL ) - return -ENOENT; + struct domain *d = current->domain; + unsigned int vcpuid = v->vcpu_id; switch ( cmd ) { @@ -103,7 +100,7 @@ int cf_check compat_vcpu_op( case VCPUOP_stop_singleshot_timer: case VCPUOP_register_vcpu_info: case VCPUOP_send_nmi: - rc = do_vcpu_op(cmd, vcpuid, arg); + rc = common_vcpu_op(cmd, v, arg); break; case VCPUOP_get_runstate_info: @@ -134,7 +131,7 @@ int cf_check compat_vcpu_op( } default: - rc = arch_compat_vcpu_op(cmd, v, arg); + rc = -ENOSYS; break; } diff --git a/xen/common/domain.c b/xen/common/domain.c index 351029f8b2..70747c02e6 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1570,15 +1570,11 @@ int default_initialise_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) return rc; } -long cf_check do_vcpu_op( - int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) +long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) { - struct domain *d = current->domain; - struct vcpu *v; long rc = 0; - - if ( (v = domain_vcpu(d, vcpuid)) == NULL ) - return -ENOENT; + struct domain *d = v->domain; + unsigned int vcpuid = v->vcpu_id; switch ( cmd ) { @@ -1750,7 +1746,7 @@ long cf_check do_vcpu_op( } default: - rc = arch_do_vcpu_op(cmd, v, arg); + rc = -ENOSYS; break; } diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h index a1b6575976..81aae7a662 100644 --- a/xen/include/xen/hypercall.h +++ b/xen/include/xen/hypercall.h @@ -110,7 +110,7 @@ do_vcpu_op( struct vcpu; extern long -arch_do_vcpu_op(int cmd, +common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
The entry point used for the vcpu_op hypercall on Arm is different from the one on x86 today, as some of the common sub-ops are not supported on Arm. The Arm specific handler filters out the not supported sub-ops and then calls the common handler. This leads to the weird call hierarchy: do_arm_vcpu_op() do_vcpu_op() arch_do_vcpu_op() Clean this up by renaming do_vcpu_op() to common_vcpu_op() and arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one of above calls can be avoided without restricting any potential future use of common sub-ops for Arm. Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Julien Grall <jgrall@amazon.com> --- V4: - don't remove HYPERCALL_ARM() V4.1: - add missing cf_check (Andrew Cooper) V5: - use v instead of current (Julien Grall) --- xen/arch/arm/domain.c | 15 ++++++++------- xen/arch/arm/include/asm/hypercall.h | 2 -- xen/arch/arm/traps.c | 2 +- xen/arch/x86/domain.c | 12 ++++++++---- xen/arch/x86/include/asm/hypercall.h | 2 +- xen/arch/x86/x86_64/domain.c | 18 +++++++++++++----- xen/common/compat/domain.c | 15 ++++++--------- xen/common/domain.c | 12 ++++-------- xen/include/xen/hypercall.h | 2 +- 9 files changed, 42 insertions(+), 38 deletions(-)