Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Tony Krowiak]

On 02/27/2018 12:52 PM, David Hildenbrand wrote:
> >       vfio_group = vfio_ap_get_group(vapdev, &local_err);
> >       if (!vfio_group) {
> >           goto out_err;
> > diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
> > index 11def14..35a6d04 100644
> > --- a/linux-headers/asm-s390/kvm.h
> > +++ b/linux-headers/asm-s390/kvm.h
> > @@ -130,6 +130,7 @@ struct kvm_s390_vm_cpu_machine {
> >   #define KVM_S390_VM_CPU_FEAT_PFMFI    11
> >   #define KVM_S390_VM_CPU_FEAT_SIGPIF   12
> >   #define KVM_S390_VM_CPU_FEAT_KSS      13
> > +#define KVM_S390_VM_CPU_FEAT_AP                14
> >   struct kvm_s390_vm_cpu_feat {
> >         __u64 feat[16];
> >   };
> > diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> > index a5619f2..65b91bd 100644
> > --- a/target/s390x/cpu_features.c
> > +++ b/target/s390x/cpu_features.c
> > @@ -36,8 +36,10 @@ static const S390FeatDef s390_features[] = {
> >       FEAT_INIT("srs", S390_FEAT_TYPE_STFL, 9, "Sense-running-status facility"),
> >       FEAT_INIT("csske", S390_FEAT_TYPE_STFL, 10, "Conditional-SSKE facility"),
> >       FEAT_INIT("ctop", S390_FEAT_TYPE_STFL, 11, "Configuration-topology 
> > facility"),
> > +    FEAT_INIT("qci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration 
> > facility"),
> >       FEAT_INIT("ipter", S390_FEAT_TYPE_STFL, 13, "IPTE-range facility"),
> >       FEAT_INIT("nonqks", S390_FEAT_TYPE_STFL, 14, "Nonquiescing key-setting 
> > facility"),
> > +    FEAT_INIT("apft", S390_FEAT_TYPE_STFL, 15, "Adjunct Processor Facilities Test 
> > facility"),
> >       FEAT_INIT("etf2", S390_FEAT_TYPE_STFL, 16, "Extended-translation facility 
> > 2"),
> >       FEAT_INIT("msa-base", S390_FEAT_TYPE_STFL, 17, "Message-security-assist 
> > facility (excluding subfunctions)"),
> >       FEAT_INIT("ldisp", S390_FEAT_TYPE_STFL, 18, "Long-displacement facility"),
> > @@ -125,6 +127,7 @@ static const S390FeatDef s390_features[] = {
> >   
> >       FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 2"),
> >       FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, "Collaborative-memory-management 
> > facility"),
> > +    FEAT_INIT("ap", S390_FEAT_TYPE_MISC, 1, "AP facilities installed"),
> How exactly is this feature communicated to the guest? How does KVM
> sense support for it?
KVM detects whether the AP instructions are installed on the host. If
the instructions are installed, the feature is allowed (enabled) and
can be turned on by userspace (QEMU).
> IOW: is this really a CPU model feature?
I believe it is a necessary feature and came about due to review comments
from Christian and Connie in v1.
> >   
> >       FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
> >       FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in 
> > parameter list)"),
> > diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
> > index 7c5915c..8998b65 100644
> > --- a/target/s390x/cpu_features_def.h
> > +++ b/target/s390x/cpu_features_def.h
> > @@ -27,8 +27,10 @@ typedef enum {
> >       S390_FEAT_SENSE_RUNNING_STATUS,
> >       S390_FEAT_CONDITIONAL_SSKE,
> >       S390_FEAT_CONFIGURATION_TOPOLOGY,
> > +    S390_FEAT_AP_QUERY_CONFIG_INFO,
> >       S390_FEAT_IPTE_RANGE,
> >       S390_FEAT_NONQ_KEY_SETTING,
> > +    S390_FEAT_AP_FACILITIES_TEST,
> >       S390_FEAT_EXTENDED_TRANSLATION_2,
> >       S390_FEAT_MSA,
> >       S390_FEAT_LONG_DISPLACEMENT,
> > @@ -118,6 +120,7 @@ typedef enum {
> >       /* Misc */
> >       S390_FEAT_DAT_ENH_2,
> >       S390_FEAT_CMM,
> > +    S390_FEAT_AP,
> >   
> >       /* PLO */
> >       S390_FEAT_PLO_CL,
> > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> > index 1d5f0da..35f91ea 100644
> > --- a/target/s390x/cpu_models.c
> > +++ b/target/s390x/cpu_models.c
> > @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
> >           { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
> >           { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
> >           { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
> > +        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
> > +        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
> >       };
> >       int i;
> >   
> > @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
> >       cpu->model->cpu_id_format = max_model->cpu_id_format;
> >       cpu->model->cpu_ver = max_model->cpu_ver;
> >   
> > +    /*
> > +     * If the AP facilities are not installed on the guest, then it makes
> > +     * no sense to enable the QCI or APFT facilities because they are only
> > +     * needed by AP facilities.
> > +     */
> > +    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
> > +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
> > +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
> > +    }
> Please don't silently disable things. Instead
>
> a) Add consistency checks (check_consistency())
> b) Mask the bits out in the KVM CPU model sensing part
>    (kvm_s390_get_host_cpu_model()) - which you already have :)
This is a remnant of a previous iteration that somehow made its way into
this patch series. It will be removed.
> > +
> >       check_consistency(cpu->model);
> >       check_compatibility(max_model, cpu->model, errp);
> >       if (*errp) {
> > diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> > index 0cdbc15..2d01b52 100644
> > --- a/target/s390x/gen-features.c
> > +++ b/target/s390x/gen-features.c
> > @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
> >       S390_FEAT_ADAPTER_INT_SUPPRESSION,
> >       S390_FEAT_EDAT_2,
> >       S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
> > +    S390_FEAT_AP,
> > +    S390_FEAT_AP_QUERY_CONFIG_INFO,
> > +    S390_FEAT_AP_FACILITIES_TEST,
> >   };
> Please keep the order as defined in target/s390x/cpu_features_def.h
Will do.
> >   
> >   static uint16_t full_GEN12_GA2[] = {
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index e13c890..ae20ed8 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
> >       { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
> >       { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
> >       { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
> > +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
> Nothing speaks against the STFL bits, want to learn more about the
> S390_FEAT_AP feature :)
There are a couple of primary reasons for the addition of this feature.

* Let's start with the fact that AP instructions absolutely must be 
installed on the host in order to virtualize
  AP devices for a guest using this patch series. There is a bit in the 
in the SIE block (ECA.28) that controls
  whether AP instructions executed on the guest are interpreted by SIE 
or intercepted by KVM. This patch series sets
  this bit so that AP instructions executed on the guest are 
interpreted by the firmware passed directly through
  to the AP devices configured for the guest in the CRYCB- a satellite 
control block of the SIE block to configure
  the AP facilities for the guest. If the AP instructions are not 
installed, the AP bus running in the guest will
  not initialize and the guest will not have access to any AP devices. 
So, the primary reason for the S390_FEAT_AP
  feature is to protect against this scenario.

* In the review of v1, Christian suggested creating a feature to prevent 
migration of a guest with AP devices
  to a system without AP support, or a system without AP instructions. 
In order to migrate to another system,
  the S390_FEAT_AP feature must be available on the target system.

I hope this clears things up for you.
>