Message-ID: <438d8b1e-e149-35f1-a8c9-ed338eb97430@linux.ibm.com>
Date:   Tue, 8 Feb 2022 15:33:58 -0500
From:   Matthew Rosato <mjrosato@...ux.ibm.com>
To:     Alex Williamson <alex.williamson@...hat.com>,
        Jason Gunthorpe <jgg@...dia.com>
Cc:     linux-s390@...r.kernel.org, cohuck@...hat.com,
        schnelle@...ux.ibm.com, farman@...ux.ibm.com, pmorel@...ux.ibm.com,
        borntraeger@...ux.ibm.com, hca@...ux.ibm.com, gor@...ux.ibm.com,
        gerald.schaefer@...ux.ibm.com, agordeev@...ux.ibm.com,
        frankja@...ux.ibm.com, david@...hat.com, imbrenda@...ux.ibm.com,
        vneethv@...ux.ibm.com, oberpar@...ux.ibm.com, freude@...ux.ibm.com,
        thuth@...hat.com, pasic@...ux.ibm.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 24/30] vfio-pci/zdev: wire up group notifier

On 2/8/22 2:26 PM, Alex Williamson wrote:
> On Tue, 8 Feb 2022 14:51:41 -0400
> Jason Gunthorpe <jgg@...dia.com> wrote:
> 
>> On Tue, Feb 08, 2022 at 10:43:19AM -0700, Alex Williamson wrote:
>>> On Fri,  4 Feb 2022 16:15:30 -0500
>>> Matthew Rosato <mjrosato@...ux.ibm.com> wrote:
>>>    
>>>> KVM zPCI passthrough device logic will need a reference to the associated
>>>> kvm guest that has access to the device.  Let's register a group notifier
>>>> for VFIO_GROUP_NOTIFY_SET_KVM to catch this information in order to create
>>>> an association between a kvm guest and the host zdev.
>>>>
>>>> Signed-off-by: Matthew Rosato <mjrosato@...ux.ibm.com>
>>>>   arch/s390/include/asm/kvm_pci.h  |  2 ++
>>>>   drivers/vfio/pci/vfio_pci_core.c |  2 ++
>>>>   drivers/vfio/pci/vfio_pci_zdev.c | 46 ++++++++++++++++++++++++++++++++
>>>>   include/linux/vfio_pci_core.h    | 10 +++++++
>>>>   4 files changed, 60 insertions(+)
>>>>
>>>> diff --git a/arch/s390/include/asm/kvm_pci.h b/arch/s390/include/asm/kvm_pci.h
>>>> index e4696f5592e1..16290b4cf2a6 100644
>>>> +++ b/arch/s390/include/asm/kvm_pci.h
>>>> @@ -16,6 +16,7 @@
>>>>   #include <linux/kvm.h>
>>>>   #include <linux/pci.h>
>>>>   #include <linux/mutex.h>
>>>> +#include <linux/notifier.h>
>>>>   #include <asm/pci_insn.h>
>>>>   #include <asm/pci_dma.h>
>>>>   
>>>> @@ -32,6 +33,7 @@ struct kvm_zdev {
>>>>   	u64 rpcit_count;
>>>>   	struct kvm_zdev_ioat ioat;
>>>>   	struct zpci_fib fib;
>>>> +	struct notifier_block nb;
>>>>   };
>>>>   
>>>>   int kvm_s390_pci_dev_open(struct zpci_dev *zdev);
>>>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
>>>> index f948e6cd2993..fc57d4d0abbe 100644
>>>> +++ b/drivers/vfio/pci/vfio_pci_core.c
>>>> @@ -452,6 +452,7 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
>>>>   
>>>>   	vfio_pci_vf_token_user_add(vdev, -1);
>>>>   	vfio_spapr_pci_eeh_release(vdev->pdev);
>>>> +	vfio_pci_zdev_release(vdev);
>>>>   	vfio_pci_core_disable(vdev);
>>>>   
>>>>   	mutex_lock(&vdev->igate);
>>>> @@ -470,6 +471,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_close_device);
>>>>   void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev)
>>>>   {
>>>>   	vfio_pci_probe_mmaps(vdev);
>>>> +	vfio_pci_zdev_open(vdev);
>>>>   	vfio_spapr_pci_eeh_open(vdev->pdev);
>>>>   	vfio_pci_vf_token_user_add(vdev, 1);
>>>>   }
>>>
>>> If this handling were for a specific device, I think we'd be suggesting
>>> this is the point at which we cross over to a vendor variant making use
>>> of vfio-pci-core rather than hooking directly into the core code.
>>
>> Personally, I think it is wrong layering for VFIO to be aware of KVM
>> like this. This marks the first time that VFIO core code itself is
>> being made aware of the KVM linkage.
> 
> I agree, but I've resigned that I've lost that battle.  Both mdev vGPU
> vendors make specific assumptions about running on a VM.  VFIO was
> never intended to be tied to KVM or the specific use case of a VM.
> 
>> It copies the same kind of design the s390 specific mdev use of
>> putting VFIO in charge of KVM functionality. If we are doing this we
>> should just give up and admit that KVM is a first-class part of struct
>> vfio_device and get rid of the notifier stuff too, at least for s390.
> 
> Euw.  You're right, I really don't like vfio core code embracing this
> dependency for s390, device specific use cases are bad enough.
> 
>> Reading the patches and descriptions pretty much everything is boiling
>> down to 'use vfio to tell the kvm architecture code to do something' -
>> which I think needs to be handled through a KVM side ioctl.
> 
> AIF at least sounds a lot like the reason we invented the irq bypass
> mechanism to allow interrupt producers and consumers to register
> independently and associate to each other with a shared token.

