Re: [Xen-devel] [PATCH RFC v1 44/74] xen/pvshim: keep track of unused pages

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 08, 2018 at 03:58:22AM -0700, Jan Beulich wrote:
> >>> On 04.01.18 at 14:05, <wei.liu2@xxxxxxxxxx> wrote:
> > +static void __init init_memmap(void)
> > +{
> > +    unsigned int i;
> > +
> > +    mem = rangeset_new(NULL, "host memory map", 0);
> > +    if ( !mem )
> > +        panic("failed to allocate host memory rangeset");
> 
> "host" is meant from the perspective of the shim on itself here aiui,
> not the underlying entity? I find using that term here at least
> misleading.

Does "failed to allocate memory tracking rangeset" seem better?

> > +    /* Mark up to the last memory page (or 4GB) as RAM. */
> > +    if ( rangeset_add_range(mem, 0, max_t(unsigned long, max_page,
> > +                                          (GB(4) - 1) >> PAGE_SHIFT)) )
> 
> Don't you also need "max_page - 1" then? Also - why the
> saturation to 4Gb?

There's the MMIO hole below 4GiB, and I wanted to prevent using memory
from there. I know there can still be MMIO holes above 4GiB, but it's
less likely.

> > +        panic("unable to add RAM to memory rangeset");
> > +
> > +    for ( i = 0; i < e820.nr_map; i++ )
> > +    {
> > +        struct e820entry *e = &e820.map[i];
> > +
> > +        if ( rangeset_add_range(mem, e->addr >> PAGE_SHIFT,
> > +                                (e->addr + e->size) >> PAGE_SHIFT) )
> 
> PFN_DOWN() and PFN_UP() respectively. Plus aren't rangeset
> ranges exclusive of their upper ends, making it necessary to
> subtract 1 from the upper bound?

Right.

> > @@ -43,11 +46,30 @@ static inline void hypervisor_early_setup(struct 
> > e820map *e820)
> >  {
> >      ASSERT_UNREACHABLE();
> >  };
> > +
> > +static inline void hypervisor_setup(void)
> > +{
> > +    ASSERT_UNREACHABLE();
> > +}
> > +
> >  static inline void hypervisor_ap_setup(void)
> >  {
> >      ASSERT_UNREACHABLE();
> >  };
> >  
> > +static inline int hypervisor_alloc_unused_page(mfn_t *mfn)
> > +{
> > +
> > +    ASSERT_UNREACHABLE();
> > +    return 0;
> > +}
> > +
> > +static inline int hypervisor_free_unused_page(mfn_t mfn)
> > +{
> > +    ASSERT_UNREACHABLE();
> > +    return 0;
> > +}
> 
> I can see the value of hypervisor_setup() stub, but are the other
> two really going to be needed, i.e. are any such allocations being
> placed into not shim specific code (doesn't seem very likely)?

Yes, I think I we can get rid of those two helpers.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel