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

[Mayank Rana <mayank.rana@oss.qualcomm.com>]

Hi Dave

On 7/23/2026 4:47 PM, Dave Jiang wrote:
> 
> 
> On 7/23/26 4:18 PM, Mayank Rana wrote:
>> 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 <mayank.rana@oss.qualcomm.com>
>>>>> ---
>>>>>     - 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;
>>
> 
> ok sounds good
> 
>> 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.
> 
> Davidlohr seems to indicate that with his code series, this patch may not be needed?
I review proposed Davidlohr fixes, and seeing that cxl_region_attach() 
will prevent this situation. Agree, this patch next version is not needed.

Thank you.

Regards,
Mayank>
> DJ
> 
>>
>> 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
>>>
>>
>