Re: [PATCH] platform/x86/dell: add buffer allocation/free functions for SMI calls

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 3/18/22 16:09, Juergen Gross wrote:
> The dcdbas driver is used to call SMI handlers for both, dcdbas and
> dell-smbios-smm. Both drivers allocate a buffer for communicating
> with the SMI handler. The physical buffer address is then passed to
> the called SMI handler via %ebx.
> 
> Unfortunately this doesn't work when running in Xen dom0, as the
> physical address obtained via virt_to_phys() is only a guest physical
> address, and not a machine physical address as needed by SMI.
> 
> The problem in dcdbas is easy to correct, as dcdbas is using
> dma_alloc_coherent() for allocating the buffer, and the machine
> physical address is available via the DMA address returned in the DMA
> handle.
> 
> In order to avoid duplicating the buffer allocation code in
> dell-smbios-smm, add a generic buffer allocation function to dcdbas
> and use it for both drivers. This is especially fine regarding driver
> dependencies, as dell-smbios-smm is already calling dcdbas to generate
> the SMI request.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
>  drivers/platform/x86/dell/dcdbas.c          | 127 +++++++++++---------
>  drivers/platform/x86/dell/dcdbas.h          |   9 ++
>  drivers/platform/x86/dell/dell-smbios-smm.c |  14 ++-
>  3 files changed, 87 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dcdbas.c 
> b/drivers/platform/x86/dell/dcdbas.c
> index 5e63d6225048..02bcac619018 100644
> --- a/drivers/platform/x86/dell/dcdbas.c
> +++ b/drivers/platform/x86/dell/dcdbas.c
> @@ -40,13 +40,10 @@
>  
>  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 *bios_buffer;
> +static struct smi_buffer smi_buf;
>  
>  static unsigned int host_control_action;
>  static unsigned int host_control_smi_type;
> @@ -54,23 +51,49 @@ static unsigned int host_control_on_shutdown;
>  
>  static bool wsmt_enabled;
>  
> +int dcdbas_smi_alloc(struct smi_buffer *smi_buffer, unsigned long size)
> +{
> +     smi_buffer->virt = dma_alloc_coherent(&dcdbas_pdev->dev, size,
> +                                           &smi_buffer->dma, GFP_KERNEL);
> +     if (!smi_buffer->virt) {
> +             dev_dbg(&dcdbas_pdev->dev,
> +                     "%s: failed to allocate memory size %lu\n",
> +                     __func__, size);
> +             return -ENOMEM;
> +     }
> +     smi_buffer->size = size;
> +
> +     dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
> +             __func__, (u32)smi_buffer->dma, smi_buffer->size);
> +
> +     return 0;
> +}
> +EXPORT_SYMBOL_GPL(dcdbas_smi_alloc);
> +
> +void dcdbas_smi_free(struct smi_buffer *smi_buffer)
> +{
> +     if (!smi_buffer->virt)
> +             return;
> +
> +     dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
> +             __func__, (u32)smi_buffer->dma, smi_buffer->size);
> +     dma_free_coherent(&dcdbas_pdev->dev, smi_buffer->size,
> +                       smi_buffer->virt, smi_buffer->dma);
> +     smi_buffer->virt = NULL;
> +     smi_buffer->dma = 0;
> +     smi_buffer->size = 0;
> +}
> +EXPORT_SYMBOL_GPL(dcdbas_smi_free);
> +
>  /**
>   * smi_data_buf_free: free SMI data buffer
>   */
>  static void smi_data_buf_free(void)
>  {
> -     if (!smi_data_buf || wsmt_enabled)
> +     if (!smi_buf.virt || wsmt_enabled)
>               return;
>  
> -     dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
> -             __func__, smi_data_buf_phys_addr, smi_data_buf_size);
> -
> -     dma_free_coherent(&dcdbas_pdev->dev, smi_data_buf_size, smi_data_buf,
> -                       smi_data_buf_handle);
> -     smi_data_buf = NULL;
> -     smi_data_buf_handle = 0;
> -     smi_data_buf_phys_addr = 0;
> -     smi_data_buf_size = 0;
> +     dcdbas_smi_free(&smi_buf);
>  }
>  
>  /**
> @@ -78,39 +101,29 @@ static void smi_data_buf_free(void)
>   */
>  static int smi_data_buf_realloc(unsigned long size)
>  {
> -     void *buf;
> -     dma_addr_t handle;
> +     struct smi_buffer tmp;
> +     int ret;
>  
> -     if (smi_data_buf_size >= size)
> +     if (smi_buf.size >= size)
>               return 0;
>  
>       if (size > max_smi_data_buf_size)
>               return -EINVAL;
>  
>       /* new buffer is needed */
> -     buf = dma_alloc_coherent(&dcdbas_pdev->dev, size, &handle, GFP_KERNEL);
> -     if (!buf) {
> -             dev_dbg(&dcdbas_pdev->dev,
> -                     "%s: failed to allocate memory size %lu\n",
> -                     __func__, size);
> -             return -ENOMEM;
> -     }
> -     /* memory zeroed by dma_alloc_coherent */
> +     ret = dcdbas_smi_alloc(&tmp, size);
> +     if (ret)
> +             return ret;
>  
> -     if (smi_data_buf)
> -             memcpy(buf, smi_data_buf, smi_data_buf_size);
> +     /* memory zeroed by dma_alloc_coherent */
> +     if (smi_buf.virt)
> +             memcpy(tmp.virt, smi_buf.virt, smi_buf.size);
>  
>       /* free any existing buffer */
>       smi_data_buf_free();
>  
>       /* set up new buffer for use */
> -     smi_data_buf = buf;
> -     smi_data_buf_handle = handle;
> -     smi_data_buf_phys_addr = (u32) virt_to_phys(buf);
> -     smi_data_buf_size = size;
> -
> -     dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n",
> -             __func__, smi_data_buf_phys_addr, smi_data_buf_size);
> +     smi_buf = tmp;
>  
>       return 0;
>  }
> @@ -119,14 +132,14 @@ static ssize_t smi_data_buf_phys_addr_show(struct 
> device *dev,
>                                          struct device_attribute *attr,
>                                          char *buf)
>  {
> -     return sprintf(buf, "%x\n", smi_data_buf_phys_addr);
> +     return sprintf(buf, "%x\n", (u32)smi_buf.dma);
>  }
>  
>  static ssize_t smi_data_buf_size_show(struct device *dev,
>                                     struct device_attribute *attr,
>                                     char *buf)
>  {
> -     return sprintf(buf, "%lu\n", smi_data_buf_size);
> +     return sprintf(buf, "%lu\n", smi_buf.size);
>  }
>  
>  static ssize_t smi_data_buf_size_store(struct device *dev,
> @@ -155,8 +168,8 @@ static ssize_t smi_data_read(struct file *filp, struct 
> kobject *kobj,
>       ssize_t ret;
>  
>       mutex_lock(&smi_data_lock);
> -     ret = memory_read_from_buffer(buf, count, &pos, smi_data_buf,
> -                                     smi_data_buf_size);
> +     ret = memory_read_from_buffer(buf, count, &pos, smi_buf.virt,
> +                                     smi_buf.size);
>       mutex_unlock(&smi_data_lock);
>       return ret;
>  }
> @@ -176,7 +189,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
> kobject *kobj,
>       if (ret)
>               goto out;
>  
> -     memcpy(smi_data_buf + pos, buf, count);
> +     memcpy(smi_buf.virt + pos, buf, count);
>       ret = count;
>  out:
>       mutex_unlock(&smi_data_lock);
> @@ -306,11 +319,11 @@ static ssize_t smi_request_store(struct device *dev,
>  
>       mutex_lock(&smi_data_lock);
>  
> -     if (smi_data_buf_size < sizeof(struct smi_cmd)) {
> +     if (smi_buf.size < sizeof(struct smi_cmd)) {
>               ret = -ENODEV;
>               goto out;
>       }
> -     smi_cmd = (struct smi_cmd *)smi_data_buf;
> +     smi_cmd = (struct smi_cmd *)smi_buf.virt;
>  
>       switch (val) {
>       case 2:
> @@ -326,20 +339,20 @@ static ssize_t smi_request_store(struct device *dev,
>                * 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
> +              * Because the address that smi_cmd (smi_buf.virt) 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 +
> +             smi_cmd->ebx = (u32)smi_buf.dma +
>                               offsetof(struct smi_cmd, command_buffer);
>               ret = dcdbas_smi_request(smi_cmd);
>               if (!ret)
>                       ret = count;
>               break;
>       case 0:
> -             memset(smi_data_buf, 0, smi_data_buf_size);
> +             memset(smi_buf.virt, 0, smi_buf.size);
>               ret = count;
>               break;
>       default:
> @@ -356,7 +369,7 @@ EXPORT_SYMBOL(dcdbas_smi_request);
>  /**
>   * host_control_smi: generate host control SMI
>   *
> - * Caller must set up the host control command in smi_data_buf.
> + * Caller must set up the host control command in smi_buf.virt.
>   */
>  static int host_control_smi(void)
>  {
> @@ -367,14 +380,14 @@ static int host_control_smi(void)
>       s8 cmd_status;
>       u8 index;
>  
> -     apm_cmd = (struct apm_cmd *)smi_data_buf;
> +     apm_cmd = (struct apm_cmd *)smi_buf.virt;
>       apm_cmd->status = ESM_STATUS_CMD_UNSUCCESSFUL;
>  
>       switch (host_control_smi_type) {
>       case HC_SMITYPE_TYPE1:
>               spin_lock_irqsave(&rtc_lock, flags);
>               /* write SMI data buffer physical address */
> -             data = (u8 *)&smi_data_buf_phys_addr;
> +             data = (u8 *)&smi_buf.dma;
>               for (index = PE1300_CMOS_CMD_STRUCT_PTR;
>                    index < (PE1300_CMOS_CMD_STRUCT_PTR + 4);
>                    index++, data++) {
> @@ -405,7 +418,7 @@ static int host_control_smi(void)
>       case HC_SMITYPE_TYPE3:
>               spin_lock_irqsave(&rtc_lock, flags);
>               /* write SMI data buffer physical address */
> -             data = (u8 *)&smi_data_buf_phys_addr;
> +             data = (u8 *)&smi_buf.dma;
>               for (index = PE1400_CMOS_CMD_STRUCT_PTR;
>                    index < (PE1400_CMOS_CMD_STRUCT_PTR + 4);
>                    index++, data++) {
> @@ -450,7 +463,7 @@ static int host_control_smi(void)
>   * This function is called by the driver after the system has
>   * finished shutting down if the user application specified a
>   * host control action to perform on shutdown.  It is safe to
> - * use smi_data_buf at this point because the system has finished
> + * use smi_buf.virt at this point because the system has finished
>   * shutting down and no userspace apps are running.
>   */
>  static void dcdbas_host_control(void)
> @@ -464,18 +477,18 @@ static void dcdbas_host_control(void)
>       action = host_control_action;
>       host_control_action = HC_ACTION_NONE;
>  
> -     if (!smi_data_buf) {
> +     if (!smi_buf.virt) {
>               dev_dbg(&dcdbas_pdev->dev, "%s: no SMI buffer\n", __func__);
>               return;
>       }
>  
> -     if (smi_data_buf_size < sizeof(struct apm_cmd)) {
> +     if (smi_buf.size < sizeof(struct apm_cmd)) {
>               dev_dbg(&dcdbas_pdev->dev, "%s: SMI buffer too small\n",
>                       __func__);
>               return;
>       }
>  
> -     apm_cmd = (struct apm_cmd *)smi_data_buf;
> +     apm_cmd = (struct apm_cmd *)smi_buf.virt;
>  
>       /* power off takes precedence */
>       if (action & HC_ACTION_HOST_CONTROL_POWEROFF) {
> @@ -583,11 +596,11 @@ static int dcdbas_check_wsmt(void)
>               return -ENOMEM;
>       }
>  
> -     /* First 8 bytes is for a semaphore, not part of the smi_data_buf */
> -     smi_data_buf_phys_addr = bios_buf_paddr + 8;
> -     smi_data_buf = bios_buffer + 8;
> -     smi_data_buf_size = remap_size - 8;
> -     max_smi_data_buf_size = smi_data_buf_size;
> +     /* First 8 bytes is for a semaphore, not part of the smi_buf.virt */
> +     smi_buf.dma = bios_buf_paddr + 8;
> +     smi_buf.virt = bios_buffer + 8;
> +     smi_buf.size = remap_size - 8;
> +     max_smi_data_buf_size = smi_buf.size;
>       wsmt_enabled = true;
>       dev_info(&dcdbas_pdev->dev,
>                "WSMT found, using firmware-provided SMI buffer.\n");
> diff --git a/drivers/platform/x86/dell/dcdbas.h 
> b/drivers/platform/x86/dell/dcdbas.h
> index c3cca5433525..942a23ddded0 100644
> --- a/drivers/platform/x86/dell/dcdbas.h
> +++ b/drivers/platform/x86/dell/dcdbas.h
> @@ -105,5 +105,14 @@ struct smm_eps_table {
>       u64 num_of_4k_pages;
>  } __packed;
>  
> +struct smi_buffer {
> +     u8 *virt;
> +     unsigned long size;
> +     dma_addr_t dma;
> +};
> +
> +int dcdbas_smi_alloc(struct smi_buffer *smi_buffer, unsigned long size);
> +void dcdbas_smi_free(struct smi_buffer *smi_buffer);
> +
>  #endif /* _DCDBAS_H_ */
>  
> diff --git a/drivers/platform/x86/dell/dell-smbios-smm.c 
> b/drivers/platform/x86/dell/dell-smbios-smm.c
> index 320c032418ac..4d375985c85f 100644
> --- a/drivers/platform/x86/dell/dell-smbios-smm.c
> +++ b/drivers/platform/x86/dell/dell-smbios-smm.c
> @@ -20,6 +20,7 @@
>  
>  static int da_command_address;
>  static int da_command_code;
> +static struct smi_buffer smi_buf;
>  static struct calling_interface_buffer *buffer;
>  static struct platform_device *platform_device;
>  static DEFINE_MUTEX(smm_mutex);
> @@ -57,7 +58,7 @@ static int dell_smbios_smm_call(struct 
> calling_interface_buffer *input)
>       command.magic = SMI_CMD_MAGIC;
>       command.command_address = da_command_address;
>       command.command_code = da_command_code;
> -     command.ebx = virt_to_phys(buffer);
> +     command.ebx = smi_buf.dma;
>       command.ecx = 0x42534931;
>  
>       mutex_lock(&smm_mutex);
> @@ -101,9 +102,10 @@ int init_dell_smbios_smm(void)
>        * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
>        * is passed to SMI handler.
>        */
> -     buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
> -     if (!buffer)
> -             return -ENOMEM;
> +     ret = dcdbas_smi_alloc(&smi_buf, PAGE_SIZE);
> +     if (ret)
> +             return ret;
> +     buffer = (void *)smi_buf.virt;
>  
>       dmi_walk(find_cmd_address, NULL);
>  
> @@ -138,7 +140,7 @@ int init_dell_smbios_smm(void)
>  
>  fail_wsmt:
>  fail_platform_device_alloc:
> -     free_page((unsigned long)buffer);
> +     dcdbas_smi_free(&smi_buf);
>       return ret;
>  }
>  
> @@ -147,6 +149,6 @@ void exit_dell_smbios_smm(void)
>       if (platform_device) {
>               dell_smbios_unregister_device(&platform_device->dev);
>               platform_device_unregister(platform_device);
> -             free_page((unsigned long)buffer);
> +             dcdbas_smi_free(&smi_buf);
>       }
>  }