[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH RFC] xen/pci: detect when BARs overlap RAM
On Wed, Jan 26, 2022 at 03:08:28PM +0100, Jan Beulich wrote: > On 26.01.2022 13:26, Roger Pau Monne wrote: > > RFC because: > > - Not sure the best way to implement is_iomem_range on Arm. BARs can > > be quite big, so iterating over every possible page is not ideal. > > - is_iomem_page cannot be used for this purpose on x86, because all > > the low 1MB will return true due to belonging to dom_io. > > I don't see an issue there - if you were to us it, you'd end up with > the same scalability issue as you point out for Arm. > > > - VF BARs are not checked. Should we also check them and disable VF > > if overlaps in a followup patch? > > Not sure about "followup patch", but once we support SR-IOV for PVH, > that'll want checking, yes. > > > --- 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. > > +{ > > + 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. > > @@ -267,11 +270,74 @@ static void check_pdev(const struct pci_dev *pdev) > > } > > break; > > > > + case PCI_HEADER_TYPE_NORMAL: > > + nbars = PCI_HEADER_NORMAL_NR_BARS; > > + rom_pos = PCI_ROM_ADDRESS; > > + break; > > + > > case PCI_HEADER_TYPE_CARDBUS: > > /* TODO */ > > break; > > } > > #undef PCI_STATUS_CHECK > > + > > + /* Check if BARs overlap with RAM regions. */ > > + val = pci_conf_read16(pdev->sbdf, PCI_COMMAND); > > + if ( !(val & PCI_COMMAND_MEMORY) || pdev->ignore_bars ) > > + return; > > + > > + pci_conf_write16(pdev->sbdf, PCI_COMMAND, val & ~PCI_COMMAND_MEMORY); > > + for ( i = 0; i < nbars; ) > > + { > > + uint64_t addr, size; > > + unsigned int reg = PCI_BASE_ADDRESS_0 + i * 4; > > + int rc = 1; > > + > > + if ( (pci_conf_read32(pdev->sbdf, reg) & PCI_BASE_ADDRESS_SPACE) != > > + PCI_BASE_ADDRESS_SPACE_MEMORY ) > > + goto next; > > + > > + rc = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size, > > + (i == nbars - 1) ? PCI_BAR_LAST : 0); > > + if ( rc < 0 ) > > + return; > > This isn't very nice, but probably the best you can do. Might be worth > a comment, though. > > > + 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" > > @@ -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. Thanks, Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |