On Fri, 17 Nov 2017 08:07:15 +0100I agree.
Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx> wrote:
On 17/11/2017 00:35, Tony Krowiak wrote:FWIW, I think the basic design is already fine, and I don't think many
On 11/16/2017 03:25 PM, Pierre Morel wrote:
On 16/11/2017 18:03, Cornelia Huck wrote:How do you suggest this discussion should be structured? Aren't the patches
On Thu, 16 Nov 2017 17:06:58 +0100
Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx> wrote:
On 16/11/2017 16:23, Tony Krowiak wrote:
On 11/14/2017 08:57 AM, Cornelia Huck wrote:I think that instead of responding to each patch individually we should
On Tue, 31 Oct 2017 15:39:09 -0400Which patches would you squash?
Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> wrote:
On 10/13/2017 01:38 PM, Tony Krowiak wrote:I think the approach is fine, and the code also looks fine for the
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
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
part. Some comments:
- various patches can be squashed together to give a better
understanding at a glance
- this needs documentation (as I already said)My plan is to take the cover letter patch and incorporate that into
then replace the cover letter patch with a more concise summary.
- some of the driver/device modelling feels a bit awkwardI am responding to each patch review individually.
patches) -- I'm not sure that my proposal is better, but I
should make sure the interdependencies are modeled correctly
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
next version may not be very useful.
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?
of the points raised would become useless. For one, if something looks
a bit strange in detail, it might be a pointer to something bigger that
should be reshuffled (like where to anchor information).
I plan on including some drawings with the documentation and will include it
One thing that would be useful for the next iteration is some ascii-artSorry, I did not see the mail, this of course change a lot of things...As stated in a previous email responding to Connie, I decided to scrap theI have not see any comment on the matrix bus.I thought it had been agreed that we should be able to ditch it?
So I totally agree with Conny on that we should stabilize the
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
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
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
representation that shows how the different parts (matrix, ap driver,
mdev, ...) tie together. That also would be useful to have in the
I have spent some time thinking about hotplug implementation and I
I expect that any of the above can be accommodated by the design. AIf we have no matrix bus anymore I prefer to wait for the cover letterI think that is pretty well spelled out in the cover letterNo, I mean the relation bus/device/driver/mdev...- which kind of devices we needWhat is still unclear? Which card generations to support?
patch and in the descriptions of the patches themselves. What is it
you don't understand?
of V2 to discuss this.
What do you mean by repartition of queues on boot?- how to handle the repartition of queues on boot, reset and hotplug
I don't understand the need to avoid implementation details. If you recall,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
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.
short writeup of what we may want to do for that would certainly help
to validate that, though.
Unbinding from the host drivers and binding to the VFIO AP matrix driver
The simple fact of unbinding from the normal drivers and rebinding toWhat is it that is not clear? This cover letter seeks to describe theyes it should, but it is not clear for me.- interaction with the host driversThe driver model should already handle that, no?
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.
the specialized ones should already take care of interactions,
shouldn't it? Combined with the validation we should be all set.
Enabling AP interrupts is accomplished using the PQAP(AQIC) instruction
If you think it can be easily added later on, that would be fine forPatches 9, 11, and 13 validate the adapters, domains and control domains- validation of the matrix for guests and host viewsI saw validation code in the patches, although I have not reviewed it.
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.
If the facilities bit (STFLE.65) indicating interrupts are available is notyou are right I forgot that it is optionalor even features we need to add likeMy 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?
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.
me. (I cannot comment on gisa details until it has been posted,
A note that we do not plan to virtualize it right now would beIf we implement interception, we must speak about this, even to say howI have implemented interception of the PQAP(TAPQ) instruction and willRight we can live without these too.- PAPQ/TAPQ-t and APQI interceptionI can't say anything about that, as this is not documented :(
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 AP is not on the table right now.Concern has no sens without interception.- virtualization of the APIs this really needed? It would complicate everything a lot.
we do not implement virtualization.
I have done a little proof of concept code to get an idea if the AP matrix
From what I remember, this would mean opening a huge can of worms for
something that might be only of limited use. I'd prefer a simplistic
but usable approach first. If virtualization should really become a
requirement in the future, it might be better served by a different
The intersection was mainly with things like the cpu model. A differentAs I stated above, these patches were submitted as an RFC for theWell, if there are no interceptions the individual patches discussions- CPU model and KVM capabilitiesThat already has been discussed with the individual patches.
getting a stamp of approval for the general design. Additional functions
hot plug and interception will be introduced in phases in the near
I stated above, I already have the implementation of PQAP(TAPQ) and will
it in the next submission. It does not change the general design one iota.
part of the interface that does not really have an impact on the rest
of the design.
(That does not mean that we don't need to figure it out, of course.)
A writeup for these two points would be most welcome. I think the baseAFAIU These two points are crucials for device driver design.I thought the point of submitting this RFC was to get a sanity check of theIn my understanding these points must be cleared before we really startThe general design already looks fine to me. Do you really expect that
to discuss the details of the implementation.
a major redesign is needed?
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 eliminating the matrix bus - based on comments made by Connie for anI am worry about the following:
- Will the matrix bus be accepted
individual patch - so no need to worry;-)
- What happens on host reset and hot plug/unplug in hostTBD, 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/restartTBD
design is sane; it does not hurt to validate, though.