linux-kernel - Re: [PATCH 1/4] cxl/mem: Fully decode device capability header

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPcyv4iV-Xq7eGBHavETiKW=kfucxDnua3a4j6MKtswz=n4gRA@mail.gmail.com>
Date:   Wed, 19 May 2021 17:50:44 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     "Weiny, Ira" <ira.weiny@...el.com>
Cc:     Ben Widawsky <ben.widawsky@...el.com>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        linux-cxl@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] cxl/mem: Fully decode device capability header

On Thu, May 6, 2021 at 3:37 PM <ira.weiny@...el.com> wrote:
>
> From: Ira Weiny <ira.weiny@...el.com>
>
> Previously only the capability ID and offset were decoded.
>
> Create a version MASK and decode the additional version and length
> fields of the header.
>

I'm not seeing a justification for why Linux would want this patch?

> Signed-off-by: Ira Weiny <ira.weiny@...el.com>
> ---
>  drivers/cxl/core.c | 15 ++++++++++++---
>  drivers/cxl/cxl.h  |  1 +
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index b3c3532b53f7..21553386e218 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -501,12 +501,21 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base,
>
>         for (cap = 1; cap <= cap_count; cap++) {
>                 void __iomem *register_block;
> -               u32 offset;
> +               u32 hdr, offset, __maybe_unused length;

What's the point of reading the length and not using it? If this is
used in a future patch then wait until then to add it.

>                 u16 cap_id;
> +               u8 version;
> +
> +               hdr = readl(base + cap * 0x10);
> +
> +               cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK, hdr);
> +               version = FIELD_GET(CXLDEV_CAP_HDR_VERSION_MASK, hdr);
> +               if (version != 1)
> +                       dev_err(dev, "Vendor cap ID: %x incorrect version (0x%x)\n",
> +                               cap_id, version);

It's not an error. Any future version needs to be backwards
compatible. All this is doing is ensuring that when hardware is
updated old kernels will start spamming the log.

>
> -               cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
> -                                  readl(base + cap * 0x10));
>                 offset = readl(base + cap * 0x10 + 0x4);
> +               length = readl(base + cap * 0x10 + 0x8);
> +
>                 register_block = base + offset;
>
>                 switch (cap_id) {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0211f44c95a2..9b315c069557 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -15,6 +15,7 @@
>  #define   CXLDEV_CAP_ARRAY_COUNT_MASK GENMASK_ULL(47, 32)
>  /* CXL 2.0 8.2.8.2 CXL Device Capability Header Register */
>  #define CXLDEV_CAP_HDR_CAP_ID_MASK GENMASK(15, 0)
> +#define CXLDEV_CAP_HDR_VERSION_MASK GENMASK(23, 16)
>  /* CXL 2.0 8.2.8.2.1 CXL Device Capabilities */
>  #define CXLDEV_CAP_CAP_ID_DEVICE_STATUS 0x1
>  #define CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX 0x2
> --
> 2.28.0.rc0.12.gb6a658bd00c9
>