Re: [PATCH v2 3/3] arm64: acpi: Enable ACPI CCEL support

[Suzuki K Poulose <suzuki.poulose@arm.com>]

Hi Will

On 18/09/2025 13:31, Will Deacon wrote:
> On Mon, Sep 08, 2025 at 11:35:19PM +0100, Suzuki K Poulose wrote:
>> Add support for ACPI CCEL by handling the EfiACPIMemoryNVS type memory.
>> As per UEFI specifications NVS memory is reserved for Firmware use even
>> after exiting boot services. Thus map the region as read-only.
>>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@kernel.org>
>> Cc: Steven Price <steven.price@arm.com>
>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>> Cc: Gavin Shan <gshan@redhat.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>> Changes since v1
>>   - Map NVS region as read-only, update comment to clarify that the region
>>     is reserved for firmware use.
>>
>> ---
>>   arch/arm64/kernel/acpi.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>> index 4d529ff7ba51..93b70f48a51f 100644
>> --- a/arch/arm64/kernel/acpi.c
>> +++ b/arch/arm64/kernel/acpi.c
>> @@ -360,6 +360,17 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
>>   			prot = PAGE_KERNEL_RO;
>>   			break;
>>   
>> +		case EFI_ACPI_MEMORY_NVS:
>> +			/*
>> +			 * ACPI NVS marks an area reserved for use by the
>> +			 * firmware, even after exiting the boot service.
>> +			 * This may be used by the firmware for sharing dynamic
>> +			 * tables/data (e.g., ACPI CCEL) with the OS. Map it
>> +			 * as read-only.
>> +			 */
>> +			prot = PAGE_KERNEL_RO;
>> +			break;
>> +
> 
> Shouldn't this be merged with the other case handling read-only mappings?
> e.g. something like:
> 

I thought about it, but went against it, to keep the code separate. But
surely this is fine. I will resend the series with the proposed change.

Suzuki

> 		switch (region->type) {
> 		case EFI_LOADER_CODE:
> 		case EFI_LOADER_DATA:
> 		case EFI_BOOT_SERVICES_CODE:
> 		case EFI_BOOT_SERVICES_DATA:
> 		case EFI_CONVENTIONAL_MEMORY:
> 		case EFI_PERSISTENT_MEMORY:
> 			if (memblock_is_map_memory(phys) ||
> 			    !memblock_is_region_memory(phys, size)) {
> 				pr_warn(FW_BUG "requested region covers kernel memory @ %pa\n", &phys);
> 				return NULL;
> 			}
> 			/*
> 			 * Mapping kernel memory is permitted if the region in
> 			 * question is covered by a single memblock with the
> 			 * NOMAP attribute set: this enables the use of ACPI
> 			 * table overrides passed via initramfs, which are
> 			 * reserved in memory using arch_reserve_mem_area()
> 			 * below. As this particular use case only requires
> 			 * read access, fall through to the R/O mapping case.
> 			 */
> 			fallthrough;
> 
> 		case EFI_RUNTIME_SERVICES_CODE:
> 			/*
> 			 * This would be unusual, but not problematic per se,
> 			 * as long as we take care not to create a writable
> 			 * mapping for executable code.
> 			 */
> 			fallthrough;
> 
> 		case EFI_ACPI_MEMORY_NVS:
> 			/*
> 			 * ACPI NVS marks an area reserved for use by the
> 			 * firmware, even after exiting the boot service.
> 			 * This may be used by the firmware for sharing dynamic
> 			 * tables/data (e.g., ACPI CCEL) with the OS. Map it
> 			 * as read-only.
> 			 */
> 			prot = PAGE_KERNEL_RO;
> 			break;
> 
> 
> With that, I'm happy to pick up the series (let me know if you want me
> to make the change above locally to save you a resend).
> 
> Will