| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87d1kiwum9.fsf@vitty.brq.redhat.com>
Date: Mon, 05 Sep 2016 11:42:22 +0200
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: Julien Grall <julien.grall@....com>
Cc: xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
x86@...nel.org, Stefano Stabellini <sstabellini@...nel.org>,
Boris Ostrovsky <boris.ostrovsky@...cle.com>,
David Vrabel <david.vrabel@...rix.com>,
Juergen Gross <jgross@...e.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>, Jan Beulich <JBeulich@...e.com>,
Andrew Cooper <andrew.cooper3@...rix.com>,
Steve Capper <Steve.Capper@....com>,
Wei Chen <Wei.Chen@....com>, Kaly Xin <Kaly.Xin@....com>
Subject: Re: [PATCH linux v3 3/9] xen: introduce xen_vcpu_id mapping
Julien Grall <julien.grall@....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@...hat.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
Powered by blists - more mailing lists