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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

>>> @@ -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.

Jan

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