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

[Srirangan Madhavan <smadhavan@nvidia.com>]

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