| Message ID | 20230213124438.14836-3-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Pre-requisite fixes for supporting 32 bit physical address | expand |
Hi Ayan, The title is a bit strange to read as a 'pfn' is not an address. So how about: xen/arm: domain_build: Track unallocated pages using the frame number On 13/02/2023 12:44, Ayan Kumar Halder wrote: > rangeset_{xxx}_range() functions are invoked with 'start' and 'size' as > arguments which are either 'uint64_t' or 'paddr_t'. However, the function > accepts 'unsigned long' for 'start' and 'size'. 'unsigned long' is 32 bits for > ARM_32. Thus, there is an implicit downcasting from 'uint64_t'/'paddr_t' to > 'unsigned long' when invoking rangeset_{xxx}_range(). > > However, it may seem there is a possibility of lose of data due to truncation. > > In reality, 'start' and 'size' are always page aligned. And ARM_32 currently > supports 40 bits as the width of physical address. > So if the addresses are page aligned, the last 12 bits contain zeroes. > Thus, we could instead pass page frame number which will contain 28 bits (40-12 > on Arm_32) and this can be represented using 'unsigned long'. > > On Arm_64, this change will not induce any adverse side effect as the width of > physical address is 48 bits. Thus, the width of 'pfn' (ie 48 - 12 = 36) can be Technically, this will an MFN rather than PFN (Yes, I now the macro is called PFN_DOWN() but this is a generic name). > represented using 'unsigned long' (which is 64 bits wide). > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > > Changes from - > > v1 - v5 - NA (New patch introduced in v6). > > xen/arch/arm/domain_build.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index a798e0b256..6a8c7206ae 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1566,7 +1566,8 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > { > start = bootinfo.mem.bank[i].start; > end = bootinfo.mem.bank[i].start + bootinfo.mem.bank[i].size; > - res = rangeset_add_range(unalloc_mem, start, end - 1); > + res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), > + PFN_DOWN(end - 1)); > if ( res ) > { > printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", > @@ -1580,7 +1581,8 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > { > start = assign_mem->bank[i].start; > end = assign_mem->bank[i].start + assign_mem->bank[i].size; > - res = rangeset_remove_range(unalloc_mem, start, end - 1); > + res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), > + PFN_DOWN(end - 1)); > if ( res ) > { > printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > @@ -1595,7 +1597,8 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > start = bootinfo.reserved_mem.bank[i].start; > end = bootinfo.reserved_mem.bank[i].start + > bootinfo.reserved_mem.bank[i].size; > - res = rangeset_remove_range(unalloc_mem, start, end - 1); > + res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), > + PFN_DOWN(end - 1)); > if ( res ) > { > printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > @@ -1607,7 +1610,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > /* Remove grant table region */ > start = kinfo->gnttab_start; > end = kinfo->gnttab_start + kinfo->gnttab_size; > - res = rangeset_remove_range(unalloc_mem, start, end - 1); > + res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end - 1)); > if ( res ) > { > printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > @@ -1617,7 +1620,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > > start = 0; > end = (1ULL << p2m_ipa_bits) - 1; > - res = rangeset_report_ranges(unalloc_mem, start, end, > + res = rangeset_report_ranges(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end), > add_ext_regions, ext_regions); I believe you also need to modify add_ext_regions() because the existing code expect an address and we will now provide a frame number. > if ( res ) > ext_regions->nr_banks = 0; > @@ -1639,7 +1642,7 @@ static int __init handle_pci_range(const struct dt_device_node *dev, > > start = addr & PAGE_MASK; > end = PAGE_ALIGN(addr + len); > - res = rangeset_remove_range(mem_holes, start, end - 1); > + res = rangeset_remove_range(mem_holes, PFN_DOWN(start),PFN_DOWN(end - 1)); Coding style: missing space after "," > if ( res ) > { > printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > @@ -1677,7 +1680,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, > /* Start with maximum possible addressable physical memory range */ > start = 0; > end = (1ULL << p2m_ipa_bits) - 1; > - res = rangeset_add_range(mem_holes, start, end); > + res = rangeset_add_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end)); > if ( res ) > { > printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", > @@ -1708,7 +1711,8 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, > > start = addr & PAGE_MASK; > end = PAGE_ALIGN(addr + size); > - res = rangeset_remove_range(mem_holes, start, end - 1); > + res = rangeset_remove_range(mem_holes, PFN_DOWN(start), > + PFN_DOWN(end - 1)); > if ( res ) > { > printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > @@ -1735,7 +1739,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, > > start = 0; > end = (1ULL << p2m_ipa_bits) - 1; > - res = rangeset_report_ranges(mem_holes, start, end, > + res = rangeset_report_ranges(mem_holes, PFN_DOWN(start), PFN_DOWN(end), > add_ext_regions, ext_regions); > if ( res ) > ext_regions->nr_banks = 0; Cheers,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index a798e0b256..6a8c7206ae 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1566,7 +1566,8 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, { start = bootinfo.mem.bank[i].start; end = bootinfo.mem.bank[i].start + bootinfo.mem.bank[i].size; - res = rangeset_add_range(unalloc_mem, start, end - 1); + res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), + PFN_DOWN(end - 1)); if ( res ) { printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", @@ -1580,7 +1581,8 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, { start = assign_mem->bank[i].start; end = assign_mem->bank[i].start + assign_mem->bank[i].size; - res = rangeset_remove_range(unalloc_mem, start, end - 1); + res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), + PFN_DOWN(end - 1)); if ( res ) { printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", @@ -1595,7 +1597,8 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, start = bootinfo.reserved_mem.bank[i].start; end = bootinfo.reserved_mem.bank[i].start + bootinfo.reserved_mem.bank[i].size; - res = rangeset_remove_range(unalloc_mem, start, end - 1); + res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), + PFN_DOWN(end - 1)); if ( res ) { printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", @@ -1607,7 +1610,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, /* Remove grant table region */ start = kinfo->gnttab_start; end = kinfo->gnttab_start + kinfo->gnttab_size; - res = rangeset_remove_range(unalloc_mem, start, end - 1); + res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end - 1)); if ( res ) { printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", @@ -1617,7 +1620,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, start = 0; end = (1ULL << p2m_ipa_bits) - 1; - res = rangeset_report_ranges(unalloc_mem, start, end, + res = rangeset_report_ranges(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end), add_ext_regions, ext_regions); if ( res ) ext_regions->nr_banks = 0; @@ -1639,7 +1642,7 @@ static int __init handle_pci_range(const struct dt_device_node *dev, start = addr & PAGE_MASK; end = PAGE_ALIGN(addr + len); - res = rangeset_remove_range(mem_holes, start, end - 1); + res = rangeset_remove_range(mem_holes, PFN_DOWN(start),PFN_DOWN(end - 1)); if ( res ) { printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", @@ -1677,7 +1680,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, /* Start with maximum possible addressable physical memory range */ start = 0; end = (1ULL << p2m_ipa_bits) - 1; - res = rangeset_add_range(mem_holes, start, end); + res = rangeset_add_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end)); if ( res ) { printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", @@ -1708,7 +1711,8 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, start = addr & PAGE_MASK; end = PAGE_ALIGN(addr + size); - res = rangeset_remove_range(mem_holes, start, end - 1); + res = rangeset_remove_range(mem_holes, PFN_DOWN(start), + PFN_DOWN(end - 1)); if ( res ) { printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", @@ -1735,7 +1739,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, start = 0; end = (1ULL << p2m_ipa_bits) - 1; - res = rangeset_report_ranges(mem_holes, start, end, + res = rangeset_report_ranges(mem_holes, PFN_DOWN(start), PFN_DOWN(end), add_ext_regions, ext_regions); if ( res ) ext_regions->nr_banks = 0;
rangeset_{xxx}_range() functions are invoked with 'start' and 'size' as arguments which are either 'uint64_t' or 'paddr_t'. However, the function accepts 'unsigned long' for 'start' and 'size'. 'unsigned long' is 32 bits for ARM_32. Thus, there is an implicit downcasting from 'uint64_t'/'paddr_t' to 'unsigned long' when invoking rangeset_{xxx}_range(). However, it may seem there is a possibility of lose of data due to truncation. In reality, 'start' and 'size' are always page aligned. And ARM_32 currently supports 40 bits as the width of physical address. So if the addresses are page aligned, the last 12 bits contain zeroes. Thus, we could instead pass page frame number which will contain 28 bits (40-12 on Arm_32) and this can be represented using 'unsigned long'. On Arm_64, this change will not induce any adverse side effect as the width of physical address is 48 bits. Thus, the width of 'pfn' (ie 48 - 12 = 36) can be represented using 'unsigned long' (which is 64 bits wide). Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from - v1 - v5 - NA (New patch introduced in v6). xen/arch/arm/domain_build.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)