Re: [RFC PATCH v2 08/28] cxl/port: Introduce a port driver

[Ben Widawsky <ben.widawsky@intel.com>]

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 ;-)

> 
> [..]
> > > > +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.

> >
> > >
> > > > +{
> > > > +       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.

> >
> > >
> > > > +
> > > > +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)?

> 
> >
> > [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'll work on the more precise version.

> 
> [..]
> > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > > index 5e2e93451928..bd7b008207a4 100644
> > > --- a/drivers/cxl/cxl.h
> > > +++ b/drivers/cxl/cxl.h
> > > @@ -302,6 +302,10 @@ struct cxl_decoder *cxl_decoder_alloc(struct
> > > cxl_port *port, int nr_targets);
> > >  int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
> > >  int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
> > >
> > > +void set_cxl_topology_host(struct device *dev);
> > > +struct device *get_cxl_topology_host(void);
> > > +void put_cxl_topology_host(struct device *dev);
> > > +
> > >  extern struct bus_type cxl_bus_type;
> > >
> > >  struct cxl_driver {
> >
> > As you've seen by now I do implement something like that you have below in a
> > later patch. I believe it will still be nice to have, but I haven't read through
> > all of your feedback yet. So I might change my mind.
> 
> The main difference I proposed is a {get,put}_cxl_topology_host() to
> wrap topology operations in a rwsem.

Makes sense. I needed a topology lock eventually anyway.