Messages in this thread | | | Subject | Re: [RFC 00/19] KVM: s390/crypto/vfio: guest dedicated crypto adapters | From | Pierre Morel <> | Date | Fri, 17 Nov 2017 08:07:15 +0100 |
| |
On 17/11/2017 00:35, Tony Krowiak wrote: > On 11/16/2017 03:25 PM, Pierre Morel wrote: >> On 16/11/2017 18:03, Cornelia Huck wrote: >>> On Thu, 16 Nov 2017 17:06:58 +0100 >>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: >>> >>>> On 16/11/2017 16:23, Tony Krowiak wrote: >>>>> On 11/14/2017 08:57 AM, Cornelia Huck wrote: >>>>>> On Tue, 31 Oct 2017 15:39:09 -0400 >>>>>> Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: >>>>>>> On 10/13/2017 01:38 PM, Tony Krowiak wrote: >>>>>>> Ping >>>>>>>> Tony Krowiak (19): >>>>>>>> KVM: s390: SIE considerations for AP Queue virtualization >>>>>>>> KVM: s390: refactor crypto initialization >>>>>>>> s390/zcrypt: new AP matrix bus >>>>>>>> s390/zcrypt: create an AP matrix device on the AP matrix bus >>>>>>>> s390/zcrypt: base implementation of AP matrix device driver >>>>>>>> s390/zcrypt: register matrix device with VFIO mediated device >>>>>>>> framework >>>>>>>> KVM: s390: introduce AP matrix configuration interface >>>>>>>> s390/zcrypt: support for assigning adapters to matrix mdev >>>>>>>> s390/zcrypt: validate adapter assignment >>>>>>>> s390/zcrypt: sysfs interfaces supporting AP domain assignment >>>>>>>> s390/zcrypt: validate domain assignment >>>>>>>> s390/zcrypt: sysfs support for control domain assignment >>>>>>>> s390/zcrypt: validate control domain assignment >>>>>>>> KVM: s390: Connect the AP mediated matrix device to KVM >>>>>>>> s390/zcrypt: introduce ioctl access to VFIO AP Matrix driver >>>>>>>> KVM: s390: interface to configure KVM guest's AP matrix >>>>>>>> KVM: s390: validate input to AP matrix config interface >>>>>>>> KVM: s390: New ioctl to configure KVM guest's AP matrix >>>>>>>> s390/facilities: enable AP facilities needed by guest >>>>>> I think the approach is fine, and the code also looks fine for the >>>>>> most >>>>>> part. Some comments: >>>>>> >>>>>> - various patches can be squashed together to give a better >>>>>> understanding at a glance >>>>> Which patches would you squash? >>>>>> - this needs documentation (as I already said) >>>>> My plan is to take the cover letter patch and incorporate that into >>>>> documentation, >>>>> then replace the cover letter patch with a more concise summary. >>>>>> - some of the driver/device modelling feels a bit awkward >>>>>> (commented in >>>>>> patches) -- I'm not sure that my proposal is better, but I >>>>>> think we >>>>>> should make sure the interdependencies are modeled correctly >>>>> I am responding to each patch review individually. >>>> >>>> I think that instead of responding to each patch individually we should >>>> have a discussion on the design because I think a lot could change and >>>> discussing about each patch as they may be completely redesigned for >>>> the >>>> next version may not be very useful. > How do you suggest this discussion should be structured? Aren't the patches > themselves an ultimate expression of the design? A lot could change, but > can't those issues can be dealt with and discussed as we move forward? > >>>> >>>> >>>> So I totally agree with Conny on that we should stabilize the >>>> bus/device/driver modeling. >>>> >>>> I think it would be here a good place to start the discussion on things >>>> like we started to discuss, Harald and I, off-line: >>>> - why a matrix bus, in which case we can avoid it >>> >>> I thought it had been agreed that we should be able to ditch it? >> >> I have not see any comment on the matrix bus. > As stated in a previous email responding to Connie, I decided to scrap the > AP matrix bus. There will only ever be one matrix device that serves two > purposes: To hold the APQNs of the queue devices bound to the VFIO AP > matrix > device driver; to serve as a parent of the mediated devices created for > guests requiring access to the APQNs reserved for their use. So, instead > of an AP matrix bus creating the matrix device, it will be created by the > VFIO AP matrix driver in /sys/devices/ap_matrix/ during driver > initialization.
Sorry, I did not see the mail, this of course change a lot of things...
>> >> >>> >>>> - which kind of devices we need >>> >>> What is still unclear? Which card generations to support? >> >> No, I mean the relation bus/device/driver/mdev... > I think that is pretty well spelled out in the cover letter > patch and in the descriptions of the patches themselves. What is it > you don't understand?
If we have no matrix bus anymore I prefer to wait for the cover letter of V2 to discuss this.
>> >> >>> >>>> - how to handle the repartition of queues on boot, reset and hotplug > What do you mean by repartition of queues on boot? >>> >>> That's something I'd like to see a writeup for. >> >> yes, and it may have an influence on the bus/device/driver/mdev design > I don't understand the need to avoid implementation details. If you recall, > the original design was modeled on AP queue devices. It was only after > implementing that design that the shortcomings were revealed which is > why we decided to base the model on the AP matrix. Keep in mind, this is > an RFC, not a final patch set. I would expect some change from the > implementation herein. In fact, I've already made many changes based on > Connie's and Christian's review comments, none of which resulted in an > overhaul of the design. >> >> >>> >>>> - interaction with the host drivers >>> >>> The driver model should already handle that, no? >> >> yes it should, but it is not clear for me. > What is it that is not clear? This cover letter seeks to describe the > patch set, so why not annotate those areas that are not clear? I'm don't > understand what it is you are expecting. I thought the purpose of > submitting these patches here was to generate discussion. >> >> >>> >>>> - validation of the matrix for guests and host views >>> >>> I saw validation code in the patches, although I have not reviewed it. > Patches 9, 11, and 13 validate the adapters, domains and control domains > configured for the mdev matrix device and patch 17 verifies that the > KVM guest's matrix does not define any APQNs in use by other guests. >>> >>> >>>> >>>> or even features we need to add like >>>> - interruptions >>> >>> My understanding is that interrupts are optional so they can be left >>> out in the first shot. With the gisa (that has not yet been posted), it >>> should not be too difficult, no? >> >> you are right I forgot that it is optional > If the facilities bit (STFLE.65) indicating interrupts are available is not > set for the guest, then the AP bus running on the guest will poll and > interrupts will not have to be handled. This patch set does not enable > interrupts, so it is not relevant at this time. We will not be able to > handle interrupts for the guest until the GISA for passthrough patches > are available. This will be addressed at that time. >> >> >>> >>>> - PAPQ/TAPQ-t and APQI interception >>> >>> I can't say anything about that, as this is not documented :( >> >> Right we can live without these too. > I have implemented interception of the PQAP(TAPQ) instruction and will > include it in the next set of patches. It was not documented here > because this patch set was submitted as an RFC for the purpose of > determining if we are on the right track with regard to the overall > AP matrix design. >> >> >>> >>>> - virtualization of the AP >>> >>> Is this really needed? It would complicate everything a lot. >> >> Concern has no sens without interception. > Virtualization of AP is not on the table right now.
If we implement interception, we must speak about this, even to say how we do not implement virtualization.
>> >>> >>>> - CPU model and KVM capabilities >>> >>> That already has been discussed with the individual patches. >> >> Well, if there are no interceptions the individual patches discussions >> are enough. > As I stated above, these patches were submitted as an RFC for the > purpose of > getting a stamp of approval for the general design. Additional functions > such as > hot plug and interception will be introduced in phases in the near > future. As > I stated above, I already have the implementation of PQAP(TAPQ) and will > include > it in the next submission. It does not change the general design one iota. >> >> >>> >>>> >>>> In my understanding these points must be cleared before we really start >>>> to discuss the details of the implementation. >>> >>> The general design already looks fine to me. Do you really expect that >>> a major redesign is needed? > I thought the point of submitting this RFC was to get a sanity check of the > design concepts. According to Christian, he discussed the design with > several maintainers at the KVM forum and they agreed this design was sane. >>> >> >> I am worry about the following: >> - Will the matrix bus be accepted > I am eliminating the matrix bus - based on comments made by Connie for an > individual patch - so no need to worry;-) >> >> - What happens on host reset and hot plug/unplug in host > TBD, but I don't anticipate a major overhaul of the design to accommodate > these eventualities, particularly hot plug which I considered while > creating this design. >> >> - What happens with the queues on guest start/halt/restart > TBD
AFAIU These two points are crucials for device driver design.
Pierre
>> >> Regards, >> >> Pierre >> >
-- Pierre Morel Linux/KVM/QEMU in Böblingen - Germany
| |