Re: [Xen-devel] [PATCH 2/2 v2] xen: arm: Set all bits in mfn_to_xen_entry()
Hi Andrew, > On 21/08/15 00:33, Chris Brand wrote: > > Ensure that every bit has a specific value. > > > > Reported-by: Julien Grall > > Signed-off-by: Chris Brand > > --- > > v2 adds comments on pxn and avail. > > This is no functional change, if the compiler is conforming to the C spec. > > The spec guarantees that structure initialisation like this causes unspecified > names to gain their default value. As these are integer bitfields, the > default > value is 0. > > What compiler is in use? It would appear that it is buggy, or at least has > buggy scalar replacement optimisations. That's right. I'd forgotten about that. This was actually suggested by Julien in a review of another patch I sent. I haven't seen any problems this fixes. Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/2] xen: arm: Ensure all PTE bits have a known value
> > P.S. Apologies for any threading issues - I have not yet managed to > > get git send-email working properly. > > I'm usually using git format-patch to get a list of files (one per > patch) and my cover letter (--cover-letter). Thanks. > Then I use git send-email *.patch which will send the patch correctly > threading. Yeah, the difficulty is finding the right set of --smtp* options to get it through our corporate mail server. I had it working once upon a time when I was working for Linaro, but something somewhere seems to have changed :-) Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] xen: arm: Set all bits in mfn_to_xen_entry()
Hi Julien, Thanks for the review. > > +.pxn = 0, > > I would add a comment to explain that this bit is reserved for PL2 stage > 1 page table. Will do. > > +.avail = 0, > > I don't think this one is necessary. avail is not used by the hardware neither > Xen. "grep -rF pt.avail xen" gives 7 matches in xen/arch/arm/mm.c, so it is used by Xen (I'll fully admit that I didn't dig in to the "how and why" of its use). > what about *t fields (pxnt, xnt, apt,...)? I figured that as we're setting table to 0, these are ignored, and any code setting table to 1 should then set them. I can obviously easily set them here (I guess all to zero would make sense) if you think it's worthwhile ... Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/2] xen: arm: Set all bits in mfn_to_xen_entry()
Ensure that every bit has a specific value. Reported-by: Julien Grall Signed-off-by: Chris Brand --- xen/include/asm-arm/page.h | 5 + 1 file changed, 5 insertions(+) diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h index 01628f3e96cb..7a56b2cb463a 100644 --- a/xen/include/asm-arm/page.h +++ b/xen/include/asm-arm/page.h @@ -202,9 +202,14 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn, unsigned attr) .ai = attr, .ns = 1, /* Hyp mode is in the non-secure world */ .user = 1,/* See below */ +.ro = 0, /* Assume read-write */ .af = 1, /* No need for access tracking */ .ng = 1, /* Makes TLB flushes easier */ +.sbz = 0, +.contig = 0, /* Assume non-contiguous */ +.pxn = 0, .xn = 1, /* No need to execute outside .text */ +.avail = 0, }};; /* Setting the User bit is strange, but the ATS1H[RW] instructions * don't seem to work otherwise, and since we never run on Xen -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 0/2] xen: arm: Ensure all PTE bits have a known value
Hi, This is more-or-less what Julien requested. He noted that pt.contig was never set to zero. When I looked further, I found other bits that were also never given a value. Looking at the callsites, they almost all seem to assume a value of zero, so that's what I went with. Patch 1 re-orders the assignments to match the declaration, making it clearer which ones are missing. Patch 2 then adds the missing bits. Chris P.S. Apologies for any threading issues - I have not yet managed to get git send-email working properly. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/2] xen: arm re-order assignments in mfn_to_xen_entry()
Shuffle lines around so that the assignments in mfn_to_xen_entry()
occur in the same order as the bits are declared in lpae_pt_t.
This makes it easier to see which ones are never given a value.
No change in behaviour.
Also fix a minor comment typo.
Signed-off-by: Chris Brand
---
xen/include/asm-arm/page.h | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 5ecfd0705e07..01628f3e96cb 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -197,18 +197,18 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn,
unsigned attr)
paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
lpae_t e = (lpae_t) {
.pt = {
-.xn = 1, /* No need to execute outside .text */
-.ng = 1, /* Makes TLB flushes easier */
-.af = 1, /* No need for access tracking */
+.valid = 1, /* Mappings are present */
+.table = 0, /* Set to 1 for links and 4k maps */
+.ai = attr,
.ns = 1, /* Hyp mode is in the non-secure world */
.user = 1,/* See below */
-.ai = attr,
-.table = 0, /* Set to 1 for links and 4k maps */
-.valid = 1, /* Mappings are present */
+.af = 1, /* No need for access tracking */
+.ng = 1, /* Makes TLB flushes easier */
+.xn = 1, /* No need to execute outside .text */
}};;
/* Setting the User bit is strange, but the ATS1H[RW] instructions
* don't seem to work otherwise, and since we never run on Xen
- * pagetables un User mode it's OK. If this changes, remember
+ * pagetables in User mode it's OK. If this changes, remember
* to update the hard-coded values in head.S too */
switch ( attr )
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 0/2] xen: arm: Ensure all PTE bits have a known value
Hi, This is more-or-less what Julien requested. He noted that pt.contig was never set to zero. When I looked further, I found other bits that were also never given a value. Looking at the callsites, they almost all seem to assume a value of zero, so that's what I went with. Patch 1 re-orders the assignments to match the declaration, making it clearer which ones are missing. Patch 2 then adds the missing bits. Chris P.S. Apologies for any threading issues - I have not yet managed to get git send-email working properly. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen: arm: Support <32MB frametables
Hi Julien, Thanks for the review. > > /* Create Xen's mappings of memory. > > - * Base and virt must be 32MB aligned and size a multiple of 32MB. > > + * Mapping_size must be either 2MB or 32MB. > > I would have generalize the function to support any mapping size. But I'm > also fine with the current solution. The code would have to be (somewhat) more complex to do that. I think I prefer to keep it simple for now. > > +const unsigned long granularity = mapping_size >> PAGE_SHIFT; > > This variable is only used in the ASSERT. On non-debug build the compiler will > throw an error because the variable will be unused. Good point. Will fix. > > +if ( mapping_size == MB(32) ) > > +pte.pt.contig = 1; /* These maps are in 16-entry contiguous > > + chunks. */ > > mfn_to_xen_entry never set the contig bit to 0 (or anything else). So the > value will be undefined for MB(2) mapping. > > It looks like to me that mfn_to_xen_entry should always set the contig bit to > 0. I'm not sure why we didn't see any issue until now. Can you please send a > patch for this? Will do. Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen: arm: Support <32MB frametables
Any thoughts on v2 ?
Chris
> -Original Message-
> From: Chris (Christopher) Brand
> Sent: Friday, 07 August, 2015 1:41 PM
> To: 'Julien Grall'; xen-devel@lists.xen.org
> Cc: Stefano Stabellini; Ian Campbell (ian.campb...@citrix.com)
> Subject: [PATCH v2] xen: arm: Support <32MB frametables
>
> setup_frametable_mappings() rounds frametable_size up to a multiple of
> 32MB. This is wasteful on systems with less than 4GB of RAM, although it
> does allow the "contig" bit to be set in the PTEs.
>
> Where the frametable is less than 32MB in size, instead round up to a
> multiple of 2MB, not setting the "contig" bit in the PTEs.
>
> Signed-off-by: Chris Brand
> ---
> Changed in v2: merged create_32mb_mappings() and
> create_2mb_mappings() into create_mappings(). A side-effect is to fix the
> bug in v1 for
> ARM64 systems with <4GB RAM.
>
> xen/arch/arm/mm.c | 37 ++---
> 1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index
> ae0f34c3c480..fd64fbfdfb93 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -628,25 +628,31 @@ void __cpuinit mmu_init_secondary_cpu(void) }
>
> /* Create Xen's mappings of memory.
> - * Base and virt must be 32MB aligned and size a multiple of 32MB.
> + * Mapping_size must be either 2MB or 32MB.
> + * Base and virt must be mapping_size aligned.
> + * Size must be a multiple of mapping_size.
> * second must be a contiguous set of second level page tables
> * covering the region starting at virt_offset. */ -static void __init
> create_32mb_mappings(lpae_t *second,
> -unsigned long virt_offset,
> -unsigned long base_mfn,
> -unsigned long nr_mfns)
> +static void __init create_mappings(lpae_t *second,
> + unsigned long virt_offset,
> + unsigned long base_mfn,
> + unsigned long nr_mfns,
> + unsigned int mapping_size)
> {
> unsigned long i, count;
> +const unsigned long granularity = mapping_size >> PAGE_SHIFT;
> lpae_t pte, *p;
>
> -ASSERT(!((virt_offset >> PAGE_SHIFT) % (16 * LPAE_ENTRIES)));
> -ASSERT(!(base_mfn % (16 * LPAE_ENTRIES)));
> -ASSERT(!(nr_mfns % (16 * LPAE_ENTRIES)));
> +ASSERT((mapping_size == MB(2)) || (mapping_size == MB(32)));
> +ASSERT(!((virt_offset >> PAGE_SHIFT) % granularity));
> +ASSERT(!(base_mfn % granularity));
> +ASSERT(!(nr_mfns % granularity));
>
> count = nr_mfns / LPAE_ENTRIES;
> p = second + second_linear_offset(virt_offset);
> pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
> -pte.pt.contig = 1; /* These maps are in 16-entry contiguous chunks. */
> +if ( mapping_size == MB(32) )
> +pte.pt.contig = 1; /* These maps are in 16-entry contiguous
> + chunks. */
> for ( i = 0; i < count; i++ )
> {
> write_pte(p + i, pte);
> @@ -660,7 +666,7 @@ static void __init create_32mb_mappings(lpae_t
> *second, void __init setup_xenheap_mappings(unsigned long base_mfn,
> unsigned long nr_mfns) {
> -create_32mb_mappings(xen_second, XENHEAP_VIRT_START, base_mfn,
> nr_mfns);
> +create_mappings(xen_second, XENHEAP_VIRT_START, base_mfn,
> nr_mfns,
> + MB(32));
>
> /* Record where the xenheap is, for translation routines. */
> xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE; @@
> -749,6 +755,7 @@ void __init setup_frametable_mappings(paddr_t ps,
> paddr_t pe)
> unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
> unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
> unsigned long base_mfn;
> +const unsigned long mapping_size = frametable_size < MB(32) ? MB(2)
> + : MB(32);
> #ifdef CONFIG_ARM_64
> lpae_t *second, pte;
> unsigned long nr_second, second_base; @@ -756,9 +763,8 @@ void __init
> setup_frametable_mappings(paddr_t ps, paddr_t pe) #endif
>
> frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
> -
> -/* Round up to 32M boundary */
> -frametable_size = (frametable_size + 0x1ff) & ~0x1ff;
> +/* Round up to 2M or 32M boundary, as appropriate. */
> +frametable_size = ROUNDUP(frametable_size, mapping_size);
> base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-
> 12));
>
> #ifdef CONFIG_ARM_64
> @@ -771,9 +777,10 @@ void __init setup_frametable_mappings(p
[Xen-devel] [PATCH v2] xen: arm: Support <32MB frametables
setup_frametable_mappings() rounds frametable_size up to a multiple
of 32MB. This is wasteful on systems with less than 4GB of RAM,
although it does allow the "contig" bit to be set in the PTEs.
Where the frametable is less than 32MB in size, instead round up
to a multiple of 2MB, not setting the "contig" bit in the PTEs.
Signed-off-by: Chris Brand
---
Changed in v2: merged create_32mb_mappings() and create_2mb_mappings()
into create_mappings(). A side-effect is to fix the bug in v1 for
ARM64 systems with <4GB RAM.
xen/arch/arm/mm.c | 37 ++---
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ae0f34c3c480..fd64fbfdfb93 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -628,25 +628,31 @@ void __cpuinit mmu_init_secondary_cpu(void)
}
/* Create Xen's mappings of memory.
- * Base and virt must be 32MB aligned and size a multiple of 32MB.
+ * Mapping_size must be either 2MB or 32MB.
+ * Base and virt must be mapping_size aligned.
+ * Size must be a multiple of mapping_size.
* second must be a contiguous set of second level page tables
* covering the region starting at virt_offset. */
-static void __init create_32mb_mappings(lpae_t *second,
-unsigned long virt_offset,
-unsigned long base_mfn,
-unsigned long nr_mfns)
+static void __init create_mappings(lpae_t *second,
+ unsigned long virt_offset,
+ unsigned long base_mfn,
+ unsigned long nr_mfns,
+ unsigned int mapping_size)
{
unsigned long i, count;
+const unsigned long granularity = mapping_size >> PAGE_SHIFT;
lpae_t pte, *p;
-ASSERT(!((virt_offset >> PAGE_SHIFT) % (16 * LPAE_ENTRIES)));
-ASSERT(!(base_mfn % (16 * LPAE_ENTRIES)));
-ASSERT(!(nr_mfns % (16 * LPAE_ENTRIES)));
+ASSERT((mapping_size == MB(2)) || (mapping_size == MB(32)));
+ASSERT(!((virt_offset >> PAGE_SHIFT) % granularity));
+ASSERT(!(base_mfn % granularity));
+ASSERT(!(nr_mfns % granularity));
count = nr_mfns / LPAE_ENTRIES;
p = second + second_linear_offset(virt_offset);
pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
-pte.pt.contig = 1; /* These maps are in 16-entry contiguous chunks. */
+if ( mapping_size == MB(32) )
+pte.pt.contig = 1; /* These maps are in 16-entry contiguous chunks. */
for ( i = 0; i < count; i++ )
{
write_pte(p + i, pte);
@@ -660,7 +666,7 @@ static void __init create_32mb_mappings(lpae_t *second,
void __init setup_xenheap_mappings(unsigned long base_mfn,
unsigned long nr_mfns)
{
-create_32mb_mappings(xen_second, XENHEAP_VIRT_START, base_mfn, nr_mfns);
+create_mappings(xen_second, XENHEAP_VIRT_START, base_mfn, nr_mfns, MB(32));
/* Record where the xenheap is, for translation routines. */
xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
@@ -749,6 +755,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t
pe)
unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
unsigned long base_mfn;
+const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) :
MB(32);
#ifdef CONFIG_ARM_64
lpae_t *second, pte;
unsigned long nr_second, second_base;
@@ -756,9 +763,8 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t
pe)
#endif
frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
-
-/* Round up to 32M boundary */
-frametable_size = (frametable_size + 0x1ff) & ~0x1ff;
+/* Round up to 2M or 32M boundary, as appropriate. */
+frametable_size = ROUNDUP(frametable_size, mapping_size);
base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
#ifdef CONFIG_ARM_64
@@ -771,9 +777,10 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t
pe)
pte.pt.table = 1;
write_pte(&xen_first[first_table_offset(FRAMETABLE_VIRT_START)+i],
pte);
}
-create_32mb_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT);
+create_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT,
mapping_size);
#else
-create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START, base_mfn,
frametable_size >> PAGE_SHIFT);
+create_mappings(xen_second, FRAMETABLE_VIRT_START,
+base_mfn, frametable_size >> PAGE_SHIFT, mapping_size);
#endif
memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: arm: Support <32MB frametables
>> OOI, you win 30MB of RAM but how does this affect the performance? > > Fair question. :-) All I can say is that I don't see any noticeable > difference on my system. > Are there performance tests that you suggest I run ? > Also, note that the new code is > only executed if you specify a previously-invalid value for xenheap_megabytes > on the > command-line, so it won't affect any existing systems. This sentence is a lie, sorry. Please ignore it. :-) > Is it worth adding a sentence > mentioning performance to the documentation, do you think ? Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: arm: Support <32MB frametables
Hi Julien, Thanks for the review. > OOI, you win 30MB of RAM but how does this affect the performance? Fair question. :-) All I can say is that I don't see any noticeable difference on my system. Are there performance tests that you suggest I run ? Also, note that the new code is only executed if you specify a previously-invalid value for xenheap_megabytes on the command-line, so it won't affect any existing systems. Is it worth adding a sentence mentioning performance to the documentation, do you think ? > Can you rework create_32mb_mappings to take the size of the mappings in > parameters? Yeah, I have a version like that here somewhere, but it wasn't as clean as I'd hoped. I'll re-visit it and send as v2. >> -/* Round up to 32M boundary */ >> -frametable_size = (frametable_size + 0x1ff) & ~0x1ff; >> +/* Round up to 2M or 32M boundary, as appropriate. */ >> +if ( frametable_size < MB(32) ) >> +mask = MB(2) - 1; >> +else >> +mask = MB(32) - 1; >> +frametable_size = (frametable_size + mask) & ~mask; > >You can use ROUNDUP(frametable_size, size) to avoid open-coding the mask. Will do. > Also, this code is common with ARM64. If we happen to have a board with a > frametable smaller than 32MB, you will round up to 2MB and crash later in > create_32mb_mappings because you don't support 2MB mapping for ARM64. > > It might be good to support 2MB for ARM64 too. Whoops! Yes, I've messed the ARM64 path up at some point while cleaning up the code. Will fix. Thanks, Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen: arm: Support <32MB frametables
setup_frametable_mappings() rounds frametable_size up to a multiple
of 32MB. This is wasteful on systems with less than 4GB of RAM,
although it does allow the "contig" bit to be set in the PTEs.
Where the frametable is less than 32MB in size, instead round up
to a multiple of 2MB, not setting the "contig" bit in the PTEs.
Signed-off-by: Chris Brand
---
xen/arch/arm/mm.c | 39 ---
1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a91ea774f1f9..47b6d5d44563 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -656,6 +656,29 @@ static void __init create_32mb_mappings(lpae_t *second,
}
#ifdef CONFIG_ARM_32
+static void __init create_2mb_mappings(lpae_t *second,
+ unsigned long virt_offset,
+ unsigned long base_mfn,
+ unsigned long nr_mfns)
+{
+unsigned long i, count;
+lpae_t pte, *p;
+
+ASSERT(!((virt_offset >> PAGE_SHIFT) % LPAE_ENTRIES));
+ASSERT(!(base_mfn % LPAE_ENTRIES));
+ASSERT(!(nr_mfns % LPAE_ENTRIES));
+
+count = nr_mfns / LPAE_ENTRIES;
+p = second + second_linear_offset(virt_offset);
+pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
+for ( i = 0; i < count; i++ )
+{
+write_pte(p + i, pte);
+pte.pt.base += 1 << LPAE_SHIFT;
+}
+flush_xen_data_tlb_local();
+}
+
/* Set up the xenheap: up to 1GB of contiguous, always-mapped memory. */
void __init setup_xenheap_mappings(unsigned long base_mfn,
unsigned long nr_mfns)
@@ -749,6 +772,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t
pe)
unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
unsigned long base_mfn;
+unsigned long mask;
#ifdef CONFIG_ARM_64
lpae_t *second, pte;
unsigned long nr_second, second_base;
@@ -757,8 +781,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t
pe)
frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
-/* Round up to 32M boundary */
-frametable_size = (frametable_size + 0x1ff) & ~0x1ff;
+/* Round up to 2M or 32M boundary, as appropriate. */
+if ( frametable_size < MB(32) )
+mask = MB(2) - 1;
+else
+mask = MB(32) - 1;
+frametable_size = (frametable_size + mask) & ~mask;
base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
#ifdef CONFIG_ARM_64
@@ -773,7 +801,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t
pe)
}
create_32mb_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT);
#else
-create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START, base_mfn,
frametable_size >> PAGE_SHIFT);
+if ( frametable_size < MB(32) )
+create_2mb_mappings(xen_second, FRAMETABLE_VIRT_START,
+base_mfn, frametable_size >> PAGE_SHIFT);
+else
+create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START,
+ base_mfn, frametable_size >> PAGE_SHIFT);
#endif
memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/8] xen: Use the correctly the Xen memory terminologies
> Based on include/xen/mm.h [1], Linux is mistakenly using MFN when GFN is > meant, > I suspect this is because the first support for Xen was for PV. This brough > some Typo : "brought" Perhaps "resulted in" would be better ? > misimplementation of helpers on ARM and make the developper confused the > expected behavior. Typo: "developer". I'd also suggest "...and confused developers about the...". [...] > For clarity and avoid new confusion, replace any reference of mfn into gnf in > any helpers used by PV drivers. Typo : "gfn" I'd suggest "...replace any reference to mfn with gfn..." [...] Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen: arm: Document xenheap_megabytes limitation
In setup_mm(), the value passed as xenheap_megabytes gets converted to pages and passed to setup_xenheap_mappings(), which in turn passes it to create_32mb_mappings(), which contains an ASSERT that the value passed is a multiple of 32MB. So specifying any value that is not an integer multiple of 32 will cause Xen to hit this assert and fail to boot. Signed-off-by: Chris Brand --- docs/misc/xen-command-line.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index 4889e27626d4..f3d3bd6ce56a 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -242,7 +242,7 @@ the NMI watchdog is also enabled. > Default: `0` (1/32 of RAM) -Amount of RAM to set aside for the Xenheap. +Amount of RAM to set aside for the Xenheap. Must be an integer multiple of 32. By default will use 1/32 of the RAM up to a maximum of 1GB and with a minimum of 32M, subject to a suitably aligned and sized contiguous -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] ARM - why does setup_frametable_size() round frametable_size to 32MB ?
Thanks, Ian. I tried that, and it does seem to work (everything boots, I can
still bring up VMs, and I see an extra 16MB of free memory). The patch I came
up with follows (it would be nice to share code between create_32mb_mappings()
and create_2mb_mappings(), but the setting of the contig bit is right in the
middle, and the functions are pretty short).
Chris
From: Chris Brand
Date: Mon, 20 Jul 2015 13:38:15 -0700
Subject: [PATCH] xen: arm: Support <32MB frametables
setup_frametable_mappings() rounds frametable_size up to a multiple
of 32MB. This is wasteful on systemes with less than 4GB of RAM,
although it does allow the "contig" bit to be set in the PTEs.
Where the frametable is less than 32MB in size, instead round up
to a multiple of 2MB, not setting the "contig" bit in the PTEs.
Signed-off-by: Chris Brand
---
xen/arch/arm/mm.c | 39 ---
1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a91ea774f1f9..a7f4864f8d8f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -656,6 +656,29 @@ static void __init create_32mb_mappings(lpae_t *second,
}
#ifdef CONFIG_ARM_32
+static void __init create_2mb_mappings(lpae_t *second,
+ unsigned long virt_offset,
+ unsigned long base_mfn,
+ unsigned long nr_mfns)
+{
+unsigned long i, count;
+lpae_t pte, *p;
+
+ASSERT(!((virt_offset >> PAGE_SHIFT) % LPAE_ENTRIES));
+ASSERT(!(base_mfn % LPAE_ENTRIES));
+ASSERT(!(nr_mfns % LPAE_ENTRIES));
+
+count = nr_mfns / LPAE_ENTRIES;
+p = second + second_linear_offset(virt_offset);
+pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
+for ( i = 0; i < count; i++ )
+{
+write_pte(p + i, pte);
+pte.pt.base += 1 << LPAE_SHIFT;
+}
+flush_xen_data_tlb_local();
+}
+
/* Set up the xenheap: up to 1GB of contiguous, always-mapped memory. */
void __init setup_xenheap_mappings(unsigned long base_mfn,
unsigned long nr_mfns)
@@ -749,6 +772,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t
pe)
unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
unsigned long base_mfn;
+unsigned long mask;
#ifdef CONFIG_ARM_64
lpae_t *second, pte;
unsigned long nr_second, second_base;
@@ -757,8 +781,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t
pe)
frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
-/* Round up to 32M boundary */
-frametable_size = (frametable_size + 0x1ff) & ~0x1ff;
+/* Round up to 2M or 32M boundary, as appropriate */
+if (frametable_size < MB(32))
+mask = MB(2) - 1;
+else
+mask = MB(32) - 1;
+frametable_size = (frametable_size + mask) & ~mask;
base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
#ifdef CONFIG_ARM_64
@@ -773,7 +801,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t
pe)
}
create_32mb_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT);
#else
-create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START, base_mfn,
frametable_size >> PAGE_SHIFT);
+if (frametable_size < MB(32))
+create_2mb_mappings(xen_second, FRAMETABLE_VIRT_START,
+base_mfn, frametable_size >> PAGE_SHIFT);
+else
+create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START,
+ base_mfn, frametable_size >> PAGE_SHIFT);
#endif
memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: arm: Avoid reading beyond the last module
> It looks like the mail as been sent in HTML. Can you resend it in plain text? Re-sent, hopefully correctly this time. Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] ARM - why does setup_frametable_size() round frametable_size to 32MB ?
Hi, I'm working on a platform with a mere 2GB of RAM, and trying to trim the Xen footprint down as much as possible. I've found two places where Xen uses more memory than it seems it needs to, one of which is the frametable. On a 2GB system, frametable_size is initially calculated as 16MB, but is then rounded up to 32MB. can somebody tell me why this is done, and therefore whether it can be avoided ? I assume it's because the code then calls create_32mb_mappings(), in which case I guess my question is what's special about 32MB ? unsigned long frametable_size = nr_pdxs * sizeof(struct page_info); [...] /* Round up to 32M boundary */ frametable_size = (frametable_size + 0x1ff) & ~0x1ff; Thanks, Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen: arm: Avoid reading beyond the last module
nr_mods is set in add_boot_module() to the number of module
array elements used. This function also ensures that nr_mods
never exceeds MAX_MODULES (the size of the array). When looping
through the array, the correct maximum index is "nr_mods-1",
not "nr_mods". If the array is full, using the latter will in
fact access beyond the end of the array.
This was done correctly in boot_module_find_by_kind() and
consider_modules() but incorrectly in discard_initial_modules()
and next_module().
Signed-off-by: Chris Brand
---
xen/arch/arm/setup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 06f8e54b1f04..5daa6db919ac 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -250,7 +250,7 @@ void __init discard_initial_modules(void)
struct bootmodules *mi = &bootinfo.modules;
int i;
-for ( i = 0; i <= mi->nr_mods; i++ )
+for ( i = 0; i < mi->nr_mods; i++ )
{
paddr_t s = mi->module[i].start;
paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
@@ -350,7 +350,7 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
paddr_t lowest = ~(paddr_t)0;
int i;
-for ( i = 0; i <= mi->nr_mods; i++ )
+for ( i = 0; i < mi->nr_mods; i++ )
{
paddr_t mod_s = mi->module[i].start;
paddr_t mod_e = mod_s + mi->module[i].size;
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen: arm: Avoid reading beyond the last module
nr_mods is set in add_boot_module() to the number of module
array elements used. This function also ensures that nr_mods
never exceeds MAX_MODULES (the size of the array). When looping
through the array, the correct maximum index is "nr_mods-1",
not "nr_mods". If the array is full, using the latter will in
fact access beyond the end of the array.
This was done correctly in boot_module_find_by_kind() and
consider_modules() but incorrectly in discard_initial_modules()
and next_module().
Signed-off-by: Chris Brand
---
xen/arch/arm/setup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 06f8e54b1f04..5daa6db919ac 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -250,7 +250,7 @@ void __init discard_initial_modules(void)
struct bootmodules *mi = &bootinfo.modules;
int i;
-for ( i = 0; i <= mi->nr_mods; i++ )
+for ( i = 0; i < mi->nr_mods; i++ )
{
paddr_t s = mi->module[i].start;
paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
@@ -350,7 +350,7 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
paddr_t lowest = ~(paddr_t)0;
int i;
-for ( i = 0; i <= mi->nr_mods; i++ )
+for ( i = 0; i < mi->nr_mods; i++ )
{
paddr_t mod_s = mi->module[i].start;
paddr_t mod_e = mod_s + mi->module[i].size;
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?
>>As said by David, Xen will parse "reg" using the #address-cells and >>#size-cells of the parent. So it's normal to see 2GB. Does the same device >>tree reports 3GB on Linux? I suspect no. > >I'm not in the office today, so I can't test this, but I believe the answer is >"yes". But in fact the answer is indeed "no". The log that I found was from a different device tree. I still think the code shouldn't be reading before the start of the two arrays, but it does function correctly. Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?
>As said by David, Xen will parse "reg" using the #address-cells and >#size-cells of the parent. So it's normal to see 2GB. Does the same device >tree reports 3GB on Linux? I suspect no. I'm not in the office today, so I can't test this, but I believe the answer is "yes". I have this log from an earlier run where I ran Linux without Xen. I *believe* used that same DT: [0.00] Memory: 208924K/3145728K available (5557K kernel code, 280K rwdata, 1740K rodata, 3892K init, 213K bss, 36836K reserved, 2899968K cma-reserved, 0K highmem) Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?
I've been trying to figure out why Xen only reports 2GB on my ARM platform that
actually has 3GB, and I think I've found a bug, but I'm not familiar enough
with the Xen code to fix it.
The relevant parts of my dts are:
/dts-v1/;
/ {
model = "Broadcom STB (7445d0)";
compatible = "brcm,bcm7445d0", "brcm,brcmstb";
#address-cells = <0x2>;
#size-cells = <0x2>;
interrupt-parent = <0x1>;
memory {
#address-cells = <0x1>;
#size-cells = <0x1>;
device_type = "memory";
reg = <0x0 0x0 0x0 0x4000 0x0 0x4000 0x0 0x4000>;
region@1000 {
contiguous-region;
reg = <0x1000 0x1f80>;
linux,phandle = <0x2>;
phandle = <0x2>;
};
region@3000 {
contiguous-region;
reg = <0x3000 0x1000>;
linux,phandle = <0x3>;
phandle = <0x3>;
};
region@4000 {
contiguous-region;
reg = <0x4000 0x4000>;
linux,phandle = <0x4>;
phandle = <0x4>;
};
region@8000 {
contiguous-region;
reg = <0x8000 0x4000>;
linux,phandle = <0x5>;
phandle = <0x5>;
};
};
As you can see, it specifies 0 + 1GB + 1GB + 1GB. When I run "xl info" in Dom0,
though, it reports "Total 2048".
Digging into the code, I found that in bootinfo.mem.nr_banks is 2 rather than
the expected 3 (or 4?). That turned out to be because in process_memory_node(),
address_cells and size_cells were both 2 and so the prop_len of 32 was
resulting in "banks" being set to 2.
Looking at device_tree_for_each_node(), it looks like something is wrong
because it contains this loop:
for ( node = 0, depth = 0;
node >=0 && depth >= 0;
node = fdt_next_node(fdt, node, &depth) )
{
[...]
ret = func(fdt, node, name, depth,
address_cells[depth-1], size_cells[depth-1], data);
which looks like it will read before the start of the array for the first node,
when depth=0. My first instinct was to remove those two "-1"s, but the
resulting code didn't run, so I figured I'd try to enlist some help :) Of
course it's possible that my problem is unrelated to this, but reading before
the start of the array definitely seems like a bug that should be fixed
(although in practice those values are never used for node 0). Looking through
the history, it seems to have been like that since the function was first
introduced
(http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=9cf4a9a467171c8a2c3d97c2bfadb1bc8b15a3d6).
I'm happy to test out any patches.
Chris
(Not subscribed to the list)
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node
>Can I get your Tested-by on this patch? Tested-by: Chris Brand Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node
Hi Julien, The patch does work exactly as advertised. When I used dtc to convert CONFIG_DTB_FILE from dtb to dts, I could see that it didn't in fact have a timer clock-frequency node. After re-creating the dtb and rebuilding Xen, "ls /proc/device-tree/timer/" shows a clock-frequency file. When I then fire up DomU and do the same command, it too has a clock-frequency node. Thanks, Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node
Hi Julien, >> A simple "export CONFIG_DTB_FILE=...". > >Are you using an absolute path? Absolute path, yes. >> So your patch isn't designed to help in this case (CNTFRQ set on core 0 >> only, no clock-frequency property in timer node of DT). > >Well, my patch is designed to propagate the "clock-frequency" property in DOMU >via the toolstack. The propagation for Dom0 is already present in Xen since >last year [1]. Understood. In my case, the DomU kernel dies during boot if it happens to be scheduled on a core other than core 0 (it comes up fine if it happens to run on core 0). Core 0 has CNTFRQ set, but the other cores don't. We will fix this for real by writing to CNTFRQ on the other cores in the PSCI code in the SMC. >Does your bootloader create Xen nodes (multiboot, chosen, ...) or the device >tree provided by the bootloader contain such nodes? If not, then you are using >an appended DT (via CONFIG_DTB_FILE) to Xen. It doesn't do that, no. In that case the mystery is why Xen apparently doesn't see the clock-frequency property. I'll keep investigating. >If you didn't yet do it, I would try to clean Xen repository and try to >rebuild Xen to see if the build system was using a stall DTB by mistake. I have, but I'll do it again. Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node
Hi Julien, >My test was limited as I don't have a platform where CNTFRQ/CNTFRQ_EL0 is not >valid. I may have done a mistake in the code. Understood. That's why I thought it would be worthwhile posting my results :-) >> What I see is that in preinit_xen_time(), the call to dt_property_read_u32() >> returns zero. When I built Xen, I set CONFIG_DTB_FILE, and looking at the >> corresponding dts file it has a timer node with a clock-frequency property. >> I know that our bootloader also creates a DTB, though, and it looks like >> that one does *not* have a clock-frequency property in the timer node, so I >> guess Xen ends up using that one somehow. CNTFRQ on core 0 (only) is also >> set to the correct frequency, so I end up with the correct frequency in my >> Dom0 kernel anyway. > >dt_property_read_u32 returns 0 when it cannot find a property or because the >size of the value is not valid. > >The device tree provided via CONFIG_DTB_FILE will always take precedence to >the one pass by the bootloader. > >How do you set CONFIG_DTB_FILE? A simple "export CONFIG_DTB_FILE=...". >I would also look to see if by any chance the wrong device tree is set via >CONFIG_DTB_FILE. I did double-check before I posted, and everything looks right to me. I certainly could have missed something. It does look like it's *somehow* getting the DT generated by the bootloader. >You can check what is the device tree used by dumping it from DOM0. >Thought, it may be slightly different (some nodes are rewritten). You can dump >it using dtc -I fs /proc/device-tree -O dts When I do that (at a shell prompt in Dom0), the timer node does not have a clock-frequency property. So your patch isn't designed to help in this case (CNTFRQ set on core 0 only, no clock-frequency property in timer node of DT). Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node
Hi Julien, >When the property "clock-frequency" is present in the DT timer node, it means >that the bootloader/firmware didn't correctly configured the CNTFRQ/CNTFRQ_EL0 on each processor. I did try this out, and it didn't affect my results. I don't understand why, though :-) What I see is that in preinit_xen_time(), the call to dt_property_read_u32() returns zero. When I built Xen, I set CONFIG_DTB_FILE, and looking at the corresponding dts file it has a timer node with a clock-frequency property. I know that our bootloader also creates a DTB, though, and it looks like that one does *not* have a clock-frequency property in the timer node, so I guess Xen ends up using that one somehow. CNTFRQ on core 0 (only) is also set to the correct frequency, so I end up with the correct frequency in my Dom0 kernel anyway. I'm guessing this is me not understanding what's happening in my system rather than anything wrong with the patch. Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node
Hi Julien, >When the property "clock-frequency" is present in the DT timer node, it means >that the bootloader/firmware didn't correctly configured the >CNTFRQ/CNTFRQ_EL0 on each processor. I will test this patch, but it doesn't apply cleanly to the version of Xen I'm currently using, so I need to update that first. I also looked at whether it would be possible to set the CNTFRQ register in the other cores when they come up. Eventually, I think we should do this in the (platform-specific) PSCI code. There doesn't seem to be a suitable hook in the platform-specific Xen code - it looks like all the code there related to bringing up secondary cores runs on the primary. Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
