[PATCH RFC 0/2] Fix RTC noack issues

[PATCH RFC 0/2] Fix RTC noack issues

[Andrew Cooper]

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.


-- 
1.7.10.4

[PATCH 1/2] x86/vRTC: Make rtc_mode_{strict, no_ack} a per-domain option

[Andrew Cooper]

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          |   11 ++++++-----
 xen/include/public/hvm/params.h |    5 ++++-
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index af249f7..e56ada5 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..4010d49 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -45,14 +45,15 @@
 #define epoch_year     1900
 #define get_year(x)    (x + epoch_year)
 
+/* This forms an ABI in HVM_PARAM_RTC_MODE */
 enum rtc_mode {
-   rtc_mode_no_ack,
-   rtc_mode_strict
+   rtc_mode_no_ack = 0,
+   rtc_mode_strict = 1
 };
 
-/* 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] \
+                      == rtc_mode_##m)
 
 static void rtc_copy_date(RTCState *s);
 static void rtc_set_time(RTCState *s);
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 517a184..2849dc3 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -145,6 +145,9 @@
 /* SHUTDOWN_* action in case of a triple fault */
 #define HVM_PARAM_TRIPLE_FAULT_REASON 31
 
-#define HVM_NR_PARAMS          32
+/* Set to 1 if domain is expected to use RTC no-ack optimisation */
+#define HVM_PARAM_RTC_MODE 32
+
+#define HVM_NR_PARAMS          33
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
-- 
1.7.10.4

[PATCH 2/2] hvmloader: Allow the toolstack to choose WAET optimisations

[Andrew Cooper]

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         |   32 +++++++++++++++++++++++++
 tools/firmware/hvmloader/acpi/static_tables.c |   12 +---------
 3 files changed, 37 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..b9e209a 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,35 @@ 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(s, "true", 4) )
+            waet->flags |= ACPI_WAET_RTC_NO_ACK;
+        else
+            waet->flags &= ~ACPI_WAET_RTC_NO_ACK;
+    }
+
+    s = xenstore_read("platform/waet-pm-reliable", NULL);
+    if ( s )
+    {
+        if ( !strncmp(s, "1", 1) || !strncmp(s, "true", 4) )
+            waet->flags |= ACPI_WAET_TIMER_ONE_READ;
+        else
+            waet->flags &= ~ACPI_WAET_TIMER_ONE_READ;
+    }
+
+    printf("WAET flags: RTC noack %d, PM reliable %d\n",
+           !!(waet->flags & ACPI_WAET_RTC_NO_ACK),
+           !!(waet->flags & ACPI_WAET_TIMER_ONE_READ));
+
     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

Re: [PATCH 1/2] x86/vRTC: Make rtc_mode_{strict, no_ack} a per-domain option

[Jan Beulich]

>>> On 26.11.13 at 21:39, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> +/* This forms an ABI in HVM_PARAM_RTC_MODE */

Irrespective of this ...

>  enum rtc_mode {
> -   rtc_mode_no_ack,
> -   rtc_mode_strict
> +   rtc_mode_no_ack = 0,
> +   rtc_mode_strict = 1

... this is a pointless change - the C specification requires them to
be 0 and 1 respectively. There would be a point to this only if you
added #define-s to the public interface (and used them here).

Jan

Re: [PATCH 2/2] hvmloader: Allow the toolstack to choose WAET optimisations

[Jan Beulich]

>>> On 26.11.13 at 21:39, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> @@ -196,8 +201,35 @@ 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(s, "true", 4) )
> +            waet->flags |= ACPI_WAET_RTC_NO_ACK;
> +        else
> +            waet->flags &= ~ACPI_WAET_RTC_NO_ACK;
> +    }
> +
> +    s = xenstore_read("platform/waet-pm-reliable", NULL);
> +    if ( s )
> +    {
> +        if ( !strncmp(s, "1", 1) || !strncmp(s, "true", 4) )
> +            waet->flags |= ACPI_WAET_TIMER_ONE_READ;
> +        else
> +            waet->flags &= ~ACPI_WAET_TIMER_ONE_READ;
> +    }

