[Qemu-devel] [PATCH] Add DSDT node for AppleSMC

[Qemu-devel] [PATCH] Add DSDT node for AppleSMC

[Gabriel L. Somlo]

AppleSMC (-device isa-applesmc) is required to boot OS X guests.
OS X expects a SMC node to be present in the ACPI DSDT. This patch
adds a SMC node to the DSDT, and dynamically patches the return value
of SMC._STA to either 0x0B if the chip is present, or otherwise to 0x00,
before booting the guest.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/misc/applesmc.c        |  1 -
 include/hw/isa/isa.h      |  2 ++
 hw/i386/acpi-dsdt.dsl     |  1 +
 hw/i386/q35-acpi-dsdt.dsl |  1 +
 hw/i386/acpi-dsdt-isa.dsl | 14 ++++++++++++++
 hw/i386/acpi-build.c      | 10 ++++++++++
 6 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 1e8d183..627adb9 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -66,7 +66,6 @@ struct AppleSMCData {
     QLIST_ENTRY(AppleSMCData) node;
 };
 
-#define TYPE_APPLE_SMC "isa-applesmc"
 #define APPLE_SMC(obj) OBJECT_CHECK(AppleSMCState, (obj), TYPE_APPLE_SMC)
 
 typedef struct AppleSMCState AppleSMCState;
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index fa45a5b..5596cdc 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -20,6 +20,8 @@
 #define TYPE_ISA_BUS "ISA"
 #define ISA_BUS(obj) OBJECT_CHECK(ISABus, (obj), TYPE_ISA_BUS)
 
+#define TYPE_APPLE_SMC "isa-applesmc"
+
 typedef struct ISADeviceClass {
     DeviceClass parent_class;
 } ISADeviceClass;
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index a377424..7f8f147 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -114,6 +114,7 @@ DefinitionBlock (
         }
     }
 
+#define SMC_PFX piix_
 #include "acpi-dsdt-isa.dsl"
 
 
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 575c5d7..7609e78 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -171,6 +171,7 @@ DefinitionBlock (
         }
     }
 
+#define SMC_PFX q35_
 #include "acpi-dsdt-isa.dsl"
 
 
diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
index 89caa16..410e658 100644
--- a/hw/i386/acpi-dsdt-isa.dsl
+++ b/hw/i386/acpi-dsdt-isa.dsl
@@ -13,9 +13,23 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#define _CONCAT_SYM(a, b) a##b
+#define CONCAT_SYM(a, b) _CONCAT_SYM(a, b)
+
 /* Common legacy ISA style devices. */
 Scope(\_SB.PCI0.ISA) {
 
+    Device (SMC) {
+        Name(_HID, EisaId("APP0001"))
+        /* _STA will be patched to 0x0B if AppleSMC is present */
+        ACPI_EXTRACT_NAME_WORD_CONST CONCAT_SYM(SMC_PFX, smc_sta)
+        Name(_STA, 0xFF00)
+        Name(_CRS, ResourceTemplate () {
+            IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
+            IRQNoFlags() { 6 }
+        })
+    }
+
     Device(RTC) {
         Name(_HID, EisaId("PNP0B00"))
         Name(_CRS, ResourceTemplate() {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 48312f5..529b655 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -36,6 +36,7 @@
 #include "hw/nvram/fw_cfg.h"
 #include "bios-linker-loader.h"
 #include "hw/loader.h"
+#include "hw/isa/isa.h"
 
 /* Supported chipsets: */
 #include "hw/acpi/piix4.h"
@@ -80,6 +81,8 @@ typedef struct AcpiMiscInfo {
 
 static void acpi_get_dsdt(AcpiMiscInfo *info)
 {
+    unsigned short smc_sta_val = 0x00;
+    unsigned short *smc_sta_off;
     Object *piix = piix4_pm_find();
     Object *lpc = ich9_lpc_find();
     assert(!!piix != !!lpc);
@@ -87,11 +90,18 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
     if (piix) {
         info->dsdt_code = AcpiDsdtAmlCode;
         info->dsdt_size = sizeof AcpiDsdtAmlCode;
+        smc_sta_off = piix_smc_sta;
     }
     if (lpc) {
         info->dsdt_code = Q35AcpiDsdtAmlCode;
         info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
+        smc_sta_off = q35_smc_sta;
     }
+
+    /* Patch in appropriate value for AppleSMC _STA */
+    if (object_resolve_path_type("", TYPE_APPLE_SMC, NULL))
+        smc_sta_val = 0x0b; /* present, enabled, and functional */
+    *(uint16_t *)(info->dsdt_code + *smc_sta_off) = cpu_to_le16(smc_sta_val);
 }
 
 static
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] Add DSDT node for AppleSMC

[Alexander Graf]

On 20.12.2013, at 16:54, Gabriel L. Somlo <gsomlo@gmail.com> wrote:

> AppleSMC (-device isa-applesmc) is required to boot OS X guests.
> OS X expects a SMC node to be present in the ACPI DSDT. This patch
> adds a SMC node to the DSDT, and dynamically patches the return value
> of SMC._STA to either 0x0B if the chip is present, or otherwise to 0x00,
> before booting the guest.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>

I haven't checked for the actual functionality, but please make sure the patch passes checkpatch.pl. I spot at least one case of missing braces - and it'd be a shame if it gets rejected over that.


Alex

[Qemu-devel] [PATCH v2] Add DSDT node for AppleSMC

[Gabriel L. Somlo]

AppleSMC (-device isa-applesmc) is required to boot OS X guests.
OS X expects a SMC node to be present in the ACPI DSDT. This patch
adds a SMC node to the DSDT, and dynamically patches the return value
of SMC._STA to either 0x0B if the chip is present, or otherwise to 0x00,
before booting the guest.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

On Fri, Dec 20, 2013 at 05:39:53PM +0100, Alexander Graf wrote:
> I haven't checked for the actual functionality, but please make sure
> the patch passes checkpatch.pl. I spot at least one case of missing
> braces - and it'd be a shame if it gets rejected over that.

OK, here's v2 complete with braces:

$ qemu/scripts/checkpatch.pl qemu-applesmc-20131220-v2.patch 
total: 0 errors, 0 warnings, 86 lines checked

qemu-applesmc-20131220-v2.patch has no obvious style problems and is ready for submission.

Thanks,
  Gabriel

 hw/misc/applesmc.c        |  1 -
 include/hw/isa/isa.h      |  2 ++
 hw/i386/acpi-dsdt.dsl     |  1 +
 hw/i386/q35-acpi-dsdt.dsl |  1 +
 hw/i386/acpi-dsdt-isa.dsl | 14 ++++++++++++++
 hw/i386/acpi-build.c      | 11 +++++++++++
 6 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 1e8d183..627adb9 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -66,7 +66,6 @@ struct AppleSMCData {
     QLIST_ENTRY(AppleSMCData) node;
 };
 
-#define TYPE_APPLE_SMC "isa-applesmc"
 #define APPLE_SMC(obj) OBJECT_CHECK(AppleSMCState, (obj), TYPE_APPLE_SMC)
 
 typedef struct AppleSMCState AppleSMCState;
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index fa45a5b..5596cdc 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -20,6 +20,8 @@
 #define TYPE_ISA_BUS "ISA"
 #define ISA_BUS(obj) OBJECT_CHECK(ISABus, (obj), TYPE_ISA_BUS)
 
+#define TYPE_APPLE_SMC "isa-applesmc"
+
 typedef struct ISADeviceClass {
     DeviceClass parent_class;
 } ISADeviceClass;
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index a377424..7f8f147 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -114,6 +114,7 @@ DefinitionBlock (
         }
     }
 
+#define SMC_PFX piix_
 #include "acpi-dsdt-isa.dsl"
 
 
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 575c5d7..7609e78 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -171,6 +171,7 @@ DefinitionBlock (
         }
     }
 
+#define SMC_PFX q35_
 #include "acpi-dsdt-isa.dsl"
 
 
diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
index 89caa16..410e658 100644
--- a/hw/i386/acpi-dsdt-isa.dsl
+++ b/hw/i386/acpi-dsdt-isa.dsl
@@ -13,9 +13,23 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#define _CONCAT_SYM(a, b) a##b
+#define CONCAT_SYM(a, b) _CONCAT_SYM(a, b)
+
 /* Common legacy ISA style devices. */
 Scope(\_SB.PCI0.ISA) {
 
+    Device (SMC) {
+        Name(_HID, EisaId("APP0001"))
+        /* _STA will be patched to 0x0B if AppleSMC is present */
+        ACPI_EXTRACT_NAME_WORD_CONST CONCAT_SYM(SMC_PFX, smc_sta)
+        Name(_STA, 0xFF00)
+        Name(_CRS, ResourceTemplate () {
+            IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
+            IRQNoFlags() { 6 }
+        })
+    }
+
     Device(RTC) {
         Name(_HID, EisaId("PNP0B00"))
         Name(_CRS, ResourceTemplate() {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 48312f5..d0cb574 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -36,6 +36,7 @@
 #include "hw/nvram/fw_cfg.h"
 #include "bios-linker-loader.h"
 #include "hw/loader.h"
+#include "hw/isa/isa.h"
 
 /* Supported chipsets: */
 #include "hw/acpi/piix4.h"
@@ -80,6 +81,8 @@ typedef struct AcpiMiscInfo {
 
 static void acpi_get_dsdt(AcpiMiscInfo *info)
 {
+    unsigned short smc_sta_val = 0x00;
+    unsigned short *smc_sta_off;
     Object *piix = piix4_pm_find();
     Object *lpc = ich9_lpc_find();
     assert(!!piix != !!lpc);
@@ -87,11 +90,19 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
     if (piix) {
         info->dsdt_code = AcpiDsdtAmlCode;
         info->dsdt_size = sizeof AcpiDsdtAmlCode;
+        smc_sta_off = piix_smc_sta;
     }
     if (lpc) {
         info->dsdt_code = Q35AcpiDsdtAmlCode;
         info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
+        smc_sta_off = q35_smc_sta;
     }
+
+    /* Patch in appropriate value for AppleSMC _STA */
+    if (object_resolve_path_type("", TYPE_APPLE_SMC, NULL)) {
+        smc_sta_val = 0x0b; /* present, enabled, and functional */
+    }
+    *(uint16_t *)(info->dsdt_code + *smc_sta_off) = cpu_to_le16(smc_sta_val);
 }
 
 static
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2] Add DSDT node for AppleSMC

[Igor Mammedov]

On Fri, 20 Dec 2013 15:52:24 -0500
"Gabriel L. Somlo" <gsomlo@gmail.com> wrote:

> AppleSMC (-device isa-applesmc) is required to boot OS X guests.
> OS X expects a SMC node to be present in the ACPI DSDT. This patch
> adds a SMC node to the DSDT, and dynamically patches the return value
> of SMC._STA to either 0x0B if the chip is present, or otherwise to 0x00,
> before booting the guest.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> On Fri, Dec 20, 2013 at 05:39:53PM +0100, Alexander Graf wrote:
> > I haven't checked for the actual functionality, but please make sure
> > the patch passes checkpatch.pl. I spot at least one case of missing
> > braces - and it'd be a shame if it gets rejected over that.
> 
> OK, here's v2 complete with braces:
> 
> $ qemu/scripts/checkpatch.pl qemu-applesmc-20131220-v2.patch 
> total: 0 errors, 0 warnings, 86 lines checked
> 
> qemu-applesmc-20131220-v2.patch has no obvious style problems and is ready for submission.
> 
> Thanks,
>   Gabriel
> 
>  hw/misc/applesmc.c        |  1 -
>  include/hw/isa/isa.h      |  2 ++
>  hw/i386/acpi-dsdt.dsl     |  1 +
>  hw/i386/q35-acpi-dsdt.dsl |  1 +
>  hw/i386/acpi-dsdt-isa.dsl | 14 ++++++++++++++
>  hw/i386/acpi-build.c      | 11 +++++++++++
>  6 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 1e8d183..627adb9 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -66,7 +66,6 @@ struct AppleSMCData {
>      QLIST_ENTRY(AppleSMCData) node;
>  };
>  
> -#define TYPE_APPLE_SMC "isa-applesmc"
>  #define APPLE_SMC(obj) OBJECT_CHECK(AppleSMCState, (obj), TYPE_APPLE_SMC)
>  
>  typedef struct AppleSMCState AppleSMCState;
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index fa45a5b..5596cdc 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -20,6 +20,8 @@
>  #define TYPE_ISA_BUS "ISA"
>  #define ISA_BUS(obj) OBJECT_CHECK(ISABus, (obj), TYPE_ISA_BUS)
>  
> +#define TYPE_APPLE_SMC "isa-applesmc"
> +
>  typedef struct ISADeviceClass {
>      DeviceClass parent_class;
>  } ISADeviceClass;
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index a377424..7f8f147 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -114,6 +114,7 @@ DefinitionBlock (
>          }
>      }
>  
> +#define SMC_PFX piix_
>  #include "acpi-dsdt-isa.dsl"
>  
>  
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index 575c5d7..7609e78 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -171,6 +171,7 @@ DefinitionBlock (
>          }
>      }
>  
> +#define SMC_PFX q35_
>  #include "acpi-dsdt-isa.dsl"
>  
>  
> diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
> index 89caa16..410e658 100644
> --- a/hw/i386/acpi-dsdt-isa.dsl
> +++ b/hw/i386/acpi-dsdt-isa.dsl
> @@ -13,9 +13,23 @@
>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#define _CONCAT_SYM(a, b) a##b
> +#define CONCAT_SYM(a, b) _CONCAT_SYM(a, b)
> +
>  /* Common legacy ISA style devices. */
>  Scope(\_SB.PCI0.ISA) {
>  
> +    Device (SMC) {
> +        Name(_HID, EisaId("APP0001"))
> +        /* _STA will be patched to 0x0B if AppleSMC is present */
> +        ACPI_EXTRACT_NAME_WORD_CONST CONCAT_SYM(SMC_PFX, smc_sta)
typically dynamic variables are put in SSDT

> +        Name(_STA, 0xFF00)
> +        Name(_CRS, ResourceTemplate () {
> +            IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
> +            IRQNoFlags() { 6 }
> +        })
> +    }
Device looks pretty simplistic, Have you considered building it completely
dynamically?
See build_ssdt() for examples, you might need to add extra building block for
ResourceTemplate{} the rest could be done with existing build_* primitives.


>      Device(RTC) {
>          Name(_HID, EisaId("PNP0B00"))
>          Name(_CRS, ResourceTemplate() {
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 48312f5..d0cb574 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -36,6 +36,7 @@
>  #include "hw/nvram/fw_cfg.h"
>  #include "bios-linker-loader.h"
>  #include "hw/loader.h"
> +#include "hw/isa/isa.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -80,6 +81,8 @@ typedef struct AcpiMiscInfo {
>  
>  static void acpi_get_dsdt(AcpiMiscInfo *info)
>  {
> +    unsigned short smc_sta_val = 0x00;
> +    unsigned short *smc_sta_off;
>      Object *piix = piix4_pm_find();
>      Object *lpc = ich9_lpc_find();
>      assert(!!piix != !!lpc);
> @@ -87,11 +90,19 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
>      if (piix) {
>          info->dsdt_code = AcpiDsdtAmlCode;
>          info->dsdt_size = sizeof AcpiDsdtAmlCode;
> +        smc_sta_off = piix_smc_sta;
>      }
>      if (lpc) {
>          info->dsdt_code = Q35AcpiDsdtAmlCode;
>          info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
> +        smc_sta_off = q35_smc_sta;
>      }
> +
> +    /* Patch in appropriate value for AppleSMC _STA */
> +    if (object_resolve_path_type("", TYPE_APPLE_SMC, NULL)) {
> +        smc_sta_val = 0x0b; /* present, enabled, and functional */
> +    }
> +    *(uint16_t *)(info->dsdt_code + *smc_sta_off) = cpu_to_le16(smc_sta_val);
>  }
>  
>  static
> -- 
> 1.8.1.4
> 
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH v2] Add DSDT node for AppleSMC

[Michael S. Tsirkin]

On Fri, Dec 20, 2013 at 03:52:24PM -0500, Gabriel L. Somlo wrote:
> AppleSMC (-device isa-applesmc) is required to boot OS X guests.
> OS X expects a SMC node to be present in the ACPI DSDT. This patch
> adds a SMC node to the DSDT, and dynamically patches the return value
> of SMC._STA to either 0x0B if the chip is present, or otherwise to 0x00,
> before booting the guest.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> On Fri, Dec 20, 2013 at 05:39:53PM +0100, Alexander Graf wrote:
> > I haven't checked for the actual functionality, but please make sure
> > the patch passes checkpatch.pl. I spot at least one case of missing
> > braces - and it'd be a shame if it gets rejected over that.
> 
> OK, here's v2 complete with braces:
> 
> $ qemu/scripts/checkpatch.pl qemu-applesmc-20131220-v2.patch 
> total: 0 errors, 0 warnings, 86 lines checked
> 
> qemu-applesmc-20131220-v2.patch has no obvious style problems and is ready for submission.
> 
> Thanks,
>   Gabriel
> 
>  hw/misc/applesmc.c        |  1 -
>  include/hw/isa/isa.h      |  2 ++
>  hw/i386/acpi-dsdt.dsl     |  1 +
>  hw/i386/q35-acpi-dsdt.dsl |  1 +
>  hw/i386/acpi-dsdt-isa.dsl | 14 ++++++++++++++
>  hw/i386/acpi-build.c      | 11 +++++++++++
>  6 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 1e8d183..627adb9 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -66,7 +66,6 @@ struct AppleSMCData {
>      QLIST_ENTRY(AppleSMCData) node;
>  };
>  
> -#define TYPE_APPLE_SMC "isa-applesmc"
>  #define APPLE_SMC(obj) OBJECT_CHECK(AppleSMCState, (obj), TYPE_APPLE_SMC)
>  
>  typedef struct AppleSMCState AppleSMCState;
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index fa45a5b..5596cdc 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -20,6 +20,8 @@
>  #define TYPE_ISA_BUS "ISA"
>  #define ISA_BUS(obj) OBJECT_CHECK(ISABus, (obj), TYPE_ISA_BUS)
>  
> +#define TYPE_APPLE_SMC "isa-applesmc"
> +
>  typedef struct ISADeviceClass {
>      DeviceClass parent_class;
>  } ISADeviceClass;
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index a377424..7f8f147 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -114,6 +114,7 @@ DefinitionBlock (
>          }
>      }
>  
> +#define SMC_PFX piix_
>  #include "acpi-dsdt-isa.dsl"
>  
>  
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index 575c5d7..7609e78 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -171,6 +171,7 @@ DefinitionBlock (
>          }
>      }
>  
> +#define SMC_PFX q35_
>  #include "acpi-dsdt-isa.dsl"
>  
>  
> diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
> index 89caa16..410e658 100644
> --- a/hw/i386/acpi-dsdt-isa.dsl
> +++ b/hw/i386/acpi-dsdt-isa.dsl
> @@ -13,9 +13,23 @@
>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#define _CONCAT_SYM(a, b) a##b
> +#define CONCAT_SYM(a, b) _CONCAT_SYM(a, b)
> +

Seems too complex. If one looks for q35_smc_sta one can not
find it since no indexing tool or grep have a chance to
understand this trickery.
Also, let's call it apple_smc not just smc everywhere.

Also, please add prefix like dsdt so it's easy to figure
out where this comes from.

So something like

#define DSDT_APPLE_SMC_STA q35_dsdt_apple_smc_sta
and
#define DSDT_APPLE_SMC_STA piix_dsdt_apple_smc_sta

and then use these everywhere.


>  /* Common legacy ISA style devices. */
>  Scope(\_SB.PCI0.ISA) {
>  
> +    Device (SMC) {
> +        Name(_HID, EisaId("APP0001"))
> +        /* _STA will be patched to 0x0B if AppleSMC is present */
> +        ACPI_EXTRACT_NAME_WORD_CONST CONCAT_SYM(SMC_PFX, smc_sta)
> +        Name(_STA, 0xFF00)
> +        Name(_CRS, ResourceTemplate () {
> +            IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
> +            IRQNoFlags() { 6 }
> +        })
> +    }
> +
>      Device(RTC) {
>          Name(_HID, EisaId("PNP0B00"))
>          Name(_CRS, ResourceTemplate() {
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 48312f5..d0cb574 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -36,6 +36,7 @@
>  #include "hw/nvram/fw_cfg.h"
>  #include "bios-linker-loader.h"
>  #include "hw/loader.h"
> +#include "hw/isa/isa.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -80,6 +81,8 @@ typedef struct AcpiMiscInfo {
>  
>  static void acpi_get_dsdt(AcpiMiscInfo *info)
>  {
> +    unsigned short smc_sta_val = 0x00;

Better initialize this in the else branch below.

> +    unsigned short *smc_sta_off;
>      Object *piix = piix4_pm_find();
>      Object *lpc = ich9_lpc_find();
>      assert(!!piix != !!lpc);
> @@ -87,11 +90,19 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
>      if (piix) {
>          info->dsdt_code = AcpiDsdtAmlCode;
>          info->dsdt_size = sizeof AcpiDsdtAmlCode;
> +        smc_sta_off = piix_smc_sta;
>      }
>      if (lpc) {
>          info->dsdt_code = Q35AcpiDsdtAmlCode;
>          info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
> +        smc_sta_off = q35_smc_sta;
>      }
> +
> +    /* Patch in appropriate value for AppleSMC _STA */
> +    if (object_resolve_path_type("", TYPE_APPLE_SMC, NULL)) {
> +        smc_sta_val = 0x0b; /* present, enabled, and functional */
> +    }
> +    *(uint16_t *)(info->dsdt_code + *smc_sta_off) = cpu_to_le16(smc_sta_val);
>  }
>  
>  static
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH v2] Add DSDT node for AppleSMC

[Gabriel L. Somlo]

On Fri, Dec 20, 2013 at 10:38:30PM +0100, Igor Mammedov wrote:
> > +    Device (SMC) {
> > +        Name(_HID, EisaId("APP0001"))
> > +        /* _STA will be patched to 0x0B if AppleSMC is present */
> > +        ACPI_EXTRACT_NAME_WORD_CONST CONCAT_SYM(SMC_PFX, smc_sta)
> typically dynamic variables are put in SSDT

When I tried statically building the SMC node as part of the SSDT, it
was not recognized by Mac OS X -- it only seems to work when it's part
of the DSDT.

> > +        Name(_STA, 0xFF00)
> > +        Name(_CRS, ResourceTemplate () {
> > +            IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
> > +            IRQNoFlags() { 6 }
> > +        })
> > +    }
> Device looks pretty simplistic, Have you considered building it completely
> dynamically?
> See build_ssdt() for examples, you might need to add extra building block for
> ResourceTemplate{} the rest could be done with existing build_* primitives.

If we consider "pasting" the SMC into the DSDT or not, based on
whether it was configured via -device applesmc or not, it comes
down to compiling the following ASL:

    Device (SMC) {
        Name(_HID, EisaId("APP0001"))
        Name(_STA, 0x0B)
        Name(_CRS, ResourceTemplate () {
            IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
            IRQNoFlags() { 6 }
        })
    }

into AML, and somehow conditionally appending the resulting blob,
unmodified, to the DSDT.

I would explore this approach before I'd consider compiling the blob
dynamically each time during QEMU startup, which would IMHO be harder
to follow.

However, I thought flipping one uint16_t from 0x0B to 0x00 (i.e., the
return value of SMC._STA) is much less intrusive to the DSDT than
pasting an entire blob dynamically during startup, which is why I
thought my approach was a bit cleaner.

Let me know what you think.

Thanks,
--Gabriel

[Qemu-devel] [PATCH v3] Add DSDT node for AppleSMC

[Gabriel L. Somlo]

AppleSMC (-device isa-applesmc) is required to boot OS X guests.
OS X expects a SMC node to be present in the ACPI DSDT. This patch
adds a SMC node to the DSDT, and dynamically patches the return value
of SMC._STA to either 0x0B if the chip is present, or otherwise to 0x00,
before booting the guest.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

On Sun, Dec 22, 2013 at 01:07:13PM +0200, Michael S. Tsirkin wrote:
>> +#define _CONCAT_SYM(a, b) a##b
>> +#define CONCAT_SYM(a, b) _CONCAT_SYM(a, b)
>
> Seems too complex. If one looks for q35_smc_sta one can not find it

You're right, I went all "Rube Goldberg" on that one - thanks for
pulling me back from the edge :)

>> +    unsigned short smc_sta_val = 0x00;
> 
> Better initialize this in the else branch below.

I threw in "applesmc_find()" to make that even cleaner and easier to
follow.

Thanks,
  Gabriel

 hw/misc/applesmc.c        |  1 -
 include/hw/isa/isa.h      |  7 +++++++
 hw/i386/acpi-dsdt.dsl     |  1 +
 hw/i386/q35-acpi-dsdt.dsl |  1 +
 hw/i386/acpi-dsdt-isa.dsl | 11 +++++++++++
 hw/i386/acpi-build.c      |  9 +++++++++
 6 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 1e8d183..627adb9 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -66,7 +66,6 @@ struct AppleSMCData {
     QLIST_ENTRY(AppleSMCData) node;
 };
 
-#define TYPE_APPLE_SMC "isa-applesmc"
 #define APPLE_SMC(obj) OBJECT_CHECK(AppleSMCState, (obj), TYPE_APPLE_SMC)
 
 typedef struct AppleSMCState AppleSMCState;
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index fa45a5b..e0c749f 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -20,6 +20,13 @@
 #define TYPE_ISA_BUS "ISA"
 #define ISA_BUS(obj) OBJECT_CHECK(ISABus, (obj), TYPE_ISA_BUS)
 
+#define TYPE_APPLE_SMC "isa-applesmc"
+
+static inline bool applesmc_find(void)
+{
+    return object_resolve_path_type("", TYPE_APPLE_SMC, NULL);
+}
+
 typedef struct ISADeviceClass {
     DeviceClass parent_class;
 } ISADeviceClass;
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index a377424..b87c6e0 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -114,6 +114,7 @@ DefinitionBlock (
         }
     }
 
+#define DSDT_APPLESMC_STA piix_dsdt_applesmc_sta
 #include "acpi-dsdt-isa.dsl"
 
 
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 575c5d7..12ff544 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -171,6 +171,7 @@ DefinitionBlock (
         }
     }
 
+#define DSDT_APPLESMC_STA q35_dsdt_applesmc_sta
 #include "acpi-dsdt-isa.dsl"
 
 
diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
index 89caa16..46942c1 100644
--- a/hw/i386/acpi-dsdt-isa.dsl
+++ b/hw/i386/acpi-dsdt-isa.dsl
@@ -16,6 +16,17 @@
 /* Common legacy ISA style devices. */
 Scope(\_SB.PCI0.ISA) {
 
+    Device (SMC) {
+        Name(_HID, EisaId("APP0001"))
+        /* _STA will be patched to 0x0B if AppleSMC is present */
+        ACPI_EXTRACT_NAME_WORD_CONST DSDT_APPLESMC_STA
+        Name(_STA, 0xFF00)
+        Name(_CRS, ResourceTemplate () {
+            IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
+            IRQNoFlags() { 6 }
+        })
+    }
+
     Device(RTC) {
         Name(_HID, EisaId("PNP0B00"))
         Name(_CRS, ResourceTemplate() {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 48312f5..30bfcd2 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -36,6 +36,7 @@
 #include "hw/nvram/fw_cfg.h"
 #include "bios-linker-loader.h"
 #include "hw/loader.h"
+#include "hw/isa/isa.h"
 
 /* Supported chipsets: */
 #include "hw/acpi/piix4.h"
@@ -80,6 +81,7 @@ typedef struct AcpiMiscInfo {
 
 static void acpi_get_dsdt(AcpiMiscInfo *info)
 {
+    unsigned short applesmc_sta_val, *applesmc_sta_off;
     Object *piix = piix4_pm_find();
     Object *lpc = ich9_lpc_find();
     assert(!!piix != !!lpc);
@@ -87,11 +89,18 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
     if (piix) {
         info->dsdt_code = AcpiDsdtAmlCode;
         info->dsdt_size = sizeof AcpiDsdtAmlCode;
+        applesmc_sta_off = piix_dsdt_applesmc_sta;
     }
     if (lpc) {
         info->dsdt_code = Q35AcpiDsdtAmlCode;
         info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
+        applesmc_sta_off = q35_dsdt_applesmc_sta;
     }
+
+    /* Patch in appropriate value for AppleSMC _STA */
+    applesmc_sta_val = applesmc_find() ? 0x0b : 0x00;
+    *(uint16_t *)(info->dsdt_code + *applesmc_sta_off) =
+        cpu_to_le16(applesmc_sta_val);
 }
 
 static
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v3] Add DSDT node for AppleSMC

[Laszlo Ersek]

On 12/22/13 16:34, Gabriel L. Somlo wrote:
> AppleSMC (-device isa-applesmc) is required to boot OS X guests.
> OS X expects a SMC node to be present in the ACPI DSDT. This patch
> adds a SMC node to the DSDT, and dynamically patches the return value
> of SMC._STA to either 0x0B if the chip is present, or otherwise to 0x00,
> before booting the guest.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> On Sun, Dec 22, 2013 at 01:07:13PM +0200, Michael S. Tsirkin wrote:
>>> +#define _CONCAT_SYM(a, b) a##b
>>> +#define CONCAT_SYM(a, b) _CONCAT_SYM(a, b)
>>
>> Seems too complex. If one looks for q35_smc_sta one can not find it
> 
> You're right, I went all "Rube Goldberg" on that one - thanks for
> pulling me back from the edge :)
> 
>>> +    unsigned short smc_sta_val = 0x00;
>>
>> Better initialize this in the else branch below.
> 
> I threw in "applesmc_find()" to make that even cleaner and easier to
> follow.
> 
> Thanks,
>   Gabriel
> 
>  hw/misc/applesmc.c        |  1 -
>  include/hw/isa/isa.h      |  7 +++++++
>  hw/i386/acpi-dsdt.dsl     |  1 +
>  hw/i386/q35-acpi-dsdt.dsl |  1 +
>  hw/i386/acpi-dsdt-isa.dsl | 11 +++++++++++
>  hw/i386/acpi-build.c      |  9 +++++++++
>  6 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 1e8d183..627adb9 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -66,7 +66,6 @@ struct AppleSMCData {
>      QLIST_ENTRY(AppleSMCData) node;
>  };
>  
> -#define TYPE_APPLE_SMC "isa-applesmc"
>  #define APPLE_SMC(obj) OBJECT_CHECK(AppleSMCState, (obj), TYPE_APPLE_SMC)
>  
>  typedef struct AppleSMCState AppleSMCState;
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index fa45a5b..e0c749f 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -20,6 +20,13 @@
>  #define TYPE_ISA_BUS "ISA"
>  #define ISA_BUS(obj) OBJECT_CHECK(ISABus, (obj), TYPE_ISA_BUS)
>  
> +#define TYPE_APPLE_SMC "isa-applesmc"
> +
> +static inline bool applesmc_find(void)
> +{
> +    return object_resolve_path_type("", TYPE_APPLE_SMC, NULL);
> +}
> +
>  typedef struct ISADeviceClass {
>      DeviceClass parent_class;
>  } ISADeviceClass;
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index a377424..b87c6e0 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -114,6 +114,7 @@ DefinitionBlock (
>          }
>      }
>  
> +#define DSDT_APPLESMC_STA piix_dsdt_applesmc_sta
>  #include "acpi-dsdt-isa.dsl"
>  
>  
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index 575c5d7..12ff544 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -171,6 +171,7 @@ DefinitionBlock (
>          }
>      }
>  
> +#define DSDT_APPLESMC_STA q35_dsdt_applesmc_sta
>  #include "acpi-dsdt-isa.dsl"
>  
>  
> diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
> index 89caa16..46942c1 100644
> --- a/hw/i386/acpi-dsdt-isa.dsl
> +++ b/hw/i386/acpi-dsdt-isa.dsl
> @@ -16,6 +16,17 @@
>  /* Common legacy ISA style devices. */
>  Scope(\_SB.PCI0.ISA) {
>  
> +    Device (SMC) {
> +        Name(_HID, EisaId("APP0001"))
> +        /* _STA will be patched to 0x0B if AppleSMC is present */
> +        ACPI_EXTRACT_NAME_WORD_CONST DSDT_APPLESMC_STA
> +        Name(_STA, 0xFF00)
> +        Name(_CRS, ResourceTemplate () {
> +            IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
> +            IRQNoFlags() { 6 }
> +        })
> +    }
> +
>      Device(RTC) {
>          Name(_HID, EisaId("PNP0B00"))
>          Name(_CRS, ResourceTemplate() {
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 48312f5..30bfcd2 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -36,6 +36,7 @@
>  #include "hw/nvram/fw_cfg.h"
>  #include "bios-linker-loader.h"
>  #include "hw/loader.h"
> +#include "hw/isa/isa.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -80,6 +81,7 @@ typedef struct AcpiMiscInfo {
>  
>  static void acpi_get_dsdt(AcpiMiscInfo *info)
>  {
> +    unsigned short applesmc_sta_val, *applesmc_sta_off;
>      Object *piix = piix4_pm_find();
>      Object *lpc = ich9_lpc_find();
>      assert(!!piix != !!lpc);
> @@ -87,11 +89,18 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
>      if (piix) {
>          info->dsdt_code = AcpiDsdtAmlCode;
>          info->dsdt_size = sizeof AcpiDsdtAmlCode;
> +        applesmc_sta_off = piix_dsdt_applesmc_sta;
>      }
>      if (lpc) {
>          info->dsdt_code = Q35AcpiDsdtAmlCode;
>          info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
> +        applesmc_sta_off = q35_dsdt_applesmc_sta;
>      }
> +
> +    /* Patch in appropriate value for AppleSMC _STA */
> +    applesmc_sta_val = applesmc_find() ? 0x0b : 0x00;
> +    *(uint16_t *)(info->dsdt_code + *applesmc_sta_off) =
> +        cpu_to_le16(applesmc_sta_val);
>  }
>  
>  static
> 

My comments below don't constitute a review by any means, this is just a
note for your consideration.

After this patch, ISA interrupt 6 is used by both "SMC" and "FDC0". The
latter depends on the FDEN object, but FDEN is currently constant 1.

Probably not a problem in practice (ie. most users won't try to specify
both a floppy disk controller and an AppleSMC device), but you might
want to handle that case nonetheless (exit with an error or some such).

I repeat, it's completely up to you (I might even be wrong).

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v3] Add DSDT node for AppleSMC

[Gabriel L. Somlo]

On Sun, Dec 22, 2013 at 04:58:58PM +0100, Laszlo Ersek wrote:
> After this patch, ISA interrupt 6 is used by both "SMC" and "FDC0". The
> latter depends on the FDEN object, but FDEN is currently constant 1.
> 
> Probably not a problem in practice (ie. most users won't try to specify
> both a floppy disk controller and an AppleSMC device), but you might
> want to handle that case nonetheless (exit with an error or some such).

I couldn't find a command line option to prevent QEMU from starting
with a floppy controller, so unless I missed it, we'd always detect
a "conflict".

According to the applesmc.c source, the emulated Apple SMC doesn't
support IRQ, so the number itself should be irrelevant. IRQ #6 is
what's used on real Apple hardware, but when I tried with a different
number (e.g. #5), OS X booted fine in QEMU (it does fail to boot if
we leave out IRQNoFlags entirely from the SMC DSDT node, though).

I could patch the value of FDEN to 0 whenever I enable the SMC _STA
method (i.e, when I patch its value to 0x0B), but that still wouldn't
take care of the fact that the emulated FDC is still present.

So, my preferred course of action would be, in this order:

	1. Do nothing :)

or

	2. Use "IRQNoFlags() { 5 }" with the SMC (or any other
	   number that isn't already allocated.

Any other suggestions or ideas would be welcome !

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v3] Add DSDT node for AppleSMC

[Laszlo Ersek]

On 12/22/13 18:14, Gabriel L. Somlo wrote:
> On Sun, Dec 22, 2013 at 04:58:58PM +0100, Laszlo Ersek wrote:
>> After this patch, ISA interrupt 6 is used by both "SMC" and "FDC0". The
>> latter depends on the FDEN object, but FDEN is currently constant 1.
>>
>> Probably not a problem in practice (ie. most users won't try to specify
>> both a floppy disk controller and an AppleSMC device), but you might
>> want to handle that case nonetheless (exit with an error or some such).
> 
> I couldn't find a command line option to prevent QEMU from starting
> with a floppy controller, so unless I missed it, we'd always detect
> a "conflict".
> 
> According to the applesmc.c source, the emulated Apple SMC doesn't
> support IRQ, so the number itself should be irrelevant. IRQ #6 is
> what's used on real Apple hardware, but when I tried with a different
> number (e.g. #5), OS X booted fine in QEMU (it does fail to boot if
> we leave out IRQNoFlags entirely from the SMC DSDT node, though).
> 
> I could patch the value of FDEN to 0 whenever I enable the SMC _STA
> method (i.e, when I patch its value to 0x0B), but that still wouldn't
> take care of the fact that the emulated FDC is still present.
> 
> So, my preferred course of action would be, in this order:
> 
> 	1. Do nothing :)
> 
> or
> 
> 	2. Use "IRQNoFlags() { 5 }" with the SMC (or any other
> 	   number that isn't already allocated.

I don't think there's anything left free:

 0 - system timer (not listed explicitly)
 1 - KBD (PNP0303)
 2 - cascade / PIC (PNP0000) (listed only in OVMF's builtin DSDT)
 3 - COM2 (PNP0501)
 4 - COM1 (PNP0501)
 5 - LNK[ABCD] (PNP0C0F)
 6 - FDC0 (PNP0700)
 7 - LPT (PNP0400)
 8 - RTC (PNP0B00)
 9 - LNKS (PNP0C0F)
10 - LNK[ABCD] (PNP0C0F)
11 - LNK[ABCD] (PNP0C0F)
12 - MOU (PNP0F13)
13 - FPU (PNP0C04) (listed only in OVMF's builtin DSDT)
14 - primary IDE (not listed explicitly)
15 - secondary IDE (not listed explicitly)

See also

http://www.plasma-online.de/english/identify/serial/pnp_id_pnp.html
http://www.webopedia.com/quick_ref/IRQnumbers.asp

You could reuse eg. #5, but then you'd have to distribute PCI LNK[ABCD]
over #10 and #11 only, which I guess is too high a price (both
patch-wise and at runtime). Rather don't touch that :)

> Any other suggestions or ideas would be welcome !

I guess the "by the book" solution would be to really stop the FDC from
being emulated when the AppleSMC is present, but I mention that idea
only because I like to waste bandwidth.

Option 1 ("Do nothing") sounds appropriate to me. Sorry for taking up
some of your time...

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH v3] Add DSDT node for AppleSMC

[Gabriel L. Somlo]

On Sun, Dec 22, 2013 at 11:21:00PM +0100, Laszlo Ersek wrote:
> > 	2. Use "IRQNoFlags() { 5 }" with the SMC (or any other
> > 	   number that isn't already allocated.
> 
> I don't think there's anything left free:
>...
> 
> I guess the "by the book" solution would be to really stop the FDC from
> being emulated when the AppleSMC is present, but I mention that idea
> only because I like to waste bandwidth.
> 
> Option 1 ("Do nothing") sounds appropriate to me.

So making the FDC optional (or at least allowing it to be left out)
sounds like it could be a fun project I could play with later (unless
anyone else beats me to it), but it would be nice if it didn't end up
a hard precondition for getting the SMC ACPI patch accepted :)

Once we can turn off the FDC, we can make it and the SMC mutually
exclusive and/or throw an error if both are enabled.

In reality (and Alex, please correct me if I'm wrong), the emulated
SMC will never generate an INT#6, unlike the real hardware chip. The
emulated SMC is just there to say "Yeah, boss, sure, let me get right
on that for you!" to OS X, to calm it down and make it think everything
is right with its little universe :) The real chip might trigger an
interrupt if something's getting too hot, or a fan stopped spinning
when it shouldn't have, but that's never going to happen on a VM guest.

OS X doesn't even assume the presence of an FDC, and any guest OS
which expects an FDC will never get unexpected conflicting interrupts
from the emulated SMC, should the latter be enabled, accidentally or
not. So in practice the risk for any trouble should be about zero...

Thanks for helping me think this stuff through !
--Gabriel

Re: [Qemu-devel] [PATCH v3] Add DSDT node for AppleSMC

[Michael S. Tsirkin]

On Sun, Dec 22, 2013 at 10:34:56AM -0500, Gabriel L. Somlo wrote:
> AppleSMC (-device isa-applesmc) is required to boot OS X guests.
> OS X expects a SMC node to be present in the ACPI DSDT. This patch
> adds a SMC node to the DSDT, and dynamically patches the return value
> of SMC._STA to either 0x0B if the chip is present, or otherwise to 0x00,
> before booting the guest.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>

Applied, thanks.

> ---
> 
> On Sun, Dec 22, 2013 at 01:07:13PM +0200, Michael S. Tsirkin wrote:
> >> +#define _CONCAT_SYM(a, b) a##b
> >> +#define CONCAT_SYM(a, b) _CONCAT_SYM(a, b)
> >
> > Seems too complex. If one looks for q35_smc_sta one can not find it
> 
> You're right, I went all "Rube Goldberg" on that one - thanks for
> pulling me back from the edge :)
> 
> >> +    unsigned short smc_sta_val = 0x00;
> > 
> > Better initialize this in the else branch below.
> 
> I threw in "applesmc_find()" to make that even cleaner and easier to
> follow.
> 
> Thanks,
>   Gabriel
> 
>  hw/misc/applesmc.c        |  1 -
>  include/hw/isa/isa.h      |  7 +++++++
>  hw/i386/acpi-dsdt.dsl     |  1 +
>  hw/i386/q35-acpi-dsdt.dsl |  1 +
>  hw/i386/acpi-dsdt-isa.dsl | 11 +++++++++++
>  hw/i386/acpi-build.c      |  9 +++++++++
>  6 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 1e8d183..627adb9 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -66,7 +66,6 @@ struct AppleSMCData {
>      QLIST_ENTRY(AppleSMCData) node;
>  };
>  
> -#define TYPE_APPLE_SMC "isa-applesmc"
>  #define APPLE_SMC(obj) OBJECT_CHECK(AppleSMCState, (obj), TYPE_APPLE_SMC)
>  
>  typedef struct AppleSMCState AppleSMCState;
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index fa45a5b..e0c749f 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -20,6 +20,13 @@
>  #define TYPE_ISA_BUS "ISA"
>  #define ISA_BUS(obj) OBJECT_CHECK(ISABus, (obj), TYPE_ISA_BUS)
>  
> +#define TYPE_APPLE_SMC "isa-applesmc"
> +
> +static inline bool applesmc_find(void)
> +{
> +    return object_resolve_path_type("", TYPE_APPLE_SMC, NULL);
> +}
> +
>  typedef struct ISADeviceClass {
>      DeviceClass parent_class;
>  } ISADeviceClass;
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index a377424..b87c6e0 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -114,6 +114,7 @@ DefinitionBlock (
>          }
>      }
>  
> +#define DSDT_APPLESMC_STA piix_dsdt_applesmc_sta
>  #include "acpi-dsdt-isa.dsl"
>  
>  
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index 575c5d7..12ff544 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -171,6 +171,7 @@ DefinitionBlock (
>          }
>      }
>  
> +#define DSDT_APPLESMC_STA q35_dsdt_applesmc_sta
>  #include "acpi-dsdt-isa.dsl"
>  
>  
> diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
> index 89caa16..46942c1 100644
> --- a/hw/i386/acpi-dsdt-isa.dsl
> +++ b/hw/i386/acpi-dsdt-isa.dsl
> @@ -16,6 +16,17 @@
>  /* Common legacy ISA style devices. */
>  Scope(\_SB.PCI0.ISA) {
>  
> +    Device (SMC) {
> +        Name(_HID, EisaId("APP0001"))
> +        /* _STA will be patched to 0x0B if AppleSMC is present */
> +        ACPI_EXTRACT_NAME_WORD_CONST DSDT_APPLESMC_STA
> +        Name(_STA, 0xFF00)
> +        Name(_CRS, ResourceTemplate () {
> +            IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
> +            IRQNoFlags() { 6 }
> +        })
> +    }
> +
>      Device(RTC) {
>          Name(_HID, EisaId("PNP0B00"))
>          Name(_CRS, ResourceTemplate() {
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 48312f5..30bfcd2 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -36,6 +36,7 @@
>  #include "hw/nvram/fw_cfg.h"
>  #include "bios-linker-loader.h"
>  #include "hw/loader.h"
> +#include "hw/isa/isa.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -80,6 +81,7 @@ typedef struct AcpiMiscInfo {
>  
>  static void acpi_get_dsdt(AcpiMiscInfo *info)
>  {
> +    unsigned short applesmc_sta_val, *applesmc_sta_off;
>      Object *piix = piix4_pm_find();
>      Object *lpc = ich9_lpc_find();
>      assert(!!piix != !!lpc);
> @@ -87,11 +89,18 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
>      if (piix) {
>          info->dsdt_code = AcpiDsdtAmlCode;
>          info->dsdt_size = sizeof AcpiDsdtAmlCode;
> +        applesmc_sta_off = piix_dsdt_applesmc_sta;
>      }
>      if (lpc) {
>          info->dsdt_code = Q35AcpiDsdtAmlCode;
>          info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
> +        applesmc_sta_off = q35_dsdt_applesmc_sta;
>      }
> +
> +    /* Patch in appropriate value for AppleSMC _STA */
> +    applesmc_sta_val = applesmc_find() ? 0x0b : 0x00;
> +    *(uint16_t *)(info->dsdt_code + *applesmc_sta_off) =
> +        cpu_to_le16(applesmc_sta_val);
>  }
>  
>  static
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH v3] Add DSDT node for AppleSMC

[Alexander Graf]

> Am 23.12.2013 um 04:19 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
> 
> On Sun, Dec 22, 2013 at 11:21:00PM +0100, Laszlo Ersek wrote:
>>>    2. Use "IRQNoFlags() { 5 }" with the SMC (or any other
>>>       number that isn't already allocated.
>> 
>> I don't think there's anything left free:
>> ...
>> 
>> I guess the "by the book" solution would be to really stop the FDC from
>> being emulated when the AppleSMC is present, but I mention that idea
>> only because I like to waste bandwidth.
>> 
>> Option 1 ("Do nothing") sounds appropriate to me.
> 
> So making the FDC optional (or at least allowing it to be left out)
> sounds like it could be a fun project I could play with later (unless
> anyone else beats me to it), but it would be nice if it didn't end up
> a hard precondition for getting the SMC ACPI patch accepted :)
> 
> Once we can turn off the FDC, we can make it and the SMC mutually
> exclusive and/or throw an error if both are enabled.
> 
> In reality (and Alex, please correct me if I'm wrong), the emulated
> SMC will never generate an INT#6, unlike the real hardware chip. The
> emulated SMC is just there to say "Yeah, boss, sure, let me get right
> on that for you!" to OS X, to calm it down and make it think everything
> is right with its little universe :) The real chip might trigger an
> interrupt if something's getting too hot, or a fan stopped spinning
> when it shouldn't have, but that's never going to happen on a VM guest.

Its main purpose is to signal you dropping your notebook, so the system can shut down your hard drive.

The main issue I can see with the irq line is that we now potentially have two edge triggered irq sources on a single line, so an OS may get confused.

But I agree, let's get that one solved later.

> 
> OS X doesn't even assume the presence of an FDC, and any guest OS
> which expects an FDC will never get unexpected conflicting interrupts
> from the emulated SMC, should the latter be enabled, accidentally or
> not. So in practice the risk for any trouble should be about zero...

It may assign the irq line to the applesmc driver, killing the fdc's functionality.

Speaking of which, does the q35 even have an fdc?


Alex

> 
> Thanks for helping me think this stuff through !
> --Gabriel

Re: [Qemu-devel] [PATCH v3] Add DSDT node for AppleSMC

[Michael S. Tsirkin]

On Wed, Dec 25, 2013 at 08:11:56PM +0100, Alexander Graf wrote:
> 
> 
> > Am 23.12.2013 um 04:19 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
> > 
> > On Sun, Dec 22, 2013 at 11:21:00PM +0100, Laszlo Ersek wrote:
> >>>    2. Use "IRQNoFlags() { 5 }" with the SMC (or any other
> >>>       number that isn't already allocated.
> >> 
> >> I don't think there's anything left free:
> >> ...
> >> 
> >> I guess the "by the book" solution would be to really stop the FDC from
> >> being emulated when the AppleSMC is present, but I mention that idea
> >> only because I like to waste bandwidth.
> >> 
> >> Option 1 ("Do nothing") sounds appropriate to me.
> > 
> > So making the FDC optional (or at least allowing it to be left out)
> > sounds like it could be a fun project I could play with later (unless
> > anyone else beats me to it), but it would be nice if it didn't end up
> > a hard precondition for getting the SMC ACPI patch accepted :)
> > 
> > Once we can turn off the FDC, we can make it and the SMC mutually
> > exclusive and/or throw an error if both are enabled.
> > 
> > In reality (and Alex, please correct me if I'm wrong), the emulated
> > SMC will never generate an INT#6, unlike the real hardware chip. The
> > emulated SMC is just there to say "Yeah, boss, sure, let me get right
> > on that for you!" to OS X, to calm it down and make it think everything
> > is right with its little universe :) The real chip might trigger an
> > interrupt if something's getting too hot, or a fan stopped spinning
> > when it shouldn't have, but that's never going to happen on a VM guest.
> 
> Its main purpose is to signal you dropping your notebook, so the system can shut down your hard drive.
> 
> The main issue I can see with the irq line is that we now potentially have two edge triggered irq sources on a single line, so an OS may get confused.
> 
> But I agree, let's get that one solved later.
> 
> > 
> > OS X doesn't even assume the presence of an FDC, and any guest OS
> > which expects an FDC will never get unexpected conflicting interrupts
> > from the emulated SMC, should the latter be enabled, accidentally or
> > not. So in practice the risk for any trouble should be about zero...
> 
> It may assign the irq line to the applesmc driver, killing the fdc's functionality.
> 
> Speaking of which, does the q35 even have an fdc?
> 
> 
> Alex

I don't think it does but this device seems to be supported with piix as well -
or is this by mistake?

> > 
> > Thanks for helping me think this stuff through !
> > --Gabriel

Re: [Qemu-devel] [PATCH v3] Add DSDT node for AppleSMC

[Alexander Graf]

On 25.12.2013, at 20:20, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Dec 25, 2013 at 08:11:56PM +0100, Alexander Graf wrote:
>> 
>> 
>>> Am 23.12.2013 um 04:19 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
>>> 
>>> On Sun, Dec 22, 2013 at 11:21:00PM +0100, Laszlo Ersek wrote:
>>>>>   2. Use "IRQNoFlags() { 5 }" with the SMC (or any other
>>>>>      number that isn't already allocated.
>>>> 
>>>> I don't think there's anything left free:
>>>> ...
>>>> 
>>>> I guess the "by the book" solution would be to really stop the FDC from
>>>> being emulated when the AppleSMC is present, but I mention that idea
>>>> only because I like to waste bandwidth.
>>>> 
>>>> Option 1 ("Do nothing") sounds appropriate to me.
>>> 
>>> So making the FDC optional (or at least allowing it to be left out)
>>> sounds like it could be a fun project I could play with later (unless
>>> anyone else beats me to it), but it would be nice if it didn't end up
>>> a hard precondition for getting the SMC ACPI patch accepted :)
>>> 
>>> Once we can turn off the FDC, we can make it and the SMC mutually
>>> exclusive and/or throw an error if both are enabled.
>>> 
>>> In reality (and Alex, please correct me if I'm wrong), the emulated
>>> SMC will never generate an INT#6, unlike the real hardware chip. The
>>> emulated SMC is just there to say "Yeah, boss, sure, let me get right
>>> on that for you!" to OS X, to calm it down and make it think everything
>>> is right with its little universe :) The real chip might trigger an
>>> interrupt if something's getting too hot, or a fan stopped spinning
>>> when it shouldn't have, but that's never going to happen on a VM guest.
>> 
>> Its main purpose is to signal you dropping your notebook, so the system can shut down your hard drive.
>> 
>> The main issue I can see with the irq line is that we now potentially have two edge triggered irq sources on a single line, so an OS may get confused.
>> 
>> But I agree, let's get that one solved later.
>> 
>>> 
>>> OS X doesn't even assume the presence of an FDC, and any guest OS
>>> which expects an FDC will never get unexpected conflicting interrupts
>>> from the emulated SMC, should the latter be enabled, accidentally or
>>> not. So in practice the risk for any trouble should be about zero...
>> 
>> It may assign the irq line to the applesmc driver, killing the fdc's functionality.
>> 
>> Speaking of which, does the q35 even have an fdc?
>> 
>> 
>> Alex
> 
> I don't think it does but this device seems to be supported with piix as well -
> or is this by mistake?

No, the device doesn't care whether the chipset is Q35 or PIIX. Back when I first got OSX working at all I was using PIIX because there was no Q35 emulation available, but the Q35 is a lot closer to what Mac OS X expects (the earliest chipset it runs on is an ICH6).

Either way, this whole thing is optional anyway, right? I don't think anyone will add the applesmc controller if he doesn't want to run an OSX guest, so we should be safe.


Alex

Re: [Qemu-devel] [PATCH v3] Add DSDT node for AppleSMC

[Michael S. Tsirkin]

On Wed, Dec 25, 2013 at 10:33:16PM +0100, Alexander Graf wrote:
> 
> On 25.12.2013, at 20:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> > On Wed, Dec 25, 2013 at 08:11:56PM +0100, Alexander Graf wrote:
> >> 
> >> 
> >>> Am 23.12.2013 um 04:19 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
> >>> 
> >>> On Sun, Dec 22, 2013 at 11:21:00PM +0100, Laszlo Ersek wrote:
> >>>>>   2. Use "IRQNoFlags() { 5 }" with the SMC (or any other
> >>>>>      number that isn't already allocated.
> >>>> 
> >>>> I don't think there's anything left free:
> >>>> ...
> >>>> 
> >>>> I guess the "by the book" solution would be to really stop the FDC from
> >>>> being emulated when the AppleSMC is present, but I mention that idea
> >>>> only because I like to waste bandwidth.
> >>>> 
> >>>> Option 1 ("Do nothing") sounds appropriate to me.
> >>> 
> >>> So making the FDC optional (or at least allowing it to be left out)
> >>> sounds like it could be a fun project I could play with later (unless
> >>> anyone else beats me to it), but it would be nice if it didn't end up
> >>> a hard precondition for getting the SMC ACPI patch accepted :)
> >>> 
> >>> Once we can turn off the FDC, we can make it and the SMC mutually
> >>> exclusive and/or throw an error if both are enabled.
> >>> 
> >>> In reality (and Alex, please correct me if I'm wrong), the emulated
> >>> SMC will never generate an INT#6, unlike the real hardware chip. The
> >>> emulated SMC is just there to say "Yeah, boss, sure, let me get right
> >>> on that for you!" to OS X, to calm it down and make it think everything
> >>> is right with its little universe :) The real chip might trigger an
> >>> interrupt if something's getting too hot, or a fan stopped spinning
> >>> when it shouldn't have, but that's never going to happen on a VM guest.
> >> 
> >> Its main purpose is to signal you dropping your notebook, so the system can shut down your hard drive.
> >> 
> >> The main issue I can see with the irq line is that we now potentially have two edge triggered irq sources on a single line, so an OS may get confused.
> >> 
> >> But I agree, let's get that one solved later.
> >> 
> >>> 
> >>> OS X doesn't even assume the presence of an FDC, and any guest OS
> >>> which expects an FDC will never get unexpected conflicting interrupts
> >>> from the emulated SMC, should the latter be enabled, accidentally or
> >>> not. So in practice the risk for any trouble should be about zero...
> >> 
> >> It may assign the irq line to the applesmc driver, killing the fdc's functionality.
> >> 
> >> Speaking of which, does the q35 even have an fdc?
> >> 
> >> 
> >> Alex
> > 
> > I don't think it does but this device seems to be supported with piix as well -
> > or is this by mistake?
> 
> No, the device doesn't care whether the chipset is Q35 or PIIX. Back when I first got OSX working at all I was using PIIX because there was no Q35 emulation available, but the Q35 is a lot closer to what Mac OS X expects (the earliest chipset it runs on is an ICH6).
> 
> Either way, this whole thing is optional anyway, right? I don't think anyone will add the applesmc controller if he doesn't want to run an OSX guest, so we should be safe.
> 
> 
> Alex

Yes, seems pretty safe - that's why I took this patch.
If you like, send an incremental one with a comment so we remember
to look as this if we ever see issues.

[Qemu-devel] [PATCH v4] Add DSDT node for AppleSMC

[Gabriel L. Somlo]

AppleSMC (-device isa-applesmc) is required to boot OS X guests.
OS X expects a SMC node to be present in the ACPI DSDT. This patch
adds a SMC node to the DSDT, and dynamically patches the return value
of SMC._STA to either 0x0B if the chip is present, or otherwise to 0x00,
before booting the guest.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

On Wed, Jan 08, 2014 at 10:38:21PM +0200, Michael S. Tsirkin wrote:
> > +        /* _STA will be patched to 0x0F if the FDC is present */
> > +        ACPI_EXTRACT_NAME_WORD_CONST DSDT_FDC_STA
> > +        Name(_STA, 0xFF00)
>
> I'm not sure why this is WORD. Spec says bits 0-4 have meaning bits up
> to 31 are cleared. So should not this be either dword (to make all
> 32 bits explicit) or byte (to make it small)?

Here's another version with this issue fixed (using DWORD, and uint32_t).

On Wed, Dec 25, 2013 at 04:12:28PM +0200, Michael S. Tsirkin wrote:
> Applied, thanks.

Please let me know if you'd rather I sent you an incremental patch
instead. v3 hasn't yet shown up in the qemu git master, so I'm not
100% sure which way to proceed at this point...

Thanks,
  Gabriel

 hw/misc/applesmc.c        |  1 -
 include/hw/isa/isa.h      |  7 +++++++
 hw/i386/acpi-dsdt.dsl     |  1 +
 hw/i386/q35-acpi-dsdt.dsl |  1 +
 hw/i386/acpi-dsdt-isa.dsl | 11 +++++++++++
 hw/i386/acpi-build.c      |  9 +++++++++
 6 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 1e8d183..627adb9 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -66,7 +66,6 @@ struct AppleSMCData {
     QLIST_ENTRY(AppleSMCData) node;
 };
 
-#define TYPE_APPLE_SMC "isa-applesmc"
 #define APPLE_SMC(obj) OBJECT_CHECK(AppleSMCState, (obj), TYPE_APPLE_SMC)
 
 typedef struct AppleSMCState AppleSMCState;
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index fa45a5b..e0c749f 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -20,6 +20,13 @@
 #define TYPE_ISA_BUS "ISA"
 #define ISA_BUS(obj) OBJECT_CHECK(ISABus, (obj), TYPE_ISA_BUS)
 
+#define TYPE_APPLE_SMC "isa-applesmc"
+
+static inline bool applesmc_find(void)
+{
+    return object_resolve_path_type("", TYPE_APPLE_SMC, NULL);
+}
+
 typedef struct ISADeviceClass {
     DeviceClass parent_class;
 } ISADeviceClass;
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index a377424..b87c6e0 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -114,6 +114,7 @@ DefinitionBlock (
         }
     }
 
+#define DSDT_APPLESMC_STA piix_dsdt_applesmc_sta
 #include "acpi-dsdt-isa.dsl"
 
 
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 575c5d7..12ff544 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -171,6 +171,7 @@ DefinitionBlock (
         }
     }
 
+#define DSDT_APPLESMC_STA q35_dsdt_applesmc_sta
 #include "acpi-dsdt-isa.dsl"
 
 
diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
index 89caa16..46942c1 100644
--- a/hw/i386/acpi-dsdt-isa.dsl
+++ b/hw/i386/acpi-dsdt-isa.dsl
@@ -16,6 +16,17 @@
 /* Common legacy ISA style devices. */
 Scope(\_SB.PCI0.ISA) {
 
+    Device (SMC) {
+        Name(_HID, EisaId("APP0001"))
+        /* _STA will be patched to 0x0B if AppleSMC is present */
+        ACPI_EXTRACT_NAME_DWORD_CONST DSDT_APPLESMC_STA
+        Name(_STA, 0xFFFFFF00)
+        Name(_CRS, ResourceTemplate () {
+            IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
+            IRQNoFlags() { 6 }
+        })
+    }
+
     Device(RTC) {
         Name(_HID, EisaId("PNP0B00"))
         Name(_CRS, ResourceTemplate() {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 48312f5..30bfcd2 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -36,6 +36,7 @@
 #include "hw/nvram/fw_cfg.h"
 #include "bios-linker-loader.h"
 #include "hw/loader.h"
+#include "hw/isa/isa.h"
 
 /* Supported chipsets: */
 #include "hw/acpi/piix4.h"
@@ -80,6 +81,7 @@ typedef struct AcpiMiscInfo {
 
 static void acpi_get_dsdt(AcpiMiscInfo *info)
 {
+    uint16_t applesmc_sta_val, *applesmc_sta_off;
     Object *piix = piix4_pm_find();
     Object *lpc = ich9_lpc_find();
     assert(!!piix != !!lpc);
@@ -87,11 +89,18 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
     if (piix) {
         info->dsdt_code = AcpiDsdtAmlCode;
         info->dsdt_size = sizeof AcpiDsdtAmlCode;
+        applesmc_sta_off = piix_dsdt_applesmc_sta;
     }
     if (lpc) {
         info->dsdt_code = Q35AcpiDsdtAmlCode;
         info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
+        applesmc_sta_off = q35_dsdt_applesmc_sta;
     }
+
+    /* Patch in appropriate value for AppleSMC _STA */
+    applesmc_sta_val = applesmc_find() ? 0x0b : 0x00;
+    *(uint32_t *)(info->dsdt_code + *applesmc_sta_off) =
+        cpu_to_le32(applesmc_sta_val);
 }
 
 static
-- 
1.8.1.4

[Qemu-devel] [PATCH] ACPI: AppleSMC _STA method should return 32bit value

[Gabriel L. Somlo]

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

This applies incrementally on top of "Add DSDT node for AppleSMC" v.3
(accepted Dec. 25, 2013), and obsoletes v.4 of same with cleaner code
in acpi-build.c.

Thanks,
  Gabriel

 hw/i386/acpi-dsdt-isa.dsl |  4 ++--
 hw/i386/acpi-build.c      | 11 +++++------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
index 46942c1..3a6e100 100644
--- a/hw/i386/acpi-dsdt-isa.dsl
+++ b/hw/i386/acpi-dsdt-isa.dsl
@@ -19,8 +19,8 @@ Scope(\_SB.PCI0.ISA) {
     Device (SMC) {
         Name(_HID, EisaId("APP0001"))
         /* _STA will be patched to 0x0B if AppleSMC is present */
-        ACPI_EXTRACT_NAME_WORD_CONST DSDT_APPLESMC_STA
-        Name(_STA, 0xFF00)
+        ACPI_EXTRACT_NAME_DWORD_CONST DSDT_APPLESMC_STA
+        Name(_STA, 0xFFFFFF00)
         Name(_CRS, ResourceTemplate () {
             IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
             IRQNoFlags() { 6 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 30bfcd2..9e5733e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -81,7 +81,7 @@ typedef struct AcpiMiscInfo {
 
 static void acpi_get_dsdt(AcpiMiscInfo *info)
 {
-    unsigned short applesmc_sta_val, *applesmc_sta_off;
+    uint16_t *applesmc_sta;
     Object *piix = piix4_pm_find();
     Object *lpc = ich9_lpc_find();
     assert(!!piix != !!lpc);
@@ -89,18 +89,17 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
     if (piix) {
         info->dsdt_code = AcpiDsdtAmlCode;
         info->dsdt_size = sizeof AcpiDsdtAmlCode;
-        applesmc_sta_off = piix_dsdt_applesmc_sta;
+        applesmc_sta = piix_dsdt_applesmc_sta;
     }
     if (lpc) {
         info->dsdt_code = Q35AcpiDsdtAmlCode;
         info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
-        applesmc_sta_off = q35_dsdt_applesmc_sta;
+        applesmc_sta = q35_dsdt_applesmc_sta;
     }
 
     /* Patch in appropriate value for AppleSMC _STA */
-    applesmc_sta_val = applesmc_find() ? 0x0b : 0x00;
-    *(uint16_t *)(info->dsdt_code + *applesmc_sta_off) =
-        cpu_to_le16(applesmc_sta_val);
+    *(uint32_t *)(info->dsdt_code + *applesmc_sta) =
+        cpu_to_le32(applesmc_find() ? 0x0b : 0x00);
 }
 
 static
-- 
1.8.1.4

[Qemu-devel] [PATCH v2] ACPI: Fix AppleSMC _STA size

[Gabriel L. Somlo]

Minimize the storage used for AppleSMC's _STA (8bit), relying on ASL
to implicitly convert it to the officially specified 32bit value.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

This applies incrementally on top of "Add DSDT node for AppleSMC" v.3
(accepted Dec. 25, 2013), and obsoletes everything I've sent out since :)

After some experimentation I realized ASL is flexible about the size of
its named variables, and happy to implicitly convert an 8bit value to
32 bits when needed. As such, I vote for the space saving option...

Thanks, and sorry for all the noise,
  Gabriel

 hw/i386/acpi-dsdt-isa.dsl |  4 ++--
 hw/i386/acpi-build.c      | 11 +++++------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
index 46942c1..3a6e100 100644
--- a/hw/i386/acpi-dsdt-isa.dsl
+++ b/hw/i386/acpi-dsdt-isa.dsl
@@ -19,8 +19,8 @@ Scope(\_SB.PCI0.ISA) {
     Device (SMC) {
         Name(_HID, EisaId("APP0001"))
         /* _STA will be patched to 0x0B if AppleSMC is present */
-        ACPI_EXTRACT_NAME_WORD_CONST DSDT_APPLESMC_STA
-        Name(_STA, 0xFF00)
+        ACPI_EXTRACT_NAME_BYTE_CONST DSDT_APPLESMC_STA
+        Name(_STA, 0xF0)
         Name(_CRS, ResourceTemplate () {
             IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
             IRQNoFlags() { 6 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 30bfcd2..9e5733e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -81,7 +81,7 @@ typedef struct AcpiMiscInfo {
 
 static void acpi_get_dsdt(AcpiMiscInfo *info)
 {
-    unsigned short applesmc_sta_val, *applesmc_sta_off;
+    uint16_t *applesmc_sta;
     Object *piix = piix4_pm_find();
     Object *lpc = ich9_lpc_find();
     assert(!!piix != !!lpc);
@@ -89,18 +89,17 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
     if (piix) {
         info->dsdt_code = AcpiDsdtAmlCode;
         info->dsdt_size = sizeof AcpiDsdtAmlCode;
-        applesmc_sta_off = piix_dsdt_applesmc_sta;
+        applesmc_sta = piix_dsdt_applesmc_sta;
     }
     if (lpc) {
         info->dsdt_code = Q35AcpiDsdtAmlCode;
         info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
-        applesmc_sta_off = q35_dsdt_applesmc_sta;
+        applesmc_sta = q35_dsdt_applesmc_sta;
     }
 
     /* Patch in appropriate value for AppleSMC _STA */
-    applesmc_sta_val = applesmc_find() ? 0x0b : 0x00;
-    *(uint16_t *)(info->dsdt_code + *applesmc_sta_off) =
-        cpu_to_le16(applesmc_sta_val);
+    *(uint8_t *)(info->dsdt_code + *applesmc_sta) =
+        applesmc_find() ? 0x0b : 0x00;
 }
 
 static
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2] ACPI: Fix AppleSMC _STA size

[Michael S. Tsirkin]

On Mon, Jan 13, 2014 at 03:27:13PM -0500, Gabriel L. Somlo wrote:
> Minimize the storage used for AppleSMC's _STA (8bit), relying on ASL
> to implicitly convert it to the officially specified 32bit value.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> This applies incrementally on top of "Add DSDT node for AppleSMC" v.3
> (accepted Dec. 25, 2013), and obsoletes everything I've sent out since :)
> 
> After some experimentation I realized ASL is flexible about the size of
> its named variables, and happy to implicitly convert an 8bit value to
> 32 bits when needed. As such, I vote for the space saving option...
> 
> Thanks, and sorry for all the noise,
>   Gabriel


Applied, thanks.

>  hw/i386/acpi-dsdt-isa.dsl |  4 ++--
>  hw/i386/acpi-build.c      | 11 +++++------
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
> index 46942c1..3a6e100 100644
> --- a/hw/i386/acpi-dsdt-isa.dsl
> +++ b/hw/i386/acpi-dsdt-isa.dsl
> @@ -19,8 +19,8 @@ Scope(\_SB.PCI0.ISA) {
>      Device (SMC) {
>          Name(_HID, EisaId("APP0001"))
>          /* _STA will be patched to 0x0B if AppleSMC is present */
> -        ACPI_EXTRACT_NAME_WORD_CONST DSDT_APPLESMC_STA
> -        Name(_STA, 0xFF00)
> +        ACPI_EXTRACT_NAME_BYTE_CONST DSDT_APPLESMC_STA
> +        Name(_STA, 0xF0)
>          Name(_CRS, ResourceTemplate () {
>              IO (Decode16, 0x0300, 0x0300, 0x01, 0x20)
>              IRQNoFlags() { 6 }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 30bfcd2..9e5733e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -81,7 +81,7 @@ typedef struct AcpiMiscInfo {
>  
>  static void acpi_get_dsdt(AcpiMiscInfo *info)
>  {
> -    unsigned short applesmc_sta_val, *applesmc_sta_off;
> +    uint16_t *applesmc_sta;
>      Object *piix = piix4_pm_find();
>      Object *lpc = ich9_lpc_find();
>      assert(!!piix != !!lpc);
> @@ -89,18 +89,17 @@ static void acpi_get_dsdt(AcpiMiscInfo *info)
>      if (piix) {
>          info->dsdt_code = AcpiDsdtAmlCode;
>          info->dsdt_size = sizeof AcpiDsdtAmlCode;
> -        applesmc_sta_off = piix_dsdt_applesmc_sta;
> +        applesmc_sta = piix_dsdt_applesmc_sta;
>      }
>      if (lpc) {
>          info->dsdt_code = Q35AcpiDsdtAmlCode;
>          info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
> -        applesmc_sta_off = q35_dsdt_applesmc_sta;
> +        applesmc_sta = q35_dsdt_applesmc_sta;
>      }
>  
>      /* Patch in appropriate value for AppleSMC _STA */
> -    applesmc_sta_val = applesmc_find() ? 0x0b : 0x00;
> -    *(uint16_t *)(info->dsdt_code + *applesmc_sta_off) =
> -        cpu_to_le16(applesmc_sta_val);
> +    *(uint8_t *)(info->dsdt_code + *applesmc_sta) =
> +        applesmc_find() ? 0x0b : 0x00;
>  }
>  
>  static
> -- 
> 1.8.1.4