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.
