Re: [Xen-devel] [PATCH v7 01/10] x86/hypervisor: make hypervisor_ap_setup return an error code
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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 01/10] x86/hypervisor: make hypervisor_ap_setup return an error code
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
> 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.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 01/10] x86/hypervisor: make hypervisor_ap_setup return an error code
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
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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 01/10] x86/hypervisor: make hypervisor_ap_setup return an error code
On Tue, Feb 04, 2020 at 05:56:21PM +0100, Roger Pau Monné wrote: > On Tue, Feb 04, 2020 at 04:48:05PM +, Wei Liu wrote: > > On Tue, Feb 04, 2020 at 03:36:55PM +, 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 > > > --- > > > > 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. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 01/10] x86/hypervisor: make hypervisor_ap_setup return an error code
On Tue, Feb 04, 2020 at 04:48:05PM +, Wei Liu wrote: > On Tue, Feb 04, 2020 at 03:36:55PM +, 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 > > --- > > 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. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 01/10] x86/hypervisor: make hypervisor_ap_setup return an error code
On Tue, Feb 04, 2020 at 03:36:55PM +, 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 > --- 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. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
