Re: [PATCH v4] powerpc/pseries/iommu: Add TCEs for 16GB pages when RAM is pre-mapped

Hello Ritesh,

Please check my responses inline.

- Gaurav

On 7/29/26 8:35 PM, Ritesh Harjani (IBM) wrote:
> Hi Gaurav,
>
> Gaurav Batra<[email protected]> writes:
>
>> In powerPC, if Dynamic DMA Window is big enough, RAM is pre-mapped. To
>> determine the size of RAM, a PAPR+ property "ibm,lrdr-capacity" is used.
>> This OF property dictates what is the max size of RAM an LPAR can have,
>> including DR added memory.
>>
>> In PowerPC, 16GB pages can be allocated at machine level and then
>> assigned to LPARs. These 16GB pages are added to LPAR memory at the time
>> of boot. The address range for these 16GB pages is above MAX RAM an LPAR
>> can have (ibm,lrdr-capacity). In the current implementation, these 16GB
> So these 16GB pages must be apperaing as "memory" nodes, because that's
> how the code must be detecting them above "ibm,lrdr-capacity"?
> Do you have the device node example for the same?
yes, these 16GB pages appear as memory node right after the max-memory 
possible
for the LPAR. Here are the properties from the device tree for a case 
where LPAR
min/desired/max is 256GB/512GB/2048GB

memory@20000000000:
ibm,expected#pages
                  00000000
ibm,phandle      80002000
device_type      "memory"
reg              00000200 00000000 00000004 00000000
phandle          00cdd850 (13490256)
name             "memory"
ibm,associativity
                  00000004 00000000 00000001 00000006 0000000c

memory@20400000000:
ibm,expected#pages
                  00000000
ibm,phandle      80002040
device_type      "memory"
reg              00000204 00000000 00000004 00000000
phandle          00cdda38 (13490744)
name             "memory"
ibm,associativity
                  00000004 00000000 00000001 00000007 0000000e


