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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -554,6 +554,8 @@ int __must_check steal_page(struct domain *d, struct 
> page_info *page,
>  int page_is_ram_type(unsigned long mfn, unsigned long mem_type);
>  /* Returns the page type(s). */
>  unsigned int page_get_ram_type(mfn_t mfn);
> +/* Check if a range falls into a hole in the memory map. */
> +bool is_memory_hole(paddr_t start, uint64_t size);

While resolving to the same type, these now also want to be
"unsigned long".

Jan