[PATCH] applesmc: add sysfs file to report OSK

[PATCH] applesmc: add sysfs file to report OSK

[Gabriel L. Somlo]

The AppleSMC contains two char[32] keys, OSK0 and OSK1, which are not
reported in the key count and index by default. These keys are used by
the OS X boot sequence, and normally don't matter when running Linux.

This patch creates a sysfs entry which reports the value of these keys
as an ASCII string, to help emulators (such as QEMU) load OS X when
running on genuine Apple hardware.

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

For extra context: To boot OS X as a guest, QEMU must (among others)
emulate the AppleSMC. To boot successfully, OS X insists on querying
the (emulated) SMC for the value of OSK0 and OSK1. Currently, these
values must be supplied on the QEMU command line as

  -device applesmc,osk="...concatenated values of OSK0 and OSK1..."

With the availability of /sys/devices/platform/applesmc.768/osk, the
emulated QEMU AppleSMC could acquire this string directly from the
(Apple-manufactured) host machine.

Thanks,
  Gabriel

 drivers/hwmon/applesmc.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index b41baff..0c7cc71 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -1013,6 +1013,23 @@ static ssize_t applesmc_key_at_index_store(struct device *dev,
 	return count;
 }
 
+static ssize_t applesmc_osk_show(struct device *dev,
+				struct device_attribute *attr, char *sysfsbuf)
+{
+	int fail;
+
+	mutex_lock(&smcreg.mutex);
+	fail = read_smc(APPLESMC_READ_CMD, "OSK0", sysfsbuf, 32) ||
+	       read_smc(APPLESMC_READ_CMD, "OSK1", sysfsbuf + 32, 32);
+	mutex_unlock(&smcreg.mutex);
+	if (fail)
+		return -1;
+
+	sysfsbuf[64] = '\n';
+	sysfsbuf[65] = '\0';
+	return 65;
+}
+
 static struct led_classdev applesmc_backlight = {
 	.name			= "smc::kbd_backlight",
 	.default_trigger	= "nand-disk",
@@ -1027,6 +1044,7 @@ static struct applesmc_node_group info_group[] = {
 	{ "key_at_index_type", applesmc_key_at_index_type_show },
 	{ "key_at_index_data_length", applesmc_key_at_index_data_length_show },
 	{ "key_at_index_data", applesmc_key_at_index_read_show },
+	{ "osk", applesmc_osk_show },
 	{ }
 };
 
-- 
1.7.7.6

Re: [PATCH] applesmc: add sysfs file to report OSK

[Alexander Graf]

On 10.12.2012, at 15:51, "Gabriel L. Somlo" <somlo@cmu.edu> wrote:

> The AppleSMC contains two char[32] keys, OSK0 and OSK1, which are not
> reported in the key count and index by default. These keys are used by
> the OS X boot sequence, and normally don't matter when running Linux.
> 
> This patch creates a sysfs entry which reports the value of these keys
> as an ASCII string, to help emulators (such as QEMU) load OS X when
> running on genuine Apple hardware.
> 
> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>

Acked-by: Alexander Graf <agraf@suse.de>

Alex

> ---
> 
> For extra context: To boot OS X as a guest, QEMU must (among others)
> emulate the AppleSMC. To boot successfully, OS X insists on querying
> the (emulated) SMC for the value of OSK0 and OSK1. Currently, these
> values must be supplied on the QEMU command line as
> 
>  -device applesmc,osk="...concatenated values of OSK0 and OSK1..."
> 
> With the availability of /sys/devices/platform/applesmc.768/osk, the
> emulated QEMU AppleSMC could acquire this string directly from the
> (Apple-manufactured) host machine.
> 
> Thanks,
>  Gabriel
> 
> drivers/hwmon/applesmc.c |   18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index b41baff..0c7cc71 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -1013,6 +1013,23 @@ static ssize_t applesmc_key_at_index_store(struct device *dev,
>    return count;
> }
> 
> +static ssize_t applesmc_osk_show(struct device *dev,
> +                struct device_attribute *attr, char *sysfsbuf)
> +{
> +    int fail;
> +
> +    mutex_lock(&smcreg.mutex);
> +    fail = read_smc(APPLESMC_READ_CMD, "OSK0", sysfsbuf, 32) ||
> +           read_smc(APPLESMC_READ_CMD, "OSK1", sysfsbuf + 32, 32);
> +    mutex_unlock(&smcreg.mutex);
> +    if (fail)
> +        return -1;
> +
> +    sysfsbuf[64] = '\n';
> +    sysfsbuf[65] = '\0';
> +    return 65;
> +}
> +
> static struct led_classdev applesmc_backlight = {
>    .name            = "smc::kbd_backlight",
>    .default_trigger    = "nand-disk",
> @@ -1027,6 +1044,7 @@ static struct applesmc_node_group info_group[] = {
>    { "key_at_index_type", applesmc_key_at_index_type_show },
>    { "key_at_index_data_length", applesmc_key_at_index_data_length_show },
>    { "key_at_index_data", applesmc_key_at_index_read_show },
> +    { "osk", applesmc_osk_show },
>    { }
> };
> 
> -- 
> 1.7.7.6
> 

Re: [PATCH] applesmc: add sysfs file to report OSK

[Guenter Roeck]

On Mon, Dec 10, 2012 at 09:51:35AM -0500, Gabriel L. Somlo wrote:
> The AppleSMC contains two char[32] keys, OSK0 and OSK1, which are not
> reported in the key count and index by default. These keys are used by
> the OS X boot sequence, and normally don't matter when running Linux.
> 
> This patch creates a sysfs entry which reports the value of these keys
> as an ASCII string, to help emulators (such as QEMU) load OS X when
> running on genuine Apple hardware.
> 
> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> ---
> 
> For extra context: To boot OS X as a guest, QEMU must (among others)
> emulate the AppleSMC. To boot successfully, OS X insists on querying
> the (emulated) SMC for the value of OSK0 and OSK1. Currently, these
> values must be supplied on the QEMU command line as
> 
>   -device applesmc,osk="...concatenated values of OSK0 and OSK1..."
> 
> With the availability of /sys/devices/platform/applesmc.768/osk, the
> emulated QEMU AppleSMC could acquire this string directly from the
> (Apple-manufactured) host machine.
> 
Hmm ... this is a non-hwmon attribute which doesn't really belong into hwmon
in the first place ... like several other attributes in the same driver.

So I'll leave it up to the maintainer to decide if we should accept it. Henrik ?

Thanks,
Guenter

> Thanks,
>   Gabriel
> 
>  drivers/hwmon/applesmc.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index b41baff..0c7cc71 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -1013,6 +1013,23 @@ static ssize_t applesmc_key_at_index_store(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t applesmc_osk_show(struct device *dev,
> +				struct device_attribute *attr, char *sysfsbuf)
> +{
> +	int fail;
> +
> +	mutex_lock(&smcreg.mutex);
> +	fail = read_smc(APPLESMC_READ_CMD, "OSK0", sysfsbuf, 32) ||
> +	       read_smc(APPLESMC_READ_CMD, "OSK1", sysfsbuf + 32, 32);
> +	mutex_unlock(&smcreg.mutex);
> +	if (fail)
> +		return -1;
> +
> +	sysfsbuf[64] = '\n';
> +	sysfsbuf[65] = '\0';
> +	return 65;
> +}
> +
>  static struct led_classdev applesmc_backlight = {
>  	.name			= "smc::kbd_backlight",
>  	.default_trigger	= "nand-disk",
> @@ -1027,6 +1044,7 @@ static struct applesmc_node_group info_group[] = {
>  	{ "key_at_index_type", applesmc_key_at_index_type_show },
>  	{ "key_at_index_data_length", applesmc_key_at_index_data_length_show },
>  	{ "key_at_index_data", applesmc_key_at_index_read_show },
> +	{ "osk", applesmc_osk_show },
>  	{ }
>  };
>  
> -- 
> 1.7.7.6
> 
> 

Re: [PATCH] applesmc: add sysfs file to report OSK

[Henrik Rydberg]

Hi Guenter,

> On Mon, Dec 10, 2012 at 09:51:35AM -0500, Gabriel L. Somlo wrote:
> > The AppleSMC contains two char[32] keys, OSK0 and OSK1, which are not
> > reported in the key count and index by default. These keys are used by
> > the OS X boot sequence, and normally don't matter when running Linux.
> > 
> > This patch creates a sysfs entry which reports the value of these keys
> > as an ASCII string, to help emulators (such as QEMU) load OS X when
> > running on genuine Apple hardware.
> > 
> > Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> > ---
> > 
> > For extra context: To boot OS X as a guest, QEMU must (among others)
> > emulate the AppleSMC. To boot successfully, OS X insists on querying
> > the (emulated) SMC for the value of OSK0 and OSK1. Currently, these
> > values must be supplied on the QEMU command line as
> > 
> >   -device applesmc,osk="...concatenated values of OSK0 and OSK1..."
> > 
> > With the availability of /sys/devices/platform/applesmc.768/osk, the
> > emulated QEMU AppleSMC could acquire this string directly from the
> > (Apple-manufactured) host machine.
> > 
> Hmm ... this is a non-hwmon attribute which doesn't really belong into hwmon
> in the first place ... like several other attributes in the same driver.
> 
> So I'll leave it up to the maintainer to decide if we should accept it. Henrik ?

Indeed, the reaons against this patch are too many. I was just about
to reply with the below:

Gabriel,

The OSK string seems constant accross machines, which renders the
patch rather pointless, no? And even if the OSK differs between a
couple of machines, the emulator could easily handle it gracefully.

There are also some technical issues with the patch below, to keep in
mind for future submissions.

>  drivers/hwmon/applesmc.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index b41baff..0c7cc71 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -1013,6 +1013,23 @@ static ssize_t applesmc_key_at_index_store(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t applesmc_osk_show(struct device *dev,
> +				struct device_attribute *attr, char *sysfsbuf)
> +{
> +	int fail;

All other functions use 'ret' here...

> +
> +	mutex_lock(&smcreg.mutex);
> +	fail = read_smc(APPLESMC_READ_CMD, "OSK0", sysfsbuf, 32) ||
> +	       read_smc(APPLESMC_READ_CMD, "OSK1", sysfsbuf + 32, 32);

The read function should propagate error messages, i.e., keep the
return values here. And please read to buffers instead.

> +	mutex_unlock(&smcreg.mutex);
> +	if (fail)
> +		return -1;

Return error here.

> +
> +	sysfsbuf[64] = '\n';
> +	sysfsbuf[65] = '\0';
> +	return 65;

A snprintf here, please.

> +}
> +
>  static struct led_classdev applesmc_backlight = {
>  	.name			= "smc::kbd_backlight",
>  	.default_trigger	= "nand-disk",
> @@ -1027,6 +1044,7 @@ static struct applesmc_node_group info_group[] = {
>  	{ "key_at_index_type", applesmc_key_at_index_type_show },
>  	{ "key_at_index_data_length", applesmc_key_at_index_data_length_show },
>  	{ "key_at_index_data", applesmc_key_at_index_read_show },
> +	{ "osk", applesmc_osk_show },

Unfortunately this is not a good place to put random things going
forward.

>  	{ }
>  };
>  
> -- 
> 1.7.7.6
> 

Given the above issues together with the weak rationale for the patch
in the first place, this patch will not be applied.

Thanks.
Henrik

Re: [PATCH] applesmc: add sysfs file to report OSK

[Alexander Graf]

On 10.12.2012, at 20:54, Henrik Rydberg wrote:

> Hi Guenter,
> 
>> On Mon, Dec 10, 2012 at 09:51:35AM -0500, Gabriel L. Somlo wrote:
>>> The AppleSMC contains two char[32] keys, OSK0 and OSK1, which are not
>>> reported in the key count and index by default. These keys are used by
>>> the OS X boot sequence, and normally don't matter when running Linux.
>>> 
>>> This patch creates a sysfs entry which reports the value of these keys
>>> as an ASCII string, to help emulators (such as QEMU) load OS X when
>>> running on genuine Apple hardware.
>>> 
>>> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
>>> ---
>>> 
>>> For extra context: To boot OS X as a guest, QEMU must (among others)
>>> emulate the AppleSMC. To boot successfully, OS X insists on querying
>>> the (emulated) SMC for the value of OSK0 and OSK1. Currently, these
>>> values must be supplied on the QEMU command line as
>>> 
>>>  -device applesmc,osk="...concatenated values of OSK0 and OSK1..."
>>> 
>>> With the availability of /sys/devices/platform/applesmc.768/osk, the
>>> emulated QEMU AppleSMC could acquire this string directly from the
>>> (Apple-manufactured) host machine.
>>> 
>> Hmm ... this is a non-hwmon attribute which doesn't really belong into hwmon
>> in the first place ... like several other attributes in the same driver.
>> 
>> So I'll leave it up to the maintainer to decide if we should accept it. Henrik ?
> 
> Indeed, the reaons against this patch are too many. I was just about
> to reply with the below:
> 
> Gabriel,
> 
> The OSK string seems constant accross machines, which renders the
> patch rather pointless, no? And even if the OSK differs between a
> couple of machines, the emulator could easily handle it gracefully.

The point is that the return value of the OSK is a copyrighted string, we can not include in any other layer. The only way to make this legally savvy is to read the key from the host.

> 
> There are also some technical issues with the patch below, to keep in
> mind for future submissions.

Sigh - most of the comments below go back to earlier review from me. He basically had a version almost exactly like what you're asking him to do :). Funny how code style taste differs.


Alex

> 
>> drivers/hwmon/applesmc.c |   18 ++++++++++++++++++
>> 1 files changed, 18 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
>> index b41baff..0c7cc71 100644
>> --- a/drivers/hwmon/applesmc.c
>> +++ b/drivers/hwmon/applesmc.c
>> @@ -1013,6 +1013,23 @@ static ssize_t applesmc_key_at_index_store(struct device *dev,
>> 	return count;
>> }
>> 
>> +static ssize_t applesmc_osk_show(struct device *dev,
>> +				struct device_attribute *attr, char *sysfsbuf)
>> +{
>> +	int fail;
> 
> All other functions use 'ret' here...
> 
>> +
>> +	mutex_lock(&smcreg.mutex);
>> +	fail = read_smc(APPLESMC_READ_CMD, "OSK0", sysfsbuf, 32) ||
>> +	       read_smc(APPLESMC_READ_CMD, "OSK1", sysfsbuf + 32, 32);
> 
> The read function should propagate error messages, i.e., keep the
> return values here. And please read to buffers instead.
> 
>> +	mutex_unlock(&smcreg.mutex);
>> +	if (fail)
>> +		return -1;
> 
> Return error here.
> 
>> +
>> +	sysfsbuf[64] = '\n';
>> +	sysfsbuf[65] = '\0';
>> +	return 65;
> 
> A snprintf here, please.
> 
>> +}
>> +
>> static struct led_classdev applesmc_backlight = {
>> 	.name			= "smc::kbd_backlight",
>> 	.default_trigger	= "nand-disk",
>> @@ -1027,6 +1044,7 @@ static struct applesmc_node_group info_group[] = {
>> 	{ "key_at_index_type", applesmc_key_at_index_type_show },
>> 	{ "key_at_index_data_length", applesmc_key_at_index_data_length_show },
>> 	{ "key_at_index_data", applesmc_key_at_index_read_show },
>> +	{ "osk", applesmc_osk_show },
> 
> Unfortunately this is not a good place to put random things going
> forward.
> 
>> 	{ }
>> };
>> 
>> -- 
>> 1.7.7.6
>> 
> 
> Given the above issues together with the weak rationale for the patch
> in the first place, this patch will not be applied.
> 
> Thanks.
> Henrik

Re: [PATCH] applesmc: add sysfs file to report OSK

[Rene Rebe]

On 10.12.2012, at 21:19, Alexander Graf <agraf@suse.de> wrote:

> 
> On 10.12.2012, at 20:54, Henrik Rydberg wrote:
> 
>> Hi Guenter,
>> 
>>> On Mon, Dec 10, 2012 at 09:51:35AM -0500, Gabriel L. Somlo wrote:
>>>> The AppleSMC contains two char[32] keys, OSK0 and OSK1, which are not
>>>> reported in the key count and index by default. These keys are used by
>>>> the OS X boot sequence, and normally don't matter when running Linux.
>>>> 
>>>> This patch creates a sysfs entry which reports the value of these keys
>>>> as an ASCII string, to help emulators (such as QEMU) load OS X when
>>>> running on genuine Apple hardware.
>>>> 
>>>> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
>>>> ---
>>>> 
>>>> For extra context: To boot OS X as a guest, QEMU must (among others)
>>>> emulate the AppleSMC. To boot successfully, OS X insists on querying
>>>> the (emulated) SMC for the value of OSK0 and OSK1. Currently, these
>>>> values must be supplied on the QEMU command line as
>>>> 
>>>> -device applesmc,osk="...concatenated values of OSK0 and OSK1..."
>>>> 
>>>> With the availability of /sys/devices/platform/applesmc.768/osk, the
>>>> emulated QEMU AppleSMC could acquire this string directly from the
>>>> (Apple-manufactured) host machine.
>>> Hmm ... this is a non-hwmon attribute which doesn't really belong into hwmon
>>> in the first place ... like several other attributes in the same driver.
>>> 
>>> So I'll leave it up to the maintainer to decide if we should accept it. Henrik ?
>> 
>> Indeed, the reaons against this patch are too many. I was just about
>> to reply with the below:
>> 
>> Gabriel,
>> 
>> The OSK string seems constant accross machines, which renders the
>> patch rather pointless, no? And even if the OSK differs between a
>> couple of machines, the emulator could easily handle it gracefully.
> 
> The point is that the return value of the OSK is a copyrighted string, we can not include in any other layer. The only way to make this legally savvy is to read the key from the host.
> 
>> 
>> There are also some technical issues with the patch below, to keep in
>> mind for future submissions.
> 
> Sigh - most of the comments below go back to earlier review from me. He basically had a version almost exactly like what you're asking him to do :). Funny how code style taste differs.

And this is exactly the reason why I'm less and less motivated to waste my lifetime with upstream work ...

> Alex
> 
>> 
>>> drivers/hwmon/applesmc.c |   18 ++++++++++++++++++
>>> 1 files changed, 18 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
>>> index b41baff..0c7cc71 100644
>>> --- a/drivers/hwmon/applesmc.c
>>> +++ b/drivers/hwmon/applesmc.c
>>> @@ -1013,6 +1013,23 @@ static ssize_t applesmc_key_at_index_store(struct device *dev,
>>>    return count;
>>> }
>>> 
>>> +static ssize_t applesmc_osk_show(struct device *dev,
>>> +                struct device_attribute *attr, char *sysfsbuf)
>>> +{
>>> +    int fail;
>> 
>> All other functions use 'ret' here...
>> 
>>> +
>>> +    mutex_lock(&smcreg.mutex);
>>> +    fail = read_smc(APPLESMC_READ_CMD, "OSK0", sysfsbuf, 32) ||
>>> +           read_smc(APPLESMC_READ_CMD, "OSK1", sysfsbuf + 32, 32);
>> 
>> The read function should propagate error messages, i.e., keep the
>> return values here. And please read to buffers instead.
>> 
>>> +    mutex_unlock(&smcreg.mutex);
>>> +    if (fail)
>>> +        return -1;
>> 
>> Return error here.
>> 
>>> +
>>> +    sysfsbuf[64] = '\n';
>>> +    sysfsbuf[65] = '\0';
>>> +    return 65;
>> 
>> A snprintf here, please.
>> 
>>> +}
>>> +
>>> static struct led_classdev applesmc_backlight = {
>>>    .name            = "smc::kbd_backlight",
>>>    .default_trigger    = "nand-disk",
>>> @@ -1027,6 +1044,7 @@ static struct applesmc_node_group info_group[] = {
>>>    { "key_at_index_type", applesmc_key_at_index_type_show },
>>>    { "key_at_index_data_length", applesmc_key_at_index_data_length_show },
>>>    { "key_at_index_data", applesmc_key_at_index_read_show },
>>> +    { "osk", applesmc_osk_show },
>> 
>> Unfortunately this is not a good place to put random things going
>> forward.
>> 
>>>    { }
>>> };
>>> 
>>> -- 
>>> 1.7.7.6
>> 
>> Given the above issues together with the weak rationale for the patch
>> in the first place, this patch will not be applied.
>> 
>> Thanks.
>> Henrik
> 

Re: [PATCH] applesmc: add sysfs file to report OSK

[Alexander Graf]

On 10.12.2012, at 21:43, Rene Rebe <rene@exactcode.com> wrote:

> 
> On 10.12.2012, at 21:19, Alexander Graf <agraf@suse.de> wrote:
> 
>> 
>> On 10.12.2012, at 20:54, Henrik Rydberg wrote:
>> 
>>> Hi Guenter,
>>> 
>>>> On Mon, Dec 10, 2012 at 09:51:35AM -0500, Gabriel L. Somlo wrote:
>>>>> The AppleSMC contains two char[32] keys, OSK0 and OSK1, which are not
>>>>> reported in the key count and index by default. These keys are used by
>>>>> the OS X boot sequence, and normally don't matter when running Linux.
>>>>> 
>>>>> This patch creates a sysfs entry which reports the value of these keys
>>>>> as an ASCII string, to help emulators (such as QEMU) load OS X when
>>>>> running on genuine Apple hardware.
>>>>> 
>>>>> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
>>>>> ---
>>>>> 
>>>>> For extra context: To boot OS X as a guest, QEMU must (among others)
>>>>> emulate the AppleSMC. To boot successfully, OS X insists on querying
>>>>> the (emulated) SMC for the value of OSK0 and OSK1. Currently, these
>>>>> values must be supplied on the QEMU command line as
>>>>> 
>>>>> -device applesmc,osk="...concatenated values of OSK0 and OSK1..."
>>>>> 
>>>>> With the availability of /sys/devices/platform/applesmc.768/osk, the
>>>>> emulated QEMU AppleSMC could acquire this string directly from the
>>>>> (Apple-manufactured) host machine.
>>>> Hmm ... this is a non-hwmon attribute which doesn't really belong into hwmon
>>>> in the first place ... like several other attributes in the same driver.
>>>> 
>>>> So I'll leave it up to the maintainer to decide if we should accept it. Henrik ?
>>> 
>>> Indeed, the reaons against this patch are too many. I was just about
>>> to reply with the below:
>>> 
>>> Gabriel,
>>> 
>>> The OSK string seems constant accross machines, which renders the
>>> patch rather pointless, no? And even if the OSK differs between a
>>> couple of machines, the emulator could easily handle it gracefully.
>> 
>> The point is that the return value of the OSK is a copyrighted string, we can not include in any other layer. The only way to make this legally savvy is to read the key from the host.
>> 
>>> 
>>> There are also some technical issues with the patch below, to keep in
>>> mind for future submissions.
>> 
>> Sigh - most of the comments below go back to earlier review from me. He basically had a version almost exactly like what you're asking him to do :). Funny how code style taste differs.
> 
> And this is exactly the reason why I'm less and less motivated to waste my lifetime with upstream work ...

It's nit that bad really. Gabriel can just send his earlier version again with the ret variable name changed and then it shouldn't be an issue to get it in.

Reading the key really is an important bit in creating a legally safe and easy method to virtualize Mac OS X on Linux. Just believe us on this one :).


Alex

Re: [PATCH] applesmc: add sysfs file to report OSK

[Rene Rebe]

On 10.12.2012, at 22:24, Alexander Graf wrote:

> On 10.12.2012, at 21:43, Rene Rebe <rene@exactcode.com> wrote:
> 
>> On 10.12.2012, at 21:19, Alexander Graf <agraf@suse.de> wrote:
>>> 
>>> On 10.12.2012, at 20:54, Henrik Rydberg wrote:
>>> 
>>>> Hi Guenter,
>>>> 
>>>>> On Mon, Dec 10, 2012 at 09:51:35AM -0500, Gabriel L. Somlo wrote:
>>>>>> The AppleSMC contains two char[32] keys, OSK0 and OSK1, which are not
>>>>>> reported in the key count and index by default. These keys are used by
>>>>>> the OS X boot sequence, and normally don't matter when running Linux.
>>>>>> 
>>>>>> This patch creates a sysfs entry which reports the value of these keys
>>>>>> as an ASCII string, to help emulators (such as QEMU) load OS X when
>>>>>> running on genuine Apple hardware.
>>>>>> 
>>>>>> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
>>>>>> ---
>>>>>> 
>>>>>> For extra context: To boot OS X as a guest, QEMU must (among others)
>>>>>> emulate the AppleSMC. To boot successfully, OS X insists on querying
>>>>>> the (emulated) SMC for the value of OSK0 and OSK1. Currently, these
>>>>>> values must be supplied on the QEMU command line as
>>>>>> 
>>>>>> -device applesmc,osk="...concatenated values of OSK0 and OSK1..."
>>>>>> 
>>>>>> With the availability of /sys/devices/platform/applesmc.768/osk, the
>>>>>> emulated QEMU AppleSMC could acquire this string directly from the
>>>>>> (Apple-manufactured) host machine.
>>>>> Hmm ... this is a non-hwmon attribute which doesn't really belong into hwmon
>>>>> in the first place ... like several other attributes in the same driver.
>>>>> 
>>>>> So I'll leave it up to the maintainer to decide if we should accept it. Henrik ?
>>>> 
>>>> Indeed, the reaons against this patch are too many. I was just about
>>>> to reply with the below:
>>>> 
>>>> Gabriel,
>>>> 
>>>> The OSK string seems constant accross machines, which renders the
>>>> patch rather pointless, no? And even if the OSK differs between a
>>>> couple of machines, the emulator could easily handle it gracefully.
>>> 
>>> The point is that the return value of the OSK is a copyrighted string, we can not include in any other layer. The only way to make this legally savvy is to read the key from the host.
>>> 
>>>> 
>>>> There are also some technical issues with the patch below, to keep in
>>>> mind for future submissions.
>>> 
>>> Sigh - most of the comments below go back to earlier review from me. He basically had a version almost exactly like what you're asking him to do :). Funny how code style taste differs.
>> 
>> And this is exactly the reason why I'm less and less motivated to waste my lifetime with upstream work ...
> 
> It's nit that bad really. Gabriel can just send his earlier version again with the ret variable name changed and then it shouldn't be an issue to get it in.
> 
> Reading the key really is an important bit in creating a legally safe and easy method to virtualize Mac OS X on Linux. Just believe us on this one :).

Sure, you do not need to convince me about that ;-)

-- 
  René Rebe, ExactCODE GmbH, Jaegerstr. 67, DE-10117 Berlin
  http://exactcode.com | http://t2-project.org | http://rene.rebe.de

[PATCH v2] applesmc: add sysfs file to report OSK

[Gabriel L. Somlo]

The AppleSMC contains two char[32] keys, OSK0 and OSK1, which are not
reported in the key count and index by default. These keys are used by
the OS X boot sequence, and normally don't matter when running Linux.

This patch creates a sysfs entry which reports the value of these keys
as an ASCII string, to help emulators (such as QEMU) load OS X when
running on genuine Apple hardware.

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

Henrik,

On Mon, Dec 10, 2012 at 09:19:51PM +0100, Alexander Graf wrote:
>
> On Mon, Dec 10, 2012 at 08:54:14PM +0100, Henrik Rydberg wrote:
> >
> > The OSK string seems constant accross machines, which renders the
> > patch rather pointless, no? And even if the OSK differs between a
> > couple of machines, the emulator could easily handle it gracefully.
>
> The point is that the return value of the OSK is a copyrighted string,
> we can not include in any other layer. The only way to make this
> legally savvy is to   read the key from the host.

I believe this second version addresses your stylistic objections.

As Alex pointed out, hardcoding the OSK in any open-source project
might lead to legal/copyright related issues, whereas simply reporting
whatever the the hardware happens to contain should keep everyone safe.

As for struct applesmc_node_group info_group[]:

> > Unfortunately this is not a good place to put random things going
> > forward.

I figured "info_group" for the "least bad" place to put the OSK, given
that it's more or less "information from/about the SMC". If you could be
persuaded by the above arguments to accept the patch, and have a better
place in mind, I'd be happy to move it there...

Thanks again,
 Gabriel

 drivers/hwmon/applesmc.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index b41baff..b37b271 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -1013,6 +1013,24 @@ static ssize_t applesmc_key_at_index_store(struct device *dev,
 	return count;
 }
 
+static ssize_t applesmc_osk_show(struct device *dev,
+				struct device_attribute *attr, char *sysfsbuf)
+{
+	int ret;
+	char buf[65];
+
+	mutex_lock(&smcreg.mutex);
+	ret = read_smc(APPLESMC_READ_CMD, "OSK0", buf, 32);
+	if (!ret)
+		ret = read_smc(APPLESMC_READ_CMD, "OSK1", buf + 32, 32);
+	mutex_unlock(&smcreg.mutex);
+	if (ret)
+		return ret;
+
+	buf[64] = '\0';
+	return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", buf);
+}
+
 static struct led_classdev applesmc_backlight = {
 	.name			= "smc::kbd_backlight",
 	.default_trigger	= "nand-disk",
@@ -1027,6 +1045,7 @@ static struct applesmc_node_group info_group[] = {
 	{ "key_at_index_type", applesmc_key_at_index_type_show },
 	{ "key_at_index_data_length", applesmc_key_at_index_data_length_show },
 	{ "key_at_index_data", applesmc_key_at_index_read_show },
+	{ "osk", applesmc_osk_show },
 	{ }
 };
 
-- 
1.7.7.6

Re: [PATCH v2] applesmc: add sysfs file to report OSK

[Gabriel L. Somlo]

Henrik, I agree with you that it's silly to poke the hardware for a
string we KNOW will NEVER change :)

That being said, I too would love to live in a world where technical
common sense prevailed over bullshit lawyerly tactics, "corporate
strategies", and the ensuing FUD.

The way things stand today, in order to boot Mac OS X on QEMU/KVM/Linux
(running on Mac hardware of course, in compliance with Apple's license)
requires the end user supplying the OSK on the QEMU command line via the
-osk="..." argument.

I would like to make it easy for people who run Linux natively on Apple
hardware (of which I am one) to run OS X from QEMU "out of the box".

I could try to hardcode the OSK inside QEMU, or I could try to include
it as a default config file entry, but I'm quite certain the QEMU project
would be uncomfortable "distributing" a string on which Apple claims
copyright. Even if that happened, distros might then balk at shipping
QEMU as part of their package repositories, for the exact same reason.

Currently, we're all beating around the bush about the OSK, with vague
hints and hoops to jump through for end users to acquire the OSK and
pass it to QEMU on the command line. This process will have the
unfortunate side effect of filtering out "normal" users :(

The only viable (from a legal CYA standpoint) thing I can think of is
to make it easy to acquire the OSK automatically, on demand, directly
from the hardware. Right now, the logical place for that is applesmc.ko.
It already controls access to the SMC, and already reports values for
various keys.

Yes it may feel silly to "go through the motions" when you KNOW it's
always going to be the same value that gets returned. But this patch
is mostly about reassuring people that lawyers will continue not to
have a reason to get involved...

Thanks for reading, and hope I'm being sufficiently persuasive :)
--Gabriel

>  drivers/hwmon/applesmc.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index b41baff..b37b271 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -1013,6 +1013,24 @@ static ssize_t applesmc_key_at_index_store(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t applesmc_osk_show(struct device *dev,
> +				struct device_attribute *attr, char *sysfsbuf)
> +{
> +	int ret;
> +	char buf[65];
> +
> +	mutex_lock(&smcreg.mutex);
> +	ret = read_smc(APPLESMC_READ_CMD, "OSK0", buf, 32);
> +	if (!ret)
> +		ret = read_smc(APPLESMC_READ_CMD, "OSK1", buf + 32, 32);
> +	mutex_unlock(&smcreg.mutex);
> +	if (ret)
> +		return ret;
> +
> +	buf[64] = '\0';
> +	return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", buf);
> +}
> +
>  static struct led_classdev applesmc_backlight = {
>  	.name			= "smc::kbd_backlight",
>  	.default_trigger	= "nand-disk",
> @@ -1027,6 +1045,7 @@ static struct applesmc_node_group info_group[] = {
>  	{ "key_at_index_type", applesmc_key_at_index_type_show },
>  	{ "key_at_index_data_length", applesmc_key_at_index_data_length_show },
>  	{ "key_at_index_data", applesmc_key_at_index_read_show },
> +	{ "osk", applesmc_osk_show },
>  	{ }
>  };
>  
> -- 
> 1.7.7.6

Re: [PATCH v2] applesmc: add sysfs file to report OSK

[Henrik Rydberg]

Hi Gabriel,

> I could try to hardcode the OSK inside QEMU, or I could try to include
> it as a default config file entry, but I'm quite certain the QEMU project
> would be uncomfortable "distributing" a string on which Apple claims
> copyright. Even if that happened, distros might then balk at shipping
> QEMU as part of their package repositories, for the exact same reason.

I understand this is frustrating, and I believe you have made a good
case for the rationale. However, the technical issues remain. Also,
there might still be other, simpler, solutions.

> The only viable (from a legal CYA standpoint) thing I can think of is
> to make it easy to acquire the OSK automatically, on demand, directly
> from the hardware. Right now, the logical place for that is applesmc.ko.
> It already controls access to the SMC, and already reports values for
> various keys.

How about encrypting the string with a key only found on an Apple
computer? There are strings available in both ACPI and EFI that could
serve such a purpose.

Regarding the patch, I agree with Guenter that putting more unrelated
things into the hwmon subsystem makes no sense. Most of the
information in applesmc should go into the hwmon, thermal, backlight
and input subsystems, but some strings should go somewhere else (maybe
/sys/firmware/smc/?). The reluctance you experience here is a
technical one; someone will need to make an effort to create a good
place for your string, and it does not help that the string is, in
fact, a constant. :-)

So, don't give up hope, but please do not expect an immediate solution.

Thanks.
Henrik

Re: [PATCH v2] applesmc: add sysfs file to report OSK

[Gabriel L. Somlo]

On Thu, Dec 13, 2012 at 08:20:11AM +0100, Henrik Rydberg wrote:
> > The only viable (from a legal CYA standpoint) thing I can think of is
> > to make it easy to acquire the OSK automatically, on demand, directly
> > from the hardware. Right now, the logical place for that is applesmc.ko.
> > It already controls access to the SMC, and already reports values for
> > various keys.
> 
> How about encrypting the string with a key only found on an Apple
> computer? There are strings available in both ACPI and EFI that could
> serve such a purpose.

I'd still be distributing the copyrighted string. I don't expect
encryption to placate the proverbial lawyers, nor the project/distro
maintainers who want to avoid them :)

Plus, now I'd also be taking on the responsibility of deciding when
it's (legally) OK to let QEMU boot OS X and when it isn't.

Besides, the most obvious Apple-only key that fits your description
is the OSK itself ! :)

> Regarding the patch, I agree with Guenter that putting more unrelated
> things into the hwmon subsystem makes no sense. Most of the
> information in applesmc should go into the hwmon, thermal, backlight
> and input subsystems, but some strings should go somewhere else (maybe
> /sys/firmware/smc/?). The reluctance you experience here is a
> technical one; someone will need to make an effort to create a good
> place for your string, and it does not help that the string is, in
> fact, a constant. :-)
> 
> So, don't give up hope, but please do not expect an immediate solution.

OK, so if we agree that for legal-CYA reasons we must pretend to
ignore the constant-ness of the key value, are we now talking about
simply where in the sysfs hierarchy to place the node that will
spit it out ?

Right now it's under /sys/devices/platform/applesmc.768/
(i.e., it's a device, it's platform specific, and it's name is
applesmc-something).

The only ties to "hwmon" are that:
	- there's a "/sys/class/hwmon/hwmonX" symlink to the above
	  directory (among others, such as the graphics card and two
	  instances of "coretemp", at least on my MacPro5,1);
	- *most* of the files in it are indeed reporting sensor
	  readings;
	- the applesmc.c device driver source file itself lives under
	  drivers/hwmon/ in the kernel source tree.

Is it common practice to split off a subset of the values reported
by such a chip and make them show up elsehere (i.e. besides the
/sys/devices/platform/<chipname> folder) ? Does the relative size
of the subsets involved count as input into that decision proces ? :)

I personally don't know enough to have an opinion on where the best
place for this might be. You suggested "/sys/firmware/smc/", and I'd
be perfectly happy with that one too. Do you know who would decide
whether that's an acceptable alternative to /sys/devices/platform/applesmc ?

Thanks again,
--Gabriel