Re: [Xen-devel] [PATCH v5 16/23] x86/mm: add pv prefix to {alloc, free}_page_type

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Sep 22, 2017 at 07:40:04AM -0600, Jan Beulich wrote:
> >>> On 14.09.17 at 14:58, <wei.liu2@xxxxxxxxxx> wrote:
> > And move the declarations to pv/mm.h. The code will be moved later.
> > 
> > The stubs contain BUG() because they aren't supposed to be called when
> > PV is disabled.
> 
> I'd prefer ASSERT_UNREACHABLE() - they return proper errors

ASSERT_UNREACHABLE() -- no problem.

> after all, and there's no need to bring down a production system.
> Additionally could you add (half) a sentence regarding the
> PGT_l*_page_table uses outside of PV specific code, which I'm
> sure you have verified can't make it into the stubs?

At this stage I can only verify it by reading the code. I can't turn off
PV yet.

To allocate a PGT_l*_page_table type page, the guest must explicitly
request such types via PV MMU hypercall; there is no code other than the
PV dom0 builder and p2m_alloc_ptp  in the hypervisor would explicitly
ask for PGT_l*_page_table type pages.

p2m_alloc_table is a bit tricky. I think it can get away with not using
PGT_l*_page_table type pages, but that is work for another day.  Anyway,
currently it frees the pages directly with free_page from different
paging modes, all of which won't go into PV mm code.

So my conclusion by reading the code is non-PV code can't make it to the
stubs

> 
> > --- a/xen/include/asm-x86/pv/mm.h
> > +++ b/xen/include/asm-x86/pv/mm.h
> > @@ -32,6 +32,11 @@ bool pv_map_ldt_shadow_page(unsigned int off);
> >  
> >  void pv_arch_init_memory(void);
> >  
> > +int pv_alloc_page_type(struct page_info *page, unsigned long type,
> > +                       int preemptible);
> > +int pv_free_page_type(struct page_info *page, unsigned long type,
> > +                      int preemptible);
> > +
> >  #else
> >  
> >  #include <xen/errno.h>
> > @@ -51,6 +56,13 @@ static inline bool pv_map_ldt_shadow_page(unsigned int 
> > off) { return false; }
> >  
> >  static inline void pv_arch_init_memory(void) {}
> >  
> > +static inline int pv_alloc_page_type(struct page_info *page, unsigned long 
> > type,
> > +                                     int preemptible)
> > +{ BUG(); return -EINVAL; }
> > +static inline int pv_free_page_type(struct page_info *page, unsigned long 
> > type,
> > +                                    int preemptible)
> > +{ BUG(); return -EINVAL; }
> 
> Take the opportunity and make all the "preemptible" parameters bool?

No problem.

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