| Message ID | 1519746259-27710-6-git-send-email-akrowiak@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: vfio-ap: guest dedicated crypto adapters | expand |
On Tue, 27 Feb 2018 10:44:19 -0500 Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: > A new CPU model feature and two new CPU model facilities are > introduced to support AP devices for a KVM guest. > > CPU model features: > > 1. The KVM_S390_VM_CPU_FEAT_AP CPU model feature indicates that > AP facilities are installed. This feature will be enabled by > the kernel only if the AP facilities are installed on the linux > host. This feature must be turned on from userspace to access > AP devices from the KVM guest. The QEMU command line to turn > this feature looks something like this: > > qemu-system-s390x ... -cpu xxx,ap=on > > CPU model facilities: > > 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates the AP Query > Configuration Information (QCI) facility is installed. This feature > will be enabled by the kernel only if the QCI is installed on > the host. This facility will be set by QEMU only if the > KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on. > (see CPU model features above) > > 2. The S390_FEAT_AP_FACILITY_TEST feature indicates that the AP > Test Facility (APFT) facility is installed. This feature will > be enabled by the kernel only if the APFT facility is installed > on the host. This facility will be set by QEMU for the KVM guest > only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on. > (see CPU model features above) > > Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> > --- > hw/vfio/ap.c | 9 +++++++++ > linux-headers/asm-s390/kvm.h | 1 + > target/s390x/cpu_features.c | 3 +++ > target/s390x/cpu_features_def.h | 3 +++ > target/s390x/cpu_models.c | 12 ++++++++++++ > target/s390x/gen-features.c | 3 +++ > target/s390x/kvm.c | 6 ++++++ > 7 files changed, 37 insertions(+), 0 deletions(-) > > diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c > index 8aa5221..b93f7d9 100644 > --- a/hw/vfio/ap.c > +++ b/hw/vfio/ap.c > @@ -19,6 +19,7 @@ > #include "hw/s390x/ap-device.h" > #include "qemu/error-report.h" > #include "qemu/queue.h" > +#include "cpu.h" > > #define VFIO_AP_DEVICE_TYPE "vfio-ap" > #define AP_SYSFSDEV_PROP_NAME "sysfsdev" > @@ -87,6 +88,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) > Error *local_err = NULL; > int ret; > > + if (!s390_has_feat(S390_FEAT_AP)) { > + error_setg(&local_err, "Invalid device configuration: "); "AP support not available" ? [The hint is not visible in every circumstance IIRC] > + error_append_hint(&local_err, > + "Verify AP facilities are enabled for the guest" > + "(ap=on)\n"); > + goto out_err; > + } > + > 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]; > }; Shouldn't that hunk have been in the previous headers update already? > 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"), FEAT_TYPE_MISC does not use bit numbering (and there's a new initializer for these bits queued in s390-next). > > 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 "not provided in the model" ? > + * 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); > + } > + > 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, > }; > > 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}, > }; > > static int query_cpu_feat(S390FeatBitmap features) > @@ -2214,6 +2215,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) > error_setg(errp, "KVM: Error querying CPU features: %d", rc); > return; > } > + /* AP facilities support is required to enable QCI and APFT support */ > + if (!test_bit(S390_FEAT_AP, model->features)) { > + clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, model->features); > + clear_bit(S390_FEAT_AP_FACILITIES_TEST, model->features); > + } Hm, do you need this twice? > /* get supported cpu subfunctions indicated via query / test bit */ > rc = query_cpu_subfunc(model->features); > if (rc) { I'm leaving a general review of the cpu model things to David.
On 02/27/2018 05:27 PM, Cornelia Huck wrote: > On Tue, 27 Feb 2018 10:44:19 -0500 > Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: >> A new CPU model feature and two new CPU model facilities are >> introduced to support AP devices for a KVM guest. >> >> CPU model features: >> >> 1. The KVM_S390_VM_CPU_FEAT_AP CPU model feature indicates that >> AP facilities are installed. This feature will be enabled by >> the kernel only if the AP facilities are installed on the linux >> host. This feature must be turned on from userspace to access >> AP devices from the KVM guest. The QEMU command line to turn >> this feature looks something like this: >> >> qemu-system-s390x ... -cpu xxx,ap=on >> >> CPU model facilities: >> >> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates the AP Query >> Configuration Information (QCI) facility is installed. This feature >> will be enabled by the kernel only if the QCI is installed on >> the host. This facility will be set by QEMU only if the >> KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on. >> (see CPU model features above) >> >> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates that the AP >> Test Facility (APFT) facility is installed. This feature will >> be enabled by the kernel only if the APFT facility is installed >> on the host. This facility will be set by QEMU for the KVM guest >> only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on. >> (see CPU model features above) >> This may needs to be reworded. See my comments below. >> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> >> --- >> hw/vfio/ap.c | 9 +++++++++ >> linux-headers/asm-s390/kvm.h | 1 + >> target/s390x/cpu_features.c | 3 +++ >> target/s390x/cpu_features_def.h | 3 +++ >> target/s390x/cpu_models.c | 12 ++++++++++++ >> target/s390x/gen-features.c | 3 +++ >> target/s390x/kvm.c | 6 ++++++ >> 7 files changed, 37 insertions(+), 0 deletions(-) >> >> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >> index 8aa5221..b93f7d9 100644 >> --- a/hw/vfio/ap.c >> +++ b/hw/vfio/ap.c >> @@ -19,6 +19,7 @@ >> #include "hw/s390x/ap-device.h" >> #include "qemu/error-report.h" >> #include "qemu/queue.h" >> +#include "cpu.h" >> >> #define VFIO_AP_DEVICE_TYPE "vfio-ap" >> #define AP_SYSFSDEV_PROP_NAME "sysfsdev" >> @@ -87,6 +88,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) >> Error *local_err = NULL; >> int ret; >> >> + if (!s390_has_feat(S390_FEAT_AP)) { >> + error_setg(&local_err, "Invalid device configuration: "); > > "AP support not available" ? > > [The hint is not visible in every circumstance IIRC] > I agree, it does not make sense to split this into a message and a hint. [..] >> @@ -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 > > > "not provided in the model" ? > >> + * 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); >> + } >> + I don't like this at all. Never liked software that overrules my user input. If I say --cpu z13,ap=off,qci=on,apft=on this would silently overrule to --cpu z13,ap=off,qci=off,apft=off from the guest perspective. There also might be other reasons why this is a bad idea. >> 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, >> }; >> >> 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}, >> }; >> >> static int query_cpu_feat(S390FeatBitmap features) >> @@ -2214,6 +2215,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) >> error_setg(errp, "KVM: Error querying CPU features: %d", rc); >> return; >> } >> + /* AP facilities support is required to enable QCI and APFT support */ >> + if (!test_bit(S390_FEAT_AP, model->features)) { >> + clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, model->features); >> + clear_bit(S390_FEAT_AP_FACILITIES_TEST, model->features); >> + } > > Hm, do you need this twice? In my opinion this has only value if we assume that HW and/or KVM is buggy and we are running host model (or it's expansion). And even the we would get a warning, and nothing bad would happen with a linux guest. While I'm not strongly opposing this, I would not mind it dropped. If we want to make sure things are consistent I would prefer the consistency check being generating an error (instead of a warning). > >> /* get supported cpu subfunctions indicated via query / test bit */ >> rc = query_cpu_subfunc(model->features); >> if (rc) { > > I'm leaving a general review of the cpu model things to David. > Except for these it's LGTM (r-b level LGTM).
> 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? IOW: is this really a CPU model feature? > > 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 :) > + > 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 > > 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 :)
>> Hm, do you need this twice? > > In my opinion this has only value if we assume that HW and/or KVM is buggy and > we are running host model (or it's expansion). > The "sanity" checks in KVM sensing code don't really hurt. But I agree, sane KVM should not produce this. > And even the we would get a warning, and nothing bad would happen with a linux > guest. > > While I'm not strongly opposing this, I would not mind it dropped. If we want > to make sure things are consistent I would prefer the consistency check being > generating an error (instead of a warning). > We use a warning as it is helpful for development (e.g. under TCG you can enable msa5, although we yield a warning due to a failing consistency check).
On 02/27/2018 06: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? > The ap_bus has a function for determining if the ap instructions are installed. I think it's basically trial execution. > IOW: is this really a CPU model feature? > I think it's best modeled with a CPU model feature. In the end it's about having or not having ap instructions in the guest, and making stable guest ABI is exactly the thing of cpu-models AFAIU. >> >> 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 > I agree, this has to go (already commented on this). > a) Add consistency checks (check_consistency()) The consistency checks are already in place. As already stated before, one could make it produce an error. > b) Mask the bits out in the KVM CPU model sensing part > (kvm_s390_get_host_cpu_model()) - which you already have :) > Getting no ap but qci and apft indicated by KVM is unlikely to happen, ever. >> + >> 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 > >> >> 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 :) > > Kernel series wise what you are looking for is '[PATCH v2 04/15] KVM: s390: CPU model support for AP virtualization'(MID: <1519741693-17440-5-git-send-email-akrowiak@linux.vnet.ibm.com>) and '[PATCH v2 08/15] KVM: s390: interface to enable AP execution mode' (MID: <1519741693-17440-9-git-send-email-akrowiak@linux.vnet.ibm.com>) Happy reviewing! Regards, Halil
On 02/27/2018 11:27 AM, Cornelia Huck wrote: > On Tue, 27 Feb 2018 10:44:19 -0500 > Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: >> A new CPU model feature and two new CPU model facilities are >> introduced to support AP devices for a KVM guest. >> >> CPU model features: >> >> 1. The KVM_S390_VM_CPU_FEAT_AP CPU model feature indicates that >> AP facilities are installed. This feature will be enabled by >> the kernel only if the AP facilities are installed on the linux >> host. This feature must be turned on from userspace to access >> AP devices from the KVM guest. The QEMU command line to turn >> this feature looks something like this: >> >> qemu-system-s390x ... -cpu xxx,ap=on >> >> CPU model facilities: >> >> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates the AP Query >> Configuration Information (QCI) facility is installed. This feature >> will be enabled by the kernel only if the QCI is installed on >> the host. This facility will be set by QEMU only if the >> KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on. >> (see CPU model features above) >> >> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates that the AP >> Test Facility (APFT) facility is installed. This feature will >> be enabled by the kernel only if the APFT facility is installed >> on the host. This facility will be set by QEMU for the KVM guest >> only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on. >> (see CPU model features above) >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> >> --- >> hw/vfio/ap.c | 9 +++++++++ >> linux-headers/asm-s390/kvm.h | 1 + >> target/s390x/cpu_features.c | 3 +++ >> target/s390x/cpu_features_def.h | 3 +++ >> target/s390x/cpu_models.c | 12 ++++++++++++ >> target/s390x/gen-features.c | 3 +++ >> target/s390x/kvm.c | 6 ++++++ >> 7 files changed, 37 insertions(+), 0 deletions(-) >> >> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >> index 8aa5221..b93f7d9 100644 >> --- a/hw/vfio/ap.c >> +++ b/hw/vfio/ap.c >> @@ -19,6 +19,7 @@ >> #include "hw/s390x/ap-device.h" >> #include "qemu/error-report.h" >> #include "qemu/queue.h" >> +#include "cpu.h" >> >> #define VFIO_AP_DEVICE_TYPE "vfio-ap" >> #define AP_SYSFSDEV_PROP_NAME "sysfsdev" >> @@ -87,6 +88,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) >> Error *local_err = NULL; >> int ret; >> >> + if (!s390_has_feat(S390_FEAT_AP)) { >> + error_setg(&local_err, "Invalid device configuration: "); > "AP support not available" ? > > [The hint is not visible in every circumstance IIRC] > >> + error_append_hint(&local_err, >> + "Verify AP facilities are enabled for the guest" >> + "(ap=on)\n"); >> + goto out_err; >> + } >> + >> 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]; >> }; > Shouldn't that hunk have been in the previous headers update already? > >> 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"), > FEAT_TYPE_MISC does not use bit numbering (and there's a new > initializer for these bits queued in s390-next). > >> >> 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 > > "not provided in the model" ? > >> + * 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); >> + } >> + >> 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, >> }; >> >> 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}, >> }; >> >> static int query_cpu_feat(S390FeatBitmap features) >> @@ -2214,6 +2215,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) >> error_setg(errp, "KVM: Error querying CPU features: %d", rc); >> return; >> } >> + /* AP facilities support is required to enable QCI and APFT support */ >> + if (!test_bit(S390_FEAT_AP, model->features)) { >> + clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, model->features); >> + clear_bit(S390_FEAT_AP_FACILITIES_TEST, model->features); >> + } > Hm, do you need this twice? No, I don't even need it once. It is a relic of a previous revision. > >> /* get supported cpu subfunctions indicated via query / test bit */ >> rc = query_cpu_subfunc(model->features); >> if (rc) { > I'm leaving a general review of the cpu model things to David. >
On 02/27/2018 11:49 AM, Halil Pasic wrote: > > On 02/27/2018 05:27 PM, Cornelia Huck wrote: >> On Tue, 27 Feb 2018 10:44:19 -0500 >> Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: >>> A new CPU model feature and two new CPU model facilities are >>> introduced to support AP devices for a KVM guest. >>> >>> CPU model features: >>> >>> 1. The KVM_S390_VM_CPU_FEAT_AP CPU model feature indicates that >>> AP facilities are installed. This feature will be enabled by >>> the kernel only if the AP facilities are installed on the linux >>> host. This feature must be turned on from userspace to access >>> AP devices from the KVM guest. The QEMU command line to turn >>> this feature looks something like this: >>> >>> qemu-system-s390x ... -cpu xxx,ap=on >>> >>> CPU model facilities: >>> >>> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates the AP Query >>> Configuration Information (QCI) facility is installed. This feature >>> will be enabled by the kernel only if the QCI is installed on >>> the host. This facility will be set by QEMU only if the >>> KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on. >>> (see CPU model features above) >>> >>> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates that the AP >>> Test Facility (APFT) facility is installed. This feature will >>> be enabled by the kernel only if the APFT facility is installed >>> on the host. This facility will be set by QEMU for the KVM guest >>> only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on. >>> (see CPU model features above) >>> > This may needs to be reworded. See my comments below. > >>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>> --- >>> hw/vfio/ap.c | 9 +++++++++ >>> linux-headers/asm-s390/kvm.h | 1 + >>> target/s390x/cpu_features.c | 3 +++ >>> target/s390x/cpu_features_def.h | 3 +++ >>> target/s390x/cpu_models.c | 12 ++++++++++++ >>> target/s390x/gen-features.c | 3 +++ >>> target/s390x/kvm.c | 6 ++++++ >>> 7 files changed, 37 insertions(+), 0 deletions(-) >>> >>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >>> index 8aa5221..b93f7d9 100644 >>> --- a/hw/vfio/ap.c >>> +++ b/hw/vfio/ap.c >>> @@ -19,6 +19,7 @@ >>> #include "hw/s390x/ap-device.h" >>> #include "qemu/error-report.h" >>> #include "qemu/queue.h" >>> +#include "cpu.h" >>> >>> #define VFIO_AP_DEVICE_TYPE "vfio-ap" >>> #define AP_SYSFSDEV_PROP_NAME "sysfsdev" >>> @@ -87,6 +88,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) >>> Error *local_err = NULL; >>> int ret; >>> >>> + if (!s390_has_feat(S390_FEAT_AP)) { >>> + error_setg(&local_err, "Invalid device configuration: "); >> "AP support not available" ? >> >> [The hint is not visible in every circumstance IIRC] >> > I agree, it does not make sense to split this into a message and > a hint. > > [..] > >>> @@ -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 >> >> "not provided in the model" ? >> >>> + * 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); >>> + } >>> + > I don't like this at all. Never liked software that overrules my user input. > If I say --cpu z13,ap=off,qci=on,apft=on this would silently overrule to > --cpu z13,ap=off,qci=off,apft=off from the guest perspective. There also might be > other reasons why this is a bad idea. > >>> 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, >>> }; >>> >>> 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}, >>> }; >>> >>> static int query_cpu_feat(S390FeatBitmap features) >>> @@ -2214,6 +2215,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) >>> error_setg(errp, "KVM: Error querying CPU features: %d", rc); >>> return; >>> } >>> + /* AP facilities support is required to enable QCI and APFT support */ >>> + if (!test_bit(S390_FEAT_AP, model->features)) { >>> + clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, model->features); >>> + clear_bit(S390_FEAT_AP_FACILITIES_TEST, model->features); >>> + } >> Hm, do you need this twice? > In my opinion this has only value if we assume that HW and/or KVM is buggy and > we are running host model (or it's expansion). > > And even the we would get a warning, and nothing bad would happen with a linux > guest. It is going, going ..... gone! > > While I'm not strongly opposing this, I would not mind it dropped. If we want > to make sure things are consistent I would prefer the consistency check being > generating an error (instead of a warning). > >>> /* get supported cpu subfunctions indicated via query / test bit */ >>> rc = query_cpu_subfunc(model->features); >>> if (rc) { >> I'm leaving a general review of the cpu model things to David. I'm looking forward to David's comments. >> > Except for these it's LGTM (r-b level LGTM).
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. > >
> 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). As mentioned in the KVM thread, I'd like to verify if there is not a AP interpretation facility. >> >> 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. Yes, I can see how this is guest ABI. But we always have to take care of ever potentially wanting to emulate this in QEMU. But if we can turn interpretation on/off using the feature flag, I am happy. >> >>> >>> 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 }, Saw this way too late :) >> >>> >>> 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. Then I request the following change in KVM: If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set (not just if an AP device is configured). This especially makes things a lot easier when it comes to handling hotplugged CPUs and avoiding race conditions when enabling these bits as mentioned in the KVM series. KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest (don't throw an operation exception). So this feature then really is guest ABI. The instructions are available. If there is no device configured, bad luck. > > * 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 wonder if it would make sense to split this even further up. E.g. to be able to differentiate between format0 and format2 crycb format support. (which is necessary to properly migrate guests with vSIE) But these would then be SIE specific CPU features in addition most properly. But also depend if there is a AP interpretation facility :) > > I hope this clears things up for you. Yes, very helpful, thanks a lot!
> The ap_bus has a function for determining if the ap instructions are > installed. I think it's basically trial execution. > Okay, just like CMM. Bad system design. But it is what it is. >> > > I think it's best modeled with a CPU model feature. In the end > it's about having or not having ap instructions in the guest, and > making stable guest ABI is exactly the thing of cpu-models AFAIU. Indeed, as mentioned in the other mail, the AP feature but then always has to say "AP instructions available", not just if an AP device has been defined. > >>> >>> 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 >> > > I agree, this has to go (already commented on this). > >> a) Add consistency checks (check_consistency()) > > The consistency checks are already in place. As already stated > before, one could make it produce an error. > >> b) Mask the bits out in the KVM CPU model sensing part >> (kvm_s390_get_host_cpu_model()) - which you already have :) >> > > Getting no ap but qci and apft indicated by KVM is unlikely to > happen, ever. I assume it would happen right now under vSIE (or I missed where we enable the new ECA bit in the vSIE code). Having such simple masking operations in the "sensing" part usually doesn't hurt. We try to produce a consistent model even though the hardware/KVM might be weird. > >>> + >>> 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 >> >>> >>> 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 :) >> >> > > Kernel series wise what you are looking for is > '[PATCH v2 04/15] KVM: s390: CPU model support for AP virtualization'(MID: <1519741693-17440-5-git-send-email-akrowiak@linux.vnet.ibm.com>) > and > '[PATCH v2 08/15] KVM: s390: interface to enable AP execution mode' (MID: <1519741693-17440-9-git-send-email-akrowiak@linux.vnet.ibm.com>) > Found it, thanks for the pointer! > Happy reviewing! > > Regards, > Halil > > >
On Wed, 28 Feb 2018 11:26:30 +0100 David Hildenbrand <david@redhat.com> wrote: > Then I request the following change in KVM: > > If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set > (not just if an AP device is configured). This especially makes things a > lot easier when it comes to handling hotplugged CPUs and avoiding race > conditions when enabling these bits as mentioned in the KVM series. > > KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest > (don't throw an operation exception). > > So this feature then really is guest ABI. The instructions are > available. If there is no device configured, bad luck. Sounds sensible from my POV.
On 28/02/2018 12:40, Cornelia Huck wrote: > On Wed, 28 Feb 2018 11:26:30 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> Then I request the following change in KVM: >> >> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set >> (not just if an AP device is configured). This especially makes things a >> lot easier when it comes to handling hotplugged CPUs and avoiding race >> conditions when enabling these bits as mentioned in the KVM series. >> >> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest >> (don't throw an operation exception). >> >> So this feature then really is guest ABI. The instructions are >> available. If there is no device configured, bad luck. > Sounds sensible from my POV. > I have a concern with this proposition and with the original code: 1) ap=on is a guest ABI feature saying to the guest you can use AP instructions 2) How we provide AP instructions to the guest can be done in three different ways: - SIE Interpretation - interception with VFIO - interception with emulation 3) We implement this with a device in QEMU and a certain level kernel support. It seems possible to set or not ECA.28 , based on the type of kernel device: - SIE interpretation -> MATRIX KVM device -> ECA.28 - Interception with VFIO and virtualization -> no ECA.28 - interception with emulation -> no ECA.28 I understand the concern with the vCPU but I think we can handle it with an indirect variable like: SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable ap_to_be_sie_interpreted=1 Then in vCPU initialization set ECA.28 based on this variable. I think it let us more doors open, what is your opinion? Regards, Pierre
On 01.03.2018 15:12, Pierre Morel wrote: > On 28/02/2018 12:40, Cornelia Huck wrote: >> On Wed, 28 Feb 2018 11:26:30 +0100 >> David Hildenbrand <david@redhat.com> wrote: >> >>> Then I request the following change in KVM: >>> >>> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set >>> (not just if an AP device is configured). This especially makes things a >>> lot easier when it comes to handling hotplugged CPUs and avoiding race >>> conditions when enabling these bits as mentioned in the KVM series. >>> >>> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest >>> (don't throw an operation exception). >>> >>> So this feature then really is guest ABI. The instructions are >>> available. If there is no device configured, bad luck. >> Sounds sensible from my POV. >> > > I have a concern with this proposition and with the original code: Very good, this is exactly what I talked to Conny about yesterday. Short version: CPU model is guest ABI, everything else is configuration. > > 1) ap=on is a guest ABI feature saying to the guest you can use AP > instructions Indeed, that's what belongs into the CPU model. > > 2) How we provide AP instructions to the guest can be done in three > different ways: > - SIE Interpretation > - interception with VFIO > - interception with emulation > Due to bad AP design we can't handle this like zPCI - completely in QEMU. That's what should control it. > 3) We implement this with a device in QEMU and a certain level kernel > support. > > It seems possible to set or not ECA.28 , based on the type of kernel device: > - SIE interpretation -> MATRIX KVM device -> ECA.28 > - Interception with VFIO and virtualization -> no ECA.28 > - interception with emulation -> no ECA.28 > > I understand the concern with the vCPU but I think we can handle it with > an indirect variable > like: > > SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable > ap_to_be_sie_interpreted=1 > Then in vCPU initialization set ECA.28 based on this variable. That's exactly why we have the cpu feature interface. E.g. CMMA -> if not enabled, not interpreted by HW (however also not intercepted to user space - no sense in doing that right now). > > I think it let us more doors open, what is your opinion? In general, for now we don't care. The kernel is the only AP bus provider. If KVM presents AP support -> AP feature can be enabled. And it should always enable it. Once we have a QEMU AP bus implementation, things get more complicated. We could provide the AP feature also without KVM (like zPCI). But weather or not to enable the KVM control has to be concluded from the other configuration. Only user space can now that and has to decide before enabling AP in KVM. So I think for now we are fine. Later, this might be tricky to check but not impossible. > > Regards, > > Pierre > >
On 03/01/2018 03:36 PM, David Hildenbrand wrote: >> I have a concern with this proposition and with the original code: > Very good, this is exactly what I talked to Conny about yesterday. > > Short version: CPU model is guest ABI, everything else is configuration. > Nod. > > So I think for now we are fine. Later, this might be tricky to check but > not impossible. > I think we are all in agreement on the important stuff. I think, we also all agree, that certain things need to be improved. E.g. the KVM code manipulating ECA.28, -cpu xxx,ap=on needs to imply no Operation Exception when guest executes an AP instruction (this is currently not the case as we can have a vm a vfio-ap device -- so open won't get called -- but with ap=on). Hope Tony will address these in the next version. Regards, Halil
On 03/01/2018 09:12 AM, Pierre Morel wrote: > On 28/02/2018 12:40, Cornelia Huck wrote: >> On Wed, 28 Feb 2018 11:26:30 +0100 >> David Hildenbrand <david@redhat.com> wrote: >> >>> Then I request the following change in KVM: >>> >>> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set >>> (not just if an AP device is configured). This especially makes >>> things a >>> lot easier when it comes to handling hotplugged CPUs and avoiding race >>> conditions when enabling these bits as mentioned in the KVM series. >>> >>> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest >>> (don't throw an operation exception). >>> >>> So this feature then really is guest ABI. The instructions are >>> available. If there is no device configured, bad luck. >> Sounds sensible from my POV. >> > > I have a concern with this proposition and with the original code: > > 1) ap=on is a guest ABI feature saying to the guest you can use AP > instructions > > 2) How we provide AP instructions to the guest can be done in three > different ways: > - SIE Interpretation > - interception with VFIO > - interception with emulation > > 3) We implement this with a device in QEMU and a certain level kernel > support. > > It seems possible to set or not ECA.28 , based on the type of kernel > device: > - SIE interpretation -> MATRIX KVM device -> ECA.28 > - Interception with VFIO and virtualization -> no ECA.28 > - interception with emulation -> no ECA.28 > > I understand the concern with the vCPU but I think we can handle it > with an indirect variable > like: > > SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the > variable ap_to_be_sie_interpreted=1 > Then in vCPU initialization set ECA.28 based on this variable. > > I think it let us more doors open, what is your opinion? I've already implemented a proof of concept similar to what you suggest to verify whether it would. I wasn't completely sure of the flow of control between the KVM notification to the device driver and the vcpu setup. If the variable is set when the device driver is notified about KVM, it has to happen before vcpu setup for this to work. I was able to verify that with my proof of concept. This discussion really belongs in the KVM/kernel patches, so I am going to continue the discussion of my proposal there. > > Regards, > > Pierre > >
On 03/01/2018 09:36 AM, David Hildenbrand wrote: > On 01.03.2018 15:12, Pierre Morel wrote: >> On 28/02/2018 12:40, Cornelia Huck wrote: >>> On Wed, 28 Feb 2018 11:26:30 +0100 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> Then I request the following change in KVM: >>>> >>>> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set >>>> (not just if an AP device is configured). This especially makes things a >>>> lot easier when it comes to handling hotplugged CPUs and avoiding race >>>> conditions when enabling these bits as mentioned in the KVM series. >>>> >>>> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest >>>> (don't throw an operation exception). >>>> >>>> So this feature then really is guest ABI. The instructions are >>>> available. If there is no device configured, bad luck. >>> Sounds sensible from my POV. >>> >> I have a concern with this proposition and with the original code: > Very good, this is exactly what I talked to Conny about yesterday. > > Short version: CPU model is guest ABI, everything else is configuration. > >> 1) ap=on is a guest ABI feature saying to the guest you can use AP >> instructions > Indeed, that's what belongs into the CPU model. > >> 2) How we provide AP instructions to the guest can be done in three >> different ways: >> - SIE Interpretation >> - interception with VFIO >> - interception with emulation >> > Due to bad AP design we can't handle this like zPCI - completely in > QEMU. That's what should control it. > >> 3) We implement this with a device in QEMU and a certain level kernel >> support. >> >> It seems possible to set or not ECA.28 , based on the type of kernel device: >> - SIE interpretation -> MATRIX KVM device -> ECA.28 >> - Interception with VFIO and virtualization -> no ECA.28 >> - interception with emulation -> no ECA.28 >> >> I understand the concern with the vCPU but I think we can handle it with >> an indirect variable >> like: >> >> SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable >> ap_to_be_sie_interpreted=1 >> Then in vCPU initialization set ECA.28 based on this variable. > That's exactly why we have the cpu feature interface. E.g. CMMA -> if > not enabled, not interpreted by HW (however also not intercepted to user > space - no sense in doing that right now). I'm not sure I am interpreting what you are saying here correctly, but in the case of AP, if ECA.28 is not set, the AP instructions will not be interpreted by HW but WILL be intercepted and forwarded to user space. > >> I think it let us more doors open, what is your opinion? > In general, for now we don't care. The kernel is the only AP bus provider. > > If KVM presents AP support -> AP feature can be enabled. And it should > always enable it. > > Once we have a QEMU AP bus implementation, things get more complicated. > We could provide the AP feature also without KVM (like zPCI). > > But weather or not to enable the KVM control has to be concluded from > the other configuration. Only user space can now that and has to decide > before enabling AP in KVM. > > So I think for now we are fine. Later, this might be tricky to check but > not impossible. Maybe we are applying the wrong semantics to this feature. The premise for this feature was to control the setting of ECA.28. It grew beyond this premise because of observations related to future considerations about emulation and full virtualization (i.e., the things Pierre mentioned above). Instead of this feature indicating AP facilities are installed on the guest, it might behoove us to return to its original intended purpose which was to indicate that AP instructions executed by the guest are interpreted by HW. In this case, we can resume setting it in the vcpu setup like it was in the earlier patch series. > >> Regards, >> >> Pierre >> >> >
On 03/01/2018 09:36 AM, David Hildenbrand wrote: > On 01.03.2018 15:12, Pierre Morel wrote: >> On 28/02/2018 12:40, Cornelia Huck wrote: >>> On Wed, 28 Feb 2018 11:26:30 +0100 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> Then I request the following change in KVM: >>>> >>>> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set >>>> (not just if an AP device is configured). This especially makes things a >>>> lot easier when it comes to handling hotplugged CPUs and avoiding race >>>> conditions when enabling these bits as mentioned in the KVM series. >>>> >>>> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest >>>> (don't throw an operation exception). >>>> >>>> So this feature then really is guest ABI. The instructions are >>>> available. If there is no device configured, bad luck. >>> Sounds sensible from my POV. >>> >> I have a concern with this proposition and with the original code: > Very good, this is exactly what I talked to Conny about yesterday. > > Short version: CPU model is guest ABI, everything else is configuration. > >> 1) ap=on is a guest ABI feature saying to the guest you can use AP >> instructions > Indeed, that's what belongs into the CPU model. > >> 2) How we provide AP instructions to the guest can be done in three >> different ways: >> - SIE Interpretation AP instructions executed on the guest will be interpreted and passed directly through to a real AP device installed on the host according to the APQN specified in the instruction, so the AP instructions must be available on the host. >> - interception with VFIO AP instructions executed on the guest will be intercepted, and interpreted by QEMU then forwarded to a real AP device installed on the host according to how the AP devices are configured in userspace - i.e., whether there is remapping, multiplexing or sharing of adapters/domains etc. This also requires that AP instructions be available on the host. >> - interception with emulation AP instructions executed on the guest will be intercepted, interpreted by QEMU and emulated. This will not require AP instructions be available on the host. In all cases above, the need to set ECA_APIE is dependent upon the device type configured for the guest. >> > Due to bad AP design we can't handle this like zPCI - completely in > QEMU. That's what should control it. > >> 3) We implement this with a device in QEMU and a certain level kernel >> support. >> >> It seems possible to set or not ECA.28 , based on the type of kernel device: >> - SIE interpretation -> MATRIX KVM device -> ECA.28 >> - Interception with VFIO and virtualization -> no ECA.28 >> - interception with emulation -> no ECA.28 >> >> I understand the concern with the vCPU but I think we can handle it with >> an indirect variable >> like: >> >> SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable >> ap_to_be_sie_interpreted=1 >> Then in vCPU initialization set ECA.28 based on this variable. > That's exactly why we have the cpu feature interface. E.g. CMMA -> if > not enabled, not interpreted by HW (however also not intercepted to user > space - no sense in doing that right now). There are two factors at play here, the device type (i.e., -device vfio_ap) and the KVM_S390_VM_CPU_FEAT_AP feature. Setting ECA_APIE makes no sense if the vfio_ap device is not configured. For the passthrough implementation, this means that the AP bus will successfully load on the guest, but there will be no AP devices detected. If the expectation is that ap=on will allow access to AP devices on the guest, that would be a mistaken assumption. If down the road a new guest AP device is introduced that allows multiplexing, device remapping etc., then it will be necessary to intercept AP instructions executed on the guest. In this case, if ap=on results in setting ECA_APIE, then the instructions will be interpreted and passed through to a device that does not exist because it won't be defined in the guest's CRYCB. Based on these two scenarios, I think what we are really saying with ap=on is that the guest will require use of the AP instructions installed on the host. Whether those instructions are executed as a result of interpretation by the hardware or because they are executed by the device driver is a separate matter. So, I am inclined to agree with Pierre for that reason. ECA_APIE should be set only if ap=on (i.e., AP instructions are available on the host) and the user of those instructions (i.e., the driver) indicate an intent to use them. > >> I think it let us more doors open, what is your opinion? > In general, for now we don't care. The kernel is the only AP bus provider. True today, but not necessarily in the future. > > If KVM presents AP support -> AP feature can be enabled. And it should > always enable it. I disagree for the reasons stated above. > > Once we have a QEMU AP bus implementation, things get more complicated. > We could provide the AP feature also without KVM (like zPCI). I am not familiar with zPCI, so I can't comment. > > But weather or not to enable the KVM control has to be concluded from > the other configuration. Only user space can now that and has to decide > before enabling AP in KVM. > > So I think for now we are fine. Later, this might be tricky to check but > not impossible. > >> Regards, >> >> Pierre >> >> >
>>> 1) ap=on is a guest ABI feature saying to the guest you can use AP >>> instructions >> Indeed, that's what belongs into the CPU model. >> >>> 2) How we provide AP instructions to the guest can be done in three >>> different ways: >>> - SIE Interpretation > AP instructions executed on the guest will be interpreted and passed > directly > through to a real AP device installed on the host according to the APQN > specified in the instruction, so the AP instructions must be available > on the > host. >>> - interception with VFIO > AP instructions executed on the guest will be intercepted, and > interpreted by > QEMU then forwarded to a real AP device installed on the host according > to how > the AP devices are configured in userspace - i.e., whether there is > remapping, > multiplexing or sharing of adapters/domains etc. This also requires that AP > instructions be available on the host. See my other mail: I think this is a conflict with vSIE. >>> - interception with emulation > AP instructions executed on the guest will be intercepted, interpreted > by QEMU > and emulated. This will not require AP instructions be available on the > host. See my other mail: I think this is a conflict with vSIE. > > In all cases above, the need to set ECA_APIE is dependent upon the device > type configured for the guest. >>> >> Due to bad AP design we can't handle this like zPCI - completely in >> QEMU. That's what should control it. >> >>> 3) We implement this with a device in QEMU and a certain level kernel >>> support. >>> >>> It seems possible to set or not ECA.28 , based on the type of kernel device: >>> - SIE interpretation -> MATRIX KVM device -> ECA.28 >>> - Interception with VFIO and virtualization -> no ECA.28 >>> - interception with emulation -> no ECA.28 >>> >>> I understand the concern with the vCPU but I think we can handle it with >>> an indirect variable >>> like: >>> >>> SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable >>> ap_to_be_sie_interpreted=1 >>> Then in vCPU initialization set ECA.28 based on this variable. >> That's exactly why we have the cpu feature interface. E.g. CMMA -> if >> not enabled, not interpreted by HW (however also not intercepted to user >> space - no sense in doing that right now). > There are two factors at play here, the device type (i.e., -device > vfio_ap) and > the KVM_S390_VM_CPU_FEAT_AP feature. Setting ECA_APIE makes no sense if the > vfio_ap device is not configured. For the passthrough implementation, this > means that the AP bus will successfully load on the guest, but there will > be no AP devices detected. If the expectation is that ap=on will allow > access > to AP devices on the guest, that would be a mistaken assumption. > > If down the road a new guest AP device is introduced that allows > multiplexing, > device remapping etc., then it will be necessary to intercept AP > instructions > executed on the guest. In this case, if ap=on results in setting ECA_APIE, > then the instructions will be interpreted and passed through to a device > that does not exist because it won't be defined in the guest's CRYCB. Again, see my other mail, this discussion is superfluous if we can't get vSIE to work with emulated devices. And it smells like this is the case. But I don't have access to documentation. > > Based on these two scenarios, I think what we are really saying with ap=on > is that the guest will require use of the AP instructions installed on the > host. Whether those instructions are executed as a result of interpretation > by the hardware or because they are executed by the device driver is a > separate matter. So, I am inclined to agree with Pierre for that reason. > ECA_APIE should be set only if ap=on (i.e., AP instructions are available > on the host) and the user of those instructions (i.e., the driver) indicate > an intent to use them. ap=on -> set ECA_APIE is what I proposed. >> >>> I think it let us more doors open, what is your opinion? >> In general, for now we don't care. The kernel is the only AP bus provider. > True today, but not necessarily in the future. I think so (vSIE). >> >> If KVM presents AP support -> AP feature can be enabled. And it should >> always enable it. > I disagree for the reasons stated above. By always enable I of course mean "ap=on" (the point was: independent of devices right now).
On 06/03/2018 18:15, David Hildenbrand wrote: >>>> 1) ap=on is a guest ABI feature saying to the guest you can use AP >>>> instructions >>> Indeed, that's what belongs into the CPU model. >>> >>>> 2) How we provide AP instructions to the guest can be done in three >>>> different ways: >>>> - SIE Interpretation >> AP instructions executed on the guest will be interpreted and passed >> directly >> through to a real AP device installed on the host according to the APQN >> specified in the instruction, so the AP instructions must be available >> on the >> host. >>>> - interception with VFIO >> AP instructions executed on the guest will be intercepted, and >> interpreted by >> QEMU then forwarded to a real AP device installed on the host according >> to how >> the AP devices are configured in userspace - i.e., whether there is >> remapping, >> multiplexing or sharing of adapters/domains etc. This also requires that AP >> instructions be available on the host. > See my other mail: I think this is a conflict with vSIE. > >>>> - interception with emulation >> AP instructions executed on the guest will be intercepted, interpreted >> by QEMU >> and emulated. This will not require AP instructions be available on the >> host. > See my other mail: I think this is a conflict with vSIE. > >> In all cases above, the need to set ECA_APIE is dependent upon the device >> type configured for the guest. >>> Due to bad AP design we can't handle this like zPCI - completely in >>> QEMU. That's what should control it. >>> >>>> 3) We implement this with a device in QEMU and a certain level kernel >>>> support. >>>> >>>> It seems possible to set or not ECA.28 , based on the type of kernel device: >>>> - SIE interpretation -> MATRIX KVM device -> ECA.28 >>>> - Interception with VFIO and virtualization -> no ECA.28 >>>> - interception with emulation -> no ECA.28 >>>> >>>> I understand the concern with the vCPU but I think we can handle it with >>>> an indirect variable >>>> like: >>>> >>>> SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable >>>> ap_to_be_sie_interpreted=1 >>>> Then in vCPU initialization set ECA.28 based on this variable. >>> That's exactly why we have the cpu feature interface. E.g. CMMA -> if >>> not enabled, not interpreted by HW (however also not intercepted to user >>> space - no sense in doing that right now). >> There are two factors at play here, the device type (i.e., -device >> vfio_ap) and >> the KVM_S390_VM_CPU_FEAT_AP feature. Setting ECA_APIE makes no sense if the >> vfio_ap device is not configured. For the passthrough implementation, this >> means that the AP bus will successfully load on the guest, but there will >> be no AP devices detected. If the expectation is that ap=on will allow >> access >> to AP devices on the guest, that would be a mistaken assumption. >> >> If down the road a new guest AP device is introduced that allows >> multiplexing, >> device remapping etc., then it will be necessary to intercept AP >> instructions >> executed on the guest. In this case, if ap=on results in setting ECA_APIE, >> then the instructions will be interpreted and passed through to a device >> that does not exist because it won't be defined in the guest's CRYCB. > Again, see my other mail, this discussion is superfluous if we can't get > vSIE to work with emulated devices. And it smells like this is the case. > But I don't have access to documentation. > >> Based on these two scenarios, I think what we are really saying with ap=on >> is that the guest will require use of the AP instructions installed on the >> host. Whether those instructions are executed as a result of interpretation >> by the hardware or because they are executed by the device driver is a >> separate matter. So, I am inclined to agree with Pierre for that reason. >> ECA_APIE should be set only if ap=on (i.e., AP instructions are available >> on the host) and the user of those instructions (i.e., the driver) indicate >> an intent to use them. > ap=on -> set ECA_APIE is what I proposed. True if we only support SIE interpretation, what you propose. but It is not what I meant. What I mean is the reverse implication ECA_APIE => ap=on But you can have ap = on and ECA_APIE = off This is interception or emulation. and the second thing is that we need two QEMU cpu features AP : guest API to say we provide AP instructions to the guest (what ever we do to provide it) ECA_APIE : kernel will setup the SIE with interpretation other said: if( !ap) return -ENODEVICE if(ECA_API) set_interpretation() else set_interception()
On Wed, 7 Mar 2018 11:09:46 +0100 Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > What I mean is the reverse implication > > ECA_APIE => ap=on > > But you can have ap = on and ECA_APIE = off > This is interception or emulation. > > and the second thing is that we need two QEMU cpu features > AP : guest API to say we provide AP instructions to the guest (what ever > we do to provide it) > ECA_APIE : kernel will setup the SIE with interpretation > > other said: > if( !ap) > return -ENODEVICE > if(ECA_API) > set_interpretation() > else > set_interception() This discussion is giving me a headache, so let's take a step back and figure out what is needed/wanted/possible. * straight passthrough of tuples, SIE doing the heavy lifting -> what this patchset is doing, and should be fine, even regarding nesting * remapping of tuples, SIE doing most of the work (IIRC it's possible to only intercept for a subset of instructions?) -> that's where it gets complicated, and IIUC we can't have any mixed straight/remap setups, and we may have issues regarding nesting Question: How important is that use case? Can we drop it and make our lives much easier? * full emulation (which would be the only option for tcg, obviously) -> even if it were doable, I doubt it would be very useful It would be great if we could have a design that would also accommodate this (and I have rooted for that in the past), but the more I hear about the issues here, the more I doubt whether this is something we should spend time on. Another question: Can some of the use cases be serviced via virtio-crypto as well (clear key)? Would that in combination with straight passthrough be enough?
On 07/03/2018 15:41, Cornelia Huck wrote: > On Wed, 7 Mar 2018 11:09:46 +0100 > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > >> What I mean is the reverse implication >> >> ECA_APIE => ap=on >> >> But you can have ap = on and ECA_APIE = off >> This is interception or emulation. >> >> and the second thing is that we need two QEMU cpu features >> AP : guest API to say we provide AP instructions to the guest (what ever >> we do to provide it) >> ECA_APIE : kernel will setup the SIE with interpretation >> >> other said: >> if( !ap) >> return -ENODEVICE >> if(ECA_API) >> set_interpretation() >> else >> set_interception() > This discussion is giving me a headache, so let's take a step back and > figure out what is needed/wanted/possible. > > * straight passthrough of tuples, SIE doing the heavy lifting > -> what this patchset is doing, and should be fine, even regarding > nesting > > * remapping of tuples, SIE doing most of the work Currently the SIE do not allow remapping. Only interception can allow remapping. > (IIRC it's possible > to only intercept for a subset of instructions?) More than possible: some AP instructions, when existing, are always intercepted and some other are intercepted based on a specific condition and on STFLE bits but for them SIE Execution control bit is ignored. However, we do not use these instructions in this patch series. > -> that's where it gets complicated, and IIUC we can't have any mixed > straight/remap setups, and we may have issues regarding nesting We do not have issues regarding nesting, we can not nest a guest doing SIE interpretation inside another doing interception. It is an architectural design, not an issue. To guaranty this, the architecture provide Effective Execution Control, EEC. The handling of ECA28 for guests execution is combined into a single description, the EECA, when operating at guest level 2 the EECA.28 is the logical AND of the guest level 1 ECA.28 and the guest level 2 ECA.28 When using vSIE we need to propagate this handling. > Question: How important is that use case? Can we drop it and make our > lives much easier? AFAIK, and as long as my information is up to date, we can not close the door to interception. In other word, we need to separate the CPU feature defining "if AP instructions are available" from the QEMU property defining "How we provide the instructions". ECA28 obviously belongs to the "how" and not to the "if". > > * full emulation (which would be the only option for tcg, obviously) > -> even if it were doable, I doubt it would be very useful > It would be great if we could have a design that would also > accommodate this (and I have rooted for that in the past), but the > more I hear about the issues here, the more I doubt whether this is > something we should spend time on. > > Another question: Can some of the use cases be serviced via > virtio-crypto as well (clear key)? Would that in combination with > straight passthrough be enough? >
On 03/07/2018 09:41 AM, Cornelia Huck wrote: > On Wed, 7 Mar 2018 11:09:46 +0100 > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > >> What I mean is the reverse implication >> >> ECA_APIE => ap=on >> >> But you can have ap = on and ECA_APIE = off >> This is interception or emulation. >> >> and the second thing is that we need two QEMU cpu features >> AP : guest API to say we provide AP instructions to the guest (what ever >> we do to provide it) >> ECA_APIE : kernel will setup the SIE with interpretation >> >> other said: >> if( !ap) >> return -ENODEVICE >> if(ECA_API) >> set_interpretation() >> else >> set_interception() > This discussion is giving me a headache, so let's take a step back and > figure out what is needed/wanted/possible. > > * straight passthrough of tuples, SIE doing the heavy lifting > -> what this patchset is doing, and should be fine, even regarding > nesting > > * remapping of tuples, SIE doing most of the work (IIRC it's possible > to only intercept for a subset of instructions?) Under the current architecture, instructions are either interpreted (with some instructions being intercepted under specific conditions) or intercepted. Therefore, when remapping tuples, it will be necessary to intercept all instructions. > -> that's where it gets complicated, and IIUC we can't have any mixed > straight/remap setups, and we may have issues regarding nesting As I said above, under the current architecture instructions are either interpreted (ECA.28 is set) or intercepted (ECA.28 is cleared). Consequently, we can't mix guests that use interpretation with guests that don't. > Question: How important is that use case? Can we drop it and make our > lives much easier? We've already had requests. > > * full emulation (which would be the only option for tcg, obviously) > -> even if it were doable, I doubt it would be very useful > It would be great if we could have a design that would also > accommodate this (and I have rooted for that in the past), but the > more I hear about the issues here, the more I doubt whether this is > something we should spend time on. If I'm not mistaken, the discussions about full emulation centered around problems related to second level guests (VSIE). It seems possible to employ full emulation for guest level 1. I'm not in a position to say whether it would be worth the effort or not. > > Another question: Can some of the use cases be serviced via > virtio-crypto as well (clear key)? Would that in combination with > straight passthrough be enough? I don't know enough about virtio-crypto to say. >
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c index 8aa5221..b93f7d9 100644 --- a/hw/vfio/ap.c +++ b/hw/vfio/ap.c @@ -19,6 +19,7 @@ #include "hw/s390x/ap-device.h" #include "qemu/error-report.h" #include "qemu/queue.h" +#include "cpu.h" #define VFIO_AP_DEVICE_TYPE "vfio-ap" #define AP_SYSFSDEV_PROP_NAME "sysfsdev" @@ -87,6 +88,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) Error *local_err = NULL; int ret; + if (!s390_has_feat(S390_FEAT_AP)) { + error_setg(&local_err, "Invalid device configuration: "); + error_append_hint(&local_err, + "Verify AP facilities are enabled for the guest" + "(ap=on)\n"); + goto out_err; + } + 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"), 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); + } + 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, }; 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}, }; static int query_cpu_feat(S390FeatBitmap features) @@ -2214,6 +2215,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) error_setg(errp, "KVM: Error querying CPU features: %d", rc); return; } + /* AP facilities support is required to enable QCI and APFT support */ + if (!test_bit(S390_FEAT_AP, model->features)) { + clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, model->features); + clear_bit(S390_FEAT_AP_FACILITIES_TEST, model->features); + } /* get supported cpu subfunctions indicated via query / test bit */ rc = query_cpu_subfunc(model->features); if (rc) {
A new CPU model feature and two new CPU model facilities are introduced to support AP devices for a KVM guest. CPU model features: 1. The KVM_S390_VM_CPU_FEAT_AP CPU model feature indicates that AP facilities are installed. This feature will be enabled by the kernel only if the AP facilities are installed on the linux host. This feature must be turned on from userspace to access AP devices from the KVM guest. The QEMU command line to turn this feature looks something like this: qemu-system-s390x ... -cpu xxx,ap=on CPU model facilities: 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates the AP Query Configuration Information (QCI) facility is installed. This feature will be enabled by the kernel only if the QCI is installed on the host. This facility will be set by QEMU only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on. (see CPU model features above) 2. The S390_FEAT_AP_FACILITY_TEST feature indicates that the AP Test Facility (APFT) facility is installed. This feature will be enabled by the kernel only if the APFT facility is installed on the host. This facility will be set by QEMU for the KVM guest only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on. (see CPU model features above) Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> --- hw/vfio/ap.c | 9 +++++++++ linux-headers/asm-s390/kvm.h | 1 + target/s390x/cpu_features.c | 3 +++ target/s390x/cpu_features_def.h | 3 +++ target/s390x/cpu_models.c | 12 ++++++++++++ target/s390x/gen-features.c | 3 +++ target/s390x/kvm.c | 6 ++++++ 7 files changed, 37 insertions(+), 0 deletions(-)