| Message ID | 7b632baafbfdc6c55c4d56225c54fd441b747286.1506571188.git.mario.limonciello@dell.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Thursday 28 September 2017 06:02:14 Mario Limonciello wrote: > The driver currently uses an SMI interface which grants direct access > to physical memory to the firmware SMM methods via a pointer. > > Now add a WMI-ACPI interface that is detected by WMI probe and preferred > over the SMI interface. > > Changing this to operate over WMI-ACPI will use an ACPI OperationRegion > for a buffer of data storage when SMM calls are performed. > > This is a safer approach to use in kernel drivers as the SMM will > only have access to that OperationRegion. > > As a result, this change removes the dependency on this driver on the > dcdbas kernel module. It's now an optional compilation option. > > When modifying this, add myself to MAINTAINERS. > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > --- > MAINTAINERS | 6 ++ > drivers/platform/x86/Kconfig | 14 ++-- > drivers/platform/x86/dell-smbios.c | 128 ++++++++++++++++++++++++++++++++----- > drivers/platform/x86/dell-smbios.h | 15 ++++- > 4 files changed, 138 insertions(+), 25 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 08b96f77f618..6d76b09f46cc 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3990,6 +3990,12 @@ S: Maintained > F: drivers/hwmon/dell-smm-hwmon.c > F: include/uapi/linux/i8k.h > > +DELL SMBIOS DRIVER > +M: Pali Rohár <pali.rohar@gmail.com> > +M: Mario Limonciello <mario.limonciello@dell.com> > +S: Maintained > +F: drivers/platform/x86/dell-smbios.* > + > DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas) > M: Doug Warzecha <Douglas_Warzecha@dell.com> > S: Maintained > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 1f7959ff055c..415886d7a857 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -92,13 +92,17 @@ config ASUS_LAPTOP > If you have an ACPI-compatible ASUS laptop, say Y or M here. > > config DELL_SMBIOS > - tristate > - select DCDBAS > + tristate "Dell SMBIOS calling interface" > + depends on ACPI_WMI > ---help--- > - This module provides common functions for kernel modules using > - Dell SMBIOS. > + This module provides common functions for kernel modules and > + userspace using Dell SMBIOS. > + > + If you have a Dell computer, say Y or M here. > > - If you have a Dell laptop, say Y or M here. > + If you have a machine from < 2007 without a WMI interface you > + may also want to enable CONFIG_DCDBAS to allow this driver to > + work. > > config DELL_LAPTOP > tristate "Dell Laptop Extras" > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c > index e9b1ca07c872..4472817ee045 100644 > --- a/drivers/platform/x86/dell-smbios.c > +++ b/drivers/platform/x86/dell-smbios.c > @@ -4,6 +4,7 @@ > * Copyright (c) Red Hat <mjg@redhat.com> > * Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com> > * Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com> > + * Copyright (c) 2017 Dell Inc. > * > * Based on documentation in the libsmbios package: > * Copyright (C) 2005-2014 Dell Inc. > @@ -22,9 +23,15 @@ > #include <linux/mutex.h> > #include <linux/slab.h> > #include <linux/io.h> > -#include "../../firmware/dcdbas.h" > +#include <linux/wmi.h> > #include "dell-smbios.h" > > +#ifdef CONFIG_DCDBAS > +#include "../../firmware/dcdbas.h" > +#endif > + > +#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492" > + > struct calling_interface_structure { > struct dmi_header header; > u16 cmdIOAddress; > @@ -33,12 +40,14 @@ struct calling_interface_structure { > struct calling_interface_token tokens[]; > } __packed; > > -static struct calling_interface_buffer *buffer; > +static void *buffer; > +static size_t bufferlen; > static DEFINE_MUTEX(buffer_mutex); > > static int da_command_address; > static int da_command_code; > static int da_num_tokens; > +struct wmi_device *wmi_dev; > static struct calling_interface_token *da_tokens; > > int dell_smbios_error(int value) > @@ -60,13 +69,15 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void) > { > mutex_lock(&buffer_mutex); > dell_smbios_clear_buffer(); > + if (wmi_dev) > + return &((struct wmi_calling_interface_buffer *) buffer)->smi; > return buffer; > } > EXPORT_SYMBOL_GPL(dell_smbios_get_buffer); > > void dell_smbios_clear_buffer(void) > { > - memset(buffer, 0, sizeof(struct calling_interface_buffer)); > + memset(buffer, 0, bufferlen); > } > EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer); > > @@ -76,7 +87,36 @@ void dell_smbios_release_buffer(void) > } > EXPORT_SYMBOL_GPL(dell_smbios_release_buffer); > > -void dell_smbios_send_request(int class, int select) > +static int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf) > +{ > + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; > + struct acpi_buffer input; > + union acpi_object *obj; > + acpi_status status; > + > + input.length = sizeof(struct wmi_calling_interface_buffer); > + input.pointer = buf; > + > + status = wmidev_evaluate_method(wmi_dev, 0, 1, &input, &output); > + if (ACPI_FAILURE(status)) { > + pr_err("%x/%x [%x,%x,%x,%x] call failed\n", > + buf->smi.class, buf->smi.select, > + buf->smi.input[0], buf->smi.input[1], > + buf->smi.input[2], buf->smi.input[3]); > + return -EIO; > + } > + obj = (union acpi_object *)output.pointer; > + if (obj->type != ACPI_TYPE_BUFFER) { > + pr_err("invalid type : %d\n", obj->type); > + return -EIO; > + } > + memcpy(buf, obj->buffer.pointer, input.length); > + > + return 0; > +} > + > +#ifdef CONFIG_DCDBAS > +static void run_smi_smbios_call(struct calling_interface_buffer *buf) > { > struct smi_cmd command; > > @@ -85,12 +125,28 @@ void dell_smbios_send_request(int class, int select) > command.command_code = da_command_code; > command.ebx = virt_to_phys(buffer); > command.ecx = 0x42534931; > - > - buffer->class = class; > - buffer->select = select; > - > dcdbas_smi_request(&command); > } > +#else > +static void run_smi_smbios_call(struct calling_interface_buffer *buf) {} > +#endif /* CONFIG_DCDBAS */ > + > +void dell_smbios_send_request(int class, int select) > +{ > + if (wmi_dev) { > + struct wmi_calling_interface_buffer *buf = buffer; > + > + buf->smi.class = class; > + buf->smi.select = select; > + run_wmi_smbios_call(buf); > + } else { > + struct calling_interface_buffer *buf = buffer; > + > + buf->class = class; > + buf->select = select; > + run_smi_smbios_call(buf); > + } > +} > EXPORT_SYMBOL_GPL(dell_smbios_send_request); > > struct calling_interface_token *dell_smbios_find_token(int tokenid) > @@ -170,10 +226,43 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy) > } > } > > -static int __init dell_smbios_init(void) > +static int dell_smbios_wmi_probe(struct wmi_device *wdev) > +{ > + /* no longer need the SMI page */ > + free_page((unsigned long)buffer); > + > + /* WMI buffer should be 32k */ > + buffer = (void *)__get_free_pages(GFP_KERNEL, 3); > + if (!buffer) > + return -ENOMEM; > + bufferlen = sizeof(struct wmi_calling_interface_buffer); > + wmi_dev = wdev; > + return 0; > +} > + > +static int dell_smbios_wmi_remove(struct wmi_device *wdev) > { > - int ret; > + wmi_dev = NULL; > + free_pages((unsigned long)buffer, 3); > + return 0; > +} This code does not seem to be safe. dell_smbios_wmi_probe and dell_smbios_wmi_remove are called at any time when kernel register new device which matches some properties OR when user manually bind this driver to that device. buffer and wmi_dev is shared as a global variable which means that when there are two devices which want to bind to this driver, kernel would get double free at removing time. Devices from the bus subsystem should not use global shared variables, but rather allocate own memory... Also note that user can at any time unbound device from driver and also can bind it again. > +static const struct wmi_device_id dell_smbios_wmi_id_table[] = { > + { .guid_string = DELL_WMI_SMBIOS_GUID }, > + { }, > +}; > + > +static struct wmi_driver dell_wmi_smbios_driver = { > + .driver = { > + .name = "dell-smbios", > + }, > + .probe = dell_smbios_wmi_probe, > + .remove = dell_smbios_wmi_remove, > + .id_table = dell_smbios_wmi_id_table, > +}; > + > +static int __init dell_smbios_init(void) > +{ > dmi_walk(find_tokens, NULL); > > if (!da_tokens) { > @@ -181,34 +270,39 @@ static int __init dell_smbios_init(void) > return -ENODEV; > } > > +#ifdef CONFIG_DCDBAS > /* > * Allocate buffer below 4GB for SMI data--only 32-bit physical addr > * is passed to SMI handler. > */ > buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32); > + bufferlen = sizeof(struct calling_interface_buffer); > +#else > + buffer = NULL; > +#endif /* CONFIG_DCDBAS */ > + wmi_driver_register(&dell_wmi_smbios_driver); > + > if (!buffer) { > - ret = -ENOMEM; > - goto fail_buffer; > + kfree(da_tokens); > + return -ENOMEM; > } > - > return 0; > - > -fail_buffer: > - kfree(da_tokens); > - return ret; > } > > static void __exit dell_smbios_exit(void) > { > kfree(da_tokens); > free_page((unsigned long)buffer); > + wmi_driver_unregister(&dell_wmi_smbios_driver); > } > > subsys_initcall(dell_smbios_init); > module_exit(dell_smbios_exit); > > + > MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>"); > MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>"); > MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>"); > +MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>"); > MODULE_DESCRIPTION("Common functions for kernel modules using Dell SMBIOS"); > MODULE_LICENSE("GPL"); > diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h > index 45cbc2292cd3..2f6fce81ee69 100644 > --- a/drivers/platform/x86/dell-smbios.h > +++ b/drivers/platform/x86/dell-smbios.h > @@ -4,6 +4,7 @@ > * Copyright (c) Red Hat <mjg@redhat.com> > * Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com> > * Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com> > + * Copyright (c) 2017 Dell Inc. > * > * Based on documentation in the libsmbios package: > * Copyright (C) 2005-2014 Dell Inc. > @@ -18,9 +19,10 @@ > > struct notifier_block; > > -/* This structure will be modified by the firmware when we enter > - * system management mode, hence the volatiles */ > - > +/* If called through fallback SMI rather than WMI this structure will be > + * modified by the firmware when we enter system management mode, hence the > + * volatiles > + */ > struct calling_interface_buffer { > u16 class; > u16 select; > @@ -28,6 +30,13 @@ struct calling_interface_buffer { > volatile u32 output[4]; > } __packed; > > +struct wmi_calling_interface_buffer { > + struct calling_interface_buffer smi; > + u32 argattrib; > + u32 blength; > + u8 data[32724]; > +} __packed; > + > struct calling_interface_token { > u16 tokenID; > u16 location; >
> -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Thursday, September 28, 2017 2:54 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; Andy > Lutomirski <luto@kernel.org>; quasisec@google.com > Subject: Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI > interface > > On Thursday 28 September 2017 06:02:14 Mario Limonciello wrote: > > The driver currently uses an SMI interface which grants direct access > > to physical memory to the firmware SMM methods via a pointer. > > > > Now add a WMI-ACPI interface that is detected by WMI probe and preferred > > over the SMI interface. > > > > Changing this to operate over WMI-ACPI will use an ACPI OperationRegion > > for a buffer of data storage when SMM calls are performed. > > > > This is a safer approach to use in kernel drivers as the SMM will > > only have access to that OperationRegion. > > > > As a result, this change removes the dependency on this driver on the > > dcdbas kernel module. It's now an optional compilation option. > > > > When modifying this, add myself to MAINTAINERS. > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > > --- > > MAINTAINERS | 6 ++ > > drivers/platform/x86/Kconfig | 14 ++-- > > drivers/platform/x86/dell-smbios.c | 128 ++++++++++++++++++++++++++++++++- > ---- > > drivers/platform/x86/dell-smbios.h | 15 ++++- > > 4 files changed, 138 insertions(+), 25 deletions(-) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 08b96f77f618..6d76b09f46cc 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3990,6 +3990,12 @@ S: Maintained > > F: drivers/hwmon/dell-smm-hwmon.c > > F: include/uapi/linux/i8k.h > > > > +DELL SMBIOS DRIVER > > +M: Pali Rohár <pali.rohar@gmail.com> > > +M: Mario Limonciello <mario.limonciello@dell.com> > > +S: Maintained > > +F: drivers/platform/x86/dell-smbios.* > > + > > DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas) > > M: Doug Warzecha <Douglas_Warzecha@dell.com> > > S: Maintained > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > > index 1f7959ff055c..415886d7a857 100644 > > --- a/drivers/platform/x86/Kconfig > > +++ b/drivers/platform/x86/Kconfig > > @@ -92,13 +92,17 @@ config ASUS_LAPTOP > > If you have an ACPI-compatible ASUS laptop, say Y or M here. > > > > config DELL_SMBIOS > > - tristate > > - select DCDBAS > > + tristate "Dell SMBIOS calling interface" > > + depends on ACPI_WMI > > ---help--- > > - This module provides common functions for kernel modules using > > - Dell SMBIOS. > > + This module provides common functions for kernel modules and > > + userspace using Dell SMBIOS. > > + > > + If you have a Dell computer, say Y or M here. > > > > - If you have a Dell laptop, say Y or M here. > > + If you have a machine from < 2007 without a WMI interface you > > + may also want to enable CONFIG_DCDBAS to allow this driver to > > + work. > > > > config DELL_LAPTOP > > tristate "Dell Laptop Extras" > > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell- > smbios.c > > index e9b1ca07c872..4472817ee045 100644 > > --- a/drivers/platform/x86/dell-smbios.c > > +++ b/drivers/platform/x86/dell-smbios.c > > @@ -4,6 +4,7 @@ > > * Copyright (c) Red Hat <mjg@redhat.com> > > * Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com> > > * Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com> > > + * Copyright (c) 2017 Dell Inc. > > * > > * Based on documentation in the libsmbios package: > > * Copyright (C) 2005-2014 Dell Inc. > > @@ -22,9 +23,15 @@ > > #include <linux/mutex.h> > > #include <linux/slab.h> > > #include <linux/io.h> > > -#include "../../firmware/dcdbas.h" > > +#include <linux/wmi.h> > > #include "dell-smbios.h" > > > > +#ifdef CONFIG_DCDBAS > > +#include "../../firmware/dcdbas.h" > > +#endif > > + > > +#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012- > B622A1EF5492" > > + > > struct calling_interface_structure { > > struct dmi_header header; > > u16 cmdIOAddress; > > @@ -33,12 +40,14 @@ struct calling_interface_structure { > > struct calling_interface_token tokens[]; > > } __packed; > > > > -static struct calling_interface_buffer *buffer; > > +static void *buffer; > > +static size_t bufferlen; > > static DEFINE_MUTEX(buffer_mutex); > > > > static int da_command_address; > > static int da_command_code; > > static int da_num_tokens; > > +struct wmi_device *wmi_dev; > > static struct calling_interface_token *da_tokens; > > > > int dell_smbios_error(int value) > > @@ -60,13 +69,15 @@ struct calling_interface_buffer > *dell_smbios_get_buffer(void) > > { > > mutex_lock(&buffer_mutex); > > dell_smbios_clear_buffer(); > > + if (wmi_dev) > > + return &((struct wmi_calling_interface_buffer *) buffer)->smi; > > return buffer; > > } > > EXPORT_SYMBOL_GPL(dell_smbios_get_buffer); > > > > void dell_smbios_clear_buffer(void) > > { > > - memset(buffer, 0, sizeof(struct calling_interface_buffer)); > > + memset(buffer, 0, bufferlen); > > } > > EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer); > > > > @@ -76,7 +87,36 @@ void dell_smbios_release_buffer(void) > > } > > EXPORT_SYMBOL_GPL(dell_smbios_release_buffer); > > > > -void dell_smbios_send_request(int class, int select) > > +static int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf) > > +{ > > + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; > > + struct acpi_buffer input; > > + union acpi_object *obj; > > + acpi_status status; > > + > > + input.length = sizeof(struct wmi_calling_interface_buffer); > > + input.pointer = buf; > > + > > + status = wmidev_evaluate_method(wmi_dev, 0, 1, &input, &output); > > + if (ACPI_FAILURE(status)) { > > + pr_err("%x/%x [%x,%x,%x,%x] call failed\n", > > + buf->smi.class, buf->smi.select, > > + buf->smi.input[0], buf->smi.input[1], > > + buf->smi.input[2], buf->smi.input[3]); > > + return -EIO; > > + } > > + obj = (union acpi_object *)output.pointer; > > + if (obj->type != ACPI_TYPE_BUFFER) { > > + pr_err("invalid type : %d\n", obj->type); > > + return -EIO; > > + } > > + memcpy(buf, obj->buffer.pointer, input.length); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_DCDBAS > > +static void run_smi_smbios_call(struct calling_interface_buffer *buf) > > { > > struct smi_cmd command; > > > > @@ -85,12 +125,28 @@ void dell_smbios_send_request(int class, int select) > > command.command_code = da_command_code; > > command.ebx = virt_to_phys(buffer); > > command.ecx = 0x42534931; > > - > > - buffer->class = class; > > - buffer->select = select; > > - > > dcdbas_smi_request(&command); > > } > > +#else > > +static void run_smi_smbios_call(struct calling_interface_buffer *buf) {} > > +#endif /* CONFIG_DCDBAS */ > > + > > +void dell_smbios_send_request(int class, int select) > > +{ > > + if (wmi_dev) { > > + struct wmi_calling_interface_buffer *buf = buffer; > > + > > + buf->smi.class = class; > > + buf->smi.select = select; > > + run_wmi_smbios_call(buf); > > + } else { > > + struct calling_interface_buffer *buf = buffer; > > + > > + buf->class = class; > > + buf->select = select; > > + run_smi_smbios_call(buf); > > + } > > +} > > EXPORT_SYMBOL_GPL(dell_smbios_send_request); > > > > struct calling_interface_token *dell_smbios_find_token(int tokenid) > > @@ -170,10 +226,43 @@ static void __init find_tokens(const struct dmi_header > *dm, void *dummy) > > } > > } > > > > -static int __init dell_smbios_init(void) > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev) > > +{ > > + /* no longer need the SMI page */ > > + free_page((unsigned long)buffer); > > + > > + /* WMI buffer should be 32k */ > > + buffer = (void *)__get_free_pages(GFP_KERNEL, 3); > > + if (!buffer) > > + return -ENOMEM; > > + bufferlen = sizeof(struct wmi_calling_interface_buffer); > > + wmi_dev = wdev; > > + return 0; > > +} > > + > > +static int dell_smbios_wmi_remove(struct wmi_device *wdev) > > { > > - int ret; > > + wmi_dev = NULL; > > + free_pages((unsigned long)buffer, 3); > > + return 0; > > +} > > This code does not seem to be safe. dell_smbios_wmi_probe and > dell_smbios_wmi_remove are called at any time when kernel register new > device which matches some properties OR when user manually bind this > driver to that device. > I'll adjust for the assumptions I made about it only happening at module init. > buffer and wmi_dev is shared as a global variable which means that when > there are two devices which want to bind to this driver, kernel would > get double free at removing time. But there is only one GUID in id_table. How can two devices bind to this? This should be an impossible scenario. > > Devices from the bus subsystem should not use global shared variables, > but rather allocate own memory... Part of the problem is that this module can operate in two modes now. I think there will have to be global shared variables when operating in SMI mode. Or maybe put those bits into a platform_device for SMI mode usage? The problem I think then becomes how do you handle dell_smbios_send_request? Without global shared memory for the module the dell-laptop and dell-wmi modules won't be able to use this. > > Also note that user can at any time unbound device from driver and also > can bind it again. I'll make some adjustments to account for this. > > > +static const struct wmi_device_id dell_smbios_wmi_id_table[] = { > > + { .guid_string = DELL_WMI_SMBIOS_GUID }, > > + { }, > > +}; > > + > > +static struct wmi_driver dell_wmi_smbios_driver = { > > + .driver = { > > + .name = "dell-smbios", > > + }, > > + .probe = dell_smbios_wmi_probe, > > + .remove = dell_smbios_wmi_remove, > > + .id_table = dell_smbios_wmi_id_table, > > +}; > > + > > +static int __init dell_smbios_init(void) > > +{ > > dmi_walk(find_tokens, NULL); > > > > if (!da_tokens) { > > @@ -181,34 +270,39 @@ static int __init dell_smbios_init(void) > > return -ENODEV; > > } > > > > +#ifdef CONFIG_DCDBAS > > /* > > * Allocate buffer below 4GB for SMI data--only 32-bit physical addr > > * is passed to SMI handler. > > */ > > buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32); > > + bufferlen = sizeof(struct calling_interface_buffer); > > +#else > > + buffer = NULL; > > +#endif /* CONFIG_DCDBAS */ > > + wmi_driver_register(&dell_wmi_smbios_driver); > > + > > if (!buffer) { > > - ret = -ENOMEM; > > - goto fail_buffer; > > + kfree(da_tokens); > > + return -ENOMEM; > > } > > - > > return 0; > > - > > -fail_buffer: > > - kfree(da_tokens); > > - return ret; > > } > > > > static void __exit dell_smbios_exit(void) > > { > > kfree(da_tokens); > > free_page((unsigned long)buffer); > > + wmi_driver_unregister(&dell_wmi_smbios_driver); > > } > > > > subsys_initcall(dell_smbios_init); > > module_exit(dell_smbios_exit); > > > > + > > MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>"); > > MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>"); > > MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>"); > > +MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>"); > > MODULE_DESCRIPTION("Common functions for kernel modules using Dell > SMBIOS"); > > MODULE_LICENSE("GPL"); > > diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell- > smbios.h > > index 45cbc2292cd3..2f6fce81ee69 100644 > > --- a/drivers/platform/x86/dell-smbios.h > > +++ b/drivers/platform/x86/dell-smbios.h > > @@ -4,6 +4,7 @@ > > * Copyright (c) Red Hat <mjg@redhat.com> > > * Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com> > > * Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com> > > + * Copyright (c) 2017 Dell Inc. > > * > > * Based on documentation in the libsmbios package: > > * Copyright (C) 2005-2014 Dell Inc. > > @@ -18,9 +19,10 @@ > > > > struct notifier_block; > > > > -/* This structure will be modified by the firmware when we enter > > - * system management mode, hence the volatiles */ > > - > > +/* If called through fallback SMI rather than WMI this structure will be > > + * modified by the firmware when we enter system management mode, hence > the > > + * volatiles > > + */ > > struct calling_interface_buffer { > > u16 class; > > u16 select; > > @@ -28,6 +30,13 @@ struct calling_interface_buffer { > > volatile u32 output[4]; > > } __packed; > > > > +struct wmi_calling_interface_buffer { > > + struct calling_interface_buffer smi; > > + u32 argattrib; > > + u32 blength; > > + u8 data[32724]; > > +} __packed; > > + > > struct calling_interface_token { > > u16 tokenID; > > u16 location; > > > > -- > Pali Rohár > pali.rohar@gmail.com
On Thursday 28 September 2017 22:43:36 Mario.Limonciello@dell.com wrote: > > > @@ -170,10 +226,43 @@ static void __init find_tokens(const struct dmi_header > > *dm, void *dummy) > > > } > > > } > > > > > > -static int __init dell_smbios_init(void) > > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev) > > > +{ > > > + /* no longer need the SMI page */ > > > + free_page((unsigned long)buffer); > > > + > > > + /* WMI buffer should be 32k */ > > > + buffer = (void *)__get_free_pages(GFP_KERNEL, 3); > > > + if (!buffer) > > > + return -ENOMEM; > > > + bufferlen = sizeof(struct wmi_calling_interface_buffer); > > > + wmi_dev = wdev; > > > + return 0; > > > +} > > > + > > > +static int dell_smbios_wmi_remove(struct wmi_device *wdev) > > > { > > > - int ret; > > > + wmi_dev = NULL; > > > + free_pages((unsigned long)buffer, 3); > > > + return 0; > > > +} > > > > This code does not seem to be safe. dell_smbios_wmi_probe and > > dell_smbios_wmi_remove are called at any time when kernel register new > > device which matches some properties OR when user manually bind this > > driver to that device. > > > I'll adjust for the assumptions I made about it only happening at module init. > > > buffer and wmi_dev is shared as a global variable which means that when > > there are two devices which want to bind to this driver, kernel would > > get double free at removing time. > > But there is only one GUID in id_table. That is truth, but ... > How can two devices bind to this? > This should be an impossible scenario. ... in ACPI DSDT can be more WMI _WDG buffers which could lead to more wmi buses and each could have own GUID. Therefore there is a theoretical chance that specially prepared ACPI DSDT code can cause this problem. > > Devices from the bus subsystem should not use global shared variables, > > but rather allocate own memory... > > Part of the problem is that this module can operate in two modes now. > I think there will have to be global shared variables when operating in SMI > mode. Or maybe put those bits into a platform_device for SMI mode usage? > > The problem I think then becomes how do you handle dell_smbios_send_request? > Without global shared memory for the module the dell-laptop and dell-wmi > modules won't be able to use this. The whole problem is that SMBIOS calls are implemented via singleton pattern. And this singleton "instance" needs to use something which is not a singleton, but a dynamic objects (wmi device <--> driver pattern). Idea how to handle it: * put wmi smbios call function into own driver * put smm smbios call function into own driver * create dispatcher function which take first available device of one of the above driver and call on them smbios call function This problem is very similar to problems in objects world... driver as a class and device as a instance. (Or if somebody has a better idea, let us know...) > > Also note that user can at any time unbound device from driver and also > > can bind it again. > I'll make some adjustments to account for this. To prevent crashing kernel, this needs to be written correctly. And user should not be able to crash kernel just when trying to unbound driver from the device.
On Wed, Sep 27, 2017 at 11:02:14PM -0500, Mario Limonciello wrote: > The driver currently uses an SMI interface which grants direct access > to physical memory to the firmware SMM methods via a pointer. > > Now add a WMI-ACPI interface that is detected by WMI probe and preferred > over the SMI interface. > > Changing this to operate over WMI-ACPI will use an ACPI OperationRegion > for a buffer of data storage when SMM calls are performed. > > This is a safer approach to use in kernel drivers as the SMM will > only have access to that OperationRegion. > > As a result, this change removes the dependency on this driver on the > dcdbas kernel module. It's now an optional compilation option. > > When modifying this, add myself to MAINTAINERS. > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > --- ... > +DELL SMBIOS DRIVER > +M: Pali Rohár <pali.rohar@gmail.com> > +M: Mario Limonciello <mario.limonciello@dell.com> > +S: Maintained > +F: drivers/platform/x86/dell-smbios.* Pali, do you agree with this? ... > int dell_smbios_error(int value) > @@ -60,13 +69,15 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void) > { > mutex_lock(&buffer_mutex); > dell_smbios_clear_buffer(); > + if (wmi_dev) > + return &((struct wmi_calling_interface_buffer *) buffer)->smi; > return buffer; Hrm, my hope had been to make this transparent at this level. ... This may need more thought. I don't care for the casting here.... hopefully enlightenment lies below... > +static int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf) > +{ > + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; > + struct acpi_buffer input; > + union acpi_object *obj; > + acpi_status status; > + > + input.length = sizeof(struct wmi_calling_interface_buffer); > + input.pointer = buf; > + > + status = wmidev_evaluate_method(wmi_dev, 0, 1, &input, &output); > + if (ACPI_FAILURE(status)) { > + pr_err("%x/%x [%x,%x,%x,%x] call failed\n", > + buf->smi.class, buf->smi.select, > + buf->smi.input[0], buf->smi.input[1], > + buf->smi.input[2], buf->smi.input[3]); > + return -EIO; > + } > + obj = (union acpi_object *)output.pointer; > + if (obj->type != ACPI_TYPE_BUFFER) { > + pr_err("invalid type : %d\n", obj->type); > + return -EIO; > + } We ensure we don't write beyond buf, but we havne't ensured we don't read beyond obj. if (obj->length != input.length) { // or maybe >= ?? pr_err("invalid buffer length : %d\n", obj->length); return -EINVAL; } > -static int __init dell_smbios_init(void) > +static int dell_smbios_wmi_probe(struct wmi_device *wdev) > +{ > + /* no longer need the SMI page */ > + free_page((unsigned long)buffer); > + > + /* WMI buffer should be 32k */ > + buffer = (void *)__get_free_pages(GFP_KERNEL, 3); Assuming PAGE_SIZE here (I know, this driver, this architecture, etc...). But, please use get_order() to determine number of pages from a linear size: __get_free_pages(GFP_KERNEL, get_order(32768)); > + if (!buffer) > + return -ENOMEM; > + bufferlen = sizeof(struct wmi_calling_interface_buffer); Use a consistent way to allocate and set the len. Maybe set bufferlen above and use get_order(bufferlen). > + wmi_dev = wdev; > + return 0; > +} > + > +static int dell_smbios_wmi_remove(struct wmi_device *wdev) > { > - int ret; > + wmi_dev = NULL; > + free_pages((unsigned long)buffer, 3); > + return 0; > +} > > +static const struct wmi_device_id dell_smbios_wmi_id_table[] = { > + { .guid_string = DELL_WMI_SMBIOS_GUID }, > + { }, > +}; > + > +static struct wmi_driver dell_wmi_smbios_driver = { > + .driver = { > + .name = "dell-smbios", > + }, > + .probe = dell_smbios_wmi_probe, > + .remove = dell_smbios_wmi_remove, > + .id_table = dell_smbios_wmi_id_table, > +}; > + > +static int __init dell_smbios_init(void) > +{ > dmi_walk(find_tokens, NULL); > > if (!da_tokens) { > @@ -181,34 +270,39 @@ static int __init dell_smbios_init(void) > return -ENODEV; > } > > +#ifdef CONFIG_DCDBAS If this cannot be avoided, then use IS_ENABLED(CONFIG_DCDBAS). I still think we should be to come up with a cleaner way to deal with the two buffers than a bunch of #ifdefs and if (wdev) {} else {} blocks. ... > diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h ... > -/* This structure will be modified by the firmware when we enter > - * system management mode, hence the volatiles */ > - > +/* If called through fallback SMI rather than WMI this structure will be > + * modified by the firmware when we enter system management mode, hence the > + * volatiles > + */ Follow coding style when modifying comment blocks (even when they didn't before) please. See coding-style 8) COmmenting. /* * If called ... ... * volatiles. */
On Saturday 30 September 2017 02:51:27 Darren Hart wrote: > > +DELL SMBIOS DRIVER > > +M: Pali Rohár <pali.rohar@gmail.com> > > +M: Mario Limonciello <mario.limonciello@dell.com> > > +S: Maintained > > +F: drivers/platform/x86/dell-smbios.* > > Pali, do you agree with this? Yes, no problem. > > -static int __init dell_smbios_init(void) > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev) > > +{ > > + /* no longer need the SMI page */ > > + free_page((unsigned long)buffer); > > + > > + /* WMI buffer should be 32k */ > > + buffer = (void *)__get_free_pages(GFP_KERNEL, 3); > > Assuming PAGE_SIZE here (I know, this driver, this architecture, > etc...). But, please use get_order() to determine number of pages > from a linear size: > > __get_free_pages(GFP_KERNEL, get_order(32768)); I agree that specifying size (instead of count) explicitly lead to more readable code.
> -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Saturday, September 30, 2017 2:16 AM > To: Darren Hart <dvhart@infradead.org> > Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; LKML <linux-kernel@vger.kernel.org>; > platform-driver-x86@vger.kernel.org; Andy Lutomirski <luto@kernel.org>; > quasisec@google.com > Subject: Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI > interface > > On Saturday 30 September 2017 02:51:27 Darren Hart wrote: > > > +DELL SMBIOS DRIVER > > > +M: Pali Rohár <pali.rohar@gmail.com> > > > +M: Mario Limonciello <mario.limonciello@dell.com> > > > +S: Maintained > > > +F: drivers/platform/x86/dell-smbios.* > > > > Pali, do you agree with this? > > Yes, no problem. > > > > -static int __init dell_smbios_init(void) > > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev) > > > +{ > > > + /* no longer need the SMI page */ > > > + free_page((unsigned long)buffer); > > > + > > > + /* WMI buffer should be 32k */ > > > + buffer = (void *)__get_free_pages(GFP_KERNEL, 3); > > > > Assuming PAGE_SIZE here (I know, this driver, this architecture, > > etc...). But, please use get_order() to determine number of pages > > from a linear size: > > > > __get_free_pages(GFP_KERNEL, get_order(32768)); > > I agree that specifying size (instead of count) explicitly lead to more > readable code. > Alright will adjust.
> -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Friday, September 29, 2017 2:36 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org; > quasisec@google.com > Subject: Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI > interface > > On Thursday 28 September 2017 22:43:36 Mario.Limonciello@dell.com wrote: > > > > @@ -170,10 +226,43 @@ static void __init find_tokens(const struct > dmi_header > > > *dm, void *dummy) > > > > } > > > > } > > > > > > > > -static int __init dell_smbios_init(void) > > > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev) > > > > +{ > > > > + /* no longer need the SMI page */ > > > > + free_page((unsigned long)buffer); > > > > + > > > > + /* WMI buffer should be 32k */ > > > > + buffer = (void *)__get_free_pages(GFP_KERNEL, 3); > > > > + if (!buffer) > > > > + return -ENOMEM; > > > > + bufferlen = sizeof(struct wmi_calling_interface_buffer); > > > > + wmi_dev = wdev; > > > > + return 0; > > > > +} > > > > + > > > > +static int dell_smbios_wmi_remove(struct wmi_device *wdev) > > > > { > > > > - int ret; > > > > + wmi_dev = NULL; > > > > + free_pages((unsigned long)buffer, 3); > > > > + return 0; > > > > +} > > > > > > This code does not seem to be safe. dell_smbios_wmi_probe and > > > dell_smbios_wmi_remove are called at any time when kernel register new > > > device which matches some properties OR when user manually bind this > > > driver to that device. > > > > > I'll adjust for the assumptions I made about it only happening at module init. > > > > > buffer and wmi_dev is shared as a global variable which means that when > > > there are two devices which want to bind to this driver, kernel would > > > get double free at removing time. > > > > But there is only one GUID in id_table. > > That is truth, but ... > > > How can two devices bind to this? > > This should be an impossible scenario. > > ... in ACPI DSDT can be more WMI _WDG buffers which could lead to more > wmi buses and each could have own GUID. Therefore there is a theoretical > chance that specially prepared ACPI DSDT code can cause this problem. I guess I'm a bit confused - is this for protecting a user who patches their own DSDT or from a vendor releasing a system with two _WDG buffers with the same GUID? I don't believe MOF has a concept of which WDG buffer GUIDs are assigned to. That could lead to massively undefined behavior too since the WDG buffer will potentially assign different ASL to process for each GUID. > > > > Devices from the bus subsystem should not use global shared variables, > > > but rather allocate own memory... > > > > Part of the problem is that this module can operate in two modes now. > > I think there will have to be global shared variables when operating in SMI > > mode. Or maybe put those bits into a platform_device for SMI mode usage? > > > > The problem I think then becomes how do you handle > dell_smbios_send_request? > > Without global shared memory for the module the dell-laptop and dell-wmi > > modules won't be able to use this. > > The whole problem is that SMBIOS calls are implemented via singleton > pattern. And this singleton "instance" needs to use something which is > not a singleton, but a dynamic objects (wmi device <--> driver pattern). > > Idea how to handle it: > * put wmi smbios call function into own driver > * put smm smbios call function into own driver > * create dispatcher function which take first available device of one of > the above driver and call on them smbios call function > > This problem is very similar to problems in objects world... driver as a > class and device as a instance. > > (Or if somebody has a better idea, let us know...) So I like the 3rd idea the most, and it's what I'm working on. I've got some problems with it that I'm still fixing, but unless someone tells me otherwise I'll go for that with v4. > > > > Also note that user can at any time unbound device from driver and also > > > can bind it again. > > I'll make some adjustments to account for this. > > To prevent crashing kernel, this needs to be written correctly. And user > should not be able to crash kernel just when trying to unbound driver > from the device. > Agree.
On Saturday 30 September 2017 22:01:04 Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Pali Rohár [mailto:pali.rohar@gmail.com] > > Sent: Friday, September 29, 2017 2:36 AM > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; > > luto@kernel.org; quasisec@google.com > > Subject: Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a > > WMI-ACPI interface > > > > On Thursday 28 September 2017 22:43:36 Mario.Limonciello@dell.com > > wrote: > > > > > @@ -170,10 +226,43 @@ static void __init find_tokens(const > > > > > struct > > > > dmi_header > > > > > > *dm, void *dummy) > > > > > > > > > } > > > > > > > > > > } > > > > > > > > > > -static int __init dell_smbios_init(void) > > > > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev) > > > > > +{ > > > > > + /* no longer need the SMI page */ > > > > > + free_page((unsigned long)buffer); > > > > > + > > > > > + /* WMI buffer should be 32k */ > > > > > + buffer = (void *)__get_free_pages(GFP_KERNEL, 3); > > > > > + if (!buffer) > > > > > + return -ENOMEM; > > > > > + bufferlen = sizeof(struct wmi_calling_interface_buffer); > > > > > + wmi_dev = wdev; > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static int dell_smbios_wmi_remove(struct wmi_device *wdev) > > > > > > > > > > { > > > > > > > > > > - int ret; > > > > > + wmi_dev = NULL; > > > > > + free_pages((unsigned long)buffer, 3); > > > > > + return 0; > > > > > +} > > > > > > > > This code does not seem to be safe. dell_smbios_wmi_probe and > > > > dell_smbios_wmi_remove are called at any time when kernel > > > > register new device which matches some properties OR when user > > > > manually bind this driver to that device. > > > > > > I'll adjust for the assumptions I made about it only happening at > > > module init. > > > > > > > buffer and wmi_dev is shared as a global variable which means > > > > that when there are two devices which want to bind to this > > > > driver, kernel would get double free at removing time. > > > > > > But there is only one GUID in id_table. > > > > That is truth, but ... > > > > > How can two devices bind to this? > > > This should be an impossible scenario. > > > > ... in ACPI DSDT can be more WMI _WDG buffers which could lead to > > more wmi buses and each could have own GUID. Therefore there is a > > theoretical chance that specially prepared ACPI DSDT code can > > cause this problem. > > I guess I'm a bit confused - is this for protecting a user who > patches their own DSDT or from a vendor releasing a system with two > _WDG buffers with the same GUID? It is protection for memory corruption done by dell-smbios driver in case such DSDT would be parsed by kernel (either by user who patches original DSDT or when vendor created such DSDT by mistake or by malicious software which patch/provide such DSDT...). DSDT is from kernel point of view external (possible untrusted) data. ACPI parser in kernel parse it if they are syntactically correct, but semantic or e.g. _WDG meaning is up to consumer -- in our case wmi and dell-smbios wmi drivers. > I don't believe MOF has a concept of which WDG buffer GUIDs are > assigned to. That could lead to massively undefined behavior too > since the WDG buffer will potentially assign different ASL to > process for each GUID. Yes, that would lead to undefined behaviour. But still it should not crash kernel. Either treat such thing as "corrupted data" and refuse to use it or treat it in deterministic way, e.g. "first come, first serve" or load it driver for all guids... E.g. DSDT on Thinkpads has more _WDG buffers and I was told that some machines with Nvidia graphics cards really have duplicate WMI GUIDs in _WDG. So such situation really happen in world, it is not just theoretical. > > The whole problem is that SMBIOS calls are implemented via > > singleton pattern. And this singleton "instance" needs to use > > something which is not a singleton, but a dynamic objects (wmi > > device <--> driver pattern). > > > > Idea how to handle it: > > * put wmi smbios call function into own driver > > * put smm smbios call function into own driver > > * create dispatcher function which take first available device of > > one of > > > > the above driver and call on them smbios call function > > > > This problem is very similar to problems in objects world... driver > > as a class and device as a instance. > > > > (Or if somebody has a better idea, let us know...) > > So I like the 3rd idea the most, and it's what I'm working on. I've > got some problems with it that I'm still fixing, but unless someone > tells me otherwise I'll go for that with v4. All above 3 points (*) were meant as one solution. Putting wmi and smm calls into own drivers and then creating dispatcher function which would use available device of one of those two drivers.
diff --git a/MAINTAINERS b/MAINTAINERS index 08b96f77f618..6d76b09f46cc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3990,6 +3990,12 @@ S: Maintained F: drivers/hwmon/dell-smm-hwmon.c F: include/uapi/linux/i8k.h +DELL SMBIOS DRIVER +M: Pali Rohár <pali.rohar@gmail.com> +M: Mario Limonciello <mario.limonciello@dell.com> +S: Maintained +F: drivers/platform/x86/dell-smbios.* + DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas) M: Doug Warzecha <Douglas_Warzecha@dell.com> S: Maintained diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 1f7959ff055c..415886d7a857 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -92,13 +92,17 @@ config ASUS_LAPTOP If you have an ACPI-compatible ASUS laptop, say Y or M here. config DELL_SMBIOS - tristate - select DCDBAS + tristate "Dell SMBIOS calling interface" + depends on ACPI_WMI ---help--- - This module provides common functions for kernel modules using - Dell SMBIOS. + This module provides common functions for kernel modules and + userspace using Dell SMBIOS. + + If you have a Dell computer, say Y or M here. - If you have a Dell laptop, say Y or M here. + If you have a machine from < 2007 without a WMI interface you + may also want to enable CONFIG_DCDBAS to allow this driver to + work. config DELL_LAPTOP tristate "Dell Laptop Extras" diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c index e9b1ca07c872..4472817ee045 100644 --- a/drivers/platform/x86/dell-smbios.c +++ b/drivers/platform/x86/dell-smbios.c @@ -4,6 +4,7 @@ * Copyright (c) Red Hat <mjg@redhat.com> * Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com> * Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com> + * Copyright (c) 2017 Dell Inc. * * Based on documentation in the libsmbios package: * Copyright (C) 2005-2014 Dell Inc. @@ -22,9 +23,15 @@ #include <linux/mutex.h> #include <linux/slab.h> #include <linux/io.h> -#include "../../firmware/dcdbas.h" +#include <linux/wmi.h> #include "dell-smbios.h" +#ifdef CONFIG_DCDBAS +#include "../../firmware/dcdbas.h" +#endif + +#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492" + struct calling_interface_structure { struct dmi_header header; u16 cmdIOAddress; @@ -33,12 +40,14 @@ struct calling_interface_structure { struct calling_interface_token tokens[]; } __packed; -static struct calling_interface_buffer *buffer; +static void *buffer; +static size_t bufferlen; static DEFINE_MUTEX(buffer_mutex); static int da_command_address; static int da_command_code; static int da_num_tokens; +struct wmi_device *wmi_dev; static struct calling_interface_token *da_tokens; int dell_smbios_error(int value) @@ -60,13 +69,15 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void) { mutex_lock(&buffer_mutex); dell_smbios_clear_buffer(); + if (wmi_dev) + return &((struct wmi_calling_interface_buffer *) buffer)->smi; return buffer; } EXPORT_SYMBOL_GPL(dell_smbios_get_buffer); void dell_smbios_clear_buffer(void) { - memset(buffer, 0, sizeof(struct calling_interface_buffer)); + memset(buffer, 0, bufferlen); } EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer); @@ -76,7 +87,36 @@ void dell_smbios_release_buffer(void) } EXPORT_SYMBOL_GPL(dell_smbios_release_buffer); -void dell_smbios_send_request(int class, int select) +static int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf) +{ + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; + struct acpi_buffer input; + union acpi_object *obj; + acpi_status status; + + input.length = sizeof(struct wmi_calling_interface_buffer); + input.pointer = buf; + + status = wmidev_evaluate_method(wmi_dev, 0, 1, &input, &output); + if (ACPI_FAILURE(status)) { + pr_err("%x/%x [%x,%x,%x,%x] call failed\n", + buf->smi.class, buf->smi.select, + buf->smi.input[0], buf->smi.input[1], + buf->smi.input[2], buf->smi.input[3]); + return -EIO; + } + obj = (union acpi_object *)output.pointer; + if (obj->type != ACPI_TYPE_BUFFER) { + pr_err("invalid type : %d\n", obj->type); + return -EIO; + } + memcpy(buf, obj->buffer.pointer, input.length); + + return 0; +} + +#ifdef CONFIG_DCDBAS +static void run_smi_smbios_call(struct calling_interface_buffer *buf) { struct smi_cmd command; @@ -85,12 +125,28 @@ void dell_smbios_send_request(int class, int select) command.command_code = da_command_code; command.ebx = virt_to_phys(buffer); command.ecx = 0x42534931; - - buffer->class = class; - buffer->select = select; - dcdbas_smi_request(&command); } +#else +static void run_smi_smbios_call(struct calling_interface_buffer *buf) {} +#endif /* CONFIG_DCDBAS */ + +void dell_smbios_send_request(int class, int select) +{ + if (wmi_dev) { + struct wmi_calling_interface_buffer *buf = buffer; + + buf->smi.class = class; + buf->smi.select = select; + run_wmi_smbios_call(buf); + } else { + struct calling_interface_buffer *buf = buffer; + + buf->class = class; + buf->select = select; + run_smi_smbios_call(buf); + } +} EXPORT_SYMBOL_GPL(dell_smbios_send_request); struct calling_interface_token *dell_smbios_find_token(int tokenid) @@ -170,10 +226,43 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy) } } -static int __init dell_smbios_init(void) +static int dell_smbios_wmi_probe(struct wmi_device *wdev) +{ + /* no longer need the SMI page */ + free_page((unsigned long)buffer); + + /* WMI buffer should be 32k */ + buffer = (void *)__get_free_pages(GFP_KERNEL, 3); + if (!buffer) + return -ENOMEM; + bufferlen = sizeof(struct wmi_calling_interface_buffer); + wmi_dev = wdev; + return 0; +} + +static int dell_smbios_wmi_remove(struct wmi_device *wdev) { - int ret; + wmi_dev = NULL; + free_pages((unsigned long)buffer, 3); + return 0; +} +static const struct wmi_device_id dell_smbios_wmi_id_table[] = { + { .guid_string = DELL_WMI_SMBIOS_GUID }, + { }, +}; + +static struct wmi_driver dell_wmi_smbios_driver = { + .driver = { + .name = "dell-smbios", + }, + .probe = dell_smbios_wmi_probe, + .remove = dell_smbios_wmi_remove, + .id_table = dell_smbios_wmi_id_table, +}; + +static int __init dell_smbios_init(void) +{ dmi_walk(find_tokens, NULL); if (!da_tokens) { @@ -181,34 +270,39 @@ static int __init dell_smbios_init(void) return -ENODEV; } +#ifdef CONFIG_DCDBAS /* * Allocate buffer below 4GB for SMI data--only 32-bit physical addr * is passed to SMI handler. */ buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32); + bufferlen = sizeof(struct calling_interface_buffer); +#else + buffer = NULL; +#endif /* CONFIG_DCDBAS */ + wmi_driver_register(&dell_wmi_smbios_driver); + if (!buffer) { - ret = -ENOMEM; - goto fail_buffer; + kfree(da_tokens); + return -ENOMEM; } - return 0; - -fail_buffer: - kfree(da_tokens); - return ret; } static void __exit dell_smbios_exit(void) { kfree(da_tokens); free_page((unsigned long)buffer); + wmi_driver_unregister(&dell_wmi_smbios_driver); } subsys_initcall(dell_smbios_init); module_exit(dell_smbios_exit); + MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>"); MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>"); MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>"); +MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>"); MODULE_DESCRIPTION("Common functions for kernel modules using Dell SMBIOS"); MODULE_LICENSE("GPL"); diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h index 45cbc2292cd3..2f6fce81ee69 100644 --- a/drivers/platform/x86/dell-smbios.h +++ b/drivers/platform/x86/dell-smbios.h @@ -4,6 +4,7 @@ * Copyright (c) Red Hat <mjg@redhat.com> * Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com> * Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com> + * Copyright (c) 2017 Dell Inc. * * Based on documentation in the libsmbios package: * Copyright (C) 2005-2014 Dell Inc. @@ -18,9 +19,10 @@ struct notifier_block; -/* This structure will be modified by the firmware when we enter - * system management mode, hence the volatiles */ - +/* If called through fallback SMI rather than WMI this structure will be + * modified by the firmware when we enter system management mode, hence the + * volatiles + */ struct calling_interface_buffer { u16 class; u16 select; @@ -28,6 +30,13 @@ struct calling_interface_buffer { volatile u32 output[4]; } __packed; +struct wmi_calling_interface_buffer { + struct calling_interface_buffer smi; + u32 argattrib; + u32 blength; + u8 data[32724]; +} __packed; + struct calling_interface_token { u16 tokenID; u16 location;
The driver currently uses an SMI interface which grants direct access to physical memory to the firmware SMM methods via a pointer. Now add a WMI-ACPI interface that is detected by WMI probe and preferred over the SMI interface. Changing this to operate over WMI-ACPI will use an ACPI OperationRegion for a buffer of data storage when SMM calls are performed. This is a safer approach to use in kernel drivers as the SMM will only have access to that OperationRegion. As a result, this change removes the dependency on this driver on the dcdbas kernel module. It's now an optional compilation option. When modifying this, add myself to MAINTAINERS. Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> --- MAINTAINERS | 6 ++ drivers/platform/x86/Kconfig | 14 ++-- drivers/platform/x86/dell-smbios.c | 128 ++++++++++++++++++++++++++++++++----- drivers/platform/x86/dell-smbios.h | 15 ++++- 4 files changed, 138 insertions(+), 25 deletions(-)