| Message ID | 1382605365-19244-2-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 2013-10-24 at 10:02 +0100, Julien Grall wrote: > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > 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? > @@ -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. Ian.
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@linaro.org> >> --- >> 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.
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); write_pte(entry, pte); @@ -129,7 +129,8 @@ static int create_p2m_entries(struct domain *d, paddr_t start_gpaddr, paddr_t end_gpaddr, paddr_t maddr, - int mattr) + int mattr, + bool_t rw) { int rc, flush; struct p2m_domain *p2m = &d->arch.p2m; @@ -201,14 +202,15 @@ static int create_p2m_entries(struct domain *d, goto out; } - pte = mfn_to_p2m_entry(page_to_mfn(page), mattr); + pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, rw); write_pte(&third[third_table_offset(addr)], pte); } break; case INSERT: { - lpae_t pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr); + lpae_t pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, + mattr, rw); write_pte(&third[third_table_offset(addr)], pte); maddr += PAGE_SIZE; } @@ -243,7 +245,7 @@ int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end) { - return create_p2m_entries(d, ALLOCATE, start, end, 0, MATTR_MEM); + return create_p2m_entries(d, ALLOCATE, start, end, 0, MATTR_MEM, 1); } int map_mmio_regions(struct domain *d, @@ -251,7 +253,8 @@ int map_mmio_regions(struct domain *d, paddr_t end_gaddr, paddr_t maddr) { - return create_p2m_entries(d, INSERT, start_gaddr, end_gaddr, maddr, MATTR_DEV); + return create_p2m_entries(d, INSERT, start_gaddr, end_gaddr, + maddr, MATTR_DEV, 1); } int guest_physmap_add_page(struct domain *d, @@ -261,7 +264,7 @@ int guest_physmap_add_page(struct domain *d, { return create_p2m_entries(d, INSERT, gpfn << PAGE_SHIFT, (gpfn + (1<<page_order)) << PAGE_SHIFT, - mfn << PAGE_SHIFT, MATTR_MEM); + mfn << PAGE_SHIFT, MATTR_MEM, 1); } void guest_physmap_remove_page(struct domain *d, @@ -270,7 +273,7 @@ void guest_physmap_remove_page(struct domain *d, { create_p2m_entries(d, REMOVE, gpfn << PAGE_SHIFT, (gpfn + (1<<page_order)) << PAGE_SHIFT, - mfn << PAGE_SHIFT, MATTR_MEM); + mfn << PAGE_SHIFT, MATTR_MEM, 0); } int p2m_alloc_table(struct domain *d) diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h index 3d0f8a9..3e9dda7 100644 --- a/xen/include/asm-arm/page.h +++ b/xen/include/asm-arm/page.h @@ -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, .p2m.read = 1, .p2m.mattr = mattr, .p2m.table = 1,
Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/p2m.c | 19 +++++++++++-------- xen/include/asm-arm/page.h | 5 +++-- 2 files changed, 14 insertions(+), 10 deletions(-)