Re: [Xen-devel] [PATCH 1/3] xen/arm: p2m: extend create_p2m_entries to support read-only mapping

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 11/11/2013 01:23 PM, Ian Campbell wrote:
> On Thu, 2013-10-24 at 10:02 +0100, Julien Grall wrote:
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > ---
> >   xen/arch/arm/p2m.c         | 19 +++++++++++--------
> >   xen/include/asm-arm/page.h |  5 +++--
> >   2 files changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 2d09fef..fdbb07b 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -111,7 +111,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, 1);
> If this is a bool_t then true/false might be more appropriate? (and all
> the other such too)
>
> I have a weak preference for avoiding such boolean parameters, since it
> is never obvious what a given 1/0/true/false is actually is without
> having to look at the prototype. At least there is only 1 such param
> here (now).
>
> We don't need full p2m typing yet but perhaps we can make a start by
> defining "typdef enum { p2m_ram_rw = N; p2m_ram_ro = M } p2m_type_t" for
> this?
Sounds good. I will update the patch series with that.

> > @@ -213,14 +213,15 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn)
> >       return e;
> >   }
> >
> > -static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
> > +static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
> > +                                      bool_t rw)
> >   {
> >       paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
> >       lpae_t e = (lpae_t) {
> >           .p2m.xn = 0,
> >           .p2m.af = 1,
> >           .p2m.sh = LPAE_SH_OUTER,
> > -        .p2m.write = 1,
> > +        .p2m.write = !!rw,
> I don't think the !! is strictly necessary for either a bool_t nor a
> single bit bitfield.
I will use p2m_type_t here and check if the type is equal to p2m_ram_rw.

--
Julien Grall

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