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

[David Hildenbrand]

>>
>> 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.

Yes, very good summary.

> 
> 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.

The only reason really is that KVM_S390_VM_CPU_FEAT are set statically,
and cannot be changed later on.

> 
> 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

Any such changes will require QEMU changes too I assume. And enabling
ECA.28 from the kernel sounds wrong - it has no idea about how AP is
modeled in user space. But these are all things for the future and we
can discuss once they start to become real.

> 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.

Exactly, this is the virtual-ap mode then I guess.

> 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.
> 

For my feeling, the interface is more complicated than needed. E.g. what
happens if KVM_S390_VM_CPU_FEAT_AP is disabled but APIE enabled?
Inconsistent. So we are exporting AP via interface X and enabling it via
interface Y. That thing, I don't like.

> 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.

I will not block this work any further, you guys are obviously the AP
experts. Most things have been clarified by now. And I'll leave the
decision to you and the maintainers. I'm just here to ask stupid
questions :)

I am fine as long as:
- The default in KVM remains interception to QEMU

I am happy if:
- Interception can be enabled/disabled dynamically.

I am very happy if:
- The kernel interface for exposing/enabling AP (APIE) is a single
  mechanism.

My suggestion, either
a) Only use KVM_S390_VM_CPU_FEAT_AP to indicate/enable APIE statically
   for the guest.
b) Only use KVM_S390_VM_CRYPTO_ENABLE_APIE to indicate/enable APIE
   dynamically.

Both things will be simple to implement.

The difference really is that, with interface a), we will never be able
to support emulated devices in QEMU (as Christian mentioned, maybe that
is never the case). Because at the time the CPU model is configured, we
have no idea about which devices will get added. And we have to make a
decision (e.g. enable APIE) before creating CPUs and therefore before
creating AP devices. We will then need most probably a new interface
that allows us to enable/disable it dynamically.

With b) however, we have more flexibility. We can defer the decision.


As said, for me the mixture of a) and b) does not make sense as of now.
But if you have good reason to go that way, feel free.


Oh, and by the way:

If you are thinking about something like "the kernel might decide to
emulate AP instead of setting ECA.28, that would require yet another
control" - that can be easily handled by renaming
KVM_S390_VM_CRYPTO_ENABLE_APIE to something like
KVM_S390_VM_CRYPTO_ENABLE_AP_KERNEL. QEMU doesn't care about how it is
handled in the kernel, it just cares about "will somebody else handle AP
or do I get the intercepts (default)". That's how for me, an ideal
interface would look. But I might be missing something :)

-- 

Thanks,

David / dhildenb