Nothing in this series really allows these to be easily controlled on a
per-domain basis - I would have expected a config file flag...

And for this second one, as hinted at in your overview mail, I
don't really see what purpose the controlling here serves: You
don't consume the setting, i.e. the sole effect is that of
controlling the ACPI table's flag value. Yet if we're fine with
the guest doing single reads (of whatever), then we're surely
fine too with it doing double reads. And hence the flag can
easily be always set (as it was till now).

If any second override was to be considered, I'd recommend
on controlling the presence of the WAET as a whole.

Jan

Re: [PATCH 2/2] hvmloader: Allow the toolstack to choose WAET optimisations

[Andrew Cooper]

On 27/11/13 10:03, Jan Beulich wrote:
>>>> On 26.11.13 at 21:39, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> @@ -196,8 +201,35 @@ 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(s, "true", 4) )
>> +            waet->flags |= ACPI_WAET_RTC_NO_ACK;
>> +        else
>> +            waet->flags &= ~ACPI_WAET_RTC_NO_ACK;
>> +    }
>> +
>> +    s = xenstore_read("platform/waet-pm-reliable", NULL);
>> +    if ( s )
>> +    {
>> +        if ( !strncmp(s, "1", 1) || !strncmp(s, "true", 4) )
>> +            waet->flags |= ACPI_WAET_TIMER_ONE_READ;
>> +        else
>> +            waet->flags &= ~ACPI_WAET_TIMER_ONE_READ;
>> +    }
> Nothing in this series really allows these to be easily controlled on a
> per-domain basis - I would have expected a config file flag...
>
> And for this second one, as hinted at in your overview mail, I
> don't really see what purpose the controlling here serves: You
> don't consume the setting, i.e. the sole effect is that of
> controlling the ACPI table's flag value. Yet if we're fine with
> the guest doing single reads (of whatever), then we're surely
> fine too with it doing double reads. And hence the flag can
> easily be always set (as it was till now).
>
> If any second override was to be considered, I'd recommend
> on controlling the presence of the WAET as a whole.
>
> Jan
>

The setting is consumed using

+    /* Inform Xen which RTC mode has been chosen */
+    p.value = !!(waet->flags & ACPI_WAET_RTC_NO_ACK);
+    hypercall_hvm_op(HVMOP_set_param, &p);

Which changes the behaviour of rtc_mode_is(s, mode) in Xen.

The presence (or lack thereof) of the WAET table does not address the problem at hand, which is that Xen's current logic of trying to guess what the guest is doing WRT RTC is wrong. It causes the guest to fall into an infinite loop expecting standards compliment behaviour, where Xen has decided that the guest has moved to the optimised behaviour.

~Andrew

Re: [PATCH 2/2] hvmloader: Allow the toolstack to choose WAET optimisations

[Jan Beulich]

>>> On 27.11.13 at 13:14, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 27/11/13 10:03, Jan Beulich wrote:
>>>>> On 26.11.13 at 21:39, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> @@ -196,8 +201,35 @@ 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(s, "true", 4) )
>>> +            waet->flags |= ACPI_WAET_RTC_NO_ACK;
>>> +        else
>>> +            waet->flags &= ~ACPI_WAET_RTC_NO_ACK;
>>> +    }
>>> +
>>> +    s = xenstore_read("platform/waet-pm-reliable", NULL);
>>> +    if ( s )
>>> +    {
>>> +        if ( !strncmp(s, "1", 1) || !strncmp(s, "true", 4) )
>>> +            waet->flags |= ACPI_WAET_TIMER_ONE_READ;
>>> +        else
>>> +            waet->flags &= ~ACPI_WAET_TIMER_ONE_READ;
>>> +    }
>> Nothing in this series really allows these to be easily controlled on a
>> per-domain basis - I would have expected a config file flag...
>>
>> And for this second one, as hinted at in your overview mail, I
>> don't really see what purpose the controlling here serves: You
>> don't consume the setting, i.e. the sole effect is that of
>> controlling the ACPI table's flag value. Yet if we're fine with
>> the guest doing single reads (of whatever), then we're surely
>> fine too with it doing double reads. And hence the flag can
>> easily be always set (as it was till now).
>>
>> If any second override was to be considered, I'd recommend
>> on controlling the presence of the WAET as a whole.
> 
> The setting is consumed using
> 
> +    /* Inform Xen which RTC mode has been chosen */
> +    p.value = !!(waet->flags & ACPI_WAET_RTC_NO_ACK);
> +    hypercall_hvm_op(HVMOP_set_param, &p);

