Re: [PATCH RFC] xen/pci: detect when BARs overlap RAM

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 26, 2022 at 05:09:56PM +0100, Jan Beulich wrote:
> 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:
> >>> +        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)?

Handling overlaps with those wasn't strictly needed to solve the issue
at hand, but I didn't take into account that an overlap with a MMCFG
region would be equally bad. I've extended the check to cover any
E820 entry.

> >>> +        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.

Done.

Thanks, Roger.