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

["Michael S. Tsirkin" <mst@redhat.com>]

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