| 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
| ||
|
Date: Fri, 15 Sep 2023 23:53:10 +0200
From: Robert Richter <rrichter@....com>
To: Dan Williams <dan.j.williams@...el.com>
Cc: Terry Bowman <terry.bowman@....com>, alison.schofield@...el.com,
vishal.l.verma@...el.com, ira.weiny@...el.com, bwidawsk@...nel.org,
dave.jiang@...el.com, Jonathan.Cameron@...wei.com,
linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
bhelgaas@...gle.com
Subject: Re: [PATCH v10 04/15] cxl/hdm: Use stored Component Register
mappings to map HDM decoder capability
Dan,
On 14.09.23 17:15:44, Dan Williams wrote:
> Dan Williams wrote:
> > Terry Bowman wrote:
> > > From: Robert Richter <rrichter@....com>
> > >
> > > Now, that the Component Register mappings are stored, use them to
> > > enable and map the HDM decoder capabilities. The Component Registers
> > > do not need to be probed again for this, remove probing code.
> > >
> > > The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> > > Endpoint's component register mappings are located in the cxlds and
> > > else in the port's structure. Provide a helper function
> > > cxl_port_get_comp_map() to locate the mappings depending on the
> > > component's type.
> > >
> > > Signed-off-by: Robert Richter <rrichter@....com>
> > > Signed-off-by: Terry Bowman <terry.bowman@....com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > > Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> > > ---
> > > drivers/cxl/core/hdm.c | 65 +++++++++++++++++++++++-------------------
> > > 1 file changed, 35 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > index 17c8ba8c75e0..892a1fb5e4c6 100644
> > > --- a/drivers/cxl/core/hdm.c
> > > +++ b/drivers/cxl/core/hdm.c
> > > @@ -81,27 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
> > > cxlhdm->interleave_mask |= GENMASK(14, 12);
> > > }
> > >
> > > -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> > > - struct cxl_component_regs *regs)
> > > -{
> > > - struct cxl_register_map map = {
> > > - .dev = &port->dev,
> > > - .resource = port->component_reg_phys,
> > > - .base = crb,
> > > - .max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> > > - };
> > > -
> > > - cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> > > - if (!map.component_map.hdm_decoder.valid) {
> > > - dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> > > - /* unique error code to indicate no HDM decoder capability */
> > > - return -ENODEV;
> > > - }
> > > -
> > > - return cxl_map_component_regs(&map, &port->dev, regs,
> > > - BIT(CXL_CM_CAP_CAP_ID_HDM));
> > > -}
> > > -
> > > static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> > > {
> > > struct cxl_hdm *cxlhdm;
> > > @@ -146,6 +125,22 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> > > return true;
> > > }
> > >
> > > +static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
> > > +{
> > > + /*
> > > + * HDM capability applies to Endpoints, USPs and VH Host
> > > + * Bridges. The Endpoint's component register mappings are
> > > + * located in the cxlds.
> > > + */
> > > + if (is_cxl_endpoint(port)) {
> > > + struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
> > > +
> > > + return &memdev->cxlds->comp_map;
> > > + }
> > > +
> > > + return &port->comp_map;
> > > +}
> >
> > This was the function I was hoping would disappear in the new version.
> > cxl_pci and cxl_port care about different register blocks and have
> > different mapping lifetimes. I think that justifies having the
> > endpoint->comp_map be valid for everything that the cxl_port driver
> > cares about even though it duplicates some informatiom from
> > cxlds->comp_map.
>
> In the interest of time I went ahead and reflowed this patch to the
> below and it is passing my tests. I also noticed some other @dev vs
> @host confusion in some of the previous register conversion so perhaps I
> should just send out v11 with this all rolled together...
just a quick update here.
We were going to send v11 a couple of days ago but I found an issue
during testing, see below. If you don't mind I will send it out next
week with a fix for that included.
>
> -- >8 --
> Subject: cxl/hdm: Use stored Component Register mappings to map HDM decoder capability
>
> From: Robert Richter <rrichter@....com>
>
> Now, that the Component Register mappings are stored, use them to
> enable and map the HDM decoder capabilities. The Component Registers
> do not need to be probed again for this, remove probing code.
>
> The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> Endpoint's component register mappings are located in the cxlds and
> else in the port's structure. Duplicate the cxlds->comp_map in
> port->comp_map for endpoint ports.
>
> Signed-off-by: Robert Richter <rrichter@....com>
> Signed-off-by: Terry Bowman <terry.bowman@....com>
> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> [rework to drop cxl_port_get_comp_map()]
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> ---
> drivers/cxl/core/hdm.c | 48 +++++++++++++++++++----------------------------
> drivers/cxl/core/port.c | 29 ++++++++++++++++++++++------
> drivers/cxl/mem.c | 5 ++---
> 3 files changed, 43 insertions(+), 39 deletions(-)
Patch look good to me.
I have a similar implementation, but did that with a
cxl_port_clone_regs() function in cxl_endpoint_port_probe(). I can
take this version instead.
During testing I found an issue freeing IO resources with devm for RCH
mode. The endpoint is not removed if the cxl_mem driver is
unbound. Then, the resources of the endpoint that also holds the IO
mappings are not freed. A subsequent IO map fails when rebinding the
driver again. It looks like cxl_mem_find_port() is broken for RCDs
preventing the port from being autoremoved. I am working on a fix for
this and will test the whole series again.
Thanks,
-Robert
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 11d9971f3e8c..ced7801516d2 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -81,26 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
> cxlhdm->interleave_mask |= GENMASK(14, 12);
> }
>
> -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> - struct cxl_component_regs *regs)
> -{
> - struct cxl_register_map map = {
> - .host = &port->dev,
> - .resource = port->component_reg_phys,
> - .base = crb,
> - .max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> - };
> -
> - cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> - if (!map.component_map.hdm_decoder.valid) {
> - dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> - /* unique error code to indicate no HDM decoder capability */
> - return -ENODEV;
> - }
> -
> - return cxl_map_component_regs(&map, regs, BIT(CXL_CM_CAP_CAP_ID_HDM));
> -}
> -
> static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> {
> struct cxl_hdm *cxlhdm;
> @@ -155,7 +135,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
> {
> struct device *dev = &port->dev;
> struct cxl_hdm *cxlhdm;
> - void __iomem *crb;
> + struct cxl_register_map *comp_map;
> int rc;
>
> cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
> @@ -164,19 +144,29 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
> cxlhdm->port = port;
> dev_set_drvdata(dev, cxlhdm);
>
> - crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
> - if (!crb && info && info->mem_enabled) {
> - cxlhdm->decoder_count = info->ranges;
> - return cxlhdm;
> - } else if (!crb) {
> + comp_map = &port->comp_map;
> + if (comp_map->resource == CXL_RESOURCE_NONE) {
> + if (info && info->mem_enabled) {
> + cxlhdm->decoder_count = info->ranges;
> + return cxlhdm;
> + }
> + WARN_ON(1);
> dev_err(dev, "No component registers mapped\n");
> return ERR_PTR(-ENXIO);
> }
>
> - rc = map_hdm_decoder_regs(port, crb, &cxlhdm->regs);
> - iounmap(crb);
> - if (rc)
> + if (!comp_map->component_map.hdm_decoder.valid) {
> + dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> + /* unique error code to indicate no HDM decoder capability */
> + return ERR_PTR(-ENODEV);
> + }
> +
> + rc = cxl_map_component_regs(comp_map, &cxlhdm->regs,
> + BIT(CXL_CM_CAP_CAP_ID_HDM));
> + if (rc) {
> + dev_dbg(dev, "Failed to map HDM capability.\n");
> return ERR_PTR(rc);
> + }
>
> parse_hdm_decoder_caps(cxlhdm);
> if (cxlhdm->decoder_count == 0) {
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 3732925162e4..64fcb5b22372 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -741,16 +741,31 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
> return port;
>
> dev = &port->dev;
> - if (is_cxl_memdev(uport_dev))
> + if (is_cxl_memdev(uport_dev)) {
> + struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev);
> + struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> rc = dev_set_name(dev, "endpoint%d", port->id);
> - else if (parent_dport)
> + if (rc)
> + goto err;
> +
> + /*
> + * The endpoint driver already enumerated the component and RAS
> + * registers. Reuse that enumeration while prepping them to be
> + * mapped by the cxl_port driver.
> + */
> + port->comp_map = cxlds->comp_map;
> + port->comp_map.host = &port->dev;
> + } else if (parent_dport) {
> rc = dev_set_name(dev, "port%d", port->id);
> - else
> - rc = dev_set_name(dev, "root%d", port->id);
> - if (rc)
> - goto err;
> + if (rc)
> + goto err;
>
> - rc = cxl_port_setup_regs(port, component_reg_phys);
> + rc = cxl_port_setup_regs(port, component_reg_phys);
> + if (rc)
> + goto err;
> + } else
> + rc = dev_set_name(dev, "root%d", port->id);
> if (rc)
> goto err;
>
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 317c7548e4e9..04107058739b 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -49,7 +49,6 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> struct cxl_dport *parent_dport)
> {
> struct cxl_port *parent_port = parent_dport->port;
> - struct cxl_dev_state *cxlds = cxlmd->cxlds;
> struct cxl_port *endpoint, *iter, *down;
> int rc;
>
> @@ -65,8 +64,8 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> ep->next = down;
> }
>
> - endpoint = devm_cxl_add_port(host, &cxlmd->dev,
> - cxlds->component_reg_phys,
> + /* Note: endpoint port component registers are derived from @cxlds */
> + endpoint = devm_cxl_add_port(host, &cxlmd->dev, CXL_RESOURCE_NONE,
> parent_dport);
> if (IS_ERR(endpoint))
> return PTR_ERR(endpoint);
Powered by blists - more mailing lists