| Message ID | 20191129013504.145455-2-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for RMA size calculation | expand |
On 29/11/2019 02:35, David Gibson wrote: > This function calculates the maximum size of the RMA as implied by the > host's page size of structure of the VRMA (there are a number of other > constraints on the RMA size which will supersede this one in many > circumstances). > > The current interface takes the current RMA size estimate, and clamps it > to the VRMA derived size. The only current caller passes in an arguably > wrong value (it will match the current RMA estimate in some but not all > cases). > > We want to fix that, but for now just keep concerns separated by having the > KVM helper function just return the VRMA derived limit, and let the caller > combine it with other constraints. We call the new function > kvmppc_vrma_limit() to more clearly indicate its limited responsibility. > > The helper should only ever be called in the KVM enabled case, so replace > its !CONFIG_KVM stub with an assert() rather than a dummy value. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Cedric Le Goater <clg@fr.ibm.com> > --- > hw/ppc/spapr.c | 5 +++-- > target/ppc/kvm.c | 5 ++--- > target/ppc/kvm_ppc.h | 7 +++---- > 3 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index d9c9a2bcee..069bd04a8d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1635,8 +1635,9 @@ void spapr_setup_hpt_and_vrma(SpaprMachineState *spapr) > spapr_reallocate_hpt(spapr, hpt_shift, &error_fatal); > > if (spapr->vrma_adjust) { > - spapr->rma_size = kvmppc_rma_size(spapr_node0_size(MACHINE(spapr)), > - spapr->htab_shift); > + hwaddr vrma_limit = kvmppc_vrma_limit(spapr->htab_shift); > + > + spapr->rma_size = MIN(spapr_node0_size(MACHINE(spapr)), vrma_limit); > } > } > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index c77f9848ec..09b3bd6443 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -2101,7 +2101,7 @@ void kvmppc_hint_smt_possible(Error **errp) > > > #ifdef TARGET_PPC64 > -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) > +uint64_t kvmppc_vrma_limit(unsigned int hash_shift) > { > struct kvm_ppc_smmu_info info; > long rampagesize, best_page_shift; > @@ -2128,8 +2128,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) > } > } > > - return MIN(current_size, > - 1ULL << (best_page_shift + hash_shift - 7)); > + return 1ULL << (best_page_shift + hash_shift - 7)); > } > #endif > > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h > index 98bd7d5da6..4f0eec4c1b 100644 > --- a/target/ppc/kvm_ppc.h > +++ b/target/ppc/kvm_ppc.h > @@ -45,7 +45,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t page_shift, > int *pfd, bool need_vfio); > int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size); > int kvmppc_reset_htab(int shift_hint); > -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift); > +uint64_t kvmppc_vrma_limit(unsigned int hash_shift); > bool kvmppc_has_cap_spapr_vfio(void); > #endif /* !CONFIG_USER_ONLY */ > bool kvmppc_has_cap_epr(void); > @@ -241,10 +241,9 @@ static inline int kvmppc_reset_htab(int shift_hint) > return 0; > } > > -static inline uint64_t kvmppc_rma_size(uint64_t current_size, > - unsigned int hash_shift) > +static inline uint64_t kvmppc_vrma_limit(unsigned int hash_shift) > { > - return ram_size; > + g_assert_not_reached(); > } > > static inline bool kvmppc_hpt_needs_host_contiguous_pages(void) >
On Fri, 29 Nov 2019 12:35:01 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > This function calculates the maximum size of the RMA as implied by the > host's page size of structure of the VRMA (there are a number of other > constraints on the RMA size which will supersede this one in many > circumstances). > > The current interface takes the current RMA size estimate, and clamps it > to the VRMA derived size. The only current caller passes in an arguably > wrong value (it will match the current RMA estimate in some but not all > cases). > > We want to fix that, but for now just keep concerns separated by having the > KVM helper function just return the VRMA derived limit, and let the caller > combine it with other constraints. We call the new function > kvmppc_vrma_limit() to more clearly indicate its limited responsibility. > > The helper should only ever be called in the KVM enabled case, so replace > its !CONFIG_KVM stub with an assert() rather than a dummy value. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > hw/ppc/spapr.c | 5 +++-- > target/ppc/kvm.c | 5 ++--- > target/ppc/kvm_ppc.h | 7 +++---- > 3 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index d9c9a2bcee..069bd04a8d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1635,8 +1635,9 @@ void spapr_setup_hpt_and_vrma(SpaprMachineState *spapr) > spapr_reallocate_hpt(spapr, hpt_shift, &error_fatal); > > if (spapr->vrma_adjust) { > - spapr->rma_size = kvmppc_rma_size(spapr_node0_size(MACHINE(spapr)), > - spapr->htab_shift); > + hwaddr vrma_limit = kvmppc_vrma_limit(spapr->htab_shift); > + > + spapr->rma_size = MIN(spapr_node0_size(MACHINE(spapr)), vrma_limit); > } > } > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index c77f9848ec..09b3bd6443 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -2101,7 +2101,7 @@ void kvmppc_hint_smt_possible(Error **errp) > > > #ifdef TARGET_PPC64 > -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) > +uint64_t kvmppc_vrma_limit(unsigned int hash_shift) > { > struct kvm_ppc_smmu_info info; > long rampagesize, best_page_shift; > @@ -2128,8 +2128,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) > } > } > > - return MIN(current_size, > - 1ULL << (best_page_shift + hash_shift - 7)); > + return 1ULL << (best_page_shift + hash_shift - 7)); > } > #endif > > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h > index 98bd7d5da6..4f0eec4c1b 100644 > --- a/target/ppc/kvm_ppc.h > +++ b/target/ppc/kvm_ppc.h > @@ -45,7 +45,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t page_shift, > int *pfd, bool need_vfio); > int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size); > int kvmppc_reset_htab(int shift_hint); > -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift); > +uint64_t kvmppc_vrma_limit(unsigned int hash_shift); > bool kvmppc_has_cap_spapr_vfio(void); > #endif /* !CONFIG_USER_ONLY */ > bool kvmppc_has_cap_epr(void); > @@ -241,10 +241,9 @@ static inline int kvmppc_reset_htab(int shift_hint) > return 0; > } > > -static inline uint64_t kvmppc_rma_size(uint64_t current_size, > - unsigned int hash_shift) > +static inline uint64_t kvmppc_vrma_limit(unsigned int hash_shift) > { > - return ram_size; > + g_assert_not_reached(); > } > > static inline bool kvmppc_hpt_needs_host_contiguous_pages(void)
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index c77f9848ec..09b3bd6443 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -2101,7 +2101,7 @@ void kvmppc_hint_smt_possible(Error **errp) > > > #ifdef TARGET_PPC64 > -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) > +uint64_t kvmppc_vrma_limit(unsigned int hash_shift) > { > struct kvm_ppc_smmu_info info; > long rampagesize, best_page_shift; > @@ -2128,8 +2128,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) > } > } > > - return MIN(current_size, > - 1ULL << (best_page_shift + hash_shift - 7)); > + return 1ULL << (best_page_shift + hash_shift - 7)); The closing ')' should be removed. C.
On 29/11/2019 12:35, David Gibson wrote: > This function calculates the maximum size of the RMA as implied by the > host's page size of structure of the VRMA (there are a number of other > constraints on the RMA size which will supersede this one in many > circumstances). > > The current interface takes the current RMA size estimate, and clamps it > to the VRMA derived size. The only current caller passes in an arguably > wrong value (it will match the current RMA estimate in some but not all > cases). > > We want to fix that, but for now just keep concerns separated by having the > KVM helper function just return the VRMA derived limit, and let the caller > combine it with other constraints. We call the new function > kvmppc_vrma_limit() to more clearly indicate its limited responsibility. > > The helper should only ever be called in the KVM enabled case, so replace > its !CONFIG_KVM stub with an assert() rather than a dummy value. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Besides not compiling, otherwise looks good. Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > hw/ppc/spapr.c | 5 +++-- > target/ppc/kvm.c | 5 ++--- > target/ppc/kvm_ppc.h | 7 +++---- > 3 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index d9c9a2bcee..069bd04a8d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1635,8 +1635,9 @@ void spapr_setup_hpt_and_vrma(SpaprMachineState *spapr) > spapr_reallocate_hpt(spapr, hpt_shift, &error_fatal); > > if (spapr->vrma_adjust) { > - spapr->rma_size = kvmppc_rma_size(spapr_node0_size(MACHINE(spapr)), > - spapr->htab_shift); > + hwaddr vrma_limit = kvmppc_vrma_limit(spapr->htab_shift); > + > + spapr->rma_size = MIN(spapr_node0_size(MACHINE(spapr)), vrma_limit); > } > } > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index c77f9848ec..09b3bd6443 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -2101,7 +2101,7 @@ void kvmppc_hint_smt_possible(Error **errp) > > > #ifdef TARGET_PPC64 > -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) > +uint64_t kvmppc_vrma_limit(unsigned int hash_shift) > { > struct kvm_ppc_smmu_info info; > long rampagesize, best_page_shift; > @@ -2128,8 +2128,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) > } > } > > - return MIN(current_size, > - 1ULL << (best_page_shift + hash_shift - 7)); > + return 1ULL << (best_page_shift + hash_shift - 7)); > } > #endif > > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h > index 98bd7d5da6..4f0eec4c1b 100644 > --- a/target/ppc/kvm_ppc.h > +++ b/target/ppc/kvm_ppc.h > @@ -45,7 +45,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t page_shift, > int *pfd, bool need_vfio); > int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size); > int kvmppc_reset_htab(int shift_hint); > -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift); > +uint64_t kvmppc_vrma_limit(unsigned int hash_shift); > bool kvmppc_has_cap_spapr_vfio(void); > #endif /* !CONFIG_USER_ONLY */ > bool kvmppc_has_cap_epr(void); > @@ -241,10 +241,9 @@ static inline int kvmppc_reset_htab(int shift_hint) > return 0; > } > > -static inline uint64_t kvmppc_rma_size(uint64_t current_size, > - unsigned int hash_shift) > +static inline uint64_t kvmppc_vrma_limit(unsigned int hash_shift) > { > - return ram_size; > + g_assert_not_reached(); > } > > static inline bool kvmppc_hpt_needs_host_contiguous_pages(void) >
On Mon, Dec 02, 2019 at 07:18:52PM +0100, Cédric Le Goater wrote: > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > index c77f9848ec..09b3bd6443 100644 > > --- a/target/ppc/kvm.c > > +++ b/target/ppc/kvm.c > > @@ -2101,7 +2101,7 @@ void kvmppc_hint_smt_possible(Error **errp) > > > > > > #ifdef TARGET_PPC64 > > -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) > > +uint64_t kvmppc_vrma_limit(unsigned int hash_shift) > > { > > struct kvm_ppc_smmu_info info; > > long rampagesize, best_page_shift; > > @@ -2128,8 +2128,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) > > } > > } > > > > - return MIN(current_size, > > - 1ULL << (best_page_shift + hash_shift - 7)); > > + return 1ULL << (best_page_shift + hash_shift - 7)); > > The closing ')' should be removed. Oops, that's embarrassing.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index d9c9a2bcee..069bd04a8d 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1635,8 +1635,9 @@ void spapr_setup_hpt_and_vrma(SpaprMachineState *spapr) spapr_reallocate_hpt(spapr, hpt_shift, &error_fatal); if (spapr->vrma_adjust) { - spapr->rma_size = kvmppc_rma_size(spapr_node0_size(MACHINE(spapr)), - spapr->htab_shift); + hwaddr vrma_limit = kvmppc_vrma_limit(spapr->htab_shift); + + spapr->rma_size = MIN(spapr_node0_size(MACHINE(spapr)), vrma_limit); } } diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c index c77f9848ec..09b3bd6443 100644 --- a/target/ppc/kvm.c +++ b/target/ppc/kvm.c @@ -2101,7 +2101,7 @@ void kvmppc_hint_smt_possible(Error **errp) #ifdef TARGET_PPC64 -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) +uint64_t kvmppc_vrma_limit(unsigned int hash_shift) { struct kvm_ppc_smmu_info info; long rampagesize, best_page_shift; @@ -2128,8 +2128,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift) } } - return MIN(current_size, - 1ULL << (best_page_shift + hash_shift - 7)); + return 1ULL << (best_page_shift + hash_shift - 7)); } #endif diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h index 98bd7d5da6..4f0eec4c1b 100644 --- a/target/ppc/kvm_ppc.h +++ b/target/ppc/kvm_ppc.h @@ -45,7 +45,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t page_shift, int *pfd, bool need_vfio); int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size); int kvmppc_reset_htab(int shift_hint); -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift); +uint64_t kvmppc_vrma_limit(unsigned int hash_shift); bool kvmppc_has_cap_spapr_vfio(void); #endif /* !CONFIG_USER_ONLY */ bool kvmppc_has_cap_epr(void); @@ -241,10 +241,9 @@ static inline int kvmppc_reset_htab(int shift_hint) return 0; } -static inline uint64_t kvmppc_rma_size(uint64_t current_size, - unsigned int hash_shift) +static inline uint64_t kvmppc_vrma_limit(unsigned int hash_shift) { - return ram_size; + g_assert_not_reached(); } static inline bool kvmppc_hpt_needs_host_contiguous_pages(void)
This function calculates the maximum size of the RMA as implied by the host's page size of structure of the VRMA (there are a number of other constraints on the RMA size which will supersede this one in many circumstances). The current interface takes the current RMA size estimate, and clamps it to the VRMA derived size. The only current caller passes in an arguably wrong value (it will match the current RMA estimate in some but not all cases). We want to fix that, but for now just keep concerns separated by having the KVM helper function just return the VRMA derived limit, and let the caller combine it with other constraints. We call the new function kvmppc_vrma_limit() to more clearly indicate its limited responsibility. The helper should only ever be called in the KVM enabled case, so replace its !CONFIG_KVM stub with an assert() rather than a dummy value. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr.c | 5 +++-- target/ppc/kvm.c | 5 ++--- target/ppc/kvm_ppc.h | 7 +++---- 3 files changed, 8 insertions(+), 9 deletions(-)