Date:   Wed, 20 Nov 2019 02:38:08 -0500
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     Jason Wang <jasowang@...hat.com>,
        Parav Pandit <parav@...lanox.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Dave Ertman <david.m.ertman@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        Kiran Patil <kiran.patil@...el.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        "Bie, Tiwei" <tiwei.bie@...el.com>
Subject: Re: [net-next v2 1/1] virtual-bus: Implementation of Virtual Bus

On Tue, Nov 19, 2019 at 09:46:53PM -0400, Jason Gunthorpe wrote:
> On Tue, Nov 19, 2019 at 07:16:21PM -0500, Michael S. Tsirkin wrote:
> > On Tue, Nov 19, 2019 at 07:10:23PM -0400, Jason Gunthorpe wrote:
> > > On Tue, Nov 19, 2019 at 04:33:40PM -0500, Michael S. Tsirkin wrote:
> > > > On Tue, Nov 19, 2019 at 03:15:47PM -0400, Jason Gunthorpe wrote:
> > > > > On Tue, Nov 19, 2019 at 01:58:42PM -0500, Michael S. Tsirkin wrote:
> > > > > > On Tue, Nov 19, 2019 at 12:46:32PM -0400, Jason Gunthorpe wrote:
> > > > > > > As always, this is all very hard to tell without actually seeing real
> > > > > > > accelerated drivers implement this. 
> > > > > > > 
> > > > > > > Your patch series might be a bit premature in this regard.
> > > > > > 
> > > > > > Actually drivers implementing this have been posted, haven't they?
> > > > > > See e.g. https://lwn.net/Articles/804379/
> > > > > 
> > > > > Is that a real driver? It looks like another example quality
> > > > > thing. 
> > > > > 
> > > > > For instance why do we need any of this if it has '#define
> > > > > IFCVF_MDEV_LIMIT 1' ?
> > > > > 
> > > > > Surely for this HW just use vfio over the entire PCI function and be
> > > > > done with it?
> > > > 
> > > > What this does is allow using it with unmodified virtio drivers
> > > > within guests.  You won't get this with passthrough as it only
> > > > implements parts of virtio in hardware.
> > > 
> > > I don't mean use vfio to perform passthrough, I mean to use vfio to
> > > implement the software parts in userspace while vfio to talk to the
> > > hardware.
> > 
> > You repeated vfio twice here, hard to decode what you meant actually.
> 
> 'while using vifo to talk to the hardware'

Sorry still have trouble reading that.

> > >   kernel -> vfio -> user space virtio driver -> qemu -> guest
> >
> > Exactly what has been implemented for control path.
> 
> I do not mean the modified mediated vfio this series proposes, I mean
> vfio-pci, on a full PCI VF, exactly like we have today.
> 
> > The interface between vfio and userspace is
> > based on virtio which is IMHO much better than
> > a vendor specific one. userspace stays vendor agnostic.
> 
> Why is that even a good thing? It is much easier to provide drivers
> via qemu/etc in user space then it is to make kernel upgrades. We've
> learned this lesson many times.
> 
> This is why we have had the philosophy that if it doesn't need to be
> in the kernel it should be in userspace.
> 
> > > Generally we don't want to see things in the kernel that can be done
> > > in userspace, and to me, at least for this driver, this looks
> > > completely solvable in userspace.
> > 
> > I don't think that extends as far as actively encouraging userspace
> > drivers poking at hardware in a vendor specific way.  
> 
> Yes, it does, if you can implement your user space requirements using
> vfio then why do you need a kernel driver?

People's requirements differ. You are happy with just pass through a VF
you can already use it. Case closed. There are enough people who have
a fixed userspace that people have built virtio accelerators,
now there's value in supporting that, and a vendor specific
userspace blob is not supporting that requirement.

> The kernel needs to be involved when there are things only the kernel
> can do. If IFC has such things they should be spelled out to justify
> using a mediated device.
> 
> > That has lots of security and portability implications and isn't
> > appropriate for everyone. 
> 
> This is already using vfio.

It's using the IOMMU parts since these are portable.
But the userspace interface is vendor-independent here.

> It doesn't make sense to claim that using
> vfio properly is somehow less secure or less portable.
> 
> What I find particularly ugly is that this 'IFC VF NIC' driver
> pretends to be a mediated vfio device, but actually bypasses all the
> mediated device ops for managing dma security and just directly plugs
> the system IOMMU for the underlying PCI device into vfio.
> 
> I suppose this little hack is what is motivating this abuse of vfio in
> the first place?
> 
> Frankly I think a kernel driver touching a PCI function for which vfio
> is now controlling the system iommu for is a violation of the security
> model, and I'm very surprised AlexW didn't NAK this idea.
>
> Perhaps it is because none of the patches actually describe how the
> DMA security model for this so-called mediated device works? :(

That can be improved, good point.

> Or perhaps it is because this submission is split up so much it is
> hard to see what is being proposed? (I note this IFC driver is the
> first user of the mdev_set_iommu_device() function)

I agree it's hard, but then 3 people seem to work on that
at the same time.

> > It is kernel's job to abstract hardware away and present a unified
> > interface as far as possible.
> 
> Sure, you could create a virtio accelerator driver framework in our
> new drivers/accel I hear was started. That could make some sense, if
> we had HW that actually required/benefited from kernel involvement.
> 
> Jason

Powered by blists - more mailing lists