linux-kernel - Re: [PATCH] acpi: Use access_width over register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJZ5v0iHJcZzF=hGLRH+tT6uqCrfHbLw_KJD5dSRRTrsbeVMUQ@mail.gmail.com>
Date:   Tue, 3 Oct 2023 20:50:41 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Jarred White <jarredwhite@...ux.microsoft.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        "open list:ACPI" <linux-acpi@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] acpi: Use access_width over register_width for system
 memory accesses

On Mon, Sep 25, 2023 at 8:06 PM Jarred White
<jarredwhite@...ux.microsoft.com> wrote:
>
> To align with ACPI 6.3+, since bit_width can be any 8-bit value, we cannot
> depend on it being always on a clean 8b boundary. Instead, use access_width
> to determine the size and use the offset and width to shift and mask the
> bit swe want to read/write out. Make sure to add a check for system memory
> since pcc redefines the access_width to subspace id.

This is fine, but what if there are systems in the field where
bit_width is invalid, but they just happen to work because of the way
it is currently handled?

> Signed-off-by: Jarred White <jarredwhite@...ux.microsoft.com>
> ---
>  drivers/acpi/cppc_acpi.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 7ff269a78c20..07619b36c056 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -777,6 +777,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>                         } else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>                                 if (gas_t->address) {
>                                         void __iomem *addr;
> +                                       size_t access_width;
>
>                                         if (!osc_cpc_flexible_adr_space_confirmed) {
>                                                 pr_debug("Flexible address space capability not supported\n");
> @@ -784,7 +785,8 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>                                                         goto out_free;
>                                         }
>
> -                                       addr = ioremap(gas_t->address, gas_t->bit_width/8);
> +                                       access_width = ((8 << (gas_t->access_width - 1)) / 8);

The 8 << (gas_t->access_width - 1) is duplicated twice below.  There
could be an inline function doing that computation.

And the outer parens above are not needed AFAICS.

> +                                       addr = ioremap(gas_t->address, access_width);
>                                         if (!addr)
>                                                 goto out_free;
>                                         cpc_ptr->cpc_regs[i-2].sys_mem_vaddr = addr;
> @@ -980,6 +982,7 @@ int __weak cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
>  static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
>  {
>         void __iomem *vaddr = NULL;
> +       int size;
>         int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>         struct cpc_reg *reg = &reg_res->cpc_entry.reg;
>
> @@ -1015,7 +1018,12 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
>                 return acpi_os_read_memory((acpi_physical_address)reg->address,
>                                 val, reg->bit_width);
>
> -       switch (reg->bit_width) {
> +       if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> +               size = (8 << (reg->access_width - 1));

Code duplication, outer perens not needed.

> +       else
> +               size = reg->bit_width;
> +
> +       switch (size) {
>         case 8:
>                 *val = readb_relaxed(vaddr);
>                 break;
> @@ -1034,12 +1042,16 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
>                 return -EFAULT;
>         }
>
> +       if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> +               *val = (*val >> reg->bit_offset) & GENMASK((reg->bit_width) - 1, 0);

The formula on the right-hand side of this is duplicated below.
Again, there could be an inline function doing this.

> +
>         return 0;
>  }
>
>  static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
>  {
>         int ret_val = 0;
> +       int size;
>         void __iomem *vaddr = NULL;
>         int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>         struct cpc_reg *reg = &reg_res->cpc_entry.reg;
> @@ -1067,7 +1079,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
>                 return acpi_os_write_memory((acpi_physical_address)reg->address,
>                                 val, reg->bit_width);
>
> -       switch (reg->bit_width) {
> +       if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +               size = (8 << (reg->access_width - 1));
> +               val = (val >> reg->bit_offset) & GENMASK((reg->bit_width) - 1, 0);
> +       } else
> +               size = reg->bit_width;

Missing braces (as per the kernel coding style).

> +
> +       switch (size) {
>         case 8:
>                 writeb_relaxed(val, vaddr);
>                 break;
> --
> 2.34.1
>