xen/arch/x86/microcode.c | 54 +++++++++++++++++++++++++++++------------------ xen/common/rcupdate.c | 4 ++++ xen/common/stop_machine.c | 7 ++++++ 3 files changed, 45 insertions(+), 20 deletions(-)
With core scheduling active it is mandatory for stop_machine_run() to
be called in idle context only (so either during boot or in a tasklet),
as otherwise a scheduling deadlock would occur: stop_machine_run()
does a cpu rendezvous by activating a tasklet on all other cpus. In
case stop_machine_run() was not called in an idle vcpu it would block
scheduling the idle vcpu on its siblings with core scheduling being
active, resulting in a hang.
Put a BUG_ON() into stop_machine_run() to test for being called in an
idle vcpu only and adapt the missing call site (ucode loading) to use a
tasklet for calling stop_machine_run().
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- rephrase commit message (Julien Grall)
V3:
- add comments (Jan Beulich)
---
xen/arch/x86/microcode.c | 54 +++++++++++++++++++++++++++++------------------
xen/common/rcupdate.c | 4 ++++
xen/common/stop_machine.c | 7 ++++++
3 files changed, 45 insertions(+), 20 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 35c1d36cdc..4cf4e66b18 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -562,30 +562,18 @@ static int do_microcode_update(void *patch)
return ret;
}
-int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
+struct ucode_buf {
+ unsigned int len;
+ char buffer[];
+};
+
+static long microcode_update_helper(void *data)
{
int ret;
- void *buffer;
+ struct ucode_buf *buffer = data;
unsigned int cpu, updated;
struct microcode_patch *patch;
- if ( len != (uint32_t)len )
- return -E2BIG;
-
- if ( microcode_ops == NULL )
- return -EINVAL;
-
- buffer = xmalloc_bytes(len);
- if ( !buffer )
- return -ENOMEM;
-
- ret = copy_from_guest(buffer, buf, len);
- if ( ret )
- {
- xfree(buffer);
- return -EFAULT;
- }
-
/* cpu_online_map must not change during update */
if ( !get_cpu_maps() )
{
@@ -607,7 +595,7 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
return -EPERM;
}
- patch = parse_blob(buffer, len);
+ patch = parse_blob(buffer->buffer, buffer->len);
xfree(buffer);
if ( IS_ERR(patch) )
{
@@ -700,6 +688,32 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
return ret;
}
+int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
+{
+ int ret;
+ struct ucode_buf *buffer;
+
+ if ( len != (uint32_t)len )
+ return -E2BIG;
+
+ if ( microcode_ops == NULL )
+ return -EINVAL;
+
+ buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
+ if ( !buffer )
+ return -ENOMEM;
+
+ ret = copy_from_guest(buffer->buffer, buf, len);
+ if ( ret )
+ {
+ xfree(buffer);
+ return -EFAULT;
+ }
+ buffer->len = len;
+
+ return continue_hypercall_on_cpu(0, microcode_update_helper, buffer);
+}
+
static int __init microcode_init(void)
{
/*
diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index 91d4ad0fd8..d76b991627 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -178,6 +178,10 @@ static int rcu_barrier_action(void *_cpu_count)
return 0;
}
+/*
+ * As rcu_barrier() is using stop_machine_run() it is allowed to be used in
+ * idle context only (see comment for stop_machine_run()).
+ */
int rcu_barrier(void)
{
atomic_t cpu_count = ATOMIC_INIT(0);
diff --git a/xen/common/stop_machine.c b/xen/common/stop_machine.c
index 33d9602217..2d5f6aef61 100644
--- a/xen/common/stop_machine.c
+++ b/xen/common/stop_machine.c
@@ -67,6 +67,12 @@ static void stopmachine_wait_state(void)
cpu_relax();
}
+/*
+ * Sync all processors and call a function on one or all of them.
+ * As stop_machine_run() is using a tasklet for syncing the processors it is
+ * mandatory to be called only on an idle vcpu, as otherwise active core
+ * scheduling might hang.
+ */
int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
{
unsigned int i, nr_cpus;
@@ -74,6 +80,7 @@ int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
int ret;
BUG_ON(!local_irq_is_enabled());
+ BUG_ON(!is_idle_vcpu(current));
/* cpu_online_map must not change. */
if ( !get_cpu_maps() )
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 28/02/2020 17:13, Juergen Gross wrote:
> @@ -700,6 +688,32 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> return ret;
> }
>
> +int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> +{
> + int ret;
> + struct ucode_buf *buffer;
> +
> + if ( len != (uint32_t)len )
> + return -E2BIG;
> +
> + if ( microcode_ops == NULL )
> + return -EINVAL;
> +
> + buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
> + if ( !buffer )
> + return -ENOMEM;
> +
> + ret = copy_from_guest(buffer->buffer, buf, len);
> + if ( ret )
> + {
> + xfree(buffer);
> + return -EFAULT;
> + }
> + buffer->len = len;
> +
> + return continue_hypercall_on_cpu(0, microcode_update_helper, buffer);
Any reason why cpu 0 here? There is no restriction at the moment, and
running the tasklet on the current CPU is surely better than poking
CPU0's tasklet queue remotely, then interrupting it.
Everything else looks ok. This adjustments could be done on commit to
save a v4.
~Andrew
P.S. Might it be sensible to have a continue_hypercall_in_tasklet()
wrapper which passes smp_processor_id() into continue_hypercall_on_cpu()?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 28.02.20 20:06, Andrew Cooper wrote:
> On 28/02/2020 17:13, Juergen Gross wrote:
>> @@ -700,6 +688,32 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>> return ret;
>> }
>>
>> +int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>> +{
>> + int ret;
>> + struct ucode_buf *buffer;
>> +
>> + if ( len != (uint32_t)len )
>> + return -E2BIG;
>> +
>> + if ( microcode_ops == NULL )
>> + return -EINVAL;
>> +
>> + buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
>> + if ( !buffer )
>> + return -ENOMEM;
>> +
>> + ret = copy_from_guest(buffer->buffer, buf, len);
>> + if ( ret )
>> + {
>> + xfree(buffer);
>> + return -EFAULT;
>> + }
>> + buffer->len = len;
>> +
>> + return continue_hypercall_on_cpu(0, microcode_update_helper, buffer);
>
> Any reason why cpu 0 here? There is no restriction at the moment, and
> running the tasklet on the current CPU is surely better than poking
> CPU0's tasklet queue remotely, then interrupting it.
As stop_machine_run() is scheduling a tasklet on all other cpus it
doesn't really matter. In the end I don't really mind either way.
>
> Everything else looks ok. This adjustments could be done on commit to
> save a v4.
>
> ~Andrew
>
> P.S. Might it be sensible to have a continue_hypercall_in_tasklet()
> wrapper which passes smp_processor_id() into continue_hypercall_on_cpu()?
When a second user is coming up, maybe.
The other would be continue_hypercall_on_bootcpu().
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 29.02.2020 06:47, Jürgen Groß wrote:
> On 28.02.20 20:06, Andrew Cooper wrote:
>> On 28/02/2020 17:13, Juergen Gross wrote:
>>> @@ -700,6 +688,32 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>> return ret;
>>> }
>>>
>>> +int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>> +{
>>> + int ret;
>>> + struct ucode_buf *buffer;
>>> +
>>> + if ( len != (uint32_t)len )
>>> + return -E2BIG;
>>> +
>>> + if ( microcode_ops == NULL )
>>> + return -EINVAL;
>>> +
>>> + buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
>>> + if ( !buffer )
>>> + return -ENOMEM;
>>> +
>>> + ret = copy_from_guest(buffer->buffer, buf, len);
>>> + if ( ret )
>>> + {
>>> + xfree(buffer);
>>> + return -EFAULT;
>>> + }
>>> + buffer->len = len;
>>> +
>>> + return continue_hypercall_on_cpu(0, microcode_update_helper, buffer);
>>
>> Any reason why cpu 0 here? There is no restriction at the moment, and
>> running the tasklet on the current CPU is surely better than poking
>> CPU0's tasklet queue remotely, then interrupting it.
>
> As stop_machine_run() is scheduling a tasklet on all other cpus it
> doesn't really matter. In the end I don't really mind either way.
I think any case where we can avoid assigning special meaning
to CPU 0 is helpful. While we won't get to being able to offline
the BSP any time soon, we shouldn't put more road blocks on the
path there.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 02.03.20 10:06, Jan Beulich wrote:
> On 29.02.2020 06:47, Jürgen Groß wrote:
>> On 28.02.20 20:06, Andrew Cooper wrote:
>>> On 28/02/2020 17:13, Juergen Gross wrote:
>>>> @@ -700,6 +688,32 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>>> return ret;
>>>> }
>>>>
>>>> +int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>>> +{
>>>> + int ret;
>>>> + struct ucode_buf *buffer;
>>>> +
>>>> + if ( len != (uint32_t)len )
>>>> + return -E2BIG;
>>>> +
>>>> + if ( microcode_ops == NULL )
>>>> + return -EINVAL;
>>>> +
>>>> + buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
>>>> + if ( !buffer )
>>>> + return -ENOMEM;
>>>> +
>>>> + ret = copy_from_guest(buffer->buffer, buf, len);
>>>> + if ( ret )
>>>> + {
>>>> + xfree(buffer);
>>>> + return -EFAULT;
>>>> + }
>>>> + buffer->len = len;
>>>> +
>>>> + return continue_hypercall_on_cpu(0, microcode_update_helper, buffer);
>>>
>>> Any reason why cpu 0 here? There is no restriction at the moment, and
>>> running the tasklet on the current CPU is surely better than poking
>>> CPU0's tasklet queue remotely, then interrupting it.
>>
>> As stop_machine_run() is scheduling a tasklet on all other cpus it
>> doesn't really matter. In the end I don't really mind either way.
>
> I think any case where we can avoid assigning special meaning
> to CPU 0 is helpful. While we won't get to being able to offline
> the BSP any time soon, we shouldn't put more road blocks on the
> path there.
As I said: fine with me. Shall I resend or can this be done while
committing?
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 02.03.2020 10:37, Jürgen Groß wrote:
> On 02.03.20 10:06, Jan Beulich wrote:
>> On 29.02.2020 06:47, Jürgen Groß wrote:
>>> On 28.02.20 20:06, Andrew Cooper wrote:
>>>> On 28/02/2020 17:13, Juergen Gross wrote:
>>>>> @@ -700,6 +688,32 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>>>> return ret;
>>>>> }
>>>>>
>>>>> +int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>>>> +{
>>>>> + int ret;
>>>>> + struct ucode_buf *buffer;
>>>>> +
>>>>> + if ( len != (uint32_t)len )
>>>>> + return -E2BIG;
>>>>> +
>>>>> + if ( microcode_ops == NULL )
>>>>> + return -EINVAL;
>>>>> +
>>>>> + buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
>>>>> + if ( !buffer )
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + ret = copy_from_guest(buffer->buffer, buf, len);
>>>>> + if ( ret )
>>>>> + {
>>>>> + xfree(buffer);
>>>>> + return -EFAULT;
>>>>> + }
>>>>> + buffer->len = len;
>>>>> +
>>>>> + return continue_hypercall_on_cpu(0, microcode_update_helper, buffer);
>>>>
>>>> Any reason why cpu 0 here? There is no restriction at the moment, and
>>>> running the tasklet on the current CPU is surely better than poking
>>>> CPU0's tasklet queue remotely, then interrupting it.
>>>
>>> As stop_machine_run() is scheduling a tasklet on all other cpus it
>>> doesn't really matter. In the end I don't really mind either way.
>>
>> I think any case where we can avoid assigning special meaning
>> to CPU 0 is helpful. While we won't get to being able to offline
>> the BSP any time soon, we shouldn't put more road blocks on the
>> path there.
>
> As I said: fine with me. Shall I resend or can this be done while
> committing?
No need to re-send. And for completeness, with the adjustment
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.