On Fri, 17 Apr 2020 14:11:05 +1000
Alexey Kardashevskiy <a...@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 <a...@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) {
