Re: [PATCH v15 09/16] PCI/CXL: Cache endpoint HDM state during PCI enumeration

[Srirangan Madhavan]

On 10/28/26 8:19 PM, Alison Schofield wrote:
> External email: Use caution opening links or attachments
>
>
> On Sun, Oct 11, 2026 at 02:14:15AM +0000, Srirangan Madhavan wrote:
> > Cache HDM state in pci_dev->hdm after BAR resource setup and before driver
> > binding, so reset can restore it without a bound CXL driver.
> >
> > Save the HDM register location, decoder settings, and Device DVSEC state.
> > Validate capability reads and decoder counts, and restore PCI_COMMAND
> > after temporary MMIO access, reporting restoration failures.
> >
> > Keep PCI discovery separate from shared HDM helpers. Move cxl_rwsem into
> > built-in state code and protect cache publication with its DPA lock, so
> > the cache remains usable when cxl_core is modular.
>
> Hi Srirangan,
>
> I felt the large-ness of this patch, so I'm coming back to the top
> here to add this comment after doing the inline review below.
>
> The commit message doesn't mention much of the preparation work that's
> included. Could that prep be presented first in prep patches, and then
> add the new code?  Something like:
>
> cxl: Build register block helpers into the kernel
> - regs.o moves from cxl_core into the built-in objects
> - devm_cxl_iomap_block(), cxl_rcrb_to_aer(), __rcrb_to_component()
>    and cxl_rcd_component_reg_phys() gain exports
> - cxl_find_regblock() and cxl_setup_regs() move to include/cxl/pci.h
> - struct cxl_register_map gains bar and bar_offset
>
> cxl: Move cxl_rwsem into built-in code
> - a new hdm_state.c holds cxl_rwsem and its export, and hdm.c drops it

Thank you for the break down. In v16 I moved the preparation into 
separate patches before the PCI HDM cache patch:

Patch 8: cxl: Move cxl_rwsem into built-in code
Patch 10: cxl: Build register block helpers into the kernel

> > @@ -8,7 +8,6 @@ CFLAGS_trace.o = -DTRACE_INCLUDE_PATH=. -I$(src)
> >
> >   cxl_core-y := port.o
> >   cxl_core-y += pmem.o
> > -cxl_core-y += regs.o
>
> Does all of reg.c need to be built in? I'm implying that that you'd
> only build in the part that PCI core needs and leave the rest in
> cxl_core. Maybe moving more code around is not worth it. Just noticing.
>
>
> snip

I understand that adding in reg.c includes the unused parts of it.
But I looked at splitting out only the helpers needed by PCI, and that 
would move roughly 300-400 lines into a separate file. I kept the whole 
file built in for now to avoid that code movement.
Please let me know if you feel that this needs to be moved after v16, 
I'll add a patch for that.

> > +static int cxl_hdm_read_decoder_settings(struct device *dev,
> > +                                      struct cxl_endpoint_decoder_settings *settings,
> > +                                      void __iomem *hdm, int id)
> > +{
> > +     u64 base, size;
> > +     u32 ctrl;
> > +     int rc;
> > +
> > +     ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> > +     base = ((u64)readl(hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id)) << 32) |
> > +            readl(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> > +     size = ((u64)readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id)) << 32) |
> > +            readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
> > +     rc = cxl_hdm_unpack_decoder(&settings->config, id, ctrl, base, size);
>
>
> This duplicates work of init_hdm_decoder().
> Could the new shared helper _unpack_decoder() grow to do the register
> reads also, so the code is shared. Also, then you could do the reads
> in low then high order, like init_hdm_decoder() does and like the
> rest of the CXL code reads its Low/High register pairs.
>
> snip

Done in v16

> > +}
>
> Add a blank line before the final return please. (subsystem style)
>
> snip

Added in v16.

