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

From: Mario.Limonciello
Date: Mon Jul 16 2018 - 09:38:01 EST

Next message: Peter Zijlstra: "Re: [tip:sched/core] sched/cputime: Ensure accurate utime and stime ratio in cputime_adjust()"
Previous message: Lukas Wunner: "Re: [PATCH 08/32] genirq: Synchronize only with single thread on free_irq()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@xxxxxxxxx]
> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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

Next message: Peter Zijlstra: "Re: [tip:sched/core] sched/cputime: Ensure accurate utime and stime ratio in cputime_adjust()"
Previous message: Lukas Wunner: "Re: [PATCH 08/32] genirq: Synchronize only with single thread on free_irq()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]