[v2] tools/firmware: add ACPI device for Windows laptop/slate mode switch

Commit Message

Paul Durrant March 24, 2017, 10:10 a.m. UTC

Microsoft have defined an ACPI device to support switching Windows 10
between laptop/desktop mode and slate/tablet mode [1].

This patch adds an SSDT containing such a device. The presence of the
device is controlled by a new 'acpi_conv' boolean in xl.cfg. The new
device will not be present by default.

[1] https://msdn.microsoft.com/en-us/windows/hardware/commercialize/design/device-experiences/continuum

Signed-off-by: Owen Smith <[e-mail]>
Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Ian Jackson <[e-mail]>
Cc: Wei Liu <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>

NOTE: Odd looking braces style in tools/libacpi/build.c is for
      consistency with existing code.

v2:
 - Fix default if xenstore key is missing
---
 docs/man/xl.cfg.pod.5.in        |  5 +++++
 tools/firmware/hvmloader/util.c |  2 ++
 tools/libacpi/Makefile          |  4 ++--
 tools/libacpi/build.c           | 11 +++++++++++
 tools/libacpi/libacpi.h         |  1 +
 tools/libacpi/ssdt_conv.asl     | 38 ++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_create.c      |  3 +++
 tools/libxl/libxl_types.idl     |  1 +
 tools/xl/xl_parse.c             |  1 +
 9 files changed, 64 insertions(+), 2 deletions(-)
 create mode 100644 tools/libacpi/ssdt_conv.asl

Comments

Jan Beulich March 24, 2017, 10:52 a.m. UTC | #1

>>> On 24.03.17 at 11:10, <[e-mail]> wrote:
> @@ -406,6 +407,16 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt,
>          printf("S4 disabled\n");
>      }
>  
> +    if ( config->table_flags & ACPI_HAS_SSDT_CONV )
> +    {
> +        ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_conv), 16);
> +        if (!ssdt) return -1;

This being an optional thing anyway, would it perhaps be better to
allow guest boot to continue, just logging a message? Otoh, if we
have no memory here anymore, the guest ought to not get very
far anyway.

> +DefinitionBlock ("SSDT_CONV.aml", "SSDT", 2, "Xen", "HVM", 0)
> +{
> +    Device (CONV) {
> +        Method (_HID, 0x0, NotSerialized) {
> +            Return("ID9001")
> +        }
> +        Name (_CID, "PNP0C60")
> +    }
> +}

And that's it? This device doesn't do anything.

The only other concern I have here is that the abbreviation "conv"
used throughout the patch is sort of ambiguous. I think it means
"convertible" here, but without knowing the context it could easily
be "conventional" or "convenience". Would there be anything
wrong with spelling it out wherever name length limitations don't
require it to be just four characters?

Jan

Paul Durrant March 24, 2017, 11:04 a.m. UTC | #2

> -----Original Message-----
> From: Jan Beulich [mailto:[e-mail]]
> Sent: 24 March 2017 10:53
> To: Paul Durrant <[e-mail]>
> Cc: Andrew Cooper <[e-mail]>; Owen Smith
> <[e-mail]>; Wei Liu <[e-mail]>; Ian Jackson
> <[e-mail]>; [e-mail]
> Subject: [PATCH v2] tools/firmware: add ACPI device for Windows
> laptop/slate mode switch
> 
> >>> On 24.03.17 at 11:10, <[e-mail]> wrote:
> > @@ -406,6 +407,16 @@ static int construct_secondary_tables(struct
> acpi_ctxt *ctxt,
> >          printf("S4 disabled\n");
> >      }
> >
> > +    if ( config->table_flags & ACPI_HAS_SSDT_CONV )
> > +    {
> > +        ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_conv), 16);
> > +        if (!ssdt) return -1;
> 
> This being an optional thing anyway, would it perhaps be better to
> allow guest boot to continue, just logging a message? Otoh, if we
> have no memory here anymore, the guest ought to not get very
> far anyway.
> 

