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 <pa...@linux.vnet.ibm.com> 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
