[Qemu-devel] [QEMU-PPC] [PATCH 0/2] target/ppc: hpt on radix and compat migration fixes

[Qemu-devel] [QEMU-PPC] [PATCH 0/2] target/ppc: hpt on radix and compat migration fixes

[Suraj Jitindar Singh]

The following patches fix 2 migration bugs.

The first being that migration of a hpt guest on a radix host currently
doesn't work. The first patch fixes this by installing the correct value
in patb_entry.

The second bug is that migration in a compat mode will currently result
in no compat mode actually being set on the receiving side. Fix this
by ensuring the compat_pvr is communicated to kvm.

The hpt on radix migration will require the host kernel to contain the patch:
KVM: PPC: Book3S HV: Fix migration and HPT resizing of HPT guests on radix hosts

Suraj Jitindar Singh (2):
  target/ppc: Move setting of patb_entry on hash table init
  target/ppc: Fix setting of cpu->compat_pvr on incoming migration

 hw/ppc/spapr.c       | 4 ++--
 target/ppc/machine.c | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.13.6

[Qemu-devel] [QEMU-PPC] [PATCH 1/2] target/ppc: Move setting of patb_entry on hash table init

[Suraj Jitindar Singh]

The patb_entry is used to store the location of the process table in
guest memory. The msb is also used to indicate the mmu mode of the
guest, that is patb_entry & 1 << 63 ? radix_mode : hash_mode.

Currently we set this to zero in spapr_setup_hpt_and_vrma() since if
this function gets called then we know we're hash. However some code
paths, such as setting up the hpt on incoming migration of a hash guest,
call spapr_reallocate_hpt() directly bypassing this higher level
function. Since we assume radix if the host is capable this results in
the msb in patb_entry being left set so in spapr_post_load() we call
kvmppc_configure_v3_mmu() and tell the host we're radix which as
expected means addresses cannot be translated once we actually run the cpu.

To fix this move the zeroing of patb_entry into spapr_reallocate_hpt().

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 hw/ppc/spapr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6841bd294b..e7af47bab0 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1373,6 +1373,8 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
             DIRTY_HPTE(HPTE(spapr->htab, i));
         }
     }
+    /* We're setting up a hash table, so that means we're not radix */
+    spapr->patb_entry = 0;
 }
 
 void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
@@ -1392,8 +1394,6 @@ void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
         spapr->rma_size = kvmppc_rma_size(spapr_node0_size(MACHINE(spapr)),
                                           spapr->htab_shift);
     }
-    /* We're setting up a hash table, so that means we're not radix */
-    spapr->patb_entry = 0;
 }
 
 static void find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
-- 
2.13.6

[Qemu-devel] [QEMU-PPC] [PATCH 2/2] target/ppc: Fix setting of cpu->compat_pvr on incoming migration

[Suraj Jitindar Singh]

cpu->compat_pvr is used to store the current compat mode of the cpu.

On the receiving side during incoming migration we check compatibility
with the compat mode by calling ppc_set_compat(). However we fail to set
the compat mode with the hypervisor since the "new" compat mode doesn't
differ from the current (due to a "cpu->compat_pvr != compat_pvr" check).
This means that kvm runs the vcpus without a compat mode, which is the
incorrect behaviour. The implication being that a compatibility mode
will never be in effect after migration.

To fix this so that the compat mode is correctly set with the
hypervisor, store the desired compat mode and reset cpu->compat_pvr to
zero before calling ppc_set_compat().

Fixes: 5dfaa532 ("ppc: fix ppc_set_compat() with KVM PR")

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

---

It is worth noting that another option was to add a force flag to the
ppc_set_compat() function, but the enacted solution seemed cleaner with
fewer code changes.
---
 target/ppc/machine.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 384caee800..24117e8f31 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -237,9 +237,11 @@ static int cpu_post_load(void *opaque, int version_id)
 
 #if defined(TARGET_PPC64)
     if (cpu->compat_pvr) {
+        uint32_t compat_pvr = cpu->compat_pvr;
         Error *local_err = NULL;
 
-        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
+        cpu->compat_pvr = 0;
+        ppc_set_compat(cpu, compat_pvr, &local_err);
         if (local_err) {
             error_report_err(local_err);
             return -1;
-- 
2.13.6

Re: [Qemu-devel] [QEMU-PPC] [PATCH 0/2] target/ppc: hpt on radix and compat migration fixes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On Fri, Nov 24, 2017 at 03:23:23PM +1100, Suraj Jitindar Singh wrote:
> The following patches fix 2 migration bugs.
> 
> The first being that migration of a hpt guest on a radix host currently
> doesn't work. The first patch fixes this by installing the correct value
> in patb_entry.
> 
> The second bug is that migration in a compat mode will currently result
> in no compat mode actually being set on the receiving side. Fix this
> by ensuring the compat_pvr is communicated to kvm.
> 
> The hpt on radix migration will require the host kernel to contain the patch:
> KVM: PPC: Book3S HV: Fix migration and HPT resizing of HPT guests on
> radix hosts

Applied to ppc-for-2.11.

> 
> Suraj Jitindar Singh (2):
>   target/ppc: Move setting of patb_entry on hash table init
>   target/ppc: Fix setting of cpu->compat_pvr on incoming migration
> 
>  hw/ppc/spapr.c       | 4 ++--
>  target/ppc/machine.c | 4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [QEMU-PPC] [PATCH 1/2] target/ppc: Move setting of patb_entry on hash table init

[Greg Kurz]

On Fri, 24 Nov 2017 15:23:24 +1100
Suraj Jitindar Singh <sjitindarsingh@gmail.com> wrote:

> The patb_entry is used to store the location of the process table in
> guest memory. The msb is also used to indicate the mmu mode of the
> guest, that is patb_entry & 1 << 63 ? radix_mode : hash_mode.
> 
> Currently we set this to zero in spapr_setup_hpt_and_vrma() since if
> this function gets called then we know we're hash. However some code
> paths, such as setting up the hpt on incoming migration of a hash guest,
> call spapr_reallocate_hpt() directly bypassing this higher level
> function. Since we assume radix if the host is capable this results in
> the msb in patb_entry being left set so in spapr_post_load() we call
> kvmppc_configure_v3_mmu() and tell the host we're radix which as
> expected means addresses cannot be translated once we actually run the cpu.
> 
> To fix this move the zeroing of patb_entry into spapr_reallocate_hpt().
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6841bd294b..e7af47bab0 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1373,6 +1373,8 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
>              DIRTY_HPTE(HPTE(spapr->htab, i));
>          }
>      }
> +    /* We're setting up a hash table, so that means we're not radix */
> +    spapr->patb_entry = 0;
>  }
>  
>  void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
> @@ -1392,8 +1394,6 @@ void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
>          spapr->rma_size = kvmppc_rma_size(spapr_node0_size(MACHINE(spapr)),
>                                            spapr->htab_shift);
>      }
> -    /* We're setting up a hash table, so that means we're not radix */
> -    spapr->patb_entry = 0;
>  }
>  
>  static void find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)

Re: [Qemu-devel] [QEMU-PPC] [PATCH 2/2] target/ppc: Fix setting of cpu->compat_pvr on incoming migration

[Greg Kurz]

On Fri, 24 Nov 2017 15:23:25 +1100
Suraj Jitindar Singh <sjitindarsingh@gmail.com> wrote:

> cpu->compat_pvr is used to store the current compat mode of the cpu.
> 
> On the receiving side during incoming migration we check compatibility
> with the compat mode by calling ppc_set_compat(). However we fail to set
> the compat mode with the hypervisor since the "new" compat mode doesn't
> differ from the current (due to a "cpu->compat_pvr != compat_pvr" check).
> This means that kvm runs the vcpus without a compat mode, which is the
> incorrect behaviour. The implication being that a compatibility mode
> will never be in effect after migration.
> 
> To fix this so that the compat mode is correctly set with the
> hypervisor, store the desired compat mode and reset cpu->compat_pvr to
> zero before calling ppc_set_compat().
> 

So we'd twist the code in cpu_post_load() because of some check done in
ppc_set_compat(), which is actually a workaround because PR KVM doesn't
support KVM_REG_PPC_ARCH_COMPAT... ouch :-\

David,

This was discussed last summer in

	http://patchwork.ozlabs.org/patch/782039/

and you wrote:

> Well, qemu expects to be able to set ARCH_COMPAT for a pseries guest,
> if that guest is going into a compatibility mode (which it usually
> does these days).  We don't want userspace to have to be constantly
> checking which KVM implementation its working against, so it makes
> sense for PR to implement the call, even if it's a no-op because it
> can't really implement the PCR fully.

I understand your point but implementing ARCH_COMPAT in PR KVM will
probably never happen... and as you say, it might be only a no-op.

So I guess we can "implement" this behaviour in kvmppc_set_compat()
with kvmppc_is_pr() and a fat comment, and drop the compat_pvr check
this patch is trying to negate.

> Fixes: 5dfaa532 ("ppc: fix ppc_set_compat() with KVM PR")
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> 
> ---
> 
> It is worth noting that another option was to add a force flag to the
> ppc_set_compat() function, but the enacted solution seemed cleaner with
> fewer code changes.
> ---
>  target/ppc/machine.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 384caee800..24117e8f31 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -237,9 +237,11 @@ static int cpu_post_load(void *opaque, int version_id)
>  
>  #if defined(TARGET_PPC64)
>      if (cpu->compat_pvr) {
> +        uint32_t compat_pvr = cpu->compat_pvr;
>          Error *local_err = NULL;
>  
> -        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> +        cpu->compat_pvr = 0;
> +        ppc_set_compat(cpu, compat_pvr, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>              return -1;