linux-cxl.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dan Williams <dan.j.williams@intel.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: linux-cxl@vger.kernel.org,
	Chet Douglas <chet.r.douglas@intel.com>,
	Alison Schofield <alison.schofield@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Vishal Verma <vishal.l.verma@intel.com>
Subject: Re: [RFC PATCH v2 08/28] cxl/port: Introduce a port driver
Date: Tue, 2 Nov 2021 10:21:47 -0700	[thread overview]
Message-ID: <CAPcyv4iAitwKm6Gm4KVQ7LX9HWu7vHe9VNtcFHC0dGvXqXFWUw@mail.gmail.com> (raw)
In-Reply-To: <20211102162720.z7b3pwf5xojledv6@intel.com>

On Tue, Nov 2, 2021 at 9:27 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-11-01 20:31:03, Dan Williams wrote:
> > On Mon, Nov 1, 2021 at 10:53 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > On 21-10-29 18:37:36, Dan Williams wrote:
> > > > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > > >
> > >
> > > [snip]
> > >
> > > > > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> > > > > index cf07ae6cea17..40b386aaedf7 100644
> > > > > --- a/drivers/cxl/Makefile
> > > > > +++ b/drivers/cxl/Makefile
> > > > > @@ -1,5 +1,6 @@
> > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > >  obj-$(CONFIG_CXL_BUS) += core/
> > > > > +obj-$(CONFIG_CXL_MEM) += cxl_port.o
> > > >
> > > > It feel odd that CONFIG_CXL_MEM builds cxl_port, why not have a
> > > > CONFIG_CXL_PORT that is simply selected by CONFIG_CXL_MEM, or a
> > > > CONFIG_CXL_PORT that defaults to the value of CONFIG_CXL_BUS?
> > > >
> > >
> > > Can you help me understand when CONFIG_CXL_MEM is useful when
> > > #CONFIG_CXL_PORT=n? I was unable to figure out such a case and so I tied the two
> > > together.
> >
> > With a 'select' dependency it's impossible to have the
> > CONFIG_CXL_PORT=n and CONFIG_CXL_MEM=m combination. The extra config
> > symbol is for idiomatic (one config-symbol per module .ko) reasons to
> > reflect the module dependency in the Kconfig.
>
> Can't argue with idiomisms ;-)

It's idiomatic because it's extensible. I expect type-2 drivers to
select port services as well.

>
> >
> > [..]
> > > > > +static inline int cxl_hdm_decoder_ig(u32 ctrl)
> > > >
> > > > No need for plain inline in C files.
> > > >
> > > > It's not clear why this simple helper needs a "cxl_hdm_decoder"
> > > > namespace prefix?
> > >
> > > I had a patch to share this with acpi driver at one point, but I dropped it. Do
> > > you care if I merge those two decoders, or just rename?
> >
> > Not sure what you mean by "merge"?
> >
>
> To have a unified function for both the cxl_acpi driver and the cxl_port driver.

Ah, yeah, CFMWS_INTERLEAVE_WAYS and CFMWS_INTERLEAVE_GRANULARITY are
doing the same work.

>
> > >
> > > >
> > > > > +{
> > > > > +       int val = FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl);
> > > > > +
> > > > > +       return 8 + val;
> > > > > +}
> > > >
> > > > Why is this return a power of 2 value...
> > >
> > > I don't understand this comment.
> >
> > Isn't this returning an IG as a bit-shift value? The "iw" is returning
> > bytes and I'm proposing they both be bytes.
> >
>
> IG is definitely wrong since it's documented in struct cxl_decoder as data
> stride. I will fix that. I don't follow what you mean by "iw" is returning
> bytes. It's returning the number of ways.

Sorry, some noise from me there. I should have said integer instead of
encoded value.

CFMWS_INTERLEAVE_GRANULARITY has the same problem (another indicator
that a unit test for all user facing values is needed to catch simple
bugs like this).