Yes, I think if this fails the guest is probably going to die anyway.

> > +DefinitionBlock ("SSDT_CONV.aml", "SSDT", 2, "Xen", "HVM", 0)
> > +{
> > +    Device (CONV) {
> > +        Method (_HID, 0x0, NotSerialized) {
> > +            Return("ID9001")
> > +        }
> > +        Name (_CID, "PNP0C60")
> > +    }
> > +}
> 
> And that's it? This device doesn't do anything.
> 

Yes, that's it! It is there merely so an in-box Windows driver can bind to it.

> The only other concern I have here is that the abbreviation "conv"
> used throughout the patch is sort of ambiguous. I think it means
> "convertible" here, but without knowing the context it could easily
> be "conventional" or "convenience". Would there be anything
> wrong with spelling it out wherever name length limitations don't
> require it to be just four characters?
> 

I thought that name would be ok since it is the name of the ACPI device itself but I could extend the xl.cfg option, bool and flag names to 'acpi_convert' to make it more obvious.

  Paul

> Jan

Jan Beulich March 24, 2017, 11:16 a.m. UTC | #3

>>> On 24.03.17 at 12:04, <[e-mail]> wrote:
>> From: Jan Beulich [mailto:[e-mail]]
>> Sent: 24 March 2017 10:53
>> The only other concern I have here is that the abbreviation "conv"
>> used throughout the patch is sort of ambiguous. I think it means
>> "convertible" here, but without knowing the context it could easily
>> be "conventional" or "convenience". Would there be anything
>> wrong with spelling it out wherever name length limitations don't
>> require it to be just four characters?
>> 
> 
> I thought that name would be ok since it is the name of the ACPI device 
> itself but I could extend the xl.cfg option, bool and flag names to 
> 'acpi_convert' to make it more obvious.

Hmm, "convert" still leaves room for speculation (in particular, seeing
an option with this name, I'd suspect it wants to convert ACPI to
something else). Is that what "conv" stands for (and not "convertible"
or anything else)?

Jan

Paul Durrant March 24, 2017, 11:19 a.m. UTC | #4

> -----Original Message-----
> From: Jan Beulich [mailto:[e-mail]]
> Sent: 24 March 2017 11:16
> To: Paul Durrant <[e-mail]>
> Cc: Andrew Cooper <[e-mail]>; Ian Jackson
> <[e-mail]>; Owen Smith <[e-mail]>; Wei Liu
> <[e-mail]>; [e-mail]
> Subject: RE: [PATCH v2] tools/firmware: add ACPI device for Windows
> laptop/slate mode switch
> 
> >>> On 24.03.17 at 12:04, <[e-mail]> wrote:
> >> From: Jan Beulich [mailto:[e-mail]]
> >> Sent: 24 March 2017 10:53
> >> The only other concern I have here is that the abbreviation "conv"
> >> used throughout the patch is sort of ambiguous. I think it means
> >> "convertible" here, but without knowing the context it could easily
> >> be "conventional" or "convenience". Would there be anything
> >> wrong with spelling it out wherever name length limitations don't
> >> require it to be just four characters?
> >>
> >
> > I thought that name would be ok since it is the name of the ACPI device
> > itself but I could extend the xl.cfg option, bool and flag names to
> > 'acpi_convert' to make it more obvious.
> 
> Hmm, "convert" still leaves room for speculation (in particular, seeing
> an option with this name, I'd suspect it wants to convert ACPI to
> something else). Is that what "conv" stands for (and not "convertible"
> or anything else)?
> 

It was chosen simply because of the name of the device. How about 'acpi_device_conv'? After all this option merely controls the inclusion of the device. Subsequent patches will have to add a PV protocol to instruct in-guest code to prod it.

  Paul

> Jan

Jan Beulich March 24, 2017, 11:32 a.m. UTC | #5

