* [Xen-devel] [PATCH v4 0/2] fix mask calculation in pdx_init_mask @ 2019-06-17 18:50 Stefano Stabellini 2019-06-17 18:50 ` [Xen-devel] [PATCH v4 1/2] xen: switch pdx_init_mask to return uint64_t Stefano Stabellini 2019-06-17 18:50 ` [Xen-devel] [PATCH v4 2/2] xen/arm: fix mask calculation in pdx_init_mask Stefano Stabellini 0 siblings, 2 replies; 8+ messages in thread From: Stefano Stabellini @ 2019-06-17 18:50 UTC (permalink / raw) To: xen-devel; +Cc: andrew.cooper3, julien.grall, sstabellini, JBeulich Hi all, This is an update on v3 of "fix mask calculation in pdx_init_mask", plus a cleanup patch. The following changes since commit 2ac48fd52d846a8c3949373aa0d776c6cb5452db: x86/x2APIC: tighten check in cluster mode IPI sending (2019-06-17 17:38:35 +0200) are available in the Git repository at: http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git for you to fetch changes up to cfba65902ec5f15df8b35b3ccf8bcdda695d5665: xen/arm: fix mask calculation in pdx_init_mask (2019-06-17 11:46:20 -0700) ---------------------------------------------------------------- Stefano Stabellini (2): xen: switch pdx_init_mask to return uint64_t xen/arm: fix mask calculation in pdx_init_mask xen/arch/arm/setup.c | 9 ++++++++- xen/arch/x86/srat.c | 2 +- xen/common/pdx.c | 9 +++++++-- xen/include/xen/pdx.h | 2 +- 4 files changed, 17 insertions(+), 5 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Xen-devel] [PATCH v4 1/2] xen: switch pdx_init_mask to return uint64_t 2019-06-17 18:50 [Xen-devel] [PATCH v4 0/2] fix mask calculation in pdx_init_mask Stefano Stabellini @ 2019-06-17 18:50 ` Stefano Stabellini 2019-06-18 10:23 ` Jan Beulich 2019-06-17 18:50 ` [Xen-devel] [PATCH v4 2/2] xen/arm: fix mask calculation in pdx_init_mask Stefano Stabellini 1 sibling, 1 reply; 8+ messages in thread From: Stefano Stabellini @ 2019-06-17 18:50 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, julien.grall, sstabellini, JBeulich, andrew.cooper3 Also change srat_region_mask to uint64_t as it is used to store the return value of pdx_init_mask. uint64_t is always greater or equal to u64. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> CC: JBeulich@suse.com CC: andrew.cooper3@citrix.com CC: julien.grall@arm.com --- xen/arch/arm/setup.c | 2 +- xen/arch/x86/srat.c | 2 +- xen/common/pdx.c | 2 +- xen/include/xen/pdx.h | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 2112715579..b03e7ac330 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -483,7 +483,7 @@ static void __init init_pdx(void) { paddr_t bank_start, bank_size, bank_end; - u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start); + uint64_t mask = pdx_init_mask(bootinfo.mem.bank[0].start); int bank; for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ ) diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c index 2d70b45909..47a4267220 100644 --- a/xen/arch/x86/srat.c +++ b/xen/arch/x86/srat.c @@ -401,7 +401,7 @@ static int __init nodes_cover_memory(void) void __init acpi_numa_arch_fixup(void) {} -static u64 __initdata srat_region_mask; +static uint64_t __initdata srat_region_mask; static int __init srat_parse_region(struct acpi_subtable_header *header, const unsigned long end) diff --git a/xen/common/pdx.c b/xen/common/pdx.c index bb7e437049..8356f03ce8 100644 --- a/xen/common/pdx.c +++ b/xen/common/pdx.c @@ -50,7 +50,7 @@ static u64 __init fill_mask(u64 mask) return mask; } -u64 __init pdx_init_mask(u64 base_addr) +uint64_t __init pdx_init_mask(uint64_t base_addr) { return fill_mask(base_addr - 1); } diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h index a151aac1a2..770fadc06c 100644 --- a/xen/include/xen/pdx.h +++ b/xen/include/xen/pdx.h @@ -13,7 +13,7 @@ extern unsigned long pfn_top_mask, ma_top_mask; (sizeof(*frame_table) & -sizeof(*frame_table))) extern unsigned long pdx_group_valid[]; -extern u64 pdx_init_mask(u64 base_addr); +extern uint64_t pdx_init_mask(u64 base_addr); extern u64 pdx_region_mask(u64 base, u64 len); extern void set_pdx_range(unsigned long smfn, unsigned long emfn); -- 2.17.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH v4 1/2] xen: switch pdx_init_mask to return uint64_t 2019-06-17 18:50 ` [Xen-devel] [PATCH v4 1/2] xen: switch pdx_init_mask to return uint64_t Stefano Stabellini @ 2019-06-18 10:23 ` Jan Beulich 2019-06-20 13:15 ` Julien Grall 0 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2019-06-18 10:23 UTC (permalink / raw) To: Stefano Stabellini Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, xen-devel >>> On 17.06.19 at 20:50, <sstabellini@kernel.org> wrote: > Also change srat_region_mask to uint64_t as it is used to store the > return value of pdx_init_mask. uint64_t is always greater or equal to > u64. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> Non-Arm bits Acked-by: Jan Beulich <jbeulich@suse.com> but could you make the title sound less like it's an actual change to the function return type? Also it's not just its return type you change. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH v4 1/2] xen: switch pdx_init_mask to return uint64_t 2019-06-18 10:23 ` Jan Beulich @ 2019-06-20 13:15 ` Julien Grall 2019-06-21 6:17 ` Jan Beulich 0 siblings, 1 reply; 8+ messages in thread From: Julien Grall @ 2019-06-20 13:15 UTC (permalink / raw) To: Jan Beulich, Stefano Stabellini Cc: Andrew Cooper, Stefano Stabellini, xen-devel Hi, On 6/18/19 11:23 AM, Jan Beulich wrote: >>>> On 17.06.19 at 20:50, <sstabellini@kernel.org> wrote: >> Also change srat_region_mask to uint64_t as it is used to store the >> return value of pdx_init_mask. uint64_t is always greater or equal to >> u64. I am a bit confused with the last sentence. In which instance, uint64_t would be greater to u64? Aren't they meant to both encode a 64-bit value? >> >> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > Non-Arm bits > Acked-by: Jan Beulich <jbeulich@suse.com> > but could you make the title sound less like it's an actual change > to the function return type? Also it's not just its return type > you change. To move this series forward, how about the following commit message: "xen: Replace u64 with uint64_t in pdx_init_mask() and callers Xen is phasing out the use of u64 in favor of uint64_t. Therefore, the instance of u64 in the pdx_init_mask() (and the callers) are now replaced with uint64_t. Take the opportunity to also modify srat_region_mask as this is used to store the result of pdx_init_mask(). " Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH v4 1/2] xen: switch pdx_init_mask to return uint64_t 2019-06-20 13:15 ` Julien Grall @ 2019-06-21 6:17 ` Jan Beulich 0 siblings, 0 replies; 8+ messages in thread From: Jan Beulich @ 2019-06-21 6:17 UTC (permalink / raw) To: Julien Grall, Stefano Stabellini Cc: Andrew Cooper, Stefano Stabellini, xen-devel >>> On 20.06.19 at 15:15, <julien.grall@arm.com> wrote: > On 6/18/19 11:23 AM, Jan Beulich wrote: >>>>> On 17.06.19 at 20:50, <sstabellini@kernel.org> wrote: >>> Also change srat_region_mask to uint64_t as it is used to store the >>> return value of pdx_init_mask. uint64_t is always greater or equal to >>> u64. > > I am a bit confused with the last sentence. In which instance, uint64_t > would be greater to u64? Aren't they meant to both encode a 64-bit value? Oh, indeed - somehow I didn't even notice this. If anything it's the other way around actually, because uint64_t is mandated to be exactly 64 bits wide, whereas there's no strict specification for u64 afaia, but I very much assume the intentions there have been the same. The proposed title and text replacements look fine to me. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Xen-devel] [PATCH v4 2/2] xen/arm: fix mask calculation in pdx_init_mask 2019-06-17 18:50 [Xen-devel] [PATCH v4 0/2] fix mask calculation in pdx_init_mask Stefano Stabellini 2019-06-17 18:50 ` [Xen-devel] [PATCH v4 1/2] xen: switch pdx_init_mask to return uint64_t Stefano Stabellini @ 2019-06-17 18:50 ` Stefano Stabellini 2019-06-20 13:20 ` Julien Grall 1 sibling, 1 reply; 8+ messages in thread From: Stefano Stabellini @ 2019-06-17 18:50 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, sstabellini, wei.liu2, konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson, tim, JBeulich The mask calculation in pdx_init_mask is wrong when the first bank starts at address 0x0. The reason is that pdx_init_mask will do '0 - 1' causing an underflow. As a result, the mask becomes 0xffffffffffffffff which is the biggest possible mask and ends up causing a significant memory waste in the frametable size computation. For instance, on platforms that have a low memory bank starting at 0x0 and a high memory bank, the frametable will end up covering all the holes in between. The purpose of the mask is to be passed as a parameter to pfn_pdx_hole_setup, which based on the mask parameter calculates pfn_pdx_hole_shift, pfn_pdx_bottom_mask, etc. which are actually the important masks for frametable initialization later on. pfn_pdx_hole_setup never compresses addresses below MAX_ORDER bits (1GB on ARM). Thus, it is safe to initialize mask passing 1ULL << (MAX_ORDER + PAGE_SHIFT) as start address to pdx_init_mask. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> CC: JBeulich@suse.com CC: andrew.cooper3@citrix.com CC: George.Dunlap@eu.citrix.com CC: ian.jackson@eu.citrix.com CC: konrad.wilk@oracle.com CC: tim@xen.org CC: wei.liu2@citrix.com --- Changes in v4: - use uint64_t - single line comment code style Changes in v3: - improve in-code comments Unchanged in v3: - (u64)1 Changes in v2: - update commit message - add in-code comments regarding update sites - improve in-code comments - move the mask initialization changes to pdx_init_mask --- xen/arch/arm/setup.c | 9 ++++++++- xen/common/pdx.c | 7 ++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index b03e7ac330..b0af90e5bf 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -483,7 +483,14 @@ static void __init init_pdx(void) { paddr_t bank_start, bank_size, bank_end; - uint64_t mask = pdx_init_mask(bootinfo.mem.bank[0].start); + /* + * Arm does not have any restrictions on the bits to compress. Pass 0 to + * let the common code further restrict the mask. + * + * If the logic changes in pfn_pdx_hole_setup we might have to + * update this function too. + */ + uint64_t mask = pdx_init_mask(0x0); int bank; for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ ) diff --git a/xen/common/pdx.c b/xen/common/pdx.c index 8356f03ce8..9990b94f73 100644 --- a/xen/common/pdx.c +++ b/xen/common/pdx.c @@ -50,9 +50,11 @@ static u64 __init fill_mask(u64 mask) return mask; } +/* We don't compress the first MAX_ORDER bit of the addresses. */ uint64_t __init pdx_init_mask(uint64_t base_addr) { - return fill_mask(base_addr - 1); + return fill_mask(max(base_addr, + (uint64_t)1 << (MAX_ORDER + PAGE_SHIFT)) - 1); } u64 __init pdx_region_mask(u64 base, u64 len) @@ -80,6 +82,9 @@ void __init pfn_pdx_hole_setup(unsigned long mask) * This guarantees that page-pointer arithmetic remains valid within * contiguous aligned ranges of 2^MAX_ORDER pages. Among others, our * buddy allocator relies on this assumption. + * + * If the logic changes here, we might have to update the ARM specific + * init_pdx too. */ for ( j = MAX_ORDER-1; ; ) { -- 2.17.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] xen/arm: fix mask calculation in pdx_init_mask 2019-06-17 18:50 ` [Xen-devel] [PATCH v4 2/2] xen/arm: fix mask calculation in pdx_init_mask Stefano Stabellini @ 2019-06-20 13:20 ` Julien Grall 2019-06-21 6:21 ` Jan Beulich 0 siblings, 1 reply; 8+ messages in thread From: Julien Grall @ 2019-06-20 13:20 UTC (permalink / raw) To: Stefano Stabellini, xen-devel, andrew.cooper3, JBeulich Cc: Stefano Stabellini, wei.liu2, konrad.wilk, George.Dunlap, ian.jackson, tim Hi, On 6/17/19 7:50 PM, Stefano Stabellini wrote: > The mask calculation in pdx_init_mask is wrong when the first bank > starts at address 0x0. The reason is that pdx_init_mask will do '0 - 1' > causing an underflow. As a result, the mask becomes 0xffffffffffffffff > which is the biggest possible mask and ends up causing a significant > memory waste in the frametable size computation. > > For instance, on platforms that have a low memory bank starting at 0x0 > and a high memory bank, the frametable will end up covering all the > holes in between. > > The purpose of the mask is to be passed as a parameter to > pfn_pdx_hole_setup, which based on the mask parameter calculates > pfn_pdx_hole_shift, pfn_pdx_bottom_mask, etc. which are actually the > important masks for frametable initialization later on. > > pfn_pdx_hole_setup never compresses addresses below MAX_ORDER bits (1GB > on ARM). Thus, it is safe to initialize mask passing 1ULL << (MAX_ORDER > + PAGE_SHIFT) as start address to pdx_init_mask. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> Reviewed-by: Julien Grall <julien.grall@arm.com> Ideally, I would like an ack from Andrew or Jan. > CC: JBeulich@suse.com > CC: andrew.cooper3@citrix.com > CC: George.Dunlap@eu.citrix.com > CC: ian.jackson@eu.citrix.com > CC: konrad.wilk@oracle.com > CC: tim@xen.org > CC: wei.liu2@citrix.com > --- > > Changes in v4: > - use uint64_t > - single line comment code style > > Changes in v3: > - improve in-code comments > > Unchanged in v3: > - (u64)1 > > Changes in v2: > - update commit message > - add in-code comments regarding update sites > - improve in-code comments > - move the mask initialization changes to pdx_init_mask > --- > xen/arch/arm/setup.c | 9 ++++++++- > xen/common/pdx.c | 7 ++++++- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index b03e7ac330..b0af90e5bf 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -483,7 +483,14 @@ static void __init init_pdx(void) > { > paddr_t bank_start, bank_size, bank_end; > > - uint64_t mask = pdx_init_mask(bootinfo.mem.bank[0].start); > + /* > + * Arm does not have any restrictions on the bits to compress. Pass 0 to > + * let the common code further restrict the mask. > + * > + * If the logic changes in pfn_pdx_hole_setup we might have to > + * update this function too. > + */ > + uint64_t mask = pdx_init_mask(0x0); > int bank; > > for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ ) > diff --git a/xen/common/pdx.c b/xen/common/pdx.c > index 8356f03ce8..9990b94f73 100644 > --- a/xen/common/pdx.c > +++ b/xen/common/pdx.c > @@ -50,9 +50,11 @@ static u64 __init fill_mask(u64 mask) > return mask; > } > > +/* We don't compress the first MAX_ORDER bit of the addresses. */ > uint64_t __init pdx_init_mask(uint64_t base_addr) > { > - return fill_mask(base_addr - 1); > + return fill_mask(max(base_addr, > + (uint64_t)1 << (MAX_ORDER + PAGE_SHIFT)) - 1); > } > > u64 __init pdx_region_mask(u64 base, u64 len) > @@ -80,6 +82,9 @@ void __init pfn_pdx_hole_setup(unsigned long mask) > * This guarantees that page-pointer arithmetic remains valid within > * contiguous aligned ranges of 2^MAX_ORDER pages. Among others, our > * buddy allocator relies on this assumption. > + * > + * If the logic changes here, we might have to update the ARM specific > + * init_pdx too. > */ > for ( j = MAX_ORDER-1; ; ) > { > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] xen/arm: fix mask calculation in pdx_init_mask 2019-06-20 13:20 ` Julien Grall @ 2019-06-21 6:21 ` Jan Beulich 0 siblings, 0 replies; 8+ messages in thread From: Jan Beulich @ 2019-06-21 6:21 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Stefano Stabellini, wei.liu2, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel >>> On 20.06.19 at 15:20, <julien.grall@arm.com> wrote: > On 6/17/19 7:50 PM, Stefano Stabellini wrote: >> The mask calculation in pdx_init_mask is wrong when the first bank >> starts at address 0x0. The reason is that pdx_init_mask will do '0 - 1' >> causing an underflow. As a result, the mask becomes 0xffffffffffffffff >> which is the biggest possible mask and ends up causing a significant >> memory waste in the frametable size computation. >> >> For instance, on platforms that have a low memory bank starting at 0x0 >> and a high memory bank, the frametable will end up covering all the >> holes in between. >> >> The purpose of the mask is to be passed as a parameter to >> pfn_pdx_hole_setup, which based on the mask parameter calculates >> pfn_pdx_hole_shift, pfn_pdx_bottom_mask, etc. which are actually the >> important masks for frametable initialization later on. >> >> pfn_pdx_hole_setup never compresses addresses below MAX_ORDER bits (1GB >> on ARM). Thus, it is safe to initialize mask passing 1ULL << (MAX_ORDER >> + PAGE_SHIFT) as start address to pdx_init_mask. >> >> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > Reviewed-by: Julien Grall <julien.grall@arm.com> > > Ideally, I would like an ack from Andrew or Jan. Acked-by: Jan Beulich <jbeulich@suse.com> with one more minor remark: >> --- a/xen/common/pdx.c >> +++ b/xen/common/pdx.c >> @@ -50,9 +50,11 @@ static u64 __init fill_mask(u64 mask) >> return mask; >> } >> >> +/* We don't compress the first MAX_ORDER bit of the addresses. */ >> uint64_t __init pdx_init_mask(uint64_t base_addr) I think the comment would benefit from "want to" getting inserted. And as a nit, it should be "bits", not "bit", plus perhaps "low" instead of "first". Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-06-21 6:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-17 18:50 [Xen-devel] [PATCH v4 0/2] fix mask calculation in pdx_init_mask Stefano Stabellini 2019-06-17 18:50 ` [Xen-devel] [PATCH v4 1/2] xen: switch pdx_init_mask to return uint64_t Stefano Stabellini 2019-06-18 10:23 ` Jan Beulich 2019-06-20 13:15 ` Julien Grall 2019-06-21 6:17 ` Jan Beulich 2019-06-17 18:50 ` [Xen-devel] [PATCH v4 2/2] xen/arm: fix mask calculation in pdx_init_mask Stefano Stabellini 2019-06-20 13:20 ` Julien Grall 2019-06-21 6:21 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).