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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.02.2022 10:57, Julien Grall wrote:
> On 02/02/2022 09:42, Jan Beulich wrote:
>> On 01.02.2022 13:45, Roger Pau Monne wrote:
>>> --- 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_memory_hole(unsigned long start, unsigned long end)
>>> +{
>>> +    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 < PFN_DOWN(entry->addr + entry->size) &&
>>> +             PFN_DOWN(entry->addr) <= end )
>>> +            return false;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>
>> Doesn't the left side of the && need to use PFN_UP()?
>>
>> I also think it would help if a brief comment ahead of the
>> function said that the range is inclusive. Otherwise the use
>> of < and >= gives the impression of something being wrong.
>> Then again it may be better to switch to <= anyway, as I
>> think you want to avoid possible zero-size regions (at which
>> point subtracting 1 for using <= is going to be valid).
>>
>> Finally I wonder whether the function parameters wouldn't
>> better be named e.g. spfn and epfn, but maybe their units can
>> be inferred from their types being unsigned long (which,
>> however, would build on the assumption that we use appropriate
>> types everywhere).
> 
> I think this a case where mfn_t would be useful to use.

Actually I did consider to suggest it when asking to convert
to frame numbers, and specifically didn't because its use will
clutter the code here quite a bit. Which isn't to mean I'd
object if the adjustment was made ...

Jan