>>> On 24.03.17 at 12:19, <[e-mail]> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:[e-mail]]
>> Sent: 24 March 2017 11:16
>> To: Paul Durrant <[e-mail]>
>> Cc: Andrew Cooper <[e-mail]>; Ian Jackson
>> <[e-mail]>; Owen Smith <[e-mail]>; Wei Liu
>> <[e-mail]>; [e-mail] 
>> Subject: RE: [PATCH v2] tools/firmware: add ACPI device for Windows
>> laptop/slate mode switch
>> 
>> >>> On 24.03.17 at 12:04, <[e-mail]> wrote:
>> >> From: Jan Beulich [mailto:[e-mail]]
>> >> Sent: 24 March 2017 10:53
>> >> The only other concern I have here is that the abbreviation "conv"
>> >> used throughout the patch is sort of ambiguous. I think it means
>> >> "convertible" here, but without knowing the context it could easily
>> >> be "conventional" or "convenience". Would there be anything
>> >> wrong with spelling it out wherever name length limitations don't
>> >> require it to be just four characters?
>> >>
>> >
>> > I thought that name would be ok since it is the name of the ACPI device
>> > itself but I could extend the xl.cfg option, bool and flag names to
>> > 'acpi_convert' to make it more obvious.
>> 
>> Hmm, "convert" still leaves room for speculation (in particular, seeing
>> an option with this name, I'd suspect it wants to convert ACPI to
>> something else). Is that what "conv" stands for (and not "convertible"
>> or anything else)?
>> 
> 
> It was chosen simply because of the name of the device. How about 
> 'acpi_device_conv'? After all this option merely controls the inclusion of 
> the device. Subsequent patches will have to add a PV protocol to instruct 
> in-guest code to prod it.

That name is not much better. Looking at MS doc, why don't we call
it by the name that they use there: acpi_laptop_slate?

Jan

Paul Durrant March 24, 2017, 11:40 a.m. UTC | #6

> -----Original Message-----
> From: Jan Beulich [mailto:[e-mail]]
> Sent: 24 March 2017 11:33
> To: Paul Durrant <[e-mail]>
> Cc: Andrew Cooper <[e-mail]>; Ian Jackson
> <[e-mail]>; Owen Smith <[e-mail]>; Wei Liu
> <[e-mail]>; [e-mail]
> Subject: RE: [PATCH v2] tools/firmware: add ACPI device for Windows
> laptop/slate mode switch
> 
> >>> On 24.03.17 at 12:19, <[e-mail]> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:[e-mail]]
> >> Sent: 24 March 2017 11:16
> >> To: Paul Durrant <[e-mail]>
> >> Cc: Andrew Cooper <[e-mail]>; Ian Jackson
> >> <[e-mail]>; Owen Smith <[e-mail]>; Wei
> Liu
> >> <[e-mail]>; [e-mail]
> >> Subject: RE: [PATCH v2] tools/firmware: add ACPI device for Windows
> >> laptop/slate mode switch
> >>
> >> >>> On 24.03.17 at 12:04, <[e-mail]> wrote:
> >> >> From: Jan Beulich [mailto:[e-mail]]
> >> >> Sent: 24 March 2017 10:53
> >> >> The only other concern I have here is that the abbreviation "conv"
> >> >> used throughout the patch is sort of ambiguous. I think it means
> >> >> "convertible" here, but without knowing the context it could easily
> >> >> be "conventional" or "convenience". Would there be anything
> >> >> wrong with spelling it out wherever name length limitations don't
> >> >> require it to be just four characters?
> >> >>
> >> >
> >> > I thought that name would be ok since it is the name of the ACPI device
> >> > itself but I could extend the xl.cfg option, bool and flag names to
> >> > 'acpi_convert' to make it more obvious.
> >>
> >> Hmm, "convert" still leaves room for speculation (in particular, seeing
> >> an option with this name, I'd suspect it wants to convert ACPI to
> >> something else). Is that what "conv" stands for (and not "convertible"
> >> or anything else)?
> >>
> >
> > It was chosen simply because of the name of the device. How about
> > 'acpi_device_conv'? After all this option merely controls the inclusion of
> > the device. Subsequent patches will have to add a PV protocol to instruct
> > in-guest code to prod it.
> 
> That name is not much better. Looking at MS doc, why don't we call
> it by the name that they use there: acpi_laptop_slate?

