The formula used to compute the address of the HPT allocated by QEMU is
open-coded in several places. This patch moves the magic to a dedicated
helper. While here, we also patch the callers to only pass the address
to KVM if we indeed have a userland HPT (ie, KVM PR).
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr.c | 9 +++++++++
hw/ppc/spapr_cpu_core.c | 12 +++++++-----
hw/ppc/spapr_hcall.c | 14 ++++++++------
include/hw/ppc/spapr.h | 1 +
4 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index caffa1276328..bf24c26b756d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex,
}
}
+target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)
+{
+ if (!spapr->htab) {
+ return 0;
+ }
+
+ return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);
+}
+
int spapr_hpt_shift_for_ramsize(uint64_t ramsize)
{
int shift;
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 85037ef71e27..581eb4d92de9 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque)
* HPT
*/
if (kvm_enabled()) {
- env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab
- | (spapr->htab_shift - 18);
- if (kvmppc_put_books_sregs(cpu) < 0) {
- error_report("Unable to update SDR1 in KVM");
- exit(1);
+ target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
+ if (sdr1) {
+ env->spr[SPR_SDR1] = sdr1;
+ if (kvmppc_put_books_sregs(cpu) < 0) {
+ error_report("Unable to update SDR1 in KVM");
+ exit(1);
+ }
}
}
}
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 6ab8c188f381..06059b44ab40 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,
if (kvm_enabled()) {
/* For KVM PR, update the HPT pointer */
- target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
- | (spapr->htab_shift - 18);
- kvmppc_update_sdr1(sdr1);
+ target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
+ if (sdr1) {
+ kvmppc_update_sdr1(sdr1);
+ }
}
pending->hpt = NULL; /* so it's not free()d */
@@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
spapr_reallocate_hpt(spapr, maxshift, &error_fatal);
if (kvm_enabled()) {
/* For KVM PR, update the HPT pointer */
- target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
- | (spapr->htab_shift - 18);
- kvmppc_update_sdr1(sdr1);
+ target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
+ if (sdr1) {
+ kvmppc_update_sdr1(sdr1);
+ }
}
}
}
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index c1b365f56431..a1f5edc15018 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -709,4 +709,5 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
int spapr_vcpu_id(PowerPCCPU *cpu);
PowerPCCPU *spapr_find_cpu(int vcpu_id);
+target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr);
#endif /* HW_SPAPR_H */
On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote:
> The formula used to compute the address of the HPT allocated by QEMU is
> open-coded in several places. This patch moves the magic to a dedicated
> helper. While here, we also patch the callers to only pass the address
> to KVM if we indeed have a userland HPT (ie, KVM PR).
The helper function seems reasonable, though I'm not sure about the
name (a. it's not just a pointer, since it includes the encoded size
and b. the name doesn't indicate this is basically KVM PR specific).
THe "only pass the address to KVM if we indeed have a userland HPT
(ie, KVM PR)" bit doesn't really work. You're doing it by testing for
(sdr1 != 0), but that can only be true if the HPT is minimum size,
which doesn't have much to do with anything meaningful.
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/ppc/spapr.c | 9 +++++++++
> hw/ppc/spapr_cpu_core.c | 12 +++++++-----
> hw/ppc/spapr_hcall.c | 14 ++++++++------
> include/hw/ppc/spapr.h | 1 +
> 4 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index caffa1276328..bf24c26b756d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex,
> }
> }
>
> +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)
> +{
> + if (!spapr->htab) {
> + return 0;
> + }
> +
> + return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);
> +}
> +
> int spapr_hpt_shift_for_ramsize(uint64_t ramsize)
> {
> int shift;
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 85037ef71e27..581eb4d92de9 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque)
> * HPT
> */
> if (kvm_enabled()) {
> - env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab
> - | (spapr->htab_shift - 18);
> - if (kvmppc_put_books_sregs(cpu) < 0) {
> - error_report("Unable to update SDR1 in KVM");
> - exit(1);
> + target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> + if (sdr1) {
> + env->spr[SPR_SDR1] = sdr1;
> + if (kvmppc_put_books_sregs(cpu) < 0) {
> + error_report("Unable to update SDR1 in KVM");
> + exit(1);
> + }
> }
> }
> }
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 6ab8c188f381..06059b44ab40 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,
>
> if (kvm_enabled()) {
> /* For KVM PR, update the HPT pointer */
> - target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> - | (spapr->htab_shift - 18);
> - kvmppc_update_sdr1(sdr1);
> + target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> + if (sdr1) {
> + kvmppc_update_sdr1(sdr1);
> + }
> }
>
> pending->hpt = NULL; /* so it's not free()d */
> @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> spapr_reallocate_hpt(spapr, maxshift, &error_fatal);
> if (kvm_enabled()) {
> /* For KVM PR, update the HPT pointer */
> - target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> - | (spapr->htab_shift - 18);
> - kvmppc_update_sdr1(sdr1);
> + target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> + if (sdr1) {
> + kvmppc_update_sdr1(sdr1);
> + }
> }
> }
> }
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index c1b365f56431..a1f5edc15018 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -709,4 +709,5 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
> int spapr_vcpu_id(PowerPCCPU *cpu);
> PowerPCCPU *spapr_find_cpu(int vcpu_id);
>
> +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr);
> #endif /* HW_SPAPR_H */
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Sun, 10 Sep 2017 13:41:41 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote:
> > The formula used to compute the address of the HPT allocated by QEMU is
> > open-coded in several places. This patch moves the magic to a dedicated
> > helper. While here, we also patch the callers to only pass the address
> > to KVM if we indeed have a userland HPT (ie, KVM PR).
>
> The helper function seems reasonable, though I'm not sure about the
> name (a. it's not just a pointer, since it includes the encoded size
> and b. the name doesn't indicate this is basically KVM PR specific).
>
Sure, I'll come up with a better name.
> THe "only pass the address to KVM if we indeed have a userland HPT
> (ie, KVM PR)" bit doesn't really work. You're doing it by testing for
> (sdr1 != 0), but that can only be true if the HPT is minimum size,
> which doesn't have much to do with anything meaningful.
>
Hmmm... if QEMU has allocated an HPT in userspace then the helper
necessarily returns a non-null value, no matter the HPT size. Am
I missing something ?
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/ppc/spapr.c | 9 +++++++++
> > hw/ppc/spapr_cpu_core.c | 12 +++++++-----
> > hw/ppc/spapr_hcall.c | 14 ++++++++------
> > include/hw/ppc/spapr.h | 1 +
> > 4 files changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index caffa1276328..bf24c26b756d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex,
> > }
> > }
> >
> > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)
> > +{
> > + if (!spapr->htab) {
> > + return 0;
> > + }
> > +
> > + return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);
> > +}
> > +
> > int spapr_hpt_shift_for_ramsize(uint64_t ramsize)
> > {
> > int shift;
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 85037ef71e27..581eb4d92de9 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque)
> > * HPT
> > */
> > if (kvm_enabled()) {
> > - env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab
> > - | (spapr->htab_shift - 18);
> > - if (kvmppc_put_books_sregs(cpu) < 0) {
> > - error_report("Unable to update SDR1 in KVM");
> > - exit(1);
> > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> > + if (sdr1) {
> > + env->spr[SPR_SDR1] = sdr1;
> > + if (kvmppc_put_books_sregs(cpu) < 0) {
> > + error_report("Unable to update SDR1 in KVM");
> > + exit(1);
> > + }
> > }
> > }
> > }
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 6ab8c188f381..06059b44ab40 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,
> >
> > if (kvm_enabled()) {
> > /* For KVM PR, update the HPT pointer */
> > - target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> > - | (spapr->htab_shift - 18);
> > - kvmppc_update_sdr1(sdr1);
> > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> > + if (sdr1) {
> > + kvmppc_update_sdr1(sdr1);
> > + }
> > }
> >
> > pending->hpt = NULL; /* so it's not free()d */
> > @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> > spapr_reallocate_hpt(spapr, maxshift, &error_fatal);
> > if (kvm_enabled()) {
> > /* For KVM PR, update the HPT pointer */
> > - target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> > - | (spapr->htab_shift - 18);
> > - kvmppc_update_sdr1(sdr1);
> > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> > + if (sdr1) {
> > + kvmppc_update_sdr1(sdr1);
> > + }
> > }
> > }
> > }
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index c1b365f56431..a1f5edc15018 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -709,4 +709,5 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
> > int spapr_vcpu_id(PowerPCCPU *cpu);
> > PowerPCCPU *spapr_find_cpu(int vcpu_id);
> >
> > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr);
> > #endif /* HW_SPAPR_H */
> >
>
On Mon, Sep 11, 2017 at 02:04:37PM +0200, Greg Kurz wrote:
> On Sun, 10 Sep 2017 13:41:41 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote:
> > > The formula used to compute the address of the HPT allocated by QEMU is
> > > open-coded in several places. This patch moves the magic to a dedicated
> > > helper. While here, we also patch the callers to only pass the address
> > > to KVM if we indeed have a userland HPT (ie, KVM PR).
> >
> > The helper function seems reasonable, though I'm not sure about the
> > name (a. it's not just a pointer, since it includes the encoded size
> > and b. the name doesn't indicate this is basically KVM PR specific).
> >
>
> Sure, I'll come up with a better name.
>
> > THe "only pass the address to KVM if we indeed have a userland HPT
> > (ie, KVM PR)" bit doesn't really work. You're doing it by testing for
> > (sdr1 != 0), but that can only be true if the HPT is minimum size,
> > which doesn't have much to do with anything meaningful.
> >
>
> Hmmm... if QEMU has allocated an HPT in userspace then the helper
> necessarily returns a non-null value, no matter the HPT size. Am
> I missing something ?
Yes, but the reverse is not true. Even if qemu *hasn't* allocated an
HPT in userspace, it will usually return a non-zero value - the only
case it won't is when the HPT is minimum size. That makes the test
pretty pointless.
>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > hw/ppc/spapr.c | 9 +++++++++
> > > hw/ppc/spapr_cpu_core.c | 12 +++++++-----
> > > hw/ppc/spapr_hcall.c | 14 ++++++++------
> > > include/hw/ppc/spapr.h | 1 +
> > > 4 files changed, 25 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index caffa1276328..bf24c26b756d 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex,
> > > }
> > > }
> > >
> > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)
> > > +{
> > > + if (!spapr->htab) {
> > > + return 0;
> > > + }
> > > +
> > > + return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);
> > > +}
> > > +
> > > int spapr_hpt_shift_for_ramsize(uint64_t ramsize)
> > > {
> > > int shift;
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 85037ef71e27..581eb4d92de9 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque)
> > > * HPT
> > > */
> > > if (kvm_enabled()) {
> > > - env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab
> > > - | (spapr->htab_shift - 18);
> > > - if (kvmppc_put_books_sregs(cpu) < 0) {
> > > - error_report("Unable to update SDR1 in KVM");
> > > - exit(1);
> > > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> > > + if (sdr1) {
> > > + env->spr[SPR_SDR1] = sdr1;
> > > + if (kvmppc_put_books_sregs(cpu) < 0) {
> > > + error_report("Unable to update SDR1 in KVM");
> > > + exit(1);
> > > + }
> > > }
> > > }
> > > }
> > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > index 6ab8c188f381..06059b44ab40 100644
> > > --- a/hw/ppc/spapr_hcall.c
> > > +++ b/hw/ppc/spapr_hcall.c
> > > @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,
> > >
> > > if (kvm_enabled()) {
> > > /* For KVM PR, update the HPT pointer */
> > > - target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> > > - | (spapr->htab_shift - 18);
> > > - kvmppc_update_sdr1(sdr1);
> > > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> > > + if (sdr1) {
> > > + kvmppc_update_sdr1(sdr1);
> > > + }
> > > }
> > >
> > > pending->hpt = NULL; /* so it's not free()d */
> > > @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> > > spapr_reallocate_hpt(spapr, maxshift, &error_fatal);
> > > if (kvm_enabled()) {
> > > /* For KVM PR, update the HPT pointer */
> > > - target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> > > - | (spapr->htab_shift - 18);
> > > - kvmppc_update_sdr1(sdr1);
> > > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> > > + if (sdr1) {
> > > + kvmppc_update_sdr1(sdr1);
> > > + }
> > > }
> > > }
> > > }
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index c1b365f56431..a1f5edc15018 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -709,4 +709,5 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
> > > int spapr_vcpu_id(PowerPCCPU *cpu);
> > > PowerPCCPU *spapr_find_cpu(int vcpu_id);
> > >
> > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr);
> > > #endif /* HW_SPAPR_H */
> > >
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Mon, 11 Sep 2017 22:50:45 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Sep 11, 2017 at 02:04:37PM +0200, Greg Kurz wrote:
> > On Sun, 10 Sep 2017 13:41:41 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote:
> > > > The formula used to compute the address of the HPT allocated by QEMU is
> > > > open-coded in several places. This patch moves the magic to a dedicated
> > > > helper. While here, we also patch the callers to only pass the address
> > > > to KVM if we indeed have a userland HPT (ie, KVM PR).
> > >
> > > The helper function seems reasonable, though I'm not sure about the
> > > name (a. it's not just a pointer, since it includes the encoded size
> > > and b. the name doesn't indicate this is basically KVM PR specific).
> > >
> >
> > Sure, I'll come up with a better name.
> >
> > > THe "only pass the address to KVM if we indeed have a userland HPT
> > > (ie, KVM PR)" bit doesn't really work. You're doing it by testing for
> > > (sdr1 != 0), but that can only be true if the HPT is minimum size,
> > > which doesn't have much to do with anything meaningful.
> > >
> >
> > Hmmm... if QEMU has allocated an HPT in userspace then the helper
> > necessarily returns a non-null value, no matter the HPT size. Am
> > I missing something ?
>
> Yes, but the reverse is not true. Even if qemu *hasn't* allocated an
> HPT in userspace, it will usually return a non-zero value - the only
> case it won't is when the HPT is minimum size. That makes the test
> pretty pointless.
>
The helper does return 0 if QEMU hasn't allocated an HPT in userspace.
[...]
> > > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)
> > > > +{
> > > > + if (!spapr->htab) {
> > > > + return 0;
> > > > + }
> > > > +
> > > > + return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);
> > > > +}
> > > > +
but I agree the patch isn't good... for the comprehension at least. I'll
rework the patchset.
Also this causes a behavior change: we won't update SDR1 anymore with KVM HV,
which already handles it BTW.
void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info)
{
atomic64_set(&kvm->arch.mmio_update, 0);
kvm->arch.hpt = *info;
kvm->arch.sdr1 = __pa(info->virt) | (info->order - 18);
Maybe I should push this behavior change to a separate patch anyway...
© 2016 - 2026 Red Hat, Inc.