Yes, these do sound quite similar, looking at it now though I haven't 
yet fully grokked irq bypass...  But with AIF you have the case where 
either the interrupt will be delivered directly to a guest from firmware 
via an s390 construct (gisa) or under various circumstances the host 
(kvm) will be prodded to perform the delivery (still via gisa) instead.

> 
> Is the purpose of IOAT to associate the device to a set of KVM page
> tables?  That seems like a container or future iommufd operation.  I

Yes, here we are establishing a relationship with the DMA table in the 
guest so that once mappings are established guest PCI operations 
(handled via special instructions in s390) don't need to go through the 
host but can be directly handled by firmware (so, effectively guest can 
keep running on its vcpu vs breaking out).

> read DTSM as supported formats for the IOAT.
> 
>> Or, at the very least, everything needs to be described in some way
>> that makes it clear what is happening to userspace, without kvm,
>> through these ioctls.

Nothing, they don't need these ioctls.  Userspace without a KVM 
registration for the device in question gets -EINVAL.

> 
> As I understand the discussion here:
> 
> https://lore.kernel.org/all/20220204211536.321475-15-mjrosato@linux.ibm.com/
> 
> The assumption is that there is no non-KVM userspace currently.  This
> seems like a regression to me.

It's more that non-KVM userspace doesn't care about what these ioctls 
are doing...  The enabling of 'interp, aif, ioat' is only pertinent when 
there is a KVM userspace, specifically because the information being 
shared / actions being performed as a result are only relevant to 
properly enabling zPCI features when the zPCI device is being passed 
through to a VM guest.  If you're just using a userspace driver to talk 
to the device (no KVM guest involved) then the kernel zPCI layer already 
has this device set up using whatever s390 facilities are available.

> 
>> This seems especially true now that it seems s390 PCI support is
>> almost truely functional, with actual new userspace instructions to
>> issue MMIO operations that work outside of KVM.
>>
>> I'm not sure how this all fits together, but I would expect an outcome
>> where DPDK could run on these new systems and not have to know
>> anything more about s390 beyond using the proper MMIO instructions via
>> some compilation time enablement.
> 
> Yes, fully enabling zPCI with vfio, but only for KVM is not optimal.

See above.  I think there is a misunderstanding here, it's not that we 
are only enabling zPCI with vfio for KVM, but rather than when using 
vfio to pass the device to a guest there is additional work that has to 
happen in order to 'fully enable' zPCI.

> 
>> (I've been reviewing s390 patches updating rdma for a parallel set of
>> stuff)
>>
>>> this is meant to extend vfio-pci proper for the whole arch.  Is there a
>>> compromise in using #ifdefs in vfio_pci_ops to call into zpci specific
>>> code that implements these arch specific hooks and the core for
>>> everything else?  SPAPR code could probably converted similarly, it
>>> exists here for legacy reasons. [Cc Jason]
>>
>> I'm not sure I get what you are suggesting? Where would these ifdefs
>> be?
> 
> Essentially just:
> 
> static const struct vfio_device_ops vfio_pci_ops = {
>          .name           = "vfio-pci",
> #ifdef CONFIG_S390
>          .open_device    = vfio_zpci_open_device,
>          .close_device   = vfio_zpci_close_device,
>          .ioctl          = vfio_zpci_ioctl,
> #else
>          .open_device    = vfio_pci_open_device,
>          .close_device   = vfio_pci_core_close_device,
>          .ioctl          = vfio_pci_core_ioctl,
> #endif
>          .read           = vfio_pci_core_read,
>          .write          = vfio_pci_core_write,
>          .mmap           = vfio_pci_core_mmap,
>          .request        = vfio_pci_core_request,
>          .match          = vfio_pci_core_match,
> };
> 
> It would at least provide more validation/exercise of the core/vendor
> split.  Thanks,
> 
> Alex
> 

Powered by blists - more mailing lists