Re: [qemu-s390x] [PATCH v3 6/7] s390x/kvm: handle AP instruction interception

[David Hildenbrand]

On 16.03.2018 00:24, Tony Krowiak wrote:
> If the CPU model indicates that AP facility is installed on
> the guest (i.e., -cpu xxxx,ap=on), then the expectation is that
> the AP bus running in the guest will initialize; however, if the
> AP instructions are not being interpreted by the firmware, then
> they will be intercepted and routed back to QEMU for handling.
> If a handler is not defined to process the intercepted instruciton,
> then an operation exception will be injected into the
> guest, in which case the AP bus will not initialize.
> 
> There are two situations where AP instructions will not be
> interpreted:
> 
> 1. The guest is not configured with a vfio-ap device (i.e.,
>    -device vfio-ap,sysfsdev=$path-to-mdev). The realize function for
>    the vfio-ap device enables interpretive execution of AP
>    instructions.
> 
> 2. The guest is a second level guest but the first level guest has
>    not enabled interpretive execution.
> 
> This patch introduces AP instruction handlers to ensure the AP bus
> module initializes on the guest when the AP facility is installed
> on the guest but AP instructions are not being interpreted. The logic
> incorporated is:
> 
> * If the CPU model indicates AP instructions are
>   installed
> 
>   * Set the status response code for the instruction to indicate that
>     the APQN contained in the instruction is not valid. This is
>     a valid response because there will be no devices configured for
>     the guest in any of the above scenarios.
> 
> * Else return an error from the handler. This will result in an
>   operation being injected into the guest and the AP bus will not

s/operation/operation exception/

>   initialize on the guest. That is commensurate with how things work
>   today.
> 
> Signed-off-by: Tony Krowiak <address@hidden>
> ---
>  hw/vfio/ap.c                 |   45 
> ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/ap-device.h |    6 +++++
>  target/s390x/kvm.c           |   14 +++++++++++++
>  3 files changed, 65 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index b397bb1..88e744d 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -148,6 +148,51 @@ static void vfio_ap_unrealize(DeviceState *dev, Error 
> **errp)
>      kvm_s390_set_interpret_ap(0);
>  }
>  

I think you are missing cpu_synchronize_state(CPU(cpu)) in all of these
functions.

> +int ap_device_handle_nqap(S390CPU *cpu)
> +{
> +    CPUS390XState *env = &cpu->env;
> +
> +    if (s390_has_feat(S390_FEAT_AP)) {
> +        env->regs[1] = 0x10000;
> +
> +        return 0;
> +    }
> +
> +    return -EOPNOTSUPP;
> +}
> +
> +int ap_device_handle_dqap(S390CPU *cpu)
> +{
> +    CPUS390XState *env = &cpu->env;
> +
> +    if (s390_has_feat(S390_FEAT_AP)) {
> +        env->regs[1] = 0x10000;
> +
> +        return 0;
> +    }
> +
> +    return -EOPNOTSUPP;
> +}
> +
> +int ap_device_handle_pqap(S390CPU *cpu)
> +{
> +    CPUS390XState *env = &cpu->env;
> +    int fc = 4 & (env->regs[0] >> 24);
> +
> +    /*
> +     * The Query Configuration Information (QCI) function (fc == 4) does not
> +     * set a response code in reg 1, so check for that along with the
> +     * AP feature.
> +     */
> +    if ((fc != 4) && s390_has_feat(S390_FEAT_AP)) {
> +        env->regs[1] = 0x10000;
> +
> +        return 0;
> +    }

This would imply an operation exception in case fc==4, which sounds very
wrong.

Hard to review without access to documentation. (hard to understand why
such stuff is to be kept confidential for decades)

> +
> +    return -EOPNOTSUPP;
> +}
> +
>  static Property vfio_ap_properties[] = {
>      DEFINE_PROP_STRING("sysfsdev", VFIOAPDevice, vdev.sysfsdev),
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
> index 693df90..d45ae38 100644
> --- a/include/hw/s390x/ap-device.h
> +++ b/include/hw/s390x/ap-device.h
> @@ -11,6 +11,8 @@
>  #ifndef HW_S390X_AP_DEVICE_H
>  #define HW_S390X_AP_DEVICE_H
>  


-- 

Thanks,

David / dhildenb