| 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
| ||
|
Message-ID: <11576596-f0e4-4e88-a200-ed22b86d5974@icloud.com>
Date: Wed, 11 Sep 2024 20:14:48 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Dan Williams <dan.j.williams@...el.com>,
quic_zijuhu <quic_zijuhu@...cinc.com>, Ira Weiny <ira.weiny@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Davidlohr Bueso <dave@...olabs.net>,
Jonathan Cameron <jonathan.cameron@...wei.com>,
Dave Jiang <dave.jiang@...el.com>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>, Timur Tabi <timur@...nel.org>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH v4 1/2] cxl/region: Find free cxl decoder by
device_for_each_child()
On 2024/9/11 02:27, Dan Williams wrote:
> Dan Williams wrote:
> [..]
>> So, while regionB would be the next decoder to allocate after regionC is
>> torn down, it is not a free decoder while decoderC and regionC have not been
>> reclaimed.
>
> The "simple" conversion is bug compatible with the current
> implementation, but here's a path to both constify the
> device_find_child() argument, *and* prevent unwanted allocations by
> allocating decoders precisely by id. Something like this, it passes a
> quick unit test run:
>
sounds good.
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 1d5007e3795a..749a281819b4 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1750,7 +1750,8 @@ static int cxl_decoder_init(struct cxl_port *port, struct cxl_decoder *cxld)
> struct device *dev;
> int rc;
>
> - rc = ida_alloc(&port->decoder_ida, GFP_KERNEL);
> + rc = ida_alloc_max(&port->decoder_ida, CXL_DECODER_NR_MAX - 1,
> + GFP_KERNEL);
> if (rc < 0)
> return rc;
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21ad5f242875..1f7b3a9ebfa3 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -794,26 +794,16 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
> return rc;
> }
>
> -static int match_free_decoder(struct device *dev, void *data)
> +static int match_decoder_id(struct device *dev, void *data)
> {
> struct cxl_decoder *cxld;
> - int *id = data;
> + int id = *(int *) data;
>
> if (!is_switch_decoder(dev))
> return 0;
>
> cxld = to_cxl_decoder(dev);
> -
> - /* enforce ordered allocation */
> - if (cxld->id != *id)
> - return 0;
> -
> - if (!cxld->region)
> - return 1;
> -
> - (*id)++;
> -
> - return 0;
> + return cxld->id == id;
> }
>
> static int match_auto_decoder(struct device *dev, void *data)
> @@ -840,16 +830,29 @@ cxl_region_find_decoder(struct cxl_port *port,
> struct cxl_region *cxlr)
> {
> struct device *dev;
> - int id = 0;
> -
> if (port == cxled_to_port(cxled))
> return &cxled->cxld;
>
> if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
> dev = device_find_child(&port->dev, &cxlr->params,
> match_auto_decoder);
> - else
> - dev = device_find_child(&port->dev, &id, match_free_decoder);
> + else {
> + int id, last;
> +
> + /*
> + * Find next available decoder, but fail new decoder
> + * allocations if out-of-order region destruction has
> + * occurred
> + */
> + id = find_first_zero_bit(port->decoder_alloc,
> + CXL_DECODER_NR_MAX);
> + last = find_last_bit(port->decoder_alloc, CXL_DECODER_NR_MAX);
> +
> + if (id >= CXL_DECODER_NR_MAX ||
> + (last < CXL_DECODER_NR_MAX && id != last + 1))
> + return NULL;
Above finding logic seems wrong.
what about below one ?
last = find_last_bit(port->decoder_alloc, CXL_DECODER_NR_MAX);
if (last >= CXL_DECODER_NR_MAX)
id = 0;
else if (last + 1 < CXL_DECODER_NR_MAX)
id = last + 1;
else
return NULL;
> + dev = device_find_child(&port->dev, &id, match_decoder_id);
> + }
> if (!dev)
> return NULL;
> /*
> @@ -943,6 +946,9 @@ static void cxl_rr_free_decoder(struct cxl_region_ref *cxl_rr)
>
> dev_WARN_ONCE(&cxlr->dev, cxld->region != cxlr, "region mismatch\n");
> if (cxld->region == cxlr) {
> + struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +
> + clear_bit(cxld->id, port->decoder_alloc);
> cxld->region = NULL;
> put_device(&cxlr->dev);
> }
> @@ -977,6 +983,7 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr,
> cxl_rr->nr_eps++;
>
> if (!cxld->region) {
> + set_bit(cxld->id, port->decoder_alloc);
> cxld->region = cxlr;
> get_device(&cxlr->dev);
> }
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 9afb407d438f..750cd027d0b0 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -578,6 +578,9 @@ struct cxl_dax_region {
> struct range hpa_range;
> };
>
> +/* Max as of CXL 3.1 (8.2.4.20.1 CXL HDM Decoder Capability Register) */
> +#define CXL_DECODER_NR_MAX 32
> +
> /**
> * struct cxl_port - logical collection of upstream port devices and
> * downstream port devices to construct a CXL memory
> @@ -591,6 +594,7 @@ struct cxl_dax_region {
> * @regions: cxl_region_ref instances, regions mapped by this port
> * @parent_dport: dport that points to this port in the parent
> * @decoder_ida: allocator for decoder ids
> + * @decoder_alloc: decoder busy/free (@cxld->region set) bitmap
> * @reg_map: component and ras register mapping parameters
> * @nr_dports: number of entries in @dports
> * @hdm_end: track last allocated HDM decoder instance for allocation ordering
> @@ -611,6 +615,7 @@ struct cxl_port {
> struct xarray regions;
> struct cxl_dport *parent_dport;
> struct ida decoder_ida;
> + DECLARE_BITMAP(decoder_alloc, CXL_DECODER_NR_MAX);
> struct cxl_register_map reg_map;
> int nr_dports;
> int hdm_end;
Powered by blists - more mailing lists