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.
