[Qemu-devel] [PATCH 0/4] QEMU MIPS KVM improvements for v2.1

[Qemu-devel] [PATCH 0/4] QEMU MIPS KVM improvements for v2.1

[James Hogan]

This patchset has a few improvements & minor fixes for MIPS KVM support.

Patches 1-2 are fixes for forward compatibility of savevm with MIPS KVM.
Patch 3 just corrects comments and an error message.
Patch 4 adds errors when the wrong type of kernel is provided.

James Hogan (4):
  mips/kvm: Init EBase to correct KSEG0
  mips_malta: Change default KVM cpu to 24Kc (no FP)
  mips_malta: Remove incorrect KVM T&E references
  mips_malta: Catch kernels linked at wrong address

 hw/mips/mips_malta.c    | 27 +++++++++++++++++++++++----
 target-mips/translate.c |  8 +++++++-
 2 files changed, 30 insertions(+), 5 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/4] mips/kvm: Init EBase to correct KSEG0

[James Hogan]

The EBase CP0 register is initialised to 0x80000000, however with KVM
the guest's KSEG0 is at 0x40000000. The incorrect value doesn't get
passed to KVM yet as KVM doesn't implement the EBase register, however
we should set it correctly now so as not to break migration/loadvm to a
future version of QEMU that does support EBase.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 target-mips/translate.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 2f91959ed7b1..d7b8c4dbc81a 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -28,6 +28,7 @@
 
 #include "exec/helper-proto.h"
 #include "exec/helper-gen.h"
+#include "sysemu/kvm.h"
 
 #define MIPS_DEBUG_DISAS 0
 //#define MIPS_DEBUG_SIGN_EXTENSIONS
@@ -16076,7 +16077,12 @@ void cpu_state_reset(CPUMIPSState *env)
     env->CP0_Random = env->tlb->nb_tlb - 1;
     env->tlb->tlb_in_use = env->tlb->nb_tlb;
     env->CP0_Wired = 0;
-    env->CP0_EBase = 0x80000000 | (cs->cpu_index & 0x3FF);
+    env->CP0_EBase = (cs->cpu_index & 0x3FF);
+    if (kvm_enabled()) {
+        env->CP0_EBase |= 0x40000000;
+    } else {
+        env->CP0_EBase |= 0x80000000;
+    }
     env->CP0_Status = (1 << CP0St_BEV) | (1 << CP0St_ERL);
     /* vectored interrupts not implemented, timer on int 7,
        no performance counters. */
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/4] mips/kvm: Init EBase to correct KSEG0

[Aurelien Jarno]

On Thu, Jun 26, 2014 at 10:44:22AM +0100, James Hogan wrote:
> The EBase CP0 register is initialised to 0x80000000, however with KVM
> the guest's KSEG0 is at 0x40000000. The incorrect value doesn't get
> passed to KVM yet as KVM doesn't implement the EBase register, however
> we should set it correctly now so as not to break migration/loadvm to a
> future version of QEMU that does support EBase.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target-mips/translate.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 2f91959ed7b1..d7b8c4dbc81a 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -28,6 +28,7 @@
>  
>  #include "exec/helper-proto.h"
>  #include "exec/helper-gen.h"
> +#include "sysemu/kvm.h"
>  
>  #define MIPS_DEBUG_DISAS 0
>  //#define MIPS_DEBUG_SIGN_EXTENSIONS
> @@ -16076,7 +16077,12 @@ void cpu_state_reset(CPUMIPSState *env)
>      env->CP0_Random = env->tlb->nb_tlb - 1;
>      env->tlb->tlb_in_use = env->tlb->nb_tlb;
>      env->CP0_Wired = 0;
> -    env->CP0_EBase = 0x80000000 | (cs->cpu_index & 0x3FF);
> +    env->CP0_EBase = (cs->cpu_index & 0x3FF);
> +    if (kvm_enabled()) {
> +        env->CP0_EBase |= 0x40000000;
> +    } else {
> +        env->CP0_EBase |= 0x80000000;
> +    }
>      env->CP0_Status = (1 << CP0St_BEV) | (1 << CP0St_ERL);
>      /* vectored interrupts not implemented, timer on int 7,
>         no performance counters. */

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH 2/4] mips_malta: Change default KVM cpu to 24Kc (no FP)

[James Hogan]

Change the default Malta CPU model for when KVM is enabled to 24Kc which
doesn't have floating point support compared to the 24Kf.

The resulting incorrect Config CP0 register value doesn't get passed to
KVM yet as KVM doesn't expose it, however we should ensure it is set
correctly now to reduce the risk of breaking migration/loadvm to a
future version of QEMU/Linux that does support them.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mips/mips_malta.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 2868ee5b0307..c0841991f4e9 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -949,7 +949,12 @@ void mips_malta_init(MachineState *machine)
 #ifdef TARGET_MIPS64
         cpu_model = "20Kc";
 #else
-        cpu_model = "24Kf";
+        if (kvm_enabled()) {
+            /* Don't enable FPU on KVM yet */
+            cpu_model = "24Kc";
+        } else {
+            cpu_model = "24Kf";
+        }
 #endif
     }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/4] mips_malta: Change default KVM cpu to 24Kc (no FP)

[Aurelien Jarno]

On Thu, Jun 26, 2014 at 10:44:23AM +0100, James Hogan wrote:
> Change the default Malta CPU model for when KVM is enabled to 24Kc which
> doesn't have floating point support compared to the 24Kf.
> 
> The resulting incorrect Config CP0 register value doesn't get passed to
> KVM yet as KVM doesn't expose it, however we should ensure it is set
> correctly now to reduce the risk of breaking migration/loadvm to a
> future version of QEMU/Linux that does support them.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/mips/mips_malta.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 2868ee5b0307..c0841991f4e9 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -949,7 +949,12 @@ void mips_malta_init(MachineState *machine)
>  #ifdef TARGET_MIPS64
>          cpu_model = "20Kc";
>  #else
> -        cpu_model = "24Kf";
> +        if (kvm_enabled()) {
> +            /* Don't enable FPU on KVM yet */
> +            cpu_model = "24Kc";
> +        } else {
> +            cpu_model = "24Kf";
> +        }
>  #endif
>      }

Given the explanations in the other mails, that looks fine to me, that
said I think we should at least warn the user that we are disabling some
features, instead of doing it silently. This is what is done for example
on x86 when a CPU feature is not available.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/4] mips_malta: Change default KVM cpu to 24Kc (no FP)

[Paolo Bonzini]

Il 27/06/2014 10:43, Aurelien Jarno ha scritto:
> On Thu, Jun 26, 2014 at 10:44:23AM +0100, James Hogan wrote:
>> Change the default Malta CPU model for when KVM is enabled to 24Kc which
>> doesn't have floating point support compared to the 24Kf.
>>
>> The resulting incorrect Config CP0 register value doesn't get passed to
>> KVM yet as KVM doesn't expose it, however we should ensure it is set
>> correctly now to reduce the risk of breaking migration/loadvm to a
>> future version of QEMU/Linux that does support them.
>>
>> Signed-off-by: James Hogan <james.hogan@imgtec.com>
>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/mips/mips_malta.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>> index 2868ee5b0307..c0841991f4e9 100644
>> --- a/hw/mips/mips_malta.c
>> +++ b/hw/mips/mips_malta.c
>> @@ -949,7 +949,12 @@ void mips_malta_init(MachineState *machine)
>>  #ifdef TARGET_MIPS64
>>          cpu_model = "20Kc";
>>  #else
>> -        cpu_model = "24Kf";
>> +        if (kvm_enabled()) {
>> +            /* Don't enable FPU on KVM yet */
>> +            cpu_model = "24Kc";
>> +        } else {
>> +            cpu_model = "24Kf";
>> +        }
>>  #endif
>>      }
>
> Given the explanations in the other mails, that looks fine to me, that
> said I think we should at least warn the user that we are disabling some
> features, instead of doing it silently. This is what is done for example
> on x86 when a CPU feature is not available.

I agree.  James, can you send v2 of this patch only?

Paolo

[Qemu-devel] [PATCH v2 2/4] mips/kvm: Disable FPU on reset with KVM

[James Hogan]

KVM doesn't yet support the MIPS FPU, or writing to the guest's Config1
register which contains the FPU implemented bit. Clear QEMU's version of
that bit on reset and display a warning that the FPU has been disabled.

The previous incorrect Config1 CP0 register value wasn't being passed to
KVM yet, however we should ensure it is set correctly now to reduce the
risk of breaking migration/loadvm to a future version of QEMU/Linux that
does support it.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
A slight variation this time, which should handle the case of the user
setting the CPU explicitly a bit better (previously only the default
Malta CPU was changed). Look reasonable?

Changes in v2:
 - Add a warning to alert user that FPU is disabled (Aurelien Jarno).
 - Slightly different approach. Disable the FPU on reset from KVM code
   instead of only changing the default CPU for Malta. This will then
   happen even if the user specifies the core type explicitly (and
   without having to modify any CPU definitions), but it does print a
   message on each reset.
 - Rewrite (and hopefully clarify) commit message.
---
 target-mips/kvm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-mips/kvm.c b/target-mips/kvm.c
index 844e5bbe5f92..97fd51a02f5c 100644
--- a/target-mips/kvm.c
+++ b/target-mips/kvm.c
@@ -61,6 +61,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
 void kvm_mips_reset_vcpu(MIPSCPU *cpu)
 {
+    CPUMIPSState *env = &cpu->env;
+
+    if (env->CP0_Config1 & (1 << CP0C1_FP)) {
+        fprintf(stderr, "Warning: FPU not supported with KVM, disabling\n");
+        env->CP0_Config1 &= ~(1 << CP0C1_FP);
+    }
+
     DPRINTF("%s\n", __func__);
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH 3/4] mips_malta: Remove incorrect KVM T&E references

[James Hogan]

Fix the error message and code comments relating to KVM not supporting
booting from the flash mapping when no kernel is provided. The issue is
a general MIPS KVM issue and isn't specific to the Trap & Emulate
version of MIPS KVM.

Reported-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mips/mips_malta.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index c0841991f4e9..76cf5f2c48f4 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1033,7 +1033,7 @@ void mips_malta_init(MachineState *machine)
     fl_idx++;
     if (kernel_filename) {
         ram_low_size = MIN(ram_size, 256 << 20);
-        /* For KVM T&E we reserve 1MB of RAM for running bootloader */
+        /* For KVM we reserve 1MB of RAM for running bootloader */
         if (kvm_enabled()) {
             ram_low_size -= 0x100000;
             bootloader_run_addr = 0x40000000 + ram_low_size;
@@ -1057,10 +1057,10 @@ void mips_malta_init(MachineState *machine)
                              bootloader_run_addr, kernel_entry);
         }
     } else {
-        /* The flash region isn't executable from a KVM T&E guest */
+        /* The flash region isn't executable from a KVM guest */
         if (kvm_enabled()) {
             error_report("KVM enabled but no -kernel argument was specified. "
-                         "Booting from flash is not supported with KVM T&E.");
+                         "Booting from flash is not supported with KVM.");
             exit(1);
         }
         /* Load firmware from flash. */
-- 
1.9.3

Re: [Qemu-devel] [PATCH 3/4] mips_malta: Remove incorrect KVM T&E references

[Aurelien Jarno]

On Thu, Jun 26, 2014 at 10:44:24AM +0100, James Hogan wrote:
> Fix the error message and code comments relating to KVM not supporting
> booting from the flash mapping when no kernel is provided. The issue is
> a general MIPS KVM issue and isn't specific to the Trap & Emulate
> version of MIPS KVM.
> 
> Reported-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/mips/mips_malta.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index c0841991f4e9..76cf5f2c48f4 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1033,7 +1033,7 @@ void mips_malta_init(MachineState *machine)
>      fl_idx++;
>      if (kernel_filename) {
>          ram_low_size = MIN(ram_size, 256 << 20);
> -        /* For KVM T&E we reserve 1MB of RAM for running bootloader */
> +        /* For KVM we reserve 1MB of RAM for running bootloader */
>          if (kvm_enabled()) {
>              ram_low_size -= 0x100000;
>              bootloader_run_addr = 0x40000000 + ram_low_size;
> @@ -1057,10 +1057,10 @@ void mips_malta_init(MachineState *machine)
>                               bootloader_run_addr, kernel_entry);
>          }
>      } else {
> -        /* The flash region isn't executable from a KVM T&E guest */
> +        /* The flash region isn't executable from a KVM guest */
>          if (kvm_enabled()) {
>              error_report("KVM enabled but no -kernel argument was specified. "
> -                         "Booting from flash is not supported with KVM T&E.");
> +                         "Booting from flash is not supported with KVM.");
>              exit(1);
>          }
>          /* Load firmware from flash. */

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>


-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH 4/4] mips_malta: Catch kernels linked at wrong address

[James Hogan]

Add error reporting if the wrong type of kernel is provided for the
current mode of acceleration.

Currently a KVM kernel linked at 0x40000000 can't be used with TCG, and
a normal kernel linked at 0x80000000 can't be used with KVM.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mips/mips_malta.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 76cf5f2c48f4..95df42e6a4d5 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -792,9 +792,23 @@ static int64_t load_kernel (void)
                 loaderparams.kernel_filename);
         exit(1);
     }
+
+    /* Sanity check where the kernel has been linked */
     if (kvm_enabled()) {
+        if (kernel_entry & 0x80000000ll) {
+            error_report("KVM guest kernels must be linked in useg. "
+                         "Did you forget to enable CONFIG_KVM_GUEST?");
+            exit(1);
+        }
+
         xlate_to_kseg0 = cpu_mips_kvm_um_phys_to_kseg0;
     } else {
+        if (!(kernel_entry & 0x80000000ll)) {
+            error_report("KVM guest kernels aren't supported with TCG. "
+                         "Did you unintentionally enable CONFIG_KVM_GUEST?");
+            exit(1);
+        }
+
         xlate_to_kseg0 = cpu_mips_phys_to_kseg0;
     }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 4/4] mips_malta: Catch kernels linked at wrong address

