Re: [Xen-devel] [PATCH v4 5/7] x86/hyperv: provide percpu hypercall input page

[Wei Liu <wl@xxxxxxx>]

On Tue, Jan 28, 2020 at 05:15:39PM +0100, Jan Beulich wrote:
> On 28.01.2020 16:50, Wei Liu wrote:
> > On Thu, Jan 23, 2020 at 04:45:38PM +0100, Jan Beulich wrote:
> >> On 22.01.2020 21:23, Wei Liu wrote:
> >>> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> >>> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> >>> @@ -27,7 +27,10 @@
> >>>  #include <asm/guest/hyperv-tlfs.h>
> >>>  #include <asm/processor.h>
> >>>  
> >>> +#include "private.h"
> >>> +
> >>>  struct ms_hyperv_info __read_mostly ms_hyperv;
> >>> +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg);
> >>
> >> Would it perhaps be helpful to make recognizable that this can hold
> >> up to a page's worth of data, either by its type or by a slightly
> >> different name?
> > 
> > I will change this to hv_pcpu_input_arg_page instead.
> 
> Or maybe hv_pcpu_input_page?

Fine by me.

> 
> >>> @@ -119,14 +122,36 @@ static void __init setup_hypercall_page(void)
> >>>      }
> >>>  }
> >>>  
> >>> +static void setup_hypercall_pcpu_arg(void)
> >>> +{
> >>> +    void *mapping;
> >>> +
> >>> +    if ( this_cpu(hv_pcpu_input_arg) )
> >>> +        return;
> >>> +
> >>> +    mapping = alloc_xenheap_page();
> >>> +    if ( !mapping )
> >>> +        panic("Failed to allocate hypercall input page for CPU%u\n",
> >>> +              smp_processor_id());
> >>
> >> panic() is likely fine for the BSP, but I don't think any AP should
> >> be able to bring down the system because of memory shortage. Such
> >> an AP should simply not come online. (Even for the BSP the question
> >> is whether Xen would be able to run despite failure here.)
> > 
> > This is no different than what is already done for Xen on Xen, i.e.
> > failure in setting up AP for any reason is fatal.
> > 
> > start_secondary doesn't even handling any failure by itself or
> > propagate failure back to caller.
> > 
> > Rewinding is a bit complex, given that we would enable hypervisor
> > features very early.
> > 
> > To achieve what you want it would require rewriting of other parts that
> > are outside of hypervisor framework.
> 
> Not sure. Comparing with start_secondary() is perhaps sub-optimal.
> The function calls smp_callin(), and there you'll find some error
> handling. I would suppose this could be extended (there or in
> start_secondary() itself, if need be) to deal with cases like this
> one. As to Xen-on-Xen - iirc that code was pretty much rushed in
> for the shim to become usable, so I wouldn't take its error
> handling model as the canonical reference.

OK. What I can do here is to write some patches to 1) make the hook
return sensible error code and b) push hypervisor_ap_setup down to
smp_callin.

Wei.

> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel