Re: [PATCH v5 16/21] KVM: vfio: add s390x hook to register KVM guest designation

Matthew Rosato
2022-04-12T14:32:39Z

On 4/12/22 9:55 AM, Jason Gunthorpe wrote:
> On Tue, Apr 12, 2022 at 09:39:44AM -0400, Matthew Rosato wrote:
>> On 4/8/22 8:45 AM, Jason Gunthorpe wrote:
>>> On Mon, Apr 04, 2022 at 01:43:44PM -0400, Matthew Rosato wrote:
>>>> At the time a KVM is associated with a vfio group, s390x zPCI devices
>>>> must register a special guest indication (GISA designation) to allow
>>>> for the use of interpretive execution facilities. This indication is
>>>> used to ensure that only the specified KVM can interact with the device.
>>>> Similarly, the indication must be removed once the KVM is no longer
>>>> associated with the device.
>>>>
>>>> This patch adds an s390-specific hook to invoke a KVM registration routine
>>>> for each device associated with the iommu group; in reality, it will be a
>>>> NOP for all but zPCI devices on s390x.
>>>>
>>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>>> virt/kvm/vfio.c | 35 ++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 34 insertions(+), 1 deletion(-)
>>>
>>> I wonder if this should be done in the vfio_pci side from the existing
>>> kvm notifier
>>>
>>
>> So you mean rather than hooking into virt as I do here, drive something out
>> of drivers/vfio/vfio.c:vfio_group_set_kvm? Note, the kvm notifier is
>> handled in vfio, not vfio_pci, so if you want to handle it in vfio_pci I
>> think we'd need to add a new routine to vfio_device_ops and only define it
>> vfio_pci for s390
>
> I've been thinking about doing that anyhow, exactly for reasons like
> this..
>
>> static const struct vfio_device_ops vfio_pci_ops = {
>> .name = "vfio-pci",
>> [...]
>> #ifdef CONFIG_S390
>> .set_kvm = vfio_pci_zdev_set_kvm,
>> #endif
>> };
>>
>> and something like...
>>
>> void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
>> {
>> struct vfio_device *vdev;
>> group->kvm = kvm;
>>
>> mutex_lock(&group->device_lock);
>> list_for_each_entry(vdev, &group->device_list, group_next) {
>> if (vdev->ops->set_kvm)
>> it->ops->set_kvm(vdev, kvm);
>> }
>> mutex_unlock(&group->device_lock);
>
> Almost, the device should be open before calling the callback
>
> And you have to inject a callback during open if the device is opened
> after the kvm was set.
>
> But I don't think you need to do this, you can just register a
> notifier in zpci when it hooks open_device like everything else,
> right?

Yes, that would also work -- I was registering a notifier for a few
prior versions of this series (granted, not from open_device) but got
the impression I should avoid registering a notifier from within
vfio_pci_zdev.

I will go ahead and add register/unregister notifiers hooked from
vfio_pci_core_finish_enable/vfio_pci_core_close_device for zpci (e.g.
vfio_pci_zdev_{open,close}) and use the notifier events to drive the
routines from patch 15.