| Message ID | 20200204153704.15934-2-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More Hyper-V infrastructures | expand |
On Tue, Feb 04, 2020 at 03:36:55PM +0000, Wei Liu wrote: > We want to be able to handle AP setup error in the upper layer. > > For Xen, remove all panic() and BUG_ON() in init_evtchn and > map_vcpuinfo. Only panic/BUG_ON when Xen can't fail gracefully. > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > --- BTW I discover an issue: init_evtchn sets HVM_PARAM_CALLBACK_IRQ every time it is called. That's unnecessary for APs. Perhaps it would be best to break that function into two, one for setting HVM_PARAM_CALLBACK_IRQ, the other for allocating and setting callback. BSP needs to call both while APs only needs to call the latter. This is out of scope for this series, but it is something to consider in the future. Wei.
On Tue, Feb 04, 2020 at 04:48:05PM +0000, Wei Liu wrote: > On Tue, Feb 04, 2020 at 03:36:55PM +0000, Wei Liu wrote: > > We want to be able to handle AP setup error in the upper layer. > > > > For Xen, remove all panic() and BUG_ON() in init_evtchn and > > map_vcpuinfo. Only panic/BUG_ON when Xen can't fail gracefully. > > > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > > --- > > BTW I discover an issue: init_evtchn sets HVM_PARAM_CALLBACK_IRQ every > time it is called. That's unnecessary for APs. Perhaps it would be best > to break that function into two, one for setting HVM_PARAM_CALLBACK_IRQ, > the other for allocating and setting callback. BSP needs to call both > while APs only needs to call the latter. We could gate the call to HVMOP_set_param on !smp_processor_id(), that way the BSP would be the only one to set HVM_PARAM_CALLBACK_IRQ. I'm not sure splitting this into a separate function is worth it. Thanks, Roger.
On Tue, Feb 04, 2020 at 05:56:21PM +0100, Roger Pau Monné wrote: > On Tue, Feb 04, 2020 at 04:48:05PM +0000, Wei Liu wrote: > > On Tue, Feb 04, 2020 at 03:36:55PM +0000, Wei Liu wrote: > > > We want to be able to handle AP setup error in the upper layer. > > > > > > For Xen, remove all panic() and BUG_ON() in init_evtchn and > > > map_vcpuinfo. Only panic/BUG_ON when Xen can't fail gracefully. > > > > > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > > > --- > > > > BTW I discover an issue: init_evtchn sets HVM_PARAM_CALLBACK_IRQ every > > time it is called. That's unnecessary for APs. Perhaps it would be best > > to break that function into two, one for setting HVM_PARAM_CALLBACK_IRQ, > > the other for allocating and setting callback. BSP needs to call both > > while APs only needs to call the latter. > > We could gate the call to HVMOP_set_param on !smp_processor_id(), that > way the BSP would be the only one to set HVM_PARAM_CALLBACK_IRQ. I'm > not sure splitting this into a separate function is worth it. This works too. But again, something for another day. Wei.
On 04.02.2020 16:36, Wei Liu wrote: > @@ -215,18 +220,19 @@ static void init_evtchn(void) > rc = xen_hypercall_set_evtchn_upcall_vector(this_cpu(vcpu_id), > evtchn_upcall_vector); > if ( rc ) > - panic("Unable to set evtchn upcall vector: %d\n", rc); > + { > + printk("Unable to set evtchn upcall vector: %d\n", rc); > + goto out; There's no need for "goto" here - "return rc" is all you need instead. As stated elsewhere, when there's complex cleanup or a fair risk of leaving out an important cleanup step, I can live with "goto" getting used. But I don't think it should be used to replace a simple "return". With this Reviewed-by: Jan Beulich <jbeulich@suse.com> with one more (optional!) suggestion and one more remark: > @@ -254,14 +260,20 @@ static void __init setup(void) > XEN_LEGACY_MAX_VCPUS); > } > > - init_evtchn(); > + BUG_ON(init_evtchn()); > } > > -static void ap_setup(void) > +static int ap_setup(void) > { > + int rc; > + > set_vcpu_id(); > - map_vcpuinfo(); > - init_evtchn(); > + > + rc = map_vcpuinfo(); > + if ( rc ) > + return rc; > + > + return init_evtchn(); > } To avoid a local variable, how about return map_vcpuinfo() ?: init_evtchn(); ? > @@ -283,8 +295,8 @@ int xg_free_unused_page(mfn_t mfn) > > static void ap_resume(void *unused) > { > - map_vcpuinfo(); > - init_evtchn(); > + BUG_ON(map_vcpuinfo()); > + BUG_ON(init_evtchn()); > } Current code structure calls for this, but in principle I don't think AP failure on resume should be any different from AP failure during boot. Nothing to be address here and now, of course. Jan
On Wed, Feb 05, 2020 at 12:12:30PM +0100, Jan Beulich wrote: > On 04.02.2020 16:36, Wei Liu wrote: > > @@ -215,18 +220,19 @@ static void init_evtchn(void) > > rc = xen_hypercall_set_evtchn_upcall_vector(this_cpu(vcpu_id), > > evtchn_upcall_vector); > > if ( rc ) > > - panic("Unable to set evtchn upcall vector: %d\n", rc); > > + { > > + printk("Unable to set evtchn upcall vector: %d\n", rc); > > + goto out; > > There's no need for "goto" here - "return rc" is all you need > instead. As stated elsewhere, when there's complex cleanup or > a fair risk of leaving out an important cleanup step, I can > live with "goto" getting used. But I don't think it should be > used to replace a simple "return". OK. That can be fixed. > > With this > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with one more (optional!) suggestion and one more remark: Thanks. > > > @@ -254,14 +260,20 @@ static void __init setup(void) > > XEN_LEGACY_MAX_VCPUS); > > } > > > > - init_evtchn(); > > + BUG_ON(init_evtchn()); > > } > > > > -static void ap_setup(void) > > +static int ap_setup(void) > > { > > + int rc; > > + > > set_vcpu_id(); > > - map_vcpuinfo(); > > - init_evtchn(); > > + > > + rc = map_vcpuinfo(); > > + if ( rc ) > > + return rc; > > + > > + return init_evtchn(); > > } > > To avoid a local variable, how about > > return map_vcpuinfo() ?: init_evtchn(); > > ? ISTR this is a GNU extension, but seeing that there is already quite a lot of it in hypercisor code, I will make the change. Wei.
On 05.02.2020 13:09, Wei Liu wrote: > On Wed, Feb 05, 2020 at 12:12:30PM +0100, Jan Beulich wrote: >> On 04.02.2020 16:36, Wei Liu wrote: >>> @@ -254,14 +260,20 @@ static void __init setup(void) >>> XEN_LEGACY_MAX_VCPUS); >>> } >>> >>> - init_evtchn(); >>> + BUG_ON(init_evtchn()); >>> } >>> >>> -static void ap_setup(void) >>> +static int ap_setup(void) >>> { >>> + int rc; >>> + >>> set_vcpu_id(); >>> - map_vcpuinfo(); >>> - init_evtchn(); >>> + >>> + rc = map_vcpuinfo(); >>> + if ( rc ) >>> + return rc; >>> + >>> + return init_evtchn(); >>> } >> >> To avoid a local variable, how about >> >> return map_vcpuinfo() ?: init_evtchn(); >> >> ? > > ISTR this is a GNU extension, but seeing that there is already quite a > lot of it in hypercisor code, I will make the change. In our own code using extensions is generally fine (as far as they're sufficiently backwards compatible). It's the public headers where we want to be more careful. Jan
diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c index 4f27b98740..e72c92ffdf 100644 --- a/xen/arch/x86/guest/hypervisor.c +++ b/xen/arch/x86/guest/hypervisor.c @@ -52,10 +52,12 @@ void __init hypervisor_setup(void) ops->setup(); } -void hypervisor_ap_setup(void) +int hypervisor_ap_setup(void) { if ( ops && ops->ap_setup ) - ops->ap_setup(); + return ops->ap_setup(); + + return 0; } void hypervisor_resume(void) diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c index 6dbc5f953f..1cf09886da 100644 --- a/xen/arch/x86/guest/xen/xen.c +++ b/xen/arch/x86/guest/xen/xen.c @@ -113,16 +113,16 @@ static int map_vcpuinfo(void) info.mfn = virt_to_mfn(&vcpu_info[vcpu]); info.offset = (unsigned long)&vcpu_info[vcpu] & ~PAGE_MASK; rc = xen_hypercall_vcpu_op(VCPUOP_register_vcpu_info, vcpu, &info); - if ( rc ) - { - BUG_ON(vcpu >= XEN_LEGACY_MAX_VCPUS); - this_cpu(vcpu_info) = &XEN_shared_info->vcpu_info[vcpu]; - } - else + if ( !rc ) { this_cpu(vcpu_info) = &vcpu_info[vcpu]; set_bit(vcpu, vcpu_info_mapped); } + else if ( vcpu < XEN_LEGACY_MAX_VCPUS ) + { + rc = 0; + this_cpu(vcpu_info) = &XEN_shared_info->vcpu_info[vcpu]; + } return rc; } @@ -202,10 +202,15 @@ static void xen_evtchn_upcall(struct cpu_user_regs *regs) ack_APIC_irq(); } -static void init_evtchn(void) +static int init_evtchn(void) { static uint8_t evtchn_upcall_vector; int rc; + struct xen_hvm_param a = { + .domid = DOMID_SELF, + .index = HVM_PARAM_CALLBACK_IRQ, + .value = 1, + }; if ( !evtchn_upcall_vector ) alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall); @@ -215,18 +220,19 @@ static void init_evtchn(void) rc = xen_hypercall_set_evtchn_upcall_vector(this_cpu(vcpu_id), evtchn_upcall_vector); if ( rc ) - panic("Unable to set evtchn upcall vector: %d\n", rc); + { + printk("Unable to set evtchn upcall vector: %d\n", rc); + goto out; + } /* Trick toolstack to think we are enlightened */ - { - struct xen_hvm_param a = { - .domid = DOMID_SELF, - .index = HVM_PARAM_CALLBACK_IRQ, - .value = 1, - }; + rc = xen_hypercall_hvm_op(HVMOP_set_param, &a); - BUG_ON(xen_hypercall_hvm_op(HVMOP_set_param, &a)); - } + if ( rc ) + printk("Unable to set HVM_PARAM_CALLBACK_IRQ\n"); + + out: + return rc; } static void __init setup(void) @@ -254,14 +260,20 @@ static void __init setup(void) XEN_LEGACY_MAX_VCPUS); } - init_evtchn(); + BUG_ON(init_evtchn()); } -static void ap_setup(void) +static int ap_setup(void) { + int rc; + set_vcpu_id(); - map_vcpuinfo(); - init_evtchn(); + + rc = map_vcpuinfo(); + if ( rc ) + return rc; + + return init_evtchn(); } int xg_alloc_unused_page(mfn_t *mfn) @@ -283,8 +295,8 @@ int xg_free_unused_page(mfn_t mfn) static void ap_resume(void *unused) { - map_vcpuinfo(); - init_evtchn(); + BUG_ON(map_vcpuinfo()); + BUG_ON(init_evtchn()); } static void resume(void) @@ -303,7 +315,7 @@ static void resume(void) panic("unable to remap vCPU info and vCPUs > legacy limit\n"); /* Setup event channel upcall vector. */ - init_evtchn(); + BUG_ON(init_evtchn()); smp_call_function(ap_resume, NULL, 1); if ( pv_console ) diff --git a/xen/include/asm-x86/guest/hypervisor.h b/xen/include/asm-x86/guest/hypervisor.h index 392f4b90ae..b503854c5b 100644 --- a/xen/include/asm-x86/guest/hypervisor.h +++ b/xen/include/asm-x86/guest/hypervisor.h @@ -25,7 +25,7 @@ struct hypervisor_ops { /* Main setup routine */ void (*setup)(void); /* AP setup */ - void (*ap_setup)(void); + int (*ap_setup)(void); /* Resume from suspension */ void (*resume)(void); }; @@ -34,7 +34,7 @@ struct hypervisor_ops { const char *hypervisor_probe(void); void hypervisor_setup(void); -void hypervisor_ap_setup(void); +int hypervisor_ap_setup(void); void hypervisor_resume(void); #else @@ -44,7 +44,7 @@ void hypervisor_resume(void); static inline const char *hypervisor_probe(void) { return NULL; } static inline void hypervisor_setup(void) { ASSERT_UNREACHABLE(); } -static inline void hypervisor_ap_setup(void) { ASSERT_UNREACHABLE(); } +static inline int hypervisor_ap_setup(void) { ASSERT_UNREACHABLE(); return 0; } static inline void hypervisor_resume(void) { ASSERT_UNREACHABLE(); } #endif /* CONFIG_GUEST */
We want to be able to handle AP setup error in the upper layer. For Xen, remove all panic() and BUG_ON() in init_evtchn and map_vcpuinfo. Only panic/BUG_ON when Xen can't fail gracefully. Signed-off-by: Wei Liu <liuwe@microsoft.com> --- v7: 1. Change init_evtchn v6: 1. Change map_vcpuinfo as well 2. Make code shorter --- xen/arch/x86/guest/hypervisor.c | 6 ++- xen/arch/x86/guest/xen/xen.c | 58 ++++++++++++++++---------- xen/include/asm-x86/guest/hypervisor.h | 6 +-- 3 files changed, 42 insertions(+), 28 deletions(-)