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

[Halil Pasic]

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