Message ID | 8b5c0b8f-b243-47b1-2ce9-f315d5c7138c@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86: (re-)wire two VCPUOP_* for 32-bit guests | expand |
On Thu, Sep 29, 2022 at 11:51:03AM +0200, Jan Beulich wrote: > With the "inversion" of VCPUOP handling, processing arch-specific ones > first, the forwarding of this sub-op from the (common) compat handler to > (common) non-compat one did no longer have the intended effect. It now > needs forwarding between the arch-specific handlers. > > Fixes: 8a96c0ea7999 ("xen: move do_vcpu_op() to arch specific code") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> This seems prone to errors, I wonder if we should add a note to do_vcpu_op() to notice that hypercalls handled there need to be explicitly forwarded from compat_vcpu_op. I've also noticed that compat_common_vcpu_op() forwards VCPUOP_set_singleshot_timer to do_vcpu_op(), but that seems to be an useless jump, shouldn't it forward directly to common_vcpu_op()? Thanks, Roger.
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Subject: [PATCH 1/2][4.17] x86: re-connect VCPUOP_send_nmi for 32-bit > guests > > With the "inversion" of VCPUOP handling, processing arch-specific ones > first, the forwarding of this sub-op from the (common) compat handler to > (common) non-compat one did no longer have the intended effect. It now > needs forwarding between the arch-specific handlers. > > Fixes: 8a96c0ea7999 ("xen: move do_vcpu_op() to arch specific code") > Signed-off-by: Jan Beulich <jbeulich@suse.com> From the cover letter I found this is a bug fix for a recently introduced regression. So I believe this should be merged for 4.17, hence: Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
On 29.09.2022 13:11, Roger Pau Monné wrote: > On Thu, Sep 29, 2022 at 11:51:03AM +0200, Jan Beulich wrote: >> With the "inversion" of VCPUOP handling, processing arch-specific ones >> first, the forwarding of this sub-op from the (common) compat handler to >> (common) non-compat one did no longer have the intended effect. It now >> needs forwarding between the arch-specific handlers. >> >> Fixes: 8a96c0ea7999 ("xen: move do_vcpu_op() to arch specific code") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. > This seems prone to errors, I wonder if we should add a note to > do_vcpu_op() to notice that hypercalls handled there need to be > explicitly forwarded from compat_vcpu_op. Perhaps. I guess if such a comment had been added by the original change, the mistake corrected here would have been noticed right away. > I've also noticed that compat_common_vcpu_op() forwards > VCPUOP_set_singleshot_timer to do_vcpu_op(), but that seems to be an > useless jump, shouldn't it forward directly to common_vcpu_op()? Looks like another (less severe) oversight, yes. Do you want to make a patch or should I? Jan
--- a/xen/arch/x86/x86_64/domain.c +++ b/xen/arch/x86/x86_64/domain.c @@ -58,6 +58,7 @@ compat_vcpu_op(int cmd, unsigned int vcp break; } + case VCPUOP_send_nmi: case VCPUOP_get_physid: rc = do_vcpu_op(cmd, vcpuid, arg); break; --- a/xen/common/compat/domain.c +++ b/xen/common/compat/domain.c @@ -99,7 +99,6 @@ int compat_common_vcpu_op(int cmd, struc case VCPUOP_stop_periodic_timer: case VCPUOP_stop_singleshot_timer: case VCPUOP_register_vcpu_info: - case VCPUOP_send_nmi: rc = common_vcpu_op(cmd, v, arg); break;
With the "inversion" of VCPUOP handling, processing arch-specific ones first, the forwarding of this sub-op from the (common) compat handler to (common) non-compat one did no longer have the intended effect. It now needs forwarding between the arch-specific handlers. Fixes: 8a96c0ea7999 ("xen: move do_vcpu_op() to arch specific code") Signed-off-by: Jan Beulich <jbeulich@suse.com>