* Re: [PATCH] virtio_mem: prevent overflow with subblock size
From: Michael S. Tsirkin @ 2020-06-08 7:08 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, Jason Wang, Pankaj Gupta, virtualization, teawater
In-Reply-To: <0930c9d0-0708-c079-29bd-b80d4e3ce446@redhat.com>
On Mon, Jun 08, 2020 at 08:58:31AM +0200, David Hildenbrand wrote:
> On 08.06.20 08:14, Michael S. Tsirkin wrote:
> > If subblock size is large (e.g. 1G) 32 bit math involving it
> > can overflow. Rather than try to catch all instances of that,
> > let's tweak block size to 64 bit.
>
> I fail to see where we could actually trigger an overflow. The reported
> warning looked like a false positive to me.
So
const uint64_t size = count * vm->subblock_size;
is it unreasonable for count to be 4K with subblock_size being 1M?
> >
> > It ripples through UAPI which is an ABI change, but it's not too late to
> > make it, and it will allow supporting >4Gbyte blocks while might
> > become necessary down the road.
> >
>
> This might break cloud-hypervisor, who's already implementing this
> protocol upstream (ccing Hui).
> https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs
>
> (blocks in the gigabyte range were never the original intention of
> virtio-mem, but I am not completely opposed to that)
So in that case, can you code up validation in the probe function?
> > Fixes: 5f1f79bbc9e26 ("virtio-mem: Paravirtualized memory hotplug")
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/virtio/virtio_mem.c | 14 +++++++-------
> > include/uapi/linux/virtio_mem.h | 4 ++--
> > 2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> > index 2f357142ea5e..7b1bece8a331 100644
> > --- a/drivers/virtio/virtio_mem.c
> > +++ b/drivers/virtio/virtio_mem.c
> > @@ -77,7 +77,7 @@ struct virtio_mem {
> > uint64_t requested_size;
> >
> > /* The device block size (for communicating with the device). */
> > - uint32_t device_block_size;
> > + uint64_t device_block_size;
> > /* The translated node id. NUMA_NO_NODE in case not specified. */
> > int nid;
> > /* Physical start address of the memory region. */
> > @@ -86,7 +86,7 @@ struct virtio_mem {
> > uint64_t region_size;
> >
> > /* The subblock size. */
> > - uint32_t subblock_size;
> > + uint64_t subblock_size;
> > /* The number of subblocks per memory block. */
> > uint32_t nb_sb_per_mb;
> >
> > @@ -1698,9 +1698,9 @@ static int virtio_mem_init(struct virtio_mem *vm)
> > * - At least the device block size.
> > * In the worst case, a single subblock per memory block.
> > */
> > - vm->subblock_size = PAGE_SIZE * 1u << max_t(uint32_t, MAX_ORDER - 1,
> > - pageblock_order);
> > - vm->subblock_size = max_t(uint32_t, vm->device_block_size,
> > + vm->subblock_size = PAGE_SIZE * 1ul << max_t(uint32_t, MAX_ORDER - 1,
> > + pageblock_order);
> > + vm->subblock_size = max_t(uint64_t, vm->device_block_size,
> > vm->subblock_size);
> > vm->nb_sb_per_mb = memory_block_size_bytes() / vm->subblock_size;
> >
> > @@ -1713,8 +1713,8 @@ static int virtio_mem_init(struct virtio_mem *vm)
> >
> > dev_info(&vm->vdev->dev, "start address: 0x%llx", vm->addr);
> > dev_info(&vm->vdev->dev, "region size: 0x%llx", vm->region_size);
> > - dev_info(&vm->vdev->dev, "device block size: 0x%x",
> > - vm->device_block_size);
> > + dev_info(&vm->vdev->dev, "device block size: 0x%llx",
> > + (unsigned long long)vm->device_block_size);
> > dev_info(&vm->vdev->dev, "memory block size: 0x%lx",
> > memory_block_size_bytes());
> > dev_info(&vm->vdev->dev, "subblock size: 0x%x",
> > diff --git a/include/uapi/linux/virtio_mem.h b/include/uapi/linux/virtio_mem.h
> > index a455c488a995..a9ffe041843c 100644
> > --- a/include/uapi/linux/virtio_mem.h
> > +++ b/include/uapi/linux/virtio_mem.h
> > @@ -185,10 +185,10 @@ struct virtio_mem_resp {
> >
> > struct virtio_mem_config {
> > /* Block size and alignment. Cannot change. */
> > - __u32 block_size;
> > + __u64 block_size;
> > /* Valid with VIRTIO_MEM_F_ACPI_PXM. Cannot change. */
> > __u16 node_id;
> > - __u16 padding;
> > + __u8 padding[6];
> > /* Start address of the memory region. Cannot change. */
> > __u64 addr;
> > /* Region size (maximum). Cannot change. */
> >
>
>
> --
> Thanks,
>
> David / dhildenb
^ permalink raw reply
* Re: [PATCH] virtio_mem: prevent overflow with subblock size
From: teawater @ 2020-06-08 7:12 UTC (permalink / raw)
To: David Hildenbrand
Cc: Michael S. Tsirkin, LKML, Jason Wang, Pankaj Gupta,
virtualization
In-Reply-To: <0930c9d0-0708-c079-29bd-b80d4e3ce446@redhat.com>
> 2020年6月8日 14:58，David Hildenbrand <david@redhat.com> 写道：
>
> On 08.06.20 08:14, Michael S. Tsirkin wrote:
>> If subblock size is large (e.g. 1G) 32 bit math involving it
>> can overflow. Rather than try to catch all instances of that,
>> let's tweak block size to 64 bit.
>
> I fail to see where we could actually trigger an overflow. The reported
> warning looked like a false positive to me.
>
>>
>> It ripples through UAPI which is an ABI change, but it's not too late to
>> make it, and it will allow supporting >4Gbyte blocks while might
>> become necessary down the road.
>>
>
> This might break cloud-hypervisor, who's already implementing this
> protocol upstream (ccing Hui).
> https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs
>
> (blocks in the gigabyte range were never the original intention of
> virtio-mem, but I am not completely opposed to that)
If you think virtio_mem need this patch, I think cloud-hypervisor should follow this update (I will post PR for it).
Best,
Hui
>
>> Fixes: 5f1f79bbc9e26 ("virtio-mem: Paravirtualized memory hotplug")
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> drivers/virtio/virtio_mem.c | 14 +++++++-------
>> include/uapi/linux/virtio_mem.h | 4 ++--
>> 2 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>> index 2f357142ea5e..7b1bece8a331 100644
>> --- a/drivers/virtio/virtio_mem.c
>> +++ b/drivers/virtio/virtio_mem.c
>> @@ -77,7 +77,7 @@ struct virtio_mem {
>> uint64_t requested_size;
>>
>> /* The device block size (for communicating with the device). */
>> - uint32_t device_block_size;
>> + uint64_t device_block_size;
>> /* The translated node id. NUMA_NO_NODE in case not specified. */
>> int nid;
>> /* Physical start address of the memory region. */
>> @@ -86,7 +86,7 @@ struct virtio_mem {
>> uint64_t region_size;
>>
>> /* The subblock size. */
>> - uint32_t subblock_size;
>> + uint64_t subblock_size;
>> /* The number of subblocks per memory block. */
>> uint32_t nb_sb_per_mb;
>>
>> @@ -1698,9 +1698,9 @@ static int virtio_mem_init(struct virtio_mem *vm)
>> * - At least the device block size.
>> * In the worst case, a single subblock per memory block.
>> */
>> - vm->subblock_size = PAGE_SIZE * 1u << max_t(uint32_t, MAX_ORDER - 1,
>> - pageblock_order);
>> - vm->subblock_size = max_t(uint32_t, vm->device_block_size,
>> + vm->subblock_size = PAGE_SIZE * 1ul << max_t(uint32_t, MAX_ORDER - 1,
>> + pageblock_order);
>> + vm->subblock_size = max_t(uint64_t, vm->device_block_size,
>> vm->subblock_size);
>> vm->nb_sb_per_mb = memory_block_size_bytes() / vm->subblock_size;
>>
>> @@ -1713,8 +1713,8 @@ static int virtio_mem_init(struct virtio_mem *vm)
>>
>> dev_info(&vm->vdev->dev, "start address: 0x%llx", vm->addr);
>> dev_info(&vm->vdev->dev, "region size: 0x%llx", vm->region_size);
>> - dev_info(&vm->vdev->dev, "device block size: 0x%x",
>> - vm->device_block_size);
>> + dev_info(&vm->vdev->dev, "device block size: 0x%llx",
>> + (unsigned long long)vm->device_block_size);
>> dev_info(&vm->vdev->dev, "memory block size: 0x%lx",
>> memory_block_size_bytes());
>> dev_info(&vm->vdev->dev, "subblock size: 0x%x",
>> diff --git a/include/uapi/linux/virtio_mem.h b/include/uapi/linux/virtio_mem.h
>> index a455c488a995..a9ffe041843c 100644
>> --- a/include/uapi/linux/virtio_mem.h
>> +++ b/include/uapi/linux/virtio_mem.h
>> @@ -185,10 +185,10 @@ struct virtio_mem_resp {
>>
>> struct virtio_mem_config {
>> /* Block size and alignment. Cannot change. */
>> - __u32 block_size;
>> + __u64 block_size;
>> /* Valid with VIRTIO_MEM_F_ACPI_PXM. Cannot change. */
>> __u16 node_id;
>> - __u16 padding;
>> + __u8 padding[6];
>> /* Start address of the memory region. Cannot change. */
>> __u64 addr;
>> /* Region size (maximum). Cannot change. */
>>
>
>
> --
> Thanks,
>
> David / dhildenb
^ permalink raw reply
* Re: [PATCH] virtio_mem: prevent overflow with subblock size
From: David Hildenbrand @ 2020-06-08 7:17 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Pankaj Gupta, teawater, linux-kernel, virtualization
In-Reply-To: <20200608030423-mutt-send-email-mst@kernel.org>
On 08.06.20 09:08, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2020 at 08:58:31AM +0200, David Hildenbrand wrote:
>> On 08.06.20 08:14, Michael S. Tsirkin wrote:
>>> If subblock size is large (e.g. 1G) 32 bit math involving it
>>> can overflow. Rather than try to catch all instances of that,
>>> let's tweak block size to 64 bit.
>>
>> I fail to see where we could actually trigger an overflow. The reported
>> warning looked like a false positive to me.
>
>
> So
>
> const uint64_t size = count * vm->subblock_size;
>
> is it unreasonable for count to be 4K with subblock_size being 1M?
virtio_mem_mb_plug_sb() and friends are only called on subblocks
residing within a single Linux memory block. (currently, 128MB .. 2G on
x86-64). A subblock on x86-64 is currently at least 4MB.
So "count * vm->subblock_size" can currently not exceed the Linux memory
block size (in practice, it is max 128MB).
>
>>>
>>> It ripples through UAPI which is an ABI change, but it's not too late to
>>> make it, and it will allow supporting >4Gbyte blocks while might
>>> become necessary down the road.
>>>
>>
>> This might break cloud-hypervisor, who's already implementing this
>> protocol upstream (ccing Hui).
>> https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs
>>
>> (blocks in the gigabyte range were never the original intention of
>> virtio-mem, but I am not completely opposed to that)
>
>
> So in that case, can you code up validation in the probe function?
If we would currently have a "block_size" > Linux memory block size, we
bail out.
virtio_mem_init():
if (vm->device_block_size > memory_block_size_bytes()) {
dev_err(&vm->vdev->dev,
"The block size is not supported (too big).\n");
return -EINVAL;
}
So what's reported can currently not happen. Having that said, changing
"subblock_size" to be an uint64_t is a good cleanup, especially for the
future.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH] virtio_mem: prevent overflow with subblock size
From: David Hildenbrand @ 2020-06-08 7:34 UTC (permalink / raw)
To: teawater; +Cc: Michael S. Tsirkin, LKML, Jason Wang, Pankaj Gupta,
virtualization
In-Reply-To: <2D9A07BA-6FDC-48FF-9A1F-62272695B3EF@linux.alibaba.com>
On 08.06.20 09:12, teawater wrote:
>
>
>> 2020年6月8日 14:58，David Hildenbrand <david@redhat.com> 写道：
>>
>> On 08.06.20 08:14, Michael S. Tsirkin wrote:
>>> If subblock size is large (e.g. 1G) 32 bit math involving it
>>> can overflow. Rather than try to catch all instances of that,
>>> let's tweak block size to 64 bit.
>>
>> I fail to see where we could actually trigger an overflow. The reported
>> warning looked like a false positive to me.
>>
>>>
>>> It ripples through UAPI which is an ABI change, but it's not too late to
>>> make it, and it will allow supporting >4Gbyte blocks while might
>>> become necessary down the road.
>>>
>>
>> This might break cloud-hypervisor, who's already implementing this
>> protocol upstream (ccing Hui).
>> https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs
>>
>> (blocks in the gigabyte range were never the original intention of
>> virtio-mem, but I am not completely opposed to that)
>
> If you think virtio_mem need this patch, I think cloud-hypervisor should follow this update (I will post PR for it).
Thanks Hui. So we can still do last-minute changes if we all agree it
makes sense.
@MST can you rephrase the patch description to highlight that this is a
preparation for the future only and not actually currently broken?
"virtio-mem: convert device block size into 64bit" ...
With that
Acked-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v3 0/5] Add a vhost RPMsg API
From: Guennadi Liakhovetski @ 2020-06-08 7:37 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, linux-remoteproc, virtualization, sound-open-firmware,
Pierre-Louis Bossart, Liam Girdwood, Jason Wang, Ohad Ben-Cohen,
Bjorn Andersson, Mathieu Poirier
In-Reply-To: <20200605063435.GA32302@ubuntu>
Hi Michael,
On Fri, Jun 05, 2020 at 08:34:35AM +0200, Guennadi Liakhovetski wrote:
>
> On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote:
[snip]
> > Another it's out of line with 1.0 spec passing guest
> > endian data around. Won't work if host and guest
> > endian-ness do not match. Should pass eveything in LE and
> > convert.
>
> Yes, I have to fix this, thanks.
Just to make sure my understanding is correct: this would involve also
modifying the current virtio_rpmsg_bus.c implementation to add
endianness conversions. That's what you meant, right?
Thanks
Guennadi
^ permalink raw reply
* Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects
From: David Stevens @ 2020-06-08 8:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Gerd Hoffmann, David Airlie, Daniel Vetter, Sumit Semwal,
Jason Wang, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
open list, ML dri-devel, open list:VIRTIO GPU DRIVER,
Linux Media Mailing List,
moderated list:DMA BUFFER SHARING FRAMEWORK, virtio-dev
In-Reply-To: <20200608015728-mutt-send-email-mst@kernel.org>
On Mon, Jun 8, 2020 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> > On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > > exported object.
> > > > > >
> > > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > > >
> > > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > > driver. We can always move it later ...
> > > >
> > > > As stated in the cover letter, this will be used by virtio-video.
> > > >
> > > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > > >
> > > > > > ---
> > > > > > drivers/virtio/Makefile | 2 +-
> > > > > > drivers/virtio/virtio.c | 6 +++
> > > > > > drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > > > include/linux/virtio.h | 1 +
> > > > > > include/linux/virtio_dma_buf.h | 58 +++++++++++++++++++++
> > > > > > 5 files changed, 155 insertions(+), 1 deletion(-)
> > > > > > create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > > > create mode 100644 include/linux/virtio_dma_buf.h
> > > > > >
> > > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > > --- a/drivers/virtio/Makefile
> > > > > > +++ b/drivers/virtio/Makefile
> > > > > > @@ -1,5 +1,5 @@
> > > > > > # SPDX-License-Identifier: GPL-2.0
> > > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > > > obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > > > obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > > > virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > > --- a/drivers/virtio/virtio.c
> > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > > }
> > > > > > EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > > >
> > > > > > +bool is_virtio_device(struct device *dev)
> > > > > > +{
> > > > > > + return dev->bus == &virtio_bus;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > > +
> > > > > > void unregister_virtio_device(struct virtio_device *dev)
> > > > > > {
> > > > > > int index = dev->index; /* save for after device release */
> > > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..23e3399b11ed
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > > @@ -0,0 +1,89 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > +/*
> > > > > > + * dma-bufs for virtio exported objects
> > > > > > + *
> > > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/virtio_dma_buf.h>
> > > > > > +
> > > > > > +/**
> > > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > > + *
> > > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > > + * for the object's UUID.
> > > > > > + */
> > > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > > + const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > > +{
> > > > > > + struct dma_buf_export_info exp_info;
> > > > > > +
> > > > > > + if (!virtio_exp_info->ops
> > > > > > + || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > > + || !virtio_exp_info->ops->get_uuid) {
> > > > > > + return ERR_PTR(-EINVAL);
> > > > > > + }
> > > > > > +
> > > > > > + exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > > + exp_info.owner = virtio_exp_info->owner;
> > > > > > + exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > > + exp_info.size = virtio_exp_info->size;
> > > > > > + exp_info.flags = virtio_exp_info->flags;
> > > > > > + exp_info.resv = virtio_exp_info->resv;
> > > > > > + exp_info.priv = virtio_exp_info->priv;
> > > > > > + BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > > + != sizeof(struct dma_buf_export_info));
> > > > >
> > > > > This is the only part that gives me pause. Why do we need this hack?
> > > > > What's wrong with just using dma_buf_export_info directly,
> > > > > and if you want the virtio ops, just using container_off?
> > > >
> > > > This approach provides a more explicit type signature and a little
> > > > more type safety, I think. If others don't think it's a worthwhile
> > > > tradeoff, I can remove it.
> > > >
> > > > -David
> > >
> > > The cost is that if dma_buf_export_info changes even slightly, we get
> > > weird crashes.
> >
> > I'm not sure I understand what types of changes you're referring to.
> > As this is written, virtio-dma-buf is just another client of the
> > dma-buf API. If this were rewritten to use dma-buf directly, then
> > whatever code calls virtio_dma_buf_export would become a client of the
> > dma-buf API. If the semantics of existing fields in the dma-buf API
> > were changed and virtio-dma-buf wasn't updated, then yes, you could
> > get weird crashes from virtio-dma-buf.
> > However, the same problem would
> > exist if virtio_dma_buf_export used dma-buf directly - changes to
> > dma-buf's semantics could cause weird crashes if the caller of
> > virtio_dma_buf_export wasn't updated properly. The only potential
> > source of problems I see is if virtio_dma_buf_export_info wasn't
> > updated properly, but virtio_dma_buf_export_info is dead simple, so I
> > don't know if that's really a problem.
> >
> > -David
>
> I think you can get weird crashes if fields in dma buf are reordered, or
> if a field size changes. You have a build bug catching overall struct
> size changes but that can remain the same due do compiler padding or
> such.
Since it's manually copying the fields instead of trying something
clever like memcpy, I don't see how reordering the fields or changing
the size of the fields would cause problems. Right now,
virtio_dma_buf_export is just a regular client of dma_buf_export, no
different than any of the other call sites in the kernel.
Overall, I don't really think that this is a problem. If someone makes
breaking changes to the semantics of dma-buf, then they will need to
update this call site, just like they will need to update all of the
other call sites in the kernel. If someone adds new functionality to
dma-buf and adds another field to dma_buf_export_info, the build bug
is a reminder to add it to virtio_dma_buf_export_info. However, if the
struct padding happens to work out such that the build bug doesn't
trigger, that doesn't really matter - it just means that the new
dma-buf feature won't be exposed by virito-dma-buf until someone needs
it and notices that the new field is missing.
-David
^ permalink raw reply
* Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects
From: Michael S. Tsirkin @ 2020-06-08 9:05 UTC (permalink / raw)
To: David Stevens
Cc: Gerd Hoffmann, David Airlie, Daniel Vetter, Sumit Semwal,
Jason Wang, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
open list, ML dri-devel, open list:VIRTIO GPU DRIVER,
Linux Media Mailing List,
moderated list:DMA BUFFER SHARING FRAMEWORK, virtio-dev
In-Reply-To: <CAD=HUj68NfNK+0go7Z-XeZ2ckWJpYsym3G+-DfJyoUm+dJDznQ@mail.gmail.com>
On Mon, Jun 08, 2020 at 05:32:26PM +0900, David Stevens wrote:
> On Mon, Jun 8, 2020 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> > > On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > > > exported object.
> > > > > > >
> > > > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > > > >
> > > > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > > > driver. We can always move it later ...
> > > > >
> > > > > As stated in the cover letter, this will be used by virtio-video.
> > > > >
> > > > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > > > >
> > > > > > > ---
> > > > > > > drivers/virtio/Makefile | 2 +-
> > > > > > > drivers/virtio/virtio.c | 6 +++
> > > > > > > drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > > > > include/linux/virtio.h | 1 +
> > > > > > > include/linux/virtio_dma_buf.h | 58 +++++++++++++++++++++
> > > > > > > 5 files changed, 155 insertions(+), 1 deletion(-)
> > > > > > > create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > > > > create mode 100644 include/linux/virtio_dma_buf.h
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > > > --- a/drivers/virtio/Makefile
> > > > > > > +++ b/drivers/virtio/Makefile
> > > > > > > @@ -1,5 +1,5 @@
> > > > > > > # SPDX-License-Identifier: GPL-2.0
> > > > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > > > > obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > > > > obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > > > > virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > > > --- a/drivers/virtio/virtio.c
> > > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > > > }
> > > > > > > EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > > > >
> > > > > > > +bool is_virtio_device(struct device *dev)
> > > > > > > +{
> > > > > > > + return dev->bus == &virtio_bus;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > > > +
> > > > > > > void unregister_virtio_device(struct virtio_device *dev)
> > > > > > > {
> > > > > > > int index = dev->index; /* save for after device release */
> > > > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..23e3399b11ed
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > > > @@ -0,0 +1,89 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > +/*
> > > > > > > + * dma-bufs for virtio exported objects
> > > > > > > + *
> > > > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/virtio_dma_buf.h>
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > > > + *
> > > > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > > > + * for the object's UUID.
> > > > > > > + */
> > > > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > > > + const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > > > +{
> > > > > > > + struct dma_buf_export_info exp_info;
> > > > > > > +
> > > > > > > + if (!virtio_exp_info->ops
> > > > > > > + || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > > > + || !virtio_exp_info->ops->get_uuid) {
> > > > > > > + return ERR_PTR(-EINVAL);
> > > > > > > + }
> > > > > > > +
> > > > > > > + exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > > > + exp_info.owner = virtio_exp_info->owner;
> > > > > > > + exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > > > + exp_info.size = virtio_exp_info->size;
> > > > > > > + exp_info.flags = virtio_exp_info->flags;
> > > > > > > + exp_info.resv = virtio_exp_info->resv;
> > > > > > > + exp_info.priv = virtio_exp_info->priv;
> > > > > > > + BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > > > + != sizeof(struct dma_buf_export_info));
> > > > > >
> > > > > > This is the only part that gives me pause. Why do we need this hack?
> > > > > > What's wrong with just using dma_buf_export_info directly,
> > > > > > and if you want the virtio ops, just using container_off?
> > > > >
> > > > > This approach provides a more explicit type signature and a little
> > > > > more type safety, I think. If others don't think it's a worthwhile
> > > > > tradeoff, I can remove it.
> > > > >
> > > > > -David
> > > >
> > > > The cost is that if dma_buf_export_info changes even slightly, we get
> > > > weird crashes.
> > >
> > > I'm not sure I understand what types of changes you're referring to.
> > > As this is written, virtio-dma-buf is just another client of the
> > > dma-buf API. If this were rewritten to use dma-buf directly, then
> > > whatever code calls virtio_dma_buf_export would become a client of the
> > > dma-buf API. If the semantics of existing fields in the dma-buf API
> > > were changed and virtio-dma-buf wasn't updated, then yes, you could
> > > get weird crashes from virtio-dma-buf.
> > > However, the same problem would
> > > exist if virtio_dma_buf_export used dma-buf directly - changes to
> > > dma-buf's semantics could cause weird crashes if the caller of
> > > virtio_dma_buf_export wasn't updated properly. The only potential
> > > source of problems I see is if virtio_dma_buf_export_info wasn't
> > > updated properly, but virtio_dma_buf_export_info is dead simple, so I
> > > don't know if that's really a problem.
> > >
> > > -David
> >
> > I think you can get weird crashes if fields in dma buf are reordered, or
> > if a field size changes. You have a build bug catching overall struct
> > size changes but that can remain the same due do compiler padding or
> > such.
>
> Since it's manually copying the fields instead of trying something
> clever like memcpy, I don't see how reordering the fields or changing
> the size of the fields would cause problems. Right now,
> virtio_dma_buf_export is just a regular client of dma_buf_export, no
> different than any of the other call sites in the kernel.
>
> Overall, I don't really think that this is a problem. If someone makes
> breaking changes to the semantics of dma-buf, then they will need to
> update this call site, just like they will need to update all of the
> other call sites in the kernel. If someone adds new functionality to
> dma-buf and adds another field to dma_buf_export_info, the build bug
> is a reminder to add it to virtio_dma_buf_export_info. However, if the
> struct padding happens to work out such that the build bug doesn't
> trigger, that doesn't really matter - it just means that the new
> dma-buf feature won't be exposed by virito-dma-buf until someone needs
> it and notices that the new field is missing.
>
> -David
Think about the reasons for the BUILD_BUG_ON being there, checking
struct sizes like this is a clear sign of something strange going on.
But really this is just unnecessary complexity anyway.
The only difference with dma_buf is get_uuid and device_attacj, isn't it?
And they are called like this:
+ */
+int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
+ uuid_t *uuid)
+{
+ const struct virtio_dma_buf_ops *ops = container_of(
+ dma_buf->ops, const struct virtio_dma_buf_ops, ops);
+
+ if (!is_virtio_dma_buf(dma_buf))
+ return -EINVAL;
+
+ return ops->get_uuid(dma_buf, uuid);
+}
So you are doing the container_of trick anyway, the extra structure
did not give us any type safety.
--
MST
^ permalink raw reply
* Re: [PATCH v3 0/5] Add a vhost RPMsg API
From: Michael S. Tsirkin @ 2020-06-08 9:09 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: kvm, linux-remoteproc, virtualization, sound-open-firmware,
Pierre-Louis Bossart, Liam Girdwood, Jason Wang, Ohad Ben-Cohen,
Bjorn Andersson, Mathieu Poirier
In-Reply-To: <20200608073715.GA10562@ubuntu>
On Mon, Jun 08, 2020 at 09:37:15AM +0200, Guennadi Liakhovetski wrote:
> Hi Michael,
>
> On Fri, Jun 05, 2020 at 08:34:35AM +0200, Guennadi Liakhovetski wrote:
> >
> > On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote:
>
> [snip]
>
> > > Another it's out of line with 1.0 spec passing guest
> > > endian data around. Won't work if host and guest
> > > endian-ness do not match. Should pass eveything in LE and
> > > convert.
> >
> > Yes, I have to fix this, thanks.
>
> Just to make sure my understanding is correct: this would involve also
> modifying the current virtio_rpmsg_bus.c implementation to add
> endianness conversions. That's what you meant, right?
>
> Thanks
> Guennadi
right and if there are legacy compat considerations, using _virtio16 and
friends types, as well as virtio16_to_cpu and friends functions.
--
MST
^ permalink raw reply
* Re: [PATCH v3 0/5] Add a vhost RPMsg API
From: Guennadi Liakhovetski @ 2020-06-08 9:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, linux-remoteproc, virtualization, sound-open-firmware,
Pierre-Louis Bossart, Liam Girdwood, Jason Wang, Ohad Ben-Cohen,
Bjorn Andersson, Mathieu Poirier
In-Reply-To: <20200608073715.GA10562@ubuntu>
Update: I looked through VirtIO 1.0 and 1.1 specs, data format their,
including byte order, is defined on a per-device type basis. RPMsg is
indeed included in the spec as device type 7, but that's the only
mention of it in both versions. It seems RPMsg over VirtIO isn't
standardised yet. Also it looks like newer interface definitions
specify using "guest native endianness" for Virtual Queue data. So
I think the same should be done for RPMsg instead of enforcing LE?
Thanks
Guennadi
On Mon, Jun 08, 2020 at 09:37:15AM +0200, Guennadi Liakhovetski wrote:
> Hi Michael,
>
> On Fri, Jun 05, 2020 at 08:34:35AM +0200, Guennadi Liakhovetski wrote:
> >
> > On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote:
>
> [snip]
>
> > > Another it's out of line with 1.0 spec passing guest
> > > endian data around. Won't work if host and guest
> > > endian-ness do not match. Should pass eveything in LE and
> > > convert.
> >
> > Yes, I have to fix this, thanks.
>
> Just to make sure my understanding is correct: this would involve also
> modifying the current virtio_rpmsg_bus.c implementation to add
> endianness conversions. That's what you meant, right?
^ permalink raw reply
* Re: [PATCH 5/6] vdpa: introduce virtio pci driver
From: Jason Wang @ 2020-06-08 9:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, virtualization, netdev, linux-kernel, rob.miller,
lingshan.zhu, eperezma, lulu, shahafs, hanand, mhabets, gdawar,
saugatm, vmireyno, zhangweining, eli
In-Reply-To: <20200608021438-mutt-send-email-mst@kernel.org>
On 2020/6/8 下午2:32, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2020 at 11:32:31AM +0800, Jason Wang wrote:
>> On 2020/6/7 下午9:51, Michael S. Tsirkin wrote:
>>> On Fri, Jun 05, 2020 at 04:54:17PM +0800, Jason Wang wrote:
>>>> On 2020/6/2 下午3:08, Jason Wang wrote:
>>>>>>> +static const struct pci_device_id vp_vdpa_id_table[] = {
>>>>>>> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID) },
>>>>>>> + { 0 }
>>>>>>> +};
>>>>>> This looks like it'll create a mess with either virtio pci
>>>>>> or vdpa being loaded at random. Maybe just don't specify
>>>>>> any IDs for now. Down the road we could get a
>>>>>> distinct vendor ID or a range of device IDs for this.
>>>>> Right, will do.
>>>>>
>>>>> Thanks
>>>> Rethink about this. If we don't specify any ID, the binding won't work.
>>> We can bind manually. It's not really for production anyway, so
>>> not a big deal imho.
>>
>> I think you mean doing it via "new_id", right.
> I really meant driver_override. This is what people have been using
> with pci-stub for years now.
Do you want me to implement "driver_overrid" in this series, or a NULL
id_table is sufficient?
>
>>>> How about using a dedicated subsystem vendor id for this?
>>>>
>>>> Thanks
>>> If virtio vendor id is used then standard driver is expected
>>> to bind, right? Maybe use a dedicated vendor id?
>>
>> I meant something like:
>>
>> static const struct pci_device_id vp_vdpa_id_table[] = {
>> { PCI_DEVICE_SUB(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID,
>> VP_TEST_VENDOR_ID, VP_TEST_DEVICE_ID) },
>> { 0 }
>> };
>>
>> Thanks
>>
> Then regular virtio will still bind to it. It has
>
> drivers/virtio/virtio_pci_common.c: { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID) },
>
>
IFCVF use this to avoid the binding to regular virtio device. Looking at
pci_match_one_device() it checks both subvendor and subdevice there.
Thanks
^ permalink raw reply
* Re: [PATCH v3 0/5] Add a vhost RPMsg API
From: Michael S. Tsirkin @ 2020-06-08 9:19 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: kvm, linux-remoteproc, virtualization, sound-open-firmware,
Pierre-Louis Bossart, Liam Girdwood, Jason Wang, Ohad Ben-Cohen,
Bjorn Andersson, Mathieu Poirier
In-Reply-To: <20200608091100.GC10562@ubuntu>
On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote:
> Update: I looked through VirtIO 1.0 and 1.1 specs, data format their,
> including byte order, is defined on a per-device type basis. RPMsg is
> indeed included in the spec as device type 7, but that's the only
> mention of it in both versions. It seems RPMsg over VirtIO isn't
> standardised yet.
Yes. And it would be very good to have some standartization before we
keep adding things. For example without any spec if host code breaks
with some guests, how do we know which side should be fixed?
> Also it looks like newer interface definitions
> specify using "guest native endianness" for Virtual Queue data.
They really don't or shouldn't. That's limited to legacy chapters.
Some definitions could have slipped through but it's not
the norm. I just quickly looked through the 1.1 spec and could
not find any instances that specify "guest native endianness"
but feel free to point them out to me.
> So
> I think the same should be done for RPMsg instead of enforcing LE?
>
> Thanks
> Guennadi
That makes hardware implementations as well as any cross-endian
hypervisors tricky.
--
MST
^ permalink raw reply
* Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects
From: David Stevens @ 2020-06-08 9:30 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Gerd Hoffmann, David Airlie, Daniel Vetter, Sumit Semwal,
Jason Wang, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
open list, ML dri-devel, open list:VIRTIO GPU DRIVER,
Linux Media Mailing List,
moderated list:DMA BUFFER SHARING FRAMEWORK, virtio-dev
In-Reply-To: <20200608045721-mutt-send-email-mst@kernel.org>
On Mon, Jun 8, 2020 at 6:05 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 08, 2020 at 05:32:26PM +0900, David Stevens wrote:
> > On Mon, Jun 8, 2020 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> > > > On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > > > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > >
> > > > > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > > > > exported object.
> > > > > > > >
> > > > > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > > > > >
> > > > > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > > > > driver. We can always move it later ...
> > > > > >
> > > > > > As stated in the cover letter, this will be used by virtio-video.
> > > > > >
> > > > > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > > > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > > > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > > > > >
> > > > > > > > ---
> > > > > > > > drivers/virtio/Makefile | 2 +-
> > > > > > > > drivers/virtio/virtio.c | 6 +++
> > > > > > > > drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > > > > > include/linux/virtio.h | 1 +
> > > > > > > > include/linux/virtio_dma_buf.h | 58 +++++++++++++++++++++
> > > > > > > > 5 files changed, 155 insertions(+), 1 deletion(-)
> > > > > > > > create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > > > > > create mode 100644 include/linux/virtio_dma_buf.h
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > > > > --- a/drivers/virtio/Makefile
> > > > > > > > +++ b/drivers/virtio/Makefile
> > > > > > > > @@ -1,5 +1,5 @@
> > > > > > > > # SPDX-License-Identifier: GPL-2.0
> > > > > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > > > > > obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > > > > > obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > > > > > virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > > > > --- a/drivers/virtio/virtio.c
> > > > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > > > > }
> > > > > > > > EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > > > > >
> > > > > > > > +bool is_virtio_device(struct device *dev)
> > > > > > > > +{
> > > > > > > > + return dev->bus == &virtio_bus;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > > > > +
> > > > > > > > void unregister_virtio_device(struct virtio_device *dev)
> > > > > > > > {
> > > > > > > > int index = dev->index; /* save for after device release */
> > > > > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..23e3399b11ed
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > > > > @@ -0,0 +1,89 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > > +/*
> > > > > > > > + * dma-bufs for virtio exported objects
> > > > > > > > + *
> > > > > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <linux/virtio_dma_buf.h>
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > > > > + *
> > > > > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > > > > + * for the object's UUID.
> > > > > > > > + */
> > > > > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > > > > + const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > > > > +{
> > > > > > > > + struct dma_buf_export_info exp_info;
> > > > > > > > +
> > > > > > > > + if (!virtio_exp_info->ops
> > > > > > > > + || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > > > > + || !virtio_exp_info->ops->get_uuid) {
> > > > > > > > + return ERR_PTR(-EINVAL);
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > > > > + exp_info.owner = virtio_exp_info->owner;
> > > > > > > > + exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > > > > + exp_info.size = virtio_exp_info->size;
> > > > > > > > + exp_info.flags = virtio_exp_info->flags;
> > > > > > > > + exp_info.resv = virtio_exp_info->resv;
> > > > > > > > + exp_info.priv = virtio_exp_info->priv;
> > > > > > > > + BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > > > > + != sizeof(struct dma_buf_export_info));
> > > > > > >
> > > > > > > This is the only part that gives me pause. Why do we need this hack?
> > > > > > > What's wrong with just using dma_buf_export_info directly,
> > > > > > > and if you want the virtio ops, just using container_off?
> > > > > >
> > > > > > This approach provides a more explicit type signature and a little
> > > > > > more type safety, I think. If others don't think it's a worthwhile
> > > > > > tradeoff, I can remove it.
> > > > > >
> > > > > > -David
> > > > >
> > > > > The cost is that if dma_buf_export_info changes even slightly, we get
> > > > > weird crashes.
> > > >
> > > > I'm not sure I understand what types of changes you're referring to.
> > > > As this is written, virtio-dma-buf is just another client of the
> > > > dma-buf API. If this were rewritten to use dma-buf directly, then
> > > > whatever code calls virtio_dma_buf_export would become a client of the
> > > > dma-buf API. If the semantics of existing fields in the dma-buf API
> > > > were changed and virtio-dma-buf wasn't updated, then yes, you could
> > > > get weird crashes from virtio-dma-buf.
> > > > However, the same problem would
> > > > exist if virtio_dma_buf_export used dma-buf directly - changes to
> > > > dma-buf's semantics could cause weird crashes if the caller of
> > > > virtio_dma_buf_export wasn't updated properly. The only potential
> > > > source of problems I see is if virtio_dma_buf_export_info wasn't
> > > > updated properly, but virtio_dma_buf_export_info is dead simple, so I
> > > > don't know if that's really a problem.
> > > >
> > > > -David
> > >
> > > I think you can get weird crashes if fields in dma buf are reordered, or
> > > if a field size changes. You have a build bug catching overall struct
> > > size changes but that can remain the same due do compiler padding or
> > > such.
> >
> > Since it's manually copying the fields instead of trying something
> > clever like memcpy, I don't see how reordering the fields or changing
> > the size of the fields would cause problems. Right now,
> > virtio_dma_buf_export is just a regular client of dma_buf_export, no
> > different than any of the other call sites in the kernel.
> >
> > Overall, I don't really think that this is a problem. If someone makes
> > breaking changes to the semantics of dma-buf, then they will need to
> > update this call site, just like they will need to update all of the
> > other call sites in the kernel. If someone adds new functionality to
> > dma-buf and adds another field to dma_buf_export_info, the build bug
> > is a reminder to add it to virtio_dma_buf_export_info. However, if the
> > struct padding happens to work out such that the build bug doesn't
> > trigger, that doesn't really matter - it just means that the new
> > dma-buf feature won't be exposed by virito-dma-buf until someone needs
> > it and notices that the new field is missing.
> >
> > -David
>
> Think about the reasons for the BUILD_BUG_ON being there, checking
> struct sizes like this is a clear sign of something strange going on.
Like I said, it's there as a reminder to update the virtio-dma-buf API
if the dma-buf API gets updated. Perhaps you could say it's a misuse
of BUILD_BUG_ON, since not updating virtio-dma-buf in such a situation
isn't a bug per se. If the BUILD_BUG_ON actually caught a real bug,
there would be a much more fundamental issue going on. The code is
doing nothing strange, nothing fundamentally different from any other
call such as in drm_gem_prime_export or udmabuf_create - it's just
setting fields in the dma_buf_export_info struct to values.
> But really this is just unnecessary complexity anyway.
>
> The only difference with dma_buf is get_uuid and device_attacj, isn't it?
>
> And they are called like this:
>
>
>
> + */
> +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> + uuid_t *uuid)
> +{
> + const struct virtio_dma_buf_ops *ops = container_of(
> + dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> +
> + if (!is_virtio_dma_buf(dma_buf))
> + return -EINVAL;
> +
> + return ops->get_uuid(dma_buf, uuid);
> +}
>
>
> So you are doing the container_of trick anyway, the extra structure
> did not give us any type safety.
Lack of type safety in one situation doesn't mean that type safety in
another situation isn't worthwhile. If that were the case, then why
does C have typedef? Why does Linux bother with types at all, since
you can cast anything to anything with strict aliasing disabled? We
can still make the virtio_dma_buf_export function more type-safe and
readable while still having some unavoidable casting.
But that being said, it's not worth further bikeshedding. I'll send
out an updated patch set.
-David
^ permalink raw reply
* Re: [PATCH 5/6] vdpa: introduce virtio pci driver
From: Michael S. Tsirkin @ 2020-06-08 9:31 UTC (permalink / raw)
To: Jason Wang
Cc: kvm, virtualization, netdev, linux-kernel, rob.miller,
lingshan.zhu, eperezma, lulu, shahafs, hanand, mhabets, gdawar,
saugatm, vmireyno, zhangweining, eli
In-Reply-To: <a1b1b7fb-b097-17b7-2e3a-0da07d2e48ae@redhat.com>
On Mon, Jun 08, 2020 at 05:18:44PM +0800, Jason Wang wrote:
>
> On 2020/6/8 下午2:32, Michael S. Tsirkin wrote:
> > On Mon, Jun 08, 2020 at 11:32:31AM +0800, Jason Wang wrote:
> > > On 2020/6/7 下午9:51, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 05, 2020 at 04:54:17PM +0800, Jason Wang wrote:
> > > > > On 2020/6/2 下午3:08, Jason Wang wrote:
> > > > > > > > +static const struct pci_device_id vp_vdpa_id_table[] = {
> > > > > > > > + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID) },
> > > > > > > > + { 0 }
> > > > > > > > +};
> > > > > > > This looks like it'll create a mess with either virtio pci
> > > > > > > or vdpa being loaded at random. Maybe just don't specify
> > > > > > > any IDs for now. Down the road we could get a
> > > > > > > distinct vendor ID or a range of device IDs for this.
> > > > > > Right, will do.
> > > > > >
> > > > > > Thanks
> > > > > Rethink about this. If we don't specify any ID, the binding won't work.
> > > > We can bind manually. It's not really for production anyway, so
> > > > not a big deal imho.
> > >
> > > I think you mean doing it via "new_id", right.
> > I really meant driver_override. This is what people have been using
> > with pci-stub for years now.
>
>
> Do you want me to implement "driver_overrid" in this series, or a NULL
> id_table is sufficient?
Doesn't the pci subsystem create driver_override for all devices
on the pci bus?
>
> >
> > > > > How about using a dedicated subsystem vendor id for this?
> > > > >
> > > > > Thanks
> > > > If virtio vendor id is used then standard driver is expected
> > > > to bind, right? Maybe use a dedicated vendor id?
> > >
> > > I meant something like:
> > >
> > > static const struct pci_device_id vp_vdpa_id_table[] = {
> > > { PCI_DEVICE_SUB(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID,
> > > VP_TEST_VENDOR_ID, VP_TEST_DEVICE_ID) },
> > > { 0 }
> > > };
> > >
> > > Thanks
> > >
> > Then regular virtio will still bind to it. It has
> >
> > drivers/virtio/virtio_pci_common.c: { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID) },
> >
> >
>
> IFCVF use this to avoid the binding to regular virtio device.
Ow. Indeed:
#define IFCVF_VENDOR_ID 0x1AF4
Which is of course not an IFCVF vendor id, it's the Red Hat vendor ID.
I missed that.
Does it actually work if you bind a virtio driver to it?
I'm guessing no otherwise they wouldn't need IFC driver, right?
> Looking at
> pci_match_one_device() it checks both subvendor and subdevice there.
>
> Thanks
But IIUC there is no guarantee that driver with a specific subvendor
matches in presence of a generic one.
So either IFC or virtio pci can win, whichever binds first.
I guess we need to blacklist IFC in virtio pci probe code. Ugh.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio_mem: prevent overflow with subblock size
From: Michael S. Tsirkin @ 2020-06-08 9:41 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, Jason Wang, Pankaj Gupta, virtualization, teawater
In-Reply-To: <d10e9515-8408-037c-385a-d69259ce70ee@redhat.com>
On Mon, Jun 08, 2020 at 09:17:45AM +0200, David Hildenbrand wrote:
> On 08.06.20 09:08, Michael S. Tsirkin wrote:
> > On Mon, Jun 08, 2020 at 08:58:31AM +0200, David Hildenbrand wrote:
> >> On 08.06.20 08:14, Michael S. Tsirkin wrote:
> >>> If subblock size is large (e.g. 1G) 32 bit math involving it
> >>> can overflow. Rather than try to catch all instances of that,
> >>> let's tweak block size to 64 bit.
> >>
> >> I fail to see where we could actually trigger an overflow. The reported
> >> warning looked like a false positive to me.
> >
> >
> > So
> >
> > const uint64_t size = count * vm->subblock_size;
> >
> > is it unreasonable for count to be 4K with subblock_size being 1M?
>
> virtio_mem_mb_plug_sb() and friends are only called on subblocks
> residing within a single Linux memory block. (currently, 128MB .. 2G on
> x86-64). A subblock on x86-64 is currently at least 4MB.
>
> So "count * vm->subblock_size" can currently not exceed the Linux memory
> block size (in practice, it is max 128MB).
>
> >
> >>>
> >>> It ripples through UAPI which is an ABI change, but it's not too late to
> >>> make it, and it will allow supporting >4Gbyte blocks while might
> >>> become necessary down the road.
> >>>
> >>
> >> This might break cloud-hypervisor, who's already implementing this
> >> protocol upstream (ccing Hui).
> >> https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs
> >>
> >> (blocks in the gigabyte range were never the original intention of
> >> virtio-mem, but I am not completely opposed to that)
> >
> >
> > So in that case, can you code up validation in the probe function?
>
> If we would currently have a "block_size" > Linux memory block size, we
> bail out.
>
> virtio_mem_init():
>
> if (vm->device_block_size > memory_block_size_bytes()) {
> dev_err(&vm->vdev->dev,
> "The block size is not supported (too big).\n");
> return -EINVAL;
> }
Sounds good.
> So what's reported can currently not happen. Having that said, changing
> "subblock_size" to be an uint64_t is a good cleanup, especially for the
> future.
OK, no need to argue about it then. I tweaked the subject as you
suggested and queued it then.
>
>
>
> --
> Thanks,
>
> David / dhildenb
^ permalink raw reply
* Re: [PATCH v3 4/4] drm/virtio: Support virtgpu exported resources
From: Michael S. Tsirkin @ 2020-06-08 9:43 UTC (permalink / raw)
To: David Stevens
Cc: Gerd Hoffmann, David Airlie, Daniel Vetter, Jason Wang,
Sumit Semwal, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
open list, ML dri-devel, open list:VIRTIO GPU DRIVER,
Linux Media Mailing List,
moderated list:DMA BUFFER SHARING FRAMEWORK, virtio-dev
In-Reply-To: <CAD=HUj5qcMLw__LfJizR6nzCR9Qmu21Sjk3i0j_8+=rxt1Hk=w@mail.gmail.com>
On Fri, May 15, 2020 at 04:26:15PM +0900, David Stevens wrote:
> > > + if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_RESOURCE_UUID)) {
> > > + vgdev->has_resource_assign_uuid = true;
> > > + }
> >
> >
> > Just a question: this relies on DMA bufs so I assume it is
> > not really assumed to work when DMA API is bypassed, right?
> > Rather than worry what does it mean, how about just
> > disabling this feature without PLATFORM_DMA for now?
>
> By PLATFORM_DMA, do you mean CONFIG_DMA_SHARED_BUFFER?
Sorry, no. I mean VIRTIO_F_IOMMU_PLATFORM which in the
future will be renamed to VIRTIO_F_PLATFORM_ACCESS.
> Virtio-gpu
> depends on DRM, which selects that feature. So I think DMA bufs should
> always be available when virtio-gpu is present.
>
> -David
^ permalink raw reply
* Re: [PATCH 5/6] vdpa: introduce virtio pci driver
From: Jason Wang @ 2020-06-08 9:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, virtualization, netdev, linux-kernel, rob.miller,
lingshan.zhu, eperezma, lulu, shahafs, hanand, mhabets, gdawar,
saugatm, vmireyno, zhangweining, eli
In-Reply-To: <20200608052041-mutt-send-email-mst@kernel.org>
On 2020/6/8 下午5:31, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2020 at 05:18:44PM +0800, Jason Wang wrote:
>> On 2020/6/8 下午2:32, Michael S. Tsirkin wrote:
>>> On Mon, Jun 08, 2020 at 11:32:31AM +0800, Jason Wang wrote:
>>>> On 2020/6/7 下午9:51, Michael S. Tsirkin wrote:
>>>>> On Fri, Jun 05, 2020 at 04:54:17PM +0800, Jason Wang wrote:
>>>>>> On 2020/6/2 下午3:08, Jason Wang wrote:
>>>>>>>>> +static const struct pci_device_id vp_vdpa_id_table[] = {
>>>>>>>>> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID) },
>>>>>>>>> + { 0 }
>>>>>>>>> +};
>>>>>>>> This looks like it'll create a mess with either virtio pci
>>>>>>>> or vdpa being loaded at random. Maybe just don't specify
>>>>>>>> any IDs for now. Down the road we could get a
>>>>>>>> distinct vendor ID or a range of device IDs for this.
>>>>>>> Right, will do.
>>>>>>>
>>>>>>> Thanks
>>>>>> Rethink about this. If we don't specify any ID, the binding won't work.
>>>>> We can bind manually. It's not really for production anyway, so
>>>>> not a big deal imho.
>>>> I think you mean doing it via "new_id", right.
>>> I really meant driver_override. This is what people have been using
>>> with pci-stub for years now.
>>
>> Do you want me to implement "driver_overrid" in this series, or a NULL
>> id_table is sufficient?
>
> Doesn't the pci subsystem create driver_override for all devices
> on the pci bus?
Yes, I miss this.
>>>>>> How about using a dedicated subsystem vendor id for this?
>>>>>>
>>>>>> Thanks
>>>>> If virtio vendor id is used then standard driver is expected
>>>>> to bind, right? Maybe use a dedicated vendor id?
>>>> I meant something like:
>>>>
>>>> static const struct pci_device_id vp_vdpa_id_table[] = {
>>>> { PCI_DEVICE_SUB(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID,
>>>> VP_TEST_VENDOR_ID, VP_TEST_DEVICE_ID) },
>>>> { 0 }
>>>> };
>>>>
>>>> Thanks
>>>>
>>> Then regular virtio will still bind to it. It has
>>>
>>> drivers/virtio/virtio_pci_common.c: { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID) },
>>>
>>>
>> IFCVF use this to avoid the binding to regular virtio device.
>
> Ow. Indeed:
>
> #define IFCVF_VENDOR_ID 0x1AF4
>
> Which is of course not an IFCVF vendor id, it's the Red Hat vendor ID.
>
> I missed that.
>
> Does it actually work if you bind a virtio driver to it?
It works.
> I'm guessing no otherwise they wouldn't need IFC driver, right?
>
Looking at the driver, they used a dedicated bar for dealing with
virtqueue state save/restore. It
>
>
>> Looking at
>> pci_match_one_device() it checks both subvendor and subdevice there.
>>
>> Thanks
>
> But IIUC there is no guarantee that driver with a specific subvendor
> matches in presence of a generic one.
> So either IFC or virtio pci can win, whichever binds first.
I'm not sure I get there. But I try manually bind IFCVF to qemu's
virtio-net-pci, and it fails.
Thanks
>
> I guess we need to blacklist IFC in virtio pci probe code. Ugh.
>
^ permalink raw reply
* Re: [PATCH 5/6] vdpa: introduce virtio pci driver
From: Michael S. Tsirkin @ 2020-06-08 9:45 UTC (permalink / raw)
To: Jason Wang
Cc: shahafs, lulu, kvm, saugatm, netdev, mhabets, vmireyno,
linux-kernel, gdawar, virtualization, eperezma, hanand,
zhangweining, eli, lingshan.zhu, rob.miller
In-Reply-To: <9d2571b6-0b95-53b3-6989-b4d801eeb623@redhat.com>
On Mon, Jun 08, 2020 at 05:43:58PM +0800, Jason Wang wrote:
> >
> > > Looking at
> > > pci_match_one_device() it checks both subvendor and subdevice there.
> > >
> > > Thanks
> >
> > But IIUC there is no guarantee that driver with a specific subvendor
> > matches in presence of a generic one.
> > So either IFC or virtio pci can win, whichever binds first.
>
>
> I'm not sure I get there. But I try manually bind IFCVF to qemu's
> virtio-net-pci, and it fails.
>
> Thanks
Right but the reverse can happen: virtio-net can bind to IFCVF first.
--
MST
^ permalink raw reply
* Re: [PATCH 5/6] vdpa: introduce virtio pci driver
From: Jason Wang @ 2020-06-08 9:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, virtualization, netdev, linux-kernel, rob.miller,
lingshan.zhu, eperezma, lulu, shahafs, hanand, mhabets, gdawar,
saugatm, vmireyno, zhangweining, eli
In-Reply-To: <20200608054453-mutt-send-email-mst@kernel.org>
On 2020/6/8 下午5:45, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2020 at 05:43:58PM +0800, Jason Wang wrote:
>>>> Looking at
>>>> pci_match_one_device() it checks both subvendor and subdevice there.
>>>>
>>>> Thanks
>>> But IIUC there is no guarantee that driver with a specific subvendor
>>> matches in presence of a generic one.
>>> So either IFC or virtio pci can win, whichever binds first.
>>
>> I'm not sure I get there. But I try manually bind IFCVF to qemu's
>> virtio-net-pci, and it fails.
>>
>> Thanks
> Right but the reverse can happen: virtio-net can bind to IFCVF first.
That's kind of expected. The PF is expected to be bound to virtio-pci to
create VF via sysfs.
Thanks
>
^ permalink raw reply
* Re: [PATCH 5/6] vdpa: introduce virtio pci driver
From: Michael S. Tsirkin @ 2020-06-08 9:54 UTC (permalink / raw)
To: Jason Wang
Cc: kvm, virtualization, netdev, linux-kernel, rob.miller,
lingshan.zhu, eperezma, lulu, shahafs, hanand, mhabets, gdawar,
saugatm, vmireyno, zhangweining, eli
In-Reply-To: <bc27064c-2309-acf3-ccd8-6182bfa2a4cd@redhat.com>
On Mon, Jun 08, 2020 at 05:46:52PM +0800, Jason Wang wrote:
>
> On 2020/6/8 下午5:45, Michael S. Tsirkin wrote:
> > On Mon, Jun 08, 2020 at 05:43:58PM +0800, Jason Wang wrote:
> > > > > Looking at
> > > > > pci_match_one_device() it checks both subvendor and subdevice there.
> > > > >
> > > > > Thanks
> > > > But IIUC there is no guarantee that driver with a specific subvendor
> > > > matches in presence of a generic one.
> > > > So either IFC or virtio pci can win, whichever binds first.
> > >
> > > I'm not sure I get there. But I try manually bind IFCVF to qemu's
> > > virtio-net-pci, and it fails.
> > >
> > > Thanks
> > Right but the reverse can happen: virtio-net can bind to IFCVF first.
>
>
> That's kind of expected. The PF is expected to be bound to virtio-pci to
> create VF via sysfs.
>
> Thanks
>
>
>
Once VFs are created, don't we want IFCVF to bind rather than
virtio-pci?
--
MST
^ permalink raw reply
* Re: [PATCH RFC v5 11/13] vhost/scsi: switch to buf APIs
From: Stefan Hajnoczi @ 2020-06-08 9:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, linux-kernel, virtualization, eperezma,
Stefan Hajnoczi, Paolo Bonzini
In-Reply-To: <20200607141057.704085-12-mst@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 471 bytes --]
On Sun, Jun 07, 2020 at 10:11:46AM -0400, Michael S. Tsirkin wrote:
> Switch to buf APIs. Doing this exposes a spec violation in vhost scsi:
> all used bufs are marked with length 0.
> Fix that is left for another day.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/scsi.c | 73 ++++++++++++++++++++++++++------------------
> 1 file changed, 44 insertions(+), 29 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 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 RFC v5 12/13] vhost/vsock: switch to the buf API
From: Stefan Hajnoczi @ 2020-06-08 9:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, linux-kernel, virtualization, eperezma,
Stefan Hajnoczi
In-Reply-To: <20200607141057.704085-13-mst@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 338 bytes --]
On Sun, Jun 07, 2020 at 10:11:49AM -0400, Michael S. Tsirkin wrote:
> A straight-forward conversion.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/vsock.c | 30 ++++++++++++++++++------------
> 1 file changed, 18 insertions(+), 12 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 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 5/6] vdpa: introduce virtio pci driver
From: Jason Wang @ 2020-06-08 10:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, virtualization, netdev, linux-kernel, rob.miller,
lingshan.zhu, eperezma, lulu, shahafs, hanand, mhabets, gdawar,
saugatm, vmireyno, zhangweining, eli
In-Reply-To: <20200608055331-mutt-send-email-mst@kernel.org>
On 2020/6/8 下午5:54, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2020 at 05:46:52PM +0800, Jason Wang wrote:
>> On 2020/6/8 下午5:45, Michael S. Tsirkin wrote:
>>> On Mon, Jun 08, 2020 at 05:43:58PM +0800, Jason Wang wrote:
>>>>>> Looking at
>>>>>> pci_match_one_device() it checks both subvendor and subdevice there.
>>>>>>
>>>>>> Thanks
>>>>> But IIUC there is no guarantee that driver with a specific subvendor
>>>>> matches in presence of a generic one.
>>>>> So either IFC or virtio pci can win, whichever binds first.
>>>> I'm not sure I get there. But I try manually bind IFCVF to qemu's
>>>> virtio-net-pci, and it fails.
>>>>
>>>> Thanks
>>> Right but the reverse can happen: virtio-net can bind to IFCVF first.
>>
>> That's kind of expected. The PF is expected to be bound to virtio-pci to
>> create VF via sysfs.
>>
>> Thanks
>>
>>
>>
> Once VFs are created, don't we want IFCVF to bind rather than
> virtio-pci?
Yes, but for PF we need virtio-pci.
Thanks
>
^ permalink raw reply
* Re: [PATCH v3 0/5] Add a vhost RPMsg API
From: Guennadi Liakhovetski @ 2020-06-08 10:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, linux-remoteproc, virtualization, sound-open-firmware,
Pierre-Louis Bossart, Liam Girdwood, Jason Wang, Ohad Ben-Cohen,
Bjorn Andersson, Mathieu Poirier
In-Reply-To: <20200608051358-mutt-send-email-mst@kernel.org>
On Mon, Jun 08, 2020 at 05:19:06AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote:
> > Update: I looked through VirtIO 1.0 and 1.1 specs, data format their,
> > including byte order, is defined on a per-device type basis. RPMsg is
> > indeed included in the spec as device type 7, but that's the only
> > mention of it in both versions. It seems RPMsg over VirtIO isn't
> > standardised yet.
>
> Yes. And it would be very good to have some standartization before we
> keep adding things. For example without any spec if host code breaks
> with some guests, how do we know which side should be fixed?
>
> > Also it looks like newer interface definitions
> > specify using "guest native endianness" for Virtual Queue data.
>
> They really don't or shouldn't. That's limited to legacy chapters.
> Some definitions could have slipped through but it's not
> the norm. I just quickly looked through the 1.1 spec and could
> not find any instances that specify "guest native endianness"
> but feel free to point them out to me.
Oh, there you go. No, sorry, my fault, it's the other way round: "guest
native" is for legacy and LE is for current / v1.0 and up.
> > So
> > I think the same should be done for RPMsg instead of enforcing LE?
>
> That makes hardware implementations as well as any cross-endian
> hypervisors tricky.
Yes, LE it is then. And we need to add some text to the spec.
In theory there could be a backward compatibility issue - in case someone
was already using virtio_rpmsg_bus.c in BE mode, but I very much doubt
that...
Thanks
Guennadi
^ permalink raw reply
* Re: [PATCH RFC v5 12/13] vhost/vsock: switch to the buf API
From: Stefano Garzarella @ 2020-06-08 10:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, kvm, virtualization, netdev, Jason Wang, eperezma,
Stefan Hajnoczi
In-Reply-To: <20200607141057.704085-13-mst@redhat.com>
On Sun, Jun 07, 2020 at 10:11:49AM -0400, Michael S. Tsirkin wrote:
> A straight-forward conversion.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/vsock.c | 30 ++++++++++++++++++------------
> 1 file changed, 18 insertions(+), 12 deletions(-)
The changes for vsock part LGTM:
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
I also did some tests with vhost-vsock (tools/testing/vsock/vsock_test
and iperf-vsock), so for vsock:
Tested-by: Stefano Garzarella <sgarzare@redhat.com>
Thanks,
Stefano
>
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index a483cec31d5c..61c6d3dd2ae3 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -103,7 +103,8 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> unsigned out, in;
> size_t nbytes;
> size_t iov_len, payload_len;
> - int head;
> + struct vhost_buf buf;
> + int ret;
>
> spin_lock_bh(&vsock->send_pkt_list_lock);
> if (list_empty(&vsock->send_pkt_list)) {
> @@ -117,16 +118,17 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> list_del_init(&pkt->list);
> spin_unlock_bh(&vsock->send_pkt_list_lock);
>
> - head = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
> - &out, &in, NULL, NULL);
> - if (head < 0) {
> + ret = vhost_get_avail_buf(vq, &buf,
> + vq->iov, ARRAY_SIZE(vq->iov),
> + &out, &in, NULL, NULL);
> + if (ret < 0) {
> spin_lock_bh(&vsock->send_pkt_list_lock);
> list_add(&pkt->list, &vsock->send_pkt_list);
> spin_unlock_bh(&vsock->send_pkt_list_lock);
> break;
> }
>
> - if (head == vq->num) {
> + if (!ret) {
> spin_lock_bh(&vsock->send_pkt_list_lock);
> list_add(&pkt->list, &vsock->send_pkt_list);
> spin_unlock_bh(&vsock->send_pkt_list_lock);
> @@ -186,7 +188,8 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> */
> virtio_transport_deliver_tap_pkt(pkt);
>
> - vhost_add_used(vq, head, sizeof(pkt->hdr) + payload_len);
> + buf.in_len = sizeof(pkt->hdr) + payload_len;
> + vhost_put_used_buf(vq, &buf);
> added = true;
>
> pkt->off += payload_len;
> @@ -440,7 +443,8 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
> struct vhost_vsock *vsock = container_of(vq->dev, struct vhost_vsock,
> dev);
> struct virtio_vsock_pkt *pkt;
> - int head, pkts = 0, total_len = 0;
> + int ret, pkts = 0, total_len = 0;
> + struct vhost_buf buf;
> unsigned int out, in;
> bool added = false;
>
> @@ -461,12 +465,13 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
> goto no_more_replies;
> }
>
> - head = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
> - &out, &in, NULL, NULL);
> - if (head < 0)
> + ret = vhost_get_avail_buf(vq, &buf,
> + vq->iov, ARRAY_SIZE(vq->iov),
> + &out, &in, NULL, NULL);
> + if (ret < 0)
> break;
>
> - if (head == vq->num) {
> + if (!ret) {
> if (unlikely(vhost_enable_notify(&vsock->dev, vq))) {
> vhost_disable_notify(&vsock->dev, vq);
> continue;
> @@ -494,7 +499,8 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
> virtio_transport_free_pkt(pkt);
>
> len += sizeof(pkt->hdr);
> - vhost_add_used(vq, head, len);
> + buf.in_len = len;
> + vhost_put_used_buf(vq, &buf);
> total_len += len;
> added = true;
> } while(likely(!vhost_exceeds_weight(vq, ++pkts, total_len)));
> --
> MST
>
^ permalink raw reply
* Re: [PATCH v3 4/4] drm/virtio: Support virtgpu exported resources
From: David Stevens @ 2020-06-08 10:36 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Gerd Hoffmann, David Airlie, Daniel Vetter, Jason Wang,
Sumit Semwal, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
open list, ML dri-devel, open list:VIRTIO GPU DRIVER,
Linux Media Mailing List,
moderated list:DMA BUFFER SHARING FRAMEWORK, virtio-dev
In-Reply-To: <20200608054234-mutt-send-email-mst@kernel.org>
On Mon, Jun 8, 2020 at 6:43 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, May 15, 2020 at 04:26:15PM +0900, David Stevens wrote:
> > > > + if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_RESOURCE_UUID)) {
> > > > + vgdev->has_resource_assign_uuid = true;
> > > > + }
> > >
> > >
> > > Just a question: this relies on DMA bufs so I assume it is
> > > not really assumed to work when DMA API is bypassed, right?
> > > Rather than worry what does it mean, how about just
> > > disabling this feature without PLATFORM_DMA for now?
> >
> > By PLATFORM_DMA, do you mean CONFIG_DMA_SHARED_BUFFER?
>
> Sorry, no. I mean VIRTIO_F_IOMMU_PLATFORM which in the
> future will be renamed to VIRTIO_F_PLATFORM_ACCESS.
Shouldn't things work independent of whether or not that feature is
set? If a virtio driver properly uses the dma_buf APIs (which virtgpu
seems to), then that should take care of any mapping/synchronization
related to VIRTIO_F_IOMMU_PLATFORM. If anything, the case where
VIRTIO_F_IOMMU_PLATFORM isn't set is easier, since then we know that
the "the device has same access [sic] to memory addresses supplied to
it as the driver has", according to the specification.
-David
^ permalink raw reply
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