Re: [Xen-devel] [PATCH 4/8] xen/arm: Store p2m type in each page of the guest

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-12-05 at 15:55 +0000, Ian Campbell wrote:
[....]

oops, hit send before I was done ;-)

> >      };
> >  
> > +    switch (t)
> > +    {
> > +    case p2m_ram_rw:
> > +    case p2m_mmio_direct:
> > +    case p2m_map_foreign:
> > +        e.p2m.write = 1;
> > +        break;
> > +    case p2m_invalid:
> > +    case p2m_ram_ro:
> > +    default:
> > +        e.p2m.write = 0;
> > +    }
> > +
> >      ASSERT(!(pa & ~PAGE_MASK));
> >      ASSERT(!(pa & ~PADDR_MASK));
> >  
> > @@ -167,7 +181,7 @@ static int p2m_create_table(struct domain *d,
> >      clear_page(p);
> >      unmap_domain_page(p);
> >  
> > -    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM);
> > +    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid);

Is invalid really the right type here?

Are you using invalid for non-leaf pages? I think the table bit suffices
to identify them, doesn't it?

Really the type field is only valid for the leafs ptes (including
superpage leafs).

If it is useful to have "pseudo-types" which don't directly correspond
to the 4 available bits perhaps make them >= 16, although then you would
need a function to take a pte and extract its type taking that into
account.

> >  
> > -int guest_physmap_add_page(struct domain *d,
> > -                           unsigned long gpfn,
> > -                           unsigned long mfn,
> > -                           unsigned int page_order)
> > +int guest_physmap_add_entry(struct domain *d,
> > +                            unsigned long gpfn,
> > +                            unsigned long mfn,
> > +                            unsigned long page_order,
> > +                            p2m_type_t t)
> >  {
> >      return create_p2m_entries(d, INSERT,
> >                                pfn_to_paddr(gpfn),
> > -                              pfn_to_paddr(gpfn + (1<<page_order)),
> > -                              pfn_to_paddr(mfn), MATTR_MEM);
> > +                              pfn_to_paddr(gpfn + (1 << page_order)),
> > +                              pfn_to_paddr(mfn), MATTR_MEM, t);
> >  }
> >  
> >  void guest_physmap_remove_page(struct domain *d,
> > @@ -331,7 +350,7 @@ void guest_physmap_remove_page(struct domain *d,
> >      create_p2m_entries(d, REMOVE,
> >                         pfn_to_paddr(gpfn),
> >                         pfn_to_paddr(gpfn + (1<<page_order)),
> > -                       pfn_to_paddr(mfn), MATTR_MEM);
> > +                       pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);

This looks odd to me, although I understand it is calling a common
helper function which requires a type elsewhere. Perhpas p2m_invalid
here really needs a placeholder. p2m_none == INT_MAX perhaps?

Ian.


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