On 02/27/2018 05:27 PM, Cornelia Huck wrote:
> On Tue, 27 Feb 2018 10:44:19 -0500
> Tony Krowiak <akrow...@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 <akrow...@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).
