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

[Julien Grall <julien.grall@arm.com>]

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 <vkuznets@redhat.com>
> ---
> 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 *)&xen_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, &eax, &ebx, &ecx, &edx);
> +	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