[PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
Tested-by: Mario Limonciello <mario.limonciello@dell.com>
Acked-by: Doug Warzecha <douglas_warzecha@dell.com>
---
v2 Bumped driver version to 5.6.0-3.3


 drivers/firmware/Kconfig  |   2 +-
 drivers/firmware/dcdbas.c | 102 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/firmware/dcdbas.h |  11 +++++
 3 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b7c748248e53..a2bd6092bfa1 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -125,7 +125,7 @@ config DELL_RBU
 
 config DCDBAS
 	tristate "Dell Systems Management Base Driver"
-	depends on X86
+	depends on X86 && ACPI
 	help
 	  The Dell Systems Management Base Driver provides a sysfs interface
 	  for systems management software to perform System Management
diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..1699cfdcefe4 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -36,12 +36,13 @@
 #include <linux/string.h>
 #include <linux/types.h>
 #include <linux/mutex.h>
+#include <linux/acpi.h>
 #include <asm/io.h>
 
 #include "dcdbas.h"
 
 #define DRIVER_NAME		"dcdbas"
-#define DRIVER_VERSION		"5.6.0-3.2"
+#define DRIVER_VERSION		"5.6.0-3.3"
 #define DRIVER_DESCRIPTION	"Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-	if (!smi_data_buf)
+	if (!smi_data_buf || wsmt_enabled)
 		return;
 
 	dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
 	if (smi_data_buf_size >= size)
 		return 0;
 
-	if (size > MAX_SMI_DATA_BUF_SIZE)
+	if (size > max_smi_data_buf_size)
 		return -EINVAL;
 
 	/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct kobject *kobj,
 {
 	ssize_t ret;
 
-	if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+	if ((pos + count) > max_smi_data_buf_size)
 		return -EINVAL;
 
 	mutex_lock(&smi_data_lock);
@@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
 		break;
 	case 1:
 		/* Calling Interface SMI */
-		smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+		smi_cmd->ebx = smi_data_buf_phys_addr
+				+ offsetof(struct smi_cmd, command_buffer);
 		ret = dcdbas_smi_request(smi_cmd);
 		if (!ret)
 			ret = count;
@@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
 	}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+	u8 sum = 0;
+	u8 *end = buffer + length;
+
+	while (buffer < end)
+		sum = (u8)(sum + *(buffer++));
+	return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+	struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+	if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
+		return NULL;
+
+	if (checksum(addr, eps->length) != 0)
+		return NULL;
+
+	return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+	struct acpi_table_wsmt *wsmt = NULL;
+	struct smm_eps_table *eps = NULL;
+	u8 *addr;
+
+	acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
+	if (!wsmt)
+		return 0;
+
+	/* Check if WSMT ACPI table shows that protection is enabled */
+	if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
+	    || !(wsmt->protection_flags
+		 & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+		return 0;
+
+	/* Scan for EPS (entry point structure) */
+	for (addr = (u8 *)__va(0xf0000);
+	     addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)) && !eps;
+	     addr += 1)
+		eps = check_eps_table(addr);
+
+	if (!eps) {
+		dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Get physical address of buffer and map to virtual address.
+	 * Table gives size in 4K pages, regardless of actual system page size.
+	 */
+	if (eps->smm_comm_buff_addr + 8 > U32_MAX) {
+		dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is above 4GB\n");
+		return -EINVAL;
+	}
+	eps_buffer = (u8 *)memremap(eps->smm_comm_buff_addr,
+				     eps->num_of_4k_pages * 4096, MEMREMAP_WB);
+	if (!eps_buffer) {
+		dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS buffer\n");
+		return -ENOMEM;
+	}
+
+	/* First 8 bytes of buffer is for semaphore */
+	smi_data_buf_phys_addr = (u32) eps->smm_comm_buff_addr + 8;
+	smi_data_buf = eps_buffer + 8;
+	smi_data_buf_size = (unsigned long) min(eps->num_of_4k_pages * 4096 - 8,
+			    (u64) ULONG_MAX);
+	max_smi_data_buf_size = smi_data_buf_size;
+	wsmt_enabled = true;
+	dev_info(&dcdbas_pdev->dev,
+		 "WSMT found, using firmware-provided SMI buffer.\n");
+	return 1;
+}
+
 /**
  * dcdbas_reboot_notify: handle reboot notification for host control
  */
@@ -548,6 +633,11 @@ static int dcdbas_probe(struct platform_device *dev)
 
 	dcdbas_pdev = dev;
 
+	/* Check if ACPI WSMT table specifies protected SMI buffer address */
+	error = dcdbas_check_wsmt();
+	if (error < 0)
+		return error;
+
 	/*
 	 * BIOS SMI calls require buffer addresses be in 32-bit address space.
 	 * This is done by setting the DMA mask below.
@@ -635,6 +725,8 @@ static void __exit dcdbas_exit(void)
 	 */
 	if (dcdbas_pdev)
 		smi_data_buf_free();
+	if (eps_buffer)
+		memunmap(eps_buffer);
 	platform_device_unregister(dcdbas_pdev_reg);
 	platform_driver_unregister(&dcdbas_driver);
 }
diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h
index ca3cb0a54ab6..7ea5b3e070b9 100644
--- a/drivers/firmware/dcdbas.h
+++ b/drivers/firmware/dcdbas.h
@@ -54,6 +54,8 @@
 
 #define SMI_CMD_MAGIC				(0x534D4931)
 
+#define SMM_EPS_SIG				"$SCB"
+
 #define DCDBAS_DEV_ATTR_RW(_name) \
 	DEVICE_ATTR(_name,0600,_name##_show,_name##_store);
 
@@ -103,5 +105,14 @@ struct apm_cmd {
 
 int dcdbas_smi_request(struct smi_cmd *smi_cmd);
 
+struct smm_eps_table {
+	char smm_comm_buff_anchor[4];
+	u8 length;
+	u8 checksum;
+	u8 version;
+	u64 smm_comm_buff_addr;
+	u64 num_of_4k_pages;
+} __packed;
+
 #endif /* _DCDBAS_H_ */
 
-- 
2.14.2

RE: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Mario.Limonciello]

> -----Original Message-----
> From: Stuart Hayes [mailto:stuart.w.hayes@gmail.com]
> Sent: Thursday, June 7, 2018 11:00 AM
> To: Warzecha, Douglas
> Cc: Limonciello, Mario; Dominguez, Jared; linux-kernel@vger.kernel.org; platform-
> driver-x86@vger.kernel.org
> Subject: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
> 
> If the WSMT ACPI table is present and indicates that a fixed communication
> buffer should be used, use the firmware-specified buffer instead of
> allocating a buffer in memory for communications between the dcdbas driver
> and firmare.
> 
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> Tested-by: Mario Limonciello <mario.limonciello@dell.com>
> Acked-by: Doug Warzecha <douglas_warzecha@dell.com>
> ---
> v2 Bumped driver version to 5.6.0-3.3
> 
> 
>  drivers/firmware/Kconfig  |   2 +-
>  drivers/firmware/dcdbas.c | 102
> +++++++++++++++++++++++++++++++++++++++++++---
>  drivers/firmware/dcdbas.h |  11 +++++
>  3 files changed, 109 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index b7c748248e53..a2bd6092bfa1 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -125,7 +125,7 @@ config DELL_RBU
> 
>  config DCDBAS
>  	tristate "Dell Systems Management Base Driver"
> -	depends on X86
> +	depends on X86 && ACPI
>  	help
>  	  The Dell Systems Management Base Driver provides a sysfs interface
>  	  for systems management software to perform System Management
> diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
> index 0bdea60c65dd..1699cfdcefe4 100644
> --- a/drivers/firmware/dcdbas.c
> +++ b/drivers/firmware/dcdbas.c
> @@ -36,12 +36,13 @@
>  #include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/mutex.h>
> +#include <linux/acpi.h>
>  #include <asm/io.h>
> 
>  #include "dcdbas.h"
> 
>  #define DRIVER_NAME		"dcdbas"
> -#define DRIVER_VERSION		"5.6.0-3.2"
> +#define DRIVER_VERSION		"5.6.0-3.3"
>  #define DRIVER_DESCRIPTION	"Dell Systems Management Base Driver"
> 
>  static struct platform_device *dcdbas_pdev;
> @@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
>  static u8 *smi_data_buf;
>  static dma_addr_t smi_data_buf_handle;
>  static unsigned long smi_data_buf_size;
> +static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
>  static u32 smi_data_buf_phys_addr;
>  static DEFINE_MUTEX(smi_data_lock);
> +static u8 *eps_buffer;
> 
>  static unsigned int host_control_action;
>  static unsigned int host_control_smi_type;
>  static unsigned int host_control_on_shutdown;
> 
> +static bool wsmt_enabled;
> +
>  /**
>   * smi_data_buf_free: free SMI data buffer
>   */
>  static void smi_data_buf_free(void)
>  {
> -	if (!smi_data_buf)
> +	if (!smi_data_buf || wsmt_enabled)
>  		return;
> 
>  	dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
> @@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
>  	if (smi_data_buf_size >= size)
>  		return 0;
> 
> -	if (size > MAX_SMI_DATA_BUF_SIZE)
> +	if (size > max_smi_data_buf_size)
>  		return -EINVAL;
> 
>  	/* new buffer is needed */
> @@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct kobject
> *kobj,
>  {
>  	ssize_t ret;
> 
> -	if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
> +	if ((pos + count) > max_smi_data_buf_size)
>  		return -EINVAL;
> 
>  	mutex_lock(&smi_data_lock);
> @@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
>  		break;
>  	case 1:
>  		/* Calling Interface SMI */
> -		smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> +		smi_cmd->ebx = smi_data_buf_phys_addr
> +				+ offsetof(struct smi_cmd, command_buffer);
>  		ret = dcdbas_smi_request(smi_cmd);
>  		if (!ret)
>  			ret = count;
> @@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
>  	}
>  }
> 
> +/* WSMT */
> +
> +static u8 checksum(u8 *buffer, u8 length)
> +{
> +	u8 sum = 0;
> +	u8 *end = buffer + length;
> +
> +	while (buffer < end)
> +		sum = (u8)(sum + *(buffer++));
> +	return sum;
> +}
> +
> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> +{
> +	struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> +
> +	if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
> +		return NULL;
> +
> +	if (checksum(addr, eps->length) != 0)
> +		return NULL;
> +
> +	return eps;
> +}
> +
> +static int dcdbas_check_wsmt(void)
> +{
> +	struct acpi_table_wsmt *wsmt = NULL;
> +	struct smm_eps_table *eps = NULL;
> +	u8 *addr;
> +
> +	acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
> +	if (!wsmt)
> +		return 0;
> +
> +	/* Check if WSMT ACPI table shows that protection is enabled */
> +	if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> +	    || !(wsmt->protection_flags
> +		 & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> +		return 0;
> +
> +	/* Scan for EPS (entry point structure) */
> +	for (addr = (u8 *)__va(0xf0000);
> +	     addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)) && !eps;
> +	     addr += 1)
> +		eps = check_eps_table(addr);
> +
> +	if (!eps) {
> +		dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS
> found\n");
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Get physical address of buffer and map to virtual address.
> +	 * Table gives size in 4K pages, regardless of actual system page size.
> +	 */
> +	if (eps->smm_comm_buff_addr + 8 > U32_MAX) {
> +		dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer
> address is above 4GB\n");
> +		return -EINVAL;
> +	}
> +	eps_buffer = (u8 *)memremap(eps->smm_comm_buff_addr,
> +				     eps->num_of_4k_pages * 4096,
> MEMREMAP_WB);
> +	if (!eps_buffer) {
> +		dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map
> EPS buffer\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* First 8 bytes of buffer is for semaphore */
> +	smi_data_buf_phys_addr = (u32) eps->smm_comm_buff_addr + 8;
> +	smi_data_buf = eps_buffer + 8;
> +	smi_data_buf_size = (unsigned long) min(eps->num_of_4k_pages * 4096 -
> 8,
> +			    (u64) ULONG_MAX);
> +	max_smi_data_buf_size = smi_data_buf_size;
> +	wsmt_enabled = true;
> +	dev_info(&dcdbas_pdev->dev,
> +		 "WSMT found, using firmware-provided SMI buffer.\n");
> +	return 1;
> +}
> +
>  /**
>   * dcdbas_reboot_notify: handle reboot notification for host control
>   */
> @@ -548,6 +633,11 @@ static int dcdbas_probe(struct platform_device *dev)
> 
>  	dcdbas_pdev = dev;
> 
> +	/* Check if ACPI WSMT table specifies protected SMI buffer address */
> +	error = dcdbas_check_wsmt();
> +	if (error < 0)
> +		return error;
> +
>  	/*
>  	 * BIOS SMI calls require buffer addresses be in 32-bit address space.
>  	 * This is done by setting the DMA mask below.
> @@ -635,6 +725,8 @@ static void __exit dcdbas_exit(void)
>  	 */
>  	if (dcdbas_pdev)
>  		smi_data_buf_free();
> +	if (eps_buffer)
> +		memunmap(eps_buffer);
>  	platform_device_unregister(dcdbas_pdev_reg);
>  	platform_driver_unregister(&dcdbas_driver);
>  }
> diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h
> index ca3cb0a54ab6..7ea5b3e070b9 100644
> --- a/drivers/firmware/dcdbas.h
> +++ b/drivers/firmware/dcdbas.h
> @@ -54,6 +54,8 @@
> 
>  #define SMI_CMD_MAGIC				(0x534D4931)
> 
> +#define SMM_EPS_SIG				"$SCB"
> +
>  #define DCDBAS_DEV_ATTR_RW(_name) \
>  	DEVICE_ATTR(_name,0600,_name##_show,_name##_store);
> 
> @@ -103,5 +105,14 @@ struct apm_cmd {
> 
>  int dcdbas_smi_request(struct smi_cmd *smi_cmd);
> 
> +struct smm_eps_table {
> +	char smm_comm_buff_anchor[4];
> +	u8 length;
> +	u8 checksum;
> +	u8 version;
> +	u64 smm_comm_buff_addr;
> +	u64 num_of_4k_pages;
> +} __packed;
> +
>  #endif /* _DCDBAS_H_ */
> 
> --
> 2.14.2

Darren, Andy,

Stuart and I were hoping you guys could bring this through the platform-driver-x86 tree.
The logic behind this is that the only consumer for dcdbas in kernel is dell-smbios.

There isn't a subsystem maintainer for someone to pull in dcdbas even though the
current maintainer (Doug W) has acked it.

Thanks

Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Darren Hart]

On Thu, Jun 07, 2018 at 04:07:50PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Stuart Hayes [mailto:stuart.w.hayes@gmail.com]
> > Sent: Thursday, June 7, 2018 11:00 AM
> > To: Warzecha, Douglas
> > Cc: Limonciello, Mario; Dominguez, Jared; linux-kernel@vger.kernel.org; platform-
> > driver-x86@vger.kernel.org
> > Subject: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
> > 
> > If the WSMT ACPI table is present and indicates that a fixed communication
> > buffer should be used, use the firmware-specified buffer instead of
> > allocating a buffer in memory for communications between the dcdbas driver
> > and firmare.
> > 
> > Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> > Tested-by: Mario Limonciello <mario.limonciello@dell.com>
> > Acked-by: Doug Warzecha <douglas_warzecha@dell.com>
> > ---
> > v2 Bumped driver version to 5.6.0-3.3
> > 
> > 
> >  drivers/firmware/Kconfig  |   2 +-
> >  drivers/firmware/dcdbas.c | 102
> > +++++++++++++++++++++++++++++++++++++++++++---
> >  drivers/firmware/dcdbas.h |  11 +++++
> >  3 files changed, 109 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index b7c748248e53..a2bd6092bfa1 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -125,7 +125,7 @@ config DELL_RBU
> > 
> >  config DCDBAS
> >  	tristate "Dell Systems Management Base Driver"
> > -	depends on X86
> > +	depends on X86 && ACPI
> >  	help
> >  	  The Dell Systems Management Base Driver provides a sysfs interface
> >  	  for systems management software to perform System Management
> > diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
> > index 0bdea60c65dd..1699cfdcefe4 100644
> > --- a/drivers/firmware/dcdbas.c
> > +++ b/drivers/firmware/dcdbas.c
> > @@ -36,12 +36,13 @@
> >  #include <linux/string.h>
> >  #include <linux/types.h>
> >  #include <linux/mutex.h>
> > +#include <linux/acpi.h>
> >  #include <asm/io.h>
> > 
> >  #include "dcdbas.h"
> > 
> >  #define DRIVER_NAME		"dcdbas"
> > -#define DRIVER_VERSION		"5.6.0-3.2"
> > +#define DRIVER_VERSION		"5.6.0-3.3"
> >  #define DRIVER_DESCRIPTION	"Dell Systems Management Base Driver"
> > 
> >  static struct platform_device *dcdbas_pdev;
> > @@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
> >  static u8 *smi_data_buf;
> >  static dma_addr_t smi_data_buf_handle;
> >  static unsigned long smi_data_buf_size;
> > +static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
> >  static u32 smi_data_buf_phys_addr;
> >  static DEFINE_MUTEX(smi_data_lock);
> > +static u8 *eps_buffer;
> > 
> >  static unsigned int host_control_action;
> >  static unsigned int host_control_smi_type;
> >  static unsigned int host_control_on_shutdown;
> > 
> > +static bool wsmt_enabled;
> > +
> >  /**
> >   * smi_data_buf_free: free SMI data buffer
> >   */
> >  static void smi_data_buf_free(void)
> >  {
> > -	if (!smi_data_buf)
> > +	if (!smi_data_buf || wsmt_enabled)
> >  		return;
> > 
> >  	dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
> > @@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
> >  	if (smi_data_buf_size >= size)
> >  		return 0;
> > 
> > -	if (size > MAX_SMI_DATA_BUF_SIZE)
> > +	if (size > max_smi_data_buf_size)
> >  		return -EINVAL;
> > 
> >  	/* new buffer is needed */
> > @@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct kobject
> > *kobj,
> >  {
> >  	ssize_t ret;
> > 
> > -	if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
> > +	if ((pos + count) > max_smi_data_buf_size)
> >  		return -EINVAL;
> > 
> >  	mutex_lock(&smi_data_lock);
> > @@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
> >  		break;
> >  	case 1:
> >  		/* Calling Interface SMI */
> > -		smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> > +		smi_cmd->ebx = smi_data_buf_phys_addr
> > +				+ offsetof(struct smi_cmd, command_buffer);
> >  		ret = dcdbas_smi_request(smi_cmd);
> >  		if (!ret)
> >  			ret = count;
> > @@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
> >  	}
> >  }
> > 
> > +/* WSMT */
> > +
> > +static u8 checksum(u8 *buffer, u8 length)
> > +{
> > +	u8 sum = 0;
> > +	u8 *end = buffer + length;
> > +
> > +	while (buffer < end)
> > +		sum = (u8)(sum + *(buffer++));
> > +	return sum;
> > +}
> > +
> > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> > +{
> > +	struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> > +
> > +	if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
> > +		return NULL;
> > +
> > +	if (checksum(addr, eps->length) != 0)
> > +		return NULL;
> > +
> > +	return eps;
> > +}
> > +
> > +static int dcdbas_check_wsmt(void)
> > +{
> > +	struct acpi_table_wsmt *wsmt = NULL;
> > +	struct smm_eps_table *eps = NULL;
> > +	u8 *addr;
> > +
> > +	acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
> > +	if (!wsmt)
> > +		return 0;
> > +
> > +	/* Check if WSMT ACPI table shows that protection is enabled */
> > +	if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> > +	    || !(wsmt->protection_flags
> > +		 & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> > +		return 0;
> > +
> > +	/* Scan for EPS (entry point structure) */
> > +	for (addr = (u8 *)__va(0xf0000);
> > +	     addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)) && !eps;
> > +	     addr += 1)
> > +		eps = check_eps_table(addr);
> > +
> > +	if (!eps) {
> > +		dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS
> > found\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/*
> > +	 * Get physical address of buffer and map to virtual address.
> > +	 * Table gives size in 4K pages, regardless of actual system page size.
> > +	 */
> > +	if (eps->smm_comm_buff_addr + 8 > U32_MAX) {
> > +		dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer
> > address is above 4GB\n");
> > +		return -EINVAL;
> > +	}
> > +	eps_buffer = (u8 *)memremap(eps->smm_comm_buff_addr,
> > +				     eps->num_of_4k_pages * 4096,
> > MEMREMAP_WB);
> > +	if (!eps_buffer) {
> > +		dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map
> > EPS buffer\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* First 8 bytes of buffer is for semaphore */
> > +	smi_data_buf_phys_addr = (u32) eps->smm_comm_buff_addr + 8;
> > +	smi_data_buf = eps_buffer + 8;
> > +	smi_data_buf_size = (unsigned long) min(eps->num_of_4k_pages * 4096 -
> > 8,
> > +			    (u64) ULONG_MAX);
> > +	max_smi_data_buf_size = smi_data_buf_size;
> > +	wsmt_enabled = true;
> > +	dev_info(&dcdbas_pdev->dev,
> > +		 "WSMT found, using firmware-provided SMI buffer.\n");
> > +	return 1;
> > +}
> > +
> >  /**
> >   * dcdbas_reboot_notify: handle reboot notification for host control
> >   */
> > @@ -548,6 +633,11 @@ static int dcdbas_probe(struct platform_device *dev)
> > 
> >  	dcdbas_pdev = dev;
> > 
> > +	/* Check if ACPI WSMT table specifies protected SMI buffer address */
> > +	error = dcdbas_check_wsmt();
> > +	if (error < 0)
> > +		return error;
> > +
> >  	/*
> >  	 * BIOS SMI calls require buffer addresses be in 32-bit address space.
> >  	 * This is done by setting the DMA mask below.
> > @@ -635,6 +725,8 @@ static void __exit dcdbas_exit(void)
> >  	 */
> >  	if (dcdbas_pdev)
> >  		smi_data_buf_free();
> > +	if (eps_buffer)
> > +		memunmap(eps_buffer);
> >  	platform_device_unregister(dcdbas_pdev_reg);
> >  	platform_driver_unregister(&dcdbas_driver);
> >  }
> > diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h
> > index ca3cb0a54ab6..7ea5b3e070b9 100644
> > --- a/drivers/firmware/dcdbas.h
> > +++ b/drivers/firmware/dcdbas.h
> > @@ -54,6 +54,8 @@
> > 
> >  #define SMI_CMD_MAGIC				(0x534D4931)
> > 
> > +#define SMM_EPS_SIG				"$SCB"
> > +
> >  #define DCDBAS_DEV_ATTR_RW(_name) \
> >  	DEVICE_ATTR(_name,0600,_name##_show,_name##_store);
> > 
> > @@ -103,5 +105,14 @@ struct apm_cmd {
> > 
> >  int dcdbas_smi_request(struct smi_cmd *smi_cmd);
> > 
> > +struct smm_eps_table {
> > +	char smm_comm_buff_anchor[4];
> > +	u8 length;
> > +	u8 checksum;
> > +	u8 version;
> > +	u64 smm_comm_buff_addr;
> > +	u64 num_of_4k_pages;
> > +} __packed;
> > +
> >  #endif /* _DCDBAS_H_ */
> > 
> > --
> > 2.14.2
> 
> Darren, Andy,
> 
> Stuart and I were hoping you guys could bring this through the platform-driver-x86 tree.
> The logic behind this is that the only consumer for dcdbas in kernel is dell-smbios.
> 
> There isn't a subsystem maintainer for someone to pull in dcdbas even though the
> current maintainer (Doug W) has acked it.

Apologies for the delay. I'm OK bringing it in in general. I'm reviewing patches
today and tomorrow to try and close out the backlog. Should have a review by EOD
tomorrow.

I've been wondering about drivers/firmware anyway - that is almost by definition
platform specific and might make more sense as part of the platform drivers,
especially if it is currently without a maintainer (although akpm will often
take such things).

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Andy Shevchenko]

On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
> If the WSMT ACPI table is present and indicates that a fixed communication
> buffer should be used, use the firmware-specified buffer instead of
> allocating a buffer in memory for communications between the dcdbas driver
> and firmare.

>  config DCDBAS
>         tristate "Dell Systems Management Base Driver"
> -       depends on X86
> +       depends on X86 && ACPI

Hmm... I'm not sure about this dependency.
So, the question is do all users actually need this? How did it work
previously? How has this been tested in case when command line has
"acpi=off" (yes, this one just for sake of test, I don't believe it's
a real use case)?

>  #include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/mutex.h>
> +#include <linux/acpi.h>

Please, try to keep an order as much as possible.
For example, in given context this line should be before string.h (I
didn't check the actual file, perhaps even upper).

>  #include <asm/io.h>
>
>  #include "dcdbas.h"

>                 /* Calling Interface SMI */
> -               smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> +               smi_cmd->ebx = smi_data_buf_phys_addr
> +                               + offsetof(struct smi_cmd, command_buffer);

Please, keep at least + on the previous line.
Also, I'm not sure what is the difference now. Especially for previous
users when this wasn't the part of the driver.
Some explanation needed.

> +static u8 checksum(u8 *buffer, u8 length)
> +{
> +       u8 sum = 0;
> +       u8 *end = buffer + length;
> +
> +       while (buffer < end)
> +               sum = (u8)(sum + *(buffer++));

Why not simple

sum += *buffer++;

?

> +       return sum;
> +}

And I would rather check if we have similar algoritms already in the
kernel which  we might re-use.

> +
> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> +{
> +       struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> +

> +       if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)

I'm not sure about strings operation here.
I would rather do like with other magic constants: introduce hex value
and compare it as unsigned integer.

Also, it might be a warning, since \0 wasn't ever checked from the
string literal. Though, I'm not sure if it applicable to strncmp()
function (it's for strncpy for sure).

> +               return NULL;
> +
> +       if (checksum(addr, eps->length) != 0)
> +               return NULL;
> +
> +       return eps;
> +}
> +
> +static int dcdbas_check_wsmt(void)
> +{
> +       struct acpi_table_wsmt *wsmt = NULL;
> +       struct smm_eps_table *eps = NULL;
> +       u8 *addr;
> +
> +       acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
> +       if (!wsmt)
> +               return 0;
> +
> +       /* Check if WSMT ACPI table shows that protection is enabled */
> +       if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> +           || !(wsmt->protection_flags
> +                & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> +               return 0;
> +
> +       /* Scan for EPS (entry point structure) */
> +       for (addr = (u8 *)__va(0xf0000);
> +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)) && !eps;

Perhaps better to do

for (...) {
 eps = ...();
 if (eps)
  break;
}

Also I've a feeling that 0xf0000 constant is defined already somewhere
under arch/x86/include/asm or evem include/linux.

> +            addr += 1)

+= 1?!
All tables I saw in BIOS are aligned with 16 bytes. Is it the case here?

Is there any other means to check if the table present? ACPI code?
Method / variable?

> +               eps = check_eps_table(addr);
> +
> +       if (!eps) {
> +               dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
> +               return -ENODEV;
> +       }
> +
> +       /*
> +        * Get physical address of buffer and map to virtual address.
> +        * Table gives size in 4K pages, regardless of actual system page size.
> +        */

> +       if (eps->smm_comm_buff_addr + 8 > U32_MAX) {

if (upper_32_bits(..._addr + 8)) {

?

> +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is above 4GB\n");
> +               return -EINVAL;
> +       }
> +       eps_buffer = (u8 *)memremap(eps->smm_comm_buff_addr,

Why casting?

> +                                    eps->num_of_4k_pages * 4096, MEMREMAP_WB);

This multiplication looks strange. Perhaps use PAGE_SIZE?

> +       if (!eps_buffer) {
> +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS buffer\n");
> +               return -ENOMEM;
> +       }
> +
> +       /* First 8 bytes of buffer is for semaphore */
> +       smi_data_buf_phys_addr = (u32) eps->smm_comm_buff_addr + 8;

lower_32_bits() ?

> +       smi_data_buf = eps_buffer + 8;

> +       smi_data_buf_size = (unsigned long) min(eps->num_of_4k_pages * 4096 - 8,
> +                           (u64) ULONG_MAX);

This is too twisted code. First, it needs explanation.
Second, it might need some refactoring.

(Yes, I got the idea, but would it be better implementation?)

> +       max_smi_data_buf_size = smi_data_buf_size;
> +       wsmt_enabled = true;
> +       dev_info(&dcdbas_pdev->dev,
> +                "WSMT found, using firmware-provided SMI buffer.\n");
> +       return 1;
> +}

>  #define SMI_CMD_MAGIC                          (0x534D4931)
>
> +#define SMM_EPS_SIG                            "$SCB"

Just integer like above and put the sting as a comment.
(Side note: above magic also looks like string)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Darren Hart]

On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
> > If the WSMT ACPI table is present and indicates that a fixed communication
> > buffer should be used, use the firmware-specified buffer instead of
> > allocating a buffer in memory for communications between the dcdbas driver
> > and firmare.
> 
> >  config DCDBAS
> >         tristate "Dell Systems Management Base Driver"
> > -       depends on X86
> > +       depends on X86 && ACPI


> 
> Hmm... I'm not sure about this dependency.
> So, the question is do all users actually need this? How did it work
> previously? How has this been tested in case when command line has
> "acpi=off" (yes, this one just for sake of test, I don't believe it's
> a real use case)?

Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
driver which intersected with this one.... this needs to be thoroughly
covered, tested, and thought through. Linus was.... generous in the
number of attempts it took us to get that right.

Did DCDBAS ever work on a system without ACPI?

> 
> >  #include <linux/string.h>
> >  #include <linux/types.h>
> >  #include <linux/mutex.h>
> > +#include <linux/acpi.h>
> 
> Please, try to keep an order as much as possible.
> For example, in given context this line should be before string.h (I
> didn't check the actual file, perhaps even upper).
> 
> >  #include <asm/io.h>
> >
> >  #include "dcdbas.h"
> 
> >                 /* Calling Interface SMI */
> > -               smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> > +               smi_cmd->ebx = smi_data_buf_phys_addr
> > +                               + offsetof(struct smi_cmd, command_buffer);
> 
> Please, keep at least + on the previous line.
> Also, I'm not sure what is the difference now. Especially for previous
> users when this wasn't the part of the driver.
> Some explanation needed.
> 
> > +static u8 checksum(u8 *buffer, u8 length)
> > +{
> > +       u8 sum = 0;
> > +       u8 *end = buffer + length;
> > +
> > +       while (buffer < end)
> > +               sum = (u8)(sum + *(buffer++));
> 
> Why not simple
> 
> sum += *buffer++;
> 
> ?
> 
> > +       return sum;
> > +}
> 
> And I would rather check if we have similar algoritms already in the
> kernel which  we might re-use.

Seems to be some options in lib/checksum.c to check.

> 
> > +
> > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> > +{
> > +       struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> > +
> 
> > +       if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
> 
> I'm not sure about strings operation here.
> I would rather do like with other magic constants: introduce hex value
> and compare it as unsigned integer.
> 
> Also, it might be a warning, since \0 wasn't ever checked from the
> string literal. Though, I'm not sure if it applicable to strncmp()
> function (it's for strncpy for sure).

I think we're OK here, and we're being consistent with the
dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
one or something else, but doing it in HEX ended up being more
confusing. The \0 isn't an issue since strncmp will only compare the n
(4) bytes.

> 
> > +               return NULL;
> > +
> > +       if (checksum(addr, eps->length) != 0)
> > +               return NULL;
> > +
> > +       return eps;
> > +}
> > +
> > +static int dcdbas_check_wsmt(void)
> > +{
> > +       struct acpi_table_wsmt *wsmt = NULL;
> > +       struct smm_eps_table *eps = NULL;
> > +       u8 *addr;
> > +
> > +       acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
> > +       if (!wsmt)
> > +               return 0;
> > +
> > +       /* Check if WSMT ACPI table shows that protection is enabled */
> > +       if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> > +           || !(wsmt->protection_flags
> > +                & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> > +               return 0;
> > +
> > +       /* Scan for EPS (entry point structure) */
> > +       for (addr = (u8 *)__va(0xf0000);
> > +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)) && !eps;
> 
> Perhaps better to do
> 
> for (...) {
>  eps = ...();
>  if (eps)
>   break;
> }
> 
> Also I've a feeling that 0xf0000 constant is defined already somewhere
> under arch/x86/include/asm or evem include/linux.

But... is it defined for this purpose? If not, I'd prefer it hardcoded
(or with a DEFINE).

> 
> > +            addr += 1)
> 
> += 1?!
> All tables I saw in BIOS are aligned with 16 bytes. Is it the case here?
> 
> Is there any other means to check if the table present? ACPI code?
> Method / variable?
> 
> > +               eps = check_eps_table(addr);
> > +
> > +       if (!eps) {
> > +               dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       /*
> > +        * Get physical address of buffer and map to virtual address.
> > +        * Table gives size in 4K pages, regardless of actual system page size.
> > +        */
> 
> > +       if (eps->smm_comm_buff_addr + 8 > U32_MAX) {
> 
> if (upper_32_bits(..._addr + 8)) {
> 
> ?
> 
> > +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is above 4GB\n");
> > +               return -EINVAL;
> > +       }
> > +       eps_buffer = (u8 *)memremap(eps->smm_comm_buff_addr,
> 
> Why casting?
> 
> > +                                    eps->num_of_4k_pages * 4096, MEMREMAP_WB);
> 
> This multiplication looks strange. Perhaps use PAGE_SIZE?
> 
> > +       if (!eps_buffer) {
> > +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS buffer\n");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       /* First 8 bytes of buffer is for semaphore */
> > +       smi_data_buf_phys_addr = (u32) eps->smm_comm_buff_addr + 8;
> 
> lower_32_bits() ?
> 
> > +       smi_data_buf = eps_buffer + 8;
> 
> > +       smi_data_buf_size = (unsigned long) min(eps->num_of_4k_pages * 4096 - 8,
> > +                           (u64) ULONG_MAX);
> 
> This is too twisted code. First, it needs explanation.
> Second, it might need some refactoring.
> 
> (Yes, I got the idea, but would it be better implementation?)
> 
> > +       max_smi_data_buf_size = smi_data_buf_size;
> > +       wsmt_enabled = true;
> > +       dev_info(&dcdbas_pdev->dev,
> > +                "WSMT found, using firmware-provided SMI buffer.\n");
> > +       return 1;
> > +}
> 
> >  #define SMI_CMD_MAGIC                          (0x534D4931)
> >
> > +#define SMM_EPS_SIG                            "$SCB"
> 
> Just integer like above and put the sting as a comment.
> (Side note: above magic also looks like string)

Given the above, I think we can use the more recognizable string - since
that is clearly how they think of this label.

Otherwise, agree with a follow-up to all of Andy's feedback.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Andy Shevchenko]

On Sat, Jun 9, 2018 at 4:04 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
>> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:

>> > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
>> > +{
>> > +       struct smm_eps_table *eps = (struct smm_eps_table *)addr;
>> > +
>>
>> > +       if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
>>
>> I'm not sure about strings operation here.
>> I would rather do like with other magic constants: introduce hex value
>> and compare it as unsigned integer.
>>
>> Also, it might be a warning, since \0 wasn't ever checked from the
>> string literal. Though, I'm not sure if it applicable to strncmp()
>> function (it's for strncpy for sure).
>
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.

Yeah, consistency is usually a priority over style. Though in the
context below I see also magic numbers instead of strings.
I have no strong opinion here which one is better to follow.

>> Also I've a feeling that 0xf0000 constant is defined already somewhere
>> under arch/x86/include/asm or evem include/linux.
>
> But... is it defined for this purpose? If not, I'd prefer it hardcoded
> (or with a DEFINE).

OK, I did a research, and the following is the result.

arch/x86/kernel/mpparse.c:636:      smp_scan_config(0xF0000, 0x10000))
arch/x86/kernel/probe_roms.c:27:        .start  = 0xf0000,

arch/x86/pci/irq.c:91: *  Search 0xf0000 -- 0xfffff for the PCI IRQ
Routing Table.
arch/x86/pci/irq.c:105: for (addr = (u8 *) __va(0xf0000); addr < (u8
*) __va(0x100000); addr += 16) {

arch/x86/platform/geode/alix.c:34:#define BIOS_SIGNATURE_TINYBIOS
         0xf0000
arch/x86/platform/olpc/olpc-xo1-pm.c:36:} ofw_bios_entry = { 0xF0000 +
PAGE_OFFSET, __KERNEL_CS };
arch/x86/platform/ts5500/ts5500.c:103:  bios = ioremap(0xf0000, 0x10000);

Perhaps it would be a good idea to introduce such constant in the future.

Note mpparse.c and irq.c, that's similar purpose of the address range.

>> >  #define SMI_CMD_MAGIC                          (0x534D4931)
>> >
>> > +#define SMM_EPS_SIG                            "$SCB"
>>
>> Just integer like above and put the sting as a comment.
>> (Side note: above magic also looks like string)
>
> Given the above, I think we can use the more recognizable string - since
> that is clearly how they think of this label.

OK with me!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
v2 Bumped driver version to 5.6.0-3.3
v3 Removed dependency on ACPI in Kconfig
   Moved the added #include to be in alphabetical order
   Added comments in smi_request_store()
   Simplified checksum code
   Changed loop searching 0xf0000 to be more readable
   Reworked calculation of remap_size & smi_data_buf_size


 drivers/firmware/dcdbas.c | 118 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/firmware/dcdbas.h |  10 ++++
 2 files changed, 122 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..e95dc9aee2fa 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include <linux/platform_device.h>
+#include <linux/acpi.h>
 #include <linux/dma-mapping.h>
 #include <linux/errno.h>
 #include <linux/cpu.h>
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME		"dcdbas"
-#define DRIVER_VERSION		"5.6.0-3.2"
+#define DRIVER_VERSION		"5.6.0-3.3"
 #define DRIVER_DESCRIPTION	"Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-	if (!smi_data_buf)
+	if (!smi_data_buf || wsmt_enabled)
 		return;
 
 	dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
 	if (smi_data_buf_size >= size)
 		return 0;
 
-	if (size > MAX_SMI_DATA_BUF_SIZE)
+	if (size > max_smi_data_buf_size)
 		return -EINVAL;
 
 	/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct kobject *kobj,
 {
 	ssize_t ret;
 
-	if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+	if ((pos + count) > max_smi_data_buf_size)
 		return -EINVAL;
 
 	mutex_lock(&smi_data_lock);
@@ -322,8 +327,14 @@ static ssize_t smi_request_store(struct device *dev,
 			ret = count;
 		break;
 	case 1:
-		/* Calling Interface SMI */
-		smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+		/* Calling Interface SMI
+		 *
+		 * Provide physical address of command buffer field within
+		 * the struct smi_cmd... can't use virt_to_phys on smi_cmd
+		 * because address may be from memremap.
+		 */
+		smi_cmd->ebx = smi_data_buf_phys_addr +
+				offsetof(struct smi_cmd, command_buffer);
 		ret = dcdbas_smi_request(smi_cmd);
 		if (!ret)
 			ret = count;
@@ -482,6 +493,94 @@ static void dcdbas_host_control(void)
 	}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+	u8 sum = 0;
+	u8 *end = buffer + length;
+
+	while (buffer < end)
+		sum += *buffer++;
+	return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+	struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+	if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+		return NULL;
+
+	if (checksum(addr, eps->length) != 0)
+		return NULL;
+
+	return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+	struct acpi_table_wsmt *wsmt = NULL;
+	struct smm_eps_table *eps = NULL;
+	u64 remap_size;
+	u8 *addr;
+
+	acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
+	if (!wsmt)
+		return 0;
+
+	/* Check if WSMT ACPI table shows that protection is enabled */
+	if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
+	    || !(wsmt->protection_flags
+		 & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+		return 0;
+
+	/* Scan for EPS (entry point structure) */
+	for (addr = (u8 *)__va(0xf0000);
+	     addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
+	     addr += 1) {
+		eps = check_eps_table(addr);
+		if (eps)
+			break;
+	}
+
+	if (!eps) {
+		dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Get physical address of buffer and map to virtual address.
+	 * Table gives size in 4K pages, regardless of actual system page size.
+	 */
+	if (upper_32_bits(eps->smm_comm_buff_addr + 8)) {
+		dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is above 4GB\n");
+		return -EINVAL;
+	}
+	/*
+	 * Limit remap size to MAX_SMI_DATA_BUF_SIZE + 8 (since the first 8
+	 * bytes are used for a semaphore, not the data buffer itself).
+	 */
+	remap_size = eps->num_of_4k_pages * PAGE_SIZE;
+	if (remap_size > MAX_SMI_DATA_BUF_SIZE + 8)
+		remap_size = MAX_SMI_DATA_BUF_SIZE + 8;
+	eps_buffer = memremap(eps->smm_comm_buff_addr, remap_size, MEMREMAP_WB);
+	if (!eps_buffer) {
+		dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS buffer\n");
+		return -ENOMEM;
+	}
+
+	/* First 8 bytes is for a semaphore, not part of the smi_data_buf */
+	smi_data_buf_phys_addr = eps->smm_comm_buff_addr + 8;
+	smi_data_buf = eps_buffer + 8;
+	smi_data_buf_size = remap_size - 8;
+	max_smi_data_buf_size = smi_data_buf_size;
+	wsmt_enabled = true;
+	dev_info(&dcdbas_pdev->dev,
+		 "WSMT found, using firmware-provided SMI buffer.\n");
+	return 1;
+}
+
 /**
  * dcdbas_reboot_notify: handle reboot notification for host control
  */
@@ -548,6 +647,11 @@ static int dcdbas_probe(struct platform_device *dev)
 
 	dcdbas_pdev = dev;
 
+	/* Check if ACPI WSMT table specifies protected SMI buffer address */
+	error = dcdbas_check_wsmt();
+	if (error < 0)
+		return error;
+
 	/*
 	 * BIOS SMI calls require buffer addresses be in 32-bit address space.
 	 * This is done by setting the DMA mask below.
@@ -635,6 +739,8 @@ static void __exit dcdbas_exit(void)
 	 */
 	if (dcdbas_pdev)
 		smi_data_buf_free();
+	if (eps_buffer)
+		memunmap(eps_buffer);
 	platform_device_unregister(dcdbas_pdev_reg);
 	platform_driver_unregister(&dcdbas_driver);
 }
diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h
index ca3cb0a54ab6..52729a494b00 100644
--- a/drivers/firmware/dcdbas.h
+++ b/drivers/firmware/dcdbas.h
@@ -53,6 +53,7 @@
 #define EXPIRED_TIMER				(0)
 
 #define SMI_CMD_MAGIC				(0x534D4931)
+#define SMM_EPS_SIG				"$SCB"
 
 #define DCDBAS_DEV_ATTR_RW(_name) \
 	DEVICE_ATTR(_name,0600,_name##_show,_name##_store);
@@ -103,5 +104,14 @@ struct apm_cmd {
 
 int dcdbas_smi_request(struct smi_cmd *smi_cmd);
 
+struct smm_eps_table {
+	char smm_comm_buff_anchor[4];
+	u8 length;
+	u8 checksum;
+	u8 version;
+	u64 smm_comm_buff_addr;
+	u64 num_of_4k_pages;
+} __packed;
+
 #endif /* _DCDBAS_H_ */
 
-- 
2.14.2

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Andy Shevchenko]

On Wed, Jun 13, 2018 at 4:24 AM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>
> If the WSMT ACPI table is present and indicates that a fixed communication
> buffer should be used, use the firmware-specified buffer instead of
> allocating a buffer in memory for communications between the dcdbas driver
> and firmare.

Thanks for an update. My comments below.

> -       if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
> +       if ((pos + count) > max_smi_data_buf_size)
>                 return -EINVAL;

Parens are redundant, but okay, not purpose of the change.

> +               /* Calling Interface SMI

I suppose the style of the comments like
/*
 * Calling ...
...

> +                *
> +                * Provide physical address of command buffer field within
> +                * the struct smi_cmd... can't use virt_to_phys on smi_cmd
> +                * because address may be from memremap.

Wait, memremap() might return a virtual address. How we be sure that
we got still physical address here?

(Also note () when referring to functions)

> +                */
> +               smi_cmd->ebx = smi_data_buf_phys_addr +
> +                               offsetof(struct smi_cmd, command_buffer);

Btw, can it be one line (~83 character are okay for my opinion).

> +       acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
> +       if (!wsmt)
> +               return 0;
> +
> +       /* Check if WSMT ACPI table shows that protection is enabled */
> +       if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)

> +           || !(wsmt->protection_flags
> +                & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))

Better to indent like

if (... ||
 !(... & ...)

> +               return 0;
> +
> +       /* Scan for EPS (entry point structure) */
> +       for (addr = (u8 *)__va(0xf0000);
> +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));

> +            addr += 1) {

This wasn't commented IIRC and changed. So, why?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
>> +               /* Calling Interface SMI
> 
> I suppose the style of the comments like
> /*
>  * Calling ...
> ...

Yes... goof on my part.  Thanks.

>> +                *
>> +                * Provide physical address of command buffer field within
>> +                * the struct smi_cmd... can't use virt_to_phys on smi_cmd
>> +                * because address may be from memremap.
> 
> Wait, memremap() might return a virtual address. How we be sure that
> we got still physical address here?
> 
> (Also note () when referring to functions)
>
>> +                */
>> +               smi_cmd->ebx = smi_data_buf_phys_addr +
>> +                               offsetof(struct smi_cmd, command_buffer);
> 
> Btw, can it be one line (~83 character are okay for my opinion).
> 

Before this patch, the address in smi_cmd always came from an alloc, so
virt_to_phys() was used to get the physical address here.  With WSMT, we
could be using a BIOS-provided buffer for SMI, in which case the address in
smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
So instead I changed this to use the physical address of smi_data_buf that
is stored in smi_data_buf_phys_addr, which will be valid regardless of how
the address of smi_data_buf was generated.

But that would be like 97 characters long if I made it all one line...

>> +       acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
>> +       if (!wsmt)
>> +               return 0;
>> +
>> +       /* Check if WSMT ACPI table shows that protection is enabled */
>> +       if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> 
>> +           || !(wsmt->protection_flags
>> +                & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> 
> Better to indent like
> 
> if (... ||
>  !(... & ...)
> 

Yes thanks.

>> +               return 0;
>> +
>> +       /* Scan for EPS (entry point structure) */
>> +       for (addr = (u8 *)__va(0xf0000);
>> +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
> 
>> +            addr += 1) {
> 
> This wasn't commented IIRC and changed. So, why?
> 

I changed this is response to your earlier comment (7 june)... you had pointed
out that it would be better if I put an "if (eps) break;" inside the for loop
instead of having "&& !eps" in the condition of the for loop.  I put the note
"Changed loop searching 0xf0000 to be more readable" in the list of changes for
patch version v3 to cover this change.

Thanks again for your time!

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Andy Shevchenko]

On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
> On 6/13/2018 3:54 AM, Andy Shevchenko wrote:

>>> +                * Provide physical address of command buffer field within
>>> +                * the struct smi_cmd... can't use virt_to_phys on smi_cmd
>>> +                * because address may be from memremap.
>>
>> Wait, memremap() might return a virtual address. How we be sure that
>> we got still physical address here?

> Before this patch, the address in smi_cmd always came from an alloc, so
> virt_to_phys() was used to get the physical address here.  With WSMT, we
> could be using a BIOS-provided buffer for SMI, in which case the address in
> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
> So instead I changed this to use the physical address of smi_data_buf that
> is stored in smi_data_buf_phys_addr, which will be valid regardless of how
> the address of smi_data_buf was generated.

Yes, but what does guarantee that memremap() will return you still
physical address?

>>> +               return 0;
>>> +
>>> +       /* Scan for EPS (entry point structure) */
>>> +       for (addr = (u8 *)__va(0xf0000);
>>> +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
>>
>>> +            addr += 1) {
>>
>> This wasn't commented IIRC and changed. So, why?

> I changed this is response to your earlier comment (7 june)... you had pointed
> out that it would be better if I put an "if (eps) break;" inside the for loop
> instead of having "&& !eps" in the condition of the for loop.  I put the note
> "Changed loop searching 0xf0000 to be more readable" in the list of changes for
> patch version v3 to cover this change.

Thanks, but here I meant += 1 vs += 16 step.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/14/2018 12:25 PM, Andy Shevchenko wrote:
> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
> 
>>>> +                * Provide physical address of command buffer field within
>>>> +                * the struct smi_cmd... can't use virt_to_phys on smi_cmd
>>>> +                * because address may be from memremap.
>>>
>>> Wait, memremap() might return a virtual address. How we be sure that
>>> we got still physical address here?
> 
>> Before this patch, the address in smi_cmd always came from an alloc, so
>> virt_to_phys() was used to get the physical address here.  With WSMT, we
>> could be using a BIOS-provided buffer for SMI, in which case the address in
>> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
>> So instead I changed this to use the physical address of smi_data_buf that
>> is stored in smi_data_buf_phys_addr, which will be valid regardless of how
>> the address of smi_data_buf was generated.
> 
> Yes, but what does guarantee that memremap() will return you still
> physical address?
> 

Sorry, I'm not sure I understand the question.

Up to now, this driver always just allocated a buffer from main memory that
it used to send/receive information from BIOS when it generated a SMI.  That's
what smi_cmd points to where this comment is.  And it was safe to use
virt_to_phys() on this address.

With this patch, though, the driver may now be using a buffer that isn't part
of main memory--it could now be using a buffer that BIOS provided the physical
address for, and this would not be part of main memory.  So smi_cmd may contain
a virtual address that memremap() provided.  And because memremap() is just
like ioremap(), the driver can no longer use virt_to_phys(smi_cmd) to get the
physical address of the buffer.

My comment is just pointing that out... I was trying to say, "the code can't
use virt_to_phys(smi_cmd) to get the virtual address here".

memremap() should always return a virtual address that points to the physical
address we send it (unless it fails of course).


>>>> +               return 0;
>>>> +
>>>> +       /* Scan for EPS (entry point structure) */
>>>> +       for (addr = (u8 *)__va(0xf0000);
>>>> +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
>>>
>>>> +            addr += 1) {
>>>
>>> This wasn't commented IIRC and changed. So, why?
> 
>> I changed this is response to your earlier comment (7 june)... you had pointed
>> out that it would be better if I put an "if (eps) break;" inside the for loop
>> instead of having "&& !eps" in the condition of the for loop.  I put the note
>> "Changed loop searching 0xf0000 to be more readable" in the list of changes for
>> patch version v3 to cover this change.
> 
> Thanks, but here I meant += 1 vs += 16 step.
> 

Sorry, I thought I had answered this earlier.  The spec does not say that the EPS
table will be on a 16-byte boundary.  And I just added a printk in this driver to
see where it is on the system I had at hand, and it isn't on a 16-byte boundary:

[ 4680.192542] dcdbas - EPS table at 000000005761efb7
[ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI buffer.
[ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version 5.6.0-3.3)

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Andy Shevchenko]

On Fri, Jun 15, 2018 at 1:31 AM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
> On 6/14/2018 12:25 PM, Andy Shevchenko wrote:
>> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
>>
>>>>> +                * Provide physical address of command buffer field within
>>>>> +                * the struct smi_cmd... can't use virt_to_phys on smi_cmd
>>>>> +                * because address may be from memremap.
>>>>
>>>> Wait, memremap() might return a virtual address. How we be sure that
>>>> we got still physical address here?
>>
>>> Before this patch, the address in smi_cmd always came from an alloc, so
>>> virt_to_phys() was used to get the physical address here.  With WSMT, we
>>> could be using a BIOS-provided buffer for SMI, in which case the address in
>>> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
>>> So instead I changed this to use the physical address of smi_data_buf that
>>> is stored in smi_data_buf_phys_addr, which will be valid regardless of how
>>> the address of smi_data_buf was generated.
>>
>> Yes, but what does guarantee that memremap() will return you still
>> physical address?

> Sorry, I'm not sure I understand the question.
>
> Up to now, this driver always just allocated a buffer from main memory that
> it used to send/receive information from BIOS when it generated a SMI.  That's
> what smi_cmd points to where this comment is.  And it was safe to use
> virt_to_phys() on this address.
>
> With this patch, though, the driver may now be using a buffer that isn't part
> of main memory--it could now be using a buffer that BIOS provided the physical
> address for, and this would not be part of main memory.

Hmm... But is it CPU address or bus address what BIOS provides?

If it's a CPU address why do you need to call memremap() on it in the
first place?
I could guess that you want to access it from CPU side and rather
would get faults.

>  So smi_cmd may contain
> a virtual address that memremap() provided.  And because memremap() is just
> like ioremap(), the driver can no longer use virt_to_phys(smi_cmd) to get the
> physical address of the buffer.

Yes, and ioremap() is dedicated for the resources that are not
available directly by the memory accesses, but rather require some bus
transactions (like MMIO).

>
> My comment is just pointing that out... I was trying to say, "the code can't
> use virt_to_phys(smi_cmd) to get the virtual address here".
>

Please, add bits from above paragraphs to elaborate this in the comment.

> memremap() should always return a virtual address that points to the physical
> address we send it (unless it fails of course).

>>>>> +       /* Scan for EPS (entry point structure) */
>>>>> +       for (addr = (u8 *)__va(0xf0000);
>>>>> +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
>>>>
>>>>> +            addr += 1) {
>>>>
>>>> This wasn't commented IIRC and changed. So, why?
>>
>>> I changed this is response to your earlier comment (7 june)... you had pointed
>>> out that it would be better if I put an "if (eps) break;" inside the for loop
>>> instead of having "&& !eps" in the condition of the for loop.  I put the note
>>> "Changed loop searching 0xf0000 to be more readable" in the list of changes for
>>> patch version v3 to cover this change.
>>
>> Thanks, but here I meant += 1 vs += 16 step.
>>
>
> Sorry, I thought I had answered this earlier.  The spec does not say that the EPS
> table will be on a 16-byte boundary.  And I just added a printk in this driver to
> see where it is on the system I had at hand, and it isn't on a 16-byte boundary:

Oh, that's sad.

Btw, does XSDT have a link to this table?

> [ 4680.192542] dcdbas - EPS table at 000000005761efb7

Can you, by the way, dump some bytes around this address, using
print_hex_dump_bytes();

where the adrress is aligned to let say 32 byte boundary and size like 64 bytes?

> [ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI buffer.
> [ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version 5.6.0-3.3)

OK, now the most important question, did you investigate "SMM
Communication ACPI Table"?
Can you utilize information in it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 06/27/2018 06:52 PM, Andy Shevchenko wrote:
> On Fri, Jun 15, 2018 at 1:31 AM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>> On 6/14/2018 12:25 PM, Andy Shevchenko wrote:
>>> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>>>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
>>>
>>>>>> +                * Provide physical address of command buffer field within
>>>>>> +                * the struct smi_cmd... can't use virt_to_phys on smi_cmd
>>>>>> +                * because address may be from memremap.
>>>>>
>>>>> Wait, memremap() might return a virtual address. How we be sure that
>>>>> we got still physical address here?
>>>
>>>> Before this patch, the address in smi_cmd always came from an alloc, so
>>>> virt_to_phys() was used to get the physical address here.  With WSMT, we
>>>> could be using a BIOS-provided buffer for SMI, in which case the address in
>>>> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
>>>> So instead I changed this to use the physical address of smi_data_buf that
>>>> is stored in smi_data_buf_phys_addr, which will be valid regardless of how
>>>> the address of smi_data_buf was generated.
>>>
>>> Yes, but what does guarantee that memremap() will return you still
>>> physical address?
> 
>> Sorry, I'm not sure I understand the question.
>>
>> Up to now, this driver always just allocated a buffer from main memory that
>> it used to send/receive information from BIOS when it generated a SMI.  That's
>> what smi_cmd points to where this comment is.  And it was safe to use
>> virt_to_phys() on this address.
>>
>> With this patch, though, the driver may now be using a buffer that isn't part
>> of main memory--it could now be using a buffer that BIOS provided the physical
>> address for, and this would not be part of main memory.
> 
> Hmm... But is it CPU address or bus address what BIOS provides?
> 
> If it's a CPU address why do you need to call memremap() on it in the
> first place?
> I could guess that you want to access it from CPU side and rather
> would get faults.
> 

The BIOS-provided EPS provides the physical (bus) address of the fixed SMI buffer.
This memory will be of type EfiReservedMemoryType, so it will not be usable RAM
to the linux kernel and won't already be mapped.

>>  So smi_cmd may contain
>> a virtual address that memremap() provided.  And because memremap() is just
>> like ioremap(), the driver can no longer use virt_to_phys(smi_cmd) to get the
>> physical address of the buffer.
> 
> Yes, and ioremap() is dedicated for the resources that are not
> available directly by the memory accesses, but rather require some bus
> transactions (like MMIO)>>
>> My comment is just pointing that out... I was trying to say, "the code can't
>> use virt_to_phys(smi_cmd) to get the virtual address here".
>>
> 
> Please, add bits from above paragraphs to elaborate this in the comment.
> 

No problem, will do.

>> memremap() should always return a virtual address that points to the physical
>> address we send it (unless it fails of course).
> 
>>>>>> +       /* Scan for EPS (entry point structure) */
>>>>>> +       for (addr = (u8 *)__va(0xf0000);
>>>>>> +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
>>>>>
>>>>>> +            addr += 1) {
>>>>>
>>>>> This wasn't commented IIRC and changed. So, why?
>>>
>>>> I changed this is response to your earlier comment (7 june)... you had pointed
>>>> out that it would be better if I put an "if (eps) break;" inside the for loop
>>>> instead of having "&& !eps" in the condition of the for loop.  I put the note
>>>> "Changed loop searching 0xf0000 to be more readable" in the list of changes for
>>>> patch version v3 to cover this change.
>>>
>>> Thanks, but here I meant += 1 vs += 16 step.
>>>
>>
>> Sorry, I thought I had answered this earlier.  The spec does not say that the EPS
>> table will be on a 16-byte boundary.  And I just added a printk in this driver to
>> see where it is on the system I had at hand, and it isn't on a 16-byte boundary:
> 
> Oh, that's sad.
> 
> Btw, does XSDT have a link to this table?
> 

The XSDT has a link to the WSMT table, but not to the EPS table that actually has
the address of the buffer that BIOS provides.  The EPS table isn't an ACPI table,
it's just a Dell-defined table.

I did realize that the debug code that printed the address of the EPS table was not
working right, and the table on my system is actually 16-byte aligned.  However,
the documentation on the EPS does not specify that it will be 16-byte aligned, so I
don't think the driver should make that assumption, especially since scanning a 64K
region byte by byte should take very little extra time over scanning every 16th byte.

>> [ 4680.192542] dcdbas - EPS table at 000000005761efb7
> 
> Can you, by the way, dump some bytes around this address, using
> print_hex_dump_bytes();
> 
> where the adrress is aligned to let say 32 byte boundary and size like 64 bytes?
> 

I guess this isn't needed since the table on my system actually is 16-byte aligned.

>> [ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI buffer.
>> [ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version 5.6.0-3.3)
> 
> OK, now the most important question, did you investigate "SMM
> Communication ACPI Table"?
> Can you utilize information in it?
> 

Dell BIOS does not provide a "SMM Communication ACPI Table", just the EPS.  It appears
that the "SMM Communication ACPI Table" is deprecated in the UEFI 2.7 spec.  Also, the
dcdbas driver is for Dell systems only.

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Andy Shevchenko]

On Fri, Jun 29, 2018 at 9:56 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
> On 06/27/2018 06:52 PM, Andy Shevchenko wrote:
>> On Fri, Jun 15, 2018 at 1:31 AM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>>> On 6/14/2018 12:25 PM, Andy Shevchenko wrote:
>>>> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>>>>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote:

>>>> Thanks, but here I meant += 1 vs += 16 step.

> I did realize that the debug code that printed the address of the EPS table was not
> working right, and the table on my system is actually 16-byte aligned.  However,
> the documentation on the EPS does not specify that it will be 16-byte aligned, so I
> don't think the driver should make that assumption, especially since scanning a 64K
> region byte by byte should take very little extra time over scanning every 16th byte.

> I guess this isn't needed since the table on my system actually is 16-byte aligned.

I think the 16 byte is a natural alignment applicable to all tables like a such.
I would rather go with it until we will get a (weird) BIOS which does
not support that.

(I believe this alignment just comes by definition of C ABIs for the
structures / types. So, each defined type is located on an aligned
boundaries)

>>> [ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI buffer.
>>> [ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version 5.6.0-3.3)
>>
>> OK, now the most important question, did you investigate "SMM
>> Communication ACPI Table"?
>> Can you utilize information in it?

> Dell BIOS does not provide a "SMM Communication ACPI Table", just the EPS.  It appears
> that the "SMM Communication ACPI Table" is deprecated in the UEFI 2.7 spec.  Also, the
> dcdbas driver is for Dell systems only.

> The EPS table isn't an ACPI table,
> it's just a Dell-defined table.

Hmm... It's either strange or has some background why is so.
OK it has a note that there is no use case, but here it is!

Mario, can we consider this as a BIOS bug which doesn't provide SMM
Communication ACPI table?

Can it be escalated on UEFI committee?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Mario.Limonciello]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Friday, June 29, 2018 2:30 PM
> To: Stuart Hayes
> Cc: Darren Hart; Limonciello, Mario; Linux Kernel Mailing List; Platform Driver
> Subject: Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table
> 
> On Fri, Jun 29, 2018 at 9:56 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
> > On 06/27/2018 06:52 PM, Andy Shevchenko wrote:
> >> On Fri, Jun 15, 2018 at 1:31 AM, Stuart Hayes <stuart.w.hayes@gmail.com>
> wrote:
> >>> On 6/14/2018 12:25 PM, Andy Shevchenko wrote:
> >>>> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com>
> wrote:
> >>>>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
> 
> >>>> Thanks, but here I meant += 1 vs += 16 step.
> 
> > I did realize that the debug code that printed the address of the EPS table was not
> > working right, and the table on my system is actually 16-byte aligned.  However,
> > the documentation on the EPS does not specify that it will be 16-byte aligned, so I
> > don't think the driver should make that assumption, especially since scanning a
> 64K
> > region byte by byte should take very little extra time over scanning every 16th
> byte.
> 
> > I guess this isn't needed since the table on my system actually is 16-byte aligned.
> 
> I think the 16 byte is a natural alignment applicable to all tables like a such.
> I would rather go with it until we will get a (weird) BIOS which does
> not support that.
> 
> (I believe this alignment just comes by definition of C ABIs for the
> structures / types. So, each defined type is located on an aligned
> boundaries)
> 
> >>> [ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI
> buffer.
> >>> [ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version
> 5.6.0-3.3)
> >>
> >> OK, now the most important question, did you investigate "SMM
> >> Communication ACPI Table"?
> >> Can you utilize information in it?
> 
> > Dell BIOS does not provide a "SMM Communication ACPI Table", just the EPS.  It
> appears
> > that the "SMM Communication ACPI Table" is deprecated in the UEFI 2.7 spec.
> Also, the
> > dcdbas driver is for Dell systems only.
> 
> > The EPS table isn't an ACPI table,
> > it's just a Dell-defined table.
> 
> Hmm... It's either strange or has some background why is so.
> OK it has a note that there is no use case, but here it is!
> 
> Mario, can we consider this as a BIOS bug which doesn't provide SMM
> Communication ACPI table?
> 
> Can it be escalated on UEFI committee?
> 

I don't believe SMM communication ACPI table has ever been implemented by Dell
on server or client BIOS.  I do agree this table describes the behavior that DCDBAS driver
has used since before even UEFI BIOS pretty accurately.

Stuart and I did discuss with server BIOS (who uses this EPS mechanism) to see if its possible
to move EPS to SMM communication ACPI table however since it's been deprecated by
UEFI 2.7 they weren't willing to adopt it.

Stuart, anything else you want to add here?

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Andy Shevchenko]

On Mon, Jul 2, 2018 at 5:07 PM,  <Mario.Limonciello@dell.com> wrote:

>> >> OK, now the most important question, did you investigate "SMM
>> >> Communication ACPI Table"?
>> >> Can you utilize information in it?
>>
>> > Dell BIOS does not provide a "SMM Communication ACPI Table", just the EPS.  It
>> appears
>> > that the "SMM Communication ACPI Table" is deprecated in the UEFI 2.7 spec.
>> Also, the
>> > dcdbas driver is for Dell systems only.
>>
>> > The EPS table isn't an ACPI table,
>> > it's just a Dell-defined table.
>>
>> Hmm... It's either strange or has some background why is so.
>> OK it has a note that there is no use case, but here it is!
>>
>> Mario, can we consider this as a BIOS bug which doesn't provide SMM
>> Communication ACPI table?
>>
>> Can it be escalated on UEFI committee?

> I don't believe SMM communication ACPI table has ever been implemented by Dell
> on server or client BIOS.  I do agree this table describes the behavior that DCDBAS driver
> has used since before even UEFI BIOS pretty accurately.

So, EPS table has been for ages in Dell machines?
Can we consider it as a predecessor of that SMM communication ACPI table?

> Stuart and I did discuss with server BIOS (who uses this EPS mechanism) to see if its possible
> to move EPS to SMM communication ACPI table however since it's been deprecated by
> UEFI 2.7 they weren't willing to adopt it.

It's pity, but the motivation to deprecate is "lack of use" which is
not true. That's why I would suggest to escalate this to UEFI
committee.

> Stuart, anything else you want to add here?

Darren, what's your opinion about this?

P.S. I'm not against this approach (just some technical comments I
already shared), but on the other hand it would be nice to have undo
that deprecation and follow the standard in new firmwares.
Would you agree?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Mario.Limonciello]

> 
> > I don't believe SMM communication ACPI table has ever been implemented by
> Dell
> > on server or client BIOS.  I do agree this table describes the behavior that DCDBAS
> driver
> > has used since before even UEFI BIOS pretty accurately.
> 
> So, EPS table has been for ages in Dell machines?
> Can we consider it as a predecessor of that SMM communication ACPI table?

No, EPS is new this year, specifically for server BIOS to be able to support SMM communication
when WSMT is enabled.  The code tests in Stuart's patch will detect if WSMT is enabled
and if it's enabled test if EPS was defined.  On server BIOS when EPS is defined dcdbas
will be able to communicate using addresses defined in EPS.

Server BIOS will support EPS for applications using dcdbas interface and may at a later time
introduce same WMI interface as client too (but applications will need time to update so
they need to support both).

Actually Stuart's patch will cause client BIOS that has WSMT enabled make dcdbas fail
initialization (as it should because dcdbas doesn't have a region that it can successfully
communicate).  

In client machines we moved this communication to ACPI buffer allocated by WMI, which
is why we have dell-smbios-wmi now in kernel.

I think once some variation of Stuart's patch is merged, I'll send a follow
up patch to drop this test because it's no longer necessary:
https://github.com/torvalds/linux/blob/master/drivers/platform/x86/dell-smbios-smm.c#L106


> 
> > Stuart and I did discuss with server BIOS (who uses this EPS mechanism) to see if
> its possible
> > to move EPS to SMM communication ACPI table however since it's been
> deprecated by
> > UEFI 2.7 they weren't willing to adopt it.
> 
> It's pity, but the motivation to deprecate is "lack of use" which is
> not true. That's why I would suggest to escalate this to UEFI
> committee.
> 
> > Stuart, anything else you want to add here?
> 
> Darren, what's your opinion about this?
> 
> P.S. I'm not against this approach (just some technical comments I
> already shared), but on the other hand it would be nice to have undo
> that deprecation and follow the standard in new firmwares.
> Would you agree?

Sure.  Due to the timing of how long this will take, even if SMM communication
ACPI table is undone from deprecation we may have to still support both EPS
and SMM communication ACPI table though (maybe it would be order of preference).

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 7/2/2018 11:15 AM, Mario.Limonciello@dell.com wrote:
>>
>>> I don't believe SMM communication ACPI table has ever been implemented by
>> Dell
>>> on server or client BIOS.  I do agree this table describes the behavior that DCDBAS
>> driver
>>> has used since before even UEFI BIOS pretty accurately.
>>
>> So, EPS table has been for ages in Dell machines?
>> Can we consider it as a predecessor of that SMM communication ACPI table?
> 
> No, EPS is new this year, specifically for server BIOS to be able to support SMM communication
> when WSMT is enabled.  The code tests in Stuart's patch will detect if WSMT is enabled
> and if it's enabled test if EPS was defined.  On server BIOS when EPS is defined dcdbas
> will be able to communicate using addresses defined in EPS.
> 
> Server BIOS will support EPS for applications using dcdbas interface and may at a later time
> introduce same WMI interface as client too (but applications will need time to update so
> they need to support both).
> 
> Actually Stuart's patch will cause client BIOS that has WSMT enabled make dcdbas fail
> initialization (as it should because dcdbas doesn't have a region that it can successfully
> communicate).  
> 
> In client machines we moved this communication to ACPI buffer allocated by WMI, which
> is why we have dell-smbios-wmi now in kernel.
> 
> I think once some variation of Stuart's patch is merged, I'll send a follow
> up patch to drop this test because it's no longer necessary:
> https://github.com/torvalds/linux/blob/master/drivers/platform/x86/dell-smbios-smm.c#L106
> 
> 
>>
>>> Stuart and I did discuss with server BIOS (who uses this EPS mechanism) to see if
>> its possible
>>> to move EPS to SMM communication ACPI table however since it's been
>> deprecated by
>>> UEFI 2.7 they weren't willing to adopt it.
>>
>> It's pity, but the motivation to deprecate is "lack of use" which is
>> not true. That's why I would suggest to escalate this to UEFI
>> committee.
>>
>>> Stuart, anything else you want to add here?
>>
>> Darren, what's your opinion about this?
>>
>> P.S. I'm not against this approach (just some technical comments I
>> already shared), but on the other hand it would be nice to have undo
>> that deprecation and follow the standard in new firmwares.
>> Would you agree?
> 
> Sure.  Due to the timing of how long this will take, even if SMM communication
> ACPI table is undone from deprecation we may have to still support both EPS
> and SMM communication ACPI table though (maybe it would be order of preference).
> 
> 

I have confirmation that the EPS table will be 16-byte aligned, so I can make that
change.  I'll send a v5 with that and the updated comment.

[PATCH v5] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
v2 Bumped driver version to 5.6.0-3.3
v3 Removed dependency on ACPI in Kconfig
   Moved the added #include to be in alphabetical order
   Added comments in smi_request_store()
   Simplified checksum code
   Changed loop searching 0xf0000 to be more readable
   Reworked calculation of remap_size & smi_data_buf_size
v4 Fixed comment that starts with "Calling Interface SMI"
   Fixed formatting of first "if" statement in dcdbas_check_wsmt()
v5 Reworked comment that starts with "Calling Interface SMI"
   Changed EPS scanning loop to check every 16 bytes


 drivers/firmware/dcdbas.c | 123 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/firmware/dcdbas.h |  10 ++++
 2 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..ae28e48ff7dc 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include <linux/platform_device.h>
+#include <linux/acpi.h>
 #include <linux/dma-mapping.h>
 #include <linux/errno.h>
 #include <linux/cpu.h>
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME		"dcdbas"
-#define DRIVER_VERSION		"5.6.0-3.2"
+#define DRIVER_VERSION		"5.6.0-3.3"
 #define DRIVER_DESCRIPTION	"Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-	if (!smi_data_buf)
+	if (!smi_data_buf || wsmt_enabled)
 		return;
 
 	dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
 	if (smi_data_buf_size >= size)
 		return 0;
 
-	if (size > MAX_SMI_DATA_BUF_SIZE)
+	if (size > max_smi_data_buf_size)
 		return -EINVAL;
 
 	/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct kobject *kobj,
 {
 	ssize_t ret;
 
-	if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+	if ((pos + count) > max_smi_data_buf_size)
 		return -EINVAL;
 
 	mutex_lock(&smi_data_lock);
@@ -322,8 +327,20 @@ static ssize_t smi_request_store(struct device *dev,
 			ret = count;
 		break;
 	case 1:
-		/* Calling Interface SMI */
-		smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+		/*
+		 * Calling Interface SMI
+		 *
+		 * Provide physical address of command buffer field within
+		 * the struct smi_cmd to BIOS.
+		 *
+		 * Because the address that smi_cmd (smi_data_buf) points to
+		 * will be from memremap() of a non-memory address if WSMT
+		 * is present, we can't use virt_to_phys() on smi_cmd, so
+		 * we have to use the physical address that was saved when
+		 * the virtual address for smi_cmd was received.
+		 */
+		smi_cmd->ebx = smi_data_buf_phys_addr +
+				offsetof(struct smi_cmd, command_buffer);
 		ret = dcdbas_smi_request(smi_cmd);
 		if (!ret)
 			ret = count;
@@ -482,6 +499,93 @@ static void dcdbas_host_control(void)
 	}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+	u8 sum = 0;
+	u8 *end = buffer + length;
+
+	while (buffer < end)
+		sum += *buffer++;
+	return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+	struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+	if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+		return NULL;
+
+	if (checksum(addr, eps->length) != 0)
+		return NULL;
+
+	return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+	struct acpi_table_wsmt *wsmt = NULL;
+	struct smm_eps_table *eps = NULL;
+	u64 remap_size;
+	u8 *addr;
+
+	acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
+	if (!wsmt)
+		return 0;
+
+	/* Check if WSMT ACPI table shows that protection is enabled */
+	if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
+	    !(wsmt->protection_flags & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+		return 0;
+
+	/* Scan for EPS (entry point structure) */
+	for (addr = (u8 *)__va(0xf0000);
+	     addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
+	     addr += 16) {
+		eps = check_eps_table(addr);
+		if (eps)
+			break;
+	}
+
+	if (!eps) {
+		dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Get physical address of buffer and map to virtual address.
+	 * Table gives size in 4K pages, regardless of actual system page size.
+	 */
+	if (upper_32_bits(eps->smm_comm_buff_addr + 8)) {
+		dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is above 4GB\n");
+		return -EINVAL;
+	}
+	/*
+	 * Limit remap size to MAX_SMI_DATA_BUF_SIZE + 8 (since the first 8
+	 * bytes are used for a semaphore, not the data buffer itself).
+	 */
+	remap_size = eps->num_of_4k_pages * PAGE_SIZE;
+	if (remap_size > MAX_SMI_DATA_BUF_SIZE + 8)
+		remap_size = MAX_SMI_DATA_BUF_SIZE + 8;
+	eps_buffer = memremap(eps->smm_comm_buff_addr, remap_size, MEMREMAP_WB);
+	if (!eps_buffer) {
+		dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS buffer\n");
+		return -ENOMEM;
+	}
+
+	/* First 8 bytes is for a semaphore, not part of the smi_data_buf */
+	smi_data_buf_phys_addr = eps->smm_comm_buff_addr + 8;
+	smi_data_buf = eps_buffer + 8;
+	smi_data_buf_size = remap_size - 8;
+	max_smi_data_buf_size = smi_data_buf_size;
+	wsmt_enabled = true;
+	dev_info(&dcdbas_pdev->dev,
+		 "WSMT found, using firmware-provided SMI buffer.\n");
+	return 1;
+}
+
 /**
  * dcdbas_reboot_notify: handle reboot notification for host control
  */
@@ -548,6 +652,11 @@ static int dcdbas_probe(struct platform_device *dev)
 
 	dcdbas_pdev = dev;
 
+	/* Check if ACPI WSMT table specifies protected SMI buffer address */
+	error = dcdbas_check_wsmt();
+	if (error < 0)
+		return error;
+
 	/*
 	 * BIOS SMI calls require buffer addresses be in 32-bit address space.
 	 * This is done by setting the DMA mask below.
@@ -635,6 +744,8 @@ static void __exit dcdbas_exit(void)
 	 */
 	if (dcdbas_pdev)
 		smi_data_buf_free();
+	if (eps_buffer)
+		memunmap(eps_buffer);
 	platform_device_unregister(dcdbas_pdev_reg);
 	platform_driver_unregister(&dcdbas_driver);
 }
diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h
index ca3cb0a54ab6..52729a494b00 100644
--- a/drivers/firmware/dcdbas.h
+++ b/drivers/firmware/dcdbas.h
@@ -53,6 +53,7 @@
 #define EXPIRED_TIMER				(0)
 
 #define SMI_CMD_MAGIC				(0x534D4931)
+#define SMM_EPS_SIG				"$SCB"
 
 #define DCDBAS_DEV_ATTR_RW(_name) \
 	DEVICE_ATTR(_name,0600,_name##_show,_name##_store);
@@ -103,5 +104,14 @@ struct apm_cmd {
 
 int dcdbas_smi_request(struct smi_cmd *smi_cmd);
 
+struct smm_eps_table {
+	char smm_comm_buff_anchor[4];
+	u8 length;
+	u8 checksum;
+	u8 version;
+	u64 smm_comm_buff_addr;
+	u64 num_of_4k_pages;
+} __packed;
+
 #endif /* _DCDBAS_H_ */
 
-- 
2.14.2

Re: [PATCH v5] dcdbas: Add support for WSMT ACPI table

[Andy Shevchenko]

On Tue, Jul 3, 2018 at 7:20 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>
> If the WSMT ACPI table is present and indicates that a fixed communication
> buffer should be used, use the firmware-specified buffer instead of
> allocating a buffer in memory for communications between the dcdbas driver
> and firmare.
>
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> ---
> v2 Bumped driver version to 5.6.0-3.3
> v3 Removed dependency on ACPI in Kconfig
>    Moved the added #include to be in alphabetical order
>    Added comments in smi_request_store()
>    Simplified checksum code
>    Changed loop searching 0xf0000 to be more readable
>    Reworked calculation of remap_size & smi_data_buf_size
> v4 Fixed comment that starts with "Calling Interface SMI"
>    Fixed formatting of first "if" statement in dcdbas_check_wsmt()
> v5 Reworked comment that starts with "Calling Interface SMI"
>    Changed EPS scanning loop to check every 16 bytes
>

Mario, any comments on this?

For now I pushed this to my review and testing queue, thanks!

>
>  drivers/firmware/dcdbas.c | 123 +++++++++++++++++++++++++++++++++++++++++++---
>  drivers/firmware/dcdbas.h |  10 ++++
>  2 files changed, 127 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
> index 0bdea60c65dd..ae28e48ff7dc 100644
> --- a/drivers/firmware/dcdbas.c
> +++ b/drivers/firmware/dcdbas.c
> @@ -21,6 +21,7 @@
>   */
>
>  #include <linux/platform_device.h>
> +#include <linux/acpi.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/errno.h>
>  #include <linux/cpu.h>
> @@ -41,7 +42,7 @@
>  #include "dcdbas.h"
>
>  #define DRIVER_NAME            "dcdbas"
> -#define DRIVER_VERSION         "5.6.0-3.2"
> +#define DRIVER_VERSION         "5.6.0-3.3"
>  #define DRIVER_DESCRIPTION     "Dell Systems Management Base Driver"
>
>  static struct platform_device *dcdbas_pdev;
> @@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
>  static u8 *smi_data_buf;
>  static dma_addr_t smi_data_buf_handle;
>  static unsigned long smi_data_buf_size;
> +static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
>  static u32 smi_data_buf_phys_addr;
>  static DEFINE_MUTEX(smi_data_lock);
> +static u8 *eps_buffer;
>
>  static unsigned int host_control_action;
>  static unsigned int host_control_smi_type;
>  static unsigned int host_control_on_shutdown;
>
> +static bool wsmt_enabled;
> +
>  /**
>   * smi_data_buf_free: free SMI data buffer
>   */
>  static void smi_data_buf_free(void)
>  {
> -       if (!smi_data_buf)
> +       if (!smi_data_buf || wsmt_enabled)
>                 return;
>
>         dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
> @@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
>         if (smi_data_buf_size >= size)
>                 return 0;
>
> -       if (size > MAX_SMI_DATA_BUF_SIZE)
> +       if (size > max_smi_data_buf_size)
>                 return -EINVAL;
>
>         /* new buffer is needed */
> @@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct kobject *kobj,
>  {
>         ssize_t ret;
>
> -       if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
> +       if ((pos + count) > max_smi_data_buf_size)
>                 return -EINVAL;
>
>         mutex_lock(&smi_data_lock);
> @@ -322,8 +327,20 @@ static ssize_t smi_request_store(struct device *dev,
>                         ret = count;
>                 break;
>         case 1:
> -               /* Calling Interface SMI */
> -               smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> +               /*
> +                * Calling Interface SMI
> +                *
> +                * Provide physical address of command buffer field within
> +                * the struct smi_cmd to BIOS.
> +                *
> +                * Because the address that smi_cmd (smi_data_buf) points to
> +                * will be from memremap() of a non-memory address if WSMT
> +                * is present, we can't use virt_to_phys() on smi_cmd, so
> +                * we have to use the physical address that was saved when
> +                * the virtual address for smi_cmd was received.
> +                */
> +               smi_cmd->ebx = smi_data_buf_phys_addr +
> +                               offsetof(struct smi_cmd, command_buffer);
>                 ret = dcdbas_smi_request(smi_cmd);
>                 if (!ret)
>                         ret = count;
> @@ -482,6 +499,93 @@ static void dcdbas_host_control(void)
>         }
>  }
>
> +/* WSMT */
> +
> +static u8 checksum(u8 *buffer, u8 length)
> +{
> +       u8 sum = 0;
> +       u8 *end = buffer + length;
> +
> +       while (buffer < end)
> +               sum += *buffer++;
> +       return sum;
> +}
> +
> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> +{
> +       struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> +
> +       if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
> +               return NULL;
> +
> +       if (checksum(addr, eps->length) != 0)
> +               return NULL;
> +
> +       return eps;
> +}
> +
> +static int dcdbas_check_wsmt(void)
> +{
> +       struct acpi_table_wsmt *wsmt = NULL;
> +       struct smm_eps_table *eps = NULL;
> +       u64 remap_size;
> +       u8 *addr;
> +
> +       acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
> +       if (!wsmt)
> +               return 0;
> +
> +       /* Check if WSMT ACPI table shows that protection is enabled */
> +       if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
> +           !(wsmt->protection_flags & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> +               return 0;
> +
> +       /* Scan for EPS (entry point structure) */
> +       for (addr = (u8 *)__va(0xf0000);
> +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
> +            addr += 16) {
> +               eps = check_eps_table(addr);
> +               if (eps)
> +                       break;
> +       }
> +
> +       if (!eps) {
> +               dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
> +               return -ENODEV;
> +       }
> +
> +       /*
> +        * Get physical address of buffer and map to virtual address.
> +        * Table gives size in 4K pages, regardless of actual system page size.
> +        */
> +       if (upper_32_bits(eps->smm_comm_buff_addr + 8)) {
> +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is above 4GB\n");
> +               return -EINVAL;
> +       }
> +       /*
> +        * Limit remap size to MAX_SMI_DATA_BUF_SIZE + 8 (since the first 8
> +        * bytes are used for a semaphore, not the data buffer itself).
> +        */
> +       remap_size = eps->num_of_4k_pages * PAGE_SIZE;
> +       if (remap_size > MAX_SMI_DATA_BUF_SIZE + 8)
> +               remap_size = MAX_SMI_DATA_BUF_SIZE + 8;
> +       eps_buffer = memremap(eps->smm_comm_buff_addr, remap_size, MEMREMAP_WB);
> +       if (!eps_buffer) {
> +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS buffer\n");
> +               return -ENOMEM;
> +       }
> +
> +       /* First 8 bytes is for a semaphore, not part of the smi_data_buf */
> +       smi_data_buf_phys_addr = eps->smm_comm_buff_addr + 8;
> +       smi_data_buf = eps_buffer + 8;
> +       smi_data_buf_size = remap_size - 8;
> +       max_smi_data_buf_size = smi_data_buf_size;
> +       wsmt_enabled = true;
> +       dev_info(&dcdbas_pdev->dev,
> +                "WSMT found, using firmware-provided SMI buffer.\n");
> +       return 1;
> +}
> +
>  /**
>   * dcdbas_reboot_notify: handle reboot notification for host control
>   */
> @@ -548,6 +652,11 @@ static int dcdbas_probe(struct platform_device *dev)
>
>         dcdbas_pdev = dev;
>
> +       /* Check if ACPI WSMT table specifies protected SMI buffer address */
> +       error = dcdbas_check_wsmt();
> +       if (error < 0)
> +               return error;
> +
>         /*
>          * BIOS SMI calls require buffer addresses be in 32-bit address space.
>          * This is done by setting the DMA mask below.
> @@ -635,6 +744,8 @@ static void __exit dcdbas_exit(void)
>          */
>         if (dcdbas_pdev)
>                 smi_data_buf_free();
> +       if (eps_buffer)
> +               memunmap(eps_buffer);
>         platform_device_unregister(dcdbas_pdev_reg);
>         platform_driver_unregister(&dcdbas_driver);
>  }
> diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h
> index ca3cb0a54ab6..52729a494b00 100644
> --- a/drivers/firmware/dcdbas.h
> +++ b/drivers/firmware/dcdbas.h
> @@ -53,6 +53,7 @@
>  #define EXPIRED_TIMER                          (0)
>
>  #define SMI_CMD_MAGIC                          (0x534D4931)
> +#define SMM_EPS_SIG                            "$SCB"
>
>  #define DCDBAS_DEV_ATTR_RW(_name) \
>         DEVICE_ATTR(_name,0600,_name##_show,_name##_store);
> @@ -103,5 +104,14 @@ struct apm_cmd {
>
>  int dcdbas_smi_request(struct smi_cmd *smi_cmd);
>
> +struct smm_eps_table {
> +       char smm_comm_buff_anchor[4];
> +       u8 length;
> +       u8 checksum;
> +       u8 version;
> +       u64 smm_comm_buff_addr;
> +       u64 num_of_4k_pages;
> +} __packed;
> +
>  #endif /* _DCDBAS_H_ */
>
> --
> 2.14.2
>



-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v5] dcdbas: Add support for WSMT ACPI table

[Mario.Limonciello]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Friday, July 13, 2018 9:35 AM
> To: Stuart Hayes
> Cc: Limonciello, Mario; Darren Hart; Linux Kernel Mailing List; Platform Driver
> Subject: Re: [PATCH v5] dcdbas: Add support for WSMT ACPI table
> 
> On Tue, Jul 3, 2018 at 7:20 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
> >
> > If the WSMT ACPI table is present and indicates that a fixed communication
> > buffer should be used, use the firmware-specified buffer instead of
> > allocating a buffer in memory for communications between the dcdbas driver
> > and firmare.
> >
> > Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> > ---
> > v2 Bumped driver version to 5.6.0-3.3
> > v3 Removed dependency on ACPI in Kconfig
> >    Moved the added #include to be in alphabetical order
> >    Added comments in smi_request_store()
> >    Simplified checksum code
> >    Changed loop searching 0xf0000 to be more readable
> >    Reworked calculation of remap_size & smi_data_buf_size
> > v4 Fixed comment that starts with "Calling Interface SMI"
> >    Fixed formatting of first "if" statement in dcdbas_check_wsmt()
> > v5 Reworked comment that starts with "Calling Interface SMI"
> >    Changed EPS scanning loop to check every 16 bytes
> >
> 
> Mario, any comments on this?

Andy,

I do think if dell-rbu moves over to platform-x86 we should probably bring
this driver over as well.  It has the same problems as dell-rbu in that it doesn't
have anyone actively looking at it and patches sit in limbo even though it has
other drivers dependent upon it.

I would think Stuart would make a good owner for this one too over Doug
who has different responsibilities today than when the driver was originally
written.

If we do end up getting the deprecated bit we referred to in spec
reversed and the FW updated will follow up later to support either
approach.

> 
> For now I pushed this to my review and testing queue, thanks!
> 
> >
> >  drivers/firmware/dcdbas.c | 123
> +++++++++++++++++++++++++++++++++++++++++++---
> >  drivers/firmware/dcdbas.h |  10 ++++
> >  2 files changed, 127 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
> > index 0bdea60c65dd..ae28e48ff7dc 100644
> > --- a/drivers/firmware/dcdbas.c
> > +++ b/drivers/firmware/dcdbas.c
> > @@ -21,6 +21,7 @@
> >   */
> >
> >  #include <linux/platform_device.h>
> > +#include <linux/acpi.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/errno.h>
> >  #include <linux/cpu.h>
> > @@ -41,7 +42,7 @@
> >  #include "dcdbas.h"
> >
> >  #define DRIVER_NAME            "dcdbas"
> > -#define DRIVER_VERSION         "5.6.0-3.2"
> > +#define DRIVER_VERSION         "5.6.0-3.3"
> >  #define DRIVER_DESCRIPTION     "Dell Systems Management Base Driver"
> >
> >  static struct platform_device *dcdbas_pdev;
> > @@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
> >  static u8 *smi_data_buf;
> >  static dma_addr_t smi_data_buf_handle;
> >  static unsigned long smi_data_buf_size;
> > +static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
> >  static u32 smi_data_buf_phys_addr;
> >  static DEFINE_MUTEX(smi_data_lock);
> > +static u8 *eps_buffer;
> >
> >  static unsigned int host_control_action;
> >  static unsigned int host_control_smi_type;
> >  static unsigned int host_control_on_shutdown;
> >
> > +static bool wsmt_enabled;
> > +
> >  /**
> >   * smi_data_buf_free: free SMI data buffer
> >   */
> >  static void smi_data_buf_free(void)
> >  {
> > -       if (!smi_data_buf)
> > +       if (!smi_data_buf || wsmt_enabled)
> >                 return;
> >
> >         dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
> > @@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
> >         if (smi_data_buf_size >= size)
> >                 return 0;
> >
> > -       if (size > MAX_SMI_DATA_BUF_SIZE)
> > +       if (size > max_smi_data_buf_size)
> >                 return -EINVAL;
> >
> >         /* new buffer is needed */
> > @@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct
> kobject *kobj,
> >  {
> >         ssize_t ret;
> >
> > -       if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
> > +       if ((pos + count) > max_smi_data_buf_size)
> >                 return -EINVAL;
> >
> >         mutex_lock(&smi_data_lock);
> > @@ -322,8 +327,20 @@ static ssize_t smi_request_store(struct device *dev,
> >                         ret = count;
> >                 break;
> >         case 1:
> > -               /* Calling Interface SMI */
> > -               smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> > +               /*
> > +                * Calling Interface SMI
> > +                *
> > +                * Provide physical address of command buffer field within
> > +                * the struct smi_cmd to BIOS.
> > +                *
> > +                * Because the address that smi_cmd (smi_data_buf) points to
> > +                * will be from memremap() of a non-memory address if WSMT
> > +                * is present, we can't use virt_to_phys() on smi_cmd, so
> > +                * we have to use the physical address that was saved when
> > +                * the virtual address for smi_cmd was received.
> > +                */
> > +               smi_cmd->ebx = smi_data_buf_phys_addr +
> > +                               offsetof(struct smi_cmd, command_buffer);
> >                 ret = dcdbas_smi_request(smi_cmd);
> >                 if (!ret)
> >                         ret = count;
> > @@ -482,6 +499,93 @@ static void dcdbas_host_control(void)
> >         }
> >  }
> >
> > +/* WSMT */
> > +
> > +static u8 checksum(u8 *buffer, u8 length)
> > +{
> > +       u8 sum = 0;
> > +       u8 *end = buffer + length;
> > +
> > +       while (buffer < end)
> > +               sum += *buffer++;
> > +       return sum;
> > +}
> > +
> > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> > +{
> > +       struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> > +
> > +       if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
> > +               return NULL;
> > +
> > +       if (checksum(addr, eps->length) != 0)
> > +               return NULL;
> > +
> > +       return eps;
> > +}
> > +
> > +static int dcdbas_check_wsmt(void)
> > +{
> > +       struct acpi_table_wsmt *wsmt = NULL;
> > +       struct smm_eps_table *eps = NULL;
> > +       u64 remap_size;
> > +       u8 *addr;
> > +
> > +       acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
> > +       if (!wsmt)
> > +               return 0;
> > +
> > +       /* Check if WSMT ACPI table shows that protection is enabled */
> > +       if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
> > +           !(wsmt->protection_flags &
> ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> > +               return 0;
> > +
> > +       /* Scan for EPS (entry point structure) */
> > +       for (addr = (u8 *)__va(0xf0000);
> > +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
> > +            addr += 16) {
> > +               eps = check_eps_table(addr);
> > +               if (eps)
> > +                       break;
> > +       }
> > +
> > +       if (!eps) {
> > +               dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       /*
> > +        * Get physical address of buffer and map to virtual address.
> > +        * Table gives size in 4K pages, regardless of actual system page size.
> > +        */
> > +       if (upper_32_bits(eps->smm_comm_buff_addr + 8)) {
> > +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is
> above 4GB\n");
> > +               return -EINVAL;
> > +       }
> > +       /*
> > +        * Limit remap size to MAX_SMI_DATA_BUF_SIZE + 8 (since the first 8
> > +        * bytes are used for a semaphore, not the data buffer itself).
> > +        */
> > +       remap_size = eps->num_of_4k_pages * PAGE_SIZE;
> > +       if (remap_size > MAX_SMI_DATA_BUF_SIZE + 8)
> > +               remap_size = MAX_SMI_DATA_BUF_SIZE + 8;
> > +       eps_buffer = memremap(eps->smm_comm_buff_addr, remap_size,
> MEMREMAP_WB);
> > +       if (!eps_buffer) {
> > +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS
> buffer\n");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       /* First 8 bytes is for a semaphore, not part of the smi_data_buf */
> > +       smi_data_buf_phys_addr = eps->smm_comm_buff_addr + 8;
> > +       smi_data_buf = eps_buffer + 8;
> > +       smi_data_buf_size = remap_size - 8;
> > +       max_smi_data_buf_size = smi_data_buf_size;
> > +       wsmt_enabled = true;
> > +       dev_info(&dcdbas_pdev->dev,
> > +                "WSMT found, using firmware-provided SMI buffer.\n");
> > +       return 1;
> > +}
> > +
> >  /**
> >   * dcdbas_reboot_notify: handle reboot notification for host control
> >   */
> > @@ -548,6 +652,11 @@ static int dcdbas_probe(struct platform_device *dev)
> >
> >         dcdbas_pdev = dev;
> >
> > +       /* Check if ACPI WSMT table specifies protected SMI buffer address */
> > +       error = dcdbas_check_wsmt();
> > +       if (error < 0)
> > +               return error;
> > +
> >         /*
> >          * BIOS SMI calls require buffer addresses be in 32-bit address space.
> >          * This is done by setting the DMA mask below.
> > @@ -635,6 +744,8 @@ static void __exit dcdbas_exit(void)
> >          */
> >         if (dcdbas_pdev)
> >                 smi_data_buf_free();
> > +       if (eps_buffer)
> > +               memunmap(eps_buffer);
> >         platform_device_unregister(dcdbas_pdev_reg);
> >         platform_driver_unregister(&dcdbas_driver);
> >  }
> > diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h
> > index ca3cb0a54ab6..52729a494b00 100644
> > --- a/drivers/firmware/dcdbas.h
> > +++ b/drivers/firmware/dcdbas.h
> > @@ -53,6 +53,7 @@
> >  #define EXPIRED_TIMER                          (0)
> >
> >  #define SMI_CMD_MAGIC                          (0x534D4931)
> > +#define SMM_EPS_SIG                            "$SCB"
> >
> >  #define DCDBAS_DEV_ATTR_RW(_name) \
> >         DEVICE_ATTR(_name,0600,_name##_show,_name##_store);
> > @@ -103,5 +104,14 @@ struct apm_cmd {
> >
> >  int dcdbas_smi_request(struct smi_cmd *smi_cmd);
> >
> > +struct smm_eps_table {
> > +       char smm_comm_buff_anchor[4];
> > +       u8 length;
> > +       u8 checksum;
> > +       u8 version;
> > +       u64 smm_comm_buff_addr;
> > +       u64 num_of_4k_pages;
> > +} __packed;
> > +
> >  #endif /* _DCDBAS_H_ */
> >
> > --
> > 2.14.2
> >
> 
> 
> 
> --
> With Best Regards,
> Andy Shevchenko

[PATCH v4] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
v2 Bumped driver version to 5.6.0-3.3
v3 Removed dependency on ACPI in Kconfig
   Moved the added #include to be in alphabetical order
   Added comments in smi_request_store()
   Simplified checksum code
   Changed loop searching 0xf0000 to be more readable
   Reworked calculation of remap_size & smi_data_buf_size
v4 Fixed comment that starts with "Calling Interface SMI"
   Fixed formatting of first "if" statement in dcdbas_check_wsmt()


 drivers/firmware/dcdbas.c | 118 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/firmware/dcdbas.h |  10 ++++
 2 files changed, 122 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..4cac70d05bfc 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include <linux/platform_device.h>
+#include <linux/acpi.h>
 #include <linux/dma-mapping.h>
 #include <linux/errno.h>
 #include <linux/cpu.h>
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME		"dcdbas"
-#define DRIVER_VERSION		"5.6.0-3.2"
+#define DRIVER_VERSION		"5.6.0-3.3"
 #define DRIVER_DESCRIPTION	"Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-	if (!smi_data_buf)
+	if (!smi_data_buf || wsmt_enabled)
 		return;
 
 	dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
 	if (smi_data_buf_size >= size)
 		return 0;
 
-	if (size > MAX_SMI_DATA_BUF_SIZE)
+	if (size > max_smi_data_buf_size)
 		return -EINVAL;
 
 	/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct kobject *kobj,
 {
 	ssize_t ret;
 
-	if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+	if ((pos + count) > max_smi_data_buf_size)
 		return -EINVAL;
 
 	mutex_lock(&smi_data_lock);
@@ -322,8 +327,15 @@ static ssize_t smi_request_store(struct device *dev,
 			ret = count;
 		break;
 	case 1:
-		/* Calling Interface SMI */
-		smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+		/*
+		 * Calling Interface SMI
+		 *
+		 * Provide physical address of command buffer field within
+		 * the struct smi_cmd... can't use virt_to_phys() on smi_cmd
+		 * because address may be from memremap().
+		 */
+		smi_cmd->ebx = smi_data_buf_phys_addr +
+				offsetof(struct smi_cmd, command_buffer);
 		ret = dcdbas_smi_request(smi_cmd);
 		if (!ret)
 			ret = count;
@@ -482,6 +494,93 @@ static void dcdbas_host_control(void)
 	}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+	u8 sum = 0;
+	u8 *end = buffer + length;
+
+	while (buffer < end)
+		sum += *buffer++;
+	return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+	struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+	if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+		return NULL;
+
+	if (checksum(addr, eps->length) != 0)
+		return NULL;
+
+	return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+	struct acpi_table_wsmt *wsmt = NULL;
+	struct smm_eps_table *eps = NULL;
+	u64 remap_size;
+	u8 *addr;
+
+	acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
+	if (!wsmt)
+		return 0;
+
+	/* Check if WSMT ACPI table shows that protection is enabled */
+	if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
+	    !(wsmt->protection_flags & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+		return 0;
+
+	/* Scan for EPS (entry point structure) */
+	for (addr = (u8 *)__va(0xf0000);
+	     addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
+	     addr += 1) {
+		eps = check_eps_table(addr);
+		if (eps)
+			break;
+	}
+
+	if (!eps) {
+		dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Get physical address of buffer and map to virtual address.
+	 * Table gives size in 4K pages, regardless of actual system page size.
+	 */
+	if (upper_32_bits(eps->smm_comm_buff_addr + 8)) {
+		dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is above 4GB\n");
+		return -EINVAL;
+	}
+	/*
+	 * Limit remap size to MAX_SMI_DATA_BUF_SIZE + 8 (since the first 8
+	 * bytes are used for a semaphore, not the data buffer itself).
+	 */
+	remap_size = eps->num_of_4k_pages * PAGE_SIZE;
+	if (remap_size > MAX_SMI_DATA_BUF_SIZE + 8)
+		remap_size = MAX_SMI_DATA_BUF_SIZE + 8;
+	eps_buffer = memremap(eps->smm_comm_buff_addr, remap_size, MEMREMAP_WB);
+	if (!eps_buffer) {
+		dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS buffer\n");
+		return -ENOMEM;
+	}
+
+	/* First 8 bytes is for a semaphore, not part of the smi_data_buf */
+	smi_data_buf_phys_addr = eps->smm_comm_buff_addr + 8;
+	smi_data_buf = eps_buffer + 8;
+	smi_data_buf_size = remap_size - 8;
+	max_smi_data_buf_size = smi_data_buf_size;
+	wsmt_enabled = true;
+	dev_info(&dcdbas_pdev->dev,
+		 "WSMT found, using firmware-provided SMI buffer.\n");
+	return 1;
+}
+
 /**
  * dcdbas_reboot_notify: handle reboot notification for host control
  */
@@ -548,6 +647,11 @@ static int dcdbas_probe(struct platform_device *dev)
 
 	dcdbas_pdev = dev;
 
+	/* Check if ACPI WSMT table specifies protected SMI buffer address */
+	error = dcdbas_check_wsmt();
+	if (error < 0)
+		return error;
+
 	/*
 	 * BIOS SMI calls require buffer addresses be in 32-bit address space.
 	 * This is done by setting the DMA mask below.
@@ -635,6 +739,8 @@ static void __exit dcdbas_exit(void)
 	 */
 	if (dcdbas_pdev)
 		smi_data_buf_free();
+	if (eps_buffer)
+		memunmap(eps_buffer);
 	platform_device_unregister(dcdbas_pdev_reg);
 	platform_driver_unregister(&dcdbas_driver);
 }
diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h
index ca3cb0a54ab6..52729a494b00 100644
--- a/drivers/firmware/dcdbas.h
+++ b/drivers/firmware/dcdbas.h
@@ -53,6 +53,7 @@
 #define EXPIRED_TIMER				(0)
 
 #define SMI_CMD_MAGIC				(0x534D4931)
+#define SMM_EPS_SIG				"$SCB"
 
 #define DCDBAS_DEV_ATTR_RW(_name) \
 	DEVICE_ATTR(_name,0600,_name##_show,_name##_store);
@@ -103,5 +104,14 @@ struct apm_cmd {
 
 int dcdbas_smi_request(struct smi_cmd *smi_cmd);
 
+struct smm_eps_table {
+	char smm_comm_buff_anchor[4];
+	u8 length;
+	u8 checksum;
+	u8 version;
+	u64 smm_comm_buff_addr;
+	u64 num_of_4k_pages;
+} __packed;
+
 #endif /* _DCDBAS_H_ */
 
-- 
2.14.2

Re: [PATCH v4] dcdbas: Add support for WSMT ACPI table

[Andy Shevchenko]

On Thu, Jun 14, 2018 at 6:45 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>
> If the WSMT ACPI table is present and indicates that a fixed communication
> buffer should be used, use the firmware-specified buffer instead of
> allocating a buffer in memory for communications between the dcdbas driver
> and firmare.

Thanks for an update.

I answered to previous thread. The questions / comments are still
applicable here.

> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> ---
> v2 Bumped driver version to 5.6.0-3.3
> v3 Removed dependency on ACPI in Kconfig
>    Moved the added #include to be in alphabetical order
>    Added comments in smi_request_store()
>    Simplified checksum code
>    Changed loop searching 0xf0000 to be more readable
>    Reworked calculation of remap_size & smi_data_buf_size
> v4 Fixed comment that starts with "Calling Interface SMI"
>    Fixed formatting of first "if" statement in dcdbas_check_wsmt()
>
>
>  drivers/firmware/dcdbas.c | 118 +++++++++++++++++++++++++++++++++++++++++++---
>  drivers/firmware/dcdbas.h |  10 ++++
>  2 files changed, 122 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
> index 0bdea60c65dd..4cac70d05bfc 100644
> --- a/drivers/firmware/dcdbas.c
> +++ b/drivers/firmware/dcdbas.c
> @@ -21,6 +21,7 @@
>   */
>
>  #include <linux/platform_device.h>
> +#include <linux/acpi.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/errno.h>
>  #include <linux/cpu.h>
> @@ -41,7 +42,7 @@
>  #include "dcdbas.h"
>
>  #define DRIVER_NAME            "dcdbas"
> -#define DRIVER_VERSION         "5.6.0-3.2"
> +#define DRIVER_VERSION         "5.6.0-3.3"
>  #define DRIVER_DESCRIPTION     "Dell Systems Management Base Driver"
>
>  static struct platform_device *dcdbas_pdev;
> @@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
>  static u8 *smi_data_buf;
>  static dma_addr_t smi_data_buf_handle;
>  static unsigned long smi_data_buf_size;
> +static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
>  static u32 smi_data_buf_phys_addr;
>  static DEFINE_MUTEX(smi_data_lock);
> +static u8 *eps_buffer;
>
>  static unsigned int host_control_action;
>  static unsigned int host_control_smi_type;
>  static unsigned int host_control_on_shutdown;
>
> +static bool wsmt_enabled;
> +
>  /**
>   * smi_data_buf_free: free SMI data buffer
>   */
>  static void smi_data_buf_free(void)
>  {
> -       if (!smi_data_buf)
> +       if (!smi_data_buf || wsmt_enabled)
>                 return;
>
>         dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
> @@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
>         if (smi_data_buf_size >= size)
>                 return 0;
>
> -       if (size > MAX_SMI_DATA_BUF_SIZE)
> +       if (size > max_smi_data_buf_size)
>                 return -EINVAL;
>
>         /* new buffer is needed */
> @@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct kobject *kobj,
>  {
>         ssize_t ret;
>
> -       if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
> +       if ((pos + count) > max_smi_data_buf_size)
>                 return -EINVAL;
>
>         mutex_lock(&smi_data_lock);
> @@ -322,8 +327,15 @@ static ssize_t smi_request_store(struct device *dev,
>                         ret = count;
>                 break;
>         case 1:
> -               /* Calling Interface SMI */
> -               smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> +               /*
> +                * Calling Interface SMI
> +                *
> +                * Provide physical address of command buffer field within
> +                * the struct smi_cmd... can't use virt_to_phys() on smi_cmd
> +                * because address may be from memremap().
> +                */
> +               smi_cmd->ebx = smi_data_buf_phys_addr +
> +                               offsetof(struct smi_cmd, command_buffer);
>                 ret = dcdbas_smi_request(smi_cmd);
>                 if (!ret)
>                         ret = count;
> @@ -482,6 +494,93 @@ static void dcdbas_host_control(void)
>         }
>  }
>
> +/* WSMT */
> +
> +static u8 checksum(u8 *buffer, u8 length)
> +{
> +       u8 sum = 0;
> +       u8 *end = buffer + length;
> +
> +       while (buffer < end)
> +               sum += *buffer++;
> +       return sum;
> +}
> +
> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> +{
> +       struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> +
> +       if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
> +               return NULL;
> +
> +       if (checksum(addr, eps->length) != 0)
> +               return NULL;
> +
> +       return eps;
> +}
> +
> +static int dcdbas_check_wsmt(void)
> +{
> +       struct acpi_table_wsmt *wsmt = NULL;
> +       struct smm_eps_table *eps = NULL;
> +       u64 remap_size;
> +       u8 *addr;
> +
> +       acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
> +       if (!wsmt)
> +               return 0;
> +
> +       /* Check if WSMT ACPI table shows that protection is enabled */
> +       if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
> +           !(wsmt->protection_flags & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> +               return 0;
> +
> +       /* Scan for EPS (entry point structure) */
> +       for (addr = (u8 *)__va(0xf0000);
> +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table));
> +            addr += 1) {
> +               eps = check_eps_table(addr);
> +               if (eps)
> +                       break;
> +       }
> +
> +       if (!eps) {
> +               dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
> +               return -ENODEV;
> +       }
> +
> +       /*
> +        * Get physical address of buffer and map to virtual address.
> +        * Table gives size in 4K pages, regardless of actual system page size.
> +        */
> +       if (upper_32_bits(eps->smm_comm_buff_addr + 8)) {
> +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is above 4GB\n");
> +               return -EINVAL;
> +       }
> +       /*
> +        * Limit remap size to MAX_SMI_DATA_BUF_SIZE + 8 (since the first 8
> +        * bytes are used for a semaphore, not the data buffer itself).
> +        */
> +       remap_size = eps->num_of_4k_pages * PAGE_SIZE;
> +       if (remap_size > MAX_SMI_DATA_BUF_SIZE + 8)
> +               remap_size = MAX_SMI_DATA_BUF_SIZE + 8;
> +       eps_buffer = memremap(eps->smm_comm_buff_addr, remap_size, MEMREMAP_WB);
> +       if (!eps_buffer) {
> +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS buffer\n");
> +               return -ENOMEM;
> +       }
> +
> +       /* First 8 bytes is for a semaphore, not part of the smi_data_buf */
> +       smi_data_buf_phys_addr = eps->smm_comm_buff_addr + 8;
> +       smi_data_buf = eps_buffer + 8;
> +       smi_data_buf_size = remap_size - 8;
> +       max_smi_data_buf_size = smi_data_buf_size;
> +       wsmt_enabled = true;
> +       dev_info(&dcdbas_pdev->dev,
> +                "WSMT found, using firmware-provided SMI buffer.\n");
> +       return 1;
> +}
> +
>  /**
>   * dcdbas_reboot_notify: handle reboot notification for host control
>   */
> @@ -548,6 +647,11 @@ static int dcdbas_probe(struct platform_device *dev)
>
>         dcdbas_pdev = dev;
>
> +       /* Check if ACPI WSMT table specifies protected SMI buffer address */
> +       error = dcdbas_check_wsmt();
> +       if (error < 0)
> +               return error;
> +
>         /*
>          * BIOS SMI calls require buffer addresses be in 32-bit address space.
>          * This is done by setting the DMA mask below.
> @@ -635,6 +739,8 @@ static void __exit dcdbas_exit(void)
>          */
>         if (dcdbas_pdev)
>                 smi_data_buf_free();
> +       if (eps_buffer)
> +               memunmap(eps_buffer);
>         platform_device_unregister(dcdbas_pdev_reg);
>         platform_driver_unregister(&dcdbas_driver);
>  }
> diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h
> index ca3cb0a54ab6..52729a494b00 100644
> --- a/drivers/firmware/dcdbas.h
> +++ b/drivers/firmware/dcdbas.h
> @@ -53,6 +53,7 @@
>  #define EXPIRED_TIMER                          (0)
>
>  #define SMI_CMD_MAGIC                          (0x534D4931)
> +#define SMM_EPS_SIG                            "$SCB"
>
>  #define DCDBAS_DEV_ATTR_RW(_name) \
>         DEVICE_ATTR(_name,0600,_name##_show,_name##_store);
> @@ -103,5 +104,14 @@ struct apm_cmd {
>
>  int dcdbas_smi_request(struct smi_cmd *smi_cmd);
>
> +struct smm_eps_table {
> +       char smm_comm_buff_anchor[4];
> +       u8 length;
> +       u8 checksum;
> +       u8 version;
> +       u64 smm_comm_buff_addr;
> +       u64 num_of_4k_pages;
> +} __packed;
> +
>  #endif /* _DCDBAS_H_ */
>
> --
> 2.14.2
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/14/2018 12:26 PM, Andy Shevchenko wrote:
> On Thu, Jun 14, 2018 at 6:45 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>>
>> If the WSMT ACPI table is present and indicates that a fixed communication
>> buffer should be used, use the firmware-specified buffer instead of
>> allocating a buffer in memory for communications between the dcdbas driver
>> and firmare.
> 
> Thanks for an update.
> 
> I answered to previous thread. The questions / comments are still
> applicable here.
> 

I answered your questions in the previous thread (a while back).

Please let me know if there are any more concerns with this.  Thanks!

 

RE: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Friday, June 8, 2018 8:04 PM
> To: Andy Shevchenko
> Cc: Stuart Hayes; Warzecha, Douglas; Limonciello, Mario; Dominguez, Jared; Linux
> Kernel Mailing List; Platform Driver
> Subject: Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
> 
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes <stuart.w.hayes@gmail.com>
> wrote:
> > > If the WSMT ACPI table is present and indicates that a fixed communication
> > > buffer should be used, use the firmware-specified buffer instead of
> > > allocating a buffer in memory for communications between the dcdbas driver
> > > and firmare.
> >
> > >  config DCDBAS
> > >         tristate "Dell Systems Management Base Driver"
> > > -       depends on X86
> > > +       depends on X86 && ACPI
> 
> 
> >
> > Hmm... I'm not sure about this dependency.
> > So, the question is do all users actually need this? How did it work
> > previously? How has this been tested in case when command line has
> > "acpi=off" (yes, this one just for sake of test, I don't believe it's
> > a real use case)?
> 
> Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
> driver which intersected with this one.... this needs to be thoroughly
> covered, tested, and thought through. Linus was.... generous in the
> number of attempts it took us to get that right.
> 
> Did DCDBAS ever work on a system without ACPI?

Yes, I would expect it would have functioned on a system with kernel's
ACPI disabled.  However I also agree this isn't a real use case with a modern
Machine.

Perhaps the right thing to do is
#ifdef CONFIG_ACPI

For the WSMT relevant items in this patch?

> 
> >
> > >  #include <linux/string.h>
> > >  #include <linux/types.h>
> > >  #include <linux/mutex.h>
> > > +#include <linux/acpi.h>
> >
> > Please, try to keep an order as much as possible.
> > For example, in given context this line should be before string.h (I
> > didn't check the actual file, perhaps even upper).
> >
> > >  #include <asm/io.h>
> > >
> > >  #include "dcdbas.h"
> >
> > >                 /* Calling Interface SMI */
> > > -               smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> > > +               smi_cmd->ebx = smi_data_buf_phys_addr
> > > +                               + offsetof(struct smi_cmd, command_buffer);
> >
> > Please, keep at least + on the previous line.
> > Also, I'm not sure what is the difference now. Especially for previous
> > users when this wasn't the part of the driver.
> > Some explanation needed.
> >
> > > +static u8 checksum(u8 *buffer, u8 length)
> > > +{
> > > +       u8 sum = 0;
> > > +       u8 *end = buffer + length;
> > > +
> > > +       while (buffer < end)
> > > +               sum = (u8)(sum + *(buffer++));
> >
> > Why not simple
> >
> > sum += *buffer++;
> >
> > ?
> >
> > > +       return sum;
> > > +}
> >
> > And I would rather check if we have similar algoritms already in the
> > kernel which  we might re-use.
> 
> Seems to be some options in lib/checksum.c to check.
> 
> >
> > > +
> > > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> > > +{
> > > +       struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> > > +
> >
> > > +       if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
> >
> > I'm not sure about strings operation here.
> > I would rather do like with other magic constants: introduce hex value
> > and compare it as unsigned integer.
> >
> > Also, it might be a warning, since \0 wasn't ever checked from the
> > string literal. Though, I'm not sure if it applicable to strncmp()
> > function (it's for strncpy for sure).
> 
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.
> 
> >
> > > +               return NULL;
> > > +
> > > +       if (checksum(addr, eps->length) != 0)
> > > +               return NULL;
> > > +
> > > +       return eps;
> > > +}
> > > +
> > > +static int dcdbas_check_wsmt(void)
> > > +{
> > > +       struct acpi_table_wsmt *wsmt = NULL;
> > > +       struct smm_eps_table *eps = NULL;
> > > +       u8 *addr;
> > > +
> > > +       acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
> > > +       if (!wsmt)
> > > +               return 0;
> > > +
> > > +       /* Check if WSMT ACPI table shows that protection is enabled */
> > > +       if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> > > +           || !(wsmt->protection_flags
> > > +                & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> > > +               return 0;
> > > +
> > > +       /* Scan for EPS (entry point structure) */
> > > +       for (addr = (u8 *)__va(0xf0000);
> > > +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)) && !eps;
> >
> > Perhaps better to do
> >
> > for (...) {
> >  eps = ...();
> >  if (eps)
> >   break;
> > }
> >
> > Also I've a feeling that 0xf0000 constant is defined already somewhere
> > under arch/x86/include/asm or evem include/linux.
> 
> But... is it defined for this purpose? If not, I'd prefer it hardcoded
> (or with a DEFINE).
> 
> >
> > > +            addr += 1)
> >
> > += 1?!
> > All tables I saw in BIOS are aligned with 16 bytes. Is it the case here?
> >
> > Is there any other means to check if the table present? ACPI code?
> > Method / variable?
> >
> > > +               eps = check_eps_table(addr);
> > > +
> > > +       if (!eps) {
> > > +               dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       /*
> > > +        * Get physical address of buffer and map to virtual address.
> > > +        * Table gives size in 4K pages, regardless of actual system page size.
> > > +        */
> >
> > > +       if (eps->smm_comm_buff_addr + 8 > U32_MAX) {
> >
> > if (upper_32_bits(..._addr + 8)) {
> >
> > ?
> >
> > > +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address
> is above 4GB\n");
> > > +               return -EINVAL;
> > > +       }
> > > +       eps_buffer = (u8 *)memremap(eps->smm_comm_buff_addr,
> >
> > Why casting?
> >
> > > +                                    eps->num_of_4k_pages * 4096, MEMREMAP_WB);
> >
> > This multiplication looks strange. Perhaps use PAGE_SIZE?
> >
> > > +       if (!eps_buffer) {
> > > +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS
> buffer\n");
> > > +               return -ENOMEM;
> > > +       }
> > > +
> > > +       /* First 8 bytes of buffer is for semaphore */
> > > +       smi_data_buf_phys_addr = (u32) eps->smm_comm_buff_addr + 8;
> >
> > lower_32_bits() ?
> >
> > > +       smi_data_buf = eps_buffer + 8;
> >
> > > +       smi_data_buf_size = (unsigned long) min(eps->num_of_4k_pages * 4096 -
> 8,
> > > +                           (u64) ULONG_MAX);
> >
> > This is too twisted code. First, it needs explanation.
> > Second, it might need some refactoring.
> >
> > (Yes, I got the idea, but would it be better implementation?)
> >
> > > +       max_smi_data_buf_size = smi_data_buf_size;
> > > +       wsmt_enabled = true;
> > > +       dev_info(&dcdbas_pdev->dev,
> > > +                "WSMT found, using firmware-provided SMI buffer.\n");
> > > +       return 1;
> > > +}
> >
> > >  #define SMI_CMD_MAGIC                          (0x534D4931)
> > >
> > > +#define SMM_EPS_SIG                            "$SCB"
> >
> > Just integer like above and put the sting as a comment.
> > (Side note: above magic also looks like string)
> 
> Given the above, I think we can use the more recognizable string - since
> that is clearly how they think of this label.
> 
> Otherwise, agree with a follow-up to all of Andy's feedback.
> 
> --
> Darren Hart
> VMware Open Source Technology Center

Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/8/2018 8:04 PM, Darren Hart wrote:
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
>> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>>> If the WSMT ACPI table is present and indicates that a fixed communication
>>> buffer should be used, use the firmware-specified buffer instead of
>>> allocating a buffer in memory for communications between the dcdbas driver
>>> and firmare.
>>
>>>  config DCDBAS
>>>         tristate "Dell Systems Management Base Driver"
>>> -       depends on X86
>>> +       depends on X86 && ACPI
> 
> 
>>
>> Hmm... I'm not sure about this dependency.
>> So, the question is do all users actually need this? How did it work
>> previously? How has this been tested in case when command line has
>> "acpi=off" (yes, this one just for sake of test, I don't believe it's
>> a real use case)?
> 
> Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
> driver which intersected with this one.... this needs to be thoroughly
> covered, tested, and thought through. Linus was.... generous in the
> number of attempts it took us to get that right.
> 
> Did DCDBAS ever work on a system without ACPI?
> 

It appears to compile ok without ACPI enabled... looks like acpi_get_table
just returns a constant when CONFIG_ACPI isn't there, which makes all the WSMT
stuff get optimized out.  So I don't guess we even need an "#ifdef CONFIG_ACPI".

>>
>>>  #include <linux/string.h>
>>>  #include <linux/types.h>
>>>  #include <linux/mutex.h>
>>> +#include <linux/acpi.h>
>>
>> Please, try to keep an order as much as possible.
>> For example, in given context this line should be before string.h (I
>> didn't check the actual file, perhaps even upper).
>>
>>>  #include <asm/io.h>
>>>
>>>  #include "dcdbas.h"
>>
>>>                 /* Calling Interface SMI */
>>> -               smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
>>> +               smi_cmd->ebx = smi_data_buf_phys_addr
>>> +                               + offsetof(struct smi_cmd, command_buffer);
>>
>> Please, keep at least + on the previous line.
>> Also, I'm not sure what is the difference now. Especially for previous
>> users when this wasn't the part of the driver.
>> Some explanation needed.
>>

I'll fix this.

>>> +static u8 checksum(u8 *buffer, u8 length)
>>> +{
>>> +       u8 sum = 0;
>>> +       u8 *end = buffer + length;
>>> +
>>> +       while (buffer < end)
>>> +               sum = (u8)(sum + *(buffer++));
>>
>> Why not simple
>>
>> sum += *buffer++;
>>
>> ?
>>
>>> +       return sum;
>>> +}
>>
>> And I would rather check if we have similar algoritms already in the
>> kernel which  we might re-use.
> 
> Seems to be some options in lib/checksum.c to check.
> 

I couldn't find anything in checksum.c or elsewhere that I could just
include that would do a byte checksum, not a word.  I copied this code
from acpi_tb_checksum (in drivers/acpi/acpica/tbprint.c), but I can
shorten it as suggested.

>>
>>> +
>>> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
>>> +{
>>> +       struct smm_eps_table *eps = (struct smm_eps_table *)addr;
>>> +
>>
>>> +       if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
>>
>> I'm not sure about strings operation here.
>> I would rather do like with other magic constants: introduce hex value
>> and compare it as unsigned integer.
>>
>> Also, it might be a warning, since \0 wasn't ever checked from the
>> string literal. Though, I'm not sure if it applicable to strncmp()
>> function (it's for strncpy for sure).
> 
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.
> 
>>
>>> +               return NULL;
>>> +
>>> +       if (checksum(addr, eps->length) != 0)
>>> +               return NULL;
>>> +
>>> +       return eps;
>>> +}
>>> +
>>> +static int dcdbas_check_wsmt(void)
>>> +{
>>> +       struct acpi_table_wsmt *wsmt = NULL;
>>> +       struct smm_eps_table *eps = NULL;
>>> +       u8 *addr;
>>> +
>>> +       acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt);
>>> +       if (!wsmt)
>>> +               return 0;
>>> +
>>> +       /* Check if WSMT ACPI table shows that protection is enabled */
>>> +       if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
>>> +           || !(wsmt->protection_flags
>>> +                & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
>>> +               return 0;
>>> +
>>> +       /* Scan for EPS (entry point structure) */
>>> +       for (addr = (u8 *)__va(0xf0000);
>>> +            addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)) && !eps;
>>
>> Perhaps better to do
>>
>> for (...) {
>>  eps = ...();
>>  if (eps)
>>   break;
>> }
>>
>> Also I've a feeling that 0xf0000 constant is defined already somewhere
>> under arch/x86/include/asm or evem include/linux.
> 
> But... is it defined for this purpose? If not, I'd prefer it hardcoded
> (or with a DEFINE).
> 
>>
>>> +            addr += 1)
>>
>> += 1?!
>> All tables I saw in BIOS are aligned with 16 bytes. Is it the case here?
>>
>> Is there any other means to check if the table present? ACPI code?
>> Method / variable?
>>

The spec doesn't say this will be aligned with 16 bytes.  It says "Pointer to
this memory region is published through a reference anchor structure SMM_EPS
located in the F-Block physical memory range anywhere between F0000h – FFFFFh.
OS driver or application needs to scan for this structure with signature “$SCB”
in the above mentioned memory range."

>>> +               eps = check_eps_table(addr);
>>> +
>>> +       if (!eps) {
>>> +               dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n");
>>> +               return -ENODEV;
>>> +       }
>>> +
>>> +       /*
>>> +        * Get physical address of buffer and map to virtual address.
>>> +        * Table gives size in 4K pages, regardless of actual system page size.
>>> +        */
>>
>>> +       if (eps->smm_comm_buff_addr + 8 > U32_MAX) {
>>
>> if (upper_32_bits(..._addr + 8)) {
>>
>> ?
>>
>>> +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is above 4GB\n");
>>> +               return -EINVAL;
>>> +       }
>>> +       eps_buffer = (u8 *)memremap(eps->smm_comm_buff_addr,
>>
>> Why casting?
>>

Oops, I'll fix that.

>>> +                                    eps->num_of_4k_pages * 4096, MEMREMAP_WB);
>>
>> This multiplication looks strange. Perhaps use PAGE_SIZE?
>>
>>> +       if (!eps_buffer) {
>>> +               dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS buffer\n");
>>> +               return -ENOMEM;
>>> +       }
>>> +
>>> +       /* First 8 bytes of buffer is for semaphore */
>>> +       smi_data_buf_phys_addr = (u32) eps->smm_comm_buff_addr + 8;
>>
>> lower_32_bits() ?
>>
>>> +       smi_data_buf = eps_buffer + 8;
>>
>>> +       smi_data_buf_size = (unsigned long) min(eps->num_of_4k_pages * 4096 - 8,
>>> +                           (u64) ULONG_MAX);
>>
>> This is too twisted code. First, it needs explanation.
>> Second, it might need some refactoring.
>>
>> (Yes, I got the idea, but would it be better implementation?)
>>

Yes this is pretty bad, I'll change it.

>>> +       max_smi_data_buf_size = smi_data_buf_size;
>>> +       wsmt_enabled = true;
>>> +       dev_info(&dcdbas_pdev->dev,
>>> +                "WSMT found, using firmware-provided SMI buffer.\n");
>>> +       return 1;
>>> +}
>>
>>>  #define SMI_CMD_MAGIC                          (0x534D4931)
>>>
>>> +#define SMM_EPS_SIG                            "$SCB"
>>
>> Just integer like above and put the sting as a comment.
>> (Side note: above magic also looks like string)
> 
> Given the above, I think we can use the more recognizable string - since
> that is clearly how they think of this label.
> 
> Otherwise, agree with a follow-up to all of Andy's feedback.
> 

I'll make the suggested changes and submit a new version.  Thank you all for
taking the time to review this!