Ok, sounds fine. 'acpi_laptop_slate' it is.

  Paul

> 
> Jan

Patch

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 52802d5..6f57aa9 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -1256,6 +1256,11 @@  ACPI table. True (1) by default.
 Include S4 (suspend-to-disk) power state in the virtual firmware ACPI
 table. True (1) by default.
 
+=item B<acpi_conv=BOOLEAN>
+
+Include the Windows laptop/slate mode switch device in the virtual
+firmware ACPI table. False (0) by default.
+
 =item B<apic=BOOLEAN>
 
 Include information regarding APIC (Advanced Programmable Interrupt
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 03cfb79..d20b000 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -947,6 +947,8 @@  void hvmloader_acpi_build_tables(struct acpi_config *config,
         config->table_flags |= ACPI_HAS_SSDT_S3;
     if ( !strncmp(xenstore_read("platform/acpi_s4", "1"), "1", 1)  )
         config->table_flags |= ACPI_HAS_SSDT_S4;
+    if ( !strncmp(xenstore_read("platform/acpi_conv", "0"), "1", 1)  )
+        config->table_flags |= ACPI_HAS_SSDT_CONV;
 
     config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC |
                             ACPI_HAS_WAET | ACPI_HAS_PMTIMER |
diff --git a/tools/libacpi/Makefile b/tools/libacpi/Makefile
index 6d8445d..3b61304 100644
--- a/tools/libacpi/Makefile
+++ b/tools/libacpi/Makefile
@@ -25,7 +25,7 @@  C_SRC-$(CONFIG_X86) = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c dsdt_pvh
 C_SRC-$(CONFIG_ARM_64) = dsdt_anycpu_arm.c
 DSDT_FILES ?= $(C_SRC-y)
 C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, $(DSDT_FILES))
-H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h)
+H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h ssdt_conv.h)
 
 MKDSDT_CFLAGS-$(CONFIG_ARM_64) = -DCONFIG_ARM_64
 MKDSDT_CFLAGS-$(CONFIG_X86) = -DCONFIG_X86
@@ -89,7 +89,7 @@  iasl:
 	@echo 
 	@exit 1
 
-build.o: ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h
+build.o: ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h ssdt_conv.h
 
 acpi.a: $(OBJS)
 	$(AR) rc $@ $(OBJS)
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index a02ffbf..698f39a 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -20,6 +20,7 @@ 
 #include "ssdt_s4.h"
 #include "ssdt_tpm.h"
 #include "ssdt_pm.h"
+#include "ssdt_conv.h"
 #include <xen/hvm/hvm_info_table.h>
 #include <xen/hvm/hvm_xs_strings.h>
 #include <xen/hvm/params.h>
@@ -406,6 +407,16 @@  static int construct_secondary_tables(struct acpi_ctxt *ctxt,
         printf("S4 disabled\n");
     }
 
