Hi Ayan, On 13/04/2023 19:37, Ayan Kumar Halder wrote: > > > handle_pci_range() and map_range_to_domain() take addr and len as uint64_t > parameters. Then frame numbers are obtained from addr and len by right > shifting > with PAGE_SHIFT. The page frame numbers are saved using unsigned long. Maybe better to say "expressed" rather than "saved"
> > Now if 64-bit >> PAGE_SHIFT, the result will have 52-bits as valid. On a > 32-bit > system, 'unsigned long' is 32-bits. Thus, there is a potential loss of value > when the result is stored as 'unsigned long'. > > To mitigate this issue, we check if the starting and end address can be > contained within the range of physical address supported on the system. If > not, > then an appropriate error is returned. > > Also, the end address is computed once and used when required. And replaced > u64 > with uint64_t. > > Signed-off-by: Ayan Kumar Halder <[email protected]> > --- > > Changes from :- > v1...v4 - NA. New patch introduced in v5. > > xen/arch/arm/domain_build.c | 30 +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 7d28b75517..b98ee506a8 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1637,15 +1637,23 @@ out: > } > > static int __init handle_pci_range(const struct dt_device_node *dev, > - u64 addr, u64 len, void *data) > + uint64_t addr, uint64_t len, void *data) > { > struct rangeset *mem_holes = data; > paddr_t start, end; > int res; > + uint64_t end_addr = addr + len - 1; > + > + if ( addr != (paddr_t)addr || end_addr != (paddr_t)end_addr ) > + { > + printk(XENLOG_ERR "addr (0x%"PRIx64") or end_addr (0x%"PRIx64") > exceeds the maximum allowed width (%d bits) for physical address\n", I don't think it is wise to print variable names (end_addr) to the user. Better would be to say explicitly: start, end address. Also to make the message shorter you could write: ... exceeds the maximum allowed PA width (%u bits) > + addr, end_addr, CONFIG_PADDR_BITS); Why CONFIG_PADDR_BITS if you already introduced PADDR_BITS macro > + return -ERANGE; > + } > > start = addr & PAGE_MASK; > - end = PAGE_ALIGN(addr + len); > - res = rangeset_remove_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end - > 1)); > + end = PAGE_ALIGN(end_addr); > + res = rangeset_remove_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end)); I doubt PFN_DOWN(end) is the same as PFN_DOWN(end - 1), so I think you should keep the behavior as it was > if ( res ) > { > printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > @@ -2330,11 +2338,19 @@ static int __init map_dt_irq_to_domain(const struct > dt_device_node *dev, > } > > int __init map_range_to_domain(const struct dt_device_node *dev, > - u64 addr, u64 len, void *data) > + uint64_t addr, uint64_t len, void *data) You changed u64 to uint64_t in a definition but not in a prototype. Please fix. > { > struct map_range_data *mr_data = data; > struct domain *d = mr_data->d; > int res; > + uint64_t end_addr = addr + len - 1; > + > + if ( addr != (paddr_t)addr || end_addr != (paddr_t)end_addr ) > + { > + printk(XENLOG_ERR "addr (0x%"PRIx64") or end_addr (0x%"PRIx64") > exceeds the maximum allowed width (%d bits) for physical address\n", > + addr, end_addr, CONFIG_PADDR_BITS); please see the remarks above about this code > + return -ERANGE; > + } > > /* > * reserved-memory regions are RAM carved out for a special purpose. > @@ -2345,13 +2361,13 @@ int __init map_range_to_domain(const struct > dt_device_node *dev, > strlen("/reserved-memory/")) != 0 ) > { > res = iomem_permit_access(d, paddr_to_pfn(addr), > - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); > + paddr_to_pfn(PAGE_ALIGN(end_addr))); > if ( res ) > { > printk(XENLOG_ERR "Unable to permit to dom%d access to" > " 0x%"PRIx64" - 0x%"PRIx64"\n", > d->domain_id, > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > + addr & PAGE_MASK, PAGE_ALIGN(end_addr) - 1); > return res; > } > } > @@ -2368,7 +2384,7 @@ int __init map_range_to_domain(const struct > dt_device_node *dev, > { > printk(XENLOG_ERR "Unable to map 0x%"PRIx64 > " - 0x%"PRIx64" in domain %d\n", > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1, > + addr & PAGE_MASK, PAGE_ALIGN(end_addr) - 1, > d->domain_id); > return res; > } > -- > 2.17.1 > > ~Michal
