Re: [qemu-s390x] [PATCH v8 3/6] s390x/kvm: enable/disable AP instruction interpretation for guest

[Tony Krowiak]

On 09/17/2018 04:43 AM, David Hildenbrand wrote:
> Am 12.09.18 um 22:08 schrieb Tony Krowiak:
> > From: Tony Krowiak <address@hidden>
> >
> > Let's use the KVM_SET_DEVICE_ATTR ioctl to enable or disable
> > hardware interpretation of AP instructions executed on the guest.
> > If the S390_FEAT_AP feature is installed, AP instructions will
> > be interpreted by default; otherwise, they will be intercepted.
> > This attribute setting may be overridden by a device. For example,
> > a device may want to provide AP instructions to the guest (i.e.,
> > S390_FEAT_AP turned on), but it may want to emulate them. In this
> > case, the AP instructions executed on the guest must be
> > intercepted; so when the device is realized, it must disable
> > interpretation.
> >
> > Signed-off-by: Tony Krowiak <address@hidden>
> > ---
> >   target/s390x/kvm.c |   16 ++++++++++++++++
> >   1 files changed, 16 insertions(+), 0 deletions(-)
> >
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index c4bd84d..28a3900 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -2297,6 +2297,19 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, 
> > Error **errp)
> >                  S390_FEAT_MAX);
> >   }
> >   
> > +static void kvm_s390_configure_apie(int interpret)
> > +{
> > +    uint64_t attr = KVM_S390_VM_CRYPTO_DISABLE_APIE;
> > +
> > +    if (interpret) {
> > +        attr = KVM_S390_VM_CRYPTO_ENABLE_APIE;
> > +    }
> > +
> > +    if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, attr)) {
> > +            kvm_s390_set_attr(attr);
> > +    }
> > +}
> > +
> >   void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
> >   {
> >       struct kvm_s390_vm_cpu_processor prop  = {
> > @@ -2346,6 +2359,9 @@ void kvm_s390_apply_cpu_model(const S390CPUModel *model, 
> > Error **errp)
> >       if (test_bit(S390_FEAT_CMM, model->features)) {
> >           kvm_s390_enable_cmma();
> >       }
> > +
> > +    /* configure hardware interpretation of AP instructions */
> > +    kvm_s390_configure_apie(test_bit(S390_FEAT_AP, model->features));
> >   }
> >   
> As it is off as default,
>
> if (test_bit(S390_FEAT_AP, model->features)) {
>         kvm_s390_configure_apie(true, model->features));
> }
>
> will save one ioctl without AP when starting a guest.
>
>
> As mentioned as replay to patch #2, instead of KVM_S390_VM_CPU_FEAT_AP,
> I think we can simply do the following in kvm_s390_get_host_cpu_model()
>
>
> /* for now, we can only provide the AP feature with HW support */
> if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO,
>      KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
>         set_bit(S390_FEAT_AP, model->features);
> }
>
>
> Apart from that, looks good to me.

Let me summarize what I think you are suggesting.

For KVM:
1. Get rid of KVM_S390_VM_CPU_FEAT_AP in KVM
2. Make AP instruction interception the default.
3. Provide the KVM_S390_VM_CRYPTO_ENABLE_APIE attribute in KVM if the
   AP instructions are installed on the host

For QEMU:
1. In the kvm_s390_get_host_cpu_model(), set the S390_FEAT_AP CPU model
   feature bit if the KVM_S390_VM_CRYPTO_ENABLE_APIE attribute is available
   in KVM.
2. In the kvm_s390_apply_cpu_model() function, if the S390_FEAT_AP bit is
   set in the guest's CPU model (i.e., ap=on), use the KVM_SET_DEVICE_ATTR
   ioctl to set the KVM_S390_VM_CRYPTO_ENABLE_APIE attribute. This will
   ultimately result in ECA.28 being set to 1 (i.e., interpret AP instructions)
   for each vcpu.
3. Down the road, if a QEMU device for emulating AP is used, the
   KVM_S390_VM_CRYPTO_DISABLE_APIE attribute can be set to instruct KVM to
   intercept AP instructions. This can be done when the device is realized.

I've discussed this with Halil -- Pierre is out until next week. We
are in agreement that while these changes are viable, they result in
a slightly more complicated implementation compared to previous versions (e.g.
kernel v9 QEMU v7), and locks us into a model that may limit design choices
down the road if/when virtual and/or emulated AP devices are implemented.

Having said that, your proposal makes perfect sense under the assumptions that:
1) The default for ECA.28 has to be zero even if the guest is supposed to have
AP instructions installed, and
2) QEMU needs to set ECA.28 via ioctl.

I don't think, however, those assumptions are necessarily justified. What we
get does not expand on what we had in any meaningful way, but forces us to add
the two new KVM attributes (KVM_S390_VM_CRYPTO_ENABLE_APIE and 
KVM_S390_VM_CRYPTO_DISABLE_APIE)
which limits our choices for how we implement future enhancements to the AP
virtualization architecture.

Let's recap the previous behavior case by case and compare it with the one
proposed by you:

1) ap=off in the requested model:
                   --> ECA.28 not set
                   --> vfio-ap device has no bus to plug into in QEMU
                   --> QEMU injects operation exceptions
2) ap=on in the requested model
     2.1)  if host does not have ap instr:
                   --> KVM_S390_VM_CPU_FEAT_AP not advertised
                   --> since we don't provide emulation compatibility fails
                   --> guest does not start
                   --> ECA.28 does not matter but remains 0
     2.2)  if host does have ap instr:
                   --> KVM_S390_VM_CPU_FEAT_AP advertised
                   --> KVM_S390_VM_CPU_FEAT_AP applied
                   --> ECA.28 set

Your proposal achieves the exact same behavior, but relying on dedicated 
operations
(the attributes) as opposed to bulk operations (that are working with bitmaps) 
and
special handling in kvm_s390_apply_cpu_model(). You had some concern with
breaking compatibility by making ECA.28 set as the default, but since it's only
if KVM_S390_VM_CPU_FEAT_AP is applied, it is perfectly fine.

Why lock ourselves into having to use the new KVM attributes now, given it 
doesn't really buy
us anything. We may, for example, choose to control ECA.28 directly from a 
kernel driver, or
create an interface that lets us control ECA.28 and other AP-related bits 
currently not
supported with one call. Also, for things like address virtualization, we would 
need an additional
kernel interface anyway, since the NQAP and DQAP instructions that talk to the 
HW will have to be
issued by the host kernel. There is also no reason why these attributes 
couldn't be added at
the time we implement virtualization or emulation if we decide to go that route.

As I said at the start of this book, I do not object to implementing the model 
you've
suggested, but before moving ahead with that, I thought I'd bounce this off of 
you.
I will go whichever way you think best. As I stated, I have no major objections 
to your
design concepts.


>