Re: [Qemu-devel] [PATCH v3 2/3] s390x/kvm: Handle bpb feature

[Halil Pasic]

On 01/18/2018 09:56 AM, Christian Borntraeger wrote:
> We need to handle the bpb control on reset and migration. Normally
> stfle.82 is transparent (and the normal guest part works without
> hypervisor activity). To prevent any issues we require full
> host kernel support for this feature.
> 
> Signed-off-by: Christian Borntraeger <address@hidden>
> ---
>  target/s390x/cpu.c              |  1 +
>  target/s390x/cpu.h              |  1 +
>  target/s390x/cpu_features.c     |  1 +
>  target/s390x/cpu_features_def.h |  1 +
>  target/s390x/gen-features.c     |  1 +
>  target/s390x/kvm.c              | 14 ++++++++++++++
>  target/s390x/machine.c          | 17 +++++++++++++++++
>  7 files changed, 36 insertions(+)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index ae3cee9..d2e6b9f 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -89,6 +89,7 @@ static void s390_cpu_reset(CPUState *s)
>      CPUS390XState *env = &cpu->env;
> 
>      env->pfault_token = -1UL;
> +    env->bpbc = false;
>      scc->parent_reset(s);
>      cpu->env.sigp_order = 0;
>      s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 1a8b6b9..c0ef85d 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -93,6 +93,7 @@ struct CPUS390XState {
> 
>      uint32_t fpc;          /* floating-point control register */
>      uint32_t cc_op;
> +    bool bpbc;             /* branch prediction blocking */
> 
>      float_status fpu_status; /* passed to softfloat lib */
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 31a4676..5d1c210 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -89,6 +89,7 @@ static const S390FeatDef s390_features[] = {
>      FEAT_INIT("msa4-base", S390_FEAT_TYPE_STFL, 77, 
> "Message-security-assist-extension-4 facility (excluding subfunctions)"),
>      FEAT_INIT("edat2", S390_FEAT_TYPE_STFL, 78, "Enhanced-DAT facility 2"),
>      FEAT_INIT("dfppc", S390_FEAT_TYPE_STFL, 80, "Decimal-floating-point 
> packed-conversion facility"),
> +    FEAT_INIT("bpb", S390_FEAT_TYPE_STFL, 82, "Branch Prediction Blocking"),
>      FEAT_INIT("vx", S390_FEAT_TYPE_STFL, 129, "Vector facility"),
>      FEAT_INIT("iep", S390_FEAT_TYPE_STFL, 130, 
> "Instruction-execution-protection facility"),
>      FEAT_INIT("sea_esop2", S390_FEAT_TYPE_STFL, 131, "Side-effect-access 
> facility and Enhanced-suppression-on-protection facility 2"),
> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
> index 4b6d4e9..4487cfd 100644
> --- a/target/s390x/cpu_features_def.h
> +++ b/target/s390x/cpu_features_def.h
> @@ -80,6 +80,7 @@ typedef enum {
>      S390_FEAT_MSA_EXT_4,
>      S390_FEAT_EDAT_2,
>      S390_FEAT_DFP_PACKED_CONVERSION,
> +    S390_FEAT_BPB,
>      S390_FEAT_VECTOR,
>      S390_FEAT_INSTRUCTION_EXEC_PROT,
>      S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index b24f6ad..563aced 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -352,6 +352,7 @@ static uint16_t base_GEN14_GA1[] = {
>   * support these features yet.
>   */
>  static uint16_t full_GEN7_GA1[] = {
> +    S390_FEAT_BPB,
>      S390_FEAT_SIE_F2,
>      S390_FEAT_SIE_SKEY,
>      S390_FEAT_SIE_GPERE,
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 6a18a41..8736001 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -490,6 +490,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_GSCB;
>      }
> 
> +    if (can_sync_regs(cs, KVM_SYNC_BPBC)) {

This is for compat machines <= 2.7, or? Should the guest of these get the
STFLE bit 82 regardless of the outcome of kvm_check_extension(kvm_state, 
KVM_CAP_S390_BPB)
if we have STFLE bit 82 in the host?

> +        cs->kvm_run->s.regs.bpbc = env->bpbc;
> +        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_BPBC;
> +    }
> +
>      /* Finally the prefix */
>      if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
>          cs->kvm_run->s.regs.prefix = env->psa;
> @@ -600,6 +605,10 @@ int kvm_arch_get_registers(CPUState *cs)
>          memcpy(env->gscb, cs->kvm_run->s.regs.gscb, 32);
>      }
> 
> +    if (can_sync_regs(cs, KVM_SYNC_BPBC)) {
> +        env->bpbc = cs->kvm_run->s.regs.bpbc;
> +    }
> +
>      /* pfault parameters */
>      if (can_sync_regs(cs, KVM_SYNC_PFAULT)) {
>          env->pfault_token = cs->kvm_run->s.regs.pft;
> @@ -2278,6 +2287,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, 
> Error **errp)
>          clear_bit(S390_FEAT_CMM_NT, model->features);
>      }
> 
> +    /* bpb needs kernel support for migration, VSIE and reset */

I would not mind being a little more verbose about the fact that
although this is a non-hyp bit it should have been a hyp manged bit,
and that is why we need the KVM_CAP_S390_BPB.

> +    if (!kvm_check_extension(kvm_state, KVM_CAP_S390_BPB)) {
> +        clear_bit(S390_FEAT_BPB, model->features);
> +    }
> +
>      /* We emulate a zPCI bus and AEN, therefore we don't need HW support */
>      if (pci_available) {
>          set_bit(S390_FEAT_ZPCI, model->features);
> diff --git a/target/s390x/machine.c b/target/s390x/machine.c
> index b78f326..84b4928 100644
> --- a/target/s390x/machine.c
> +++ b/target/s390x/machine.c
> @@ -194,6 +194,22 @@ const VMStateDescription vmstate_gscb = {
>          }
>  };
> 
> +static bool bpbc_needed(void *opaque)
> +{
> +    return s390_has_feat(S390_FEAT_BPB);
> +}
> +
> +const VMStateDescription vmstate_bpbc = {
> +    .name = "cpu/bpbc",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = bpbc_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(env.bpbc, S390CPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  const VMStateDescription vmstate_s390_cpu = {
>      .name = "cpu",
>      .post_load = cpu_post_load,
> @@ -228,6 +244,7 @@ const VMStateDescription vmstate_s390_cpu = {
>          &vmstate_riccb,
>          &vmstate_exval,
>          &vmstate_gscb,
> +        &vmstate_bpbc,
>          NULL
>      },
>  };
> 

My comments aren't anything major, so:

Acked-by: Halil Pasic <address@hidden>