Re: [Xen-devel] [PATCH RFC 01/12] x86/paging: introduce paging_set_allocation

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, Jul 29, 2016 at 05:47:24PM +0100, Andrew Cooper wrote:
> On 29/07/16 17:28, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> > index 107fc8c..1b270df 100644
> > --- a/xen/arch/x86/mm/paging.c
> > +++ b/xen/arch/x86/mm/paging.c
> > @@ -953,6 +953,22 @@ void paging_write_p2m_entry(struct p2m_domain *p2m, 
> > unsigned long gfn,
> >          safe_write_pte(p, new);
> >  }
> >  
> > +int paging_set_allocation(struct domain *d, unsigned long pages)
> > +{
> > +    int rc;
> > +
> > +    ASSERT(paging_mode_enabled(d));
> > +
> > +    paging_lock(d);
> > +    if ( hap_enabled(d) )
> > +        rc = hap_set_allocation(d, pages, NULL);
> > +    else
> > +        rc = sh_set_allocation(d, pages, NULL);
> 
> (without looking at the rest of the series) Your NMI is probably a
> watchdog timeout from this call, as passing NULL means "non-preemptible".

I don't think so, the NMI I saw happened while the guest was booting.

> As this is for the construction of dom0, it would be better to take a
> preemptible pointer, loop in construct_dom0(), with a
> process_pending_softirqs() call in between.

Now fixed.

> > +    paging_unlock(d);
> > +
> > +    return rc;
> > +}
> > +
> >  /*
> >   * Local variables:
> >   * mode: C
> > diff --git a/xen/arch/x86/mm/shadow/common.c 
> > b/xen/arch/x86/mm/shadow/common.c
> > index c22362f..452e22e 100644
> > --- a/xen/arch/x86/mm/shadow/common.c
> > +++ b/xen/arch/x86/mm/shadow/common.c
> > @@ -1604,9 +1604,8 @@ shadow_free_p2m_page(struct domain *d, struct 
> > page_info *pg)
> >   * Input will be rounded up to at least shadow_min_acceptable_pages(),
> >   * plus space for the p2m table.
> >   * Returns 0 for success, non-zero for failure. */
> > -static unsigned int sh_set_allocation(struct domain *d,
> > -                                      unsigned int pages,
> > -                                      int *preempted)
> > +unsigned int sh_set_allocation(struct domain *d, unsigned int pages,
> > +                               int *preempted)
> >  {
> >      struct page_info *sp;
> >      unsigned int lower_bound;
> > diff --git a/xen/include/asm-x86/hap.h b/xen/include/asm-x86/hap.h
> > index c613836..e3c9c98 100644
> > --- a/xen/include/asm-x86/hap.h
> > +++ b/xen/include/asm-x86/hap.h
> > @@ -46,7 +46,8 @@ int   hap_track_dirty_vram(struct domain *d,
> >                             XEN_GUEST_HANDLE_64(uint8) dirty_bitmap);
> >  
> >  extern const struct paging_mode *hap_paging_get_mode(struct vcpu *);
> > -void hap_set_alloc_for_pvh_dom0(struct domain *d, unsigned long num_pages);
> > +unsigned int hap_set_allocation(struct domain *d, unsigned int pages,
> > +                           int *preempted);
> 
> I also note from this change that there is an unsigned long => unsigned
> int truncation in the internals of *_set_allocation().  This should
> definitely be fixed, although possibly wants to be a separate change.

Right, let me take a stab at this.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel