[PATCH qemu] spapr: Add PVR setting capability

[PATCH qemu] spapr: Add PVR setting capability

[Alexey Kardashevskiy]

At the moment the VCPU init sequence includes setting PVR which in case of
KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
virtualized by the hardware. In order to cope with various CPU revisions
only top 16bit of PVR are checked which works for minor revision updates.

However in every CPU generation starting POWER7 (at least) there were CPUs
supporting the (almost) same POWER ISA level but having different top
16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
with a new PVR family. We would normally add the PVR mask for the new one
too, the problem with it is that although the physical machines exist,
P9+ is not going to be released as a product, and this situation is likely
to repeat in the future.

Instead of adding every new CPU family in QEMU, this adds a new sPAPR
machine capability to force PVR setting/checking. It is "on" by default
to preserve the existing behavior. When "off", it is the user's
responsibility to specify the correct CPU.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 include/hw/ppc/spapr.h |  5 ++++-
 hw/ppc/spapr.c         |  1 +
 hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
 target/ppc/kvm.c       | 16 ++++++++++++++--
 4 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index e579eaf28c05..5ccac4d56871 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -81,8 +81,10 @@ typedef enum {
 #define SPAPR_CAP_CCF_ASSIST            0x09
 /* Implements PAPR FWNMI option */
 #define SPAPR_CAP_FWNMI                 0x0A
+/* Implements PAPR PVR option */
+#define SPAPR_CAP_PVR                   0x0B
 /* Num Caps */
-#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
+#define SPAPR_CAP_NUM                   (SPAPR_CAP_PVR + 1)
 
 /*
  * Capability Values
@@ -912,6 +914,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
 extern const VMStateDescription vmstate_spapr_cap_large_decr;
 extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
 extern const VMStateDescription vmstate_spapr_cap_fwnmi;
+extern const VMStateDescription vmstate_spapr_cap_pvr;
 
 static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
 {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 841b5ec59b12..ecc74c182b9f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
     smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
     smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
+    smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
     spapr_caps_add_properties(smc, &error_abort);
     smc->irq = &spapr_irq_dual;
     smc->dr_phb_enabled = true;
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index eb54f9422722..398b72b77f9f 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
     }
 }
 
+static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
+{
+    if (val) {
+        return;
+    }
+    warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is supported");
+}
+
 SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
     [SPAPR_CAP_HTM] = {
         .name = "htm",
@@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
         .type = "bool",
         .apply = cap_fwnmi_apply,
     },
+    [SPAPR_CAP_PVR] = {
+        .name = "pvr",
+        .description = "Enforce PVR in KVM",
+        .index = SPAPR_CAP_PVR,
+        .get = spapr_cap_get_bool,
+        .set = spapr_cap_set_bool,
+        .type = "bool",
+        .apply = cap_pvr_apply,
+    },
 };
 
 static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
@@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
 SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
 SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
 SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
+SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
 
 void spapr_caps_init(SpaprMachineState *spapr)
 {
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 03d0667e8f94..a4adc29b6522 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *cenv = &cpu->env;
     int ret;
+    SpaprMachineState *spapr;
 
     /* Synchronize sregs with kvm */
     ret = kvm_arch_sync_sregs(cpu);
     if (ret) {
         if (ret == -EINVAL) {
             error_report("Register sync failed... If you're using kvm-hv.ko,"
-                         " only \"-cpu host\" is possible");
+                         " only \"-cpu host\" is supported");
+        }
+        /*
+         * The user chose not to set PVR which makes sense if we are running
+         * on a CPU with known ISA level but unknown PVR.
+         */
+        spapr = (SpaprMachineState *)
+            object_dynamic_cast(OBJECT(qdev_get_machine()), TYPE_SPAPR_MACHINE);
+
+        if (spapr && spapr->eff.caps[SPAPR_CAP_PVR] == SPAPR_CAP_OFF) {
+            ret = 0;
+        } else {
+            return ret;
         }
-        return ret;
     }
 
     switch (cenv->mmu_model) {
-- 
2.17.1

Re: [PATCH qemu] spapr: Add PVR setting capability

[Alexey Kardashevskiy]

On 17/04/2020 14:11, Alexey Kardashevskiy wrote:
> At the moment the VCPU init sequence includes setting PVR which in case of
> KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
> virtualized by the hardware. In order to cope with various CPU revisions
> only top 16bit of PVR are checked which works for minor revision updates.
> 
> However in every CPU generation starting POWER7 (at least) there were CPUs
> supporting the (almost) same POWER ISA level but having different top
> 16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
> with a new PVR family. We would normally add the PVR mask for the new one
> too, the problem with it is that although the physical machines exist,
> P9+ is not going to be released as a product, and this situation is likely
> to repeat in the future.
> 
> Instead of adding every new CPU family in QEMU, this adds a new sPAPR
> machine capability to force PVR setting/checking. It is "on" by default
> to preserve the existing behavior. When "off", it is the user's
> responsibility to specify the correct CPU.

Ping?



> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  include/hw/ppc/spapr.h |  5 ++++-
>  hw/ppc/spapr.c         |  1 +
>  hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
>  target/ppc/kvm.c       | 16 ++++++++++++++--
>  4 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index e579eaf28c05..5ccac4d56871 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -81,8 +81,10 @@ typedef enum {
>  #define SPAPR_CAP_CCF_ASSIST            0x09
>  /* Implements PAPR FWNMI option */
>  #define SPAPR_CAP_FWNMI                 0x0A
> +/* Implements PAPR PVR option */
> +#define SPAPR_CAP_PVR                   0x0B
>  /* Num Caps */
> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_PVR + 1)
>  
>  /*
>   * Capability Values
> @@ -912,6 +914,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
> +extern const VMStateDescription vmstate_spapr_cap_pvr;
>  
>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>  {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 841b5ec59b12..ecc74c182b9f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
> +    smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
>      spapr_caps_add_properties(smc, &error_abort);
>      smc->irq = &spapr_irq_dual;
>      smc->dr_phb_enabled = true;
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index eb54f9422722..398b72b77f9f 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>      }
>  }
>  
> +static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
> +{
> +    if (val) {
> +        return;
> +    }
> +    warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is supported");
> +}
> +
>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      [SPAPR_CAP_HTM] = {
>          .name = "htm",
> @@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>          .type = "bool",
>          .apply = cap_fwnmi_apply,
>      },
> +    [SPAPR_CAP_PVR] = {
> +        .name = "pvr",
> +        .description = "Enforce PVR in KVM",
> +        .index = SPAPR_CAP_PVR,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
> +        .apply = cap_pvr_apply,
> +    },
>  };
>  
>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> @@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
> +SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
>  
>  void spapr_caps_init(SpaprMachineState *spapr)
>  {
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 03d0667e8f94..a4adc29b6522 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *cenv = &cpu->env;
>      int ret;
> +    SpaprMachineState *spapr;
>  
>      /* Synchronize sregs with kvm */
>      ret = kvm_arch_sync_sregs(cpu);
>      if (ret) {
>          if (ret == -EINVAL) {
>              error_report("Register sync failed... If you're using kvm-hv.ko,"
> -                         " only \"-cpu host\" is possible");
> +                         " only \"-cpu host\" is supported");
> +        }
> +        /*
> +         * The user chose not to set PVR which makes sense if we are running
> +         * on a CPU with known ISA level but unknown PVR.
> +         */
> +        spapr = (SpaprMachineState *)
> +            object_dynamic_cast(OBJECT(qdev_get_machine()), TYPE_SPAPR_MACHINE);
> +
> +        if (spapr && spapr->eff.caps[SPAPR_CAP_PVR] == SPAPR_CAP_OFF) {
> +            ret = 0;
> +        } else {
> +            return ret;
>          }
> -        return ret;
>      }
>  
>      switch (cenv->mmu_model) {
> 

-- 
Alexey

Re: [PATCH qemu] spapr: Add PVR setting capability

[Greg Kurz]

On Fri, 17 Apr 2020 14:11:05 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> At the moment the VCPU init sequence includes setting PVR which in case of
> KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
> virtualized by the hardware. In order to cope with various CPU revisions
> only top 16bit of PVR are checked which works for minor revision updates.
> 
> However in every CPU generation starting POWER7 (at least) there were CPUs
> supporting the (almost) same POWER ISA level but having different top
> 16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
> with a new PVR family. We would normally add the PVR mask for the new one
> too, the problem with it is that although the physical machines exist,
> P9+ is not going to be released as a product, and this situation is likely
> to repeat in the future.
> 
> Instead of adding every new CPU family in QEMU, this adds a new sPAPR
> machine capability to force PVR setting/checking. It is "on" by default
> to preserve the existing behavior. When "off", it is the user's
> responsibility to specify the correct CPU.
> 

I don't quite understand the motivation for this... what does this
buy us ?

> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  include/hw/ppc/spapr.h |  5 ++++-
>  hw/ppc/spapr.c         |  1 +
>  hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
>  target/ppc/kvm.c       | 16 ++++++++++++++--
>  4 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index e579eaf28c05..5ccac4d56871 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -81,8 +81,10 @@ typedef enum {
>  #define SPAPR_CAP_CCF_ASSIST            0x09
>  /* Implements PAPR FWNMI option */
>  #define SPAPR_CAP_FWNMI                 0x0A
> +/* Implements PAPR PVR option */
> +#define SPAPR_CAP_PVR                   0x0B
>  /* Num Caps */
> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_PVR + 1)
>  
>  /*
>   * Capability Values
> @@ -912,6 +914,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
> +extern const VMStateDescription vmstate_spapr_cap_pvr;
>  
>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>  {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 841b5ec59b12..ecc74c182b9f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
> +    smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
>      spapr_caps_add_properties(smc, &error_abort);
>      smc->irq = &spapr_irq_dual;
>      smc->dr_phb_enabled = true;
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index eb54f9422722..398b72b77f9f 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>      }
>  }
>  
> +static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
> +{
> +    if (val) {
> +        return;
> +    }
> +    warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is supported");
> +}
> +
>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      [SPAPR_CAP_HTM] = {
>          .name = "htm",
> @@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>          .type = "bool",
>          .apply = cap_fwnmi_apply,
>      },
> +    [SPAPR_CAP_PVR] = {
> +        .name = "pvr",
> +        .description = "Enforce PVR in KVM",
> +        .index = SPAPR_CAP_PVR,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
> +        .apply = cap_pvr_apply,
> +    },
>  };
>  
>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> @@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
> +SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
>  
>  void spapr_caps_init(SpaprMachineState *spapr)
>  {
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 03d0667e8f94..a4adc29b6522 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *cenv = &cpu->env;
>      int ret;
> +    SpaprMachineState *spapr;
>  

We generally try to avoid adding such explicit dependencies to the
machine code within the target directory... A virtual hypervisor
hook could possibly do the trick but this would require to set
PowerPCCPU::vhyp before kvm_arch_init_vcpu() gets called, eg.
when the vCPU is created in spapr_create_vcpu() rather than
when it gets realized.

>      /* Synchronize sregs with kvm */
>      ret = kvm_arch_sync_sregs(cpu);
>      if (ret) {
>          if (ret == -EINVAL) {
>              error_report("Register sync failed... If you're using kvm-hv.ko,"
> -                         " only \"-cpu host\" is possible");
> +                         " only \"-cpu host\" is supported");
> +        }
> +        /*
> +         * The user chose not to set PVR which makes sense if we are running
> +         * on a CPU with known ISA level but unknown PVR.
> +         */
> +        spapr = (SpaprMachineState *)
> +            object_dynamic_cast(OBJECT(qdev_get_machine()), TYPE_SPAPR_MACHINE);
> +
> +        if (spapr && spapr->eff.caps[SPAPR_CAP_PVR] == SPAPR_CAP_OFF) {
> +            ret = 0;
> +        } else {
> +            return ret;
>          }
> -        return ret;
>      }
>  
>      switch (cenv->mmu_model) {

Re: [PATCH qemu] spapr: Add PVR setting capability

[Cédric Le Goater]

On 5/4/20 1:30 PM, Greg Kurz wrote:
> On Fri, 17 Apr 2020 14:11:05 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> At the moment the VCPU init sequence includes setting PVR which in case of
>> KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
>> virtualized by the hardware. In order to cope with various CPU revisions
>> only top 16bit of PVR are checked which works for minor revision updates.
>>
>> However in every CPU generation starting POWER7 (at least) there were CPUs
>> supporting the (almost) same POWER ISA level but having different top
>> 16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
>> with a new PVR family. We would normally add the PVR mask for the new one
>> too, the problem with it is that although the physical machines exist,
>> P9+ is not going to be released as a product, and this situation is likely
>> to repeat in the future.
>>
>> Instead of adding every new CPU family in QEMU, this adds a new sPAPR
>> machine capability to force PVR setting/checking. It is "on" by default
>> to preserve the existing behavior. When "off", it is the user's
>> responsibility to specify the correct CPU.
>>
> 
> I don't quite understand the motivation for this... what does this
> buy us ?

So we could use the command line options : 

 -cpu POWER8 -machine pseries,pvr=off

instead of

 -cpu host -machine pseries,max-cpu-compat=POWER8  

is that it ? 

I am not sure I get it either.


>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  include/hw/ppc/spapr.h |  5 ++++-
>>  hw/ppc/spapr.c         |  1 +
>>  hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
>>  target/ppc/kvm.c       | 16 ++++++++++++++--
>>  4 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index e579eaf28c05..5ccac4d56871 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -81,8 +81,10 @@ typedef enum {
>>  #define SPAPR_CAP_CCF_ASSIST            0x09
>>  /* Implements PAPR FWNMI option */
>>  #define SPAPR_CAP_FWNMI                 0x0A
>> +/* Implements PAPR PVR option */
>> +#define SPAPR_CAP_PVR                   0x0B
>>  /* Num Caps */
>> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
>> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_PVR + 1)
>>  
>>  /*
>>   * Capability Values
>> @@ -912,6 +914,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
>>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
>>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>> +extern const VMStateDescription vmstate_spapr_cap_pvr;
>>  
>>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>>  {
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 841b5ec59b12..ecc74c182b9f 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
>> +    smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
>>      spapr_caps_add_properties(smc, &error_abort);
>>      smc->irq = &spapr_irq_dual;
>>      smc->dr_phb_enabled = true;
>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>> index eb54f9422722..398b72b77f9f 100644
>> --- a/hw/ppc/spapr_caps.c
>> +++ b/hw/ppc/spapr_caps.c
>> @@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>>      }
>>  }
>>  
>> +static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
>> +{
>> +    if (val) {
>> +        return;
>> +    }
>> +    warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is supported");
>> +}
>> +
>>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>      [SPAPR_CAP_HTM] = {
>>          .name = "htm",
>> @@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>          .type = "bool",
>>          .apply = cap_fwnmi_apply,
>>      },
>> +    [SPAPR_CAP_PVR] = {
>> +        .name = "pvr",
>> +        .description = "Enforce PVR in KVM",
>> +        .index = SPAPR_CAP_PVR,
>> +        .get = spapr_cap_get_bool,
>> +        .set = spapr_cap_set_bool,
>> +        .type = "bool",
>> +        .apply = cap_pvr_apply,
>> +    },
>>  };
>>  
>>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
>> @@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
>> +SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
>>  
>>  void spapr_caps_init(SpaprMachineState *spapr)
>>  {
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 03d0667e8f94..a4adc29b6522 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>>      CPUPPCState *cenv = &cpu->env;
>>      int ret;
>> +    SpaprMachineState *spapr;
>>  
> 
> We generally try to avoid adding such explicit dependencies to the
> machine code within the target directory... A virtual hypervisor
> hook could possibly do the trick but this would require to set
> PowerPCCPU::vhyp before kvm_arch_init_vcpu() gets called, eg.
> when the vCPU is created in spapr_create_vcpu() rather than
> when it gets realized.
> 
>>      /* Synchronize sregs with kvm */
>>      ret = kvm_arch_sync_sregs(cpu);
>>      if (ret) {
>>          if (ret == -EINVAL) {
>>              error_report("Register sync failed... If you're using kvm-hv.ko,"
>> -                         " only \"-cpu host\" is possible");
>> +                         " only \"-cpu host\" is supported");
>> +        }

we would still have the error above even if pvr=off ? 

C. 

>> +        /*
>> +         * The user chose not to set PVR which makes sense if we are running
>> +         * on a CPU with known ISA level but unknown PVR.
>> +         */
>> +        spapr = (SpaprMachineState *)
>> +            object_dynamic_cast(OBJECT(qdev_get_machine()), TYPE_SPAPR_MACHINE);
>> +
>> +        if (spapr && spapr->eff.caps[SPAPR_CAP_PVR] == SPAPR_CAP_OFF) {
>> +            ret = 0;
>> +        } else {
>> +            return ret;
>>          }
>> -        return ret;
>>      }
>>  
>>      switch (cenv->mmu_model) {
> 
> 

Re: [PATCH qemu] spapr: Add PVR setting capability

[Alexey Kardashevskiy]

On 04/05/2020 21:38, Cédric Le Goater wrote:
> 
> 
> On 5/4/20 1:30 PM, Greg Kurz wrote:
>> On Fri, 17 Apr 2020 14:11:05 +1000
>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>> At the moment the VCPU init sequence includes setting PVR which in case of
>>> KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
>>> virtualized by the hardware. In order to cope with various CPU revisions
>>> only top 16bit of PVR are checked which works for minor revision updates.
>>>
>>> However in every CPU generation starting POWER7 (at least) there were CPUs
>>> supporting the (almost) same POWER ISA level but having different top
>>> 16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
>>> with a new PVR family. We would normally add the PVR mask for the new one
>>> too, the problem with it is that although the physical machines exist,
>>> P9+ is not going to be released as a product, and this situation is likely
>>> to repeat in the future.
>>>
>>> Instead of adding every new CPU family in QEMU, this adds a new sPAPR
>>> machine capability to force PVR setting/checking. It is "on" by default
>>> to preserve the existing behavior. When "off", it is the user's
>>> responsibility to specify the correct CPU.
>>>
>>
>> I don't quite understand the motivation for this... what does this
>> buy us ?
> 
> So we could use the command line options : 
> 
>  -cpu POWER8 -machine pseries,pvr=off
> 
> instead of
> 
>  -cpu host -machine pseries,max-cpu-compat=POWER8  
> 
> is that it ? 

This does not work for my case.

> 
> I am not sure I get it either.


QEMU has to know the host CPU family to work with HV KVM: QEMU reads
pvr, picks a CPU class and goes from there, the max-compat applies lot
later and does not affect the CPU class which QEMU chooses.

Now I have a machine ("swift") with 0x004F1100 and QEMU has no idea what
0x004Fxxxx is (but I know it is POWER9) and there is currently no way to
bypass the PVR check which QEMU requests from KVM which KVM PR passes
but KVM HV fails as it has to match. Adding a new CPU family to the
upstream QEMU makes no sense as there will only be a handful of those
ever. And forcing PVR does not buy us that much anyway.




> 
> 
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>  include/hw/ppc/spapr.h |  5 ++++-
>>>  hw/ppc/spapr.c         |  1 +
>>>  hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
>>>  target/ppc/kvm.c       | 16 ++++++++++++++--
>>>  4 files changed, 37 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>> index e579eaf28c05..5ccac4d56871 100644
>>> --- a/include/hw/ppc/spapr.h
>>> +++ b/include/hw/ppc/spapr.h
>>> @@ -81,8 +81,10 @@ typedef enum {
>>>  #define SPAPR_CAP_CCF_ASSIST            0x09
>>>  /* Implements PAPR FWNMI option */
>>>  #define SPAPR_CAP_FWNMI                 0x0A
>>> +/* Implements PAPR PVR option */
>>> +#define SPAPR_CAP_PVR                   0x0B
>>>  /* Num Caps */
>>> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
>>> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_PVR + 1)
>>>  
>>>  /*
>>>   * Capability Values
>>> @@ -912,6 +914,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
>>>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
>>>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>>>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>>> +extern const VMStateDescription vmstate_spapr_cap_pvr;
>>>  
>>>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>>>  {
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 841b5ec59b12..ecc74c182b9f 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>>>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>>>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>>>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
>>> +    smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
>>>      spapr_caps_add_properties(smc, &error_abort);
>>>      smc->irq = &spapr_irq_dual;
>>>      smc->dr_phb_enabled = true;
>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>> index eb54f9422722..398b72b77f9f 100644
>>> --- a/hw/ppc/spapr_caps.c
>>> +++ b/hw/ppc/spapr_caps.c
>>> @@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>>>      }
>>>  }
>>>  
>>> +static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
>>> +{
>>> +    if (val) {
>>> +        return;
>>> +    }
>>> +    warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is supported");
>>> +}
>>> +
>>>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>>      [SPAPR_CAP_HTM] = {
>>>          .name = "htm",
>>> @@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>>          .type = "bool",
>>>          .apply = cap_fwnmi_apply,
>>>      },
>>> +    [SPAPR_CAP_PVR] = {
>>> +        .name = "pvr",
>>> +        .description = "Enforce PVR in KVM",
>>> +        .index = SPAPR_CAP_PVR,
>>> +        .get = spapr_cap_get_bool,
>>> +        .set = spapr_cap_set_bool,
>>> +        .type = "bool",
>>> +        .apply = cap_pvr_apply,
>>> +    },
>>>  };
>>>  
>>>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
>>> @@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>>>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>>>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>>>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
>>> +SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
>>>  
>>>  void spapr_caps_init(SpaprMachineState *spapr)
>>>  {
>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>> index 03d0667e8f94..a4adc29b6522 100644
>>> --- a/target/ppc/kvm.c
>>> +++ b/target/ppc/kvm.c
>>> @@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>      CPUPPCState *cenv = &cpu->env;
>>>      int ret;
>>> +    SpaprMachineState *spapr;
>>>  
>>
>> We generally try to avoid adding such explicit dependencies to the
>> machine code within the target directory... A virtual hypervisor
>> hook could possibly do the trick but this would require to set
>> PowerPCCPU::vhyp before kvm_arch_init_vcpu() gets called, eg.
>> when the vCPU is created in spapr_create_vcpu() rather than
>> when it gets realized.
>>
>>>      /* Synchronize sregs with kvm */
>>>      ret = kvm_arch_sync_sregs(cpu);
>>>      if (ret) {
>>>          if (ret == -EINVAL) {
>>>              error_report("Register sync failed... If you're using kvm-hv.ko,"
>>> -                         " only \"-cpu host\" is possible");
>>> +                         " only \"-cpu host\" is supported");
>>> +        }
> 
> we would still have the error above even if pvr=off ?


Yup. We would still print a message but continue, for the moment it is a
debugging tool. Thanks,



> 
> C. 
> 
>>> +        /*
>>> +         * The user chose not to set PVR which makes sense if we are running
>>> +         * on a CPU with known ISA level but unknown PVR.
>>> +         */
>>> +        spapr = (SpaprMachineState *)
>>> +            object_dynamic_cast(OBJECT(qdev_get_machine()), TYPE_SPAPR_MACHINE);
>>> +
>>> +        if (spapr && spapr->eff.caps[SPAPR_CAP_PVR] == SPAPR_CAP_OFF) {
>>> +            ret = 0;
>>> +        } else {
>>> +            return ret;
>>>          }
>>> -        return ret;
>>>      }
>>>  
>>>      switch (cenv->mmu_model) {
>>
>>

-- 
Alexey

Re: [PATCH qemu] spapr: Add PVR setting capability

[Alexey Kardashevskiy]

On 04/05/2020 21:30, Greg Kurz wrote:
> On Fri, 17 Apr 2020 14:11:05 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> At the moment the VCPU init sequence includes setting PVR which in case of
>> KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
>> virtualized by the hardware. In order to cope with various CPU revisions
>> only top 16bit of PVR are checked which works for minor revision updates.
>>
>> However in every CPU generation starting POWER7 (at least) there were CPUs
>> supporting the (almost) same POWER ISA level but having different top
>> 16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
>> with a new PVR family. We would normally add the PVR mask for the new one
>> too, the problem with it is that although the physical machines exist,
>> P9+ is not going to be released as a product, and this situation is likely
>> to repeat in the future.
>>
>> Instead of adding every new CPU family in QEMU, this adds a new sPAPR
>> machine capability to force PVR setting/checking. It is "on" by default
>> to preserve the existing behavior. When "off", it is the user's
>> responsibility to specify the correct CPU.
>>
> 
> I don't quite understand the motivation for this... what does this
> buy us ?

I answered that part in another mail in this thread, shortly this is to
make QEMU work with HV KVM on unknown-to-QEMU CPU family (0x004f0000).


> 
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  include/hw/ppc/spapr.h |  5 ++++-
>>  hw/ppc/spapr.c         |  1 +
>>  hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
>>  target/ppc/kvm.c       | 16 ++++++++++++++--
>>  4 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index e579eaf28c05..5ccac4d56871 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -81,8 +81,10 @@ typedef enum {
>>  #define SPAPR_CAP_CCF_ASSIST            0x09
>>  /* Implements PAPR FWNMI option */
>>  #define SPAPR_CAP_FWNMI                 0x0A
>> +/* Implements PAPR PVR option */
>> +#define SPAPR_CAP_PVR                   0x0B
>>  /* Num Caps */
>> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
>> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_PVR + 1)
>>  
>>  /*
>>   * Capability Values
>> @@ -912,6 +914,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
>>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
>>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>> +extern const VMStateDescription vmstate_spapr_cap_pvr;
>>  
>>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>>  {
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 841b5ec59b12..ecc74c182b9f 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
>> +    smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
>>      spapr_caps_add_properties(smc, &error_abort);
>>      smc->irq = &spapr_irq_dual;
>>      smc->dr_phb_enabled = true;
>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>> index eb54f9422722..398b72b77f9f 100644
>> --- a/hw/ppc/spapr_caps.c
>> +++ b/hw/ppc/spapr_caps.c
>> @@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>>      }
>>  }
>>  
>> +static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
>> +{
>> +    if (val) {
>> +        return;
>> +    }
>> +    warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is supported");
>> +}
>> +
>>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>      [SPAPR_CAP_HTM] = {
>>          .name = "htm",
>> @@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>          .type = "bool",
>>          .apply = cap_fwnmi_apply,
>>      },
>> +    [SPAPR_CAP_PVR] = {
>> +        .name = "pvr",
>> +        .description = "Enforce PVR in KVM",
>> +        .index = SPAPR_CAP_PVR,
>> +        .get = spapr_cap_get_bool,
>> +        .set = spapr_cap_set_bool,
>> +        .type = "bool",
>> +        .apply = cap_pvr_apply,
>> +    },
>>  };
>>  
>>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
>> @@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
>> +SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
>>  
>>  void spapr_caps_init(SpaprMachineState *spapr)
>>  {
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 03d0667e8f94..a4adc29b6522 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>>      CPUPPCState *cenv = &cpu->env;
>>      int ret;
>> +    SpaprMachineState *spapr;
>>  
> 
> We generally try to avoid adding such explicit dependencies to the
> machine code within the target directory... A virtual hypervisor
> hook could possibly do the trick but this would require to set
> PowerPCCPU::vhyp before kvm_arch_init_vcpu() gets called, eg.
> when the vCPU is created in spapr_create_vcpu() rather than
> when it gets realized.

The only thing kvm_arch_sync_sregs() does is setting PVR, and it does
not do even that for Book3E so there is dependency already, mine at
least explicit :)

I am really not sure what setting PVR buys us, KVM PR will take
anything, KVM HV will only take the same family PVR and reject others
while it could still run more configurations, let's say -cpu POWER8 on
actual POWER9 machine. Dunno. The capability seems a harmless way of
relaxing this restriction. Thanks,




> 
>>      /* Synchronize sregs with kvm */
>>      ret = kvm_arch_sync_sregs(cpu);
>>      if (ret) {
>>          if (ret == -EINVAL) {
>>              error_report("Register sync failed... If you're using kvm-hv.ko,"
>> -                         " only \"-cpu host\" is possible");
>> +                         " only \"-cpu host\" is supported");
>> +        }
>> +        /*
>> +         * The user chose not to set PVR which makes sense if we are running
>> +         * on a CPU with known ISA level but unknown PVR.
>> +         */
>> +        spapr = (SpaprMachineState *)
>> +            object_dynamic_cast(OBJECT(qdev_get_machine()), TYPE_SPAPR_MACHINE);
>> +
>> +        if (spapr && spapr->eff.caps[SPAPR_CAP_PVR] == SPAPR_CAP_OFF) {
>> +            ret = 0;
>> +        } else {
>> +            return ret;
>>          }
>> -        return ret;
>>      }
>>  
>>      switch (cenv->mmu_model) {
> 

-- 
Alexey

Re: [PATCH qemu] spapr: Add PVR setting capability

[David Gibson]

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

On Tue, May 05, 2020 at 10:56:17AM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 04/05/2020 21:30, Greg Kurz wrote:
> > On Fri, 17 Apr 2020 14:11:05 +1000
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > 
> >> At the moment the VCPU init sequence includes setting PVR which in case of
> >> KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
> >> virtualized by the hardware. In order to cope with various CPU revisions
> >> only top 16bit of PVR are checked which works for minor revision updates.
> >>
> >> However in every CPU generation starting POWER7 (at least) there were CPUs
> >> supporting the (almost) same POWER ISA level but having different top
> >> 16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
> >> with a new PVR family. We would normally add the PVR mask for the new one
> >> too, the problem with it is that although the physical machines exist,
> >> P9+ is not going to be released as a product, and this situation is likely
> >> to repeat in the future.
> >>
> >> Instead of adding every new CPU family in QEMU, this adds a new sPAPR
> >> machine capability to force PVR setting/checking. It is "on" by default
> >> to preserve the existing behavior. When "off", it is the user's
> >> responsibility to specify the correct CPU.
> >>
> > 
> > I don't quite understand the motivation for this... what does this
> > buy us ?
> 
> I answered that part in another mail in this thread, shortly this is to
> make QEMU work with HV KVM on unknown-to-QEMU CPU family (0x004f0000).
> 
> 
> > 
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >>  include/hw/ppc/spapr.h |  5 ++++-
> >>  hw/ppc/spapr.c         |  1 +
> >>  hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
> >>  target/ppc/kvm.c       | 16 ++++++++++++++--
> >>  4 files changed, 37 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index e579eaf28c05..5ccac4d56871 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -81,8 +81,10 @@ typedef enum {
> >>  #define SPAPR_CAP_CCF_ASSIST            0x09
> >>  /* Implements PAPR FWNMI option */
> >>  #define SPAPR_CAP_FWNMI                 0x0A
> >> +/* Implements PAPR PVR option */
> >> +#define SPAPR_CAP_PVR                   0x0B
> >>  /* Num Caps */
> >> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
> >> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_PVR + 1)
> >>  
> >>  /*
> >>   * Capability Values
> >> @@ -912,6 +914,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
> >>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
> >>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
> >>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
> >> +extern const VMStateDescription vmstate_spapr_cap_pvr;
> >>  
> >>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
> >>  {
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 841b5ec59b12..ecc74c182b9f 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> >>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> >>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
> >>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
> >> +    smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
> >>      spapr_caps_add_properties(smc, &error_abort);
> >>      smc->irq = &spapr_irq_dual;
> >>      smc->dr_phb_enabled = true;
> >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> >> index eb54f9422722..398b72b77f9f 100644
> >> --- a/hw/ppc/spapr_caps.c
> >> +++ b/hw/ppc/spapr_caps.c
> >> @@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
> >>      }
> >>  }
> >>  
> >> +static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
> >> +{
> >> +    if (val) {
> >> +        return;
> >> +    }
> >> +    warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is supported");
> >> +}
> >> +
> >>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> >>      [SPAPR_CAP_HTM] = {
> >>          .name = "htm",
> >> @@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> >>          .type = "bool",
> >>          .apply = cap_fwnmi_apply,
> >>      },
> >> +    [SPAPR_CAP_PVR] = {
> >> +        .name = "pvr",
> >> +        .description = "Enforce PVR in KVM",
> >> +        .index = SPAPR_CAP_PVR,
> >> +        .get = spapr_cap_get_bool,
> >> +        .set = spapr_cap_set_bool,
> >> +        .type = "bool",
> >> +        .apply = cap_pvr_apply,
> >> +    },
> >>  };
> >>  
> >>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> >> @@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
> >>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
> >>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
> >>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
> >> +SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
> >>  
> >>  void spapr_caps_init(SpaprMachineState *spapr)
> >>  {
> >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >> index 03d0667e8f94..a4adc29b6522 100644
> >> --- a/target/ppc/kvm.c
> >> +++ b/target/ppc/kvm.c
> >> @@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> >>      CPUPPCState *cenv = &cpu->env;
> >>      int ret;
> >> +    SpaprMachineState *spapr;
> >>  
> > 
> > We generally try to avoid adding such explicit dependencies to the
> > machine code within the target directory... A virtual hypervisor
> > hook could possibly do the trick but this would require to set
> > PowerPCCPU::vhyp before kvm_arch_init_vcpu() gets called, eg.
> > when the vCPU is created in spapr_create_vcpu() rather than
> > when it gets realized.
> 
> The only thing kvm_arch_sync_sregs() does is setting PVR, and it does
> not do even that for Book3E so there is dependency already, mine at
> least explicit :)
> 
> I am really not sure what setting PVR buys us, KVM PR will take
> anything,

PR will take anything, but it changes the behaviour.  Basically PR
will try to match its behaviour to the PVR you specify.  It can't
entirely succeeed in all cases, of course, but that's PR for you.

From an API point of view, the idea is that setting the PVR here
specifies the model you want the vcpu to appear to be.  But, KVM is
free to say "can't do that".

> KVM HV will only take the same family PVR and reject others
> while it could still run more configurations, let's say -cpu POWER8 on
> actual POWER9 machine. Dunno. The capability seems a harmless way of
> relaxing this restriction. Thanks,
> 
> 
> 
> 
> > 
> >>      /* Synchronize sregs with kvm */
> >>      ret = kvm_arch_sync_sregs(cpu);
> >>      if (ret) {
> >>          if (ret == -EINVAL) {
> >>              error_report("Register sync failed... If you're using kvm-hv.ko,"
> >> -                         " only \"-cpu host\" is possible");
> >> +                         " only \"-cpu host\" is supported");
> >> +        }
> >> +        /*
> >> +         * The user chose not to set PVR which makes sense if we are running
> >> +         * on a CPU with known ISA level but unknown PVR.
> >> +         */
> >> +        spapr = (SpaprMachineState *)
> >> +            object_dynamic_cast(OBJECT(qdev_get_machine()), TYPE_SPAPR_MACHINE);
> >> +
> >> +        if (spapr && spapr->eff.caps[SPAPR_CAP_PVR] == SPAPR_CAP_OFF) {
> >> +            ret = 0;
> >> +        } else {
> >> +            return ret;
> >>          }
> >> -        return ret;
> >>      }
> >>  
> >>      switch (cenv->mmu_model) {
> > 
> 

-- 
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: [PATCH qemu] spapr: Add PVR setting capability

[Alexey Kardashevskiy]

On 05/05/2020 15:50, David Gibson wrote:
> On Tue, May 05, 2020 at 10:56:17AM +1000, Alexey Kardashevskiy wrote:
>>
>>
>> On 04/05/2020 21:30, Greg Kurz wrote:
>>> On Fri, 17 Apr 2020 14:11:05 +1000
>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>>> At the moment the VCPU init sequence includes setting PVR which in case of
>>>> KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
>>>> virtualized by the hardware. In order to cope with various CPU revisions
>>>> only top 16bit of PVR are checked which works for minor revision updates.
>>>>
>>>> However in every CPU generation starting POWER7 (at least) there were CPUs
>>>> supporting the (almost) same POWER ISA level but having different top
>>>> 16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
>>>> with a new PVR family. We would normally add the PVR mask for the new one
>>>> too, the problem with it is that although the physical machines exist,
>>>> P9+ is not going to be released as a product, and this situation is likely
>>>> to repeat in the future.
>>>>
>>>> Instead of adding every new CPU family in QEMU, this adds a new sPAPR
>>>> machine capability to force PVR setting/checking. It is "on" by default
>>>> to preserve the existing behavior. When "off", it is the user's
>>>> responsibility to specify the correct CPU.
>>>>
>>>
>>> I don't quite understand the motivation for this... what does this
>>> buy us ?
>>
>> I answered that part in another mail in this thread, shortly this is to
>> make QEMU work with HV KVM on unknown-to-QEMU CPU family (0x004f0000).
>>
>>
>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>>  include/hw/ppc/spapr.h |  5 ++++-
>>>>  hw/ppc/spapr.c         |  1 +
>>>>  hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
>>>>  target/ppc/kvm.c       | 16 ++++++++++++++--
>>>>  4 files changed, 37 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index e579eaf28c05..5ccac4d56871 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -81,8 +81,10 @@ typedef enum {
>>>>  #define SPAPR_CAP_CCF_ASSIST            0x09
>>>>  /* Implements PAPR FWNMI option */
>>>>  #define SPAPR_CAP_FWNMI                 0x0A
>>>> +/* Implements PAPR PVR option */
>>>> +#define SPAPR_CAP_PVR                   0x0B
>>>>  /* Num Caps */
>>>> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
>>>> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_PVR + 1)
>>>>  
>>>>  /*
>>>>   * Capability Values
>>>> @@ -912,6 +914,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
>>>>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
>>>>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>>>>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>>>> +extern const VMStateDescription vmstate_spapr_cap_pvr;
>>>>  
>>>>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>>>>  {
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 841b5ec59b12..ecc74c182b9f 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>>>>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>>>>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>>>>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
>>>> +    smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
>>>>      spapr_caps_add_properties(smc, &error_abort);
>>>>      smc->irq = &spapr_irq_dual;
>>>>      smc->dr_phb_enabled = true;
>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>>> index eb54f9422722..398b72b77f9f 100644
>>>> --- a/hw/ppc/spapr_caps.c
>>>> +++ b/hw/ppc/spapr_caps.c
>>>> @@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>>>>      }
>>>>  }
>>>>  
>>>> +static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
>>>> +{
>>>> +    if (val) {
>>>> +        return;
>>>> +    }
>>>> +    warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is supported");
>>>> +}
>>>> +
>>>>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>>>      [SPAPR_CAP_HTM] = {
>>>>          .name = "htm",
>>>> @@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>>>          .type = "bool",
>>>>          .apply = cap_fwnmi_apply,
>>>>      },
>>>> +    [SPAPR_CAP_PVR] = {
>>>> +        .name = "pvr",
>>>> +        .description = "Enforce PVR in KVM",
>>>> +        .index = SPAPR_CAP_PVR,
>>>> +        .get = spapr_cap_get_bool,
>>>> +        .set = spapr_cap_set_bool,
>>>> +        .type = "bool",
>>>> +        .apply = cap_pvr_apply,
>>>> +    },
>>>>  };
>>>>  
>>>>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
>>>> @@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>>>>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>>>>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>>>>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
>>>> +SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
>>>>  
>>>>  void spapr_caps_init(SpaprMachineState *spapr)
>>>>  {
>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>> index 03d0667e8f94..a4adc29b6522 100644
>>>> --- a/target/ppc/kvm.c
>>>> +++ b/target/ppc/kvm.c
>>>> @@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>>      CPUPPCState *cenv = &cpu->env;
>>>>      int ret;
>>>> +    SpaprMachineState *spapr;
>>>>  
>>>
>>> We generally try to avoid adding such explicit dependencies to the
>>> machine code within the target directory... A virtual hypervisor
>>> hook could possibly do the trick but this would require to set
>>> PowerPCCPU::vhyp before kvm_arch_init_vcpu() gets called, eg.
>>> when the vCPU is created in spapr_create_vcpu() rather than
>>> when it gets realized.
>>
>> The only thing kvm_arch_sync_sregs() does is setting PVR, and it does
>> not do even that for Book3E so there is dependency already, mine at
>> least explicit :)
>>
>> I am really not sure what setting PVR buys us, KVM PR will take
>> anything,
> 
> PR will take anything, but it changes the behaviour.  Basically PR
> will try to match its behaviour to the PVR you specify.  It can't
> entirely succeeed in all cases, of course, but that's PR for you.
>
> From an API point of view, the idea is that setting the PVR here
> specifies the model you want the vcpu to appear to be.  But, KVM is
> free to say "can't do that".


Except pseries does not care what the CPU appears to be and uses
KVM_REG_PPC_ARCH_COMPAT instead.

So, is this a no? Thanks,


> 
>> KVM HV will only take the same family PVR and reject others
>> while it could still run more configurations, let's say -cpu POWER8 on
>> actual POWER9 machine. Dunno. The capability seems a harmless way of
>> relaxing this restriction. Thanks,
>>
>>
>>
>>
>>>
>>>>      /* Synchronize sregs with kvm */
>>>>      ret = kvm_arch_sync_sregs(cpu);
>>>>      if (ret) {
>>>>          if (ret == -EINVAL) {
>>>>              error_report("Register sync failed... If you're using kvm-hv.ko,"
>>>> -                         " only \"-cpu host\" is possible");
>>>> +                         " only \"-cpu host\" is supported");
>>>> +        }
>>>> +        /*
>>>> +         * The user chose not to set PVR which makes sense if we are running
>>>> +         * on a CPU with known ISA level but unknown PVR.
>>>> +         */
>>>> +        spapr = (SpaprMachineState *)
>>>> +            object_dynamic_cast(OBJECT(qdev_get_machine()), TYPE_SPAPR_MACHINE);
>>>> +
>>>> +        if (spapr && spapr->eff.caps[SPAPR_CAP_PVR] == SPAPR_CAP_OFF) {
>>>> +            ret = 0;
>>>> +        } else {
>>>> +            return ret;
>>>>          }
>>>> -        return ret;
>>>>      }
>>>>  
>>>>      switch (cenv->mmu_model) {
>>>
>>
> 

-- 
Alexey

Re: [PATCH qemu] spapr: Add PVR setting capability

[David Gibson]

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

On Tue, May 05, 2020 at 04:18:40PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 05/05/2020 15:50, David Gibson wrote:
> > On Tue, May 05, 2020 at 10:56:17AM +1000, Alexey Kardashevskiy wrote:
> >>
> >>
> >> On 04/05/2020 21:30, Greg Kurz wrote:
> >>> On Fri, 17 Apr 2020 14:11:05 +1000
> >>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>
> >>>> At the moment the VCPU init sequence includes setting PVR which in case of
> >>>> KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
> >>>> virtualized by the hardware. In order to cope with various CPU revisions
> >>>> only top 16bit of PVR are checked which works for minor revision updates.
> >>>>
> >>>> However in every CPU generation starting POWER7 (at least) there were CPUs
> >>>> supporting the (almost) same POWER ISA level but having different top
> >>>> 16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
> >>>> with a new PVR family. We would normally add the PVR mask for the new one
> >>>> too, the problem with it is that although the physical machines exist,
> >>>> P9+ is not going to be released as a product, and this situation is likely
> >>>> to repeat in the future.
> >>>>
> >>>> Instead of adding every new CPU family in QEMU, this adds a new sPAPR
> >>>> machine capability to force PVR setting/checking. It is "on" by default
> >>>> to preserve the existing behavior. When "off", it is the user's
> >>>> responsibility to specify the correct CPU.
> >>>>
> >>>
> >>> I don't quite understand the motivation for this... what does this
> >>> buy us ?
> >>
> >> I answered that part in another mail in this thread, shortly this is to
> >> make QEMU work with HV KVM on unknown-to-QEMU CPU family (0x004f0000).
> >>
> >>
> >>>
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> ---
> >>>>  include/hw/ppc/spapr.h |  5 ++++-
> >>>>  hw/ppc/spapr.c         |  1 +
> >>>>  hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
> >>>>  target/ppc/kvm.c       | 16 ++++++++++++++--
> >>>>  4 files changed, 37 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >>>> index e579eaf28c05..5ccac4d56871 100644
> >>>> --- a/include/hw/ppc/spapr.h
> >>>> +++ b/include/hw/ppc/spapr.h
> >>>> @@ -81,8 +81,10 @@ typedef enum {
> >>>>  #define SPAPR_CAP_CCF_ASSIST            0x09
> >>>>  /* Implements PAPR FWNMI option */
> >>>>  #define SPAPR_CAP_FWNMI                 0x0A
> >>>> +/* Implements PAPR PVR option */
> >>>> +#define SPAPR_CAP_PVR                   0x0B
> >>>>  /* Num Caps */
> >>>> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
> >>>> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_PVR + 1)
> >>>>  
> >>>>  /*
> >>>>   * Capability Values
> >>>> @@ -912,6 +914,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
> >>>>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
> >>>>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
> >>>>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
> >>>> +extern const VMStateDescription vmstate_spapr_cap_pvr;
> >>>>  
> >>>>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
> >>>>  {
> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>> index 841b5ec59b12..ecc74c182b9f 100644
> >>>> --- a/hw/ppc/spapr.c
> >>>> +++ b/hw/ppc/spapr.c
> >>>> @@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> >>>>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> >>>>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
> >>>>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
> >>>> +    smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
> >>>>      spapr_caps_add_properties(smc, &error_abort);
> >>>>      smc->irq = &spapr_irq_dual;
> >>>>      smc->dr_phb_enabled = true;
> >>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> >>>> index eb54f9422722..398b72b77f9f 100644
> >>>> --- a/hw/ppc/spapr_caps.c
> >>>> +++ b/hw/ppc/spapr_caps.c
> >>>> @@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
> >>>>      }
> >>>>  }
> >>>>  
> >>>> +static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
> >>>> +{
> >>>> +    if (val) {
> >>>> +        return;
> >>>> +    }
> >>>> +    warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is supported");
> >>>> +}
> >>>> +
> >>>>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> >>>>      [SPAPR_CAP_HTM] = {
> >>>>          .name = "htm",
> >>>> @@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> >>>>          .type = "bool",
> >>>>          .apply = cap_fwnmi_apply,
> >>>>      },
> >>>> +    [SPAPR_CAP_PVR] = {
> >>>> +        .name = "pvr",
> >>>> +        .description = "Enforce PVR in KVM",
> >>>> +        .index = SPAPR_CAP_PVR,
> >>>> +        .get = spapr_cap_get_bool,
> >>>> +        .set = spapr_cap_set_bool,
> >>>> +        .type = "bool",
> >>>> +        .apply = cap_pvr_apply,
> >>>> +    },
> >>>>  };
> >>>>  
> >>>>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> >>>> @@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
> >>>>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
> >>>>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
> >>>>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
> >>>> +SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
> >>>>  
> >>>>  void spapr_caps_init(SpaprMachineState *spapr)
> >>>>  {
> >>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >>>> index 03d0667e8f94..a4adc29b6522 100644
> >>>> --- a/target/ppc/kvm.c
> >>>> +++ b/target/ppc/kvm.c
> >>>> @@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >>>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> >>>>      CPUPPCState *cenv = &cpu->env;
> >>>>      int ret;
> >>>> +    SpaprMachineState *spapr;
> >>>>  
> >>>
> >>> We generally try to avoid adding such explicit dependencies to the
> >>> machine code within the target directory... A virtual hypervisor
> >>> hook could possibly do the trick but this would require to set
> >>> PowerPCCPU::vhyp before kvm_arch_init_vcpu() gets called, eg.
> >>> when the vCPU is created in spapr_create_vcpu() rather than
> >>> when it gets realized.
> >>
> >> The only thing kvm_arch_sync_sregs() does is setting PVR, and it does
> >> not do even that for Book3E so there is dependency already, mine at
> >> least explicit :)
> >>
> >> I am really not sure what setting PVR buys us, KVM PR will take
> >> anything,
> > 
> > PR will take anything, but it changes the behaviour.  Basically PR
> > will try to match its behaviour to the PVR you specify.  It can't
> > entirely succeeed in all cases, of course, but that's PR for you.
> >
> > From an API point of view, the idea is that setting the PVR here
> > specifies the model you want the vcpu to appear to be.  But, KVM is
> > free to say "can't do that".
> 
> 
> Except pseries does not care what the CPU appears to be and uses
> KVM_REG_PPC_ARCH_COMPAT instead.

I'm not really sure what you mean by that.  The exact cpu model can be
relevant for pseries in certain edge cases, although in most cases
it's the compat mode that will matter in practice.

> So, is this a no? Thanks,

Yes, although not really for the reasons above.  As you say this is
just for the handful of people who have never-released machines, which
makes this some ugly and confusing cruft for nearly everyone.  So, I'm
more inclined to say that if you happen to have one of those machines,
you get to apply the workaround patch yourself.

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