Re: [PATCH v12 07/12] cxl: Validate HDM ranges before CXL reset

[Srirangan Madhavan]

On 9/11/26 6:33 PM, Jonathan Cameron wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, 10 Sep 2026 07:08:03 +0000
> Srirangan Madhavan <smadhavan@xxxxxxxxxx> wrote:
>
> > Before reset, require cached HDM decoder state, collect enabled decoder
> > ranges, and reserve them with request_mem_region(). This rejects reset
> > while affected CXL memory is busy and keeps the validation stable
> > through reset.
> >
> > If CPU cache invalidation support is available, invalidate the affected
> > ranges before reset.
>
> If it is not what happens? I'm thinking dead system as coherency just broke
> but maybe not. My gut is no reset without cache invalidation support.
>
> In general document your cache invalidation flow in this commit message. I'd
> suspicious you don't have a pair of invalidations, one before to ensure
> all caches are clean and one after to evict prefetched lines if your
> device was mean enough to serve garbage during the reset
> (which many will do!)
>
> If more comes in later patches, but a breadcrumb in here to say so.
>
> > If the runtime backend is unavailable, continue
> > after the range reservation succeeds.
> >
> > Reject CXL Reset when no cached HDM decoder state is available. The reset
> > path needs the cached address map to validate affected ranges and perform
> > CPU cache invalidation. Also reject normalized-addressing decoders for
> > now because the cached decoder range is not a system physical address.
> >
> > Signed-off-by: Srirangan Madhavan <smadhavan@xxxxxxxxxx>
>
>
>
> > +static int cxl_hdm_range_flush_cache(struct cxl_hdm_range *range)
> > +{
> > +     struct pci_dev *pdev = range->pdev;
> > +     const struct range *hpa_range = &range->hpa_range;
> > +     int rc;
> > +
> > +     rc = cpu_cache_invalidate_memregion(hpa_range->start, range->len);
> > +     if (rc)
> > +             pci_err(pdev,
> > +                     "failed to invalidate CPU cache [%#llx-%#llx]: %d\n",
> > +                     hpa_range->start, hpa_range->end, rc);
> What does the wrapper really bring?
>
> > +
> > +     return rc;
> > +}
> > +
> > +static int cxl_hdm_ranges_flush_cpu_caches(struct cxl_hdm_range_context *ctx,
> > +                                        struct pci_dev *pdev)
> > +{
> > +     struct cxl_hdm_range *range;
> > +     int rc;
> > +
> > +     if (list_empty(&ctx->ranges))
> > +             return 0;
> > +
> > +     if (!cpu_cache_has_invalidate_memregion()) {
> > +             pci_warn(pdev,
> > +                      "CPU cache synchronization unavailable; continuing without cache invalidation\n");
>
> boom.  Unless you have very strong reasonsing for me this is a no.
> This introduces extremely subtle cache coherency corruption into your
> system and no one wants to debug results of that.
>
> > +             return 0;
> > +     }
> > +
> > +     list_for_each_entry(range, &ctx->ranges, list) {
> > +             rc = cxl_hdm_range_flush_cache(range);
> > +             if (rc)
> > +                     return rc;
> > +     }
> > +
> > +     return 0;
> > +}

These go into the v13 patch 11 now. I have:
  - reset is rejected when active HDM ranges exist and CPU cache
    invalidation is unavailable;
  - the affected ranges are invalidated before reset and again after 
state restoration;
  - range reservations and IOMMU exclusion remain active through the
    second invalidation; and
  - added description  for invalidation sequence in the commit message.

The previous wrapper around one invalidation call is now a range-context 
helper used by both invalidation passes.

https://lore.kernel.org/linux-cxl/20260922083924.2451158-12-smadhavan@xxxxxxxxxx/

--
Regards,
Srirangan