* Re: [Qemu-devel] [RFC 0/3] VirtIO RDMA
From: Hannes Reinecke @ 2019-04-19 11:16 UTC (permalink / raw)
To: Yuval Shaia, Cornelia Huck
Cc: linux-rdma, jgg, virtualization, qemu-devel, mst
In-Reply-To: <20190415103546.GA6854@lap1>
On 4/15/19 12:35 PM, Yuval Shaia wrote:
> On Thu, Apr 11, 2019 at 07:02:15PM +0200, Cornelia Huck wrote:
>> On Thu, 11 Apr 2019 14:01:54 +0300
>> Yuval Shaia <yuval.shaia@oracle.com> wrote:
>>
>>> Data center backends use more and more RDMA or RoCE devices and more and
>>> more software runs in virtualized environment.
>>> There is a need for a standard to enable RDMA/RoCE on Virtual Machines.
>>>
>>> Virtio is the optimal solution since is the de-facto para-virtualizaton
>>> technology and also because the Virtio specification
>>> allows Hardware Vendors to support Virtio protocol natively in order to
>>> achieve bare metal performance.
>>>
>>> This RFC is an effort to addresses challenges in defining the RDMA/RoCE
>>> Virtio Specification and a look forward on possible implementation
>>> techniques.
>>>
>>> Open issues/Todo list:
>>> List is huge, this is only start point of the project.
>>> Anyway, here is one example of item in the list:
>>> - Multi VirtQ: Every QP has two rings and every CQ has one. This means that
>>> in order to support for example 32K QPs we will need 64K VirtQ. Not sure
>>> that this is reasonable so one option is to have one for all and
>>> multiplex the traffic on it. This is not good approach as by design it
>>> introducing an optional starvation. Another approach would be multi
>>> queues and round-robin (for example) between them.
>>>
Typically there will be a one-to-one mapping between QPs and CPUs (on
the guest). So while one would need to be prepared to support quite some
QPs, the expectation is that the actual number of QPs used will be
rather low.
In a similar vein, multiplexing QPs would be defeating the purpose, as
the overall idea was to have _independent_ QPs to enhance parallelism.
>>> Expectations from this posting:
>>> In general, any comment is welcome, starting from hey, drop this as it is a
>>> very bad idea, to yeah, go ahead, we really want it.
>>> Idea here is that since it is not a minor effort i first want to know if
>>> there is some sort interest in the community for such device.
>>
>> My first reaction is: Sounds sensible, but it would be good to have a
>> spec for this :)
>>
>> You'll need a spec if you want this to go forward anyway, so at least a
>> sketch would be good to answer questions such as how many virtqueues
>> you use for which purpose, what is actually put on the virtqueues,
>> whether there are negotiable features, and what the expectations for
>> the device and the driver are. It also makes it easier to understand
>> how this is supposed to work in practice.
>>
>> If folks agree that this sounds useful, the next step would be to
>> reserve an id for the device type.
>
> Thanks for the tips, will sure do that, it is that first i wanted to make
> sure there is a use case here.
>
> Waiting for any feedback from the community.
>
I really do like the ides; in fact, it saved me from coding a similar
thing myself :-)
However, I'm still curious about the overall intent of this driver.
Where would the I/O be routed _to_ ?
It's nice that we have a virtualized driver, but this driver is
intended to do I/O (even if it doesn't _do_ any I/O ATM :-)
And this I/O needs to be send to (and possibly received from)
something.
So what exactly is this something?
An existing piece of HW on the host?
If so, wouldn't it be more efficient to use vfio, either by using SR-IOV
or by using virtio-mdev?
Another guest?
If so, how would we route the I/O from one guest to the other?
Shared memory? Implementing a full-blown RDMA switch in qemu?
Oh, and I would _love_ to have a discussion about this at KVM Forum.
Maybe I'll manage to whip up guest-to-guest RDMA connection using
ivshmem ... let's see.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v5 1/6] libnvdimm: nd_region flush callback support
From: Dan Williams @ 2019-04-18 18:14 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Pankaj Gupta, cohuck, Jan Kara, KVM list, Michael S. Tsirkin,
david, Qemu Developers, virtualization, Andreas Dilger,
Ross Zwisler, Andrea Arcangeli, Dave Jiang, linux-nvdimm,
Vishal L Verma, Matthew Wilcox, Linux ACPI, Jeff Moyer,
linux-ext4, Len Brown, kilobyte, Rik van Riel, yuval shaia,
Stefan Hajnoczi, Paolo Bonzini, lcapitulino, Nites
In-Reply-To: <20190418161833.GA22970@infradead.org>
On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote:
> > > > I'd either add a comment about avoiding retpoline overhead here or just
> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't
> > > > get confused by the code.
> > >
> > > Isn't this premature optimization? I really don't like adding things
> > > like this without some numbers to show it's worth it.
> >
> > I don't think it's premature given this optimization technique is
> > already being deployed elsewhere, see:
> >
> > https://lwn.net/Articles/774347/
>
> For one this one was backed by numbers, and second after feedback
> from Linux we switched to the NULL pointer check instead.
Ok I should have noticed the switch to NULL pointer check. However,
the question still stands do we want everyone to run numbers to
justify this optimization, or make it a new common kernel coding
practice to do:
if (!object->op)
generic_op(object);
else
object->op(object);
...in hot paths? I agree with not doing premature optimization in
principle, but this hack is minimally intrusive from a readability
perspective similar to likely()/unlikely() usage which also don't come
with numbers on a per patch basis.
^ permalink raw reply
* Re: [PATCH v5 1/6] libnvdimm: nd_region flush callback support
From: Christoph Hellwig @ 2019-04-18 16:18 UTC (permalink / raw)
To: Dan Williams
Cc: Pankaj Gupta, cohuck, Jan Kara, KVM list, Michael S. Tsirkin,
david, Qemu Developers, virtualization, Andreas Dilger,
Ross Zwisler, Andrea Arcangeli, Dave Jiang, linux-nvdimm,
Vishal L Verma, Matthew Wilcox, Christoph Hellwig, Linux ACPI,
Jeff Moyer, linux-ext4, Len Brown, kilobyte, Rik van Riel,
yuval shaia, Stefan Hajnoczi, Paolo Bonzini
In-Reply-To: <CAPcyv4jrNHqW3FuGkeo==ZoJr7LKahCv4_esnokfy7enek8=2Q@mail.gmail.com>
On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote:
> > > I'd either add a comment about avoiding retpoline overhead here or just
> > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't
> > > get confused by the code.
> >
> > Isn't this premature optimization? I really don't like adding things
> > like this without some numbers to show it's worth it.
>
> I don't think it's premature given this optimization technique is
> already being deployed elsewhere, see:
>
> https://lwn.net/Articles/774347/
For one this one was backed by numbers, and second after feedback
from Linux we switched to the NULL pointer check instead.
^ permalink raw reply
* Re: [PATCH v5 1/6] libnvdimm: nd_region flush callback support
From: Jeff Moyer @ 2019-04-18 16:10 UTC (permalink / raw)
To: Dan Williams
Cc: Pankaj Gupta, cohuck, Jan Kara, KVM list, Michael S. Tsirkin,
david, Qemu Developers, virtualization, Andreas Dilger,
Ross Zwisler, Andrea Arcangeli, Dave Jiang, linux-nvdimm,
Vishal L Verma, Matthew Wilcox, Christoph Hellwig, Linux ACPI,
linux-ext4, Len Brown, kilobyte, Rik van Riel, yuval shaia,
Stefan Hajnoczi, Paolo Bonzini, lcapitulino
In-Reply-To: <CAPcyv4jrNHqW3FuGkeo==ZoJr7LKahCv4_esnokfy7enek8=2Q@mail.gmail.com>
Dan Williams <dan.j.williams@intel.com> writes:
> On Fri, Apr 12, 2019 at 6:12 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>>
>> Jan Kara <jack@suse.cz> writes:
>>
>> > On Thu 11-04-19 07:51:48, Dan Williams wrote:
>> >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta@redhat.com> wrote:
>> >> > + } else {
>> >> > + if (nd_region->flush(nd_region))
>> >> > + rc = -EIO;
>> >>
>> >> Given the common case wants to be fast and synchronous I think we
>> >> should try to avoid retpoline overhead by default. So something like
>> >> this:
>> >>
>> >> if (nd_region->flush == generic_nvdimm_flush)
>> >> rc = generic_nvdimm_flush(...);
>> >
>> > I'd either add a comment about avoiding retpoline overhead here or just
>> > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't
>> > get confused by the code.
>>
>> Isn't this premature optimization? I really don't like adding things
>> like this without some numbers to show it's worth it.
>
> I don't think it's premature given this optimization technique is
> already being deployed elsewhere, see:
>
> https://lwn.net/Articles/774347/
The technique is fine, but that doesn't mean it should be applied
everywhere. Is *this* code path really going to benefit from the
optimization?
-Jeff
^ permalink raw reply
* Re: [PATCH v5 1/6] libnvdimm: nd_region flush callback support
From: Dan Williams @ 2019-04-18 16:05 UTC (permalink / raw)
To: Jeff Moyer
Cc: Pankaj Gupta, cohuck, Jan Kara, KVM list, Michael S. Tsirkin,
david, Qemu Developers, virtualization, Andreas Dilger,
Ross Zwisler, Andrea Arcangeli, Dave Jiang, linux-nvdimm,
Vishal L Verma, Matthew Wilcox, Christoph Hellwig, Linux ACPI,
linux-ext4, Len Brown, kilobyte, Rik van Riel, yuval shaia,
Stefan Hajnoczi, Paolo Bonzini, lcapitulino
In-Reply-To: <x494l73z7th.fsf@segfault.boston.devel.redhat.com>
On Fri, Apr 12, 2019 at 6:12 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Jan Kara <jack@suse.cz> writes:
>
> > On Thu 11-04-19 07:51:48, Dan Williams wrote:
> >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta@redhat.com> wrote:
> >> > + } else {
> >> > + if (nd_region->flush(nd_region))
> >> > + rc = -EIO;
> >>
> >> Given the common case wants to be fast and synchronous I think we
> >> should try to avoid retpoline overhead by default. So something like
> >> this:
> >>
> >> if (nd_region->flush == generic_nvdimm_flush)
> >> rc = generic_nvdimm_flush(...);
> >
> > I'd either add a comment about avoiding retpoline overhead here or just
> > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't
> > get confused by the code.
>
> Isn't this premature optimization? I really don't like adding things
> like this without some numbers to show it's worth it.
I don't think it's premature given this optimization technique is
already being deployed elsewhere, see:
https://lwn.net/Articles/774347/
^ permalink raw reply
* Stats for XDP actions (was: Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames)
From: Toke Høiland-Jørgensen @ 2019-04-18 14:24 UTC (permalink / raw)
To: David Ahern, Jesper Dangaard Brouer
Cc: Jakub Kicinski, hawk, mst, netdev, John Fastabend, virtualization,
Saeed Mahameed, Daniel Borkmann, David Miller, Tariq Toukan
In-Reply-To: <bdcfedd6-465d-4485-e268-25c4ce6b9fcf@gmail.com>
David Ahern <dsahern@gmail.com> writes:
> On 2/4/19 3:53 AM, Jesper Dangaard Brouer wrote:
>> On Sat, 2 Feb 2019 14:27:26 -0700
>> David Ahern <dsahern@gmail.com> wrote:
>>
>>> On 1/31/19 1:15 PM, Jesper Dangaard Brouer wrote:
>>>>>
>>>>> David, Jesper, care to chime in where we ended up in that last thread
>>>>> discussion this?
>>>>
>>>> IHMO packets RX and TX on a device need to be accounted, in standard
>>>> counters, regardless of XDP. For XDP RX the packet is counted as RX,
>>>> regardless if XDP choose to XDP_DROP. On XDP TX which is via
>>>> XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to
>>>> account the packet in a TX counter (this if often delayed to DMA TX
>>>> completion handling). We cannot break the expectation that RX and TX
>>>> counter are visible to userspace stats tools. XDP should not make these
>>>> packets invisible.
>>>
>>> Agreed. What I was pushing on that last thread was Rx, Tx and dropped
>>> are all accounted by the driver in standard stats. Basically if the
>>> driver touched it, the driver's counters should indicate that.
>>
>> Sound like we all agree (except with the dropped counter, see below).
>>
>> Do notice that mlx5 driver doesn't do this. It is actually rather
>> confusing to use XDP on mlx5, as when XDP "consume" which include
>> XDP_DROP, XDP_REDIRECT or XDP_TX, then the driver standard stats are
>> not incremented... the packet is invisible to "ifconfig" stat based
>> tools.
>
> mlx5 needs some work. As I recall it still has the bug/panic removing
> xdp programs - at least I don't recall seeing a patch for it.
>
>>
>>
>>> The push back was on dropped packets and whether that counter should be
>>> bumped on XDP_DROP.
>>
>> My opinion is the XDP_DROP action should NOT increment the drivers drop
>> counter. First of all the "dropped" counter is also use for other
>> stuff, which will confuse that this counter express. Second, choosing
>> XDP_DROP is a policy choice, it still means it was RX-ed at the driver
>> level.
>>
>
> Understood. Hopefully in March I will get some time to come back to this
> and propose an idea on what I would like to see - namely, the admin has
> a config option at load time to enable driver counters versus custom map
> counters. (meaning the operator of the node chooses standard stats over
> strict performance.) But of course that means the drivers have the code
> to collect those stats.
Hi David
I don't recall seeing any follow-up on this. Did you have a chance to
formulate your ideas? :)
-Toke
^ permalink raw reply
* Re: [PATCH v3 09/26] compat_ioctl: move drivers to compat_ptr_ioctl
From: Stefan Hajnoczi @ 2019-04-18 11:10 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-usb, kvm, Michael S. Tsirkin, Alexander Shishkin,
Jarkko Sakkinen, virtualization, Benjamin Tissoires, linux-mtd,
Peter Huewe, linux1394-devel, linux-stm32, devel, Marek Vasut,
y2038, Richard Weinberger, Jason Gunthorpe, Jason Gunthorpe,
linux-arm-kernel, linux-input, Tomas Winkler, Alexandre Torgue,
Jiri Kosina, Alexander Viro, Willem de Bruijn <willemb>
In-Reply-To: <20190416202013.4034148-10-arnd@arndb.de>
[-- Attachment #1.1: Type: text/plain, Size: 1566 bytes --]
On Tue, Apr 16, 2019 at 10:19:47PM +0200, Arnd Bergmann wrote:
> Each of these drivers has a copy of the same trivial helper function to
> convert the pointer argument and then call the native ioctl handler.
>
> We now have a generic implementation of that, so use it.
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/char/ppdev.c | 12 +---------
> drivers/char/tpm/tpm_vtpm_proxy.c | 12 +---------
> drivers/firewire/core-cdev.c | 12 +---------
> drivers/hid/usbhid/hiddev.c | 11 +--------
> drivers/hwtracing/stm/core.c | 12 +---------
> drivers/misc/mei/main.c | 22 +----------------
> drivers/mtd/ubi/cdev.c | 36 +++-------------------------
> drivers/net/tap.c | 12 +---------
> drivers/staging/pi433/pi433_if.c | 12 +---------
> drivers/usb/core/devio.c | 16 +------------
> drivers/vfio/vfio.c | 39 +++----------------------------
> drivers/vhost/net.c | 12 +---------
> drivers/vhost/scsi.c | 12 +---------
> drivers/vhost/test.c | 12 +---------
> drivers/vhost/vsock.c | 12 +---------
> fs/fat/file.c | 13 +----------
> 16 files changed, 20 insertions(+), 237 deletions(-)
The vhost parts look good:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] vhost/scsi: drop unnecessary smp_mb__after_atomic()
From: Stefan Hajnoczi @ 2019-04-18 8:56 UTC (permalink / raw)
To: Jason Wang
Cc: Andrea Parri, kvm, Michael S. Tsirkin, linux-kernel,
Linux Virtualization, Stefan Hajnoczi
In-Reply-To: <c18ed846-940b-0b10-3d40-639b1f8dc499@redhat.com>
On Wed, Apr 17, 2019 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
> On 2019/4/16 下午6:47, Stefan Hajnoczi wrote:
> > The vhost_scsi.ko code used several atomic variables at one point.
> > Later they were dropped or converted to regular ints protected by a
> > mutex.
> >
> > The commit that made these changes left an unused smp_mb__after_atomic()
> > in vhost_scsi_set_endpoint(). It was previously used after incrementing
> > vhost_scsi->vhost_ref_cnt but this field has been dropped:
> >
> > - atomic_inc(&vs->vhost_ref_cnt);
> > smp_mb__after_atomic_inc();
> >
> > Reported-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > Fixes: 101998f6fcd680 ("tcm_vhost: Post-merge review changes requested by MST")
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > drivers/vhost/scsi.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > index 618fb6461017..c090d177bd75 100644
> > --- a/drivers/vhost/scsi.c
> > +++ b/drivers/vhost/scsi.c
> > @@ -1443,7 +1443,6 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
> > tpg->tv_tpg_vhost_count++;
> > tpg->vhost_scsi = vs;
> > vs_tpg[tpg->tport_tpgt] = tpg;
> > - smp_mb__after_atomic();
> > match = true;
> > }
> > mutex_unlock(&tpg->tv_tpg_mutex);
>
>
> Looks like Paolo has sent an identical patch before this one.
Yes, Paolo's patch is fine.
Stefan
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v5 1/6] libnvdimm: nd_region flush callback support
From: Pankaj Gupta @ 2019-04-18 6:27 UTC (permalink / raw)
To: Jeff Moyer, Jan Kara, Dan Williams
Cc: KVM list, Michael S. Tsirkin, david, Qemu Developers,
virtualization, Andreas Dilger, Ross Zwisler, Andrea Arcangeli,
Dave Jiang, linux-nvdimm, Vishal L Verma, Matthew Wilcox,
Christoph Hellwig, Linux ACPI, linux-ext4, Len Brown, kilobyte,
Rik van Riel, yuval shaia, Stefan Hajnoczi, Igor Mammedov,
lcapitulino, Nitesh Narayan Lal, Theodore Ts'o, Xiao
In-Reply-To: <x494l73z7th.fsf@segfault.boston.devel.redhat.com>
Hello,
Thank you for the suggestions on this.
>
> > On Thu 11-04-19 07:51:48, Dan Williams wrote:
> >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta@redhat.com> wrote:
> >> > + } else {
> >> > + if (nd_region->flush(nd_region))
> >> > + rc = -EIO;
> >>
> >> Given the common case wants to be fast and synchronous I think we
> >> should try to avoid retpoline overhead by default. So something like
> >> this:
> >>
> >> if (nd_region->flush == generic_nvdimm_flush)
> >> rc = generic_nvdimm_flush(...);
> >
> > I'd either add a comment about avoiding retpoline overhead here or just
> > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't
> > get confused by the code.
>
> Isn't this premature optimization? I really don't like adding things
> like this without some numbers to show it's worth it.
Can we add the optimization after this version is merged.
Best regards,
Pankaj
>
> -Jeff
>
>
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Thiago Jung Bauermann @ 2019-04-17 21:42 UTC (permalink / raw)
To: David Gibson
Cc: Mike Anderson, Michael S. Tsirkin, Jean-Philippe Brucker,
Benjamin Herrenschmidt, Alexey Kardashevskiy, Ram Pai,
linux-kernel, virtualization, Paul Mackerras, iommu, linuxppc-dev,
Christoph Hellwig
In-Reply-To: <20190325005710.GC29295@umbus>
David Gibson <david@gibson.dropbear.id.au> writes:
> On Sat, Mar 23, 2019 at 05:01:35PM -0400, Michael S. Tsirkin wrote:
>> On Thu, Mar 21, 2019 at 09:05:04PM -0300, Thiago Jung Bauermann wrote:
>> > Michael S. Tsirkin <mst@redhat.com> writes:
> [snip]
>> > >> > Is there any justification to doing that beyond someone putting
>> > >> > out slow code in the past?
>> > >>
>> > >> The definition of the ACCESS_PLATFORM flag is generic and captures the
>> > >> notion of memory access restrictions for the device. Unfortunately, on
>> > >> powerpc pSeries guests it also implies that the IOMMU is turned on
>> > >
>> > > IIUC that's really because on pSeries IOMMU is *always* turned on.
>> > > Platform has no way to say what you want it to say
>> > > which is bypass the iommu for the specific device.
>> >
>> > Yes, that's correct. pSeries guests running on KVM are in a gray area
>> > where theoretically they use an IOMMU but in practice KVM ignores it.
>> > It's unfortunate but it's the reality on the ground today. :-/
>
> Um.. I'm not sure what you mean by this. As far as I'm concerned
> there is always a guest-visible (paravirtualized) IOMMU, and that will
> be backed onto the host IOMMU when necessary.
There is, but vhost will ignore it and directly map the guest memory
when ACCESS_PLATFORM (the flag previously known as IOMMU_PLATFORM) isn't
set. From QEMU's hw/virtio/vhost.c:
static int vhost_dev_has_iommu(struct vhost_dev *dev)
{
VirtIODevice *vdev = dev->vdev;
return virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
}
static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
hwaddr *plen, int is_write)
{
if (!vhost_dev_has_iommu(dev)) {
return cpu_physical_memory_map(addr, plen, is_write);
} else {
return (void *)(uintptr_t)addr;
}
}
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Thiago Jung Bauermann @ 2019-04-17 21:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mike Anderson, Jean-Philippe Brucker, Benjamin Herrenschmidt,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
Paul Mackerras, iommu, linuxppc-dev, Christoph Hellwig,
David Gibson
In-Reply-To: <20190323165456-mutt-send-email-mst@kernel.org>
Michael S. Tsirkin <mst@redhat.com> writes:
> On Thu, Mar 21, 2019 at 09:05:04PM -0300, Thiago Jung Bauermann wrote:
>>
>> Michael S. Tsirkin <mst@redhat.com> writes:
>>
>> > On Wed, Mar 20, 2019 at 01:13:41PM -0300, Thiago Jung Bauermann wrote:
>> >> >From what I understand of the ACCESS_PLATFORM definition, the host will
>> >> only ever try to access memory addresses that are supplied to it by the
>> >> guest, so all of the secure guest memory that the host cares about is
>> >> accessible:
>> >>
>> >> If this feature bit is set to 0, then the device has same access to
>> >> memory addresses supplied to it as the driver has. In particular,
>> >> the device will always use physical addresses matching addresses
>> >> used by the driver (typically meaning physical addresses used by the
>> >> CPU) and not translated further, and can access any address supplied
>> >> to it by the driver. When clear, this overrides any
>> >> platform-specific description of whether device access is limited or
>> >> translated in any way, e.g. whether an IOMMU may be present.
>> >>
>> >> All of the above is true for POWER guests, whether they are secure
>> >> guests or not.
>> >>
>> >> Or are you saying that a virtio device may want to access memory
>> >> addresses that weren't supplied to it by the driver?
>> >
>> > Your logic would apply to IOMMUs as well. For your mode, there are
>> > specific encrypted memory regions that driver has access to but device
>> > does not. that seems to violate the constraint.
>>
>> Right, if there's a pre-configured 1:1 mapping in the IOMMU such that
>> the device can ignore the IOMMU for all practical purposes I would
>> indeed say that the logic would apply to IOMMUs as well. :-)
>>
>> I guess I'm still struggling with the purpose of signalling to the
>> driver that the host may not have access to memory addresses that it
>> will never try to access.
>
> For example, one of the benefits is to signal to host that driver does
> not expect ability to access all memory. If it does, host can
> fail initialization gracefully.
But why would the ability to access all memory be necessary or even
useful? When would the host access memory that the driver didn't tell it
to access?
>> >> >> > But the name "sev_active" makes me scared because at least AMD guys who
>> >> >> > were doing the sensible thing and setting ACCESS_PLATFORM
>> >> >>
>> >> >> My understanding is, AMD guest-platform knows in advance that their
>> >> >> guest will run in secure mode and hence sets the flag at the time of VM
>> >> >> instantiation. Unfortunately we dont have that luxury on our platforms.
>> >> >
>> >> > Well you do have that luxury. It looks like that there are existing
>> >> > guests that already acknowledge ACCESS_PLATFORM and you are not happy
>> >> > with how that path is slow. So you are trying to optimize for
>> >> > them by clearing ACCESS_PLATFORM and then you have lost ability
>> >> > to invoke DMA API.
>> >> >
>> >> > For example if there was another flag just like ACCESS_PLATFORM
>> >> > just not yet used by anyone, you would be all fine using that right?
>> >>
>> >> Yes, a new flag sounds like a great idea. What about the definition
>> >> below?
>> >>
>> >> VIRTIO_F_ACCESS_PLATFORM_NO_IOMMU This feature has the same meaning as
>> >> VIRTIO_F_ACCESS_PLATFORM both when set and when not set, with the
>> >> exception that the IOMMU is explicitly defined to be off or bypassed
>> >> when accessing memory addresses supplied to the device by the
>> >> driver. This flag should be set by the guest if offered, but to
>> >> allow for backward-compatibility device implementations allow for it
>> >> to be left unset by the guest. It is an error to set both this flag
>> >> and VIRTIO_F_ACCESS_PLATFORM.
>> >
>> > It looks kind of narrow but it's an option.
>>
>> Great!
>>
>> > I wonder how we'll define what's an iommu though.
>>
>> Hm, it didn't occur to me it could be an issue. I'll try.
I rephrased it in terms of address translation. What do you think of
this version? The flag name is slightly different too:
VIRTIO_F_ACCESS_PLATFORM_NO_TRANSLATION This feature has the same
meaning as VIRTIO_F_ACCESS_PLATFORM both when set and when not set,
with the exception that address translation is guaranteed to be
unnecessary when accessing memory addresses supplied to the device
by the driver. Which is to say, the device will always use physical
addresses matching addresses used by the driver (typically meaning
physical addresses used by the CPU) and not translated further. This
flag should be set by the guest if offered, but to allow for
backward-compatibility device implementations allow for it to be
left unset by the guest. It is an error to set both this flag and
VIRTIO_F_ACCESS_PLATFORM.
>> > Another idea is maybe something like virtio-iommu?
>>
>> You mean, have legacy guests use virtio-iommu to request an IOMMU
>> bypass? If so, it's an interesting idea for new guests but it doesn't
>> help with guests that are out today in the field, which don't have A
>> virtio-iommu driver.
>
> I presume legacy guests don't use encrypted memory so why do we
> worry about them at all?
They don't use encrypted memory, but a host machine will run a mix of
secure and legacy guests. And since the hypervisor doesn't know whether
a guest will be secure or not at the time it is launched, legacy guests
will have to be launched with the same configuration as secure guests.
>> >> > Is there any justification to doing that beyond someone putting
>> >> > out slow code in the past?
>> >>
>> >> The definition of the ACCESS_PLATFORM flag is generic and captures the
>> >> notion of memory access restrictions for the device. Unfortunately, on
>> >> powerpc pSeries guests it also implies that the IOMMU is turned on
>> >
>> > IIUC that's really because on pSeries IOMMU is *always* turned on.
>> > Platform has no way to say what you want it to say
>> > which is bypass the iommu for the specific device.
>>
>> Yes, that's correct. pSeries guests running on KVM are in a gray area
>> where theoretically they use an IOMMU but in practice KVM ignores it.
>> It's unfortunate but it's the reality on the ground today. :-/
>
> Well it's not just the reality, virt setups need something that
> emulated IOMMUs don't provide. That is not uncommon, e.g.
> intel's VTD has a "cache mode" field which AFAIK is only used for virt.
That's good to know. Thanks for this example.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH 3/3] virtio-gpu api: VIRTIO_GPU_F_RESSOURCE_V2
From: Chia-I Wu @ 2019-04-17 18:06 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: virtio, Tomeu Vizoso, Michael S. Tsirkin, David Airlie, open list,
ML dri-devel, Gurchetan Singh, David Airlie,
open list:VIRTIO CORE, NET AND BLOCK DRIVERS
In-Reply-To: <20190417095750.lre3xrg4dlgskfjg@sirius.home.kraxel.org>
On Wed, Apr 17, 2019 at 2:57 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Apr 12, 2019 at 04:34:20PM -0700, Chia-I Wu wrote:
> > Hi,
> >
> > I am still new to virgl, and missed the last round of discussion about
> > resource_create_v2.
> >
> > From the discussion below, semantically resource_create_v2 creates a host
> > resource object _without_ any storage; memory_create creates a host memory
> > object which provides the storage. Is that correct?
>
> Right now all resource_create_* variants create a resource object with
> host storage. memory_create creates guest storage, and
> resource_attach_memory binds things together. Then you have to transfer
> the data.
In Gurchetan's Vulkan example, the host storage allocation happens in
(some variant of) memory_create, not in resource_create_v2. Maybe
that's what got me confused.
>
> Hmm, maybe we need a flag indicating that host storage is not needed,
> for resources where we want establish some kind of shared mapping later
> on.
This makes sense, to support both Vulkan and non-Vulkan models.
This differs from this patch, but I think a full-fledged resource
should logically have three components
- a RESOURCE component that has not storage
- a MEMORY component that provides the storage
- a BACKING component that is for transfers
resource_attach_backing sets the BACKING component. BACKING always
uses guest pages and supports only transfers into or out of MEMORY.
resource_attach_memory sets the MEMORY component. MEMORY can use host
or guest pages, and must always support GPU operations. When a MEMORY
is mappable in the guest, we can skip BACKING and achieve zero-copy.
resource_create_* can then get a flag to indicate whether only
RESOURCE is created or RESOURCE+MEMORY is created.
>
> > Do we expect these new commands to be supported by OpenGL, which does not
> > separate resources and memories?
>
> Well, for opengl you need a 1:1 relationship between memory region and
> resource.
>
> > > Yes, even though it is not clear yet how we are going to handle
> > > host-allocated buffers in the vhost-user case ...
> >
> > This might be another dumb question, but is this only an issue for
> > vhost-user(-gpu) case? What mechanisms are used to map host dma-buf into
> > the guest address space?
>
> qemu can change the address space, that includes mmap()ing stuff there.
> An external vhost-user process can't do this, it can only read the
> address space layout, and read/write from/to guest memory.
I thought vhost-user process can work with the host-allocated dmabuf
directly. That is,
qemu: injects dmabuf pages into guest address space
vhost-user: work with the dmabuf
guest: can read/write those pages
>
> > But one needs to create the resource first to know which memory types can
> > be attached to it. I think the metadata needs to be returned with
> > resource_create_v2.
>
> There is a resource_info reply for that.
>
> > That should be good enough. But by returning alignments, we can minimize
> > the gaps when attaching multiple resources, especially when the resources
> > are only used by GPU.
>
> We can add alignments to the resource_info reply.
>
> cheers,
> Gerd
>
^ permalink raw reply
* Re: [PATCH 3/3] virtio-gpu api: VIRTIO_GPU_F_RESSOURCE_V2
From: Gerd Hoffmann @ 2019-04-17 9:57 UTC (permalink / raw)
To: Chia-I Wu
Cc: virtio, Tomeu Vizoso, Michael S. Tsirkin, David Airlie, open list,
ML dri-devel, Gurchetan Singh, David Airlie,
open list:VIRTIO CORE, NET AND BLOCK DRIVERS
In-Reply-To: <CAPaKu7QsDp=kRsMzMf0YcvafsXBsr_m4bstuZiWAcmYiV_AxnQ@mail.gmail.com>
On Fri, Apr 12, 2019 at 04:34:20PM -0700, Chia-I Wu wrote:
> Hi,
>
> I am still new to virgl, and missed the last round of discussion about
> resource_create_v2.
>
> From the discussion below, semantically resource_create_v2 creates a host
> resource object _without_ any storage; memory_create creates a host memory
> object which provides the storage. Is that correct?
Right now all resource_create_* variants create a resource object with
host storage. memory_create creates guest storage, and
resource_attach_memory binds things together. Then you have to transfer
the data.
Hmm, maybe we need a flag indicating that host storage is not needed,
for resources where we want establish some kind of shared mapping later
on.
> Do we expect these new commands to be supported by OpenGL, which does not
> separate resources and memories?
Well, for opengl you need a 1:1 relationship between memory region and
resource.
> > Yes, even though it is not clear yet how we are going to handle
> > host-allocated buffers in the vhost-user case ...
>
> This might be another dumb question, but is this only an issue for
> vhost-user(-gpu) case? What mechanisms are used to map host dma-buf into
> the guest address space?
qemu can change the address space, that includes mmap()ing stuff there.
An external vhost-user process can't do this, it can only read the
address space layout, and read/write from/to guest memory.
> But one needs to create the resource first to know which memory types can
> be attached to it. I think the metadata needs to be returned with
> resource_create_v2.
There is a resource_info reply for that.
> That should be good enough. But by returning alignments, we can minimize
> the gaps when attaching multiple resources, especially when the resources
> are only used by GPU.
We can add alignments to the resource_info reply.
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH] vhost/scsi: drop unnecessary smp_mb__after_atomic()
From: Jason Wang @ 2019-04-17 5:57 UTC (permalink / raw)
To: Stefan Hajnoczi, kvm
Cc: Andrea Parri, Michael S. Tsirkin, linux-kernel, virtualization
In-Reply-To: <20190416104748.26942-1-stefanha@redhat.com>
On 2019/4/16 下午6:47, Stefan Hajnoczi wrote:
> The vhost_scsi.ko code used several atomic variables at one point.
> Later they were dropped or converted to regular ints protected by a
> mutex.
>
> The commit that made these changes left an unused smp_mb__after_atomic()
> in vhost_scsi_set_endpoint(). It was previously used after incrementing
> vhost_scsi->vhost_ref_cnt but this field has been dropped:
>
> - atomic_inc(&vs->vhost_ref_cnt);
> smp_mb__after_atomic_inc();
>
> Reported-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Fixes: 101998f6fcd680 ("tcm_vhost: Post-merge review changes requested by MST")
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> drivers/vhost/scsi.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 618fb6461017..c090d177bd75 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1443,7 +1443,6 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
> tpg->tv_tpg_vhost_count++;
> tpg->vhost_scsi = vs;
> vs_tpg[tpg->tport_tpgt] = tpg;
> - smp_mb__after_atomic();
> match = true;
> }
> mutex_unlock(&tpg->tv_tpg_mutex);
Looks like Paolo has sent an identical patch before this one.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] vhost-scsi: remove incorrect memory barrier
From: Jason Wang @ 2019-04-17 5:56 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel; +Cc: Andrea Parri, virtualization, stefanha, mst
In-Reply-To: <1555404968-39927-1-git-send-email-pbonzini@redhat.com>
On 2019/4/16 下午4:56, Paolo Bonzini wrote:
> At this point, vs_tpg is not public at all; tv_tpg_vhost_count
> is accessed under tpg->tv_tpg_mutex; tpg->vhost_scsi is
> accessed under vhost_scsi_mutex. Therefor there are no atomic
> operations involved at all here, just remove the barrier.
>
> Reported-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> drivers/vhost/scsi.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 618fb6461017..c090d177bd75 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1443,7 +1443,6 @@ static void vhost_scsi_flush(struct vhost_scsi *vs)
> tpg->tv_tpg_vhost_count++;
> tpg->vhost_scsi = vs;
> vs_tpg[tpg->tport_tpgt] = tpg;
> - smp_mb__after_atomic();
> match = true;
> }
> mutex_unlock(&tpg->tv_tpg_mutex);
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 09/26] compat_ioctl: move drivers to compat_ptr_ioctl
From: Jiri Kosina @ 2019-04-16 20:31 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-usb, kvm, Michael S. Tsirkin, Alexander Shishkin,
Jarkko Sakkinen, virtualization, Benjamin Tissoires, linux-mtd,
Peter Huewe, linux1394-devel, linux-stm32, devel, Jason Gunthorpe,
y2038, Richard Weinberger, Marek Vasut, Jason Gunthorpe,
linux-arm-kernel, linux-input, Tomas Winkler, Alexandre Torgue,
Alexander Viro, Stefan Hajnoczi, Willem
In-Reply-To: <20190416202013.4034148-10-arnd@arndb.de>
On Tue, 16 Apr 2019, Arnd Bergmann wrote:
> Each of these drivers has a copy of the same trivial helper function to
> convert the pointer argument and then call the native ioctl handler.
>
> We now have a generic implementation of that, so use it.
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/char/ppdev.c | 12 +---------
> drivers/char/tpm/tpm_vtpm_proxy.c | 12 +---------
> drivers/firewire/core-cdev.c | 12 +---------
> drivers/hid/usbhid/hiddev.c | 11 +--------
For hiddev.c:
Reviewed-by: Jiri Kosina <jkosina@suse.cz>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH v3 09/26] compat_ioctl: move drivers to compat_ptr_ioctl
From: Arnd Bergmann @ 2019-04-16 20:19 UTC (permalink / raw)
To: Alexander Viro
Cc: linux-usb, kvm, Michael S. Tsirkin, Alexander Shishkin,
Jarkko Sakkinen, virtualization, Benjamin Tissoires, linux-mtd,
Peter Huewe, linux1394-devel, linux-stm32, devel, Marek Vasut,
y2038, Richard Weinberger, Jason Gunthorpe, Jason Gunthorpe,
linux-arm-kernel, linux-input, Tomas Winkler, Alexandre Torgue,
Arnd Bergmann, Jiri Kosina, Stefan Hajnoczi
In-Reply-To: <20190416202013.4034148-1-arnd@arndb.de>
Each of these drivers has a copy of the same trivial helper function to
convert the pointer argument and then call the native ioctl handler.
We now have a generic implementation of that, so use it.
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/char/ppdev.c | 12 +---------
drivers/char/tpm/tpm_vtpm_proxy.c | 12 +---------
drivers/firewire/core-cdev.c | 12 +---------
drivers/hid/usbhid/hiddev.c | 11 +--------
drivers/hwtracing/stm/core.c | 12 +---------
drivers/misc/mei/main.c | 22 +----------------
drivers/mtd/ubi/cdev.c | 36 +++-------------------------
drivers/net/tap.c | 12 +---------
drivers/staging/pi433/pi433_if.c | 12 +---------
drivers/usb/core/devio.c | 16 +------------
drivers/vfio/vfio.c | 39 +++----------------------------
drivers/vhost/net.c | 12 +---------
drivers/vhost/scsi.c | 12 +---------
drivers/vhost/test.c | 12 +---------
drivers/vhost/vsock.c | 12 +---------
fs/fat/file.c | 13 +----------
16 files changed, 20 insertions(+), 237 deletions(-)
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index 1ae77b41050a..e96c8d9623e0 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -674,14 +674,6 @@ static long pp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
return ret;
}
-#ifdef CONFIG_COMPAT
-static long pp_compat_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- return pp_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
-}
-#endif
-
static int pp_open(struct inode *inode, struct file *file)
{
unsigned int minor = iminor(inode);
@@ -790,9 +782,7 @@ static const struct file_operations pp_fops = {
.write = pp_write,
.poll = pp_poll,
.unlocked_ioctl = pp_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = pp_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.open = pp_open,
.release = pp_release,
};
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index d74f3de74ae6..fb845f0a430b 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -675,20 +675,10 @@ static long vtpmx_fops_ioctl(struct file *f, unsigned int ioctl,
}
}
-#ifdef CONFIG_COMPAT
-static long vtpmx_fops_compat_ioctl(struct file *f, unsigned int ioctl,
- unsigned long arg)
-{
- return vtpmx_fops_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
-}
-#endif
-
static const struct file_operations vtpmx_fops = {
.owner = THIS_MODULE,
.unlocked_ioctl = vtpmx_fops_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = vtpmx_fops_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.llseek = noop_llseek,
};
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 16a7045736a9..fb934680fdd3 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1659,14 +1659,6 @@ static long fw_device_op_ioctl(struct file *file,
return dispatch_ioctl(file->private_data, cmd, (void __user *)arg);
}
-#ifdef CONFIG_COMPAT
-static long fw_device_op_compat_ioctl(struct file *file,
- unsigned int cmd, unsigned long arg)
-{
- return dispatch_ioctl(file->private_data, cmd, compat_ptr(arg));
-}
-#endif
-
static int fw_device_op_mmap(struct file *file, struct vm_area_struct *vma)
{
struct client *client = file->private_data;
@@ -1808,7 +1800,5 @@ const struct file_operations fw_device_ops = {
.mmap = fw_device_op_mmap,
.release = fw_device_op_release,
.poll = fw_device_op_poll,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = fw_device_op_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
};
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index a746017fac17..ef4a1cd389d6 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -855,13 +855,6 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
return r;
}
-#ifdef CONFIG_COMPAT
-static long hiddev_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
- return hiddev_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
-}
-#endif
-
static const struct file_operations hiddev_fops = {
.owner = THIS_MODULE,
.read = hiddev_read,
@@ -871,9 +864,7 @@ static const struct file_operations hiddev_fops = {
.release = hiddev_release,
.unlocked_ioctl = hiddev_ioctl,
.fasync = hiddev_fasync,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = hiddev_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.llseek = noop_llseek,
};
diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index c7ba8acfd4d5..454da259f144 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -840,23 +840,13 @@ stm_char_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
return err;
}
-#ifdef CONFIG_COMPAT
-static long
-stm_char_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
- return stm_char_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
-}
-#else
-#define stm_char_compat_ioctl NULL
-#endif
-
static const struct file_operations stm_fops = {
.open = stm_char_open,
.release = stm_char_release,
.write = stm_char_write,
.mmap = stm_char_mmap,
.unlocked_ioctl = stm_char_ioctl,
- .compat_ioctl = stm_char_compat_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
.llseek = no_llseek,
};
diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
index 87281b3695e6..cc6af92cdef0 100644
--- a/drivers/misc/mei/main.c
+++ b/drivers/misc/mei/main.c
@@ -535,24 +535,6 @@ static long mei_ioctl(struct file *file, unsigned int cmd, unsigned long data)
return rets;
}
-/**
- * mei_compat_ioctl - the compat IOCTL function
- *
- * @file: pointer to file structure
- * @cmd: ioctl command
- * @data: pointer to mei message structure
- *
- * Return: 0 on success , <0 on error
- */
-#ifdef CONFIG_COMPAT
-static long mei_compat_ioctl(struct file *file,
- unsigned int cmd, unsigned long data)
-{
- return mei_ioctl(file, cmd, (unsigned long)compat_ptr(data));
-}
-#endif
-
-
/**
* mei_poll - the poll function
*
@@ -855,9 +837,7 @@ static const struct file_operations mei_fops = {
.owner = THIS_MODULE,
.read = mei_read,
.unlocked_ioctl = mei_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = mei_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.open = mei_open,
.release = mei_release,
.write = mei_write,
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 947a8adbc799..265d34fa3efa 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -1091,36 +1091,6 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd,
return err;
}
-#ifdef CONFIG_COMPAT
-static long vol_cdev_compat_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- unsigned long translated_arg = (unsigned long)compat_ptr(arg);
-
- return vol_cdev_ioctl(file, cmd, translated_arg);
-}
-
-static long ubi_cdev_compat_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- unsigned long translated_arg = (unsigned long)compat_ptr(arg);
-
- return ubi_cdev_ioctl(file, cmd, translated_arg);
-}
-
-static long ctrl_cdev_compat_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- unsigned long translated_arg = (unsigned long)compat_ptr(arg);
-
- return ctrl_cdev_ioctl(file, cmd, translated_arg);
-}
-#else
-#define vol_cdev_compat_ioctl NULL
-#define ubi_cdev_compat_ioctl NULL
-#define ctrl_cdev_compat_ioctl NULL
-#endif
-
/* UBI volume character device operations */
const struct file_operations ubi_vol_cdev_operations = {
.owner = THIS_MODULE,
@@ -1131,7 +1101,7 @@ const struct file_operations ubi_vol_cdev_operations = {
.write = vol_cdev_write,
.fsync = vol_cdev_fsync,
.unlocked_ioctl = vol_cdev_ioctl,
- .compat_ioctl = vol_cdev_compat_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
};
/* UBI character device operations */
@@ -1139,13 +1109,13 @@ const struct file_operations ubi_cdev_operations = {
.owner = THIS_MODULE,
.llseek = no_llseek,
.unlocked_ioctl = ubi_cdev_ioctl,
- .compat_ioctl = ubi_cdev_compat_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
};
/* UBI control character device operations */
const struct file_operations ubi_ctrl_cdev_operations = {
.owner = THIS_MODULE,
.unlocked_ioctl = ctrl_cdev_ioctl,
- .compat_ioctl = ctrl_cdev_compat_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
.llseek = no_llseek,
};
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 2ea9b4976f4a..ebe425e65992 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -1123,14 +1123,6 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
}
}
-#ifdef CONFIG_COMPAT
-static long tap_compat_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- return tap_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
-}
-#endif
-
static const struct file_operations tap_fops = {
.owner = THIS_MODULE,
.open = tap_open,
@@ -1140,9 +1132,7 @@ static const struct file_operations tap_fops = {
.poll = tap_poll,
.llseek = no_llseek,
.unlocked_ioctl = tap_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = tap_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
};
static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b2314636dc89..ab7dfc7c2917 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -935,16 +935,6 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
return retval;
}
-#ifdef CONFIG_COMPAT
-static long
-pi433_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
-{
- return pi433_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
-}
-#else
-#define pi433_compat_ioctl NULL
-#endif /* CONFIG_COMPAT */
-
/*-------------------------------------------------------------------------*/
static int pi433_open(struct inode *inode, struct file *filp)
@@ -1101,7 +1091,7 @@ static const struct file_operations pi433_fops = {
.write = pi433_write,
.read = pi433_read,
.unlocked_ioctl = pi433_ioctl,
- .compat_ioctl = pi433_compat_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
.open = pi433_open,
.release = pi433_release,
.llseek = no_llseek,
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index fa783531ee88..d75052b36584 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -2568,18 +2568,6 @@ static long usbdev_ioctl(struct file *file, unsigned int cmd,
return ret;
}
-#ifdef CONFIG_COMPAT
-static long usbdev_compat_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- int ret;
-
- ret = usbdev_do_ioctl(file, cmd, compat_ptr(arg));
-
- return ret;
-}
-#endif
-
/* No kernel lock - fine */
static __poll_t usbdev_poll(struct file *file,
struct poll_table_struct *wait)
@@ -2603,9 +2591,7 @@ const struct file_operations usbdev_file_operations = {
.read = usbdev_read,
.poll = usbdev_poll,
.unlocked_ioctl = usbdev_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = usbdev_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.mmap = usbdev_mmap,
.open = usbdev_open,
.release = usbdev_release,
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index a3030cdf3c18..a5efe82584a5 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1200,15 +1200,6 @@ static long vfio_fops_unl_ioctl(struct file *filep,
return ret;
}
-#ifdef CONFIG_COMPAT
-static long vfio_fops_compat_ioctl(struct file *filep,
- unsigned int cmd, unsigned long arg)
-{
- arg = (unsigned long)compat_ptr(arg);
- return vfio_fops_unl_ioctl(filep, cmd, arg);
-}
-#endif /* CONFIG_COMPAT */
-
static int vfio_fops_open(struct inode *inode, struct file *filep)
{
struct vfio_container *container;
@@ -1291,9 +1282,7 @@ static const struct file_operations vfio_fops = {
.read = vfio_fops_read,
.write = vfio_fops_write,
.unlocked_ioctl = vfio_fops_unl_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = vfio_fops_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.mmap = vfio_fops_mmap,
};
@@ -1572,15 +1561,6 @@ static long vfio_group_fops_unl_ioctl(struct file *filep,
return ret;
}
-#ifdef CONFIG_COMPAT
-static long vfio_group_fops_compat_ioctl(struct file *filep,
- unsigned int cmd, unsigned long arg)
-{
- arg = (unsigned long)compat_ptr(arg);
- return vfio_group_fops_unl_ioctl(filep, cmd, arg);
-}
-#endif /* CONFIG_COMPAT */
-
static int vfio_group_fops_open(struct inode *inode, struct file *filep)
{
struct vfio_group *group;
@@ -1636,9 +1616,7 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
static const struct file_operations vfio_group_fops = {
.owner = THIS_MODULE,
.unlocked_ioctl = vfio_group_fops_unl_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = vfio_group_fops_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.open = vfio_group_fops_open,
.release = vfio_group_fops_release,
};
@@ -1703,24 +1681,13 @@ static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
return device->ops->mmap(device->device_data, vma);
}
-#ifdef CONFIG_COMPAT
-static long vfio_device_fops_compat_ioctl(struct file *filep,
- unsigned int cmd, unsigned long arg)
-{
- arg = (unsigned long)compat_ptr(arg);
- return vfio_device_fops_unl_ioctl(filep, cmd, arg);
-}
-#endif /* CONFIG_COMPAT */
-
static const struct file_operations vfio_device_fops = {
.owner = THIS_MODULE,
.release = vfio_device_fops_release,
.read = vfio_device_fops_read,
.write = vfio_device_fops_write,
.unlocked_ioctl = vfio_device_fops_unl_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = vfio_device_fops_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.mmap = vfio_device_fops_mmap,
};
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index df51a35cf537..1642b3573230 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -1765,14 +1765,6 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
}
}
-#ifdef CONFIG_COMPAT
-static long vhost_net_compat_ioctl(struct file *f, unsigned int ioctl,
- unsigned long arg)
-{
- return vhost_net_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
-}
-#endif
-
static ssize_t vhost_net_chr_read_iter(struct kiocb *iocb, struct iov_iter *to)
{
struct file *file = iocb->ki_filp;
@@ -1808,9 +1800,7 @@ static const struct file_operations vhost_net_fops = {
.write_iter = vhost_net_chr_write_iter,
.poll = vhost_net_chr_poll,
.unlocked_ioctl = vhost_net_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = vhost_net_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.open = vhost_net_open,
.llseek = noop_llseek,
};
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 618fb6461017..f9b14c39d89b 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1721,21 +1721,11 @@ vhost_scsi_ioctl(struct file *f,
}
}
-#ifdef CONFIG_COMPAT
-static long vhost_scsi_compat_ioctl(struct file *f, unsigned int ioctl,
- unsigned long arg)
-{
- return vhost_scsi_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
-}
-#endif
-
static const struct file_operations vhost_scsi_fops = {
.owner = THIS_MODULE,
.release = vhost_scsi_release,
.unlocked_ioctl = vhost_scsi_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = vhost_scsi_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.open = vhost_scsi_open,
.llseek = noop_llseek,
};
diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
index 40589850eb33..61d4d98c8f70 100644
--- a/drivers/vhost/test.c
+++ b/drivers/vhost/test.c
@@ -298,21 +298,11 @@ static long vhost_test_ioctl(struct file *f, unsigned int ioctl,
}
}
-#ifdef CONFIG_COMPAT
-static long vhost_test_compat_ioctl(struct file *f, unsigned int ioctl,
- unsigned long arg)
-{
- return vhost_test_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
-}
-#endif
-
static const struct file_operations vhost_test_fops = {
.owner = THIS_MODULE,
.release = vhost_test_release,
.unlocked_ioctl = vhost_test_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = vhost_test_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.open = vhost_test_open,
.llseek = noop_llseek,
};
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index bb5fc0e9fbc2..9a86202678b6 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -716,23 +716,13 @@ static long vhost_vsock_dev_ioctl(struct file *f, unsigned int ioctl,
}
}
-#ifdef CONFIG_COMPAT
-static long vhost_vsock_dev_compat_ioctl(struct file *f, unsigned int ioctl,
- unsigned long arg)
-{
- return vhost_vsock_dev_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
-}
-#endif
-
static const struct file_operations vhost_vsock_fops = {
.owner = THIS_MODULE,
.open = vhost_vsock_dev_open,
.release = vhost_vsock_dev_release,
.llseek = noop_llseek,
.unlocked_ioctl = vhost_vsock_dev_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = vhost_vsock_dev_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
};
static struct miscdevice vhost_vsock_misc = {
diff --git a/fs/fat/file.c b/fs/fat/file.c
index b3bed32946b1..f173d9261115 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -171,15 +171,6 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
}
}
-#ifdef CONFIG_COMPAT
-static long fat_generic_compat_ioctl(struct file *filp, unsigned int cmd,
- unsigned long arg)
-
-{
- return fat_generic_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
-}
-#endif
-
static int fat_file_release(struct inode *inode, struct file *filp)
{
if ((filp->f_mode & FMODE_WRITE) &&
@@ -209,9 +200,7 @@ const struct file_operations fat_file_operations = {
.mmap = generic_file_mmap,
.release = fat_file_release,
.unlocked_ioctl = fat_generic_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = fat_generic_compat_ioctl,
-#endif
+ .compat_ioctl = compat_ptr_ioctl,
.fsync = fat_file_fsync,
.splice_read = generic_file_splice_read,
.splice_write = iter_file_splice_write,
--
2.20.0
^ permalink raw reply related
* Re: [RFC PATCH 04/12] s390/cio: introduce cio DMA pool
From: Sebastian Ott @ 2019-04-16 12:50 UTC (permalink / raw)
To: Halil Pasic
Cc: Farhan Ali, linux-s390, Eric Farman, Claudio Imbrenda,
Vasily Gorbik, kvm, Cornelia Huck, virtualization,
Martin Schwidefsky, Viktor Mihajlovski, Janosch Frank
In-Reply-To: <20190412173017.04b768bb@oc2783563651>
On Fri, 12 Apr 2019, Halil Pasic wrote:
> On Fri, 12 Apr 2019 14:12:31 +0200 (CEST)
> Sebastian Ott <sebott@linux.ibm.com> wrote:
> > On Fri, 12 Apr 2019, Halil Pasic wrote:
> > > On Thu, 11 Apr 2019 20:25:01 +0200 (CEST)
> > > Sebastian Ott <sebott@linux.ibm.com> wrote:
> > > > I don't think we should use this global DMA pool. I guess it's OK for
> > > > stuff like airq (where we don't have a struct device at hand) but for
> > > > CCW we should use the device we have. Yes, this way we waste some memory
> > > > but all dma memory a device uses should fit in a page - so the wastage
> > > > is not too much.
>
> Regarding the wastage. Let us do the math together in search for an
> upper (wastage) limit.
[...]
> Currently we need at least 224 bytes per device that is ~ 6%
> of a PAGE_SIZE.
Yes, we basically waste the whole page. I'm ok with that if the benefit is
to play nice with the kernel APIs.
> > For practical
> > matters: DMA debugging will complain about misuse of a specific device or
> > driver.
> >
>
> Do you mean CONFIG_DMA_API_DEBUG and CONFIG_DMA_API_DEBUG_SG? I've been
> running with those and did not see any complaints. Maybe we should
> clarify this one offline...
I didn't mean to imply that there are bugs already - just that when used
as intended the DMA_DEBUG_API can complain about stuff like "your device
is gone but you have still DMA memory set up for it" which will not work
if you don't use the correct device...
Sebastian
^ permalink raw reply
* Re: [RFC PATCH 00/12] s390: virtio: support protected virtualization
From: David Hildenbrand @ 2019-04-16 11:50 UTC (permalink / raw)
To: Halil Pasic
Cc: Vasily Gorbik, linux-s390, Eric Farman, Claudio Imbrenda, kvm,
Sebastian Ott, Cornelia Huck, virtualization, Martin Schwidefsky,
Farhan Ali, Viktor Mihajlovski, Janosch Frank
In-Reply-To: <20190416131005.6f3e05eb@oc2783563651>
On 16.04.19 13:10, Halil Pasic wrote:
> On Fri, 12 Apr 2019 15:47:50 +0200
> David Hildenbrand <david@redhat.com> wrote:
>
>> On 05.04.19 01:16, Halil Pasic wrote:
>>> Enhanced virtualization protection technology may require the use of
>>> bounce buffers for I/O. While support for this was built into the virtio
>>> core, virtio-ccw wasn't changed accordingly.
>>
>> Can you elaborate some more about the general approach (Enhanced
>> virtualization protection technology, ultravisor, concept, issues, how
>> to squeeze it into QEMU/KVM/kernel) etc =
>>
>> For my taste, this cover letter misses some important context :)
>>
>
> I'm aware. Unfortunately we don't have a decision yet about which parts
> of the protected virtualization architecture are going to be PoP
> material.
Oh, okay.
>
> You can get some more context immediately by having a look at Martin's
> features branch and looking at the s390/protvirt and s390/uv patches.
As I don't have time to dig through random branches to
discover/reverse-engineer the obvious, I won't be reviewing this patch
series. But as I am not an I/O expert, this might not be bad at all :)
Maybe other people can help.
>
> I will try to provide more background information for v1. But having
> a remotely complete and reliable documentation will take some time. What
> I can offer at the moment is answers to specific questions.
Waiting for v1 then.
Cheers!
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 00/15] Share TTM code among framebuffer drivers
From: Koenig, Christian @ 2019-04-16 11:10 UTC (permalink / raw)
To: Daniel Vetter
Cc: airlied@linux.ie, puck.chen@hisilicon.com, Zhang, Jerry,
dri-devel@lists.freedesktop.org,
virtualization@lists.linux-foundation.org,
z.liuxinliang@hisilicon.com, hdegoede@redhat.com,
kong.kongxinwei@hisilicon.com, Huang, Ray, Thomas Zimmermann,
zourongrong@gmail.com
In-Reply-To: <CAKMK7uGKK2gP-1Oht7ARAqPs4ccUeAh9EPPke+wX1JWLeT_S7Q@mail.gmail.com>
Am 16.04.19 um 13:03 schrieb Daniel Vetter:
> On Tue, Apr 16, 2019 at 12:05 PM Koenig, Christian
> <Christian.Koenig@amd.com> wrote:
>> Am 15.04.19 um 21:17 schrieb Daniel Vetter:
>>> On Mon, Apr 15, 2019 at 6:21 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> Hi
>>>>
>>>> Am 15.04.19 um 17:54 schrieb Daniel Vetter:
>>>>> On Tue, Apr 09, 2019 at 09:50:40AM +0200, Thomas Zimmermann wrote:
>>>>>> Hi
>>>>>>
>>>>>> Am 09.04.19 um 09:12 schrieb kraxel@redhat.com:
>>>>>> [SNIP]
>>>>>>> I'd expect the same applies to the vbox driver.
>>>>>>>
>>>>>>> Dunno about the other drm drivers and the fbdev drivers you plan to
>>>>>>> migrate over.
>>>>>> The AST HW can support up to 512 MiB, but 32-64 MiB seems more realistic
>>>>>> for a server. It's similar with mgag200 HW. The old fbdev-supported
>>>>>> device are all somewhere in the range between cirrus and bochs. Some
>>>>>> drivers would probably benefit from the cirrus approach, some could use
>>>>>> VRAM directly.
>>>>> I think for dumb scanout with vram all we need is:
>>>>> - pin framebuffers, which potentially moves the underlying bo into vram
>>>>> - unpin framebuffers (which is just accounting, we don't want to move the
>>>>> bo on every flip!)
>>>>> - if a pin doesn't find enough space, move one of the unpinned bo still
>>>>> resident in vram out
>>>> For dumb buffers, I'd expect userspace to have a working set of only a
>>>> front and back buffer (plus maybe a third one). This working set has to
>>>> reside in VRAM for performance reasons; non-WS BOs from other userspace
>>>> programs don't have to be.
>>>>
>>>> So we could simplify even more: if there's not enough free space in
>>>> vram, remove all unpinned BO's. This would avoid the need to implement
>>>> an LRU algorithm or another eviction strategy. Userspace with a WS
>>>> larger than the absolute VRAM would see degraded performance but
>>>> otherwise still work.
>>> You still need a list of unpinned bo, and the lru scan algorithm is
>>> just a few lines of code more than unpinning everything. Plus it'd be
>>> a neat example of the drm_mm scan logic. Given that some folks might
>>> think that not having lru evict si a problem and get them to type
>>> their own, I'd just add it. But up to you. Plus with ttm you get it no
>>> matter what.
>> Well how about making an drm_lru component which just does the following
>> (and nothing else, please :):
>>
>> 1. Keep a list of objects and a spinlock protecting the list.
>>
>> 2. Offer helpers for adding/deleting/moving stuff from the list.
>>
>> 3. Offer a functionality to do the necessary dance of picking the first
>> entry where we can trylock it's reservation object.
>>
>> 4. Offer bulk move functionality similar to what TTM does at the moment
>> (can be implemented later on).
> At a basic level, this is list_head of drm_gem_object. Not sure that's
> all that useful (outside of the fairly simplistic vram helpers we're
> discussing here). Reasons for that is that there's a lot of trickery
> in selecting which is the best object to pick in any given case (e.g.
> do you want to use drm_mm scanning, or is there a slab of objects you
> prefer to throw out because that avoids. Given that I'm not sure
> implementing the entire scanning/drm_lru logic is beneficial.
>
> The magic trylock+kref_get_unless_zero otoh could be worth
> implementing as a helper, together with a note about how to build your
> own custom lru algorithm. Same for some bulk/nonblocking movement
> helpers maybe. Both not really needed for the dumb scanout vram
> helpers we're discussing here.
Yeah, exactly that's what I wanted to get towards as well.
This magic trylock+kref_get is what needs to be handled correctly by all
drivers which implement an LRU.
LRU bulk move is something which is tricky to get right as well, but so
far only amdgpu uses it so it only make sense to share when somebody
else wants the same approach.
Christian.
> -Daniel
>
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH 00/15] Share TTM code among framebuffer drivers
From: Daniel Vetter @ 2019-04-16 11:03 UTC (permalink / raw)
To: Koenig, Christian
Cc: airlied@linux.ie, puck.chen@hisilicon.com, Zhang, Jerry,
dri-devel@lists.freedesktop.org,
virtualization@lists.linux-foundation.org,
z.liuxinliang@hisilicon.com, hdegoede@redhat.com,
kong.kongxinwei@hisilicon.com, Huang, Ray, Thomas Zimmermann,
zourongrong@gmail.com
In-Reply-To: <9a3af097-d907-30da-73db-bcf00223e70b@amd.com>
On Tue, Apr 16, 2019 at 12:05 PM Koenig, Christian
<Christian.Koenig@amd.com> wrote:
>
> Am 15.04.19 um 21:17 schrieb Daniel Vetter:
> > On Mon, Apr 15, 2019 at 6:21 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Hi
> >>
> >> Am 15.04.19 um 17:54 schrieb Daniel Vetter:
> >>> On Tue, Apr 09, 2019 at 09:50:40AM +0200, Thomas Zimmermann wrote:
> >>>> Hi
> >>>>
> >>>> Am 09.04.19 um 09:12 schrieb kraxel@redhat.com:
> >>>> [SNIP]
> >>>>> I'd expect the same applies to the vbox driver.
> >>>>>
> >>>>> Dunno about the other drm drivers and the fbdev drivers you plan to
> >>>>> migrate over.
> >>>> The AST HW can support up to 512 MiB, but 32-64 MiB seems more realistic
> >>>> for a server. It's similar with mgag200 HW. The old fbdev-supported
> >>>> device are all somewhere in the range between cirrus and bochs. Some
> >>>> drivers would probably benefit from the cirrus approach, some could use
> >>>> VRAM directly.
> >>> I think for dumb scanout with vram all we need is:
> >>> - pin framebuffers, which potentially moves the underlying bo into vram
> >>> - unpin framebuffers (which is just accounting, we don't want to move the
> >>> bo on every flip!)
> >>> - if a pin doesn't find enough space, move one of the unpinned bo still
> >>> resident in vram out
> >> For dumb buffers, I'd expect userspace to have a working set of only a
> >> front and back buffer (plus maybe a third one). This working set has to
> >> reside in VRAM for performance reasons; non-WS BOs from other userspace
> >> programs don't have to be.
> >>
> >> So we could simplify even more: if there's not enough free space in
> >> vram, remove all unpinned BO's. This would avoid the need to implement
> >> an LRU algorithm or another eviction strategy. Userspace with a WS
> >> larger than the absolute VRAM would see degraded performance but
> >> otherwise still work.
> > You still need a list of unpinned bo, and the lru scan algorithm is
> > just a few lines of code more than unpinning everything. Plus it'd be
> > a neat example of the drm_mm scan logic. Given that some folks might
> > think that not having lru evict si a problem and get them to type
> > their own, I'd just add it. But up to you. Plus with ttm you get it no
> > matter what.
>
> Well how about making an drm_lru component which just does the following
> (and nothing else, please :):
>
> 1. Keep a list of objects and a spinlock protecting the list.
>
> 2. Offer helpers for adding/deleting/moving stuff from the list.
>
> 3. Offer a functionality to do the necessary dance of picking the first
> entry where we can trylock it's reservation object.
>
> 4. Offer bulk move functionality similar to what TTM does at the moment
> (can be implemented later on).
At a basic level, this is list_head of drm_gem_object. Not sure that's
all that useful (outside of the fairly simplistic vram helpers we're
discussing here). Reasons for that is that there's a lot of trickery
in selecting which is the best object to pick in any given case (e.g.
do you want to use drm_mm scanning, or is there a slab of objects you
prefer to throw out because that avoids. Given that I'm not sure
implementing the entire scanning/drm_lru logic is beneficial.
The magic trylock+kref_get_unless_zero otoh could be worth
implementing as a helper, together with a note about how to build your
own custom lru algorithm. Same for some bulk/nonblocking movement
helpers maybe. Both not really needed for the dumb scanout vram
helpers we're discussing here.
-Daniel
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* [PATCH] vhost/scsi: drop unnecessary smp_mb__after_atomic()
From: Stefan Hajnoczi @ 2019-04-16 10:47 UTC (permalink / raw)
To: kvm
Cc: Andrea Parri, Michael S. Tsirkin, linux-kernel, virtualization,
Stefan Hajnoczi
The vhost_scsi.ko code used several atomic variables at one point.
Later they were dropped or converted to regular ints protected by a
mutex.
The commit that made these changes left an unused smp_mb__after_atomic()
in vhost_scsi_set_endpoint(). It was previously used after incrementing
vhost_scsi->vhost_ref_cnt but this field has been dropped:
- atomic_inc(&vs->vhost_ref_cnt);
smp_mb__after_atomic_inc();
Reported-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Fixes: 101998f6fcd680 ("tcm_vhost: Post-merge review changes requested by MST")
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
drivers/vhost/scsi.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 618fb6461017..c090d177bd75 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1443,7 +1443,6 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
tpg->tv_tpg_vhost_count++;
tpg->vhost_scsi = vs;
vs_tpg[tpg->tport_tpgt] = tpg;
- smp_mb__after_atomic();
match = true;
}
mutex_unlock(&tpg->tv_tpg_mutex);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 00/15] Share TTM code among framebuffer drivers
From: Koenig, Christian @ 2019-04-16 10:05 UTC (permalink / raw)
To: Daniel Vetter, Thomas Zimmermann
Cc: airlied@linux.ie, puck.chen@hisilicon.com, Zhang, Jerry,
dri-devel@lists.freedesktop.org,
virtualization@lists.linux-foundation.org,
z.liuxinliang@hisilicon.com, hdegoede@redhat.com,
kong.kongxinwei@hisilicon.com, Huang, Ray, zourongrong@gmail.com
In-Reply-To: <CAKMK7uG+E1uTwSifSu5TrXwQXqu0Cpr30B02_S9bf95HEyqY_g@mail.gmail.com>
Am 15.04.19 um 21:17 schrieb Daniel Vetter:
> On Mon, Apr 15, 2019 at 6:21 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 15.04.19 um 17:54 schrieb Daniel Vetter:
>>> On Tue, Apr 09, 2019 at 09:50:40AM +0200, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 09.04.19 um 09:12 schrieb kraxel@redhat.com:
>>>> [SNIP]
>>>>> I'd expect the same applies to the vbox driver.
>>>>>
>>>>> Dunno about the other drm drivers and the fbdev drivers you plan to
>>>>> migrate over.
>>>> The AST HW can support up to 512 MiB, but 32-64 MiB seems more realistic
>>>> for a server. It's similar with mgag200 HW. The old fbdev-supported
>>>> device are all somewhere in the range between cirrus and bochs. Some
>>>> drivers would probably benefit from the cirrus approach, some could use
>>>> VRAM directly.
>>> I think for dumb scanout with vram all we need is:
>>> - pin framebuffers, which potentially moves the underlying bo into vram
>>> - unpin framebuffers (which is just accounting, we don't want to move the
>>> bo on every flip!)
>>> - if a pin doesn't find enough space, move one of the unpinned bo still
>>> resident in vram out
>> For dumb buffers, I'd expect userspace to have a working set of only a
>> front and back buffer (plus maybe a third one). This working set has to
>> reside in VRAM for performance reasons; non-WS BOs from other userspace
>> programs don't have to be.
>>
>> So we could simplify even more: if there's not enough free space in
>> vram, remove all unpinned BO's. This would avoid the need to implement
>> an LRU algorithm or another eviction strategy. Userspace with a WS
>> larger than the absolute VRAM would see degraded performance but
>> otherwise still work.
> You still need a list of unpinned bo, and the lru scan algorithm is
> just a few lines of code more than unpinning everything. Plus it'd be
> a neat example of the drm_mm scan logic. Given that some folks might
> think that not having lru evict si a problem and get them to type
> their own, I'd just add it. But up to you. Plus with ttm you get it no
> matter what.
Well how about making an drm_lru component which just does the following
(and nothing else, please :):
1. Keep a list of objects and a spinlock protecting the list.
2. Offer helpers for adding/deleting/moving stuff from the list.
3. Offer a functionality to do the necessary dance of picking the first
entry where we can trylock it's reservation object.
4. Offer bulk move functionality similar to what TTM does at the moment
(can be implemented later on).
Regards,
Christian.
^ permalink raw reply
* Re: [Qemu-devel] [RFC 3/3] RDMA/virtio-rdma: VirtIO rdma driver
From: Yuval Shaia @ 2019-04-16 8:56 UTC (permalink / raw)
To: Bart Van Assche; +Cc: mst, linux-rdma, cohuck, qemu-devel, virtualization, jgg
In-Reply-To: <f133d122-5b00-2e3b-f5cc-efc6d2cc3d3b@acm.org>
On Mon, Apr 15, 2019 at 06:07:52PM -0700, Bart Van Assche wrote:
> On 4/11/19 4:01 AM, Yuval Shaia wrote:
> > +++ b/drivers/infiniband/hw/virtio/Kconfig
> > @@ -0,0 +1,6 @@
> > +config INFINIBAND_VIRTIO_RDMA
> > + tristate "VirtIO Paravirtualized RDMA Driver"
> > + depends on NETDEVICES && ETHERNET && PCI && INET
> > + ---help---
> > + This driver provides low-level support for VirtIO Paravirtual
> > + RDMA adapter.
>
> Does this driver really depend on Ethernet, or does it also work with
> Ethernet support disabled?
The device should eventually expose Ethernet interface as well as IB.
>
> > +static inline struct virtio_rdma_info *to_vdev(struct ib_device *ibdev)
> > +{
> > + return container_of(ibdev, struct virtio_rdma_info, ib_dev);
> > +}
>
> Is it really worth to introduce this function? Have you considered to
> use container_of(ibdev, struct virtio_rdma_info, ib_dev) directly instead
> of to_vdev()?
Agree, not sure really needed, just saw that some drivers uses this pattern.
>
> > +static void rdma_ctrl_ack(struct virtqueue *vq)
> > +{
> > + struct virtio_rdma_info *dev = vq->vdev->priv;
> > +
> > + wake_up(&dev->acked);
> > +
> > + printk("%s\n", __func__);
> > +}
>
> Should that printk() be changed into pr_debug()? The same comment holds for
> all other printk() calls.
All prints will be removed, this is still wip.
>
> > +#define VIRTIO_RDMA_BOARD_ID 1
> > +#define VIRTIO_RDMA_HW_NAME "virtio-rdma"
> > +#define VIRTIO_RDMA_HW_REV 1
> > +#define VIRTIO_RDMA_DRIVER_VER "1.0"
>
> Is a driver version number useful in an upstream driver?
I've noticed that other drivers exposes this in sysfs.
>
> > +struct ib_cq *virtio_rdma_create_cq(struct ib_device *ibdev,
> > + const struct ib_cq_init_attr *attr,
> > + struct ib_ucontext *context,
> > + struct ib_udata *udata)
> > +{
> > + struct scatterlist in, out;
> > + struct virtio_rdma_ib_cq *vcq;
> > + struct cmd_create_cq *cmd;
> > + struct rsp_create_cq *rsp;
> > + struct ib_cq *cq = NULL;
> > + int rc;
> > +
> > + /* TODO: Check MAX_CQ */
> > +
> > + cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
> > + if (!cmd)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + rsp = kmalloc(sizeof(*rsp), GFP_ATOMIC);
> > + if (!rsp) {
> > + kfree(cmd);
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > + vcq = kzalloc(sizeof(*vcq), GFP_KERNEL);
> > + if (!vcq)
> > + goto out;
>
> Are you sure that you want to mix GFP_ATOMIC and GFP_KERNEL in a single
> function?
Right, a mistake.
>
> Thanks,
>
> Bart.
Thanks.
>
^ permalink raw reply
* [PATCH] vhost-scsi: remove incorrect memory barrier
From: Paolo Bonzini @ 2019-04-16 8:56 UTC (permalink / raw)
To: linux-kernel; +Cc: Andrea Parri, virtualization, stefanha, mst
At this point, vs_tpg is not public at all; tv_tpg_vhost_count
is accessed under tpg->tv_tpg_mutex; tpg->vhost_scsi is
accessed under vhost_scsi_mutex. Therefor there are no atomic
operations involved at all here, just remove the barrier.
Reported-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
drivers/vhost/scsi.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 618fb6461017..c090d177bd75 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1443,7 +1443,6 @@ static void vhost_scsi_flush(struct vhost_scsi *vs)
tpg->tv_tpg_vhost_count++;
tpg->vhost_scsi = vs;
vs_tpg[tpg->tport_tpgt] = tpg;
- smp_mb__after_atomic();
match = true;
}
mutex_unlock(&tpg->tv_tpg_mutex);
--
1.8.3.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox