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 <so...@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
