| Message ID | 1386560047-17500-5-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote: > Use the field 'avail' to store the type of the page. Rename it to 'p2mt' for > convenience. The p2m is redundant in most uses, and the t isn't very descriptive. I think "type" would be fine. > The information stored in this field will be retrieved in a future patch to > hange the behaviour when the page is removed. "change" > Also introduce guest_physmap_add_entry to map and set a specific p2m type for > a page. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v2: > - Rename 'avail' field to 'p2mt' in the p2m structure > - Add BUILD_BUG_ON to check if the enum value will fit in the field > - Implement grant mapping type > --- > xen/arch/arm/p2m.c | 58 ++++++++++++++++++++++++++++++++------------ > xen/include/asm-arm/p2m.h | 18 +++++++++++--- > xen/include/asm-arm/page.h | 2 +- > 3 files changed, 58 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 8f8b47e..e43804c 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -124,7 +124,8 @@ int p2m_pod_decrease_reservation(struct domain *d, > return -ENOSYS; > } > > -static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr) > +static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr, > + p2m_type_t t) > { > paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT; > lpae_t e = (lpae_t) { > @@ -132,12 +133,34 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr) > .p2m.af = 1, > .p2m.sh = LPAE_SH_OUTER, > .p2m.read = 1, > - .p2m.write = 1, > .p2m.mattr = mattr, > .p2m.table = 1, > .p2m.valid = 1, > + .p2m.p2mt = t, > }; > > + BUILD_BUG_ON(p2m_max_real_type > (2 << 4)); > + > + switch (t) > + { > + case p2m_grant_map_rw: > + e.p2m.xn = 1; > + /* Fallthrough */ > + case p2m_ram_rw: > + case p2m_mmio_direct: > + case p2m_map_foreign: > + e.p2m.write = 1; > + break; > + > + case p2m_grant_map_ro: > + e.p2m.xn = 1; > + /* Fallthrough */ > + case p2m_invalid: > + case p2m_ram_ro: > + default: I think you've enumerated all the options, so it is better to leave out the default -- then at least some compilers will complain if we add a new type and forget to add it here. The bulk of the patch looks good though, thanks. Ian.
On 12/09/2013 04:03 PM, Ian Campbell wrote: > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote: >> Use the field 'avail' to store the type of the page. Rename it to 'p2mt' for >> convenience. > > The p2m is redundant in most uses, and the t isn't very descriptive. I > think "type" would be fine. > >> The information stored in this field will be retrieved in a future patch to >> hange the behaviour when the page is removed. > > "change" > >> Also introduce guest_physmap_add_entry to map and set a specific p2m type for >> a page. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> >> --- >> Changes in v2: >> - Rename 'avail' field to 'p2mt' in the p2m structure >> - Add BUILD_BUG_ON to check if the enum value will fit in the field >> - Implement grant mapping type >> --- >> xen/arch/arm/p2m.c | 58 ++++++++++++++++++++++++++++++++------------ >> xen/include/asm-arm/p2m.h | 18 +++++++++++--- >> xen/include/asm-arm/page.h | 2 +- >> 3 files changed, 58 insertions(+), 20 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 8f8b47e..e43804c 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -124,7 +124,8 @@ int p2m_pod_decrease_reservation(struct domain *d, >> return -ENOSYS; >> } >> >> -static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr) >> +static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr, >> + p2m_type_t t) >> { >> paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT; >> lpae_t e = (lpae_t) { >> @@ -132,12 +133,34 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr) >> .p2m.af = 1, >> .p2m.sh = LPAE_SH_OUTER, >> .p2m.read = 1, >> - .p2m.write = 1, >> .p2m.mattr = mattr, >> .p2m.table = 1, >> .p2m.valid = 1, >> + .p2m.p2mt = t, >> }; >> >> + BUILD_BUG_ON(p2m_max_real_type > (2 << 4)); >> + >> + switch (t) >> + { >> + case p2m_grant_map_rw: >> + e.p2m.xn = 1; >> + /* Fallthrough */ >> + case p2m_ram_rw: >> + case p2m_mmio_direct: >> + case p2m_map_foreign: >> + e.p2m.write = 1; >> + break; >> + >> + case p2m_grant_map_ro: >> + e.p2m.xn = 1; >> + /* Fallthrough */ >> + case p2m_invalid: >> + case p2m_ram_ro: >> + default: > > I think you've enumerated all the options, so it is better to leave out > the default -- then at least some compilers will complain if we add a > new type and forget to add it here. > Ok. I will remove it.
On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> + BUILD_BUG_ON(p2m_max_real_type > (2 << 4));
2<<4 is 32.
I think you want either 1<<4, 2^4 or just 16 (my preference is 16). And
I think it should be >= since the valid values are 0..15 inclusive.
Ian.
On 12/09/2013 04:53 PM, Ian Campbell wrote: > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote: >> + BUILD_BUG_ON(p2m_max_real_type > (2 << 4)); > > 2<<4 is 32. > > I think you want either 1<<4, 2^4 or just 16 (my preference is 16). And Right. > I think it should be >= since the valid values are 0..15 inclusive. No, because p2m_max_real_type won't be store in the p2m, so we can have 16 values [0..15] and p2m_max_real_type will be the 17th (ie 16).
On Tue, 2013-12-10 at 01:55 +0000, Julien Grall wrote: > On 12/09/2013 04:53 PM, Ian Campbell wrote: > > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote: > >> + BUILD_BUG_ON(p2m_max_real_type > (2 << 4)); > > > > 2<<4 is 32. > > > > I think you want either 1<<4, 2^4 or just 16 (my preference is 16). And > > Right. > > > I think it should be >= since the valid values are 0..15 inclusive. > > No, because p2m_max_real_type won't be store in the p2m, so we can have > 16 values [0..15] and p2m_max_real_type will be the 17th (ie 16). If p2m_max_real_type will be the 17th (ie 16) then the check needs to be p2m_max_real_type >= 16. Ian.
On 12/10/2013 09:37 AM, Ian Campbell wrote: > On Tue, 2013-12-10 at 01:55 +0000, Julien Grall wrote: >> On 12/09/2013 04:53 PM, Ian Campbell wrote: >>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote: >>>> + BUILD_BUG_ON(p2m_max_real_type > (2 << 4)); >>> >>> 2<<4 is 32. >>> >>> I think you want either 1<<4, 2^4 or just 16 (my preference is 16). And >> >> Right. >> >>> I think it should be >= since the valid values are 0..15 inclusive. >> >> No, because p2m_max_real_type won't be store in the p2m, so we can have >> 16 values [0..15] and p2m_max_real_type will be the 17th (ie 16). > > If p2m_max_real_type will be the 17th (ie 16) then the check needs to be > p2m_max_real_type >= 16. No because p2m_max_real_type = 16 is valid. Which is invalid is foo = 16 and p2m_max_real_type = 17.
On Tue, 2013-12-10 at 13:50 +0000, Julien Grall wrote: > > On 12/10/2013 09:37 AM, Ian Campbell wrote: > > On Tue, 2013-12-10 at 01:55 +0000, Julien Grall wrote: > >> On 12/09/2013 04:53 PM, Ian Campbell wrote: > >>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote: > >>>> + BUILD_BUG_ON(p2m_max_real_type > (2 << 4)); > >>> > >>> 2<<4 is 32. > >>> > >>> I think you want either 1<<4, 2^4 or just 16 (my preference is 16). And > >> > >> Right. > >> > >>> I think it should be >= since the valid values are 0..15 inclusive. > >> > >> No, because p2m_max_real_type won't be store in the p2m, so we can have > >> 16 values [0..15] and p2m_max_real_type will be the 17th (ie 16). > > > > If p2m_max_real_type will be the 17th (ie 16) then the check needs to be > > p2m_max_real_type >= 16. > > No because p2m_max_real_type = 16 is valid. > Which is invalid is foo = 16 and p2m_max_real_type = 17. Oh right, because it's actually the one after the last valid one. OK. Ian.
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 8f8b47e..e43804c 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -124,7 +124,8 @@ int p2m_pod_decrease_reservation(struct domain *d, return -ENOSYS; } -static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr) +static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr, + p2m_type_t t) { paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT; lpae_t e = (lpae_t) { @@ -132,12 +133,34 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr) .p2m.af = 1, .p2m.sh = LPAE_SH_OUTER, .p2m.read = 1, - .p2m.write = 1, .p2m.mattr = mattr, .p2m.table = 1, .p2m.valid = 1, + .p2m.p2mt = t, }; + BUILD_BUG_ON(p2m_max_real_type > (2 << 4)); + + switch (t) + { + case p2m_grant_map_rw: + e.p2m.xn = 1; + /* Fallthrough */ + case p2m_ram_rw: + case p2m_mmio_direct: + case p2m_map_foreign: + e.p2m.write = 1; + break; + + case p2m_grant_map_ro: + e.p2m.xn = 1; + /* Fallthrough */ + case p2m_invalid: + case p2m_ram_ro: + default: + e.p2m.write = 0; + } + ASSERT(!(pa & ~PAGE_MASK)); ASSERT(!(pa & ~PADDR_MASK)); @@ -167,7 +190,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); write_pte(entry, pte); @@ -185,7 +208,8 @@ static int create_p2m_entries(struct domain *d, paddr_t start_gpaddr, paddr_t end_gpaddr, paddr_t maddr, - int mattr) + int mattr, + p2m_type_t t) { int rc, flush; struct p2m_domain *p2m = &d->arch.p2m; @@ -260,14 +284,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, t); 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, t); write_pte(&third[third_table_offset(addr)], pte); maddr += PAGE_SIZE; } @@ -302,7 +327,8 @@ 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, p2m_ram_rw); } int map_mmio_regions(struct domain *d, @@ -310,18 +336,20 @@ 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, p2m_mmio_direct); } -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 +359,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); } int p2m_alloc_table(struct domain *d) diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index f621d15..56bbf4d 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -68,11 +68,21 @@ int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end); int map_mmio_regions(struct domain *d, paddr_t start_gaddr, paddr_t end_gaddr, paddr_t maddr); +int guest_physmap_add_entry(struct domain *d, + unsigned long gfn, + unsigned long mfn, + unsigned long page_order, + p2m_type_t t); + /* Untyped version for RAM only, for compatibility */ -int guest_physmap_add_page(struct domain *d, - unsigned long gfn, - unsigned long mfn, - unsigned int page_order); +static inline int guest_physmap_add_page(struct domain *d, + unsigned long gfn, + unsigned long mfn, + unsigned int page_order) +{ + return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw); +} + void guest_physmap_remove_page(struct domain *d, unsigned long gpfn, unsigned long mfn, unsigned int page_order); diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h index 0625464..a363a62 100644 --- a/xen/include/asm-arm/page.h +++ b/xen/include/asm-arm/page.h @@ -153,7 +153,7 @@ typedef struct { unsigned long contig:1; /* In a block of 16 contiguous entries */ unsigned long sbz2:1; unsigned long xn:1; /* eXecute-Never */ - unsigned long avail:4; /* Ignored by hardware */ + unsigned long p2mt:4; /* Ignore by hardware. Used to store p2m types */ unsigned long sbz1:5; } __attribute__((__packed__)) lpae_p2m_t;
Use the field 'avail' to store the type of the page. Rename it to 'p2mt' for convenience. The information stored in this field will be retrieved in a future patch to hange the behaviour when the page is removed. Also introduce guest_physmap_add_entry to map and set a specific p2m type for a page. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v2: - Rename 'avail' field to 'p2mt' in the p2m structure - Add BUILD_BUG_ON to check if the enum value will fit in the field - Implement grant mapping type --- xen/arch/arm/p2m.c | 58 ++++++++++++++++++++++++++++++++------------ xen/include/asm-arm/p2m.h | 18 +++++++++++--- xen/include/asm-arm/page.h | 2 +- 3 files changed, 58 insertions(+), 20 deletions(-)