linux-kernel - Re: [PATCH] x86/Xen: streamline (and fix) PV CPU enumeration

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eda3938f-02a5-e5a2-4718-d75bc488f296@oracle.com>
Date:   Thu, 28 Mar 2019 12:50:05 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Jan Beulich <JBeulich@...e.com>
Cc:     Stefano Stabellini <sstabellini@...nel.org>,
        xen-devel <xen-devel@...ts.xenproject.org>,
        Juergen Gross <jgross@...e.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/Xen: streamline (and fix) PV CPU enumeration

On 3/28/19 5:03 AM, Jan Beulich wrote:
>>>> On 27.03.19 at 18:07, <boris.ostrovsky@...cle.com> wrote:
>> On 3/27/19 11:12 AM, Jan Beulich wrote:
>>> -
>>> -static void __init xen_filter_cpu_maps(void)
>>> +static void __init _get_smp_config(unsigned int early)
>>>  {
>>>  	int i, rc;
>>>  	unsigned int subtract = 0;
>>>  
>>> -	if (!xen_initial_domain())
>>> +	if (early)
>>>  		return;
>>>  
>>>  	num_processors = 0;
>>
>> Is there a reason to set_cpu_possible() here (not in the diff, but in
>> this routine)? This will be called from setup_arch() before
>> prefill_possible_map(), which will clear and then re-initialize
>> __cpu_possible_mask.
> I don't think it's needed before my change either, so I'd call
> removing this an orthogonal change. As said in the commit
> message, the goal was to leave this function alone as far as
> possible.
>
> Before my patch, xen_filter_cpu_maps() gets called long after
> prefill_possible_map(), and by the purpose of the latter function
> the possible map shouldn't be altered anymore once that has
> run. Adding bits to it is surely not going to have the intended
> effect (setup_per_cpu_areas() has already run), while removing
> bits may have some effect, but comes too late at least for
> setup_per_cpu_areas().

OK. Then it looks like even though your patch changes behavior slightly
(so technically I guess it's not purely a cleanup) this shouldn't makes
any difference at least as far as possible cpu mask is concerned: if we
don't have percpu areas set up we can't do much for that vcpu since it
seems to me xen_vcpu_setup(), for example, won't do well.


>
> And if we were to remove this, I think the CONFIG_HOTPLUG_CPU
> section should go away as well. After all prefill_possible_map()
> also sets nr_cpu_ids. To be honest, it was largely this code
> fragment which made me want not touch the function more than
> necessary: The comment there makes not clear to me at all why
> all of this needs to be in an #ifdef in the first place.

This was introduced by cf405ae612b0 ("xen/smp: Fix crash when booting
with ACPI hotplug CPUs.").

I am not sure this is still relevant. The ACPI hotplug code had changed,
not significantly but sufficiently enough to alter behavior.
acpi_processor_hotadd_init() now fails before it gets a chance to call
arch_register_cpu() for vcpu>dom0_max_vcpus.


>
> Let me know whether you really want me to fold this extra
> cleanup into this patch. If so, I'd then wonder whether the
> set_cpu_present() from xen_pv_smp_prepare_cpus() shouldn't
> be moved here, too. And the fiddling with the possible map
> there looks bogus as well: Bring-up of CPUs past the command
> line option should be avoided at boot time, but they shouldn't
> be excluded from getting brought up later on. Note how
> native_smp_prepare_cpus() ignores its parameter altogether.

Yes, that does look strange.

Given especially xen_pv_smp_prepare_cpus(), I think re-working proper
setting of present/possible masks is well beyond the scope of your
original patch.

-boris