+    if ( config->table_flags & ACPI_HAS_SSDT_CONV )
+    {
+        ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_conv), 16);
+        if (!ssdt) return -1;
+        memcpy(ssdt, ssdt_conv, sizeof(ssdt_conv));
+        table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt);
+    } else {
+        printf("CONV disabled\n");
+    }
+
     /* TPM TCPA and SSDT. */
     if ( (config->table_flags & ACPI_HAS_TCPA) &&
          (config->tis_hdr[0] == tis_signature[0]) &&
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index 67bd67f..e5b91aa 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -35,6 +35,7 @@ 
 #define ACPI_HAS_VGA         (1<<12)
 #define ACPI_HAS_8042        (1<<13)
 #define ACPI_HAS_CMOS_RTC    (1<<14)
+#define ACPI_HAS_SSDT_CONV   (1<<15)
 
 struct xen_vmemrange;
 struct acpi_numa {
diff --git a/tools/libacpi/ssdt_conv.asl b/tools/libacpi/ssdt_conv.asl
new file mode 100644
index 0000000..d2eb744
--- /dev/null
+++ b/tools/libacpi/ssdt_conv.asl
@@ -0,0 +1,38 @@ 
+/*
+ * ssdt_conv.asl
+ *
+ * Copyright (c) 2017  Citrix Systems, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+/*
+ * Windows laptop/slate mode device
+ *
+ * See https://msdn.microsoft.com/en-us/windows/hardware/commercialize/design/device-experiences/continuum#method-2----use-the-injection-interface
+ */
+
+DefinitionBlock ("SSDT_CONV.aml", "SSDT", 2, "Xen", "HVM", 0)
+{
+    Device (CONV) {
+        Method (_HID, 0x0, NotSerialized) {
+            Return("ID9001")
+        }
+        Name (_CID, "PNP0C60")
+    }
+}
+
+/*
+ * Local variables:
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 25389e1..cb41b08 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -313,6 +313,7 @@  int libxl__domain_build_info_setdefault(libxl__gc *gc,
         libxl_defbool_setdefault(&b_info->u.hvm.acpi,               true);
         libxl_defbool_setdefault(&b_info->u.hvm.acpi_s3,            true);
         libxl_defbool_setdefault(&b_info->u.hvm.acpi_s4,            true);
+        libxl_defbool_setdefault(&b_info->u.hvm.acpi_conv,          false);
         libxl_defbool_setdefault(&b_info->u.hvm.nx,                 true);
         libxl_defbool_setdefault(&b_info->u.hvm.viridian,           false);
         libxl_defbool_setdefault(&b_info->u.hvm.hpet,               true);
@@ -458,6 +459,8 @@  int libxl__domain_build(libxl__gc *gc,
         localents[i++] = libxl_defbool_val(info->u.hvm.acpi_s3) ? "1" : "0";
         localents[i++] = "platform/acpi_s4";
         localents[i++] = libxl_defbool_val(info->u.hvm.acpi_s4) ? "1" : "0";
+        localents[i++] = "platform/acpi_conv";
+        localents[i++] = libxl_defbool_val(info->u.hvm.acpi_conv) ? "1" : "0";
         if (info->u.hvm.mmio_hole_memkb) {
             uint64_t max_ram_below_4g =
                 (1ULL << 32) - (info->u.hvm.mmio_hole_memkb << 10);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 2475a4d..7e9f781 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -507,6 +507,7 @@  libxl_domain_build_info = Struct("domain_build_info",[
                                        ("acpi",             libxl_defbool),
                                        ("acpi_s3",          libxl_defbool),
                                        ("acpi_s4",          libxl_defbool),
+                                       ("acpi_conv",        libxl_defbool),
                                        ("nx",               libxl_defbool),
                                        ("viridian",         libxl_defbool),
                                        ("viridian_enable",  libxl_bitmap),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index b72f990..b8eb546 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1065,6 +1065,7 @@  void parse_config_data(const char *config_source,
         xlu_cfg_get_defbool(config, "apic", &b_info->u.hvm.apic, 0);
         xlu_cfg_get_defbool(config, "acpi_s3", &b_info->u.hvm.acpi_s3, 0);
         xlu_cfg_get_defbool(config, "acpi_s4", &b_info->u.hvm.acpi_s4, 0);
+        xlu_cfg_get_defbool(config, "acpi_conv", &b_info->u.hvm.acpi_conv, 0);
         xlu_cfg_get_defbool(config, "nx", &b_info->u.hvm.nx, 0);
         xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
         xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 0);