Re: [PATCH net-next v9 0/4] vsock/virtio/vhost: MSG_ZEROCOPY preparations
On Tue, Sep 19, 2023 at 03:35:51PM +0200, Stefano Garzarella wrote: > On Tue, Sep 19, 2023 at 03:19:54PM +0200, Paolo Abeni wrote: > > On Tue, 2023-09-19 at 09:54 +0200, Stefano Garzarella wrote: > > > On Mon, Sep 18, 2023 at 07:56:00PM +0300, Arseniy Krasnov wrote: > > > > Hi Stefano, > > > > > > > > thanks for review! So when this patchset will be merged to net-next, > > > > I'll start sending next part of MSG_ZEROCOPY patchset, e.g. AF_VSOCK + > > > > Documentation/ patches. > > > > > > Ack, if it is not a very big series, maybe better to include also the > > > tests so we can run them before merge the feature. > > > > I understand that at least 2 follow-up series are waiting for this, one > > of them targeting net-next and the bigger one targeting the virtio > > tree. Am I correct? > > IIUC the next series will touch only the vsock core > (net/vmw_vsock/af_vsock.c), tests, and documentation. > > The virtio part should be fully covered by this series. > > @Arseniy feel free to correct me! > > > > > DaveM suggests this should go via the virtio tree, too. Any different > > opinion? > > For this series should be fine, I'm not sure about the next series. > Merging this with the virtio tree, then it forces us to do it for > followup as well right? > > In theory followup is more on the core, so better with net-next, but > it's also true that for now only virtio transports support it, so it > might be okay to continue with virtio. > > @Michael WDYT? > > Thanks, > Stefano I didn't get DaveM's mail - was this off-list? I think net-next is easier because the follow up belongs in net-next. But if not I can take it, sure. Let me know. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: virtio-sound linux driver conformance to spec
On Tue, Sep 19, 2023 at 04:18:57PM +0200, Matias Ezequiel Vara Larsen wrote: > On Tue, Sep 19, 2023 at 05:43:56AM -0400, Michael S. Tsirkin wrote: > > On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote: > > > Hello, > > > > > > This email is to report a behavior of the Linux virtio-sound driver that > > > looks like it is not conforming to the VirtIO specification. The kernel > > > driver is moving buffers from the used ring to the available ring > > > without knowing if the content has been updated from the user. If the > > > device picks up buffers from the available ring just after it is > > > notified, it happens that the content is old. > > > > Then, what happens, exactly? Do things still work? > > We are currently developing a vhost-user backend for virtio-sound and > what happens is that if the backend implementation decides to copy the > content of a buffer from a request that just arrived to the available > ring, it gets the old content thus reproducing some sections two times. > For example, we observe that when issuing `aplay FrontLeft.wav`, we hear > `Front, front left...`. To fix this issue, our current implementation > delays reading from guest memory just until the audio engine requires. > However, the first implementation shall also work since it is conforming > to the specification. > > Matias Sounds like it. How hard is it to change the behaviour though? Does it involve changing userspace? Maybe we need to fix the spec after all... -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [virtio-comment] Re: virtio-sound linux driver conformance to spec
On Tue, Sep 19, 2023 at 08:58:32AM +0200, Paolo Bonzini wrote: > On 9/19/23 02:35, Anton Yakovlev wrote: > > > > If the Linux virtio sound driver violates a specification, then there > > must be > > a conformance statement that the driver does not follow. As far as I know, > > there is no such thing at the moment. > > There is one in 2.7.13.3: "The device MAY access the descriptor chains the > driver created and the memory they refer to immediately" > > And likewise for packed virtqueues in 2.8.21.1: "The device MAY access the > descriptor and any following descriptors the driver created and the memory > they refer to immediately" > > I think it's a mistake to use MAY here, as opposed to "may". This is not an > optional feature, it's a MUST NOT requirement on the driver's part that > should be in 2.7.13.3.1 and 2.8.21.1.1. > > This does not prevent the virtio-snd spec from overriding this. If an > override is desirable (for example because other hardware behaves like > this), there should be a provision in 2.7.13.3.1 and 2.8.21.1.1. For > example: > > 2.7.13.3.1 Unless the device specification specifies otherwise, the driver > MUST NOT write to the descriptor chains and the memory they refer to, > between the /idx/ update and the time the device places the driver on the > used ring. > > 2.8.21.1.1 "Unless the device specification specifies otherwise, the driver > MUST NOT write to the descriptor, to any following descriptors the driver > created, nor to the memory the refer to, between the /flags/ update and the > time the device places the driver on the used ring. > > > In the virtio-snd there would be a normative statement like > > 5.14.6.8.1.1 The device MUST NOT read from available device-readable > buffers beyond the first buffer_bytes / period_bytes periods. > > 5.14.6.8.1.2 The driver MAY write to device-readable buffers beyond the > first buffer_bytes / period_bytes periods, even after offering them to the > device. > > > > As an aside, here are two other statements that have a similar issue: > > - 2.6.1.1.2 "the driver MAY release any resource associated with that > virtqueue" (instead 2.6.1.1.1 should have something like "After a queue has > been reset by the driver, the device MUST NOT access any resource associated > with a virtqueue"). > > - 2.7.5.1 "[the device] MAY do so for debugging or diagnostic purposes" > (this is not normative and can be just "may") The spec should not make an exception for virtio-sound because the virtqueue model was not intended as a shared memory mechanism. Allowing it would prevent message-passing implementations of virtqueues. Instead the device should use Shared Memory Regions: https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-10200010 BTW, the virtio-sound spec already has VIRTIO_SND_PCM_F_SHMEM_HOST and VIRTIO_SND_PCM_F_SHMEM_GUEST bits reserved but they currently have no meaning. I wonder what that was intended for? Stefan signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 1/2] iommu/virtio: Make use of ops->iotlb_sync_map
On Tue, Sep 19, 2023 at 09:15:19AM +0100, Jean-Philippe Brucker wrote:
> On Mon, Sep 18, 2023 at 05:37:47PM +0100, Robin Murphy wrote:
> > > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > > index 17dcd826f5c2..3649586f0e5c 100644
> > > --- a/drivers/iommu/virtio-iommu.c
> > > +++ b/drivers/iommu/virtio-iommu.c
> > > @@ -189,6 +189,12 @@ static int viommu_sync_req(struct viommu_dev *viommu)
> > > int ret;
> > > unsigned long flags;
> > > + /*
> > > + * .iotlb_sync_map and .flush_iotlb_all may be called before the viommu
> > > + * is initialized e.g. via iommu_create_device_direct_mappings()
> > > + */
> > > + if (!viommu)
> > > + return 0;
> >
> > Minor nit: I'd be inclined to make that check explicitly in the places where
> > it definitely is expected, rather than allowing *any* sync to silently do
> > nothing if called incorrectly. Plus then they could use
> > vdomain->nr_endpoints for consistency with the equivalent checks elsewhere
> > (it did take me a moment to figure out how we could get to .iotlb_sync_map
> > with a NULL viommu without viommu_map_pages() blowing up first...)
This makes more sense to me
Ultimately this driver should reach a point where every iommu_domain
always has a non-null domain->viommu because it will be set during
alloc.
But it can still have nr_endpoints == 0, doesn't it make sense to
avoid sync in this case?
(btw this driver is missing locking around vdomain->nr_endpoints)
> They're not strictly equivalent: this check works around a temporary issue
> with the IOMMU core, which calls map/unmap before the domain is
> finalized.
Where? The above points to iommu_create_device_direct_mappings() but
it doesn't because the pgsize_bitmap == 0:
static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
struct device *dev)
{
struct iommu_resv_region *entry;
struct list_head mappings;
unsigned long pg_size;
int ret = 0;
pg_size = domain->pgsize_bitmap ? 1UL << __ffs(domain->pgsize_bitmap) :
0;
INIT_LIST_HEAD();
if (WARN_ON_ONCE(iommu_is_dma_domain(domain) && !pg_size))
Indeed, the driver should be failing all map's until the domain is
finalized because it has no way to check the IOVA matches the eventual
aperture.
Jason
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: virtio-sound linux driver conformance to spec
On Tue, Sep 19, 2023 at 05:43:56AM -0400, Michael S. Tsirkin wrote: > On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote: > > Hello, > > > > This email is to report a behavior of the Linux virtio-sound driver that > > looks like it is not conforming to the VirtIO specification. The kernel > > driver is moving buffers from the used ring to the available ring > > without knowing if the content has been updated from the user. If the > > device picks up buffers from the available ring just after it is > > notified, it happens that the content is old. > > Then, what happens, exactly? Do things still work? We are currently developing a vhost-user backend for virtio-sound and what happens is that if the backend implementation decides to copy the content of a buffer from a request that just arrived to the available ring, it gets the old content thus reproducing some sections two times. For example, we observe that when issuing `aplay FrontLeft.wav`, we hear `Front, front left...`. To fix this issue, our current implementation delays reading from guest memory just until the audio engine requires. However, the first implementation shall also work since it is conforming to the specification. Matias ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v9 0/4] vsock/virtio/vhost: MSG_ZEROCOPY preparations
On Tue, Sep 19, 2023 at 03:19:54PM +0200, Paolo Abeni wrote: On Tue, 2023-09-19 at 09:54 +0200, Stefano Garzarella wrote: On Mon, Sep 18, 2023 at 07:56:00PM +0300, Arseniy Krasnov wrote: > Hi Stefano, > > thanks for review! So when this patchset will be merged to net-next, > I'll start sending next part of MSG_ZEROCOPY patchset, e.g. AF_VSOCK + > Documentation/ patches. Ack, if it is not a very big series, maybe better to include also the tests so we can run them before merge the feature. I understand that at least 2 follow-up series are waiting for this, one of them targeting net-next and the bigger one targeting the virtio tree. Am I correct? IIUC the next series will touch only the vsock core (net/vmw_vsock/af_vsock.c), tests, and documentation. The virtio part should be fully covered by this series. @Arseniy feel free to correct me! DaveM suggests this should go via the virtio tree, too. Any different opinion? For this series should be fine, I'm not sure about the next series. Merging this with the virtio tree, then it forces us to do it for followup as well right? In theory followup is more on the core, so better with net-next, but it's also true that for now only virtio transports support it, so it might be okay to continue with virtio. @Michael WDYT? Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v9 0/4] vsock/virtio/vhost: MSG_ZEROCOPY preparations
On Tue, 2023-09-19 at 09:54 +0200, Stefano Garzarella wrote: > On Mon, Sep 18, 2023 at 07:56:00PM +0300, Arseniy Krasnov wrote: > > Hi Stefano, > > > > thanks for review! So when this patchset will be merged to net-next, > > I'll start sending next part of MSG_ZEROCOPY patchset, e.g. AF_VSOCK + > > Documentation/ patches. > > Ack, if it is not a very big series, maybe better to include also the > tests so we can run them before merge the feature. I understand that at least 2 follow-up series are waiting for this, one of them targeting net-next and the bigger one targeting the virtio tree. Am I correct? DaveM suggests this should go via the virtio tree, too. Any different opinion? Thanks! Paolo ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: virtio-sound linux driver conformance to spec
On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote: > Hello, > > This email is to report a behavior of the Linux virtio-sound driver that > looks like it is not conforming to the VirtIO specification. The kernel > driver is moving buffers from the used ring to the available ring > without knowing if the content has been updated from the user. If the > device picks up buffers from the available ring just after it is > notified, it happens that the content is old. Then, what happens, exactly? Do things still work? > This problem can be fixed > by waiting a period of time after the device dequeues a buffer from the > available ring. The driver should not be allowed to change the content > of a buffer in the available ring. When buffers are enqueued in the > available ring, the device can consume them immediately. > > 1. Is the actual driver implementation following the spec? > 2. Shall the spec be extended to support such a use-case? > > Thanks, Matias ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 1/2] iommu/virtio: Make use of ops->iotlb_sync_map
On 2023-09-19 09:15, Jean-Philippe Brucker wrote: On Mon, Sep 18, 2023 at 05:37:47PM +0100, Robin Murphy wrote: diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c index 17dcd826f5c2..3649586f0e5c 100644 --- a/drivers/iommu/virtio-iommu.c +++ b/drivers/iommu/virtio-iommu.c @@ -189,6 +189,12 @@ static int viommu_sync_req(struct viommu_dev *viommu) int ret; unsigned long flags; + /* +* .iotlb_sync_map and .flush_iotlb_all may be called before the viommu +* is initialized e.g. via iommu_create_device_direct_mappings() +*/ + if (!viommu) + return 0; Minor nit: I'd be inclined to make that check explicitly in the places where it definitely is expected, rather than allowing *any* sync to silently do nothing if called incorrectly. Plus then they could use vdomain->nr_endpoints for consistency with the equivalent checks elsewhere (it did take me a moment to figure out how we could get to .iotlb_sync_map with a NULL viommu without viommu_map_pages() blowing up first...) They're not strictly equivalent: this check works around a temporary issue with the IOMMU core, which calls map/unmap before the domain is finalized. Once we merge domain_alloc() and finalize(), then this check disappears, but we still need to test nr_endpoints in map/unmap to handle detached domains (and we still need to fix the synchronization of nr_endpoints against attach/detach). That's why I preferred doing this on viommu and keeping it in one place. Fair enough - it just seems to me that in both cases it's a detached domain, so its previous history of whether it's ever been otherwise or not shouldn't matter. Even once viommu is initialised, does it really make sense to send sync commands for a mapping on a detached domain where we haven't actually sent any map/unmap commands? Thanks, Robin. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 1/2] iommu/virtio: Make use of ops->iotlb_sync_map
On Mon, Sep 18, 2023 at 05:37:47PM +0100, Robin Murphy wrote: > > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c > > index 17dcd826f5c2..3649586f0e5c 100644 > > --- a/drivers/iommu/virtio-iommu.c > > +++ b/drivers/iommu/virtio-iommu.c > > @@ -189,6 +189,12 @@ static int viommu_sync_req(struct viommu_dev *viommu) > > int ret; > > unsigned long flags; > > + /* > > +* .iotlb_sync_map and .flush_iotlb_all may be called before the viommu > > +* is initialized e.g. via iommu_create_device_direct_mappings() > > +*/ > > + if (!viommu) > > + return 0; > > Minor nit: I'd be inclined to make that check explicitly in the places where > it definitely is expected, rather than allowing *any* sync to silently do > nothing if called incorrectly. Plus then they could use > vdomain->nr_endpoints for consistency with the equivalent checks elsewhere > (it did take me a moment to figure out how we could get to .iotlb_sync_map > with a NULL viommu without viommu_map_pages() blowing up first...) They're not strictly equivalent: this check works around a temporary issue with the IOMMU core, which calls map/unmap before the domain is finalized. Once we merge domain_alloc() and finalize(), then this check disappears, but we still need to test nr_endpoints in map/unmap to handle detached domains (and we still need to fix the synchronization of nr_endpoints against attach/detach). That's why I preferred doing this on viommu and keeping it in one place. Thanks, Jean ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v9 0/4] vsock/virtio/vhost: MSG_ZEROCOPY preparations
On Mon, Sep 18, 2023 at 07:56:00PM +0300, Arseniy Krasnov wrote: Hi Stefano, thanks for review! So when this patchset will be merged to net-next, I'll start sending next part of MSG_ZEROCOPY patchset, e.g. AF_VSOCK + Documentation/ patches. Ack, if it is not a very big series, maybe better to include also the tests so we can run them before merge the feature. WDYT? Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [virtio-comment] Re: virtio-sound linux driver conformance to spec
On 9/19/23 02:35, Anton Yakovlev wrote: If the Linux virtio sound driver violates a specification, then there must be a conformance statement that the driver does not follow. As far as I know, there is no such thing at the moment. There is one in 2.7.13.3: "The device MAY access the descriptor chains the driver created and the memory they refer to immediately" And likewise for packed virtqueues in 2.8.21.1: "The device MAY access the descriptor and any following descriptors the driver created and the memory they refer to immediately" I think it's a mistake to use MAY here, as opposed to "may". This is not an optional feature, it's a MUST NOT requirement on the driver's part that should be in 2.7.13.3.1 and 2.8.21.1.1. This does not prevent the virtio-snd spec from overriding this. If an override is desirable (for example because other hardware behaves like this), there should be a provision in 2.7.13.3.1 and 2.8.21.1.1. For example: 2.7.13.3.1 Unless the device specification specifies otherwise, the driver MUST NOT write to the descriptor chains and the memory they refer to, between the /idx/ update and the time the device places the driver on the used ring. 2.8.21.1.1 "Unless the device specification specifies otherwise, the driver MUST NOT write to the descriptor, to any following descriptors the driver created, nor to the memory the refer to, between the /flags/ update and the time the device places the driver on the used ring. In the virtio-snd there would be a normative statement like 5.14.6.8.1.1 The device MUST NOT read from available device-readable buffers beyond the first buffer_bytes / period_bytes periods. 5.14.6.8.1.2 The driver MAY write to device-readable buffers beyond the first buffer_bytes / period_bytes periods, even after offering them to the device. As an aside, here are two other statements that have a similar issue: - 2.6.1.1.2 "the driver MAY release any resource associated with that virtqueue" (instead 2.6.1.1.1 should have something like "After a queue has been reset by the driver, the device MUST NOT access any resource associated with a virtqueue"). - 2.7.5.1 "[the device] MAY do so for debugging or diagnostic purposes" (this is not normative and can be just "may") Thanks, Paolo ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
