871b72dd "x86: microcode: use smp_call_function_single instead of
set_cpus_allowed, cleanup of synchronization logic" included:
static int mc_sysdev_resume(struct sys_device *dev)
{
[...]
+ /*
+ * All non-bootup cpus are still disabled,
+ * so only CPU 0 will apply ucode here.
+ *
+ * Moreover, there can be no concurrent
+ * updates from any other places at this point.
+ */
+ WARN_ON(cpu != 0);
However suspend/resume under Xen doesn't need to hot unplug all the CPUs, so we
don't; the hypervisor can manage the context save/restore for all CPUs.
It would be unnecessary to load microcode.ko in a Xen domU but if it does occur
(e.g. because a distro installs the tools by default) we would like to avoid
the warning on resume.
Since the real constraint here is that we are running on the CPU for which we
would like to load microcode (which in all practical circumstances is CPU0)
just check for that and return if we are resuming a different CPU.
There is no danger of concurrent updates, even if we ignore the fact that all
but one CPUs are unplugged on native, because sysdev_resume() is single
threaded.
Signed-off-by: Ian Campbell <[email protected]>
Cc: Jeremy Fitzhardinge <[email protected]>
Cc: Dmitry Adamushko <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
arch/x86/kernel/microcode_core.c | 11 +----------
1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 378e9a8..1153062 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -438,18 +438,9 @@ static int mc_sysdev_resume(struct sys_device *dev)
int cpu = dev->id;
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- if (!cpu_online(cpu))
+ if (cpu != smp_processor_id())
return 0;
- /*
- * All non-bootup cpus are still disabled,
- * so only CPU 0 will apply ucode here.
- *
- * Moreover, there can be no concurrent
- * updates from any other places at this point.
- */
- WARN_ON(cpu != 0);
-
if (uci->valid && uci->mc)
microcode_ops->apply_microcode(cpu);
--
1.5.6.5
On 12/17/2009 05:53 AM, Ian Campbell wrote:
> 871b72dd "x86: microcode: use smp_call_function_single instead of
> set_cpus_allowed, cleanup of synchronization logic" included:
>
> static int mc_sysdev_resume(struct sys_device *dev)
> {
> [...]
> + /*
> + * All non-bootup cpus are still disabled,
> + * so only CPU 0 will apply ucode here.
> + *
> + * Moreover, there can be no concurrent
> + * updates from any other places at this point.
> + */
> + WARN_ON(cpu != 0);
>
> However suspend/resume under Xen doesn't need to hot unplug all the CPUs, so we
> don't; the hypervisor can manage the context save/restore for all CPUs.
>
Did you see a problem with this in practice, or just by inspection?
The Xen microcode driver will only load in a privileged domain, so I
don't think this path can ever be exercised.
Regardless, the Xen microcode driver changes aren't upstream yet, so
there's no need to apply this there yet.
Thanks,
J
> It would be unnecessary to load microcode.ko in a Xen domU but if it does occur
> (e.g. because a distro installs the tools by default) we would like to avoid
> the warning on resume.
>
> Since the real constraint here is that we are running on the CPU for which we
> would like to load microcode (which in all practical circumstances is CPU0)
> just check for that and return if we are resuming a different CPU.
>
> There is no danger of concurrent updates, even if we ignore the fact that all
> but one CPUs are unplugged on native, because sysdev_resume() is single
> threaded.
>
> Signed-off-by: Ian Campbell<[email protected]>
> Cc: Jeremy Fitzhardinge<[email protected]>
> Cc: Dmitry Adamushko<[email protected]>
> Cc: Hugh Dickins<[email protected]>
> Cc: Ingo Molnar<[email protected]>
> ---
> arch/x86/kernel/microcode_core.c | 11 +----------
> 1 files changed, 1 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> index 378e9a8..1153062 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -438,18 +438,9 @@ static int mc_sysdev_resume(struct sys_device *dev)
> int cpu = dev->id;
> struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
>
> - if (!cpu_online(cpu))
> + if (cpu != smp_processor_id())
> return 0;
>
> - /*
> - * All non-bootup cpus are still disabled,
> - * so only CPU 0 will apply ucode here.
> - *
> - * Moreover, there can be no concurrent
> - * updates from any other places at this point.
> - */
> - WARN_ON(cpu != 0);
> -
> if (uci->valid&& uci->mc)
> microcode_ops->apply_microcode(cpu);
>
>
On Thu, 2009-12-17 at 18:45 +0000, Jeremy Fitzhardinge wrote:
> On 12/17/2009 05:53 AM, Ian Campbell wrote:
> > 871b72dd "x86: microcode: use smp_call_function_single instead of
> > set_cpus_allowed, cleanup of synchronization logic" included:
> >
> > static int mc_sysdev_resume(struct sys_device *dev)
> > {
> > [...]
> > + /*
> > + * All non-bootup cpus are still disabled,
> > + * so only CPU 0 will apply ucode here.
> > + *
> > + * Moreover, there can be no concurrent
> > + * updates from any other places at this point.
> > + */
> > + WARN_ON(cpu != 0);
> >
> > However suspend/resume under Xen doesn't need to hot unplug all the CPUs, so we
> > don't; the hypervisor can manage the context save/restore for all CPUs.
> >
>
> Did you see a problem with this in practice, or just by inspection?
In practice.
> The Xen microcode driver will only load in a privileged domain, so I
> don't think this path can ever be exercised.
This was on bare v2.6.32 + your bugfixes branch, there was no domain 0
support or Xen microcode driver.
The regular native microcode driver can load in a domU and generally
silently does the right thing (i.e. nothing) apart from this issue.
Distro initscripts will tend to try and load the native driver and they
are very unlikely to special case Xen domU to avoid it.
> Regardless, the Xen microcode driver changes aren't upstream yet, so
> there's no need to apply this there yet.
This issue is in the generic microcode code so it is also needed now. I
guess once the Xen microcode driver is upstreamed it will fix domU by
suppressing the driver from loading at all except in dom0.
Ian.
>
> Thanks,
> J
>
> > It would be unnecessary to load microcode.ko in a Xen domU but if it does occur
> > (e.g. because a distro installs the tools by default) we would like to avoid
> > the warning on resume.
> >
> > Since the real constraint here is that we are running on the CPU for which we
> > would like to load microcode (which in all practical circumstances is CPU0)
> > just check for that and return if we are resuming a different CPU.
> >
> > There is no danger of concurrent updates, even if we ignore the fact that all
> > but one CPUs are unplugged on native, because sysdev_resume() is single
> > threaded.
> >
> > Signed-off-by: Ian Campbell<[email protected]>
> > Cc: Jeremy Fitzhardinge<[email protected]>
> > Cc: Dmitry Adamushko<[email protected]>
> > Cc: Hugh Dickins<[email protected]>
> > Cc: Ingo Molnar<[email protected]>
> > ---
> > arch/x86/kernel/microcode_core.c | 11 +----------
> > 1 files changed, 1 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> > index 378e9a8..1153062 100644
> > --- a/arch/x86/kernel/microcode_core.c
> > +++ b/arch/x86/kernel/microcode_core.c
> > @@ -438,18 +438,9 @@ static int mc_sysdev_resume(struct sys_device *dev)
> > int cpu = dev->id;
> > struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> >
> > - if (!cpu_online(cpu))
> > + if (cpu != smp_processor_id())
> > return 0;
> >
> > - /*
> > - * All non-bootup cpus are still disabled,
> > - * so only CPU 0 will apply ucode here.
> > - *
> > - * Moreover, there can be no concurrent
> > - * updates from any other places at this point.
> > - */
> > - WARN_ON(cpu != 0);
> > -
> > if (uci->valid&& uci->mc)
> > microcode_ops->apply_microcode(cpu);
> >
> >
>
On 12/18/2009 01:07 AM, Ian Campbell wrote:
> On Thu, 2009-12-17 at 18:45 +0000, Jeremy Fitzhardinge wrote:
>
>> On 12/17/2009 05:53 AM, Ian Campbell wrote:
>>
>>> 871b72dd "x86: microcode: use smp_call_function_single instead of
>>> set_cpus_allowed, cleanup of synchronization logic" included:
>>>
>>> static int mc_sysdev_resume(struct sys_device *dev)
>>> {
>>> [...]
>>> + /*
>>> + * All non-bootup cpus are still disabled,
>>> + * so only CPU 0 will apply ucode here.
>>> + *
>>> + * Moreover, there can be no concurrent
>>> + * updates from any other places at this point.
>>> + */
>>> + WARN_ON(cpu != 0);
>>>
>>> However suspend/resume under Xen doesn't need to hot unplug all the CPUs, so we
>>> don't; the hypervisor can manage the context save/restore for all CPUs.
>>>
>>>
>> Did you see a problem with this in practice, or just by inspection?
>>
> In practice.
>
>
>> The Xen microcode driver will only load in a privileged domain, so I
>> don't think this path can ever be exercised.
>>
> This was on bare v2.6.32 + your bugfixes branch, there was no domain 0
> support or Xen microcode driver.
>
> The regular native microcode driver can load in a domU and generally
> silently does the right thing (i.e. nothing) apart from this issue.
> Distro initscripts will tend to try and load the native driver and they
> are very unlikely to special case Xen domU to avoid it.
>
OK, I see. The patch looks reasonable to me then.
Alternatively we could try to prevent the native microcode drivers from
loading in the Xen case, but since the code just checks the CPU vendor
it doesn't give us much to work with aside from adding an explicit Xen
test. Making the resume code more robust seems like the better option.
J
>
>> Regardless, the Xen microcode driver changes aren't upstream yet, so
>> there's no need to apply this there yet.
>>
> This issue is in the generic microcode code so it is also needed now. I
> guess once the Xen microcode driver is upstreamed it will fix domU by
> suppressing the driver from loading at all except in dom0.
>
> Ian.
>
>
>> Thanks,
>> J
>>
>>
>>> It would be unnecessary to load microcode.ko in a Xen domU but if it does occur
>>> (e.g. because a distro installs the tools by default) we would like to avoid
>>> the warning on resume.
>>>
>>> Since the real constraint here is that we are running on the CPU for which we
>>> would like to load microcode (which in all practical circumstances is CPU0)
>>> just check for that and return if we are resuming a different CPU.
>>>
>>> There is no danger of concurrent updates, even if we ignore the fact that all
>>> but one CPUs are unplugged on native, because sysdev_resume() is single
>>> threaded.
>>>
>>> Signed-off-by: Ian Campbell<[email protected]>
>>> Cc: Jeremy Fitzhardinge<[email protected]>
>>> Cc: Dmitry Adamushko<[email protected]>
>>> Cc: Hugh Dickins<[email protected]>
>>> Cc: Ingo Molnar<[email protected]>
>>> ---
>>> arch/x86/kernel/microcode_core.c | 11 +----------
>>> 1 files changed, 1 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
>>> index 378e9a8..1153062 100644
>>> --- a/arch/x86/kernel/microcode_core.c
>>> +++ b/arch/x86/kernel/microcode_core.c
>>> @@ -438,18 +438,9 @@ static int mc_sysdev_resume(struct sys_device *dev)
>>> int cpu = dev->id;
>>> struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
>>>
>>> - if (!cpu_online(cpu))
>>> + if (cpu != smp_processor_id())
>>> return 0;
>>>
>>> - /*
>>> - * All non-bootup cpus are still disabled,
>>> - * so only CPU 0 will apply ucode here.
>>> - *
>>> - * Moreover, there can be no concurrent
>>> - * updates from any other places at this point.
>>> - */
>>> - WARN_ON(cpu != 0);
>>> -
>>> if (uci->valid&& uci->mc)
>>> microcode_ops->apply_microcode(cpu);
>>>
>>>
>>>
>>
>
>
Andrew,
may we have this patch in -mm? It has an ack from Jeremy as well.
thanks,
Anton.
On Thu, Dec 17, 2009 at 01:53:44PM +0000, Ian Campbell wrote:
> 871b72dd "x86: microcode: use smp_call_function_single instead of
> set_cpus_allowed, cleanup of synchronization logic" included:
>
> static int mc_sysdev_resume(struct sys_device *dev)
> {
> [...]
> + /*
> + * All non-bootup cpus are still disabled,
> + * so only CPU 0 will apply ucode here.
> + *
> + * Moreover, there can be no concurrent
> + * updates from any other places at this point.
> + */
> + WARN_ON(cpu != 0);
>
> However suspend/resume under Xen doesn't need to hot unplug all the CPUs, so we
> don't; the hypervisor can manage the context save/restore for all CPUs.
>
> It would be unnecessary to load microcode.ko in a Xen domU but if it does occur
> (e.g. because a distro installs the tools by default) we would like to avoid
> the warning on resume.
>
> Since the real constraint here is that we are running on the CPU for which we
> would like to load microcode (which in all practical circumstances is CPU0)
> just check for that and return if we are resuming a different CPU.
>
> There is no danger of concurrent updates, even if we ignore the fact that all
> but one CPUs are unplugged on native, because sysdev_resume() is single
> threaded.
>
> Signed-off-by: Ian Campbell <[email protected]>
> Cc: Jeremy Fitzhardinge <[email protected]>
> Cc: Dmitry Adamushko <[email protected]>
> Cc: Hugh Dickins <[email protected]>
> Cc: Ingo Molnar <[email protected]>
Acked-by: Anton Arapov <[email protected]>
> ---
> arch/x86/kernel/microcode_core.c | 11 +----------
> 1 files changed, 1 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> index 378e9a8..1153062 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -438,18 +438,9 @@ static int mc_sysdev_resume(struct sys_device *dev)
> int cpu = dev->id;
> struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
>
> - if (!cpu_online(cpu))
> + if (cpu != smp_processor_id())
> return 0;
>
> - /*
> - * All non-bootup cpus are still disabled,
> - * so only CPU 0 will apply ucode here.
> - *
> - * Moreover, there can be no concurrent
> - * updates from any other places at this point.
> - */
> - WARN_ON(cpu != 0);
> -
> if (uci->valid && uci->mc)
> microcode_ops->apply_microcode(cpu);
>
> --
> 1.5.6.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
--
Anton