> > +static void cxl_hdm_cache_dvsec_ctrl(struct pci_dev *pdev, u16 ctrl)
> > +{
> > +     guard(rwsem_write)(&cxl_rwsem.dpa);
> > +     if (!pdev->hdm)
> > +             return;
> > +
> > +     pdev->hdm->dvsec_ctrl = ctrl;
> > +     pdev->hdm->dvsec_ctrl_valid = true;
>
>
>
> Could hdm_state.c add accessors for this, like the existing
> cxl_hdm_cache_present() and cxl_hdm_cache_publish()? That way cxl_core
> doesn't write pdev->hdm directly and the dpa locking stays in one place.
> The same could be done for cxl_hdm_get_cached_info() and
> cxl_hdm_check_decoder_count() added in Patch 8/16.
Done in v16:
Patch 9, cxl: Refresh cached PCI HDM decoder settings, moves the decoder 
refresh and decoder-count checks into hdm_state.c.

Patch 11, PCI/CXL: Cache endpoint HDM state during PCI enumeration, 
moves the DVSEC control update there as well.

These functions take cxl_rwsem.dpa, so the callers no longer access 
pdev->hdm directly.

> Can this function end in a single return, with the 0 or 1 result
> in a variable, so that the cache is updated in one place?
>
> snip
Fixed in v16.
> > @@ -200,6 +200,7 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> >
> >        return ret_val;
> >   }
> > +EXPORT_SYMBOL_NS_GPL(devm_cxl_iomap_block, "CXL");
>
>
> Why not EXPORT_SYMBOL_FOR_MODULES(..., "cxl_core") here, like the hdm_regs.c
> helpers?
>
> snip

I changed this to use EXPORT_SYMBOL_FOR_MODULES in v16 patch 10.

> > +static struct cxl_hdm_info *__cxl_pci_hdm_read_info(struct pci_dev *pdev,
> > +                                                 struct cxl_register_map *map,
> > +                                                 int dvsec)
> > +{
> > +     struct cxl_reg_map *hdm_map = &map->component_map.hdm_decoder;
> > +     resource_size_t hdm_offset;
> > +     void __iomem *hdm;
> > +     int decoder_count;
> > +     u16 dvsec_ctrl;
> > +     int rc;
> > +
> > +     hdm = cxl_pci_hdm_map_regs(pdev, map, &hdm_offset, &decoder_count);
> > +     if (IS_ERR(hdm))
> > +             return ERR_CAST(hdm);
> > +
> > +     rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL,
> > +                               &dvsec_ctrl);
> > +     if (rc) {
> > +             rc = pcibios_err_to_errno(rc);
> > +             goto out_unmap;
> > +     }
> > +
> > +     struct cxl_hdm_info *info =
> > +             kzalloc_flex(*info, settings, decoder_count, GFP_KERNEL);
>
> Move declaration to top of func.
> checkpatch should be complaining about mixing statement & declarations.

Done in v16. But I rechecked and for some reason checkpatch is not 
flagging this.

> > +     if (!info) {
> > +             rc = -ENOMEM;
> > +             goto out_unmap;

> This returns the __cxl_pci_hdm_read_info() result from two places, and then
> has three more returns after a failed PCI_COMMAND restore. Try to end in a
> single return. Li Ming's suggestion to warn and keep the info on a failed
> restore would also reduce the tail to one return info

Ack. Fixed this in v16.

> > +             return rc;
> > +     if (!(cap & PCI_DVSEC_CXL_MEM_CAPABLE) ||
> > +         !cxl_dvsec_hdm_count_valid(FIELD_GET(PCI_DVSEC_CXL_HDM_COUNT, cap)))
> > +             return 0;
> > +
>
> This cache is built for every function w CXL.mem, not just type2.
> So I believe we are needlessly reading Type decoders at PCI enumeration.
>
> Can this check the capability bits like cxl_reset_function() does
> and limit this work to type2 devices that can use it?
>
> snip to end...
>
> -- Alison

Done in v16 patch 11. Cache initialization now checks for CXL.cache, 
CXL.mem, CXL Reset, and Reset Memory Clear capability before reading the 
HDM decoder state.

--
Regards,
Srirangan