* [PATCH] x86: don't deliver NMI to PVH Dom0
@ 2014-12-11 10:47 Jan Beulich
2014-12-11 11:41 ` Andrew Cooper
2014-12-11 13:00 ` Tim Deegan
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2014-12-11 10:47 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]
... for the time being: The mechanism used depends on the domain's use
of the IRET hypercall.
Also drop two bogus code lines spotted while going through the involved
code paths: Addresses of per-CPU variables can't possibly be NULL, and
the setting of st->vcpu in send_guest_trap()'s MCE case is redundant
with an earlier cmpxchgptr().
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3168,7 +3168,6 @@ static void nmi_mce_softirq(void)
int cpu = smp_processor_id();
struct softirq_trap *st = &per_cpu(softirq_trap, cpu);
- BUG_ON(st == NULL);
BUG_ON(st->vcpu == NULL);
/* Set the tmp value unconditionally, so that
@@ -3233,7 +3232,7 @@ static void nmi_hwdom_report(unsigned in
{
struct domain *d = hardware_domain;
- if ( (d == NULL) || (d->vcpu == NULL) || (d->vcpu[0] == NULL) )
+ if ( !d || !d->vcpu || !d->vcpu[0] || !is_pv_domain(d) /* PVH fixme */ )
return;
set_bit(reason_idx, nmi_reason(d));
@@ -3674,7 +3673,6 @@ int send_guest_trap(struct domain *d, ui
if ( !test_and_set_bool(v->mce_pending) ) {
st->domain = d;
- st->vcpu = v;
st->processor = v->processor;
/* not safe to wake up a vcpu here */
[-- Attachment #2: x86-PVH-suppress-NMI.patch --]
[-- Type: text/plain, Size: 1379 bytes --]
x86: don't deliver NMI to PVH Dom0
... for the time being: The mechanism used depends on the domain's use
of the IRET hypercall.
Also drop two bogus code lines spotted while going through the involved
code paths: Addresses of per-CPU variables can't possibly be NULL, and
the setting of st->vcpu in send_guest_trap()'s MCE case is redundant
with an earlier cmpxchgptr().
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3168,7 +3168,6 @@ static void nmi_mce_softirq(void)
int cpu = smp_processor_id();
struct softirq_trap *st = &per_cpu(softirq_trap, cpu);
- BUG_ON(st == NULL);
BUG_ON(st->vcpu == NULL);
/* Set the tmp value unconditionally, so that
@@ -3233,7 +3232,7 @@ static void nmi_hwdom_report(unsigned in
{
struct domain *d = hardware_domain;
- if ( (d == NULL) || (d->vcpu == NULL) || (d->vcpu[0] == NULL) )
+ if ( !d || !d->vcpu || !d->vcpu[0] || !is_pv_domain(d) /* PVH fixme */ )
return;
set_bit(reason_idx, nmi_reason(d));
@@ -3674,7 +3673,6 @@ int send_guest_trap(struct domain *d, ui
if ( !test_and_set_bool(v->mce_pending) ) {
st->domain = d;
- st->vcpu = v;
st->processor = v->processor;
/* not safe to wake up a vcpu here */
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: don't deliver NMI to PVH Dom0
2014-12-11 10:47 [PATCH] x86: don't deliver NMI to PVH Dom0 Jan Beulich
@ 2014-12-11 11:41 ` Andrew Cooper
2014-12-11 13:00 ` Tim Deegan
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2014-12-11 11:41 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 11/12/14 10:47, Jan Beulich wrote:
> ... for the time being: The mechanism used depends on the domain's use
> of the IRET hypercall.
>
> Also drop two bogus code lines spotted while going through the involved
> code paths: Addresses of per-CPU variables can't possibly be NULL, and
> the setting of st->vcpu in send_guest_trap()'s MCE case is redundant
> with an earlier cmpxchgptr().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -3168,7 +3168,6 @@ static void nmi_mce_softirq(void)
> int cpu = smp_processor_id();
> struct softirq_trap *st = &per_cpu(softirq_trap, cpu);
>
> - BUG_ON(st == NULL);
> BUG_ON(st->vcpu == NULL);
>
> /* Set the tmp value unconditionally, so that
> @@ -3233,7 +3232,7 @@ static void nmi_hwdom_report(unsigned in
> {
> struct domain *d = hardware_domain;
>
> - if ( (d == NULL) || (d->vcpu == NULL) || (d->vcpu[0] == NULL) )
> + if ( !d || !d->vcpu || !d->vcpu[0] || !is_pv_domain(d) /* PVH fixme */ )
> return;
>
> set_bit(reason_idx, nmi_reason(d));
> @@ -3674,7 +3673,6 @@ int send_guest_trap(struct domain *d, ui
>
> if ( !test_and_set_bool(v->mce_pending) ) {
> st->domain = d;
> - st->vcpu = v;
> st->processor = v->processor;
>
> /* not safe to wake up a vcpu here */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: don't deliver NMI to PVH Dom0
2014-12-11 10:47 [PATCH] x86: don't deliver NMI to PVH Dom0 Jan Beulich
2014-12-11 11:41 ` Andrew Cooper
@ 2014-12-11 13:00 ` Tim Deegan
2014-12-11 13:09 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Tim Deegan @ 2014-12-11 13:00 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
At 10:47 +0000 on 11 Dec (1418291241), Jan Beulich wrote:
> ... for the time being: The mechanism used depends on the domain's use
> of the IRET hypercall.
Can you elaborate? Looking at the existing code it seems like what it
does is set v->nmi_pending and make sure the vcpu gets scheduled
appropriately.
The HVM code will deliver an NMI if it sees v->nmi_pending (which is
what vlapic_accept_irq does, for example). Is there some other piece
I'm missing?
Cheers,
Tim.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: don't deliver NMI to PVH Dom0
2014-12-11 13:00 ` Tim Deegan
@ 2014-12-11 13:09 ` Jan Beulich
2014-12-11 13:16 ` Tim Deegan
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2014-12-11 13:09 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel, Keir Fraser
>>> On 11.12.14 at 14:00, <tim@xen.org> wrote:
> At 10:47 +0000 on 11 Dec (1418291241), Jan Beulich wrote:
>> ... for the time being: The mechanism used depends on the domain's use
>> of the IRET hypercall.
>
> Can you elaborate? Looking at the existing code it seems like what it
> does is set v->nmi_pending and make sure the vcpu gets scheduled
> appropriately.
>
> The HVM code will deliver an NMI if it sees v->nmi_pending (which is
> what vlapic_accept_irq does, for example). Is there some other piece
> I'm missing?
Just so that others see the answer too: The problem is that the
temporary affinity adjustment gets undone in the HYPERVISOR_iret
handler, yet PVH can't call that hypercall.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: don't deliver NMI to PVH Dom0
2014-12-11 13:09 ` Jan Beulich
@ 2014-12-11 13:16 ` Tim Deegan
2014-12-11 13:20 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Tim Deegan @ 2014-12-11 13:16 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
At 13:09 +0000 on 11 Dec (1418299748), Jan Beulich wrote:
> >>> On 11.12.14 at 14:00, <tim@xen.org> wrote:
> > At 10:47 +0000 on 11 Dec (1418291241), Jan Beulich wrote:
> >> ... for the time being: The mechanism used depends on the domain's use
> >> of the IRET hypercall.
> >
> > Can you elaborate? Looking at the existing code it seems like what it
> > does is set v->nmi_pending and make sure the vcpu gets scheduled
> > appropriately.
> >
> > The HVM code will deliver an NMI if it sees v->nmi_pending (which is
> > what vlapic_accept_irq does, for example). Is there some other piece
> > I'm missing?
>
> Just so that others see the answer too: The problem is that the
> temporary affinity adjustment gets undone in the HYPERVISOR_iret
> handler, yet PVH can't call that hypercall.
Thanks. So to make this work for PVH we'd want to do something like
calling async_exception_cleanup() from vlapic_EOI_set()?
Tim.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: don't deliver NMI to PVH Dom0
2014-12-11 13:16 ` Tim Deegan
@ 2014-12-11 13:20 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2014-12-11 13:20 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel, Keir Fraser
>>> On 11.12.14 at 14:16, <tim@xen.org> wrote:
> At 13:09 +0000 on 11 Dec (1418299748), Jan Beulich wrote:
>> >>> On 11.12.14 at 14:00, <tim@xen.org> wrote:
>> > At 10:47 +0000 on 11 Dec (1418291241), Jan Beulich wrote:
>> >> ... for the time being: The mechanism used depends on the domain's use
>> >> of the IRET hypercall.
>> >
>> > Can you elaborate? Looking at the existing code it seems like what it
>> > does is set v->nmi_pending and make sure the vcpu gets scheduled
>> > appropriately.
>> >
>> > The HVM code will deliver an NMI if it sees v->nmi_pending (which is
>> > what vlapic_accept_irq does, for example). Is there some other piece
>> > I'm missing?
>>
>> Just so that others see the answer too: The problem is that the
>> temporary affinity adjustment gets undone in the HYPERVISOR_iret
>> handler, yet PVH can't call that hypercall.
>
> Thanks. So to make this work for PVH we'd want to do something like
> calling async_exception_cleanup() from vlapic_EOI_set()?
No, that's the wrong place - there's no EOI involved in ACK-ing an NMI.
Instead there ought to be (optional?) VM exits upon clearing of the
NMI masked state, and their handling code should presumably call that
function.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-11 13:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-11 10:47 [PATCH] x86: don't deliver NMI to PVH Dom0 Jan Beulich
2014-12-11 11:41 ` Andrew Cooper
2014-12-11 13:00 ` Tim Deegan
2014-12-11 13:09 ` Jan Beulich
2014-12-11 13:16 ` Tim Deegan
2014-12-11 13:20 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.