On Wed, Feb 02, 2022 at 10:42:22AM +0100, 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()?
Hm, I had is using PFN_UP before and switched to PFN_DOWN for some
weird reasoning.
>
> 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).
Right, so that would end up being:
start <= PFN_DOWN(entry->addr + entry->size - 1) &&
Rejecting entries with size == 0 beforehand.
> 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 guess I should switch to using mfn_t for the types and convert them
locally to unsigned long for the comparisons.
> > --- 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".
Doh, yes, sorry. Will convert them to mfn_t if we agree on that.
Thanks, Roger.
