xen/arch/x86/domctl.c | 61 +--------------------------------- xen/arch/x86/gdbsx.c | 70 +++++++++++++++++++++++++++++++++++++++- xen/arch/x86/include/asm/gdbsx.h | 16 +++++++-- 3 files changed, 83 insertions(+), 64 deletions(-)
When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL
pointer. One of several bugs here is known-but-compiled-out subops falling
into the default chain and hitting unrelated logic.
Remove the CONFIG_GDBSX ifdefary in arch_do_domctl() by implementing
gdbsx_domctl() and moving the logic across.
As minor cleanup,
* gdbsx_guest_mem_io() can become static
* Remove opencoding of domain_vcpu() and %pd
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Juergen Gross <jgross@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
v2:
* Implement the "split into new function" approach from the RFC.
v3:
* Switch to int.
* Insert missing break.
* static inline for stub.
---
xen/arch/x86/domctl.c | 61 +---------------------------------
xen/arch/x86/gdbsx.c | 70 +++++++++++++++++++++++++++++++++++++++-
xen/arch/x86/include/asm/gdbsx.h | 16 +++++++--
3 files changed, 83 insertions(+), 64 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index c20ab4352715..9131acb8a230 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -816,71 +816,12 @@ long arch_do_domctl(
}
#endif
-#ifdef CONFIG_GDBSX
case XEN_DOMCTL_gdbsx_guestmemio:
- ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio);
- if ( !ret )
- copyback = true;
- break;
-
case XEN_DOMCTL_gdbsx_pausevcpu:
- {
- struct vcpu *v;
-
- ret = -EBUSY;
- if ( !d->controller_pause_count )
- break;
- ret = -EINVAL;
- if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
- (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
- break;
- ret = vcpu_pause_by_systemcontroller(v);
- break;
- }
-
case XEN_DOMCTL_gdbsx_unpausevcpu:
- {
- struct vcpu *v;
-
- ret = -EBUSY;
- if ( !d->controller_pause_count )
- break;
- ret = -EINVAL;
- if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
- (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
- break;
- ret = vcpu_unpause_by_systemcontroller(v);
- if ( ret == -EINVAL )
- printk(XENLOG_G_WARNING
- "WARN: d%d attempting to unpause %pv which is not paused\n",
- currd->domain_id, v);
- break;
- }
-
case XEN_DOMCTL_gdbsx_domstatus:
- {
- struct vcpu *v;
-
- domctl->u.gdbsx_domstatus.vcpu_id = -1;
- domctl->u.gdbsx_domstatus.paused = d->controller_pause_count > 0;
- if ( domctl->u.gdbsx_domstatus.paused )
- {
- for_each_vcpu ( d, v )
- {
- if ( v->arch.gdbsx_vcpu_event )
- {
- domctl->u.gdbsx_domstatus.vcpu_id = v->vcpu_id;
- domctl->u.gdbsx_domstatus.vcpu_ev =
- v->arch.gdbsx_vcpu_event;
- v->arch.gdbsx_vcpu_event = 0;
- break;
- }
- }
- }
- copyback = true;
+ ret = gdbsx_domctl(d, domctl, ©back);
break;
- }
-#endif
case XEN_DOMCTL_setvcpuextstate:
case XEN_DOMCTL_getvcpuextstate:
diff --git a/xen/arch/x86/gdbsx.c b/xen/arch/x86/gdbsx.c
index 6ef46e8ea77d..21442f5dff1a 100644
--- a/xen/arch/x86/gdbsx.c
+++ b/xen/arch/x86/gdbsx.c
@@ -152,7 +152,8 @@ static unsigned int dbg_rw_guest_mem(struct domain *dp, unsigned long addr,
return len;
}
-int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop)
+static int gdbsx_guest_mem_io(
+ struct domain *d, struct xen_domctl_gdbsx_memio *iop)
{
if ( d && !d->is_dying )
{
@@ -178,6 +179,73 @@ void domain_pause_for_debugger(void)
send_global_virq(VIRQ_DEBUGGER);
}
+int gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback)
+{
+ struct vcpu *v;
+ int ret;
+
+ switch ( domctl->cmd )
+ {
+ case XEN_DOMCTL_gdbsx_guestmemio:
+ ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio);
+ if ( !ret )
+ *copyback = true;
+ break;
+
+ case XEN_DOMCTL_gdbsx_pausevcpu:
+ ret = -EBUSY;
+ if ( !d->controller_pause_count )
+ break;
+ ret = -EINVAL;
+ if ( (v = domain_vcpu(d, domctl->u.gdbsx_pauseunp_vcpu.vcpu)) == NULL )
+ break;
+ ret = vcpu_pause_by_systemcontroller(v);
+ break;
+
+ case XEN_DOMCTL_gdbsx_unpausevcpu:
+ ret = -EBUSY;
+ if ( !d->controller_pause_count )
+ break;
+ ret = -EINVAL;
+ if ( (v = domain_vcpu(d, domctl->u.gdbsx_pauseunp_vcpu.vcpu)) == NULL )
+ break;
+ ret = vcpu_unpause_by_systemcontroller(v);
+ if ( ret == -EINVAL )
+ printk(XENLOG_G_WARNING
+ "WARN: %pd attempting to unpause %pv which is not paused\n",
+ current->domain, v);
+ break;
+
+ case XEN_DOMCTL_gdbsx_domstatus:
+ ret = 0;
+ domctl->u.gdbsx_domstatus.vcpu_id = -1;
+ domctl->u.gdbsx_domstatus.paused = d->controller_pause_count > 0;
+ if ( domctl->u.gdbsx_domstatus.paused )
+ {
+ for_each_vcpu ( d, v )
+ {
+ if ( v->arch.gdbsx_vcpu_event )
+ {
+ domctl->u.gdbsx_domstatus.vcpu_id = v->vcpu_id;
+ domctl->u.gdbsx_domstatus.vcpu_ev =
+ v->arch.gdbsx_vcpu_event;
+ v->arch.gdbsx_vcpu_event = 0;
+ break;
+ }
+ }
+ }
+ *copyback = true;
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
+ ret = -ENOSYS;
+ break;
+ }
+
+ return ret;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/include/asm/gdbsx.h b/xen/arch/x86/include/asm/gdbsx.h
index 938eb74e2e25..e906be9ea318 100644
--- a/xen/arch/x86/include/asm/gdbsx.h
+++ b/xen/arch/x86/include/asm/gdbsx.h
@@ -2,18 +2,28 @@
#ifndef __X86_GDBX_H__
#define __X86_GDBX_H__
-#ifdef CONFIG_GDBSX
+#include <xen/stdbool.h>
struct domain;
-struct xen_domctl_gdbsx_memio;
+struct xen_domctl;
-int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop);
+#ifdef CONFIG_GDBSX
void domain_pause_for_debugger(void);
+int gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback);
+
#else
+#include <xen/errno.h>
+
static inline void domain_pause_for_debugger(void) {}
+static inline int gdbsx_domctl(
+ struct domain *d, struct xen_domctl *domctl, bool *copyback)
+{
+ return -ENOSYS;
+}
+
#endif /* CONFIG_GDBSX */
#endif /* __X86_GDBX_H__ */
--
2.11.0
On 25.04.2022 14:37, Andrew Cooper wrote:
> When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL
> pointer. One of several bugs here is known-but-compiled-out subops falling
> into the default chain and hitting unrelated logic.
>
> Remove the CONFIG_GDBSX ifdefary in arch_do_domctl() by implementing
> gdbsx_domctl() and moving the logic across.
>
> As minor cleanup,
> * gdbsx_guest_mem_io() can become static
> * Remove opencoding of domain_vcpu() and %pd
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Technically
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Yet as mentioned before, ...
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -816,71 +816,12 @@ long arch_do_domctl(
> }
> #endif
>
> -#ifdef CONFIG_GDBSX
> case XEN_DOMCTL_gdbsx_guestmemio:
> - ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio);
> - if ( !ret )
> - copyback = true;
> - break;
> -
> case XEN_DOMCTL_gdbsx_pausevcpu:
> - {
> - struct vcpu *v;
> -
> - ret = -EBUSY;
> - if ( !d->controller_pause_count )
> - break;
> - ret = -EINVAL;
> - if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
> - (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
> - break;
> - ret = vcpu_pause_by_systemcontroller(v);
> - break;
> - }
> -
> case XEN_DOMCTL_gdbsx_unpausevcpu:
> - {
> - struct vcpu *v;
> -
> - ret = -EBUSY;
> - if ( !d->controller_pause_count )
> - break;
> - ret = -EINVAL;
> - if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
> - (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
> - break;
> - ret = vcpu_unpause_by_systemcontroller(v);
> - if ( ret == -EINVAL )
> - printk(XENLOG_G_WARNING
> - "WARN: d%d attempting to unpause %pv which is not paused\n",
> - currd->domain_id, v);
> - break;
> - }
> -
> case XEN_DOMCTL_gdbsx_domstatus:
> - {
> - struct vcpu *v;
> -
> - domctl->u.gdbsx_domstatus.vcpu_id = -1;
> - domctl->u.gdbsx_domstatus.paused = d->controller_pause_count > 0;
> - if ( domctl->u.gdbsx_domstatus.paused )
> - {
> - for_each_vcpu ( d, v )
> - {
> - if ( v->arch.gdbsx_vcpu_event )
> - {
> - domctl->u.gdbsx_domstatus.vcpu_id = v->vcpu_id;
> - domctl->u.gdbsx_domstatus.vcpu_ev =
> - v->arch.gdbsx_vcpu_event;
> - v->arch.gdbsx_vcpu_event = 0;
> - break;
> - }
> - }
> - }
> - copyback = true;
> + ret = gdbsx_domctl(d, domctl, ©back);
> break;
> - }
> -#endif
... I'm not overly happy with the retaining of the case labels here
(and the knock on effect it'll have for other subsystem domctl-s),
so unlike usually this R-b isn't implicitly an A-b. Which doesn't
matter in practice, aiui ...
Jan
On 25/04/2022 2:36 pm, Jan Beulich wrote:
> On 25.04.2022 14:37, Andrew Cooper wrote:
>> When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL
>> pointer. One of several bugs here is known-but-compiled-out subops
>> falling
>> into the default chain and hitting unrelated logic.
>>
>> Remove the CONFIG_GDBSX ifdefary in arch_do_domctl() by implementing
>> gdbsx_domctl() and moving the logic across.
>>
>> As minor cleanup,
>> * gdbsx_guest_mem_io() can become static
>> * Remove opencoding of domain_vcpu() and %pd
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Technically
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
I'll tweak the commit message now that the IOMMU fix has gone in.
> Yet as mentioned before, ...
>
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -816,71 +816,12 @@ long arch_do_domctl(
>> }
>> #endif
>>
>> -#ifdef CONFIG_GDBSX
>> case XEN_DOMCTL_gdbsx_guestmemio:
>> - ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio);
>> - if ( !ret )
>> - copyback = true;
>> - break;
>> -
>> case XEN_DOMCTL_gdbsx_pausevcpu:
>> - {
>> - struct vcpu *v;
>> -
>> - ret = -EBUSY;
>> - if ( !d->controller_pause_count )
>> - break;
>> - ret = -EINVAL;
>> - if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
>> - (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) ==
>> NULL )
>> - break;
>> - ret = vcpu_pause_by_systemcontroller(v);
>> - break;
>> - }
>> -
>> case XEN_DOMCTL_gdbsx_unpausevcpu:
>> - {
>> - struct vcpu *v;
>> -
>> - ret = -EBUSY;
>> - if ( !d->controller_pause_count )
>> - break;
>> - ret = -EINVAL;
>> - if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
>> - (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) ==
>> NULL )
>> - break;
>> - ret = vcpu_unpause_by_systemcontroller(v);
>> - if ( ret == -EINVAL )
>> - printk(XENLOG_G_WARNING
>> - "WARN: d%d attempting to unpause %pv which is not
>> paused\n",
>> - currd->domain_id, v);
>> - break;
>> - }
>> -
>> case XEN_DOMCTL_gdbsx_domstatus:
>> - {
>> - struct vcpu *v;
>> -
>> - domctl->u.gdbsx_domstatus.vcpu_id = -1;
>> - domctl->u.gdbsx_domstatus.paused = d->controller_pause_count
>> > 0;
>> - if ( domctl->u.gdbsx_domstatus.paused )
>> - {
>> - for_each_vcpu ( d, v )
>> - {
>> - if ( v->arch.gdbsx_vcpu_event )
>> - {
>> - domctl->u.gdbsx_domstatus.vcpu_id = v->vcpu_id;
>> - domctl->u.gdbsx_domstatus.vcpu_ev =
>> - v->arch.gdbsx_vcpu_event;
>> - v->arch.gdbsx_vcpu_event = 0;
>> - break;
>> - }
>> - }
>> - }
>> - copyback = true;
>> + ret = gdbsx_domctl(d, domctl, ©back);
>> break;
>> - }
>> -#endif
>
> ... I'm not overly happy with the retaining of the case labels here
> (and the knock on effect it'll have for other subsystem domctl-s),
The crash in do_iommu_op() happened because things which weren't iommu
subops ended up in a function only expecting iommu subops, *because*
unrelated case labels got compiled out.
We've also had multiple XSAs elsewhere created by related "just chain
everything through default:" patterns. The legacy MSR paths are still
especially guilty of doing this.
The case labels need to never ever get compiled out, even if their
subsystem has been.
So you have a choice between this patch, or a pattern of the form:
case XEN_DOMCTL_gdbsx_domstatus:
if ( !IS_ENABLED(CONFIG_GDBSX) )
{
ret = -Exxx;
break;
}
...
but the top level case labels need to stay one way or another.
For this patch, it moves a chunk of logic out of a fairly generic file
into its proper subsystem file, and a few extra case labels is a very
cheap price to pay.
~Andrew
© 2016 - 2024 Red Hat, Inc.