[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH RFC] xen/pci: detect when BARs overlap RAM
On 26.01.2022 16:45, Roger Pau Monné wrote: > On Wed, Jan 26, 2022 at 03:08:28PM +0100, Jan Beulich wrote: >> On 26.01.2022 13:26, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/mm.c >>> +++ b/xen/arch/x86/mm.c >>> @@ -479,6 +479,26 @@ unsigned int page_get_ram_type(mfn_t mfn) >>> return type ?: RAM_TYPE_UNKNOWN; >>> } >>> >>> +bool is_iomem_range(uint64_t start, uint64_t size) >> >> Might be nice to have this sit next it is_iomem_page(). And wouldn't >> at least "start" better be paddr_t? > > (paddr_t, size_t) would be better, but AFAICT size_t can be an > unsigned long and on Arm32 that won't be suitable for holding a 64bit > BAR size. Indeed. We'd need a resource_size_t or alike. >>> +{ >>> + unsigned int i; >>> + >>> + for ( i = 0; i < e820.nr_map; i++ ) >>> + { >>> + struct e820entry *entry = &e820.map[i]; >> >> const? >> >>> + if ( entry->type != E820_RAM && entry->type != E820_ACPI && >>> + entry->type != E820_NVS ) >>> + continue; >> >> I think UNUSABLE also needs checking for here. Even further, it might >> be best to reject any range overlapping an E820 entry, since colliding >> with a RESERVED one could mean an overlap with e.g. MMCFG space. > > Hm, I've though about adding UNUSABLE and RESERVED (and should have > added a note about this), but that might be too restrictive. Why (other than because of firmware bugs)? >>> + if ( size && !is_iomem_range(addr, size) ) >>> + { >>> + /* >>> + * Return without enabling memory decoding if BAR overlaps with >>> + * RAM region. >>> + */ >>> + printk(XENLOG_WARNING >>> + "%pp: disabled: BAR%u [%" PRIx64 ", %" PRIx64 >>> + ") overlaps with RAM\n", >> >> Mentioning "RAM" in comment and log message is potentially misleading >> when considering what other types get also checked (irrespective of my >> earlier comment). (Ftaod I don't mind the title using "RAM".) > > Would the message below be better? > > "%pp disabled: BAR%u [%" PRIx64 ", %" PRIx64 ") overlap detected\n" This or maybe "%pp disabled: BAR%u [%" PRIx64 ", %" PRIx64 ") overlaps with memory map\n" in particular if, on x86, we'd be checking for any E820 entry type. >>> @@ -399,8 +465,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, >>> u8 bus, u8 devfn) >>> break; >>> } >>> >>> - check_pdev(pdev); >>> apply_quirks(pdev); >>> + check_pdev(pdev); >> >> I can't find the description say anything about this re-ordering. What's >> the deal here? > > Should have mentioned: this is required so that ignore_bars is set > prior to calling check_pdev. Ah, I see. Makes sense, but indeed wants mentioning. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |