lkml.org 
[lkml]   [2016]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping
    Date
    Julien Grall <julien.grall@arm.com> 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 <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.

    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(&xen_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)
    >>
    >> 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?

    Nothing, actually, but xen_hvm_guest_init() happens early and we always
    get these notifiers in the right order (and, reading Boris' reply, we're
    gonna make it explicit in future).

    >
    >> 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,

    --
    Vitaly

    \
     
     \ /
      Last update: 2016-09-17 09:58    [W:2.391 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site