Re: [PATCH v2] cxl/hdm: Enforce CFMWS memory type policy at decoder commit time

Hi Dave

On 7/15/2026 3:26 PM, Mayank Rana wrote:
> Hi Dave
> 
> Thank you for review comments.
> 
> On 7/13/2026 8:47 AM, Dave Jiang wrote:
>>
>>
>> On 7/10/26 5:33 PM, Mayank Rana wrote:
>>> A CXL Fixed Memory Window Structure (CFMWS) in the ACPI CEDT table
>>> carries a restrictions field that describes the memory types a platform
>>> window supports.  cfmws_to_decoder_flags() translates this into
>>> CXL_DECODER_F_TYPE2 (HDM-DB permitted) and CXL_DECODER_F_TYPE3 (HDM-H
>>> permitted) flags on the root decoder.
>>>
>>> However, nothing currently prevents a caller from committing an endpoint
>>> or switch decoder whose target_type conflicts with the CFMWS policy:
>>>
>>>    - A decoder with target_type == CXL_DECODER_DEVMEM (HDM-DB) can be
>>>      committed on a window that lacks CXL_DECODER_F_TYPE2 (HDM-H only).
>>>      A dual-capable device will accept HOSTONLY=0 + COMMIT=1 without
>>>      asserting COMMIT_ERROR, silently violating platform policy.
>>>
>>>    - Symmetrically, a decoder with target_type == 
>>> CXL_DECODER_HOSTONLYMEM
>>>      (HDM-H) can be committed on a window that lacks CXL_DECODER_F_TYPE3
>>>      (HDM-DB only), again violating ACPI CFMWS restrictions.
>>>
>>> Add a check in cxl_decoder_commit() that walks from the decoder's
>>> assigned region to the root decoder and rejects the commit with
>>> -EOPNOTSUPP if the decoder's target_type requires a capability flag that
>>> the CFMWS window does not advertise:
>>>
>>>    - CXL_DECODER_DEVMEM      requires CXL_DECODER_F_TYPE2 on root 
>>> decoder
>>>    - CXL_DECODER_HOSTONLYMEM requires CXL_DECODER_F_TYPE3 on the root
>>>      decoder
>>>
>>> required_flag is initialized to zero so that any future target_type
>>> values not covered by the if/else-if chain leave the enforcement check
>>> a no-op via the leading required_flag && guard.
>>>
>>> This makes the CFMWS restrictions field authoritative for memory type
>>> enforcement in both directions, regardless of individual device 
>>> capability.
>>> The existing COMMIT_ERROR path in cxld_await_commit() remains as a
>>> secondary safeguard for devices that cannot support the requested mode.
>>
>> Can you please trim the commit log? The AI generated verbiage is 
>> excessively wordy. A simple and more to the point short log that 
>> conveys all the information would be appreciated.
> ok. noted down.
>>>
>>> Fixes: 3e23d17ce198 ("cxl/acpi: Use the ACPI CFMWS to create static 
>>> decoder objects")
>>> Assisted-by: Claude:claude-sonnet-4-6
>>> Signed-off-by: Mayank Rana <[email protected]>
>>> ---
>>>    - Initialize required_flag = 0 and use explicit else-if for
>>>      CXL_DECODER_HOSTONLYMEM; guard enforcement with required_flag &&
>>>      so unknown target_type values skip the check cleanly
>>>
>>> Changes in v2:
>>>    - Extend enforcement to cover both directions: HDM-H commits on
>>>      HDM-DB-only windows are now rejected in addition to HDM-DB commits
>>>      on HDM-H-only windows (reported by Sashiko AI review)
>>>    - Generalize condition from single DEVMEM check to type-dispatch
>>>      selecting required_flag based on target_type
>>>    - Update commit message and subject to reflect bidirectional policy
>>>
>>> Testing
>>> -------
>>>
>>> The bug and fix were validated using QEMU with a modified CXL 
>>> configuration.
>>>
>>> Two test-only changes were applied (not part of this patch):
>>>
>>>    1. QEMU ACPI (hw/acpi/cxl.c): CFMWS restrictions field changed from
>>>       0x0f to 0x02 (ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYMEM only, without
>>>       RESTRICT_DEVMEM).  This causes cfmws_to_decoder_flags() to set
>>>       CXL_DECODER_F_TYPE3 only on the root decoder, simulating a 
>>> platform
>>>       that does not permit HDM-DB.
>>>
>>>    2. Kernel (drivers/cxl/core/hdm.c): init_hdm_decoder() uncommitted
>>>       endpoint path changed to force CXL_DECODER_DEVMEM unconditionally,
>>>       simulating a dual-capable device (supports both HDM-H and HDM-DB).
>>>       QEMU's cxl-type3 device reports CXL_DEVTYPE_CLASSMEM and defaults
>>>       to HOSTONLYMEM; this override exercises the DEVMEM commit path 
>>> that
>>>       a real Type-2 or dual-capable Type-3 device would trigger.
>>>
>>> Bug reproduction (without this patch):
>>>
>>>    # cxl create-region -m mem0 -d decoder0.0 -t pmem
>>>    created 1 region
>>>
>>>    HDM-DB committed silently despite CFMWS advertising HDM-H only.
>>>    No error or warning in dmesg.
>>>
>>> Fix validation (with this patch):
>>>
>>>    # cxl create-region -m mem0 -d decoder0.0 -t pmem
>>>    cxl region: cmd_create_region: created 0 regions
>>>
>>>    dmesg:
>>>      cxl_core: cxl region0: mem0:decoder2.0 type mismatch: 2 vs 3
>>>      cxl_port endpoint2: failed to attach decoder2.0 to region0: -6
>>>
>>>    The decoder's target_type (2=DEVMEM) mismatches the region's required
>>>    type (3=HOSTONLYMEM) enforced by the CFMWS restriction -- commit 
>>> blocked.
>>>
>>> QEMU invocation:
>>>
>>>    qemu-system-x86_64 \
>>>      -kernel bzImage \
>>>      -append "root=/dev/sda rw console=ttyS0" \
>>>      -drive file=rootfs.img,index=0,media=disk,format=raw \
>>>      -M q35,cxl=on -m 4G,maxmem=8G,slots=8 -smp 4 \
>>>      -object memory-backend-file,id=cxl-mem1,share=on,mem-path=/tmp/ 
>>> cxltest.raw,size=256M \
>>>      -object memory-backend-file,id=cxl-lsa1,share=on,mem-path=/tmp/ 
>>> lsa.raw,size=256M \
>>>      -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
>>>      -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
>>>      -device cxl-type3,bus=root_port13,persistent-memdev=cxl- 
>>> mem1,lsa=cxl-lsa1,id=cxl-pmem0,sn=0x1 \
>>>      -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G \
>>>      -nographic
>>>
>>>
>>> drivers/cxl/core/hdm.c | 29 +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>> index 0c80b76a5f9b..127a187cdadb 100644
>>> --- a/drivers/cxl/core/hdm.c
>>> +++ b/drivers/cxl/core/hdm.c
>>> @@ -802,6 +802,7 @@ static int cxl_decoder_commit(struct cxl_decoder 
>>> *cxld)
>>>       struct cxl_port *port = to_cxl_port(cxld->dev.parent);
>>>       struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
>>>       void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>>> +    struct cxl_root_decoder *cxlrd;
>>>       int id = cxld->id, rc;
>>>       if (cxld->flags & CXL_DECODER_F_ENABLE)
>>> @@ -834,6 +835,34 @@ static int cxl_decoder_commit(struct cxl_decoder 
>>> *cxld)
>>>           }
>>>       }
>>> +    /*
>>> +     * Enforce CFMWS memory type policy: reject commits where the 
>>> decoder
>>> +     * target_type conflicts with the root decoder's CFMWS 
>>> restrictions.
>>> +     * - HDM-DB (DEVMEM) requires CXL_DECODER_F_TYPE2 on the root 
>>> decoder.
>>> +     * - HDM-H (HOSTONLYMEM) requires CXL_DECODER_F_TYPE3 on the 
>>> root decoder.
>>> +     * - Unknown target_type values leave required_flag zero; skip 
>>> enforcement.
>>> +     */
>>> +    if (cxld->region) {
>>
>> I don't think we should gate the check based on whether cxld->region 
>> is valid or not. Can you please take a look at core/ 
>> region.c:271:cxl_region_decode_reset() and do something similar to 
>> acquire the root decoder by walking up the port hierachy? That should 
>> apply the policy check unconditionally. I would also suggest putting 
>> this entire block in a helper function.
> will refer suggested API, and rework upon this.
Thanks for the pointer -- I looked closely at cxl_region_decode_reset()
and want to flag something before sending v3, since a literal port-walk
doesn't fully replace the region lookup here.

cxl_region_decode_reset() doesn't derive the decoder from the port walk
alone: it's called with cxlr already known, and at each port level uses 
cxl_rr_load(iter, cxlr) -- keyed by the region -- to find the decoder 
assigned to that region at that port. The walk finds the *port*; cxlr 
still finds the *decoder*.

The reason that matters for this check specifically: a single CXL root
port can host multiple root decoders, one per CFMWS window, each with
its own restrictions. A pure walk up to is_cxl_root(port) would tell us 
this decoder's SPA range originates somewhere under that root port, but 
not *which* of its root decoders' CFMWS restrictions apply -- only the 
region (or the port's region_ref tracking, which is  also region-keyed) 
can disambiguate that.

So v3 keeps the region-based lookup (cxld->region->dev.parent ->
to_cxl_root_decoder()), but addresses what I take to be the actual 
concern -- silently skipping the policy check -- by making the check 
itself unconditional instead of gated:

if (dev_WARN_ONCE(&cxld->dev, !cxld->region,
                        "commit without region assignment\n"))
             return -ENXIO;

A decoder reaching commit() without a region assigned is now treated as
a bug (loud WARN + rejected commit) rather than something the check 
quietly steps around. By the time cxl_decoder_commit() runs, the region 
assignment has already happened during region assembly 
(cxl_rr_assign_decoder()), so this should not be reachable in practice. 
Also pulled the whole block into cxl_decoder_cfmws_check() as suggested.

v3 to follow shortly with this and the F_DEVMEM/F_HOSTONLY rename per
Davidlohr's BI series.

Thanks,
Mayank
>> DJ
>>
>>> +        unsigned long required_flag = 0;
>>> +        const char *type_name;
>>> +
>>> +        cxlrd = to_cxl_root_decoder(cxld->region->dev.parent);
>>> +        if (cxld->target_type == CXL_DECODER_DEVMEM) {
>>> +            required_flag = CXL_DECODER_F_TYPE2;
>>> +            type_name = "HDM-DB";
>>> +        } else if (cxld->target_type == CXL_DECODER_HOSTONLYMEM) {
>>> +            required_flag = CXL_DECODER_F_TYPE3;
>>> +            type_name = "HDM-H";
>>> +        }
>>> +
>>> +        if (required_flag && !(cxlrd->cxlsd.cxld.flags & 
>>> required_flag)) {
>>> +            dev_err(&port->dev,
>>> +                "%s commit rejected on %s, CFMWS does not permit 
>>> this memory type\n",
>>> +                type_name, dev_name(&cxld->dev));
>>> +            return -EOPNOTSUPP;
>>> +        }
>>> +    }
>>> +
>>>       scoped_guard(rwsem_read, &cxl_rwsem.dpa)
>>>           setup_hw_decoder(cxld, hdm);
>>
> Regards,
> Mayank
>