Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-08 Thread Wei Chen 
Hi Vitaly,

On 2016/9/5 17:42, Vitaly Kuznetsov wrote:
> Julien Grall  writes:
>
>> Hi Vitaly,
>>
>> On 26/07/16 13:30, Vitaly Kuznetsov wrote:
>>> It may happen that Xen's and Linux's ideas of vCPU id diverge. In
>>> particular, when we crash on a secondary vCPU we may want to do kdump
>>> and unlike plain kexec where we do migrate_to_reboot_cpu() we try booting
>>> on the vCPU which crashed. This doesn't work very well for PVHVM guests as
>>> we have a number of hypercalls where we pass vCPU id as a parameter. These
>>> hypercalls either fail or do something unexpected. To solve the issue
>>> introduce percpu xen_vcpu_id mapping. ARM and PV guests get direct mapping
>>> for now. Boot CPU for PVHVM guest gets its id from CPUID. With secondary
>>> CPUs it is a bit more trickier. Currently, we initialize IPI vectors
>>> before these CPUs boot so we can't use CPUID. Use ACPI ids from MADT
>>> instead.
>>>
>>> Signed-off-by: Vitaly Kuznetsov 
>>> ---
>>> Changes since v2:
>>> - Use uint32_t for xen_vcpu_id mapping [Julien Grall]
>>>
>>> Changes since v1:
>>> - Introduce xen_vcpu_nr() helper [David Vrabel]
>>> - Use ACPI ids instead of vLAPIC ids /2 [Andrew Cooper, Jan Beulich]
>>> ---
>>>  arch/arm/xen/enlighten.c | 10 ++
>>>  arch/x86/xen/enlighten.c | 23 ++-
>>>  include/xen/xen-ops.h|  6 ++
>>>  3 files changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>>> index 75cd734..fe32267 100644
>>> --- a/arch/arm/xen/enlighten.c
>>> +++ b/arch/arm/xen/enlighten.c
>>> @@ -46,6 +46,10 @@ struct shared_info *HYPERVISOR_shared_info = (void 
>>> *)_dummy_shared_info;
>>>  DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
>>>  static struct vcpu_info __percpu *xen_vcpu_info;
>>>
>>> +/* Linux <-> Xen vCPU id mapping */
>>> +DEFINE_PER_CPU(uint32_t, xen_vcpu_id) = U32_MAX;
>>> +EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
>>> +
>>>  /* These are unused until we support booting "pre-ballooned" */
>>>  unsigned long xen_released_pages;
>>>  struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] 
>>> __initdata;
>>> @@ -179,6 +183,9 @@ static void xen_percpu_init(void)
>>>  pr_info("Xen: initializing cpu%d\n", cpu);
>>>  vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>>>
>>> +/* Direct vCPU id mapping for ARM guests. */
>>> +per_cpu(xen_vcpu_id, cpu) = cpu;
>>> +
>>
>> We did some internal testing on ARM64 with the latest Linux kernel
>> (4.8-rc4) and noticed that this patch is breaking SMP support. Sorry
>> for noticing the issue that late.
>
> Sorry for the breakage :-(
>
>>
>> This function is called on the running CPU whilst some code (e.g
>> init_control_block in drivers/xen/events/events_fifo.c) is executed
>> whilst preparing the CPU on the boot CPU.
>>
>> So xen_vcpu_nr(cpu) will always return 0 in this case and
>> init_control_block will fail to execute.
>>
>
> I see,
>
> CPU_UP_PREPARE event happens before xen_starting_cpu() is called.
>
>
>> I am not sure how to fix. I guess we could setup per_cpu(xen_vcpu_id,
>> *) in xen_guest_init. Any opinions?
>
> As we're not doing kexec on ARM we can fix the immediate issue. I don't
> know much about ARM and unfortunatelly I don't have a setup to test but
> it seems there is no early_per_cpu* infrastructure for ARM so we may fix
> it with the following:
>
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 3d2cef6..f193414 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -170,9 +170,6 @@ static int xen_starting_cpu(unsigned int cpu)
> pr_info("Xen: initializing cpu%d\n", cpu);
> vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>
> -   /* Direct vCPU id mapping for ARM guests. */
> -   per_cpu(xen_vcpu_id, cpu) = cpu;
> -
> info.mfn = virt_to_gfn(vcpup);
> info.offset = xen_offset_in_page(vcpup);
>
> @@ -330,6 +327,7 @@ static int __init xen_guest_init(void)
>  {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> +   int cpu;
>
> if (!xen_domain())
> return 0;
> @@ -380,7 +378,8 @@ static int __init xen_guest_init(void)
> return -ENOMEM;
>
> /* Direct vCPU id mapping for ARM guests. */
> -   per_cpu(xen_vcpu_id, 0) = 0;
> +   for_each_possible_cpu(cpu)
> +   per_cpu(xen_vcpu_id, cpu) = cpu;
>
> xen_auto_xlat_grant_frames.count = gnttab_max_grant_frames();
> if (xen_xlate_map_ballooned_pages(_auto_xlat_grant_frames.pfn,
>
> (not tested, if we can't use for_each_possible_cpu() that early we'll
> have to check against NR_CPUS instead).
>

I have tested this patch just now, it can work with the latest Linux
kernel and latest Xen hypervisor on my ARM platform:

[0.299927] xen:events: Using FIFO-based ABI
[0.304259] Xen: initializing cpu0
[0.336402] EFI services will not be available.
[0.388985] Detected PIPT

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-07 Thread Julien Grall 

Hi Vitaly,

On 07/09/2016 12:23, Vitaly Kuznetsov wrote:

BTW, were you able to try the patch I suggested? In my opinion it would
be preferable to fix the immediate SMP issue now and play with MPIDR
info later.


Not yet sorry. I will see if I can try to today or tomorrow.

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-07 Thread Vitaly Kuznetsov 
Julien Grall  writes:

> Hi Vitaly,
>
> On 07/09/2016 10:07, Vitaly Kuznetsov wrote:
>> Stefano Stabellini  writes:
>>> I don't know that much about cpuid, but the virtual MPIDR is constructed
>>> from the vcpu id right now:
>>>
>>> v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);
>>>
>>> [...]
>>>
>>> static inline register_t vcpuid_to_vaffinity(unsigned int vcpuid)
>>> {
>>> register_t vaff;
>>>
>>> vaff = (vcpuid & 0x0f) << MPIDR_LEVEL_SHIFT(0);
>>> vaff |= ((vcpuid >> 4) & MPIDR_LEVEL_MASK) << MPIDR_LEVEL_SHIFT(1);
>>>
>>> return vaff;
>>> }
>>
>> This could work but only in case there is a way to get MPIDR for _other_
>> cpu (e.g. CPU0 needs to get MPIDR of CPU1 when CPU1 is not yet runnning)
>> or we'll have to change the machinery of how we bring up secondary CPUs
>> - e.g. CPUn starts, writes its id somewhere and 'hangs' waiting for CPU0
>> to set up event channels.
>
> You can get the MPIDR from both the device tree and ACPI. The firmware
> table is parsed at boot time and the value is stored in
> cpu_logical_map().

Good,

in this case we can easily do the same trick we did for x86 and we
don't need to change the way secondary CPUs are strarted.

BTW, were you able to try the patch I suggested? In my opinion it would
be preferable to fix the immediate SMP issue now and play with MPIDR
info later.

>
> Regards,

-- 
  Vitaly

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-07 Thread Julien Grall 

Hi Vitaly,

On 07/09/2016 10:07, Vitaly Kuznetsov wrote:

Stefano Stabellini  writes:

I don't know that much about cpuid, but the virtual MPIDR is constructed
from the vcpu id right now:

v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);

[...]

static inline register_t vcpuid_to_vaffinity(unsigned int vcpuid)
{
register_t vaff;

vaff = (vcpuid & 0x0f) << MPIDR_LEVEL_SHIFT(0);
vaff |= ((vcpuid >> 4) & MPIDR_LEVEL_MASK) << MPIDR_LEVEL_SHIFT(1);

return vaff;
}


This could work but only in case there is a way to get MPIDR for _other_
cpu (e.g. CPU0 needs to get MPIDR of CPU1 when CPU1 is not yet runnning)
or we'll have to change the machinery of how we bring up secondary CPUs
- e.g. CPUn starts, writes its id somewhere and 'hangs' waiting for CPU0
to set up event channels.


You can get the MPIDR from both the device tree and ACPI. The firmware 
table is parsed at boot time and the value is stored in cpu_logical_map().


Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-07 Thread David Vrabel 
On 07/09/16 10:07, Vitaly Kuznetsov wrote:
> Stefano Stabellini  writes:
> 
>> On Tue, 6 Sep 2016, Vitaly Kuznetsov wrote:
>>> Stefano Stabellini  writes:
>>>
 On Mon, 5 Sep 2016, Vitaly Kuznetsov wrote:
> Julien Grall  writes:
>
>> Hi Vitaly,
>>
>> On 26/07/16 13:30, Vitaly Kuznetsov wrote:
>>> It may happen that Xen's and Linux's ideas of vCPU id diverge. In
>>> particular, when we crash on a secondary vCPU we may want to do kdump
>>> and unlike plain kexec where we do migrate_to_reboot_cpu() we try 
>>> booting
>>> on the vCPU which crashed. This doesn't work very well for PVHVM guests 
>>> as
>>> we have a number of hypercalls where we pass vCPU id as a parameter. 
>>> These
>>> hypercalls either fail or do something unexpected. To solve the issue
>>> introduce percpu xen_vcpu_id mapping. ARM and PV guests get direct 
>>> mapping
>>> for now. Boot CPU for PVHVM guest gets its id from CPUID. With secondary
>>> CPUs it is a bit more trickier. Currently, we initialize IPI vectors
>>> before these CPUs boot so we can't use CPUID. Use ACPI ids from MADT
>>> instead.
>>>
>>> Signed-off-by: Vitaly Kuznetsov 
>>> ---
>>> Changes since v2:
>>> - Use uint32_t for xen_vcpu_id mapping [Julien Grall]
>>>
>>> Changes since v1:
>>> - Introduce xen_vcpu_nr() helper [David Vrabel]
>>> - Use ACPI ids instead of vLAPIC ids /2 [Andrew Cooper, Jan Beulich]
>>> ---
>>>  arch/arm/xen/enlighten.c | 10 ++
>>>  arch/x86/xen/enlighten.c | 23 ++-
>>>  include/xen/xen-ops.h|  6 ++
>>>  3 files changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>>> index 75cd734..fe32267 100644
>>> --- a/arch/arm/xen/enlighten.c
>>> +++ b/arch/arm/xen/enlighten.c
>>> @@ -46,6 +46,10 @@ struct shared_info *HYPERVISOR_shared_info = (void 
>>> *)_dummy_shared_info;
>>>  DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
>>>  static struct vcpu_info __percpu *xen_vcpu_info;
>>>
>>> +/* Linux <-> Xen vCPU id mapping */
>>> +DEFINE_PER_CPU(uint32_t, xen_vcpu_id) = U32_MAX;
>>> +EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
>>> +
>>>  /* These are unused until we support booting "pre-ballooned" */
>>>  unsigned long xen_released_pages;
>>>  struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] 
>>> __initdata;
>>> @@ -179,6 +183,9 @@ static void xen_percpu_init(void)
>>> pr_info("Xen: initializing cpu%d\n", cpu);
>>> vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>>>
>>> +   /* Direct vCPU id mapping for ARM guests. */
>>> +   per_cpu(xen_vcpu_id, cpu) = cpu;
>>> +
>>
>> We did some internal testing on ARM64 with the latest Linux kernel
>> (4.8-rc4) and noticed that this patch is breaking SMP support. Sorry
>> for noticing the issue that late.
>
> Sorry for the breakage :-(
>
>>
>> This function is called on the running CPU whilst some code (e.g
>> init_control_block in drivers/xen/events/events_fifo.c) is executed
>> whilst preparing the CPU on the boot CPU.
>>
>> So xen_vcpu_nr(cpu) will always return 0 in this case and
>> init_control_block will fail to execute.
>>
>
> I see,
>
> CPU_UP_PREPARE event happens before xen_starting_cpu() is called.
>
>
>> I am not sure how to fix. I guess we could setup per_cpu(xen_vcpu_id,
>> *) in xen_guest_init. Any opinions?
>
> As we're not doing kexec on ARM we can fix the immediate issue. I don't
> know much about ARM and unfortunatelly I don't have a setup to test but
> it seems there is no early_per_cpu* infrastructure for ARM so we may fix
> it with the following:
>
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 3d2cef6..f193414 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -170,9 +170,6 @@ static int xen_starting_cpu(unsigned int cpu)
> pr_info("Xen: initializing cpu%d\n", cpu);
> vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>  
> -   /* Direct vCPU id mapping for ARM guests. */
> -   per_cpu(xen_vcpu_id, cpu) = cpu;
> -
> info.mfn = virt_to_gfn(vcpup);
> info.offset = xen_offset_in_page(vcpup);
>  
> @@ -330,6 +327,7 @@ static int __init xen_guest_init(void)
>  {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> +   int cpu;
>  
> if (!xen_domain())
> return 0;
> @@ -380,7 +378,8 @@ static int __init xen_guest_init(void)
> return -ENOMEM;
>  
> /* Direct vCPU id

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-07 Thread Vitaly Kuznetsov 
Stefano Stabellini  writes:

> On Tue, 6 Sep 2016, Vitaly Kuznetsov wrote:
>> Stefano Stabellini  writes:
>> 
>> > On Mon, 5 Sep 2016, Vitaly Kuznetsov wrote:
>> >> Julien Grall  writes:
>> >> 
>> >> > Hi Vitaly,
>> >> >
>> >> > On 26/07/16 13:30, Vitaly Kuznetsov wrote:
>> >> >> It may happen that Xen's and Linux's ideas of vCPU id diverge. In
>> >> >> particular, when we crash on a secondary vCPU we may want to do kdump
>> >> >> and unlike plain kexec where we do migrate_to_reboot_cpu() we try 
>> >> >> booting
>> >> >> on the vCPU which crashed. This doesn't work very well for PVHVM 
>> >> >> guests as
>> >> >> we have a number of hypercalls where we pass vCPU id as a parameter. 
>> >> >> These
>> >> >> hypercalls either fail or do something unexpected. To solve the issue
>> >> >> introduce percpu xen_vcpu_id mapping. ARM and PV guests get direct 
>> >> >> mapping
>> >> >> for now. Boot CPU for PVHVM guest gets its id from CPUID. With 
>> >> >> secondary
>> >> >> CPUs it is a bit more trickier. Currently, we initialize IPI vectors
>> >> >> before these CPUs boot so we can't use CPUID. Use ACPI ids from MADT
>> >> >> instead.
>> >> >>
>> >> >> Signed-off-by: Vitaly Kuznetsov 
>> >> >> ---
>> >> >> Changes since v2:
>> >> >> - Use uint32_t for xen_vcpu_id mapping [Julien Grall]
>> >> >>
>> >> >> Changes since v1:
>> >> >> - Introduce xen_vcpu_nr() helper [David Vrabel]
>> >> >> - Use ACPI ids instead of vLAPIC ids /2 [Andrew Cooper, Jan Beulich]
>> >> >> ---
>> >> >>  arch/arm/xen/enlighten.c | 10 ++
>> >> >>  arch/x86/xen/enlighten.c | 23 ++-
>> >> >>  include/xen/xen-ops.h|  6 ++
>> >> >>  3 files changed, 38 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> >> >> index 75cd734..fe32267 100644
>> >> >> --- a/arch/arm/xen/enlighten.c
>> >> >> +++ b/arch/arm/xen/enlighten.c
>> >> >> @@ -46,6 +46,10 @@ struct shared_info *HYPERVISOR_shared_info = (void 
>> >> >> *)_dummy_shared_info;
>> >> >>  DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
>> >> >>  static struct vcpu_info __percpu *xen_vcpu_info;
>> >> >>
>> >> >> +/* Linux <-> Xen vCPU id mapping */
>> >> >> +DEFINE_PER_CPU(uint32_t, xen_vcpu_id) = U32_MAX;
>> >> >> +EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
>> >> >> +
>> >> >>  /* These are unused until we support booting "pre-ballooned" */
>> >> >>  unsigned long xen_released_pages;
>> >> >>  struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] 
>> >> >> __initdata;
>> >> >> @@ -179,6 +183,9 @@ static void xen_percpu_init(void)
>> >> >>pr_info("Xen: initializing cpu%d\n", cpu);
>> >> >>vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>> >> >>
>> >> >> +  /* Direct vCPU id mapping for ARM guests. */
>> >> >> +  per_cpu(xen_vcpu_id, cpu) = cpu;
>> >> >> +
>> >> >
>> >> > We did some internal testing on ARM64 with the latest Linux kernel
>> >> > (4.8-rc4) and noticed that this patch is breaking SMP support. Sorry
>> >> > for noticing the issue that late.
>> >> 
>> >> Sorry for the breakage :-(
>> >> 
>> >> >
>> >> > This function is called on the running CPU whilst some code (e.g
>> >> > init_control_block in drivers/xen/events/events_fifo.c) is executed
>> >> > whilst preparing the CPU on the boot CPU.
>> >> >
>> >> > So xen_vcpu_nr(cpu) will always return 0 in this case and
>> >> > init_control_block will fail to execute.
>> >> >
>> >> 
>> >> I see,
>> >> 
>> >> CPU_UP_PREPARE event happens before xen_starting_cpu() is called.
>> >> 
>> >> 
>> >> > I am not sure how to fix. I guess we could setup per_cpu(xen_vcpu_id,
>> >> > *) in xen_guest_init. Any opinions?
>> >> 
>> >> As we're not doing kexec on ARM we can fix the immediate issue. I don't
>> >> know much about ARM and unfortunatelly I don't have a setup to test but
>> >> it seems there is no early_per_cpu* infrastructure for ARM so we may fix
>> >> it with the following:
>> >> 
>> >> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> >> index 3d2cef6..f193414 100644
>> >> --- a/arch/arm/xen/enlighten.c
>> >> +++ b/arch/arm/xen/enlighten.c
>> >> @@ -170,9 +170,6 @@ static int xen_starting_cpu(unsigned int cpu)
>> >> pr_info("Xen: initializing cpu%d\n", cpu);
>> >> vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>> >>  
>> >> -   /* Direct vCPU id mapping for ARM guests. */
>> >> -   per_cpu(xen_vcpu_id, cpu) = cpu;
>> >> -
>> >> info.mfn = virt_to_gfn(vcpup);
>> >> info.offset = xen_offset_in_page(vcpup);
>> >>  
>> >> @@ -330,6 +327,7 @@ static int __init xen_guest_init(void)
>> >>  {
>> >> struct xen_add_to_physmap xatp;
>> >> struct shared_info *shared_info_page = NULL;
>> >> +   int cpu;
>> >>  
>> >> if (!xen_domain())
>> >> return 0;
>> >> @@ -380,7 +378,8 @@ static int __init xen_guest_init(void)
>> >> return -ENOMEM;
>> >>

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-06 Thread Stefano Stabellini 
On Tue, 6 Sep 2016, Vitaly Kuznetsov wrote:
> Stefano Stabellini  writes:
> 
> > On Mon, 5 Sep 2016, Vitaly Kuznetsov wrote:
> >> Julien Grall  writes:
> >> 
> >> > Hi Vitaly,
> >> >
> >> > On 26/07/16 13:30, Vitaly Kuznetsov wrote:
> >> >> It may happen that Xen's and Linux's ideas of vCPU id diverge. In
> >> >> particular, when we crash on a secondary vCPU we may want to do kdump
> >> >> and unlike plain kexec where we do migrate_to_reboot_cpu() we try 
> >> >> booting
> >> >> on the vCPU which crashed. This doesn't work very well for PVHVM guests 
> >> >> as
> >> >> we have a number of hypercalls where we pass vCPU id as a parameter. 
> >> >> These
> >> >> hypercalls either fail or do something unexpected. To solve the issue
> >> >> introduce percpu xen_vcpu_id mapping. ARM and PV guests get direct 
> >> >> mapping
> >> >> for now. Boot CPU for PVHVM guest gets its id from CPUID. With secondary
> >> >> CPUs it is a bit more trickier. Currently, we initialize IPI vectors
> >> >> before these CPUs boot so we can't use CPUID. Use ACPI ids from MADT
> >> >> instead.
> >> >>
> >> >> Signed-off-by: Vitaly Kuznetsov 
> >> >> ---
> >> >> Changes since v2:
> >> >> - Use uint32_t for xen_vcpu_id mapping [Julien Grall]
> >> >>
> >> >> Changes since v1:
> >> >> - Introduce xen_vcpu_nr() helper [David Vrabel]
> >> >> - Use ACPI ids instead of vLAPIC ids /2 [Andrew Cooper, Jan Beulich]
> >> >> ---
> >> >>  arch/arm/xen/enlighten.c | 10 ++
> >> >>  arch/x86/xen/enlighten.c | 23 ++-
> >> >>  include/xen/xen-ops.h|  6 ++
> >> >>  3 files changed, 38 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> >> >> index 75cd734..fe32267 100644
> >> >> --- a/arch/arm/xen/enlighten.c
> >> >> +++ b/arch/arm/xen/enlighten.c
> >> >> @@ -46,6 +46,10 @@ struct shared_info *HYPERVISOR_shared_info = (void 
> >> >> *)_dummy_shared_info;
> >> >>  DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
> >> >>  static struct vcpu_info __percpu *xen_vcpu_info;
> >> >>
> >> >> +/* Linux <-> Xen vCPU id mapping */
> >> >> +DEFINE_PER_CPU(uint32_t, xen_vcpu_id) = U32_MAX;
> >> >> +EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
> >> >> +
> >> >>  /* These are unused until we support booting "pre-ballooned" */
> >> >>  unsigned long xen_released_pages;
> >> >>  struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] 
> >> >> __initdata;
> >> >> @@ -179,6 +183,9 @@ static void xen_percpu_init(void)
> >> >> pr_info("Xen: initializing cpu%d\n", cpu);
> >> >> vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
> >> >>
> >> >> +   /* Direct vCPU id mapping for ARM guests. */
> >> >> +   per_cpu(xen_vcpu_id, cpu) = cpu;
> >> >> +
> >> >
> >> > We did some internal testing on ARM64 with the latest Linux kernel
> >> > (4.8-rc4) and noticed that this patch is breaking SMP support. Sorry
> >> > for noticing the issue that late.
> >> 
> >> Sorry for the breakage :-(
> >> 
> >> >
> >> > This function is called on the running CPU whilst some code (e.g
> >> > init_control_block in drivers/xen/events/events_fifo.c) is executed
> >> > whilst preparing the CPU on the boot CPU.
> >> >
> >> > So xen_vcpu_nr(cpu) will always return 0 in this case and
> >> > init_control_block will fail to execute.
> >> >
> >> 
> >> I see,
> >> 
> >> CPU_UP_PREPARE event happens before xen_starting_cpu() is called.
> >> 
> >> 
> >> > I am not sure how to fix. I guess we could setup per_cpu(xen_vcpu_id,
> >> > *) in xen_guest_init. Any opinions?
> >> 
> >> As we're not doing kexec on ARM we can fix the immediate issue. I don't
> >> know much about ARM and unfortunatelly I don't have a setup to test but
> >> it seems there is no early_per_cpu* infrastructure for ARM so we may fix
> >> it with the following:
> >> 
> >> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> >> index 3d2cef6..f193414 100644
> >> --- a/arch/arm/xen/enlighten.c
> >> +++ b/arch/arm/xen/enlighten.c
> >> @@ -170,9 +170,6 @@ static int xen_starting_cpu(unsigned int cpu)
> >> pr_info("Xen: initializing cpu%d\n", cpu);
> >> vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
> >>  
> >> -   /* Direct vCPU id mapping for ARM guests. */
> >> -   per_cpu(xen_vcpu_id, cpu) = cpu;
> >> -
> >> info.mfn = virt_to_gfn(vcpup);
> >> info.offset = xen_offset_in_page(vcpup);
> >>  
> >> @@ -330,6 +327,7 @@ static int __init xen_guest_init(void)
> >>  {
> >> struct xen_add_to_physmap xatp;
> >> struct shared_info *shared_info_page = NULL;
> >> +   int cpu;
> >>  
> >> if (!xen_domain())
> >> return 0;
> >> @@ -380,7 +378,8 @@ static int __init xen_guest_init(void)
> >> return -ENOMEM;
> >>  
> >> /* Direct vCPU id mapping for ARM guests. */
> >> -   per_cpu(xen_vcpu_id, 0) = 0;
> >> +   for_each_possible_cpu(cpu)
> >> +

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-06 Thread Vitaly Kuznetsov 
Stefano Stabellini  writes:

> On Mon, 5 Sep 2016, Vitaly Kuznetsov wrote:
>> Julien Grall  writes:
>> 
>> > Hi Vitaly,
>> >
>> > On 26/07/16 13:30, Vitaly Kuznetsov wrote:
>> >> It may happen that Xen's and Linux's ideas of vCPU id diverge. In
>> >> particular, when we crash on a secondary vCPU we may want to do kdump
>> >> and unlike plain kexec where we do migrate_to_reboot_cpu() we try booting
>> >> on the vCPU which crashed. This doesn't work very well for PVHVM guests as
>> >> we have a number of hypercalls where we pass vCPU id as a parameter. These
>> >> hypercalls either fail or do something unexpected. To solve the issue
>> >> introduce percpu xen_vcpu_id mapping. ARM and PV guests get direct mapping
>> >> for now. Boot CPU for PVHVM guest gets its id from CPUID. With secondary
>> >> CPUs it is a bit more trickier. Currently, we initialize IPI vectors
>> >> before these CPUs boot so we can't use CPUID. Use ACPI ids from MADT
>> >> instead.
>> >>
>> >> Signed-off-by: Vitaly Kuznetsov 
>> >> ---
>> >> Changes since v2:
>> >> - Use uint32_t for xen_vcpu_id mapping [Julien Grall]
>> >>
>> >> Changes since v1:
>> >> - Introduce xen_vcpu_nr() helper [David Vrabel]
>> >> - Use ACPI ids instead of vLAPIC ids /2 [Andrew Cooper, Jan Beulich]
>> >> ---
>> >>  arch/arm/xen/enlighten.c | 10 ++
>> >>  arch/x86/xen/enlighten.c | 23 ++-
>> >>  include/xen/xen-ops.h|  6 ++
>> >>  3 files changed, 38 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> >> index 75cd734..fe32267 100644
>> >> --- a/arch/arm/xen/enlighten.c
>> >> +++ b/arch/arm/xen/enlighten.c
>> >> @@ -46,6 +46,10 @@ struct shared_info *HYPERVISOR_shared_info = (void 
>> >> *)_dummy_shared_info;
>> >>  DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
>> >>  static struct vcpu_info __percpu *xen_vcpu_info;
>> >>
>> >> +/* Linux <-> Xen vCPU id mapping */
>> >> +DEFINE_PER_CPU(uint32_t, xen_vcpu_id) = U32_MAX;
>> >> +EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
>> >> +
>> >>  /* These are unused until we support booting "pre-ballooned" */
>> >>  unsigned long xen_released_pages;
>> >>  struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] 
>> >> __initdata;
>> >> @@ -179,6 +183,9 @@ static void xen_percpu_init(void)
>> >>   pr_info("Xen: initializing cpu%d\n", cpu);
>> >>   vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>> >>
>> >> + /* Direct vCPU id mapping for ARM guests. */
>> >> + per_cpu(xen_vcpu_id, cpu) = cpu;
>> >> +
>> >
>> > We did some internal testing on ARM64 with the latest Linux kernel
>> > (4.8-rc4) and noticed that this patch is breaking SMP support. Sorry
>> > for noticing the issue that late.
>> 
>> Sorry for the breakage :-(
>> 
>> >
>> > This function is called on the running CPU whilst some code (e.g
>> > init_control_block in drivers/xen/events/events_fifo.c) is executed
>> > whilst preparing the CPU on the boot CPU.
>> >
>> > So xen_vcpu_nr(cpu) will always return 0 in this case and
>> > init_control_block will fail to execute.
>> >
>> 
>> I see,
>> 
>> CPU_UP_PREPARE event happens before xen_starting_cpu() is called.
>> 
>> 
>> > I am not sure how to fix. I guess we could setup per_cpu(xen_vcpu_id,
>> > *) in xen_guest_init. Any opinions?
>> 
>> As we're not doing kexec on ARM we can fix the immediate issue. I don't
>> know much about ARM and unfortunatelly I don't have a setup to test but
>> it seems there is no early_per_cpu* infrastructure for ARM so we may fix
>> it with the following:
>> 
>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> index 3d2cef6..f193414 100644
>> --- a/arch/arm/xen/enlighten.c
>> +++ b/arch/arm/xen/enlighten.c
>> @@ -170,9 +170,6 @@ static int xen_starting_cpu(unsigned int cpu)
>> pr_info("Xen: initializing cpu%d\n", cpu);
>> vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>>  
>> -   /* Direct vCPU id mapping for ARM guests. */
>> -   per_cpu(xen_vcpu_id, cpu) = cpu;
>> -
>> info.mfn = virt_to_gfn(vcpup);
>> info.offset = xen_offset_in_page(vcpup);
>>  
>> @@ -330,6 +327,7 @@ static int __init xen_guest_init(void)
>>  {
>> struct xen_add_to_physmap xatp;
>> struct shared_info *shared_info_page = NULL;
>> +   int cpu;
>>  
>> if (!xen_domain())
>> return 0;
>> @@ -380,7 +378,8 @@ static int __init xen_guest_init(void)
>> return -ENOMEM;
>>  
>> /* Direct vCPU id mapping for ARM guests. */
>> -   per_cpu(xen_vcpu_id, 0) = 0;
>> +   for_each_possible_cpu(cpu)
>> +   per_cpu(xen_vcpu_id, cpu) = cpu;
>>  
>> xen_auto_xlat_grant_frames.count = gnttab_max_grant_frames();
>> if (xen_xlate_map_ballooned_pages(_auto_xlat_grant_frames.pfn,
>> 
>> (not tested, if we can't use for_each_possible_cpu() that early we'll
>> have to check against NR_CPUS instead).
>
> Kind of defeat the purpose of

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-05 Thread Stefano Stabellini 
On Mon, 5 Sep 2016, Vitaly Kuznetsov wrote:
> Julien Grall  writes:
> 
> > Hi Vitaly,
> >
> > On 26/07/16 13:30, Vitaly Kuznetsov wrote:
> >> It may happen that Xen's and Linux's ideas of vCPU id diverge. In
> >> particular, when we crash on a secondary vCPU we may want to do kdump
> >> and unlike plain kexec where we do migrate_to_reboot_cpu() we try booting
> >> on the vCPU which crashed. This doesn't work very well for PVHVM guests as
> >> we have a number of hypercalls where we pass vCPU id as a parameter. These
> >> hypercalls either fail or do something unexpected. To solve the issue
> >> introduce percpu xen_vcpu_id mapping. ARM and PV guests get direct mapping
> >> for now. Boot CPU for PVHVM guest gets its id from CPUID. With secondary
> >> CPUs it is a bit more trickier. Currently, we initialize IPI vectors
> >> before these CPUs boot so we can't use CPUID. Use ACPI ids from MADT
> >> instead.
> >>
> >> Signed-off-by: Vitaly Kuznetsov 
> >> ---
> >> Changes since v2:
> >> - Use uint32_t for xen_vcpu_id mapping [Julien Grall]
> >>
> >> Changes since v1:
> >> - Introduce xen_vcpu_nr() helper [David Vrabel]
> >> - Use ACPI ids instead of vLAPIC ids /2 [Andrew Cooper, Jan Beulich]
> >> ---
> >>  arch/arm/xen/enlighten.c | 10 ++
> >>  arch/x86/xen/enlighten.c | 23 ++-
> >>  include/xen/xen-ops.h|  6 ++
> >>  3 files changed, 38 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> >> index 75cd734..fe32267 100644
> >> --- a/arch/arm/xen/enlighten.c
> >> +++ b/arch/arm/xen/enlighten.c
> >> @@ -46,6 +46,10 @@ struct shared_info *HYPERVISOR_shared_info = (void 
> >> *)_dummy_shared_info;
> >>  DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
> >>  static struct vcpu_info __percpu *xen_vcpu_info;
> >>
> >> +/* Linux <-> Xen vCPU id mapping */
> >> +DEFINE_PER_CPU(uint32_t, xen_vcpu_id) = U32_MAX;
> >> +EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
> >> +
> >>  /* These are unused until we support booting "pre-ballooned" */
> >>  unsigned long xen_released_pages;
> >>  struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] 
> >> __initdata;
> >> @@ -179,6 +183,9 @@ static void xen_percpu_init(void)
> >>pr_info("Xen: initializing cpu%d\n", cpu);
> >>vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
> >>
> >> +  /* Direct vCPU id mapping for ARM guests. */
> >> +  per_cpu(xen_vcpu_id, cpu) = cpu;
> >> +
> >
> > We did some internal testing on ARM64 with the latest Linux kernel
> > (4.8-rc4) and noticed that this patch is breaking SMP support. Sorry
> > for noticing the issue that late.
> 
> Sorry for the breakage :-(
> 
> >
> > This function is called on the running CPU whilst some code (e.g
> > init_control_block in drivers/xen/events/events_fifo.c) is executed
> > whilst preparing the CPU on the boot CPU.
> >
> > So xen_vcpu_nr(cpu) will always return 0 in this case and
> > init_control_block will fail to execute.
> >
> 
> I see,
> 
> CPU_UP_PREPARE event happens before xen_starting_cpu() is called.
> 
> 
> > I am not sure how to fix. I guess we could setup per_cpu(xen_vcpu_id,
> > *) in xen_guest_init. Any opinions?
> 
> As we're not doing kexec on ARM we can fix the immediate issue. I don't
> know much about ARM and unfortunatelly I don't have a setup to test but
> it seems there is no early_per_cpu* infrastructure for ARM so we may fix
> it with the following:
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 3d2cef6..f193414 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -170,9 +170,6 @@ static int xen_starting_cpu(unsigned int cpu)
> pr_info("Xen: initializing cpu%d\n", cpu);
> vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>  
> -   /* Direct vCPU id mapping for ARM guests. */
> -   per_cpu(xen_vcpu_id, cpu) = cpu;
> -
> info.mfn = virt_to_gfn(vcpup);
> info.offset = xen_offset_in_page(vcpup);
>  
> @@ -330,6 +327,7 @@ static int __init xen_guest_init(void)
>  {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> +   int cpu;
>  
> if (!xen_domain())
> return 0;
> @@ -380,7 +378,8 @@ static int __init xen_guest_init(void)
> return -ENOMEM;
>  
> /* Direct vCPU id mapping for ARM guests. */
> -   per_cpu(xen_vcpu_id, 0) = 0;
> +   for_each_possible_cpu(cpu)
> +   per_cpu(xen_vcpu_id, cpu) = cpu;
>  
> xen_auto_xlat_grant_frames.count = gnttab_max_grant_frames();
> if (xen_xlate_map_ballooned_pages(_auto_xlat_grant_frames.pfn,
> 
> (not tested, if we can't use for_each_possible_cpu() that early we'll
> have to check against NR_CPUS instead).

Kind of defeat the purpose of xen_vcpu_id, but I guess it should work.


> But unfortunatelly we'll have to get back to this in future. Turns out
> we need to know Xen's idea of vCPU id _before_ this vCPU starts
>

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-05 Thread Vitaly Kuznetsov 
Julien Grall  writes:

> Hi Vitaly,
>
> On 26/07/16 13:30, Vitaly Kuznetsov wrote:
>> It may happen that Xen's and Linux's ideas of vCPU id diverge. In
>> particular, when we crash on a secondary vCPU we may want to do kdump
>> and unlike plain kexec where we do migrate_to_reboot_cpu() we try booting
>> on the vCPU which crashed. This doesn't work very well for PVHVM guests as
>> we have a number of hypercalls where we pass vCPU id as a parameter. These
>> hypercalls either fail or do something unexpected. To solve the issue
>> introduce percpu xen_vcpu_id mapping. ARM and PV guests get direct mapping
>> for now. Boot CPU for PVHVM guest gets its id from CPUID. With secondary
>> CPUs it is a bit more trickier. Currently, we initialize IPI vectors
>> before these CPUs boot so we can't use CPUID. Use ACPI ids from MADT
>> instead.
>>
>> Signed-off-by: Vitaly Kuznetsov 
>> ---
>> Changes since v2:
>> - Use uint32_t for xen_vcpu_id mapping [Julien Grall]
>>
>> Changes since v1:
>> - Introduce xen_vcpu_nr() helper [David Vrabel]
>> - Use ACPI ids instead of vLAPIC ids /2 [Andrew Cooper, Jan Beulich]
>> ---
>>  arch/arm/xen/enlighten.c | 10 ++
>>  arch/x86/xen/enlighten.c | 23 ++-
>>  include/xen/xen-ops.h|  6 ++
>>  3 files changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> index 75cd734..fe32267 100644
>> --- a/arch/arm/xen/enlighten.c
>> +++ b/arch/arm/xen/enlighten.c
>> @@ -46,6 +46,10 @@ struct shared_info *HYPERVISOR_shared_info = (void 
>> *)_dummy_shared_info;
>>  DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
>>  static struct vcpu_info __percpu *xen_vcpu_info;
>>
>> +/* Linux <-> Xen vCPU id mapping */
>> +DEFINE_PER_CPU(uint32_t, xen_vcpu_id) = U32_MAX;
>> +EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
>> +
>>  /* These are unused until we support booting "pre-ballooned" */
>>  unsigned long xen_released_pages;
>>  struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] 
>> __initdata;
>> @@ -179,6 +183,9 @@ static void xen_percpu_init(void)
>>  pr_info("Xen: initializing cpu%d\n", cpu);
>>  vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>>
>> +/* Direct vCPU id mapping for ARM guests. */
>> +per_cpu(xen_vcpu_id, cpu) = cpu;
>> +
>
> We did some internal testing on ARM64 with the latest Linux kernel
> (4.8-rc4) and noticed that this patch is breaking SMP support. Sorry
> for noticing the issue that late.

Sorry for the breakage :-(

>
> This function is called on the running CPU whilst some code (e.g
> init_control_block in drivers/xen/events/events_fifo.c) is executed
> whilst preparing the CPU on the boot CPU.
>
> So xen_vcpu_nr(cpu) will always return 0 in this case and
> init_control_block will fail to execute.
>

I see,

CPU_UP_PREPARE event happens before xen_starting_cpu() is called.


> I am not sure how to fix. I guess we could setup per_cpu(xen_vcpu_id,
> *) in xen_guest_init. Any opinions?

As we're not doing kexec on ARM we can fix the immediate issue. I don't
know much about ARM and unfortunatelly I don't have a setup to test but
it seems there is no early_per_cpu* infrastructure for ARM so we may fix
it with the following:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 3d2cef6..f193414 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -170,9 +170,6 @@ static int xen_starting_cpu(unsigned int cpu)
pr_info("Xen: initializing cpu%d\n", cpu);
vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
 
-   /* Direct vCPU id mapping for ARM guests. */
-   per_cpu(xen_vcpu_id, cpu) = cpu;
-
info.mfn = virt_to_gfn(vcpup);
info.offset = xen_offset_in_page(vcpup);
 
@@ -330,6 +327,7 @@ static int __init xen_guest_init(void)
 {
struct xen_add_to_physmap xatp;
struct shared_info *shared_info_page = NULL;
+   int cpu;
 
if (!xen_domain())
return 0;
@@ -380,7 +378,8 @@ static int __init xen_guest_init(void)
return -ENOMEM;
 
/* Direct vCPU id mapping for ARM guests. */
-   per_cpu(xen_vcpu_id, 0) = 0;
+   for_each_possible_cpu(cpu)
+   per_cpu(xen_vcpu_id, cpu) = cpu;
 
xen_auto_xlat_grant_frames.count = gnttab_max_grant_frames();
if (xen_xlate_map_ballooned_pages(_auto_xlat_grant_frames.pfn,

(not tested, if we can't use for_each_possible_cpu() that early we'll
have to check against NR_CPUS instead).

But unfortunatelly we'll have to get back to this in future. Turns out
we need to know Xen's idea of vCPU id _before_ this vCPU starts
executing code. On x86 we used ACPI_ID from MADT. Is there anything like
it on ARM?

>
> [...]
>
>> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
>> index 0f87db2..c833912 100644
>> --- a/arch/x86/xen/enlighten.c
>> +++ b/arch/x86/xen/enlighten.c
>> @@ -1795,6 +1806,12 @@ static void __init init_hvm_pv_info(void)
>>
>>

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-04 Thread Boris Ostrovsky 
On 09/02/2016 11:29 AM, Julien Grall wrote:
>
>> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
>> index 0f87db2..c833912 100644
>> --- a/arch/x86/xen/enlighten.c
>> +++ b/arch/x86/xen/enlighten.c
>> @@ -1795,6 +1806,12 @@ static void __init init_hvm_pv_info(void)
>>
>>  xen_setup_features();
>>
>> +cpuid(base + 4, , , , );
>> +if (eax & XEN_HVM_CPUID_VCPU_ID_PRESENT)
>> +this_cpu_write(xen_vcpu_id, ebx);
>> +else
>> +this_cpu_write(xen_vcpu_id, smp_processor_id());
>> +
>>  pv_info.name = "Xen HVM";
>>
>>  xen_domain_type = XEN_HVM_DOMAIN;
>> @@ -1806,6 +1823,10 @@ static int xen_hvm_cpu_notify(struct
>> notifier_block *self, unsigned long action,
>>  int cpu = (long)hcpu;
>>  switch (action) {
>>  case CPU_UP_PREPARE:
>> +if (cpu_acpi_id(cpu) != U32_MAX)
>> +per_cpu(xen_vcpu_id, cpu) = cpu_acpi_id(cpu);
>> +else
>> +per_cpu(xen_vcpu_id, cpu) = cpu;
>
> I have not tried myself. But looking at the code, the notifiers
> xen_hvm_cpu_notifier and evtchn_fifo_cpu_notifier have the same
> priority. So what does prevent the code above to be executed after the
> event channel callback?


We will be converting to new hotplug state machine where this order will
be guaranteed:

 https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01914.html


-boris


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-09-02 Thread Julien Grall 

Hi Vitaly,

On 26/07/16 13:30, Vitaly Kuznetsov wrote:

It may happen that Xen's and Linux's ideas of vCPU id diverge. In
particular, when we crash on a secondary vCPU we may want to do kdump
and unlike plain kexec where we do migrate_to_reboot_cpu() we try booting
on the vCPU which crashed. This doesn't work very well for PVHVM guests as
we have a number of hypercalls where we pass vCPU id as a parameter. These
hypercalls either fail or do something unexpected. To solve the issue
introduce percpu xen_vcpu_id mapping. ARM and PV guests get direct mapping
for now. Boot CPU for PVHVM guest gets its id from CPUID. With secondary
CPUs it is a bit more trickier. Currently, we initialize IPI vectors
before these CPUs boot so we can't use CPUID. Use ACPI ids from MADT
instead.

Signed-off-by: Vitaly Kuznetsov 
---
Changes since v2:
- Use uint32_t for xen_vcpu_id mapping [Julien Grall]

Changes since v1:
- Introduce xen_vcpu_nr() helper [David Vrabel]
- Use ACPI ids instead of vLAPIC ids /2 [Andrew Cooper, Jan Beulich]
---
 arch/arm/xen/enlighten.c | 10 ++
 arch/x86/xen/enlighten.c | 23 ++-
 include/xen/xen-ops.h|  6 ++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 75cd734..fe32267 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -46,6 +46,10 @@ struct shared_info *HYPERVISOR_shared_info = (void 
*)_dummy_shared_info;
 DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
 static struct vcpu_info __percpu *xen_vcpu_info;

+/* Linux <-> Xen vCPU id mapping */
+DEFINE_PER_CPU(uint32_t, xen_vcpu_id) = U32_MAX;
+EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
+
 /* These are unused until we support booting "pre-ballooned" */
 unsigned long xen_released_pages;
 struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
@@ -179,6 +183,9 @@ static void xen_percpu_init(void)
pr_info("Xen: initializing cpu%d\n", cpu);
vcpup = per_cpu_ptr(xen_vcpu_info, cpu);

+   /* Direct vCPU id mapping for ARM guests. */
+   per_cpu(xen_vcpu_id, cpu) = cpu;
+


We did some internal testing on ARM64 with the latest Linux kernel 
(4.8-rc4) and noticed that this patch is breaking SMP support. Sorry for 
noticing the issue that late.


This function is called on the running CPU whilst some code (e.g 
init_control_block in drivers/xen/events/events_fifo.c) is executed 
whilst preparing the CPU on the boot CPU.


So xen_vcpu_nr(cpu) will always return 0 in this case and 
init_control_block will fail to execute.


I am not sure how to fix. I guess we could setup per_cpu(xen_vcpu_id, *) 
in xen_guest_init. Any opinions?


[...]


diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 0f87db2..c833912 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1795,6 +1806,12 @@ static void __init init_hvm_pv_info(void)

xen_setup_features();

+   cpuid(base + 4, , , , );
+   if (eax & XEN_HVM_CPUID_VCPU_ID_PRESENT)
+   this_cpu_write(xen_vcpu_id, ebx);
+   else
+   this_cpu_write(xen_vcpu_id, smp_processor_id());
+
pv_info.name = "Xen HVM";

xen_domain_type = XEN_HVM_DOMAIN;
@@ -1806,6 +1823,10 @@ static int xen_hvm_cpu_notify(struct notifier_block 
*self, unsigned long action,
int cpu = (long)hcpu;
switch (action) {
case CPU_UP_PREPARE:
+   if (cpu_acpi_id(cpu) != U32_MAX)
+   per_cpu(xen_vcpu_id, cpu) = cpu_acpi_id(cpu);
+   else
+   per_cpu(xen_vcpu_id, cpu) = cpu;


I have not tried myself. But looking at the code, the notifiers 
xen_hvm_cpu_notifier and evtchn_fifo_cpu_notifier have the same 
priority. So what does prevent the code above to be executed after the 
event channel callback?



xen_vcpu_setup(cpu);
if (xen_have_vector_callback) {
if (xen_feature(XENFEAT_hvm_safe_pvclock))
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 86abe07..648ce814 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -9,6 +9,12 @@

 DECLARE_PER_CPU(struct vcpu_info *, xen_vcpu);

+DECLARE_PER_CPU(uint32_t, xen_vcpu_id);
+static inline int xen_vcpu_nr(int cpu)
+{
+   return per_cpu(xen_vcpu_id, cpu);
+}
+
 void xen_arch_pre_suspend(void);
 void xen_arch_post_suspend(int suspend_cancelled);


Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping

2016-07-26 Thread Vitaly Kuznetsov 
It may happen that Xen's and Linux's ideas of vCPU id diverge. In
particular, when we crash on a secondary vCPU we may want to do kdump
and unlike plain kexec where we do migrate_to_reboot_cpu() we try booting
on the vCPU which crashed. This doesn't work very well for PVHVM guests as
we have a number of hypercalls where we pass vCPU id as a parameter. These
hypercalls either fail or do something unexpected. To solve the issue
introduce percpu xen_vcpu_id mapping. ARM and PV guests get direct mapping
for now. Boot CPU for PVHVM guest gets its id from CPUID. With secondary
CPUs it is a bit more trickier. Currently, we initialize IPI vectors
before these CPUs boot so we can't use CPUID. Use ACPI ids from MADT
instead.

Signed-off-by: Vitaly Kuznetsov 
---
Changes since v2:
- Use uint32_t for xen_vcpu_id mapping [Julien Grall]

Changes since v1:
- Introduce xen_vcpu_nr() helper [David Vrabel]
- Use ACPI ids instead of vLAPIC ids /2 [Andrew Cooper, Jan Beulich]
---
 arch/arm/xen/enlighten.c | 10 ++
 arch/x86/xen/enlighten.c | 23 ++-
 include/xen/xen-ops.h|  6 ++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 75cd734..fe32267 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -46,6 +46,10 @@ struct shared_info *HYPERVISOR_shared_info = (void 
*)_dummy_shared_info;
 DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
 static struct vcpu_info __percpu *xen_vcpu_info;
 
+/* Linux <-> Xen vCPU id mapping */
+DEFINE_PER_CPU(uint32_t, xen_vcpu_id) = U32_MAX;
+EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
+
 /* These are unused until we support booting "pre-ballooned" */
 unsigned long xen_released_pages;
 struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
@@ -179,6 +183,9 @@ static void xen_percpu_init(void)
pr_info("Xen: initializing cpu%d\n", cpu);
vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
 
+   /* Direct vCPU id mapping for ARM guests. */
+   per_cpu(xen_vcpu_id, cpu) = cpu;
+
info.mfn = virt_to_gfn(vcpup);
info.offset = xen_offset_in_page(vcpup);
 
@@ -328,6 +335,9 @@ static int __init xen_guest_init(void)
if (xen_vcpu_info == NULL)
return -ENOMEM;
 
+   /* Direct vCPU id mapping for ARM guests. */
+   per_cpu(xen_vcpu_id, 0) = 0;
+
if (gnttab_setup_auto_xlat_frames(grant_frames)) {
free_percpu(xen_vcpu_info);
return -ENOMEM;
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 0f87db2..c833912 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -59,6 +59,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -118,6 +119,10 @@ DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
  */
 DEFINE_PER_CPU(struct vcpu_info, xen_vcpu_info);
 
+/* Linux <-> Xen vCPU id mapping */
+DEFINE_PER_CPU(uint32_t, xen_vcpu_id) = U32_MAX;
+EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
+
 enum xen_domain_type xen_domain_type = XEN_NATIVE;
 EXPORT_SYMBOL_GPL(xen_domain_type);
 
@@ -1135,8 +1140,11 @@ void xen_setup_vcpu_info_placement(void)
 {
int cpu;
 
-   for_each_possible_cpu(cpu)
+   for_each_possible_cpu(cpu) {
+   /* Set up direct vCPU id mapping for PV guests. */
+   per_cpu(xen_vcpu_id, cpu) = cpu;
xen_vcpu_setup(cpu);
+   }
 
/* xen_vcpu_setup managed to place the vcpu_info within the
 * percpu area for all cpus, so make use of it. Note that for
@@ -1727,6 +1735,9 @@ asmlinkage __visible void __init xen_start_kernel(void)
 #endif
xen_raw_console_write("about to get started...\n");
 
+   /* Let's presume PV guests always boot on vCPU with id 0. */
+   per_cpu(xen_vcpu_id, 0) = 0;
+
xen_setup_runstate_info(0);
 
xen_efi_init();
@@ -1795,6 +1806,12 @@ static void __init init_hvm_pv_info(void)
 
xen_setup_features();
 
+   cpuid(base + 4, , , , );
+   if (eax & XEN_HVM_CPUID_VCPU_ID_PRESENT)
+   this_cpu_write(xen_vcpu_id, ebx);
+   else
+   this_cpu_write(xen_vcpu_id, smp_processor_id());
+
pv_info.name = "Xen HVM";
 
xen_domain_type = XEN_HVM_DOMAIN;
@@ -1806,6 +1823,10 @@ static int xen_hvm_cpu_notify(struct notifier_block 
*self, unsigned long action,
int cpu = (long)hcpu;
switch (action) {
case CPU_UP_PREPARE:
+   if (cpu_acpi_id(cpu) != U32_MAX)
+   per_cpu(xen_vcpu_id, cpu) = cpu_acpi_id(cpu);
+   else
+   per_cpu(xen_vcpu_id, cpu) = cpu;
xen_vcpu_setup(cpu);
if (xen_have_vector_callback) {
if (xen_feature(XENFEAT_hvm_safe_pvclock))
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 86abe07..648ce814 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -9,6 +9,12 @@