| Message ID | 20190514123125.29086-6-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Provide a generic function to update Xen PT | expand |
On Tue, 14 May 2019, Julien Grall wrote: > At the moment, the flags are not enough to describe what kind of update > will done on the VA range. They need to be used in conjunction with the > enum xenmap_operation. > > It would be more convenient to have all the information for the update > in a single place. > > Two new flags are added to remove the relience on xenmap_operation: > - _PAGE_PRESENT: Indicate whether we are adding/removing the mapping > - _PAGE_POPULATE: Indicate whether we only populate page-tables > > Signed-off-by: Julien Grall <julien.grall@arm.com> > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com> Looking ahead in this series, I know that this is done so that later on you can remove enum xenmap_operation. But what is the end goal? Why do we want to remove enum xenmap_operation? I guess it is to make the code more reusable? > --- > Changes in v2: > - Add Andrii's reviewed-by > --- > xen/arch/arm/mm.c | 2 +- > xen/include/asm-arm/page.h | 9 +++++++-- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 9de2a1150f..2192dede55 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -1083,7 +1083,7 @@ int map_pages_to_xen(unsigned long virt, > > int populate_pt_range(unsigned long virt, unsigned long nr_mfns) > { > - return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0); > + return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE); > } > > int destroy_xen_mappings(unsigned long v, unsigned long e) > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h > index 2bcdb0f1a5..caf2fac1ff 100644 > --- a/xen/include/asm-arm/page.h > +++ b/xen/include/asm-arm/page.h > @@ -76,6 +76,8 @@ > * > * [0:2] Memory Attribute Index > * [3:4] Permission flags > + * [5] Present bit > + * [6] Populate page table [5] is pretty clear. As a nit, I would probably write: [5] Page Present because there is no need to say "bit", the [5] means it is a bit. Otherwise, something like the following: [5] Present in memory but it's unimportant. It's [6] that we might want to explain a bit better: if we say "Populate page table" then every time we want the Xen pagetable to be populated we would need to pass _PAGE_POPULATE, even when the page is present in memory. Do you see what I mean? I am not entirely sure how to make it clearer. Maybe: [6] Only populate page tables "Only" being the key word. > */ > #define PAGE_AI_MASK(x) ((x) & 0x7U) > > @@ -86,12 +88,15 @@ > #define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U) > #define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U) > > +#define _PAGE_PRESENT (1U << 5) > +#define _PAGE_POPULATE (1U << 6) > + > /* > * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not > * meant to be used outside of this header. > */ > -#define _PAGE_DEVICE _PAGE_XN > -#define _PAGE_NORMAL MT_NORMAL > +#define _PAGE_DEVICE (_PAGE_XN|_PAGE_PRESENT) > +#define _PAGE_NORMAL (MT_NORMAL|_PAGE_PRESENT) > > #define PAGE_HYPERVISOR_RO (_PAGE_NORMAL|_PAGE_RO|_PAGE_XN) > #define PAGE_HYPERVISOR_RX (_PAGE_NORMAL|_PAGE_RO) > -- > 2.11.0 >
Hi Stefano, On 11/06/2019 23:35, Stefano Stabellini wrote: > On Tue, 14 May 2019, Julien Grall wrote: >> At the moment, the flags are not enough to describe what kind of update >> will done on the VA range. They need to be used in conjunction with the >> enum xenmap_operation. >> >> It would be more convenient to have all the information for the update >> in a single place. >> >> Two new flags are added to remove the relience on xenmap_operation: >> - _PAGE_PRESENT: Indicate whether we are adding/removing the mapping >> - _PAGE_POPULATE: Indicate whether we only populate page-tables >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com> > > Looking ahead in this series, I know that this is done so that later on > you can remove enum xenmap_operation. But what is the end goal? Why do > we want to remove enum xenmap_operation? I guess it is to make the code > more reusable? The end goal is to streamline as much as possible to page-table update. I wanted to have all the information in flags because it is much easier to reason with one variable over two variables. Furthermore, x86 code allows map_pages_to_xen(...) to destroy mappings but not the underlying page-tables. This is used for instance for the vunmap to avoid re-creating the page-tables afterwards. I have been thinking to introduce similar things on Arm. Keeping the xenmap_operation would make it awkward to support it because you would have to translate the flags to the actual operations. >> --- >> Changes in v2: >> - Add Andrii's reviewed-by >> --- >> xen/arch/arm/mm.c | 2 +- >> xen/include/asm-arm/page.h | 9 +++++++-- >> 2 files changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >> index 9de2a1150f..2192dede55 100644 >> --- a/xen/arch/arm/mm.c >> +++ b/xen/arch/arm/mm.c >> @@ -1083,7 +1083,7 @@ int map_pages_to_xen(unsigned long virt, >> >> int populate_pt_range(unsigned long virt, unsigned long nr_mfns) >> { >> - return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0); >> + return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE); >> } >> >> int destroy_xen_mappings(unsigned long v, unsigned long e) >> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h >> index 2bcdb0f1a5..caf2fac1ff 100644 >> --- a/xen/include/asm-arm/page.h >> +++ b/xen/include/asm-arm/page.h >> @@ -76,6 +76,8 @@ >> * >> * [0:2] Memory Attribute Index >> * [3:4] Permission flags >> + * [5] Present bit >> + * [6] Populate page table > > [5] is pretty clear. As a nit, I would probably write: > > [5] Page Present Better alternative, I will update the comment. > > because there is no need to say "bit", the [5] means it is a bit. > Otherwise, something like the following: > > [5] Present in memory > > but it's unimportant. > > It's [6] that we might want to explain a bit better: if we say "Populate > page table" then every time we want the Xen pagetable to be populated we > would need to pass _PAGE_POPULATE, even when the page is present in > memory. Do you see what I mean? I am not entirely sure how to make it > clearer. Maybe: To be honest, I was not entirely happy with the current comment. But I also wasn't able to find a better one :). > > [6] Only populate page tables I am happy with this alternative. I will update the comment. Cheers,
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 9de2a1150f..2192dede55 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1083,7 +1083,7 @@ int map_pages_to_xen(unsigned long virt, int populate_pt_range(unsigned long virt, unsigned long nr_mfns) { - return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0); + return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE); } int destroy_xen_mappings(unsigned long v, unsigned long e) diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h index 2bcdb0f1a5..caf2fac1ff 100644 --- a/xen/include/asm-arm/page.h +++ b/xen/include/asm-arm/page.h @@ -76,6 +76,8 @@ * * [0:2] Memory Attribute Index * [3:4] Permission flags + * [5] Present bit + * [6] Populate page table */ #define PAGE_AI_MASK(x) ((x) & 0x7U) @@ -86,12 +88,15 @@ #define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U) #define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U) +#define _PAGE_PRESENT (1U << 5) +#define _PAGE_POPULATE (1U << 6) + /* * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not * meant to be used outside of this header. */ -#define _PAGE_DEVICE _PAGE_XN -#define _PAGE_NORMAL MT_NORMAL +#define _PAGE_DEVICE (_PAGE_XN|_PAGE_PRESENT) +#define _PAGE_NORMAL (MT_NORMAL|_PAGE_PRESENT) #define PAGE_HYPERVISOR_RO (_PAGE_NORMAL|_PAGE_RO|_PAGE_XN) #define PAGE_HYPERVISOR_RX (_PAGE_NORMAL|_PAGE_RO)
At the moment, the flags are not enough to describe what kind of update will done on the VA range. They need to be used in conjunction with the enum xenmap_operation. It would be more convenient to have all the information for the update in a single place. Two new flags are added to remove the relience on xenmap_operation: - _PAGE_PRESENT: Indicate whether we are adding/removing the mapping - _PAGE_POPULATE: Indicate whether we only populate page-tables Signed-off-by: Julien Grall <julien.grall@arm.com> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com> --- Changes in v2: - Add Andrii's reviewed-by --- xen/arch/arm/mm.c | 2 +- xen/include/asm-arm/page.h | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-)