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