Note that I said "this second one", i.e. referring to
ACPI_WAET_TIMER_ONE_READ.

Jan

Re: [PATCH 2/2] hvmloader: Allow the toolstack to choose WAET optimisations

[Andrew Cooper]

On 27/11/13 12:56, Jan Beulich wrote:
>>>> On 27.11.13 at 13:14, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 27/11/13 10:03, Jan Beulich wrote:
>>>>>> On 26.11.13 at 21:39, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> @@ -196,8 +201,35 @@ 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(s, "true", 4) )
>>>> +            waet->flags |= ACPI_WAET_RTC_NO_ACK;
>>>> +        else
>>>> +            waet->flags &= ~ACPI_WAET_RTC_NO_ACK;
>>>> +    }
>>>> +
>>>> +    s = xenstore_read("platform/waet-pm-reliable", NULL);
>>>> +    if ( s )
>>>> +    {
>>>> +        if ( !strncmp(s, "1", 1) || !strncmp(s, "true", 4) )
>>>> +            waet->flags |= ACPI_WAET_TIMER_ONE_READ;
>>>> +        else
>>>> +            waet->flags &= ~ACPI_WAET_TIMER_ONE_READ;
>>>> +    }
>>> Nothing in this series really allows these to be easily controlled on a
>>> per-domain basis - I would have expected a config file flag...
>>>
>>> And for this second one, as hinted at in your overview mail, I
>>> don't really see what purpose the controlling here serves: You
>>> don't consume the setting, i.e. the sole effect is that of
>>> controlling the ACPI table's flag value. Yet if we're fine with
>>> the guest doing single reads (of whatever), then we're surely
>>> fine too with it doing double reads. And hence the flag can
>>> easily be always set (as it was till now).
>>>
>>> If any second override was to be considered, I'd recommend
>>> on controlling the presence of the WAET as a whole.
>> The setting is consumed using
>>
>> +    /* Inform Xen which RTC mode has been chosen */
>> +    p.value = !!(waet->flags & ACPI_WAET_RTC_NO_ACK);
>> +    hypercall_hvm_op(HVMOP_set_param, &p);
> Note that I said "this second one", i.e. referring to
> ACPI_WAET_TIMER_ONE_READ.
>
> Jan
>

Ah apologies - I missed that.  I did bring that up in the covering
letter as to whether it was worth having an option for it in the first
place.

~Andrew

Re: [PATCH 1/2] x86/vRTC: Make rtc_mode_{strict, no_ack} a per-domain option

[Tim Deegan]

