Re: [PATCH v2] xen/pci: detect when BARs are not suitably positioned

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.01.2022 10:47, Roger Pau Monné wrote:
> On Thu, Jan 27, 2022 at 09:48:16AM +0100, Jan Beulich wrote:
>> On 27.01.2022 09:22, Roger Pau Monne wrote:
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -1625,6 +1625,17 @@ bool is_iomem_page(mfn_t mfn)
>>>      return !mfn_valid(mfn);
>>>  }
>>>  
>>> +bool is_iomem_range(paddr_t start, uint64_t size)
>>> +{
>>> +    unsigned int i;
>>> +
>>> +    for ( i = 0; i < PFN_UP(size); i++ )
>>> +        if ( !is_iomem_page(_mfn(PFN_DOWN(start) + i)) )
>>> +            return false;
>>> +
>>> +    return true;
>>> +}
>>
>> I'm afraid you can't very well call this non-RFC as long as this doesn't
>> scale. Or if you're building on this still being dead code on Arm, then
>> some kind of "fixme" annotation would be needed here (or the function be
>> omitted altogether, for Arm folks to sort out before they actually start
>> selecting the HAS_PCI Kconfig setting).
> 
> I was expecting the lack of 'RFC' tag to attract the attention of the
> maintainers for feedback.

Oh, I see.

>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -783,6 +783,23 @@ bool is_iomem_page(mfn_t mfn)
>>>      return (page_get_owner(page) == dom_io);
>>>  }
>>>  
>>> +bool is_iomem_range(paddr_t start, uint64_t size)
>>> +{
>>> +    unsigned int i;
>>> +
>>> +    for ( i = 0; i < e820.nr_map; i++ )
>>> +    {
>>> +        const struct e820entry *entry = &e820.map[i];
>>> +
>>> +        /* Do not allow overlaps with any memory range. */
>>> +        if ( start < entry->addr + entry->size &&
>>> +             entry->addr < start + size )
>>> +            return false;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>
>> I should have realized (and pointed out) earlier that with the type
>> check dropped the function name no longer represents what the
>> function does. It now really is unsuitable for about anything other
>> that the checking of BARs.
> 
> is_devmem_range would be more suitable?

I'd make it even more tight and use e.g. is_pci_bar_range(). Or
maybe is_uncovered_range()?

> I will wrap this in an #ifdef HAS_PCI section now.

What would be the point of that, as long as X86 selects HAS_PCI
unconditionally? (Arguably one may want to turn off PCI support in
shim-only builds.)

Jan