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

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

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.
> 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