[Xen-devel] [PATCH] xen: make sure stop_machine_run() is always called in a tasklet

Juergen Gross posted 1 patch 4 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200211093527.6811-1-jgross@suse.com
There is a newer version of this series
xen/arch/x86/microcode.c  | 54 +++++++++++++++++++++++++++++------------------
xen/common/stop_machine.c |  1 +
2 files changed, 35 insertions(+), 20 deletions(-)
[Xen-devel] [PATCH] xen: make sure stop_machine_run() is always called in a tasklet
Posted by Juergen Gross 4 years, 1 month ago
With core scheduling active it is mandatory for stop_machine_run() to
be called in a tasklet only, 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>
---
 xen/arch/x86/microcode.c  | 54 +++++++++++++++++++++++++++++------------------
 xen/common/stop_machine.c |  1 +
 2 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index c0fb690f79..8e61769377 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -561,30 +561,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() )
     {
@@ -606,7 +594,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) )
     {
@@ -699,6 +687,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/stop_machine.c b/xen/common/stop_machine.c
index 33d9602217..fe7f7d4447 100644
--- a/xen/common/stop_machine.c
+++ b/xen/common/stop_machine.c
@@ -74,6 +74,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-devel
Re: [Xen-devel] [PATCH] xen: make sure stop_machine_run() is always called in a tasklet
Posted by Julien Grall 4 years, 1 month ago
Hi,

On 11/02/2020 10:35, Juergen Gross wrote:
> With core scheduling active it is mandatory for stop_machine_run() to
> be called in a tasklet only, 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.

This suggests it is not safe to call stop_machine_run() outside a 
tasklet but still under "idle vCPU" context. However, alternative 
patching on Arm during boot will not be in a tasklet. Is it going to be 
safe?


Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make sure stop_machine_run() is always called in a tasklet
Posted by Jürgen Groß 4 years, 1 month ago
On 13.02.20 10:01, Julien Grall wrote:
> Hi,
> 
> On 11/02/2020 10:35, Juergen Gross wrote:
>> With core scheduling active it is mandatory for stop_machine_run() to
>> be called in a tasklet only, 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.
> 
> This suggests it is not safe to call stop_machine_run() outside a 
> tasklet but still under "idle vCPU" context. However, alternative 
> patching on Arm during boot will not be in a tasklet. Is it going to be 
> safe?

Yes.

I can rephrase that part to make it clear.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make sure stop_machine_run() is always called in a tasklet
Posted by Julien Grall 4 years, 1 month ago
Hi,

On 13/02/2020 11:01, Jürgen Groß wrote:
> On 13.02.20 10:01, Julien Grall wrote:
>> Hi,
>>
>> On 11/02/2020 10:35, Juergen Gross wrote:
>>> With core scheduling active it is mandatory for stop_machine_run() to
>>> be called in a tasklet only, 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.
>>
>> This suggests it is not safe to call stop_machine_run() outside a 
>> tasklet but still under "idle vCPU" context. However, alternative 
>> patching on Arm during boot will not be in a tasklet. Is it going to 
>> be safe?
> 
> Yes.
> 
> I can rephrase that part to make it clear.

That would nice. Thank you!

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make sure stop_machine_run() is always called in a tasklet
Posted by Igor Druzhinin 4 years, 1 month ago
On 11/02/2020 09:35, Juergen Gross wrote:
> With core scheduling active it is mandatory for stop_machine_run() to
> be called in a tasklet only, 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.

I suppose rcu_barrier() is fine due to process_pending_softirqs() being
called inside? I'm a little concerned by imposing is_vcpu_idle() restriction
in that case as rcu_barrier() could be technically called from a non-tasklet
context.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make sure stop_machine_run() is always called in a tasklet
Posted by Jürgen Groß 4 years, 1 month ago
On 14.02.20 15:06, Igor Druzhinin wrote:
> On 11/02/2020 09:35, Juergen Gross wrote:
>> With core scheduling active it is mandatory for stop_machine_run() to
>> be called in a tasklet only, 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.
> 
> I suppose rcu_barrier() is fine due to process_pending_softirqs() being
> called inside? I'm a little concerned by imposing is_vcpu_idle() restriction
> in that case as rcu_barrier() could be technically called from a non-tasklet
> context.

No, stop_machine_run() with core scheduling active can only work when
called in an idle vcpu.

OTOH it would be fairly easy to add another softirq for a similar
purpose and have a sync_machine_run() using that instead of tasklets.
This could be used for ucode loading, too.

stop_machine_run() and sync_machine_run() could use a common main
function. The patch should be rather simple.

Thoughts?


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make sure stop_machine_run() is always called in a tasklet
Posted by Igor Druzhinin 4 years, 1 month ago
On 14/02/2020 16:39, Jürgen Groß wrote:
> On 14.02.20 15:06, Igor Druzhinin wrote:
>> On 11/02/2020 09:35, Juergen Gross wrote:
>>> With core scheduling active it is mandatory for stop_machine_run() to
>>> be called in a tasklet only, 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.
>>
>> I suppose rcu_barrier() is fine due to process_pending_softirqs() being
>> called inside? I'm a little concerned by imposing is_vcpu_idle() restriction
>> in that case as rcu_barrier() could be technically called from a non-tasklet
>> context.
> 
> No, stop_machine_run() with core scheduling active can only work when
> called in an idle vcpu.
> 
> OTOH it would be fairly easy to add another softirq for a similar
> purpose and have a sync_machine_run() using that instead of tasklets.
> This could be used for ucode loading, too.
> 
> stop_machine_run() and sync_machine_run() could use a common main
> function. The patch should be rather simple.
> 
> Thoughts?

I have a patch on the list (which I was planning to send a v2 for) that
fixes another issue with rcu_barrier():
https://lists.xenproject.org/archives/html/xen-devel/2020-01/msg02273.html

As I understand it now that wouldn't work with core-scheduling. Do you think
it's possible to synchronously wait for tasklets to finish in non-tasklet
context (because that's what the purpose of rcu_barrier() is)?

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make sure stop_machine_run() is always called in a tasklet
Posted by Jürgen Groß 4 years, 1 month ago
On 14.02.20 18:34, Igor Druzhinin wrote:
> On 14/02/2020 16:39, Jürgen Groß wrote:
>> On 14.02.20 15:06, Igor Druzhinin wrote:
>>> On 11/02/2020 09:35, Juergen Gross wrote:
>>>> With core scheduling active it is mandatory for stop_machine_run() to
>>>> be called in a tasklet only, 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.
>>>
>>> I suppose rcu_barrier() is fine due to process_pending_softirqs() being
>>> called inside? I'm a little concerned by imposing is_vcpu_idle() restriction
>>> in that case as rcu_barrier() could be technically called from a non-tasklet
>>> context.
>>
>> No, stop_machine_run() with core scheduling active can only work when
>> called in an idle vcpu.
>>
>> OTOH it would be fairly easy to add another softirq for a similar
>> purpose and have a sync_machine_run() using that instead of tasklets.
>> This could be used for ucode loading, too.
>>
>> stop_machine_run() and sync_machine_run() could use a common main
>> function. The patch should be rather simple.
>>
>> Thoughts?
> 
> I have a patch on the list (which I was planning to send a v2 for) that
> fixes another issue with rcu_barrier():
> https://lists.xenproject.org/archives/html/xen-devel/2020-01/msg02273.html
> 
> As I understand it now that wouldn't work with core-scheduling. Do you think
> it's possible to synchronously wait for tasklets to finish in non-tasklet
> context (because that's what the purpose of rcu_barrier() is)?

No, won't work, unless we add preemption (basically would need per-vcpu
stacks instead of per-pcpu ones).

What might work IMO would be to do rcu_process_callbacks() no longer
during idle, but to have a specific softirq for that purpose. This would
remove the need to involve scheduling for rcu_barrier(). A brief check
of process_pending_softirqs() callers seems to allow that, but I'd like
to have a second opinion from someone having more rcu knowledge than me.
Single problematic users of process_pending_softirqs() could still be
switched to a variant not allowing the new rcu softirq.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel