Re: [PATCH v5 9/9] xen: retrieve reserved pages on populate_physmap

[Julien Grall <julien@xxxxxxx>]

Hi,

On 31/05/2022 10:40, Jan Beulich wrote:
> On 31.05.2022 11:35, Julien Grall wrote:
> > On 31/05/2022 09:54, Jan Beulich wrote:
> > > On 31.05.2022 05:12, Penny Zheng wrote:
> > > > --- a/xen/common/memory.c
> > > > +++ b/xen/common/memory.c
> > > > @@ -245,6 +245,29 @@ static void populate_physmap(struct memop_args *a)
> > > >    
> > > >                    mfn = _mfn(gpfn);
> > > >                }
> > > > +            else if ( is_domain_using_staticmem(d) )
> > > > +            {
> > > > +                /*
> > > > +                 * No easy way to guarantee the retrieved pages are contiguous,
> > > > +                 * so forbid non-zero-order requests here.
> > > > +                 */
> > > > +                if ( a->extent_order != 0 )
> > > > +                {
> > > > +                    gdprintk(XENLOG_WARNING,
> > > > +                             "Cannot allocate static order-%u pages for static 
> > > > %pd\n",
> > > > +                             a->extent_order, d);
> > > > +                    goto out;
> > > > +                }
> > > > +
> > > > +                mfn = acquire_reserved_page(d, a->memflags);
> > > > +                if ( mfn_eq(mfn, INVALID_MFN) )
> > > > +                {
> > > > +                    gdprintk(XENLOG_WARNING,
> > > > +                             "%pd: failed to retrieve a reserved page\n",
> > > > +                             d);
> > > > +                    goto out;
> > > > +                }
> > > > +            }
> > > I'm not convinced of having these gdprintk()s here.
> > There are a number of time where I wished some error paths would contain
> > debug printk(). Instead, I often end up to add them myself when I
> > struggle to find the reason of a failure.
> But this model doesn't scale - we don't want to have log messages on
> each and every error path. I agree having such for very unlikely
> errors, but order != 0 is clearly a call site mistake and memory
> allocation requests failing also ought to not be entirely unexpected.
The problem is from the guest PoV, the error for both is the same. So it 
would be difficult (not impossible) for the developper to know what's 
the exact problem.
But note that we already have a gdprintk() for allocation failure in the 
non-direct map case. So I think they should be here for consistency.
If you want to drop the existing one, then this is a separate 
discussion. And, just so you know, I would strongly argue against 
removing them for the reason I stated above.
Cheers,

--
Julien Grall