[Aurelien Jarno]

On Thu, Jun 26, 2014 at 10:44:25AM +0100, James Hogan wrote:
> Add error reporting if the wrong type of kernel is provided for the
> current mode of acceleration.
> 
> Currently a KVM kernel linked at 0x40000000 can't be used with TCG, and
> a normal kernel linked at 0x80000000 can't be used with KVM.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/mips/mips_malta.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 76cf5f2c48f4..95df42e6a4d5 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -792,9 +792,23 @@ static int64_t load_kernel (void)
>                  loaderparams.kernel_filename);
>          exit(1);
>      }
> +
> +    /* Sanity check where the kernel has been linked */
>      if (kvm_enabled()) {
> +        if (kernel_entry & 0x80000000ll) {
> +            error_report("KVM guest kernels must be linked in useg. "
> +                         "Did you forget to enable CONFIG_KVM_GUEST?");
> +            exit(1);
> +        }
> +
>          xlate_to_kseg0 = cpu_mips_kvm_um_phys_to_kseg0;
>      } else {
> +        if (!(kernel_entry & 0x80000000ll)) {
> +            error_report("KVM guest kernels aren't supported with TCG. "
> +                         "Did you unintentionally enable CONFIG_KVM_GUEST?");
> +            exit(1);
> +        }
> +
>          xlate_to_kseg0 = cpu_mips_phys_to_kseg0;
>      }

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
 

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 0/4] QEMU MIPS KVM improvements for v2.1

[Paolo Bonzini]

Il 26/06/2014 11:44, James Hogan ha scritto:
> This patchset has a few improvements & minor fixes for MIPS KVM support.
>
> Patches 1-2 are fixes for forward compatibility of savevm with MIPS KVM.
> Patch 3 just corrects comments and an error message.
> Patch 4 adds errors when the wrong type of kernel is provided.
>
> James Hogan (4):
>   mips/kvm: Init EBase to correct KSEG0
>   mips_malta: Change default KVM cpu to 24Kc (no FP)
>   mips_malta: Remove incorrect KVM T&E references
>   mips_malta: Catch kernels linked at wrong address
>
>  hw/mips/mips_malta.c    | 27 +++++++++++++++++++++++----
>  target-mips/translate.c |  8 +++++++-
>  2 files changed, 30 insertions(+), 5 deletions(-)
>

Looks good, I'm just a bit unsure about patch 2.  Is FPU support on the 
todo list?  Is the idea to add a KVM capability when you add it?

Paolo

Re: [Qemu-devel] [PATCH 0/4] QEMU MIPS KVM improvements for v2.1

[James Hogan]

On 26/06/14 11:12, Paolo Bonzini wrote:
> Il 26/06/2014 11:44, James Hogan ha scritto:
>> This patchset has a few improvements & minor fixes for MIPS KVM support.
>>
>> Patches 1-2 are fixes for forward compatibility of savevm with MIPS KVM.
>> Patch 3 just corrects comments and an error message.
>> Patch 4 adds errors when the wrong type of kernel is provided.
>>
>> James Hogan (4):
>>   mips/kvm: Init EBase to correct KSEG0
>>   mips_malta: Change default KVM cpu to 24Kc (no FP)
>>   mips_malta: Remove incorrect KVM T&E references
>>   mips_malta: Catch kernels linked at wrong address
>>
>>  hw/mips/mips_malta.c    | 27 +++++++++++++++++++++++----
>>  target-mips/translate.c |  8 +++++++-
>>  2 files changed, 30 insertions(+), 5 deletions(-)
>>
> 
> Looks good, I'm just a bit unsure about patch 2.  Is FPU support on the
> todo list?  Is the idea to add a KVM capability when you add it?

At the moment KVM exposes a certain configuration to the guest,
regardless of what QEMU thinks the configuration is (see
kvm_trap_emul_vcpu_setup()). Patch 2 simply changes QEMU to match what
KVM is emulating.

FP is definitely on the todo list for VZ at least (and should in theory
be doable in T&E too), but I haven't thought too much about the details yet.

I'd guess a KVM capability to indicate that KVM supports the API would
make sense.

Even then there are multiple configurations that might want to be
used/distinguished somehow, e.g.:
* never support FP in guest (probably default for backward compatibility)
* always support FP, always via emulation in hypervisor (all FP
instructions trapping)
* always support FP, using hardware if available (minimal trapping to
guarantee guest sees an FPU)
* support FP only if host supports it, using hardware (minimal trapping
if hardware, trapping to guest OS to emulate FP if not)

Maybe which one of the above is chosen would affect what fields can be
read/written to guest Config registers with GET/SET_ONE_REG API (probing
guest config registers is closely analogous to how VZ hypervisor
controls/probes what features are/can be exposed to the guest).

Cheers
James