This series allows the toolstack to choose (by way of XenStore platform flags) whether the domain is expecting to make use of RTC mode noack performance optimisations. It also switches the implict default back to rtc_mode_strict, which is the safe default for Xen to have. This fixes a longstanding regression in Win2k3SP2 only where its access pattern causes it to fall into an infinite loop reading RTC RegC. Questions for thought: * Does it make sense to allow customisation of ACPI_WAET_TIMER_ONE_READ ? There is no sitation where an admin should turn it off. * I believe this is safe wrt migrations, but am not certain enough to say for sure. This is a critical point, as the bad default (and regression) has been in Xen since late in the Xen-4.3 development cycle. --- Changes in v2: * Remove the hypervisor enum and replace with a formal ABI in params.h * Leave PM timer reads alone and accept fewer options for "true" -- 1.7.10.4
Andrew Cooper
2013-Dec-05 11:09 UTC
[Patch v2 1/2] x86/vRTC: Make rtc_mode_{strict, no_ack} a per-domain option
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
xen/arch/x86/hvm/hvm.c | 4 ++++
xen/arch/x86/hvm/rtc.c | 21 ++++++++-------------
xen/include/public/hvm/params.h | 11 ++++++++++-
3 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e2ba9de..06e52cb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4116,6 +4116,10 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE_PARAM(void) arg)
if ( a.value > SHUTDOWN_MAX )
rc = -EINVAL;
break;
+ case HVM_PARAM_RTC_MODE:
+ if ( a.value > 1 )
+ rc = -EINVAL;
+ break;
}
if ( rc == 0 )
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index cdedefe..644a255 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -45,14 +45,9 @@
#define epoch_year 1900
#define get_year(x) (x + epoch_year)
-enum rtc_mode {
- rtc_mode_no_ack,
- rtc_mode_strict
-};
-
-/* This must be in sync with how hvmloader sets the ACPI WAET flags. */
-#define mode_is(d, m) ((void)(d), rtc_mode_##m == rtc_mode_no_ack)
-#define rtc_mode_is(s, m) mode_is(vrtc_domain(s), m)
+#define rtc_mode_is(s, m) ( \
+ vrtc_domain(s)->arch.hvm_domain.params[HVM_PARAM_RTC_MODE] \
+ == HVM_PARAM_RTC_MODE_ ## m)
static void rtc_copy_date(RTCState *s);
static void rtc_set_time(RTCState *s);
@@ -63,7 +58,7 @@ static void rtc_update_irq(RTCState *s)
{
ASSERT(spin_is_locked(&s->lock));
- if ( rtc_mode_is(s, strict) && (s->hw.cmos_data[RTC_REG_C] &
RTC_IRQF) )
+ if ( rtc_mode_is(s, STRICT) && (s->hw.cmos_data[RTC_REG_C] &
RTC_IRQF) )
return;
/* IRQ is raised if any source is both raised & enabled */
@@ -73,7 +68,7 @@ static void rtc_update_irq(RTCState *s)
return;
s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
- if ( rtc_mode_is(s, no_ack) )
+ if ( rtc_mode_is(s, NO_ACK) )
hvm_isa_irq_deassert(vrtc_domain(s), RTC_IRQ);
hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
}
@@ -84,8 +79,8 @@ bool_t rtc_periodic_interrupt(void *opaque)
bool_t ret;
spin_lock(&s->lock);
- ret = rtc_mode_is(s, no_ack) || !(s->hw.cmos_data[RTC_REG_C] &
RTC_IRQF);
- if ( rtc_mode_is(s, no_ack) || !(s->hw.cmos_data[RTC_REG_C] &
RTC_PF) )
+ ret = rtc_mode_is(s, NO_ACK) || !(s->hw.cmos_data[RTC_REG_C] &
RTC_IRQF);
+ if ( rtc_mode_is(s, NO_ACK) || !(s->hw.cmos_data[RTC_REG_C] &
RTC_PF) )
{
s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
rtc_update_irq(s);
@@ -647,7 +642,7 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
case RTC_REG_C:
ret = s->hw.cmos_data[s->hw.cmos_index];
s->hw.cmos_data[RTC_REG_C] = 0x00;
- if ( (ret & RTC_IRQF) && !rtc_mode_is(s, no_ack) )
+ if ( (ret & RTC_IRQF) && !rtc_mode_is(s, NO_ACK) )
hvm_isa_irq_deassert(d, RTC_IRQ);
rtc_update_irq(s);
check_update_timer(s);
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 517a184..23289bd 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -145,6 +145,15 @@
/* SHUTDOWN_* action in case of a triple fault */
#define HVM_PARAM_TRIPLE_FAULT_REASON 31
-#define HVM_NR_PARAMS 32
+/*
+ * Domains RTC Mode, as informed in the WAET table.
+ * One of HVM_PARAM_RTC_MODE_*
+ */
+#define HVM_PARAM_RTC_MODE 32
+#define HVM_PARAM_RTC_MODE_STRICT 0 /* Normal behaviour */
+#define HVM_PARAM_RTC_MODE_NO_ACK 1 /* No need to ACK RegC */
+
+
+#define HVM_NR_PARAMS 33
#endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
--
1.7.10.4
Andrew Cooper
2013-Dec-05 11:09 UTC
[Patch v2 2/2] hvmloader: Allow the toolstack to choose WAET optimisations
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/firmware/hvmloader/acpi/acpi2_0.h | 4 ++++
tools/firmware/hvmloader/acpi/build.c | 19 +++++++++++++++++++
tools/firmware/hvmloader/acpi/static_tables.c | 12 +-----------
3 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/tools/firmware/hvmloader/acpi/acpi2_0.h
b/tools/firmware/hvmloader/acpi/acpi2_0.h
index 7b22d80..bebe4e6 100644
--- a/tools/firmware/hvmloader/acpi/acpi2_0.h
+++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
@@ -303,6 +303,10 @@ struct acpi_20_waet {
struct acpi_header header;
uint32_t flags;
};
+#define ACPI_WAET_RTC_NO_ACK (1<<0) /* RTC requires no int
acknowledge */
+#define ACPI_WAET_TIMER_ONE_READ (1<<1) /* PM timer requires only one
read */
+
+#define ACPI_WAET_DEFAULT_FLAGS (ACPI_WAET_TIMER_ONE_READ)
/*
* Multiple APIC Flags.
diff --git a/tools/firmware/hvmloader/acpi/build.c
b/tools/firmware/hvmloader/acpi/build.c
index f1dd3f0..a7f5596 100644
--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -23,6 +23,8 @@
#include "ssdt_pm.h"
#include "../config.h"
#include "../util.h"
+#include "../hypercall.h"
+#include <xen/hvm/params.h>
#include <xen/hvm/hvm_xs_strings.h>
#define ACPI_MAX_SECONDARY_TABLES 16
@@ -189,6 +191,9 @@ static struct acpi_20_hpet *construct_hpet(void)
static struct acpi_20_waet *construct_waet(void)
{
struct acpi_20_waet *waet;
+ const char *s;
+ struct xen_hvm_param p + { .domid = DOMID_SELF, .index =
HVM_PARAM_RTC_MODE };
waet = mem_alloc(sizeof(*waet), 16);
if (!waet) return NULL;
@@ -196,8 +201,22 @@ static struct acpi_20_waet *construct_waet(void)
memcpy(waet, &Waet, sizeof(*waet));
waet->header.length = sizeof(*waet);
+
+ s = xenstore_read("platform/waet-rtc-noack", NULL);
+ if ( s )
+ {
+ if ( !strncmp(s, "1", 1) )
+ waet->flags |= ACPI_WAET_RTC_NO_ACK;
+ else
+ waet->flags &= ~ACPI_WAET_RTC_NO_ACK;
+ }
+
set_checksum(waet, offsetof(struct acpi_header, checksum), sizeof(*waet));
+ /* Inform Xen which RTC mode has been chosen */
+ p.value = !!(waet->flags & ACPI_WAET_RTC_NO_ACK);
+ hypercall_hvm_op(HVMOP_set_param, &p);
+
return waet;
}
diff --git a/tools/firmware/hvmloader/acpi/static_tables.c
b/tools/firmware/hvmloader/acpi/static_tables.c
index 323ae31..5c699ba 100644
--- a/tools/firmware/hvmloader/acpi/static_tables.c
+++ b/tools/firmware/hvmloader/acpi/static_tables.c
@@ -136,16 +136,6 @@ struct acpi_20_rsdp Rsdp = {
.length = sizeof(struct acpi_20_rsdp)
};
-#define ACPI_WAET_RTC_NO_ACK (1<<0) /* RTC requires no int
acknowledge */
-#define ACPI_WAET_TIMER_ONE_READ (1<<1) /* PM timer requires only one
read */
-
-/*
- * The state of the RTC flag getting passed to the guest must be in
- * sync with the mode selection in the hypervisor RTC emulation code.
- */
-#define ACPI_WAET_FLAGS (ACPI_WAET_RTC_NO_ACK | \
- ACPI_WAET_TIMER_ONE_READ)
-
struct acpi_20_waet Waet = {
.header = {
.signature = ACPI_2_0_WAET_SIGNATURE,
@@ -157,7 +147,7 @@ struct acpi_20_waet Waet = {
.creator_id = ACPI_CREATOR_ID,
.creator_revision = ACPI_CREATOR_REVISION
},
- .flags = ACPI_WAET_FLAGS
+ .flags = ACPI_WAET_DEFAULT_FLAGS
};
/*
--
1.7.10.4
Keir Fraser
2013-Dec-05 15:38 UTC
Re: [Patch v2 2/2] hvmloader: Allow the toolstack to choose WAET optimisations
On 05/12/2013 11:09, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Keir Fraser <keir@xen.org> > CC: Jan Beulich <JBeulich@suse.com> > CC: Tim Deegan <tim@xen.org> > CC: Ian Campbell <Ian.Campbell@citrix.com> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>Acked-by: Keir Fraser <keir@xen.org>
>>> On 05.12.13 at 12:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > This series allows the toolstack to choose (by way of XenStore platform > flags) > whether the domain is expecting to make use of RTC mode noack performance > optimisations. > > It also switches the implict default back to rtc_mode_strict, which is the > safe default for Xen to have. > > This fixes a longstanding regression in Win2k3SP2 only where its access > pattern causes it to fall into an infinite loop reading RTC RegC.It''s not really clear to me how to read this: Since you still keep both modes and don''t change their behavior, I don''t see how you address the infinite loop in case such a domain got started with the mode set to no-ack. After all, both modes should - provided they''re consistent with what the WAET says - work with any guest (and hence there''s no "good" or "bad" default mode).> Questions for thought: > > * Does it make sense to allow customisation of ACPI_WAET_TIMER_ONE_READ ? > There is no sitation where an admin should turn it off.As said before - without currently seeing a use for making this configurable, we should leave it alone.> * I believe this is safe wrt migrations, but am not certain enough to say for > sure. This is a critical point, as the bad default (and regression) has > been in Xen since late in the Xen-4.3 development cycle.So a guest started on 4.3 and evaluating WAET won''t read REG_C during interrupts. I can''t see how this is not going to be a problem when on 4.4 the default would be the opposite. I think you''d need to default guests getting migrated to either the fixed no-ack mode that 4.3 used, or have the tools inspect its WAET and have them set the mode according to the bit there. It was only the inverse change that was safe (from 4.2 to 4.3): A guest reading REG_C even if it isn''t required to would still work correctly. Jan
Jan Beulich
2013-Dec-06 09:43 UTC
Re: [Patch v2 2/2] hvmloader: Allow the toolstack to choose WAET optimisations
>>> On 05.12.13 at 12:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > --- a/tools/firmware/hvmloader/acpi/acpi2_0.h > +++ b/tools/firmware/hvmloader/acpi/acpi2_0.h > @@ -303,6 +303,10 @@ struct acpi_20_waet { > struct acpi_header header; > uint32_t flags; > }; > +#define ACPI_WAET_RTC_NO_ACK (1<<0) /* RTC requires no int acknowledge */ > +#define ACPI_WAET_TIMER_ONE_READ (1<<1) /* PM timer requires only one read */ > + > +#define ACPI_WAET_DEFAULT_FLAGS (ACPI_WAET_TIMER_ONE_READ)Just like said on patch 1 - at least theoretically the safe mode (i.e. independent on whether the guest actually looks at WAET) is ACPI_WAET_RTC_NO_ACK set. Furthermore I don''t see why this definition needs to be in the header file. It could easily remain in the single consuming one.> @@ -196,8 +201,22 @@ static struct acpi_20_waet *construct_waet(void) > memcpy(waet, &Waet, sizeof(*waet)); > > waet->header.length = sizeof(*waet); > + > + s = xenstore_read("platform/waet-rtc-noack", NULL); > + if ( s ) > + { > + if ( !strncmp(s, "1", 1) )strncmp() or strcmp()?> + waet->flags |= ACPI_WAET_RTC_NO_ACK; > + else > + waet->flags &= ~ACPI_WAET_RTC_NO_ACK; > + } > + > set_checksum(waet, offsetof(struct acpi_header, checksum), sizeof(*waet)); > > + /* Inform Xen which RTC mode has been chosen */ > + p.value = !!(waet->flags & ACPI_WAET_RTC_NO_ACK);This either ought to be using the pseudo enumeration you added to the public header, or have build time checks that 0 and 1 have the expected meaning. Jan