Jan Beulich
2013-Jul-02 07:02 UTC
[PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
As the mode not conforming to the hardware specification (by allowing
the guest to skip the REG C reads in its interrupt handler) is a
Viridian invention, it seems logical to tie this mode to that extension
being enabled. If the extension is disabled, proper hardware emulation
will be done instead.
The main thing necessary here is the synchronization of the RTC
emulation code and the setting of the respective flag in hvmloader''s
creation of the ACPI WAET table.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/firmware/hvmloader/acpi/acpi2_0.h
+++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
@@ -304,6 +304,9 @@ struct acpi_20_waet {
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 */
+
/*
* Multiple APIC Flags.
*/
--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -22,8 +22,10 @@
#include "ssdt_tpm.h"
#include "ssdt_pm.h"
#include "../config.h"
+#include "../hypercall.h"
#include "../util.h"
#include <xen/hvm/hvm_xs_strings.h>
+#include <xen/hvm/params.h>
#define ACPI_MAX_SECONDARY_TABLES 16
@@ -189,6 +191,7 @@ static struct acpi_20_hpet *construct_hp
static struct acpi_20_waet *construct_waet(void)
{
struct acpi_20_waet *waet;
+ xen_hvm_param_t param;
waet = mem_alloc(sizeof(*waet), 16);
if (!waet) return NULL;
@@ -196,6 +199,19 @@ static struct acpi_20_waet *construct_wa
memcpy(waet, &Waet, sizeof(*waet));
waet->header.length = sizeof(*waet);
+
+ /*
+ * Check whether Viridian emulation is enabled: 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.
+ */
+ param.domid = DOMID_SELF;
+ param.index = HVM_PARAM_VIRIDIAN;
+ if ( hypercall_hvm_op(HVMOP_get_param, ¶m) )
+ BUG();
+ if ( param.value )
+ waet->flags |= ACPI_WAET_RTC_NO_ACK;
+
set_checksum(waet, offsetof(struct acpi_header, checksum), sizeof(*waet));
return waet;
--- 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_TIMER_ONE_READ
};
/*
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -51,7 +51,9 @@ enum rtc_mode {
};
/* 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 mode_is(d, m) (is_viridian_domain(d) ? \
+ rtc_mode_##m == rtc_mode_no_ack : \
+ rtc_mode_##m == rtc_mode_strict)
#define rtc_mode_is(s, m) mode_is(vrtc_domain(s), m)
static void rtc_copy_date(RTCState *s);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Paul Durrant
2013-Jul-02 08:01 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 02 July 2013 08:03 > To: xen-devel > Cc: Paul Durrant; George Dunlap; Keir (Xen.org); Tim (Xen.org) > Subject: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian > emulation > > As the mode not conforming to the hardware specification (by allowing > the guest to skip the REG C reads in its interrupt handler) is a > Viridian invention, it seems logical to tie this mode to that extension > being enabled. If the extension is disabled, proper hardware emulation > will be done instead. > > The main thing necessary here is the synchronization of the RTC > emulation code and the setting of the respective flag in hvmloader''s > creation of the ACPI WAET table. >Do we need to hardcode no_ack in the viridian case? Can we not be more flexible and just have the WAET reflect whatever mode is in use? Paul
Jan Beulich
2013-Jul-02 08:22 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
>>> On 02.07.13 at 10:01, Paul Durrant <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 02 July 2013 08:03 >> To: xen-devel >> Cc: Paul Durrant; George Dunlap; Keir (Xen.org); Tim (Xen.org) >> Subject: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian >> emulation >> >> As the mode not conforming to the hardware specification (by allowing >> the guest to skip the REG C reads in its interrupt handler) is a >> Viridian invention, it seems logical to tie this mode to that extension >> being enabled. If the extension is disabled, proper hardware emulation >> will be done instead. >> >> The main thing necessary here is the synchronization of the RTC >> emulation code and the setting of the respective flag in hvmloader''s >> creation of the ACPI WAET table. >> > > Do we need to hardcode no_ack in the viridian case? Can we not be more > flexible and just have the WAET reflect whatever mode is in use?We could, but is the extra work involved in coding this up (would require a new HVM param) worth it? Newer Windows really wants it that way... Jan
Paul Durrant
2013-Jul-02 08:34 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 02 July 2013 09:22 > To: Paul Durrant > Cc: George Dunlap; xen-devel; Keir (Xen.org); Tim (Xen.org) > Subject: RE: [PATCH] x86/HVM: tie RTC emulation mode to enabling of > Viridian emulation > > >>> On 02.07.13 at 10:01, Paul Durrant <Paul.Durrant@citrix.com> wrote: > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: 02 July 2013 08:03 > >> To: xen-devel > >> Cc: Paul Durrant; George Dunlap; Keir (Xen.org); Tim (Xen.org) > >> Subject: [PATCH] x86/HVM: tie RTC emulation mode to enabling of > Viridian > >> emulation > >> > >> As the mode not conforming to the hardware specification (by allowing > >> the guest to skip the REG C reads in its interrupt handler) is a > >> Viridian invention, it seems logical to tie this mode to that extension > >> being enabled. If the extension is disabled, proper hardware emulation > >> will be done instead. > >> > >> The main thing necessary here is the synchronization of the RTC > >> emulation code and the setting of the respective flag in hvmloader''s > >> creation of the ACPI WAET table. > >> > > > > Do we need to hardcode no_ack in the viridian case? Can we not be more > > flexible and just have the WAET reflect whatever mode is in use? > > We could, but is the extra work involved in coding this up (would > require a new HVM param) worth it? Newer Windows really wants > it that way... >Ok. Fair enough. I guess the parameter could be introduced should the need arise. Ack-ed by: Paul Durrant <paul.durrant@citrix.com>
Tim Deegan
2013-Jul-02 09:11 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
At 08:02 +0100 on 02 Jul (1372752161), Jan Beulich wrote:> As the mode not conforming to the hardware specification (by allowing > the guest to skip the REG C reads in its interrupt handler) is a > Viridian invention, it seems logical to tie this mode to that extension > being enabled. If the extension is disabled, proper hardware emulation > will be done instead. > > The main thing necessary here is the synchronization of the RTC > emulation code and the setting of the respective flag in hvmloader''s > creation of the ACPI WAET table. > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Wasn''t this going to have its own param, defaulting to off on create and to on on migrate? I suspect most people just leave the viridian flag on for all domains. Tim.> --- a/tools/firmware/hvmloader/acpi/acpi2_0.h > +++ b/tools/firmware/hvmloader/acpi/acpi2_0.h > @@ -304,6 +304,9 @@ struct acpi_20_waet { > 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 */ > + > /* > * Multiple APIC Flags. > */ > --- a/tools/firmware/hvmloader/acpi/build.c > +++ b/tools/firmware/hvmloader/acpi/build.c > @@ -22,8 +22,10 @@ > #include "ssdt_tpm.h" > #include "ssdt_pm.h" > #include "../config.h" > +#include "../hypercall.h" > #include "../util.h" > #include <xen/hvm/hvm_xs_strings.h> > +#include <xen/hvm/params.h> > > #define ACPI_MAX_SECONDARY_TABLES 16 > > @@ -189,6 +191,7 @@ static struct acpi_20_hpet *construct_hp > static struct acpi_20_waet *construct_waet(void) > { > struct acpi_20_waet *waet; > + xen_hvm_param_t param; > > waet = mem_alloc(sizeof(*waet), 16); > if (!waet) return NULL; > @@ -196,6 +199,19 @@ static struct acpi_20_waet *construct_wa > memcpy(waet, &Waet, sizeof(*waet)); > > waet->header.length = sizeof(*waet); > + > + /* > + * Check whether Viridian emulation is enabled: 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. > + */ > + param.domid = DOMID_SELF; > + param.index = HVM_PARAM_VIRIDIAN; > + if ( hypercall_hvm_op(HVMOP_get_param, ¶m) ) > + BUG(); > + if ( param.value ) > + waet->flags |= ACPI_WAET_RTC_NO_ACK; > + > set_checksum(waet, offsetof(struct acpi_header, checksum), sizeof(*waet)); > > return waet; > --- 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_TIMER_ONE_READ > }; > > /* > --- a/xen/arch/x86/hvm/rtc.c > +++ b/xen/arch/x86/hvm/rtc.c > @@ -51,7 +51,9 @@ enum rtc_mode { > }; > > /* 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 mode_is(d, m) (is_viridian_domain(d) ? \ > + rtc_mode_##m == rtc_mode_no_ack : \ > + rtc_mode_##m == rtc_mode_strict) > #define rtc_mode_is(s, m) mode_is(vrtc_domain(s), m) > > static void rtc_copy_date(RTCState *s); > > >> x86/HVM: tie RTC emulation mode to enabling of Viridian emulation > > As the mode not conforming to the hardware specification (by allowing > the guest to skip the REG C reads in its interrupt handler) is a > Viridian invention, it seems logical to tie this mode to that extension > being enabled. If the extension is disabled, proper hardware emulation > will be done instead. > > The main thing necessary here is the synchronization of the RTC > emulation code and the setting of the respective flag in hvmloader''s > creation of the ACPI WAET table. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/tools/firmware/hvmloader/acpi/acpi2_0.h > +++ b/tools/firmware/hvmloader/acpi/acpi2_0.h > @@ -304,6 +304,9 @@ struct acpi_20_waet { > 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 */ > + > /* > * Multiple APIC Flags. > */ > --- a/tools/firmware/hvmloader/acpi/build.c > +++ b/tools/firmware/hvmloader/acpi/build.c > @@ -22,8 +22,10 @@ > #include "ssdt_tpm.h" > #include "ssdt_pm.h" > #include "../config.h" > +#include "../hypercall.h" > #include "../util.h" > #include <xen/hvm/hvm_xs_strings.h> > +#include <xen/hvm/params.h> > > #define ACPI_MAX_SECONDARY_TABLES 16 > > @@ -189,6 +191,7 @@ static struct acpi_20_hpet *construct_hp > static struct acpi_20_waet *construct_waet(void) > { > struct acpi_20_waet *waet; > + xen_hvm_param_t param; > > waet = mem_alloc(sizeof(*waet), 16); > if (!waet) return NULL; > @@ -196,6 +199,19 @@ static struct acpi_20_waet *construct_wa > memcpy(waet, &Waet, sizeof(*waet)); > > waet->header.length = sizeof(*waet); > + > + /* > + * Check whether Viridian emulation is enabled: 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. > + */ > + param.domid = DOMID_SELF; > + param.index = HVM_PARAM_VIRIDIAN; > + if ( hypercall_hvm_op(HVMOP_get_param, ¶m) ) > + BUG(); > + if ( param.value ) > + waet->flags |= ACPI_WAET_RTC_NO_ACK; > + > set_checksum(waet, offsetof(struct acpi_header, checksum), sizeof(*waet)); > > return waet; > --- 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_TIMER_ONE_READ > }; > > /* > --- a/xen/arch/x86/hvm/rtc.c > +++ b/xen/arch/x86/hvm/rtc.c > @@ -51,7 +51,9 @@ enum rtc_mode { > }; > > /* 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 mode_is(d, m) (is_viridian_domain(d) ? \ > + rtc_mode_##m == rtc_mode_no_ack : \ > + rtc_mode_##m == rtc_mode_strict) > #define rtc_mode_is(s, m) mode_is(vrtc_domain(s), m) > > static void rtc_copy_date(RTCState *s);
Jan Beulich
2013-Jul-02 09:27 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
>>> On 02.07.13 at 11:11, Tim Deegan <tim@xen.org> wrote: > At 08:02 +0100 on 02 Jul (1372752161), Jan Beulich wrote: >> As the mode not conforming to the hardware specification (by allowing >> the guest to skip the REG C reads in its interrupt handler) is a >> Viridian invention, it seems logical to tie this mode to that extension >> being enabled. If the extension is disabled, proper hardware emulation >> will be done instead. >> >> The main thing necessary here is the synchronization of the RTC >> emulation code and the setting of the respective flag in hvmloader''s >> creation of the ACPI WAET table. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Wasn''t this going to have its own param, defaulting to off on create and > to on on migrate? I suspect most people just leave the viridian flag on > for all domains.In which case there would be no behavioral difference to what we''re going to release with 4.3. (That''s leaving aside the fact that I think people doing so is not the best practice.) In any case, while originally I indeed had considered having this have its own param, when putting the patch together I realized that there''s little point (but extra work) in doing so. Jan
Tim Deegan
2013-Jul-02 09:51 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
At 10:27 +0100 on 02 Jul (1372760862), Jan Beulich wrote:> >>> On 02.07.13 at 11:11, Tim Deegan <tim@xen.org> wrote: > > At 08:02 +0100 on 02 Jul (1372752161), Jan Beulich wrote: > >> As the mode not conforming to the hardware specification (by allowing > >> the guest to skip the REG C reads in its interrupt handler) is a > >> Viridian invention, it seems logical to tie this mode to that extension > >> being enabled. If the extension is disabled, proper hardware emulation > >> will be done instead. > >> > >> The main thing necessary here is the synchronization of the RTC > >> emulation code and the setting of the respective flag in hvmloader''s > >> creation of the ACPI WAET table. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Wasn''t this going to have its own param, defaulting to off on create and > > to on on migrate? I suspect most people just leave the viridian flag on > > for all domains. > > In which case there would be no behavioral difference to what > we''re going to release with 4.3. (That''s leaving aside the fact that > I think people doing so is not the best practice.)Why not? The Viridian interfaces is pretty well essential for running recent Windows, and shouldn''t be harmful for other OSes. Tim.> In any case, while originally I indeed had considered having this > have its own param, when putting the patch together I realized > that there''s little point (but extra work) in doing so.
Andrew Cooper
2013-Jul-02 10:10 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
On 02/07/13 08:02, Jan Beulich wrote:> As the mode not conforming to the hardware specification (by allowing > the guest to skip the REG C reads in its interrupt handler) is a > Viridian invention, it seems logical to tie this mode to that extension > being enabled. If the extension is disabled, proper hardware emulation > will be done instead. > > The main thing necessary here is the synchronization of the RTC > emulation code and the setting of the respective flag in hvmloader''s > creation of the ACPI WAET table. > > Signed-off-by: Jan Beulich <jbeulich@suse.com>This looks to be safe migrating forwards Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>> > --- a/tools/firmware/hvmloader/acpi/acpi2_0.h > +++ b/tools/firmware/hvmloader/acpi/acpi2_0.h > @@ -304,6 +304,9 @@ struct acpi_20_waet { > 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 */ > + > /* > * Multiple APIC Flags. > */ > --- a/tools/firmware/hvmloader/acpi/build.c > +++ b/tools/firmware/hvmloader/acpi/build.c > @@ -22,8 +22,10 @@ > #include "ssdt_tpm.h" > #include "ssdt_pm.h" > #include "../config.h" > +#include "../hypercall.h" > #include "../util.h" > #include <xen/hvm/hvm_xs_strings.h> > +#include <xen/hvm/params.h> > > #define ACPI_MAX_SECONDARY_TABLES 16 > > @@ -189,6 +191,7 @@ static struct acpi_20_hpet *construct_hp > static struct acpi_20_waet *construct_waet(void) > { > struct acpi_20_waet *waet; > + xen_hvm_param_t param; > > waet = mem_alloc(sizeof(*waet), 16); > if (!waet) return NULL; > @@ -196,6 +199,19 @@ static struct acpi_20_waet *construct_wa > memcpy(waet, &Waet, sizeof(*waet)); > > waet->header.length = sizeof(*waet); > + > + /* > + * Check whether Viridian emulation is enabled: 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. > + */ > + param.domid = DOMID_SELF; > + param.index = HVM_PARAM_VIRIDIAN; > + if ( hypercall_hvm_op(HVMOP_get_param, ¶m) ) > + BUG(); > + if ( param.value ) > + waet->flags |= ACPI_WAET_RTC_NO_ACK; > + > set_checksum(waet, offsetof(struct acpi_header, checksum), sizeof(*waet)); > > return waet; > --- 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_TIMER_ONE_READ > }; > > /* > --- a/xen/arch/x86/hvm/rtc.c > +++ b/xen/arch/x86/hvm/rtc.c > @@ -51,7 +51,9 @@ enum rtc_mode { > }; > > /* 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 mode_is(d, m) (is_viridian_domain(d) ? \ > + rtc_mode_##m == rtc_mode_no_ack : \ > + rtc_mode_##m == rtc_mode_strict) > #define rtc_mode_is(s, m) mode_is(vrtc_domain(s), m) > > static void rtc_copy_date(RTCState *s); > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Jul-02 10:22 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
>>> On 02.07.13 at 11:51, Tim Deegan <tim@xen.org> wrote: > At 10:27 +0100 on 02 Jul (1372760862), Jan Beulich wrote: >> >>> On 02.07.13 at 11:11, Tim Deegan <tim@xen.org> wrote: >> > At 08:02 +0100 on 02 Jul (1372752161), Jan Beulich wrote: >> >> As the mode not conforming to the hardware specification (by allowing >> >> the guest to skip the REG C reads in its interrupt handler) is a >> >> Viridian invention, it seems logical to tie this mode to that extension >> >> being enabled. If the extension is disabled, proper hardware emulation >> >> will be done instead. >> >> >> >> The main thing necessary here is the synchronization of the RTC >> >> emulation code and the setting of the respective flag in hvmloader''s >> >> creation of the ACPI WAET table. >> >> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> > >> > Wasn''t this going to have its own param, defaulting to off on create and >> > to on on migrate? I suspect most people just leave the viridian flag on >> > for all domains. >> >> In which case there would be no behavioral difference to what >> we''re going to release with 4.3. (That''s leaving aside the fact that >> I think people doing so is not the best practice.) > > Why not? The Viridian interfaces is pretty well essential for running > recent Windows, and shouldn''t be harmful for other OSes.Shouldn''t. But as we learned it occasionally is - Linux when built without CONFIG_XEN_PVHVM detects the HyperV functionality, and tried using HyperV functionality that Xen doesn''t really emulate (see commits 32068f65 ["x86: Hyper-V: register clocksource only if its advertised"] and db34bbb7 ["X86: Add a check to catch Xen emulation of Hyper-V"]). Jan
Tim Deegan
2013-Jul-02 10:35 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
At 11:22 +0100 on 02 Jul (1372764141), Jan Beulich wrote:> >>> On 02.07.13 at 11:51, Tim Deegan <tim@xen.org> wrote: > > At 10:27 +0100 on 02 Jul (1372760862), Jan Beulich wrote: > >> >>> On 02.07.13 at 11:11, Tim Deegan <tim@xen.org> wrote: > >> > At 08:02 +0100 on 02 Jul (1372752161), Jan Beulich wrote: > >> >> As the mode not conforming to the hardware specification (by allowing > >> >> the guest to skip the REG C reads in its interrupt handler) is a > >> >> Viridian invention, it seems logical to tie this mode to that extension > >> >> being enabled. If the extension is disabled, proper hardware emulation > >> >> will be done instead. > >> >> > >> >> The main thing necessary here is the synchronization of the RTC > >> >> emulation code and the setting of the respective flag in hvmloader''s > >> >> creation of the ACPI WAET table. > >> >> > >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> > > >> > Wasn''t this going to have its own param, defaulting to off on create and > >> > to on on migrate? I suspect most people just leave the viridian flag on > >> > for all domains. > >> > >> In which case there would be no behavioral difference to what > >> we''re going to release with 4.3. (That''s leaving aside the fact that > >> I think people doing so is not the best practice.) > > > > Why not? The Viridian interfaces is pretty well essential for running > > recent Windows, and shouldn''t be harmful for other OSes. > > Shouldn''t. But as we learned it occasionally is - Linux when built > without CONFIG_XEN_PVHVM detects the HyperV functionality, > and tried using HyperV functionality that Xen doesn''t really emulate > (see commits 32068f65 ["x86: Hyper-V: register clocksource only if > its advertised"] and db34bbb7 ["X86: Add a check to catch Xen > emulation of Hyper-V"]).So the argument is that host admins should already be disabling this for _all_ non-windows VMs to work around a bug in some linux kernels, and therefore it''s OK to hook this vaguely related feature onto it? That seems to be below the standard that you''d expect from other people. Anyway, surely we want this bit turned off by default even on Windows, to avoid running pointless timers if Windows decides not to use the RTC. Tim.
George Dunlap
2013-Jul-02 13:01 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
On Tue, Jul 2, 2013 at 11:35 AM, Tim Deegan <tim@xen.org> wrote:> At 11:22 +0100 on 02 Jul (1372764141), Jan Beulich wrote: >> >>> On 02.07.13 at 11:51, Tim Deegan <tim@xen.org> wrote: >> > At 10:27 +0100 on 02 Jul (1372760862), Jan Beulich wrote: >> >> >>> On 02.07.13 at 11:11, Tim Deegan <tim@xen.org> wrote: >> >> > At 08:02 +0100 on 02 Jul (1372752161), Jan Beulich wrote: >> >> >> As the mode not conforming to the hardware specification (by allowing >> >> >> the guest to skip the REG C reads in its interrupt handler) is a >> >> >> Viridian invention, it seems logical to tie this mode to that extension >> >> >> being enabled. If the extension is disabled, proper hardware emulation >> >> >> will be done instead. >> >> >> >> >> >> The main thing necessary here is the synchronization of the RTC >> >> >> emulation code and the setting of the respective flag in hvmloader''s >> >> >> creation of the ACPI WAET table. >> >> >> >> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> > >> >> > Wasn''t this going to have its own param, defaulting to off on create and >> >> > to on on migrate? I suspect most people just leave the viridian flag on >> >> > for all domains. >> >> >> >> In which case there would be no behavioral difference to what >> >> we''re going to release with 4.3. (That''s leaving aside the fact that >> >> I think people doing so is not the best practice.) >> > >> > Why not? The Viridian interfaces is pretty well essential for running >> > recent Windows, and shouldn''t be harmful for other OSes. >> >> Shouldn''t. But as we learned it occasionally is - Linux when built >> without CONFIG_XEN_PVHVM detects the HyperV functionality, >> and tried using HyperV functionality that Xen doesn''t really emulate >> (see commits 32068f65 ["x86: Hyper-V: register clocksource only if >> its advertised"] and db34bbb7 ["X86: Add a check to catch Xen >> emulation of Hyper-V"]). > > So the argument is that host admins should already be disabling this > for _all_ non-windows VMs to work around a bug in some linux kernels, > and therefore it''s OK to hook this vaguely related feature onto it? > That seems to be below the standard that you''d expect from other > people. > > Anyway, surely we want this bit turned off by default even on Windows, > to avoid running pointless timers if Windows decides not to use the RTC.FWIW I agree with this reasoning. Working around bugs in Linux is a losing game; and disabling Viridian features that aren''t a positive benefit to Windows seems like a good strategy. -George
Jan Beulich
2013-Jul-02 14:04 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
>>> On 02.07.13 at 15:01, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > On Tue, Jul 2, 2013 at 11:35 AM, Tim Deegan <tim@xen.org> wrote: >> At 11:22 +0100 on 02 Jul (1372764141), Jan Beulich wrote: >>> >>> On 02.07.13 at 11:51, Tim Deegan <tim@xen.org> wrote: >>> > At 10:27 +0100 on 02 Jul (1372760862), Jan Beulich wrote: >>> >> >>> On 02.07.13 at 11:11, Tim Deegan <tim@xen.org> wrote: >>> >> > At 08:02 +0100 on 02 Jul (1372752161), Jan Beulich wrote: >>> >> >> As the mode not conforming to the hardware specification (by allowing >>> >> >> the guest to skip the REG C reads in its interrupt handler) is a >>> >> >> Viridian invention, it seems logical to tie this mode to that extension >>> >> >> being enabled. If the extension is disabled, proper hardware emulation >>> >> >> will be done instead. >>> >> >> >>> >> >> The main thing necessary here is the synchronization of the RTC >>> >> >> emulation code and the setting of the respective flag in hvmloader''s >>> >> >> creation of the ACPI WAET table. >>> >> >> >>> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >> > >>> >> > Wasn''t this going to have its own param, defaulting to off on create and >>> >> > to on on migrate? I suspect most people just leave the viridian flag on >>> >> > for all domains. >>> >> >>> >> In which case there would be no behavioral difference to what >>> >> we''re going to release with 4.3. (That''s leaving aside the fact that >>> >> I think people doing so is not the best practice.) >>> > >>> > Why not? The Viridian interfaces is pretty well essential for running >>> > recent Windows, and shouldn''t be harmful for other OSes. >>> >>> Shouldn''t. But as we learned it occasionally is - Linux when built >>> without CONFIG_XEN_PVHVM detects the HyperV functionality, >>> and tried using HyperV functionality that Xen doesn''t really emulate >>> (see commits 32068f65 ["x86: Hyper-V: register clocksource only if >>> its advertised"] and db34bbb7 ["X86: Add a check to catch Xen >>> emulation of Hyper-V"]). >> >> So the argument is that host admins should already be disabling this >> for _all_ non-windows VMs to work around a bug in some linux kernels, >> and therefore it''s OK to hook this vaguely related feature onto it? >> That seems to be below the standard that you''d expect from other >> people. >> >> Anyway, surely we want this bit turned off by default even on Windows, >> to avoid running pointless timers if Windows decides not to use the RTC. > > FWIW I agree with this reasoning. Working around bugs in Linux is a > losing game; and disabling Viridian features that aren''t a positive > benefit to Windows seems like a good strategy.How do you know this is not "a positive benefit"? I very much think that to e.g. W2K3 it is - at the expense of the hypervisor having to keep timers alive that it could otherwise put to rest. Jan
Konrad Rzeszutek Wilk
2013-Jul-02 14:19 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
On Tue, Jul 02, 2013 at 11:22:21AM +0100, Jan Beulich wrote:> >>> On 02.07.13 at 11:51, Tim Deegan <tim@xen.org> wrote: > > At 10:27 +0100 on 02 Jul (1372760862), Jan Beulich wrote: > >> >>> On 02.07.13 at 11:11, Tim Deegan <tim@xen.org> wrote: > >> > At 08:02 +0100 on 02 Jul (1372752161), Jan Beulich wrote: > >> >> As the mode not conforming to the hardware specification (by allowing > >> >> the guest to skip the REG C reads in its interrupt handler) is a > >> >> Viridian invention, it seems logical to tie this mode to that extension > >> >> being enabled. If the extension is disabled, proper hardware emulation > >> >> will be done instead. > >> >> > >> >> The main thing necessary here is the synchronization of the RTC > >> >> emulation code and the setting of the respective flag in hvmloader''s > >> >> creation of the ACPI WAET table. > >> >> > >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> > > >> > Wasn''t this going to have its own param, defaulting to off on create and > >> > to on on migrate? I suspect most people just leave the viridian flag on > >> > for all domains. > >> > >> In which case there would be no behavioral difference to what > >> we''re going to release with 4.3. (That''s leaving aside the fact that > >> I think people doing so is not the best practice.) > > > > Why not? The Viridian interfaces is pretty well essential for running > > recent Windows, and shouldn''t be harmful for other OSes. > > Shouldn''t. But as we learned it occasionally is - Linux when built > without CONFIG_XEN_PVHVM detects the HyperV functionality, > and tried using HyperV functionality that Xen doesn''t really emulate > (see commits 32068f65 ["x86: Hyper-V: register clocksource only if > its advertised"] and db34bbb7 ["X86: Add a check to catch Xen > emulation of Hyper-V"]).Shouldn''t those patches be in stable tree by now?> > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Tim Deegan
2013-Jul-02 14:23 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
At 15:04 +0100 on 02 Jul (1372777451), Jan Beulich wrote:> >>> On 02.07.13 at 15:01, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > > On Tue, Jul 2, 2013 at 11:35 AM, Tim Deegan <tim@xen.org> wrote: > >> At 11:22 +0100 on 02 Jul (1372764141), Jan Beulich wrote: > >>> >>> On 02.07.13 at 11:51, Tim Deegan <tim@xen.org> wrote: > >>> > At 10:27 +0100 on 02 Jul (1372760862), Jan Beulich wrote: > >>> >> >>> On 02.07.13 at 11:11, Tim Deegan <tim@xen.org> wrote: > >>> >> > At 08:02 +0100 on 02 Jul (1372752161), Jan Beulich wrote: > >>> >> >> As the mode not conforming to the hardware specification (by allowing > >>> >> >> the guest to skip the REG C reads in its interrupt handler) is a > >>> >> >> Viridian invention, it seems logical to tie this mode to that extension > >>> >> >> being enabled. If the extension is disabled, proper hardware emulation > >>> >> >> will be done instead. > >>> >> >> > >>> >> >> The main thing necessary here is the synchronization of the RTC > >>> >> >> emulation code and the setting of the respective flag in hvmloader''s > >>> >> >> creation of the ACPI WAET table. > >>> >> >> > >>> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >>> >> > > >>> >> > Wasn''t this going to have its own param, defaulting to off on create and > >>> >> > to on on migrate? I suspect most people just leave the viridian flag on > >>> >> > for all domains. > >>> >> > >>> >> In which case there would be no behavioral difference to what > >>> >> we''re going to release with 4.3. (That''s leaving aside the fact that > >>> >> I think people doing so is not the best practice.) > >>> > > >>> > Why not? The Viridian interfaces is pretty well essential for running > >>> > recent Windows, and shouldn''t be harmful for other OSes. > >>> > >>> Shouldn''t. But as we learned it occasionally is - Linux when built > >>> without CONFIG_XEN_PVHVM detects the HyperV functionality, > >>> and tried using HyperV functionality that Xen doesn''t really emulate > >>> (see commits 32068f65 ["x86: Hyper-V: register clocksource only if > >>> its advertised"] and db34bbb7 ["X86: Add a check to catch Xen > >>> emulation of Hyper-V"]). > >> > >> So the argument is that host admins should already be disabling this > >> for _all_ non-windows VMs to work around a bug in some linux kernels, > >> and therefore it''s OK to hook this vaguely related feature onto it? > >> That seems to be below the standard that you''d expect from other > >> people. > >> > >> Anyway, surely we want this bit turned off by default even on Windows, > >> to avoid running pointless timers if Windows decides not to use the RTC. > > > > FWIW I agree with this reasoning. Working around bugs in Linux is a > > losing game; and disabling Viridian features that aren''t a positive > > benefit to Windows seems like a good strategy. > > How do you know this is not "a positive benefit"? I very much think > that to e.g. W2K3 it is - at the expense of the hypervisor having to > keep timers alive that it could otherwise put to rest.AFAIK, Windows 8 is tickless, so presumably doesn''t get any benefit from RTC EOI tricks (but does require Viridian). Tim.
Jan Beulich
2013-Jul-02 14:38 UTC
Re: [PATCH] x86/HVM: tie RTC emulation mode to enabling of Viridian emulation
>>> On 02.07.13 at 16:19, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Tue, Jul 02, 2013 at 11:22:21AM +0100, Jan Beulich wrote: >> >>> On 02.07.13 at 11:51, Tim Deegan <tim@xen.org> wrote: >> > At 10:27 +0100 on 02 Jul (1372760862), Jan Beulich wrote: >> >> >>> On 02.07.13 at 11:11, Tim Deegan <tim@xen.org> wrote: >> >> > At 08:02 +0100 on 02 Jul (1372752161), Jan Beulich wrote: >> >> >> As the mode not conforming to the hardware specification (by allowing >> >> >> the guest to skip the REG C reads in its interrupt handler) is a >> >> >> Viridian invention, it seems logical to tie this mode to that extension >> >> >> being enabled. If the extension is disabled, proper hardware emulation >> >> >> will be done instead. >> >> >> >> >> >> The main thing necessary here is the synchronization of the RTC >> >> >> emulation code and the setting of the respective flag in hvmloader''s >> >> >> creation of the ACPI WAET table. >> >> >> >> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> > >> >> > Wasn''t this going to have its own param, defaulting to off on create and >> >> > to on on migrate? I suspect most people just leave the viridian flag on >> >> > for all domains. >> >> >> >> In which case there would be no behavioral difference to what >> >> we''re going to release with 4.3. (That''s leaving aside the fact that >> >> I think people doing so is not the best practice.) >> > >> > Why not? The Viridian interfaces is pretty well essential for running >> > recent Windows, and shouldn''t be harmful for other OSes. >> >> Shouldn''t. But as we learned it occasionally is - Linux when built >> without CONFIG_XEN_PVHVM detects the HyperV functionality, >> and tried using HyperV functionality that Xen doesn''t really emulate >> (see commits 32068f65 ["x86: Hyper-V: register clocksource only if >> its advertised"] and db34bbb7 ["X86: Add a check to catch Xen >> emulation of Hyper-V"]). > > Shouldn''t those patches be in stable tree by now?Perhaps they are, but that''s not the point: I was trying to point out that blindly enabling Viridian emulation for Linux guests is not a uniformly harmless thing. Jan