This LPAR has 2 16GB pages addigned to it and so, there are 2 16GB nodes
presented to the LPAR.
>
>
>> pages are being excluded from pre-mapped TCEs. A driver can have DMA
>> buffers allocated from 16GB pages. This results in platform to raise an
>> EEH when DMA is attempted on buffers in 16GB memory range.
>>
>> commit 6aa989ab2bd0 ("powerpc/pseries/iommu: memory notifier incorrectly
>> adds TCEs for pmemory")
>>
>> Prior to the above patch, memblock_end_of_DRAM() was being used to
>> determine the MAX memory of an LPAR. This included 16GB pages as well.
>> The issue with using memblock_end_of_DRAM() is that when pmemory is
>> converted to RAM via daxctl command, the DDW engine will incorrectly try
>> to add TCEs for pmemory as well.
>>
>> Below is the address distribution of RAM, 16GB pages and pmemory for an
>> LPAR with max memory of 256GB, memory allocated 64GB, 2 16GB pages and
>> assigned pmemory of 8GB.
>>
>> RANGE                                 SIZE  STATE REMOVABLE     BLOCK
>> 0x0000000000000000-0x0000000fffffffff  64G online       yes     0-255
>> 0x0000004000000000-0x00000047ffffffff  32G online       yes 1024-1151
>>
>> cat /sys/bus/nd/devices/region0/resource
>> 0x40100000000
>> cat /sys/bus/nd/devices/region0/size
>> 8589934592
> Is there any formal placement contract in the PAPR which tells where
> would PMEM appear in the logical real address range for LPAR?
I went through the PAPR for placement requirements for pmemory but, 
didn't see anything.
I was able to get access to a machine which has 8TB of memory. I checked 
the
placement of pmemory with various memory settings and 2 16GB of pages. 
Size of
pmemory assigned to the LPAR is 8GB.

mem     256/512/2048    (min/desired/max)

[root@ltcmcka1n-lp1 ~]# lsmem
RANGE                                  SIZE  STATE REMOVABLE  BLOCK
0x0000000000000000-0x0000007fffffffff  512G online       yes 0-2047
0x0000020000000000-0x00000207ffffffff   32G online       yes 8192-8319

[root@ltcmcka1n-lp1 ~]# cat /sys/bus/nd/devices/region0/resource
0x40010000000   <=====  pmemory placement above 4TB


mem     256/512/4096

[root@ltcmcka1n-lp1 ~]# lsmem
RANGE                                  SIZE  STATE REMOVABLE  BLOCK
0x0000000000000000-0x0000007fffffffff  512G online       yes 0-2047
0x0000040000000000-0x00000407ffffffff   32G online       yes 16384-16511

[root@ltcmcka1n-lp1 ~]# cat /sys/bus/nd/devices/region0/resource
0x80010000000   <===== 8TB


mem     256/512/6144

[root@ltcmcka1n-lp1 ~]# lsmem
RANGE                                  SIZE  STATE REMOVABLE  BLOCK
0x0000000000000000-0x0000007fffffffff  512G online       yes 0-2047
0x0000060000000000-0x00000607ffffffff   32G online       yes 24576-24703

[root@ltcmcka1n-lp1 ~]# cat /sys/bus/nd/devices/region0/resource
0x80010000000   <===== 8TB


In all these cases, pmemory was assigned to above power-of-2 which included
memory + 16GB pages.
>
>> The approach to fix this problem is to revert back the code changes
>> introduced by the above patch and to stash away the MAX memory of an
>> LPAR, including 16GB pages, at the LPAR boot time. This value is then
>> used whenever TCEs are needed to be pre-mapped - enable_DDW() or,
>> iommu_mem_notifier()
> So, can it appear for e.g. in above case from 288G to 512G? In your
> example we see pmem coming at 4TB, but what if the system DRAM was above
> 4TB? In that case, could vPMEM appear at non-power-of-2 address just
> above ibm,lrdr-capacity+16G?
>
> The reason for the ask is, I see a small window where we don't have any
> direct pre-mappings, but we do allow for core dma mapping apis to use the
> direct-mapping window.
>
> Here...
>
> In the current code we check for:
>
> @@ -2419,23 +2425,35 @@ static int iommu_mem_notifier(struct notifier_block *nb, unsigned long action,
>   {
> +	unsigned long max_ram_pages = pseries_ddw_max_ram >> PAGE_SHIFT;
>   	int ret = 0;
> <...>
> +	if (arg->start_pfn >= max_ram_pages)
> +		return NOTIFY_OK;
>
> & but in enable_ddw() {
>      <...>
> 	int max_ram_len = order_base_2(pseries_ddw_max_ram);
>      <...>
> 	/* For pre-mapped memory, set bus_dma_limit to the max RAM */
> 	if (direct_mapping)
> 		dev->dev.bus_dma_limit = dev->dev.archdata.dma_offset +
> 						(1ULL << max_ram_len);
>
> So, we don't pre-map anything above pseries_ddw_max_ram in the
> iommu_notifier() but we allow for dma_mapping to use direct map until
> order_base_2(pseries_ddw_max_ram).
> So it looks like that, there is a small gap left in direct_mapping
> window for e.g. in above case between 288G to 512G (because of
> order_base_2()), where there are no pre-mapped TCE entries but if the
> PMEM is added within that range, then we do allow to use the
> direct-mapping path (due to bus_dma_limit) - which means an EEH could
> occur.
>
> Could you please check this claim?

Now, as per PAPR, when an OS is making an RTAS call to create DDW, the size
of the DDW needs to be power-of-2. But, I didn't see any such 
requirement for
the placement of pmemory. Even though, in my experiment above, pmemory 
was always given
a region above power-of-2 of (RAM + 16GB), I think, we cannot assume 
that will be the
case all the time. It could be different for different configurations.

As you correctly pointed out in the enable_ddw() code, this could lead 
to EEH if
pmemory is assigned to the same power-of-2 chunk as (RAM + 16GB pages).

It would be better to set device bus_dma_limit to pseries_ddw_max_ram 
and *not*

max_ram_pages, which in this case is the size of the DDW.

I will fix this in my next version of the patch. Please let me know if you have any
other concerns. I will incorporate all those changes in the next version of the patch.

Thanks a lot for spending time and checking this code in deep.

>
> -ritesh
>
>
>> Fixes: 6aa989ab2bd0 ("powerpc/pseries/iommu: memory notifier incorrectly adds TCEs for pmemory")
>> Cc:[email protected]
>> Signed-off-by: Gaurav Batra<[email protected]>
>> ---
>>
>> Change log:
>>
>> V3 -> V4
>>
>> 1. Ritesh: Change pseries_ddw_max_ram to __ro_after_init;
>>
>>     Response: Incorporated changes
>>
>> 2. Ritesh: Mark function ddw_memory_hotplug_max() as __init function
>>
>>     Response: Incorporated changes
>>
>> 3. Ritesh: This patch was made in v6.15. Since we want this to be backported, I
>>     would suggested add a CC stable tag as well.
>>
>>     Response: Incorporated changes
>>
>> V2 -> V3
>>
>> 1. Harsh: Remove R-b tags from the change log
>>
>>     Response: Incorporated changes
>>
>> 2. Harsh: Change WARN_ON() to WARN_ONCE()
>>
>>     Response: Incorporated changes
>>
>> 3. Harsh: Fix indendation
>>
>>     Response: Incorporated changes
>>
>> 4. Harsh: Replace comment with a log if limit < arg->nr_pages ?
>>
>>     Response: Doesn't seems to be needed since the WARN_ONCE() will log this
>>     scenario. I removed the comment instead.
>>
>> V1 -> V2
>>
>> 1. Harsh: Not only start_pfn, but end_pfn also needs to be within allowed
>>     range, which may require clamping arg->nr_pages if crossing the limits.
>>
>>     Response: Incorporated changes.
>>
>>
>>   arch/powerpc/platforms/pseries/iommu.c | 60 ++++++++++++++++++--------
>>   1 file changed, 42 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>> index 3e1f915fe4f6..7f63adafaf91 100644
>> --- a/arch/powerpc/platforms/pseries/iommu.c
>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>> @@ -69,6 +69,8 @@ static struct iommu_table *iommu_pseries_alloc_table(int node)
>>   	return tbl;
>>   }
>>   
>> +static phys_addr_t pseries_ddw_max_ram __ro_after_init;
>> +
>>   #ifdef CONFIG_IOMMU_API
>>   static struct iommu_table_group_ops spapr_tce_table_group_ops;
>>   #endif
>> @@ -1283,15 +1285,19 @@ struct failed_ddw_pdn {
>>   
>>   static LIST_HEAD(failed_ddw_pdn_list);
>>   
>> -static phys_addr_t ddw_memory_hotplug_max(void)
>> +static phys_addr_t __init ddw_memory_hotplug_max(void)
>>   {
>> -	resource_size_t max_addr;
>> +	resource_size_t max_addr = memory_hotplug_max();
>> +	struct device_node *memory;
>>   
>> -#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
>> -	max_addr = hot_add_drconf_memory_max();
>> -#else
>> -	max_addr = memblock_end_of_DRAM();
>> -#endif
>> +	for_each_node_by_type(memory, "memory") {
>> +		struct resource res;
>> +
>> +		if (of_address_to_resource(memory, 0, &res))
>> +			continue;
>> +
>> +		max_addr = max_t(resource_size_t, max_addr, res.end + 1);
>> +	}
>>   
>>   	return max_addr;
>>   }
>> @@ -1446,7 +1452,7 @@ static struct property *ddw_property_create(const char *propname, u32 liobn, u64
>>   static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn, u64 dma_mask)
>>   {
>>   	int len = 0, ret;
>> -	int max_ram_len = order_base_2(ddw_memory_hotplug_max());
>> +	int max_ram_len = order_base_2(pseries_ddw_max_ram);
>>   	struct ddw_query_response query;
>>   	struct ddw_create_response create;
>>   	int page_shift;
>> @@ -1668,7 +1674,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn, u64 dma_mas
>>   
>>   	if (direct_mapping) {
>>   		/* DDW maps the whole partition, so enable direct DMA mapping */
>> -		ret = walk_system_ram_range(0, ddw_memory_hotplug_max() >> PAGE_SHIFT,
>> +		ret = walk_system_ram_range(0, pseries_ddw_max_ram >> PAGE_SHIFT,
>>   					    win64->value, tce_setrange_multi_pSeriesLP_walk);
>>   		if (ret) {
>>   			dev_info(&dev->dev, "failed to map DMA window for %pOF: %d\n",
>> @@ -2419,23 +2425,35 @@ static int iommu_mem_notifier(struct notifier_block *nb, unsigned long action,
>>   {
>>   	struct dma_win *window;
>>   	struct memory_notify *arg = data;
>> +	unsigned long limit = arg->nr_pages;
>> +	unsigned long max_ram_pages = pseries_ddw_max_ram >> PAGE_SHIFT;
>>   	int ret = 0;
>>   
>>   	/* This notifier can get called when onlining persistent memory as well.
>>   	 * TCEs are not pre-mapped for persistent memory. Persistent memory will
>> -	 * always be above ddw_memory_hotplug_max()
>> +	 * always be above pseries_ddw_max_ram
>>   	 */
>> +	if (arg->start_pfn >= max_ram_pages)
>> +		return NOTIFY_OK;
>> +
>> +	/* RAM is being DLPAR'ed. The range should never exceed max ram.
>> +	 * Just in case, clamp the range and throw a warning.
>> +	 */
>> +	if (arg->start_pfn + limit > max_ram_pages) {
>> +		limit = max_ram_pages - arg->start_pfn;
>> +		WARN_ONCE(1, "Limiting Page Range %lx - %lx to Max Mem Pages: %lx\n",
>> +					arg->start_pfn, arg->start_pfn + arg->nr_pages,
>> +					max_ram_pages);
>> +	}
>>   
>>   	switch (action) {
>>   	case MEM_GOING_ONLINE:
>>   		spin_lock(&dma_win_list_lock);
>>   		list_for_each_entry(window, &dma_win_list, list) {
>> -			if (window->direct && (arg->start_pfn << PAGE_SHIFT) <
>> -				ddw_memory_hotplug_max()) {
>> +			if (window->direct) {
>>   				ret |= tce_setrange_multi_pSeriesLP(arg->start_pfn,
>> -						arg->nr_pages, window->prop);
>> +						limit, window->prop);
>>   			}
>> -			/* XXX log error */
>>   		}
>>   		spin_unlock(&dma_win_list_lock);
>>   		break;
>> @@ -2443,12 +2461,10 @@ static int iommu_mem_notifier(struct notifier_block *nb, unsigned long action,
>>   	case MEM_OFFLINE:
>>   		spin_lock(&dma_win_list_lock);
>>   		list_for_each_entry(window, &dma_win_list, list) {
>> -			if (window->direct && (arg->start_pfn << PAGE_SHIFT) <
>> -				ddw_memory_hotplug_max()) {
>> +			if (window->direct) {
>>   				ret |= tce_clearrange_multi_pSeriesLP(arg->start_pfn,
>> -						arg->nr_pages, window->prop);
>> +						limit, window->prop);
>>   			}
>> -			/* XXX log error */
>>   		}
>>   		spin_unlock(&dma_win_list_lock);
>>   		break;
>> @@ -2532,6 +2548,14 @@ void __init iommu_init_early_pSeries(void)
>>   	register_memory_notifier(&iommu_mem_nb);
>>   
>>   	set_pci_dma_ops(&dma_iommu_ops);
>> +
>> +	/* During init determine the max memory an LPAR can have and set it. This
>> +	 * will be used for pre-mapping RAM in DDW. memblock_end_of_DRAM() can
>> +	 * change during the running of LPAR - daxctl can add pmemory as
>> +	 * "system-ram". This memory range should not be pre-mapped in DDW since
>> +	 * the address of pmemory can be much higher than the DDW size.
>> +	 */
>> +	pseries_ddw_max_ram = ddw_memory_hotplug_max();
>>   }
>>   
>>   static int __init disable_multitce(char *str)
>>
>> base-commit: 6d35786de28116ecf78797a62b84e6bf3c45aa5a
>> -- 
>> 2.39.3