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

[Halil Pasic]

On 04/06/2018 11:11 AM, David Hildenbrand wrote:
> On 06.04.2018 10:40, Cornelia Huck wrote:
>> On Thu, 5 Apr 2018 19:17:47 +0200
>> Halil Pasic <address@hidden> wrote:
>>
>>> On 04/05/2018 06:38 PM, Tony Krowiak wrote:
>>>>> Hard to really give good advice without access to the documentation, but:
>>>>> - If we tell the guest that the feature is available, but it does not
>>>>>    get any cards to use, returning an empty matrix makes the most sense
>>>>>    to me.
>>>>> - I would not tie starting the guest to the presence of a vfio-ap
>>>>>    device. Having the feature available in theory but without any
>>>>>    devices actually being usable by the guest does not really sound
>>>>>    wrong (can we hotplug this later?)  
>>>> For this phase of development, it is my opinion that introducing AP 
>>>> instruction
>>>> interception handlers is superfluous for the following reasons:
>>>>
>>>> 1. Interception handling was introduced solely to ensure an operation 
>>>> exception would
>>>>    not be injected into the guest when CPU model feature for AP (i.e., 
>>>> ap=on)
>>>>    is specified but a VFIO AP device (i.e., -device vfio-ap,sysfsdev=$path)
>>>>    is not.
>>>>
>>>> 2. The implementation of guest dedicated crypto adapters uses AP 
>>>> instruction
>>>>    interpretation to virtualize AP devices for a guest. As such, the NQAP,
>>>>    DQAP and most variants of the PQAP instructions will not be
>>>>    intercepted.
>>>>
>>>> 3. Hot plugging AP devices is not being supported for this phase of 
>>>> development.
>>>>
>>>> It is my opinion that introducing these interception handlers at this time 
>>>> is
>>>> unnecessary and risks opening a can of worms that would be
>>>> better dealt with in a subsequent phase. For that reason and the reasons 
>>>> stated
>>>> above, I think the better option is to terminate starting the guest if the
>>>> CPU model feature for AP is enabled but an AP device is not defined for the
>>>> guest. This restriction, of course, will be removed when hot plug is 
>>>> implemented
>>>> in a subsequent development phase.  
>>>
>>> I second that! I agree that having ap instructions but not having the
>>> possibility to actually do AP crypto is probably not what the user wants.
>>> Preventing such a guest form starting (with a nice message) sounds 
>>> reasonable
>>> to me.
>>
>> One problem I have with that is that it feels backwards to me.
>>
>> The situation "you cannot add this device unless $FEATURE is present"
>> is quite common and thus easily understood. Now, this would introduce
>> the situation "you cannot present $FEATURE unless this device is also
>> present, and that right at the start". I'm not sure how you are
>> supposed to correlate a cpu feature with the existence of a device.


I think it can be done straightforward and with less LOC than interception
and emulation of 'nothing to see here' requires.

> 
> I agree. Don't make things harder than they are. This smells like "cpu
> feature can only be provided if another magical QEMU command line option
> is present". Don't do that.

Yes it is conceptually ugly. I'm 100% with you. That's why it should go
away soon. From the practicality perspective however I would even argue that 
it's
helpful to the user: tells 'oops you have forgotten something'. IMHO
it's a shortcut of type make the problem smaller. Regarding what is
harder and what is easier: the author is probably the most fit to decide
that. If it is harder, it makes no sense, as this is all about cutting
corners.

> 
> Is it really that hard to implement a very simple interception handler
> that says t all instructions "yeah, I'm alive, but no, nothing to see here".
> 

I find it somewhat difficult to reason about what is static and what is
dynamic in the AP architecture. To put something together that seems to
work should be relatively easy. I could even say, I hope Tony tested the
no device case with v3 and it apparently seemed to work -- as I don't see
any does not work disclaimer. But getting all the stuff correct is IMHO
a bit more of a challenge.

>>
>>> I agree with Connie, the approach 'hold the line' (until future hotplugs)
>>> is the most reasonable thing to do *in the long run*. But I think it's 
>>> better
>>> to limit ourselves to the simplest case for now, I don't see any problems
>>> with doing the hotplug support later.
>>
>> Yes, having to add handlers that add very little benefit sucks, I
>> agree. But I fear if we add the "feature needs device" dependency, we
>> open another can of worms, including the question what happens if we
>> want to support hotplug in the future (I'm not altogether sure how to
>> handle the whole checking from qemu).
>>

We do want hotplug in the future AFAIK. The idea was to just remove the
limitation when everything is in place.

Regarding the implementation, the idea was to use 
qemu_add_machine_init_done_notifier and  only catch both of the following true
* the cpu model has ap=on
* on the ap bus (I think it would be nice to have a bus with max_dev = 1)
  there is no device

When hotplug becomes available for vfio-ap we would just remove that code.
For me it seemed legit. We have a precedence for not really complete stuff
with vfio-ccw. So if it was OK to defer the stuff that was deferred there,
I think, ap=on suddenly working without the strange device should be OK too.

>> Making sure that we have both the feature and the device when using a
>> management software (e.g. libvirt) makes a lot of sense (and is
>> probably also easier to implement), but it won't help us with the issue
>> of the interception handlers, unfortunately.
>>

I disagree. Conceptually hotplug of vfio-ap is perfectly legit. So doing
something like this in management software sounds wrong. The idea here
is cutting corners in order to have something that works reasonably well
but with a couple of well defined limitations sooner.

Regards,
Halil