>
> > >
> > > >
> > > > > +
> > > > > +static inline int cxl_hdm_decoder_iw(u32 ctrl)
> > > > > +{
> > > > > +       int val = FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl);
> > > > > +
> > > > > +       return 1 << val;
> > > >
> > > > ...while this one is converted to absolute values.
> > > >
> > > > These could just be:
> > > >
> > > > unsigned int to_interleave_granularity(u32 ctrl)
> > > > unsigned int to_interleave_ways(u32 ctrl)
> > > >
> > > > ...and return units in bytes.
> > > >
> > > > > +}
> > > > > +
> > > > > +static void get_caps(struct cxl_port *port, struct cxl_port_data *cpd)
> > > > > +{
> > > > > +       void __iomem *hdm_decoder = cpd->regs.hdm_decoder;
> > > > > +       struct port_caps *caps = &cpd->caps;
> > > > > +       u32 hdm_cap;
> > > > > +
> > > > > +       hdm_cap = readl(hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
> > > > > +
> > > > > +       caps->count = cxl_hdm_decoder_count(hdm_cap);
> > > > > +       caps->tc = FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
> > > > > +       caps->interleave11_8 =
> > > > > +               FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap);
> > > > > +       caps->interleave14_12 =
> > > > > +               FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap);
> > > > > +}
> > > > > +
> > > > > +static int map_regs(struct cxl_port *port, void __iomem *crb,
> > > > > +                   struct cxl_port_data *cpd)
> > > > > +{
> > > > > +       struct cxl_register_map map;
> > > > > +       struct cxl_component_reg_map *comp_map = &map.component_map;
> > > > > +
> > > > > +       cxl_probe_component_regs(&port->dev, crb, comp_map);
> > > > > +       if (!comp_map->hdm_decoder.valid) {
> > > > > +               dev_err(&port->dev, "HDM decoder registers invalid\n");
> > > > > +               return -ENXIO;
> > > > > +       }
> > > >
> > > > Perhaps promote cxl_probe_regs() from the cxl_pci to the core and make
> > > > it take a dev instead of a pdev, then you can do:
> > > >
> > > > cxl_probe_regs(&port_dev->dev, CXL_REGLOC_RBI_COMPONENT)
> > > >
> > > > ...instead of open coding it again?
> > > >
> > > > > +
> > > > > +       cpd->regs.hdm_decoder = crb + comp_map->hdm_decoder.offset;
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static u64 get_decoder_size(void __iomem *hdm_decoder, int n)
> > > > > +{
> > > > > +       u32 ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(n));
> > > > > +
> > > > > +       if (!!FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> > > > > +               return 0;
> > > > > +
> > > > > +       return ioread64_hi_lo(hdm_decoder +
> > > > > +                             CXL_HDM_DECODER0_SIZE_LOW_OFFSET(n));
> > > > > +}
> > > > > +
> > > > > +static bool is_endpoint_port(struct cxl_port *port)
> > > > > +{
> > > > > +       if (!port->uport->driver)
> > > > > +               return false;
> > > > > +
> > > > > +       return to_cxl_drv(port->uport->driver)->id ==
> > > > > +              CXL_DEVICE_MEMORY_EXPANDER;
> > > >
> > > > Why does endpoint port device type determination need to reach through
> > > > and read the driver type?
> > > >
> > >
> > > I couldn't figure out a better way at this point in enumeration. I'm open to
> > > suggestions.
> >
> > list_empty(&port->dports)?
> >
>
> That seems obvious enough. In the v2 series it's possible due to failures for a
> switch port to also have list_empty(&port->dports). Should ports not get added
> if they have 0 dports (unless they're endpoints)?

Sounds reasonable, I can't imagine a usable non-endpoint port that has
no dports.

>
> >
> > >
> > > [snip]
> > >
> > > > > +
> > > > > +       /*
> > > > > +        * Enable HDM decoders for this port.
> > > > > +        *
> > > > > +        * FIXME: If the component was using DVSEC range registers for decode,
> > > > > +        * this will destroy that.
> > > >
> > > > Yeah, definitely need to check that before this patch can move
> > > > forward. Perhaps a port should not even be registered if DVSEC
> > > > Memory_Size && Mem_Enable are non zero, that device is explicitly
> > > > opting out of being a part of the CXL 2.0 subsystem hierarchy.
> > > > However, we might still need to track it and potentially reserve it
> > > > out of CFMWS capacity to make sure nothing else collides with it. I'll
> > > > also note that "ECN: Devices operating in CXL 1.1 mode with no RCRB"
> > > > was recently published that reads on what the driver should do here.
> > > >
> > >
> > > I believe we want to create the port since we might decide to reset and want
> > > control back over it and as you said, safety check other things. The reason it
> > > was left as a FIXME is because this belongs in the PCI driver which I didn't
> > > really want to touch at this time. I will go back and add that for the next
> > > version.
> >
> > Ok.
> >
> > >
> > > > > +        */
> > > > > +       ctrl = readl(portdata->regs.hdm_decoder + CXL_HDM_DECODER_CTRL_OFFSET);
> > > > > +       ctrl |= CXL_HDM_DECODER_ENABLE;
> > > > > +       writel(ctrl, portdata->regs.hdm_decoder + CXL_HDM_DECODER_CTRL_OFFSET);
> > > >
> > > > I feel like that if the driver finds it enabled it should leave it
> > > > enabled at ->remove() time, as you have it here, as BIOS might not be
> > > > expecting the OS to disable a decoder it set up. However, if the
> > > > driver actually does the enable, then it should pair it with a disable
> > > > at the end of time, so not a blind enable, but one that conditionally
> > > > arranges for the unwind.
> > >
> > > My thought was that once we enumerate a port, all of it's architectural state
> > > belongs to the OS. For us that means blanket enable/disable. I don't feel
> > > strongly about this.
> >
> > I think the driver needs to tread carefully when it comes to potential
> > BIOS interactions. The minimum BIOS collision would be to not toggle
> > the enable off if the OS cannot assert that it set it. A more precise
> > policy would be checking if the device contributes to any EFI memory
> > map ranges, or any locked CFMWS entries.
>
> Locked == bit 4, fixed, right? The more precise policy makes sense to me, though
> I still wonder what happens if we reset a device for that case?

I expect it will be painful and likely crash the system if that device
is contributing to active System RAM. There is definitely some more
exclusion needed to coordinate with secondary bus reset on the PCI
side. I can imagine someone trying to passthrough a CXL device to a VM
unaware that it will trigger a reset and crash the system.

  reply	other threads:[~2021-11-02 17:22 UTC|newest]

Thread overview: 112+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-22 18:36 [RFC PATCH v2 00/28] CXL Region Creation / HDM decoder programming Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 01/28] cxl: Rename CXL_MEM to CXL_PCI Ben Widawsky
2021-10-29 20:15   ` Dan Williams
2021-10-29 21:20     ` Ben Widawsky
2021-10-29 21:39       ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 02/28] cxl: Move register block enumeration to core Ben Widawsky
2021-10-29 20:23   ` Dan Williams
2021-10-29 21:23     ` Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 03/28] cxl/acpi: Map component registers for Root Ports Ben Widawsky
2021-10-29 20:28   ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 04/28] cxl: Add helper for new drivers Ben Widawsky
2021-10-29 20:30   ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 05/28] cxl/core: Convert decoder range to resource Ben Widawsky
2021-10-29 20:50   ` Dan Williams
2021-10-29 21:26     ` Ben Widawsky
2021-10-29 22:22       ` Dan Williams
2021-10-29 22:37         ` Ben Widawsky
2021-11-01 14:33           ` Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 06/28] cxl: Introduce endpoint decoders Ben Widawsky
2021-10-29 21:00   ` Dan Williams
2021-10-29 22:02     ` Ben Widawsky
2021-10-29 22:25       ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 07/28] cxl/core: Move target population locking to caller Ben Widawsky
2021-10-29 23:03   ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 08/28] cxl/port: Introduce a port driver Ben Widawsky
2021-10-30  1:37   ` Dan Williams
2021-10-31 17:53     ` Dan Williams
2021-10-31 18:10       ` Dan Williams
2021-11-01 17:36         ` Ben Widawsky
2021-11-01 17:53     ` Ben Widawsky
2021-11-01 17:54       ` Ben Widawsky
2021-11-02  3:31       ` Dan Williams
2021-11-02 16:27         ` Ben Widawsky
2021-11-02 17:21           ` Dan Williams [this message]
2021-11-02 16:58         ` Ben Widawsky
2021-11-04 19:10           ` Dan Williams
2021-11-04 19:49             ` Ben Widawsky
2021-11-04 20:04               ` Dan Williams
2021-11-04 21:25                 ` Ben Widawsky
2021-11-04 16:37     ` Ben Widawsky
2021-11-04 19:17       ` Dan Williams
2021-11-04 19:46         ` Ben Widawsky
2021-11-04 20:00           ` Dan Williams
2021-11-04 21:26             ` Ben Widawsky
2021-11-03 15:18   ` Jonathan Cameron
2021-10-22 18:36 ` [RFC PATCH v2 09/28] cxl/acpi: Map single port host bridge component registers Ben Widawsky
2021-10-31 18:03   ` Dan Williams
2021-11-01 17:07     ` Ben Widawsky
2021-11-02  2:15       ` Dan Williams
2021-11-02 16:31         ` Ben Widawsky
2021-11-02 17:46           ` Dan Williams
2021-11-02 17:57             ` Ben Widawsky
2021-11-02 18:10               ` Dan Williams
2021-11-02 18:27                 ` Ben Widawsky
2021-11-02 18:49                   ` Dan Williams
2021-11-02 21:15                     ` Ben Widawsky
2021-11-02 21:34                       ` Dan Williams
2021-11-02 21:47                         ` Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 10/28] cxl/core: Store global list of root ports Ben Widawsky
2021-10-31 18:32   ` Dan Williams
2021-11-01 18:43     ` Ben Widawsky
2021-11-02  2:04       ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 11/28] cxl/acpi: Rescan bus at probe completion Ben Widawsky
2021-10-31 19:25   ` Dan Williams
2021-11-01 18:56     ` Ben Widawsky
2021-11-01 21:45       ` Ben Widawsky
2021-11-02  1:56         ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 12/28] cxl/core: Store component register base for memdevs Ben Widawsky
2021-10-31 20:13   ` Dan Williams
2021-11-01 21:50     ` Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 13/28] cxl: Flesh out register names Ben Widawsky
2021-10-31 20:18   ` Dan Williams
2021-11-01 22:00     ` Ben Widawsky
2021-11-02  1:53       ` Dan Williams
2021-11-03 15:53   ` Jonathan Cameron
2021-11-03 16:03     ` Ben Widawsky
2021-11-03 16:42       ` Jonathan Cameron
2021-11-03 17:05         ` Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 14/28] cxl: Hide devm host for ports Ben Widawsky
2021-10-31 21:14   ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 15/28] cxl/core: Introduce API to scan switch ports Ben Widawsky
2021-11-01  5:39   ` Dan Williams
2021-11-01 22:56     ` Ben Widawsky
2021-11-02  1:45       ` Dan Williams
2021-11-02 16:39         ` Ben Widawsky
2021-11-02 20:00           ` Dan Williams
2021-11-16 16:50         ` Ben Widawsky
2021-11-16 17:51           ` Dan Williams
2021-11-16 18:02             ` Ben Widawsky
2021-11-03 16:08   ` Jonathan Cameron
2021-11-10 17:49     ` Ben Widawsky
2021-11-10 18:10       ` Jonathan Cameron
2021-11-10 21:03         ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 16/28] cxl: Introduce cxl_mem driver Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 17/28] cxl: Disable switch hierarchies for now Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 18/28] cxl/region: Add region creation ABI Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 19/28] cxl/region: Introduce concept of region configuration Ben Widawsky
2021-12-15 17:47   ` Jonathan Cameron
2021-10-22 18:37 ` [RFC PATCH v2 20/28] cxl/region: Introduce a cxl_region driver Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 21/28] cxl/acpi: Handle address space allocation Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 22/28] cxl/region: Address " Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 23/28] cxl/region: Implement XHB verification Ben Widawsky
2022-01-06 16:55   ` Jonathan Cameron
2022-01-06 16:58     ` Ben Widawsky
2022-01-06 17:33       ` Jonathan Cameron
2022-01-06 18:10         ` Jonathan Cameron
2022-01-06 18:34           ` Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 24/28] cxl/region: HB port config verification Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 25/28] cxl/region: Record host bridge target list Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 26/28] cxl/mem: Store the endpoint's uport Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 27/28] cxl/region: Gather HDM decoder resources Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 28/28] cxl: Program decoders for regions Ben Widawsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPcyv4iAitwKm6Gm4KVQ7LX9HWu7vHe9VNtcFHC0dGvXqXFWUw@mail.gmail.com \
    --to=dan.j.williams@intel.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=alison.schofield@intel.com \
    --cc=ben.widawsky@intel.com \
    --cc=chet.r.douglas@intel.com \
    --cc=ira.weiny@intel.com \
    --cc=linux-cxl@vger.kernel.org \
    --cc=vishal.l.verma@intel.com \
    --subject='Re: [RFC PATCH v2 08/28] cxl/port: Introduce a port driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).