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

[Srirangan Madhavan <smadhavan@nvidia.com>]

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@nvidia.com> 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@nvidia.com>
> 
> 
> 
>> +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@nvidia.com/

-- 
Regards,
Srirangan