Re: [RFC 00/19] KVM: s390/crypto/vfio: guest dedicated crypto adapters

[Cornelia Huck <cohuck@redhat.com>]

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

> - which kind of devices we need

What is still unclear? Which card generations to support?

> - how to handle the repartition of queues on boot, reset and hotplug

That's something I'd like to see a writeup for.

> - interaction with the host drivers

The driver model should already handle that, no?

> - validation of the matrix for guests and host views

I saw validation code in the patches, although I have not reviewed it.

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

> - PAPQ/TAPQ-t and APQI interception

I can't say anything about that, as this is not documented :(

> - virtualization of the AP

Is this really needed? It would complicate everything a lot.

> - CPU model and KVM capabilities

That already has been discussed with the individual patches.

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