At 09:55 +0000 on 27 Nov (1385542548), Jan Beulich wrote:
> >>> On 26.11.13 at 21:39, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > +/* This forms an ABI in HVM_PARAM_RTC_MODE */
> 
> Irrespective of this ...
> 
> >  enum rtc_mode {
> > -   rtc_mode_no_ack,
> > -   rtc_mode_strict
> > +   rtc_mode_no_ack = 0,
> > +   rtc_mode_strict = 1
> 
> ... this is a pointless change - the C specification requires them to
> be 0 and 1 respectively. There would be a point to this only if you
> added #define-s to the public interface (and used them here).

I think that's a good idea -- the code assumes that the toolstack's
choice of 0 and 1 match this enum, so it should be made explicit here
(so later engineers don't reshuffle the enum for some reason).

Tim.

Re: [PATCH 2/2] hvmloader: Allow the toolstack to choose WAET optimisations

[Ian Campbell]

On Tue, 2013-11-26 at 20:39 +0000, Andrew Cooper wrote:

> +    s = xenstore_read("platform/waet-rtc-noack", NULL);
[...]
> +    s = xenstore_read("platform/waet-pm-reliable", NULL);

What writes these? Is the default in their absence sane?

Ian.

Re: [PATCH 2/2] hvmloader: Allow the toolstack to choose WAET optimisations

[Ian Campbell]

On Tue, 2013-11-26 at 20:39 +0000, Andrew Cooper 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>
> ---
>  tools/firmware/hvmloader/acpi/acpi2_0.h       |    4 ++++
>  tools/firmware/hvmloader/acpi/build.c         |   32 +++++++++++++++++++++++++
>  tools/firmware/hvmloader/acpi/static_tables.c |   12 +---------
>  3 files changed, 37 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..b9e209a 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,35 @@ 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(s, "true", 4) )

Oh, and we generally seem to only care in hvmloader about "1" and "0"
rather than true/false wibble/woggle etc. Since this is an internal
protocol I think we don't need to be terribly accepting.

You need to update docs/misc/xenstore-paths.markdown.

Ian.

Re: [PATCH 2/2] hvmloader: Allow the toolstack to choose WAET optimisations

[Andrew Cooper]

On 29/11/13 09:55, Ian Campbell wrote:
> On Tue, 2013-11-26 at 20:39 +0000, Andrew Cooper 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>
>> ---
>>  tools/firmware/hvmloader/acpi/acpi2_0.h       |    4 ++++
>>  tools/firmware/hvmloader/acpi/build.c         |   32 +++++++++++++++++++++++++
>>  tools/firmware/hvmloader/acpi/static_tables.c |   12 +---------
>>  3 files changed, 37 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..b9e209a 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,35 @@ 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(s, "true", 4) )
> Oh, and we generally seem to only care in hvmloader about "1" and "0"
> rather than true/false wibble/woggle etc. Since this is an internal
> protocol I think we don't need to be terribly accepting.
>
> You need to update docs/misc/xenstore-paths.markdown.
>
> Ian.
>

Xapi works with "true/false" in these values.  In the past, all platform
flags were not dealt with by hvmloader directly - they were all integers
in HVMPARAMS or the start-info table.

>From a "debugging issues with reference to xenstore" point of view, it
is substantially easier to have booleans as true/false, to visually
differentiate them from genuine integer 0/1's

In my copious free time, I was going to formally introduce
"xenstore_read_bool()" in hvmloader.

~Andrew

Re: [PATCH 2/2] hvmloader: Allow the toolstack to choose WAET optimisations

[Ian Campbell]

On Fri, 2013-11-29 at 10:57 +0000, Andrew Cooper wrote:
> On 29/11/13 09:55, Ian Campbell wrote:
> > On Tue, 2013-11-26 at 20:39 +0000, Andrew Cooper 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>
> >> ---
> >>  tools/firmware/hvmloader/acpi/acpi2_0.h       |    4 ++++
> >>  tools/firmware/hvmloader/acpi/build.c         |   32 +++++++++++++++++++++++++
> >>  tools/firmware/hvmloader/acpi/static_tables.c |   12 +---------
> >>  3 files changed, 37 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..b9e209a 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,35 @@ 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(s, "true", 4) )
> > Oh, and we generally seem to only care in hvmloader about "1" and "0"
> > rather than true/false wibble/woggle etc. Since this is an internal
> > protocol I think we don't need to be terribly accepting.
> >
> > You need to update docs/misc/xenstore-paths.markdown.
> >
> > Ian.
> >
> 
> Xapi works with "true/false" in these values.

It sounds like xapi needs fixing then.

>   In the past, all platform
> flags were not dealt with by hvmloader directly - they were all integers
> in HVMPARAMS or the start-info table.
> 
> From a "debugging issues with reference to xenstore" point of view, it
> is substantially easier to have booleans as true/false, to visually
> differentiate them from genuine integer 0/1's

Regardless, the xenstore convention is 0 and 1, mixing styles is worse
than either option.

Ian.

> In my copious free time, I was going to formally introduce
> "xenstore_read_bool()" in hvmloader.
> 
> ~Andrew