xen/arch/x86/microcode.c | 54 +++++++++++++++++++++++++++++------------------ xen/common/stop_machine.c | 1 + 2 files changed, 35 insertions(+), 20 deletions(-)
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
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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.