Re: [PATCH v6 9/9] ALSA: virtio: introduce device suspend/resume support
On 02.03.2021 07:48, Takashi Iwai wrote: On Tue, 02 Mar 2021 07:29:20 +0100, Anton Yakovlev wrote: On 28.02.2021 13:05, Takashi Iwai wrote: On Sat, 27 Feb 2021 09:59:56 +0100, Anton Yakovlev wrote: [snip] --- a/sound/virtio/virtio_pcm.c +++ b/sound/virtio/virtio_pcm.c @@ -109,6 +109,7 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss, SNDRV_PCM_INFO_BATCH | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_RESUME | SNDRV_PCM_INFO_PAUSE; Actually you don't need to set SNDRV_PCM_INFO_RESUME. This flag means that the driver supports the full resume procedure, which isn't often the case; with this, the driver is supposed to resume the stream exactly from the suspended position. Most drivers don't set this but implement only the suspend-stop action. Then the application (or the sound backend) will re-setup the stream and restart accordingly. I tried to resume driver without SNDRV_PCM_INFO_RESUME, and alsa-lib called only ops->prepare(). It makes sense for a typical hw, but we have "clean" unconfigured device on resume. And we must set hw parameters as a first step. It means, that code should be more or less the same. And maybe it's better to set SNDRV_PCM_INFO_RESUME, since it allows us to resume substream in any situation (regardless of application behavior). I can refactor code to only send requests from trigger(RESUME) path and not to call ops itself. It should make code more straitforward. What do you say? How about calling hw_params(NULL) conditionally in the prepare? Then the question is that condition. When ops->prepare() is called, the substream is in SUSPENDED state or not? If not then we need to track this in some additional field (and it will make logic a little bit clumsy, since that field is needed to be carefully handled in other places). Doing the full stack work in the trigger callback is bad from the API design POV; in general the trigger callback is supposed to be as short as possible. Yeah, but usually original subsystem design does not take into account para-virtualized devices, which usually have it's own slightly different reality. And we need to introduce some tricks. Takashi -- Anton Yakovlev Senior Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/2 15:24, Viresh Kumar wrote: On 02-03-21, 14:24, Jie Deng wrote: Not for the full duplex. As Paolo explained in those links. We defined a combined request called "write-read request" " This is when a write is followed by a read: the master starts off the transmission with a write, then sends a second START, then continues with a read from the same address. I think above talks about the real I2C protocol at bus level ? In theory there's no difference between one multi-segment transaction and many single-segment transactions _in a single-master scenario_. However, it is a plausible configuration to have multiple guests sharing an I2C host device as if they were multiple master. So the spec should provide a way at least to support for transactions with 1 write and 1 read segment in one request to the same address. " From the perspective of specification design, it hopes to provide more choices while from the perspective of specific implementation, we can choose what we need as long as it does not violate the specification. That is fine, but what I was not able to understand was how do we get to know if we should expect both write and read bufs after the out header or only one of them ? I think I have understood that part now, we need to look at incnt and outcnt and make out what kind of transfer we need to do. - If outcnt == 1 and incnt == 2, then it is read operation. - If outcnt == 2 and incnt == 1, then it is write operation. - If outcnt == 2 and incnt == 2, then it is read-write operation. Anything else is invalid. Is my understanding correct here ? Correct. By checking the sequences of descriptor's R/W in the virtqueue, You can know the type of request. A simple state machine can be used to classify in this case. O I I : read request. O O I : write request. O O I I : read-write request. But if only using the first two types like in this driver, the backend will be much easier to implement since the request is fixed to 3 descriptors and we only need to check the second descriptor to know the type. Since the current Linux driver doesn't use this mechanism. I'm considering to move the "struct virtio_i2c_req" into the driver and use one "buf" instead. Linux can very much have its own definition of the structure and so I am not in favor of any such structure in the spec as well, it confuses people (like me) :). ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] vhost-vdpa: honor CAP_IPC_LOCK
When CAP_IPC_LOCK is set we should not check locked memory against rlimit as what has been implemented in mlock(). Signed-off-by: Jason Wang --- drivers/vhost/vdpa.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index ef688c8c0e0e..e93572e2e344 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -638,7 +638,8 @@ static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, mmap_read_lock(dev->mm); lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; - if (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit) { + if (!capable(CAP_IPC_LOCK) && + (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit)) { ret = -ENOMEM; goto unlock; } -- 2.18.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero
On Mon, Mar 01, 2021 at 11:56:50AM +0800, Jason Wang wrote: > > On 2021/3/1 5:34 上午, Michael S. Tsirkin wrote: > > On Wed, Feb 24, 2021 at 10:24:41AM -0800, Si-Wei Liu wrote: > > > > Detecting it isn't enough though, we will need a new ioctl to notify > > > > the kernel that it's a legacy guest. Ugh :( > > > Well, although I think adding an ioctl is doable, may I know what the use > > > case there will be for kernel to leverage such info directly? Is there a > > > case QEMU can't do with dedicate ioctls later if there's indeed > > > differentiation (legacy v.s. modern) needed? > > BTW a good API could be > > > > #define VHOST_SET_ENDIAN _IOW(VHOST_VIRTIO, ?, int) > > #define VHOST_GET_ENDIAN _IOW(VHOST_VIRTIO, ?, int) > > > > we did it per vring but maybe that was a mistake ... > > > Actually, I wonder whether it's good time to just not support legacy driver > for vDPA. Consider: > > 1) It's definition is no-normative > 2) A lot of budren of codes > > So qemu can still present the legacy device since the config space or other > stuffs that is presented by vhost-vDPA is not expected to be accessed by > guest directly. Qemu can do the endian conversion when necessary in this > case? > > Thanks > Overall I would be fine with this approach but we need to avoid breaking working userspace, qemu releases with vdpa support are out there and seem to work for people. Any changes need to take that into account and document compatibility concerns. I note that any hardware implementation is already broken for legacy except on platforms with strong ordering which might be helpful in reducing the scope. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-vdpa: honor CAP_IPC_LOCK
On Tue, Mar 02, 2021 at 04:14:18AM -0500, Jason Wang wrote: > When CAP_IPC_LOCK is set we should not check locked memory against > rlimit as what has been implemented in mlock(). > > Signed-off-by: Jason Wang Indeed and it's not just mlock. Documentation/admin-guide/perf-security.rst: RLIMIT_MEMLOCK and perf_event_mlock_kb resource constraints are ignored for processes with the CAP_IPC_LOCK capability. and let's add a Fixes: tag? > --- > drivers/vhost/vdpa.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index ef688c8c0e0e..e93572e2e344 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -638,7 +638,8 @@ static int vhost_vdpa_process_iotlb_update(struct > vhost_vdpa *v, > mmap_read_lock(dev->mm); > > lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > - if (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit) { > + if (!capable(CAP_IPC_LOCK) && > + (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit)) { > ret = -ENOMEM; > goto unlock; > } > -- > 2.18.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote: > > On 2021/3/1 23:19, Arnd Bergmann wrote: > > On Mon, Mar 1, 2021 at 7:41 AM Jie Deng wrote: > > > > > --- /dev/null > > > +++ b/include/uapi/linux/virtio_i2c.h > > > @@ -0,0 +1,56 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */ The uapi VIRTIO header files are BSD licensed so they can be easily used by other projects (including other operating systems and hypervisors that don't use Linux). Please see the license headers in or . > > > +/* > > > + * Definitions for virtio I2C Adpter > > > + * > > > + * Copyright (c) 2021 Intel Corporation. All rights reserved. > > > + */ > > > + > > > +#ifndef _UAPI_LINUX_VIRTIO_I2C_H > > > +#define _UAPI_LINUX_VIRTIO_I2C_H > > Why is this a uapi header? Can't this all be moved into the driver > > itself? Linux VIRTIO drivers provide a uapi header with structs and constants that describe the device interface. This allows other software like QEMU, other operating systems, etc to reuse these headers instead of redefining everything. These files should contain: 1. Device-specific feature bits (VIRTIO__F_) 2. VIRTIO Configuration Space layout (struct virtio__config) 3. Virtqueue request layout (struct virtio__) For examples, see and . > > > +/** > > > + * struct virtio_i2c_req - the virtio I2C request structure > > > + * @out_hdr: the OUT header of the virtio I2C message > > > + * @write_buf: contains one I2C segment being written to the device > > > + * @read_buf: contains one I2C segment being read from the device > > > + * @in_hdr: the IN header of the virtio I2C message > > > + */ > > > +struct virtio_i2c_req { > > > + struct virtio_i2c_out_hdr out_hdr; > > > + u8 *write_buf; > > > + u8 *read_buf; > > > + struct virtio_i2c_in_hdr in_hdr; > > > +}; > > In particular, this structure looks like it is only ever usable between > > the transfer functions in the driver itself, it is shared with neither > > user space nor the virtio host side. I agree. This struct is not part of the device interface. It's part of the Linux driver implementation. This belongs inside the driver code and not in include/uapi/ where public headers are located. Stefan signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V2] vhost-vdpa: honor CAP_IPC_LOCK
When CAP_IPC_LOCK is set we should not check locked memory against rlimit as what has been implemented in mlock() and documented in Documentation/admin-guide/perf-security.rst: " RLIMIT_MEMLOCK and perf_event_mlock_kb resource constraints are ignored for processes with the CAP_IPC_LOCK capability. " Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") Signed-off-by: Jason Wang --- drivers/vhost/vdpa.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index ef688c8c0e0e..e93572e2e344 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -638,7 +638,8 @@ static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, mmap_read_lock(dev->mm); lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; - if (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit) { + if (!capable(CAP_IPC_LOCK) && + (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit)) { ret = -ENOMEM; goto unlock; } -- 2.18.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-vdpa: honor CAP_IPC_LOCK
On 2021/3/2 5:51 下午, Michael S. Tsirkin wrote: On Tue, Mar 02, 2021 at 04:14:18AM -0500, Jason Wang wrote: When CAP_IPC_LOCK is set we should not check locked memory against rlimit as what has been implemented in mlock(). Signed-off-by: Jason Wang Indeed and it's not just mlock. Documentation/admin-guide/perf-security.rst: RLIMIT_MEMLOCK and perf_event_mlock_kb resource constraints are ignored for processes with the CAP_IPC_LOCK capability. and let's add a Fixes: tag? Sure, V2 is posted. Thanks --- drivers/vhost/vdpa.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index ef688c8c0e0e..e93572e2e344 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -638,7 +638,8 @@ static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, mmap_read_lock(dev->mm); lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; - if (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit) { + if (!capable(CAP_IPC_LOCK) && + (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit)) { ret = -ENOMEM; goto unlock; } -- 2.18.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero
On 2021/3/2 5:47 下午, Michael S. Tsirkin wrote: On Mon, Mar 01, 2021 at 11:56:50AM +0800, Jason Wang wrote: On 2021/3/1 5:34 上午, Michael S. Tsirkin wrote: On Wed, Feb 24, 2021 at 10:24:41AM -0800, Si-Wei Liu wrote: Detecting it isn't enough though, we will need a new ioctl to notify the kernel that it's a legacy guest. Ugh :( Well, although I think adding an ioctl is doable, may I know what the use case there will be for kernel to leverage such info directly? Is there a case QEMU can't do with dedicate ioctls later if there's indeed differentiation (legacy v.s. modern) needed? BTW a good API could be #define VHOST_SET_ENDIAN _IOW(VHOST_VIRTIO, ?, int) #define VHOST_GET_ENDIAN _IOW(VHOST_VIRTIO, ?, int) we did it per vring but maybe that was a mistake ... Actually, I wonder whether it's good time to just not support legacy driver for vDPA. Consider: 1) It's definition is no-normative 2) A lot of budren of codes So qemu can still present the legacy device since the config space or other stuffs that is presented by vhost-vDPA is not expected to be accessed by guest directly. Qemu can do the endian conversion when necessary in this case? Thanks Overall I would be fine with this approach but we need to avoid breaking working userspace, qemu releases with vdpa support are out there and seem to work for people. Any changes need to take that into account and document compatibility concerns. Agree, let me check. I note that any hardware implementation is already broken for legacy except on platforms with strong ordering which might be helpful in reducing the scope. Yes. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Tue, Mar 2, 2021 at 10:51 AM Stefan Hajnoczi wrote: > On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote: > > > > +/* > > > > + * Definitions for virtio I2C Adpter > > > > + * > > > > + * Copyright (c) 2021 Intel Corporation. All rights reserved. > > > > + */ > > > > + > > > > +#ifndef _UAPI_LINUX_VIRTIO_I2C_H > > > > +#define _UAPI_LINUX_VIRTIO_I2C_H > > > Why is this a uapi header? Can't this all be moved into the driver > > > itself? > > Linux VIRTIO drivers provide a uapi header with structs and constants > that describe the device interface. This allows other software like > QEMU, other operating systems, etc to reuse these headers instead of > redefining everything. > > These files should contain: > 1. Device-specific feature bits (VIRTIO__F_) > 2. VIRTIO Configuration Space layout (struct virtio__config) > 3. Virtqueue request layout (struct virtio__) > > For examples, see and . Ok, makes sense. So it's not strictly uapi but just helpful for virtio applications to reference these. I suppose it is slightly harder when building qemu on other operating systems though, how does it get these headers on BSD or Windows? Arnd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 06/27] virtio: Add virtio_queue_get_used_notify_split
On Tue, Jan 12, 2021 at 07:21:27PM +0100, Eugenio Perez Martin wrote: > On Mon, Dec 7, 2020 at 5:58 PM Stefan Hajnoczi wrote: > > > > On Fri, Nov 20, 2020 at 07:50:44PM +0100, Eugenio Pérez wrote: > > > This function is just used for a few commits, so SW LM is developed > > > incrementally, and it is deleted after it is useful. > > > > > > For a few commits, only the events (irqfd, eventfd) are forwarded. > > > > s/eventfd/ioeventfd/ (irqfd is also an eventfd) > > > > Oops, will fix, thanks! > > > > +bool virtio_queue_get_used_notify_split(VirtQueue *vq) > > > +{ > > > +VRingMemoryRegionCaches *caches; > > > +hwaddr pa = offsetof(VRingUsed, flags); > > > +uint16_t flags; > > > + > > > +RCU_READ_LOCK_GUARD(); > > > + > > > +caches = vring_get_region_caches(vq); > > > +assert(caches); > > > +flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa); > > > +return !(VRING_USED_F_NO_NOTIFY & flags); > > > +} > > > > QEMU stores the notification status: > > > > void virtio_queue_set_notification(VirtQueue *vq, int enable) > > { > > vq->notification = enable; < here > > > > if (!vq->vring.desc) { > > return; > > } > > > > if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { > > virtio_queue_packed_set_notification(vq, enable); > > } else { > > virtio_queue_split_set_notification(vq, enable); > > > > I'm wondering why it's necessary to fetch from guest RAM instead of > > using vq->notification? It also works for both split and packed > > queues so the code would be simpler. > > To use vq->notification makes sense at the end of the series. > > However, at this stage (just routing notifications, not descriptors), > vhost device is the one updating that flag, not qemu. Since we cannot > just migrate used ring memory to qemu without migrating descriptors > ring too, qemu needs to check guest's memory looking for vhost device > updates on that flag. > > I can see how that deserves better documentation or even a better > name. Also, this function should be in the shadow vq file, not > virtio.c. I can't think of a reason why QEMU needs to know the flag value that the vhost device has set. This flag is a hint to the guest driver indicating whether the device wants to receive notifications. Can you explain why QEMU needs to look at the value of the flag? Stefan signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa/mlx5: Fix wrong use of bit numbers
On Tue, Mar 02, 2021 at 07:23:06AM +0200, Eli Cohen wrote: > On Mon, Mar 01, 2021 at 10:33:14AM -0500, Michael S. Tsirkin wrote: > > On Mon, Mar 01, 2021 at 03:52:45PM +0800, Jason Wang wrote: > > > > > > On 2021/3/1 2:28 下午, Eli Cohen wrote: > > > > VIRTIO_F_VERSION_1 is a bit number. Use BIT_ULL() with mask > > > > conditionals. > > > > > > > > Also, in mlx5_vdpa_is_little_endian() use BIT_ULL for consistency with > > > > the rest of the code. > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 > > > > devices") > > > > Signed-off-by: Eli Cohen > > > > > > > > > Acked-by: Jason Wang > > > > And CC stable I guess? > > Is this a question or a request? :-) A question. net patches are cc'd by net maintainer. > > > > > > > > > --- > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > index dc7031132fff..7d21b857a94a 100644 > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > @@ -821,7 +821,7 @@ static int create_virtqueue(struct mlx5_vdpa_net > > > > *ndev, struct mlx5_vdpa_virtque > > > > MLX5_SET(virtio_q, vq_ctx, event_qpn_or_msix, > > > > mvq->fwqp.mqp.qpn); > > > > MLX5_SET(virtio_q, vq_ctx, queue_size, mvq->num_ent); > > > > MLX5_SET(virtio_q, vq_ctx, virtio_version_1_0, > > > > -!!(ndev->mvdev.actual_features & VIRTIO_F_VERSION_1)); > > > > +!!(ndev->mvdev.actual_features & > > > > BIT_ULL(VIRTIO_F_VERSION_1))); > > > > MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > @@ -1578,7 +1578,7 @@ static void teardown_virtqueues(struct > > > > mlx5_vdpa_net *ndev) > > > > static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev > > > > *mvdev) > > > > { > > > > return virtio_legacy_is_little_endian() || > > > > - (mvdev->actual_features & (1ULL << VIRTIO_F_VERSION_1)); > > > > + (mvdev->actual_features & BIT_ULL(VIRTIO_F_VERSION_1)); > > > > } > > > > static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 > > > > val) > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero
On Mon, 1 Mar 2021 11:51:08 +0800 Jason Wang wrote: > On 2021/3/1 5:25 上午, Michael S. Tsirkin wrote: > > On Fri, Feb 26, 2021 at 04:19:16PM +0800, Jason Wang wrote: > >> On 2021/2/26 2:53 上午, Michael S. Tsirkin wrote: > >>> Confused. What is wrong with the above? It never reads the > >>> field unless the feature has been offered by device. > >> > >> So the spec said: > >> > >> " > >> > >> The following driver-read-only field, max_virtqueue_pairs only exists if > >> VIRTIO_NET_F_MQ is set. > >> > >> " > >> > >> If I read this correctly, there will be no max_virtqueue_pairs field if the > >> VIRTIO_NET_F_MQ is not offered by device? If yes the offsetof() violates > >> what spec said. > >> > >> Thanks > > I think that's a misunderstanding. This text was never intended to > > imply that field offsets change beased on feature bits. > > We had this pain with legacy and we never wanted to go back there. > > > > This merely implies that without VIRTIO_NET_F_MQ the field > > should not be accessed. Exists in the sense "is accessible to driver". > > > > Let's just clarify that in the spec, job done. > > > Ok, agree. That will make things more eaiser. Yes, that makes much more sense. What about adding the following to the "Basic Facilities of a Virtio Device/Device Configuration Space" section of the spec: "If an optional configuration field does not exist, the corresponding space is still present, but reserved." (Do we need to specify what a device needs to do if the driver tries to access a non-existing field? We cannot really make assumptions about config space accesses; virtio-ccw can just copy a chunk of config space that contains non-existing fields... I guess the device could ignore writes and return zeroes on read?) I've opened https://github.com/oasis-tcs/virtio-spec/issues/98 for the spec issues. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space
On Tue, Mar 02, 2021 at 12:05:35PM +0800, Jason Wang wrote: On 2021/2/16 5:44 下午, Stefano Garzarella wrote: vdpa_get_config() and vdpa_set_config() now return the amount of bytes read and written, so let's return them to the user space. We also modify vhost_vdpa_config_validate() to return 0 (bytes read or written) instead of an error, when the buffer length is 0. Signed-off-by: Stefano Garzarella --- drivers/vhost/vdpa.c | 26 +++--- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 21eea2be5afa..b754c53171a7 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -191,9 +191,6 @@ static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa *v, struct vdpa_device *vdpa = v->vdpa; u32 size = vdpa->config->get_config_size(vdpa); - if (c->len == 0) - return -EINVAL; - return min(c->len, size); } @@ -204,6 +201,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v, struct vhost_vdpa_config config; unsigned long size = offsetof(struct vhost_vdpa_config, buf); ssize_t config_size; + long ret; u8 *buf; if (copy_from_user(&config, c, size)) @@ -217,15 +215,18 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v, if (!buf) return -ENOMEM; - vdpa_get_config(vdpa, config.off, buf, config_size); - - if (copy_to_user(c->buf, buf, config_size)) { - kvfree(buf); - return -EFAULT; + ret = vdpa_get_config(vdpa, config.off, buf, config_size); + if (ret < 0) { + ret = -EFAULT; + goto out; } + if (copy_to_user(c->buf, buf, config_size)) + ret = -EFAULT; + +out: kvfree(buf); - return 0; + return ret; } static long vhost_vdpa_set_config(struct vhost_vdpa *v, @@ -235,6 +236,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, struct vhost_vdpa_config config; unsigned long size = offsetof(struct vhost_vdpa_config, buf); ssize_t config_size; + long ret; u8 *buf; if (copy_from_user(&config, c, size)) @@ -248,10 +250,12 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, if (IS_ERR(buf)) return PTR_ERR(buf); - vdpa_set_config(vdpa, config.off, buf, config_size); + ret = vdpa_set_config(vdpa, config.off, buf, config_size); + if (ret < 0) + ret = -EFAULT; kvfree(buf); - return 0; + return ret; } So I wonder whether it's worth to return the number of bytes since we can't propogate the result to driver or driver doesn't care about that. Okay, but IIUC user space application that issue VHOST_VDPA_GET_CONFIG ioctl can use the return value. Should we change also 'struct virtio_config_ops' to propagate this value also to virtio drivers? Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 01/10] vdpa: add get_config_size callback in vdpa_config_ops
On Tue, Mar 02, 2021 at 12:14:13PM +0800, Jason Wang wrote: On 2021/2/16 5:44 下午, Stefano Garzarella wrote: This new callback is used to get the size of the configuration space of vDPA devices. Signed-off-by: Stefano Garzarella --- include/linux/vdpa.h | 4 drivers/vdpa/ifcvf/ifcvf_main.c | 6 ++ drivers/vdpa/mlx5/net/mlx5_vnet.c | 6 ++ drivers/vdpa/vdpa_sim/vdpa_sim.c | 9 + 4 files changed, 25 insertions(+) diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h index 4ab5494503a8..fddf42b17573 100644 --- a/include/linux/vdpa.h +++ b/include/linux/vdpa.h @@ -150,6 +150,9 @@ struct vdpa_iova_range { * @set_status:Set the device status * @vdev: vdpa device * @status: virtio device status + * @get_config_size: Get the size of the configuration space + * @vdev: vdpa device + * Returns size_t: configuration size Rethink about this, how much we could gain by introducing a dedicated ops here? E.g would it be simpler if we simply introduce a max_config_size to vdpa device? Mainly because in this way we don't have to add new parameters to the vdpa_alloc_device() function. We do the same for example for 'get_device_id', 'get_vendor_id', 'get_vq_num_max'. All of these are usually static, but we have ops. I think because it's easier to extend. I don't know if it's worth adding a new structure for these static values at this point, like 'struct vdpa_config_params'. Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 9/9] ALSA: virtio: introduce device suspend/resume support
On 02.03.2021 10:11, Takashi Iwai wrote: On Tue, 02 Mar 2021 09:09:33 +0100, Anton Yakovlev wrote: On 02.03.2021 07:48, Takashi Iwai wrote: On Tue, 02 Mar 2021 07:29:20 +0100, Anton Yakovlev wrote: On 28.02.2021 13:05, Takashi Iwai wrote: On Sat, 27 Feb 2021 09:59:56 +0100, Anton Yakovlev wrote: [snip] --- a/sound/virtio/virtio_pcm.c +++ b/sound/virtio/virtio_pcm.c @@ -109,6 +109,7 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss, SNDRV_PCM_INFO_BATCH | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_RESUME | SNDRV_PCM_INFO_PAUSE; Actually you don't need to set SNDRV_PCM_INFO_RESUME. This flag means that the driver supports the full resume procedure, which isn't often the case; with this, the driver is supposed to resume the stream exactly from the suspended position. Most drivers don't set this but implement only the suspend-stop action. Then the application (or the sound backend) will re-setup the stream and restart accordingly. I tried to resume driver without SNDRV_PCM_INFO_RESUME, and alsa-lib called only ops->prepare(). It makes sense for a typical hw, but we have "clean" unconfigured device on resume. And we must set hw parameters as a first step. It means, that code should be more or less the same. And maybe it's better to set SNDRV_PCM_INFO_RESUME, since it allows us to resume substream in any situation (regardless of application behavior). I can refactor code to only send requests from trigger(RESUME) path and not to call ops itself. It should make code more straitforward. What do you say? How about calling hw_params(NULL) conditionally in the prepare? Then the question is that condition. When ops->prepare() is called, the substream is in SUSPENDED state or not? If not then we need to track this in some additional field (and it will make logic a little bit clumsy, since that field is needed to be carefully handled in other places). Yes, you'd need to have a suspend/resume PM callback in the driver that flips the internal flag to invalidate the hw_parmas, and in the prepare callback, just call hw_params(NULL) if that flag is set. Doing the full stack work in the trigger callback is bad from the API design POV; in general the trigger callback is supposed to be as short as possible. Yeah, but usually original subsystem design does not take into account para-virtualized devices, which usually have it's own slightly different reality. And we need to introduce some tricks. The hardware drivers do a lot of more things in either suspend/resume PM callbacks or prepare callback for re-setup of the hardware. We can follow the similar pattern. Heavy-lifting works in the trigger callbacks is really something to avoid. Ok, I redone this part and now the driver sets parameters for the device in ops->prepare() if the substream was suspended. And everything works fine. Thanks! I will send a new patch set soon. Takashi -- Anton Yakovlev Senior Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v7 5/9] ALSA: virtio: handling control and I/O messages for the PCM device
The driver implements a message-based transport for I/O substream operations. Before the start of the substream, the hardware buffer is sliced into I/O messages, the number of which is equal to the current number of periods. The size of each message is equal to the current size of one period. I/O messages are organized in an ordered queue. The completion of the I/O message indicates an elapsed period (the only exception is the end of the stream for the capture substream). Upon completion, the message is automatically re-added to the end of the queue. Signed-off-by: Anton Yakovlev --- sound/virtio/Makefile | 3 +- sound/virtio/virtio_card.c| 22 +- sound/virtio/virtio_card.h| 9 + sound/virtio/virtio_pcm.c | 32 +++ sound/virtio/virtio_pcm.h | 40 sound/virtio/virtio_pcm_msg.c | 414 ++ 6 files changed, 515 insertions(+), 5 deletions(-) create mode 100644 sound/virtio/virtio_pcm_msg.c diff --git a/sound/virtio/Makefile b/sound/virtio/Makefile index 69162a545a41..626af3cc3ed7 100644 --- a/sound/virtio/Makefile +++ b/sound/virtio/Makefile @@ -5,5 +5,6 @@ obj-$(CONFIG_SND_VIRTIO) += virtio_snd.o virtio_snd-objs := \ virtio_card.o \ virtio_ctl_msg.o \ - virtio_pcm.o + virtio_pcm.o \ + virtio_pcm_msg.o diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c index 11c76ee311b7..57b9b7f3a9c0 100644 --- a/sound/virtio/virtio_card.c +++ b/sound/virtio/virtio_card.c @@ -55,6 +55,12 @@ static void virtsnd_event_send(struct virtqueue *vqueue, static void virtsnd_event_dispatch(struct virtio_snd *snd, struct virtio_snd_event *event) { + switch (le32_to_cpu(event->hdr.code)) { + case VIRTIO_SND_EVT_PCM_PERIOD_ELAPSED: + case VIRTIO_SND_EVT_PCM_XRUN: + virtsnd_pcm_event(snd, event); + break; + } } /** @@ -101,11 +107,15 @@ static int virtsnd_find_vqs(struct virtio_snd *snd) struct virtio_device *vdev = snd->vdev; static vq_callback_t *callbacks[VIRTIO_SND_VQ_MAX] = { [VIRTIO_SND_VQ_CONTROL] = virtsnd_ctl_notify_cb, - [VIRTIO_SND_VQ_EVENT] = virtsnd_event_notify_cb + [VIRTIO_SND_VQ_EVENT] = virtsnd_event_notify_cb, + [VIRTIO_SND_VQ_TX] = virtsnd_pcm_tx_notify_cb, + [VIRTIO_SND_VQ_RX] = virtsnd_pcm_rx_notify_cb }; static const char *names[VIRTIO_SND_VQ_MAX] = { [VIRTIO_SND_VQ_CONTROL] = "virtsnd-ctl", - [VIRTIO_SND_VQ_EVENT] = "virtsnd-event" + [VIRTIO_SND_VQ_EVENT] = "virtsnd-event", + [VIRTIO_SND_VQ_TX] = "virtsnd-tx", + [VIRTIO_SND_VQ_RX] = "virtsnd-rx" }; struct virtqueue *vqs[VIRTIO_SND_VQ_MAX] = { 0 }; unsigned int i; @@ -318,8 +328,12 @@ static void virtsnd_remove(struct virtio_device *vdev) vdev->config->del_vqs(vdev); vdev->config->reset(vdev); - for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) - cancel_work_sync(&snd->substreams[i].elapsed_period); + for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) { + struct virtio_pcm_substream *vss = &snd->substreams[i]; + + cancel_work_sync(&vss->elapsed_period); + virtsnd_pcm_msg_free(vss); + } kfree(snd->event_msgs); } diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h index 77a1b7255370..c43f9744d362 100644 --- a/sound/virtio/virtio_card.h +++ b/sound/virtio/virtio_card.h @@ -79,4 +79,13 @@ virtsnd_rx_queue(struct virtio_snd *snd) return &snd->queues[VIRTIO_SND_VQ_RX]; } +static inline struct virtio_snd_queue * +virtsnd_pcm_queue(struct virtio_pcm_substream *vss) +{ + if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK) + return virtsnd_tx_queue(vss->snd); + else + return virtsnd_rx_queue(vss->snd); +} + #endif /* VIRTIO_SND_CARD_H */ diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c index e16567e2e214..2dcd763efa29 100644 --- a/sound/virtio/virtio_pcm.c +++ b/sound/virtio/virtio_pcm.c @@ -353,6 +353,8 @@ int virtsnd_pcm_parse_cfg(struct virtio_snd *snd) vss->snd = snd; vss->sid = i; INIT_WORK(&vss->elapsed_period, virtsnd_pcm_period_elapsed); + init_waitqueue_head(&vss->msg_empty); + spin_lock_init(&vss->lock); rc = virtsnd_pcm_build_hw(vss, &info[i]); if (rc) @@ -477,3 +479,33 @@ int virtsnd_pcm_build_devs(struct virtio_snd *snd) return 0; } + +/** + * virtsnd_pcm_event() - Handle the PCM device event notification. + * @snd: VirtIO sound device. + * @event: VirtIO sound event. + * + * Context: Interrupt context. + */ +void virtsnd_pcm_event(struct virtio_snd *snd, struct virtio_snd_event *event) +{ + struct virtio_pcm_substream *vss; +
[PATCH v7 4/9] ALSA: virtio: build PCM devices and substream hardware descriptors
Like the HDA specification, the virtio sound device specification links PCM substreams, jacks and PCM channel maps into functional groups. For each discovered group, a PCM device is created, the number of which coincides with the group number. Introduce the module parameters for setting the hardware buffer parameters: pcm_buffer_ms [=160] pcm_periods_min [=2] pcm_periods_max [=16] pcm_period_ms_min [=10] pcm_period_ms_max [=80] Signed-off-by: Anton Yakovlev --- sound/virtio/Makefile | 3 +- sound/virtio/virtio_card.c | 18 ++ sound/virtio/virtio_card.h | 10 + sound/virtio/virtio_pcm.c | 479 + sound/virtio/virtio_pcm.h | 72 ++ 5 files changed, 581 insertions(+), 1 deletion(-) create mode 100644 sound/virtio/virtio_pcm.c create mode 100644 sound/virtio/virtio_pcm.h diff --git a/sound/virtio/Makefile b/sound/virtio/Makefile index dc551e637441..69162a545a41 100644 --- a/sound/virtio/Makefile +++ b/sound/virtio/Makefile @@ -4,5 +4,6 @@ obj-$(CONFIG_SND_VIRTIO) += virtio_snd.o virtio_snd-objs := \ virtio_card.o \ - virtio_ctl_msg.o + virtio_ctl_msg.o \ + virtio_pcm.o diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c index b757b2444078..11c76ee311b7 100644 --- a/sound/virtio/virtio_card.c +++ b/sound/virtio/virtio_card.c @@ -209,6 +209,16 @@ static int virtsnd_build_devs(struct virtio_snd *snd) VIRTIO_SND_CARD_NAME " at %s/%s", dev_name(dev->parent), dev_name(dev)); + rc = virtsnd_pcm_parse_cfg(snd); + if (rc) + return rc; + + if (snd->nsubstreams) { + rc = virtsnd_pcm_build_devs(snd); + if (rc) + return rc; + } + return snd_card_register(snd->card); } @@ -237,6 +247,9 @@ static int virtsnd_validate(struct virtio_device *vdev) return -EINVAL; } + if (virtsnd_pcm_validate(vdev)) + return -EINVAL; + return 0; } @@ -259,6 +272,7 @@ static int virtsnd_probe(struct virtio_device *vdev) snd->vdev = vdev; INIT_LIST_HEAD(&snd->ctl_msgs); + INIT_LIST_HEAD(&snd->pcm_list); vdev->priv = snd; @@ -293,6 +307,7 @@ static int virtsnd_probe(struct virtio_device *vdev) static void virtsnd_remove(struct virtio_device *vdev) { struct virtio_snd *snd = vdev->priv; + unsigned int i; virtsnd_disable_event_vq(snd); virtsnd_ctl_msg_cancel_all(snd); @@ -303,6 +318,9 @@ static void virtsnd_remove(struct virtio_device *vdev) vdev->config->del_vqs(vdev); vdev->config->reset(vdev); + for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) + cancel_work_sync(&snd->substreams[i].elapsed_period); + kfree(snd->event_msgs); } diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h index 1e76eeff160f..77a1b7255370 100644 --- a/sound/virtio/virtio_card.h +++ b/sound/virtio/virtio_card.h @@ -12,9 +12,13 @@ #include #include "virtio_ctl_msg.h" +#include "virtio_pcm.h" #define VIRTIO_SND_CARD_DRIVER "virtio-snd" #define VIRTIO_SND_CARD_NAME "VirtIO SoundCard" +#define VIRTIO_SND_PCM_NAME"VirtIO PCM" + +struct virtio_pcm_substream; /** * struct virtio_snd_queue - Virtqueue wrapper structure. @@ -33,6 +37,9 @@ struct virtio_snd_queue { * @card: ALSA sound card. * @ctl_msgs: Pending control request list. * @event_msgs: Device events. + * @pcm_list: VirtIO PCM device list. + * @substreams: VirtIO PCM substreams. + * @nsubstreams: Number of PCM substreams. */ struct virtio_snd { struct virtio_device *vdev; @@ -40,6 +47,9 @@ struct virtio_snd { struct snd_card *card; struct list_head ctl_msgs; struct virtio_snd_event *event_msgs; + struct list_head pcm_list; + struct virtio_pcm_substream *substreams; + u32 nsubstreams; }; /* Message completion timeout in milliseconds (module parameter). */ diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c new file mode 100644 index ..e16567e2e214 --- /dev/null +++ b/sound/virtio/virtio_pcm.c @@ -0,0 +1,479 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * virtio-snd: Virtio sound device + * Copyright (C) 2021 OpenSynergy GmbH + */ +#include +#include + +#include "virtio_card.h" + +static u32 pcm_buffer_ms = 160; +module_param(pcm_buffer_ms, uint, 0644); +MODULE_PARM_DESC(pcm_buffer_ms, "PCM substream buffer time in milliseconds"); + +static u32 pcm_periods_min = 2; +module_param(pcm_periods_min, uint, 0644); +MODULE_PARM_DESC(pcm_periods_min, "Minimum number of PCM periods"); + +static u32 pcm_periods_max = 16; +module_param(pcm_periods_max, uint, 0644); +MODULE_PARM_DESC(pcm_periods_max, "Maximum number of PCM periods"); + +static u32 pcm_period_ms_min = 10; +module_param(pcm_period_ms_min, uint, 0644); +MODULE_PARM_DESC(pcm_period_ms_min, "Minimum PCM peri
[PATCH v7 6/9] ALSA: virtio: PCM substream operators
Introduce the operators required for the operation of substreams. Signed-off-by: Anton Yakovlev --- sound/virtio/Makefile | 3 +- sound/virtio/virtio_pcm.c | 2 + sound/virtio/virtio_pcm.h | 5 + sound/virtio/virtio_pcm_ops.c | 445 ++ 4 files changed, 454 insertions(+), 1 deletion(-) create mode 100644 sound/virtio/virtio_pcm_ops.c diff --git a/sound/virtio/Makefile b/sound/virtio/Makefile index 626af3cc3ed7..34493226793f 100644 --- a/sound/virtio/Makefile +++ b/sound/virtio/Makefile @@ -6,5 +6,6 @@ virtio_snd-objs := \ virtio_card.o \ virtio_ctl_msg.o \ virtio_pcm.o \ - virtio_pcm_msg.o + virtio_pcm_msg.o \ + virtio_pcm_ops.o diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c index 2dcd763efa29..c10d91fff2fb 100644 --- a/sound/virtio/virtio_pcm.c +++ b/sound/virtio/virtio_pcm.c @@ -470,6 +470,8 @@ int virtsnd_pcm_build_devs(struct virtio_snd *snd) for (kss = ks->substream; kss; kss = kss->next) vs->substreams[kss->number]->substream = kss; + + snd_pcm_set_ops(vpcm->pcm, i, &virtsnd_pcm_ops); } snd_pcm_set_managed_buffer_all(vpcm->pcm, diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h index 6722f1139666..efd0228746cf 100644 --- a/sound/virtio/virtio_pcm.h +++ b/sound/virtio/virtio_pcm.h @@ -29,6 +29,8 @@ struct virtio_pcm_msg; * @hw_ptr: Substream hardware pointer value in bytes [0 ... buffer_bytes). * @xfer_enabled: Data transfer state (0 - off, 1 - on). * @xfer_xrun: Data underflow/overflow state (0 - no xrun, 1 - xrun). + * @stopped: True if the substream is stopped and must be released on the device + * side. * @msgs: Allocated I/O messages. * @nmsgs: Number of allocated I/O messages. * @msg_last_enqueued: Index of the last I/O message added to the virtqueue. @@ -49,6 +51,7 @@ struct virtio_pcm_substream { size_t hw_ptr; bool xfer_enabled; bool xfer_xrun; + bool stopped; struct virtio_pcm_msg **msgs; unsigned int nmsgs; int msg_last_enqueued; @@ -80,6 +83,8 @@ struct virtio_pcm { struct virtio_pcm_stream streams[SNDRV_PCM_STREAM_LAST + 1]; }; +extern const struct snd_pcm_ops virtsnd_pcm_ops; + int virtsnd_pcm_validate(struct virtio_device *vdev); int virtsnd_pcm_parse_cfg(struct virtio_snd *snd); diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c new file mode 100644 index ..0682a2df6c8c --- /dev/null +++ b/sound/virtio/virtio_pcm_ops.c @@ -0,0 +1,445 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * virtio-snd: Virtio sound device + * Copyright (C) 2021 OpenSynergy GmbH + */ +#include + +#include "virtio_card.h" + +/* + * I/O messages lifetime + * - + * + * Allocation: + * Messages are initially allocated in the ops->hw_params() after the size and + * number of periods have been successfully negotiated. + * + * Freeing: + * Messages can be safely freed after the queue has been successfully flushed + * (RELEASE command in the ops->sync_stop()) and the ops->hw_free() has been + * called. + * + * When the substream stops, the ops->sync_stop() waits until the device has + * completed all pending messages. This wait can be interrupted either by a + * signal or due to a timeout. In this case, the device can still access + * messages even after calling ops->hw_free(). It can also issue an interrupt, + * and the interrupt handler will also try to access message structures. + * + * Therefore, freeing of already allocated messages occurs: + * + * - in ops->hw_params(), if this operator was called several times in a row, + * or if ops->hw_free() failed to free messages previously; + * + * - in ops->hw_free(), if the queue has been successfully flushed; + * + * - in dev->release(). + */ + +/* Map for converting ALSA format to VirtIO format. */ +struct virtsnd_a2v_format { + snd_pcm_format_t alsa_bit; + unsigned int vio_bit; +}; + +static const struct virtsnd_a2v_format g_a2v_format_map[] = { + { SNDRV_PCM_FORMAT_IMA_ADPCM, VIRTIO_SND_PCM_FMT_IMA_ADPCM }, + { SNDRV_PCM_FORMAT_MU_LAW, VIRTIO_SND_PCM_FMT_MU_LAW }, + { SNDRV_PCM_FORMAT_A_LAW, VIRTIO_SND_PCM_FMT_A_LAW }, + { SNDRV_PCM_FORMAT_S8, VIRTIO_SND_PCM_FMT_S8 }, + { SNDRV_PCM_FORMAT_U8, VIRTIO_SND_PCM_FMT_U8 }, + { SNDRV_PCM_FORMAT_S16_LE, VIRTIO_SND_PCM_FMT_S16 }, + { SNDRV_PCM_FORMAT_U16_LE, VIRTIO_SND_PCM_FMT_U16 }, + { SNDRV_PCM_FORMAT_S18_3LE, VIRTIO_SND_PCM_FMT_S18_3 }, + { SNDRV_PCM_FORMAT_U18_3LE, VIRTIO_SND_PCM_FMT_U18_3 }, + { SNDRV_PCM_FORMAT_S20_3LE, VIRTIO_SND_PCM_FMT_S20_3 }, + { SNDRV_PCM_FORMAT_U20_3LE, VIRTIO_SND_PCM_FMT_U20_3 }, + { SNDRV_PCM_FORMAT_S24_3LE, VIRTIO_SND_PCM_FMT_S24_3 }, + { SNDRV_PCM_FORMAT_U24_3LE, VIRTIO_SND_PCM
[PATCH v7 7/9] ALSA: virtio: introduce jack support
Enumerate all available jacks and create ALSA controls. At the moment jacks have a simple implementation and can only be used to receive notifications about a plugged in/out device. Signed-off-by: Anton Yakovlev --- sound/virtio/Makefile | 1 + sound/virtio/virtio_card.c | 14 +++ sound/virtio/virtio_card.h | 12 ++ sound/virtio/virtio_jack.c | 233 + 4 files changed, 260 insertions(+) create mode 100644 sound/virtio/virtio_jack.c diff --git a/sound/virtio/Makefile b/sound/virtio/Makefile index 34493226793f..09f485291285 100644 --- a/sound/virtio/Makefile +++ b/sound/virtio/Makefile @@ -5,6 +5,7 @@ obj-$(CONFIG_SND_VIRTIO) += virtio_snd.o virtio_snd-objs := \ virtio_card.o \ virtio_ctl_msg.o \ + virtio_jack.o \ virtio_pcm.o \ virtio_pcm_msg.o \ virtio_pcm_ops.o diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c index 57b9b7f3a9c0..89bd66c1256e 100644 --- a/sound/virtio/virtio_card.c +++ b/sound/virtio/virtio_card.c @@ -56,6 +56,10 @@ static void virtsnd_event_dispatch(struct virtio_snd *snd, struct virtio_snd_event *event) { switch (le32_to_cpu(event->hdr.code)) { + case VIRTIO_SND_EVT_JACK_CONNECTED: + case VIRTIO_SND_EVT_JACK_DISCONNECTED: + virtsnd_jack_event(snd, event); + break; case VIRTIO_SND_EVT_PCM_PERIOD_ELAPSED: case VIRTIO_SND_EVT_PCM_XRUN: virtsnd_pcm_event(snd, event); @@ -219,10 +223,20 @@ static int virtsnd_build_devs(struct virtio_snd *snd) VIRTIO_SND_CARD_NAME " at %s/%s", dev_name(dev->parent), dev_name(dev)); + rc = virtsnd_jack_parse_cfg(snd); + if (rc) + return rc; + rc = virtsnd_pcm_parse_cfg(snd); if (rc) return rc; + if (snd->njacks) { + rc = virtsnd_jack_build_devs(snd); + if (rc) + return rc; + } + if (snd->nsubstreams) { rc = virtsnd_pcm_build_devs(snd); if (rc) diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h index c43f9744d362..f154313c79fd 100644 --- a/sound/virtio/virtio_card.h +++ b/sound/virtio/virtio_card.h @@ -18,6 +18,7 @@ #define VIRTIO_SND_CARD_NAME "VirtIO SoundCard" #define VIRTIO_SND_PCM_NAME"VirtIO PCM" +struct virtio_jack; struct virtio_pcm_substream; /** @@ -38,6 +39,8 @@ struct virtio_snd_queue { * @ctl_msgs: Pending control request list. * @event_msgs: Device events. * @pcm_list: VirtIO PCM device list. + * @jacks: VirtIO jacks. + * @njacks: Number of jacks. * @substreams: VirtIO PCM substreams. * @nsubstreams: Number of PCM substreams. */ @@ -48,6 +51,8 @@ struct virtio_snd { struct list_head ctl_msgs; struct virtio_snd_event *event_msgs; struct list_head pcm_list; + struct virtio_jack *jacks; + u32 njacks; struct virtio_pcm_substream *substreams; u32 nsubstreams; }; @@ -88,4 +93,11 @@ virtsnd_pcm_queue(struct virtio_pcm_substream *vss) return virtsnd_rx_queue(vss->snd); } +int virtsnd_jack_parse_cfg(struct virtio_snd *snd); + +int virtsnd_jack_build_devs(struct virtio_snd *snd); + +void virtsnd_jack_event(struct virtio_snd *snd, + struct virtio_snd_event *event); + #endif /* VIRTIO_SND_CARD_H */ diff --git a/sound/virtio/virtio_jack.c b/sound/virtio/virtio_jack.c new file mode 100644 index ..c69f1dcdcc84 --- /dev/null +++ b/sound/virtio/virtio_jack.c @@ -0,0 +1,233 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * virtio-snd: Virtio sound device + * Copyright (C) 2021 OpenSynergy GmbH + */ +#include +#include +#include + +#include "virtio_card.h" + +/** + * DOC: Implementation Status + * + * At the moment jacks have a simple implementation and can only be used to + * receive notifications about a plugged in/out device. + * + * VIRTIO_SND_R_JACK_REMAP + * is not supported + */ + +/** + * struct virtio_jack - VirtIO jack. + * @jack: Kernel jack control. + * @nid: Functional group node identifier. + * @features: Jack virtio feature bit map (1 << VIRTIO_SND_JACK_F_XXX). + * @defconf: Pin default configuration value. + * @caps: Pin capabilities value. + * @connected: Current jack connection status. + * @type: Kernel jack type (SND_JACK_XXX). + */ +struct virtio_jack { + struct snd_jack *jack; + u32 nid; + u32 features; + u32 defconf; + u32 caps; + bool connected; + int type; +}; + +/** + * virtsnd_jack_get_label() - Get the name string for the jack. + * @vjack: VirtIO jack. + * + * Returns the jack name based on the default pin configuration value (see HDA + * specification). + * + * Context: Any context. + * Return: Name string. + */ +static const char *virtsnd_jack_get_label(struct virtio_jack *vjack) +{ + unsigned int def
[PATCH v7 8/9] ALSA: virtio: introduce PCM channel map support
Enumerate all available PCM channel maps and create ALSA controls. Signed-off-by: Anton Yakovlev --- sound/virtio/Makefile | 1 + sound/virtio/virtio_card.c | 10 ++ sound/virtio/virtio_card.h | 8 ++ sound/virtio/virtio_chmap.c | 219 sound/virtio/virtio_pcm.h | 4 + 5 files changed, 242 insertions(+) create mode 100644 sound/virtio/virtio_chmap.c diff --git a/sound/virtio/Makefile b/sound/virtio/Makefile index 09f485291285..2742bddb8874 100644 --- a/sound/virtio/Makefile +++ b/sound/virtio/Makefile @@ -4,6 +4,7 @@ obj-$(CONFIG_SND_VIRTIO) += virtio_snd.o virtio_snd-objs := \ virtio_card.o \ + virtio_chmap.o \ virtio_ctl_msg.o \ virtio_jack.o \ virtio_pcm.o \ diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c index 89bd66c1256e..1c03fcc41c3b 100644 --- a/sound/virtio/virtio_card.c +++ b/sound/virtio/virtio_card.c @@ -231,6 +231,10 @@ static int virtsnd_build_devs(struct virtio_snd *snd) if (rc) return rc; + rc = virtsnd_chmap_parse_cfg(snd); + if (rc) + return rc; + if (snd->njacks) { rc = virtsnd_jack_build_devs(snd); if (rc) @@ -243,6 +247,12 @@ static int virtsnd_build_devs(struct virtio_snd *snd) return rc; } + if (snd->nchmaps) { + rc = virtsnd_chmap_build_devs(snd); + if (rc) + return rc; + } + return snd_card_register(snd->card); } diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h index f154313c79fd..86ef3941895e 100644 --- a/sound/virtio/virtio_card.h +++ b/sound/virtio/virtio_card.h @@ -43,6 +43,8 @@ struct virtio_snd_queue { * @njacks: Number of jacks. * @substreams: VirtIO PCM substreams. * @nsubstreams: Number of PCM substreams. + * @chmaps: VirtIO channel maps. + * @nchmaps: Number of channel maps. */ struct virtio_snd { struct virtio_device *vdev; @@ -55,6 +57,8 @@ struct virtio_snd { u32 njacks; struct virtio_pcm_substream *substreams; u32 nsubstreams; + struct virtio_snd_chmap_info *chmaps; + u32 nchmaps; }; /* Message completion timeout in milliseconds (module parameter). */ @@ -100,4 +104,8 @@ int virtsnd_jack_build_devs(struct virtio_snd *snd); void virtsnd_jack_event(struct virtio_snd *snd, struct virtio_snd_event *event); +int virtsnd_chmap_parse_cfg(struct virtio_snd *snd); + +int virtsnd_chmap_build_devs(struct virtio_snd *snd); + #endif /* VIRTIO_SND_CARD_H */ diff --git a/sound/virtio/virtio_chmap.c b/sound/virtio/virtio_chmap.c new file mode 100644 index ..5bc924933a59 --- /dev/null +++ b/sound/virtio/virtio_chmap.c @@ -0,0 +1,219 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * virtio-snd: Virtio sound device + * Copyright (C) 2021 OpenSynergy GmbH + */ +#include + +#include "virtio_card.h" + +/* VirtIO->ALSA channel position map */ +static const u8 g_v2a_position_map[] = { + [VIRTIO_SND_CHMAP_NONE] = SNDRV_CHMAP_UNKNOWN, + [VIRTIO_SND_CHMAP_NA] = SNDRV_CHMAP_NA, + [VIRTIO_SND_CHMAP_MONO] = SNDRV_CHMAP_MONO, + [VIRTIO_SND_CHMAP_FL] = SNDRV_CHMAP_FL, + [VIRTIO_SND_CHMAP_FR] = SNDRV_CHMAP_FR, + [VIRTIO_SND_CHMAP_RL] = SNDRV_CHMAP_RL, + [VIRTIO_SND_CHMAP_RR] = SNDRV_CHMAP_RR, + [VIRTIO_SND_CHMAP_FC] = SNDRV_CHMAP_FC, + [VIRTIO_SND_CHMAP_LFE] = SNDRV_CHMAP_LFE, + [VIRTIO_SND_CHMAP_SL] = SNDRV_CHMAP_SL, + [VIRTIO_SND_CHMAP_SR] = SNDRV_CHMAP_SR, + [VIRTIO_SND_CHMAP_RC] = SNDRV_CHMAP_RC, + [VIRTIO_SND_CHMAP_FLC] = SNDRV_CHMAP_FLC, + [VIRTIO_SND_CHMAP_FRC] = SNDRV_CHMAP_FRC, + [VIRTIO_SND_CHMAP_RLC] = SNDRV_CHMAP_RLC, + [VIRTIO_SND_CHMAP_RRC] = SNDRV_CHMAP_RRC, + [VIRTIO_SND_CHMAP_FLW] = SNDRV_CHMAP_FLW, + [VIRTIO_SND_CHMAP_FRW] = SNDRV_CHMAP_FRW, + [VIRTIO_SND_CHMAP_FLH] = SNDRV_CHMAP_FLH, + [VIRTIO_SND_CHMAP_FCH] = SNDRV_CHMAP_FCH, + [VIRTIO_SND_CHMAP_FRH] = SNDRV_CHMAP_FRH, + [VIRTIO_SND_CHMAP_TC] = SNDRV_CHMAP_TC, + [VIRTIO_SND_CHMAP_TFL] = SNDRV_CHMAP_TFL, + [VIRTIO_SND_CHMAP_TFR] = SNDRV_CHMAP_TFR, + [VIRTIO_SND_CHMAP_TFC] = SNDRV_CHMAP_TFC, + [VIRTIO_SND_CHMAP_TRL] = SNDRV_CHMAP_TRL, + [VIRTIO_SND_CHMAP_TRR] = SNDRV_CHMAP_TRR, + [VIRTIO_SND_CHMAP_TRC] = SNDRV_CHMAP_TRC, + [VIRTIO_SND_CHMAP_TFLC] = SNDRV_CHMAP_TFLC, + [VIRTIO_SND_CHMAP_TFRC] = SNDRV_CHMAP_TFRC, + [VIRTIO_SND_CHMAP_TSL] = SNDRV_CHMAP_TSL, + [VIRTIO_SND_CHMAP_TSR] = SNDRV_CHMAP_TSR, + [VIRTIO_SND_CHMAP_LLFE] = SNDRV_CHMAP_LLFE, + [VIRTIO_SND_CHMAP_RLFE] = SNDRV_CHMAP_RLFE, + [VIRTIO_SND_CHMAP_BC] = SNDRV_CHMAP_BC, + [VIRTIO_SND_CHMAP_BLC] = SNDRV_CHMAP_BLC, + [VIRTIO_SND_CHMAP_BRC] = SNDRV_CHMAP_BRC +}; + +/** + * virtsnd_chmap_parse_cfg() - Parse the channel map configura
[PATCH v7 9/9] ALSA: virtio: introduce device suspend/resume support
All running PCM substreams are stopped on device suspend and restarted on device resume. Signed-off-by: Anton Yakovlev --- sound/virtio/virtio_card.c| 56 +++ sound/virtio/virtio_pcm.h | 3 ++ sound/virtio/virtio_pcm_ops.c | 33 - 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c index 1c03fcc41c3b..ae9128063917 100644 --- a/sound/virtio/virtio_card.c +++ b/sound/virtio/virtio_card.c @@ -362,6 +362,58 @@ static void virtsnd_remove(struct virtio_device *vdev) kfree(snd->event_msgs); } +#ifdef CONFIG_PM_SLEEP +/** + * virtsnd_freeze() - Suspend device. + * @vdev: VirtIO parent device. + * + * Context: Any context. + * Return: 0 on success, -errno on failure. + */ +static int virtsnd_freeze(struct virtio_device *vdev) +{ + struct virtio_snd *snd = vdev->priv; + unsigned int i; + + virtsnd_disable_event_vq(snd); + virtsnd_ctl_msg_cancel_all(snd); + + vdev->config->del_vqs(vdev); + vdev->config->reset(vdev); + + for (i = 0; i < snd->nsubstreams; ++i) + cancel_work_sync(&snd->substreams[i].elapsed_period); + + kfree(snd->event_msgs); + snd->event_msgs = NULL; + + return 0; +} + +/** + * virtsnd_restore() - Resume device. + * @vdev: VirtIO parent device. + * + * Context: Any context. + * Return: 0 on success, -errno on failure. + */ +static int virtsnd_restore(struct virtio_device *vdev) +{ + struct virtio_snd *snd = vdev->priv; + int rc; + + rc = virtsnd_find_vqs(snd); + if (rc) + return rc; + + virtio_device_ready(vdev); + + virtsnd_enable_event_vq(snd); + + return 0; +} +#endif /* CONFIG_PM_SLEEP */ + static const struct virtio_device_id id_table[] = { { VIRTIO_ID_SOUND, VIRTIO_DEV_ANY_ID }, { 0 }, @@ -374,6 +426,10 @@ static struct virtio_driver virtsnd_driver = { .validate = virtsnd_validate, .probe = virtsnd_probe, .remove = virtsnd_remove, +#ifdef CONFIG_PM_SLEEP + .freeze = virtsnd_freeze, + .restore = virtsnd_restore, +#endif }; static int __init init(void) diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h index 1353fdc9bd69..062eb8e8f2cf 100644 --- a/sound/virtio/virtio_pcm.h +++ b/sound/virtio/virtio_pcm.h @@ -31,6 +31,8 @@ struct virtio_pcm_msg; * @xfer_xrun: Data underflow/overflow state (0 - no xrun, 1 - xrun). * @stopped: True if the substream is stopped and must be released on the device * side. + * @suspended: True if the substream is suspended and must be reconfigured on + * the device side at resume. * @msgs: Allocated I/O messages. * @nmsgs: Number of allocated I/O messages. * @msg_last_enqueued: Index of the last I/O message added to the virtqueue. @@ -52,6 +54,7 @@ struct virtio_pcm_substream { bool xfer_enabled; bool xfer_xrun; bool stopped; + bool suspended; struct virtio_pcm_msg **msgs; unsigned int nmsgs; int msg_last_enqueued; diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c index 0682a2df6c8c..f8bfb87624be 100644 --- a/sound/virtio/virtio_pcm_ops.c +++ b/sound/virtio/virtio_pcm_ops.c @@ -115,6 +115,7 @@ static int virtsnd_pcm_open(struct snd_pcm_substream *substream) SNDRV_PCM_HW_PARAM_PERIODS); vss->stopped = !!virtsnd_pcm_msg_pending_num(vss); + vss->suspended = false; /* * If the substream has already been used, then the I/O queue may be in @@ -272,16 +273,31 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream) struct virtio_device *vdev = vss->snd->vdev; struct virtio_snd_msg *msg; - if (virtsnd_pcm_msg_pending_num(vss)) { - dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n", - vss->sid); - return -EBADFD; + if (!vss->suspended) { + if (virtsnd_pcm_msg_pending_num(vss)) { + dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n", + vss->sid); + return -EBADFD; + } + + vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream); + vss->hw_ptr = 0; + vss->msg_last_enqueued = -1; + } else { + struct snd_pcm_runtime *runtime = substream->runtime; + unsigned int buffer_bytes = snd_pcm_lib_buffer_bytes(substream); + unsigned int period_bytes = snd_pcm_lib_period_bytes(substream); + int rc; + + rc = virtsnd_pcm_dev_set_params(vss, buffer_bytes, period_bytes, + runtime->channels, + runtime->format, runtime->rate); + if (rc) +
[PATCH v7 0/9] ALSA: add virtio sound driver
This series implements a driver part of the virtio sound device specification v8 [1]. The driver supports PCM playback and capture substreams, jack and channel map controls. A message-based transport is used to write/read PCM frames to/from a device. As a device part was used OpenSynergy proprietary implementation. v7 changes: - Moved the snd_pcm_period_elapsed() call from the interrupt handler to the kernel worker for being consistent with the non-atomic mode of the PCM device. - Removed SNDRV_PCM_INFO_RESUME flag. Now ops->prepare() sets the parameters for the substream if it was previously suspended. - Some additional code readability improvements/comments. [1] https://lists.oasis-open.org/archives/virtio-dev/202003/msg00185.html Anton Yakovlev (9): uapi: virtio_ids: add a sound device type ID from OASIS spec ALSA: virtio: add virtio sound driver ALSA: virtio: handling control messages ALSA: virtio: build PCM devices and substream hardware descriptors ALSA: virtio: handling control and I/O messages for the PCM device ALSA: virtio: PCM substream operators ALSA: virtio: introduce jack support ALSA: virtio: introduce PCM channel map support ALSA: virtio: introduce device suspend/resume support MAINTAINERS | 9 + include/uapi/linux/virtio_ids.h | 1 + include/uapi/linux/virtio_snd.h | 334 + sound/Kconfig | 2 + sound/Makefile | 3 +- sound/virtio/Kconfig| 10 + sound/virtio/Makefile | 13 + sound/virtio/virtio_card.c | 449 sound/virtio/virtio_card.h | 111 +++ sound/virtio/virtio_chmap.c | 219 ++ sound/virtio/virtio_ctl_msg.c | 310 +++ sound/virtio/virtio_ctl_msg.h | 78 + sound/virtio/virtio_jack.c | 233 +++ sound/virtio/virtio_pcm.c | 513 sound/virtio/virtio_pcm.h | 124 sound/virtio/virtio_pcm_msg.c | 414 ++ sound/virtio/virtio_pcm_ops.c | 464 + 17 files changed, 3286 insertions(+), 1 deletion(-) create mode 100644 include/uapi/linux/virtio_snd.h create mode 100644 sound/virtio/Kconfig create mode 100644 sound/virtio/Makefile create mode 100644 sound/virtio/virtio_card.c create mode 100644 sound/virtio/virtio_card.h create mode 100644 sound/virtio/virtio_chmap.c create mode 100644 sound/virtio/virtio_ctl_msg.c create mode 100644 sound/virtio/virtio_ctl_msg.h create mode 100644 sound/virtio/virtio_jack.c create mode 100644 sound/virtio/virtio_pcm.c create mode 100644 sound/virtio/virtio_pcm.h create mode 100644 sound/virtio/virtio_pcm_msg.c create mode 100644 sound/virtio/virtio_pcm_ops.c -- 2.30.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v7 3/9] ALSA: virtio: handling control messages
The control queue can be used by different parts of the driver to send commands to the device. Control messages can be either synchronous or asynchronous. The lifetime of a message is controlled by a reference count. Introduce a module parameter to set the message completion timeout: msg_timeout_ms [=1000] Signed-off-by: Anton Yakovlev --- sound/virtio/Makefile | 3 +- sound/virtio/virtio_card.c| 13 ++ sound/virtio/virtio_card.h| 7 + sound/virtio/virtio_ctl_msg.c | 310 ++ sound/virtio/virtio_ctl_msg.h | 78 + 5 files changed, 410 insertions(+), 1 deletion(-) create mode 100644 sound/virtio/virtio_ctl_msg.c create mode 100644 sound/virtio/virtio_ctl_msg.h diff --git a/sound/virtio/Makefile b/sound/virtio/Makefile index 8c87ebb9982b..dc551e637441 100644 --- a/sound/virtio/Makefile +++ b/sound/virtio/Makefile @@ -3,5 +3,6 @@ obj-$(CONFIG_SND_VIRTIO) += virtio_snd.o virtio_snd-objs := \ - virtio_card.o + virtio_card.o \ + virtio_ctl_msg.o diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c index 5a37056858e9..b757b2444078 100644 --- a/sound/virtio/virtio_card.c +++ b/sound/virtio/virtio_card.c @@ -11,6 +11,10 @@ #include "virtio_card.h" +u32 virtsnd_msg_timeout_ms = MSEC_PER_SEC; +module_param_named(msg_timeout_ms, virtsnd_msg_timeout_ms, uint, 0644); +MODULE_PARM_DESC(msg_timeout_ms, "Message completion timeout in milliseconds"); + static void virtsnd_remove(struct virtio_device *vdev); /** @@ -96,9 +100,11 @@ static int virtsnd_find_vqs(struct virtio_snd *snd) { struct virtio_device *vdev = snd->vdev; static vq_callback_t *callbacks[VIRTIO_SND_VQ_MAX] = { + [VIRTIO_SND_VQ_CONTROL] = virtsnd_ctl_notify_cb, [VIRTIO_SND_VQ_EVENT] = virtsnd_event_notify_cb }; static const char *names[VIRTIO_SND_VQ_MAX] = { + [VIRTIO_SND_VQ_CONTROL] = "virtsnd-ctl", [VIRTIO_SND_VQ_EVENT] = "virtsnd-event" }; struct virtqueue *vqs[VIRTIO_SND_VQ_MAX] = { 0 }; @@ -226,6 +232,11 @@ static int virtsnd_validate(struct virtio_device *vdev) return -EINVAL; } + if (!virtsnd_msg_timeout_ms) { + dev_err(&vdev->dev, "msg_timeout_ms value cannot be zero\n"); + return -EINVAL; + } + return 0; } @@ -247,6 +258,7 @@ static int virtsnd_probe(struct virtio_device *vdev) return -ENOMEM; snd->vdev = vdev; + INIT_LIST_HEAD(&snd->ctl_msgs); vdev->priv = snd; @@ -283,6 +295,7 @@ static void virtsnd_remove(struct virtio_device *vdev) struct virtio_snd *snd = vdev->priv; virtsnd_disable_event_vq(snd); + virtsnd_ctl_msg_cancel_all(snd); if (snd->card) snd_card_free(snd->card); diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h index b903b1b12e90..1e76eeff160f 100644 --- a/sound/virtio/virtio_card.h +++ b/sound/virtio/virtio_card.h @@ -11,6 +11,8 @@ #include #include +#include "virtio_ctl_msg.h" + #define VIRTIO_SND_CARD_DRIVER "virtio-snd" #define VIRTIO_SND_CARD_NAME "VirtIO SoundCard" @@ -29,15 +31,20 @@ struct virtio_snd_queue { * @vdev: Underlying virtio device. * @queues: Virtqueue wrappers. * @card: ALSA sound card. + * @ctl_msgs: Pending control request list. * @event_msgs: Device events. */ struct virtio_snd { struct virtio_device *vdev; struct virtio_snd_queue queues[VIRTIO_SND_VQ_MAX]; struct snd_card *card; + struct list_head ctl_msgs; struct virtio_snd_event *event_msgs; }; +/* Message completion timeout in milliseconds (module parameter). */ +extern u32 virtsnd_msg_timeout_ms; + static inline struct virtio_snd_queue * virtsnd_control_queue(struct virtio_snd *snd) { diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c new file mode 100644 index ..26ff7e7cc041 --- /dev/null +++ b/sound/virtio/virtio_ctl_msg.c @@ -0,0 +1,310 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * virtio-snd: Virtio sound device + * Copyright (C) 2021 OpenSynergy GmbH + */ +#include +#include + +#include "virtio_card.h" + +/** + * struct virtio_snd_msg - Control message. + * @sg_request: Scattergather list containing a device request (header). + * @sg_response: Scattergather list containing a device response (status). + * @list: Pending message list entry. + * @notify: Request completed notification. + * @ref_count: Reference count used to manage a message lifetime. + */ +struct virtio_snd_msg { + struct scatterlist sg_request; + struct scatterlist sg_response; + struct list_head list; + struct completion notify; + refcount_t ref_count; +}; + +/** + * virtsnd_ctl_msg_ref() - Increment reference counter for the message. + * @msg: Control message. + * + * Context: Any context. + */ +void virtsnd_ctl_msg_ref(struct virtio_snd_ms
[PATCH v7 2/9] ALSA: virtio: add virtio sound driver
Introduce skeleton of the virtio sound driver. The driver implements the virtio sound device specification, which has become part of the virtio standard. Initial initialization of the device, virtqueues and creation of an empty ALSA sound device. Signed-off-by: Anton Yakovlev --- MAINTAINERS | 9 + include/uapi/linux/virtio_snd.h | 334 sound/Kconfig | 2 + sound/Makefile | 3 +- sound/virtio/Kconfig| 10 + sound/virtio/Makefile | 7 + sound/virtio/virtio_card.c | 324 +++ sound/virtio/virtio_card.h | 65 +++ 8 files changed, 753 insertions(+), 1 deletion(-) create mode 100644 include/uapi/linux/virtio_snd.h create mode 100644 sound/virtio/Kconfig create mode 100644 sound/virtio/Makefile create mode 100644 sound/virtio/virtio_card.c create mode 100644 sound/virtio/virtio_card.h diff --git a/MAINTAINERS b/MAINTAINERS index c71664ca8bfd..4369946434ad 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19049,6 +19049,15 @@ W: https://virtio-mem.gitlab.io/ F: drivers/virtio/virtio_mem.c F: include/uapi/linux/virtio_mem.h +VIRTIO SOUND DRIVER +M: Anton Yakovlev +M: "Michael S. Tsirkin" +L: virtualization@lists.linux-foundation.org +L: alsa-de...@alsa-project.org (moderated for non-subscribers) +S: Maintained +F: include/uapi/linux/virtio_snd.h +F: sound/virtio/* + VIRTUAL BOX GUEST DEVICE DRIVER M: Hans de Goede M: Arnd Bergmann diff --git a/include/uapi/linux/virtio_snd.h b/include/uapi/linux/virtio_snd.h new file mode 100644 index ..dfe49547a7b0 --- /dev/null +++ b/include/uapi/linux/virtio_snd.h @@ -0,0 +1,334 @@ +/* SPDX-License-Identifier: BSD-3-Clause */ +/* + * Copyright (C) 2021 OpenSynergy GmbH + */ +#ifndef VIRTIO_SND_IF_H +#define VIRTIO_SND_IF_H + +#include + +/*** + * CONFIGURATION SPACE + */ +struct virtio_snd_config { + /* # of available physical jacks */ + __le32 jacks; + /* # of available PCM streams */ + __le32 streams; + /* # of available channel maps */ + __le32 chmaps; +}; + +enum { + /* device virtqueue indexes */ + VIRTIO_SND_VQ_CONTROL = 0, + VIRTIO_SND_VQ_EVENT, + VIRTIO_SND_VQ_TX, + VIRTIO_SND_VQ_RX, + /* # of device virtqueues */ + VIRTIO_SND_VQ_MAX +}; + +/*** + * COMMON DEFINITIONS + */ + +/* supported dataflow directions */ +enum { + VIRTIO_SND_D_OUTPUT = 0, + VIRTIO_SND_D_INPUT +}; + +enum { + /* jack control request types */ + VIRTIO_SND_R_JACK_INFO = 1, + VIRTIO_SND_R_JACK_REMAP, + + /* PCM control request types */ + VIRTIO_SND_R_PCM_INFO = 0x0100, + VIRTIO_SND_R_PCM_SET_PARAMS, + VIRTIO_SND_R_PCM_PREPARE, + VIRTIO_SND_R_PCM_RELEASE, + VIRTIO_SND_R_PCM_START, + VIRTIO_SND_R_PCM_STOP, + + /* channel map control request types */ + VIRTIO_SND_R_CHMAP_INFO = 0x0200, + + /* jack event types */ + VIRTIO_SND_EVT_JACK_CONNECTED = 0x1000, + VIRTIO_SND_EVT_JACK_DISCONNECTED, + + /* PCM event types */ + VIRTIO_SND_EVT_PCM_PERIOD_ELAPSED = 0x1100, + VIRTIO_SND_EVT_PCM_XRUN, + + /* common status codes */ + VIRTIO_SND_S_OK = 0x8000, + VIRTIO_SND_S_BAD_MSG, + VIRTIO_SND_S_NOT_SUPP, + VIRTIO_SND_S_IO_ERR +}; + +/* common header */ +struct virtio_snd_hdr { + __le32 code; +}; + +/* event notification */ +struct virtio_snd_event { + /* VIRTIO_SND_EVT_XXX */ + struct virtio_snd_hdr hdr; + /* optional event data */ + __le32 data; +}; + +/* common control request to query an item information */ +struct virtio_snd_query_info { + /* VIRTIO_SND_R_XXX_INFO */ + struct virtio_snd_hdr hdr; + /* item start identifier */ + __le32 start_id; + /* item count to query */ + __le32 count; + /* item information size in bytes */ + __le32 size; +}; + +/* common item information header */ +struct virtio_snd_info { + /* function group node id (High Definition Audio Specification 7.1.2) */ + __le32 hda_fn_nid; +}; + +/*** + * JACK CONTROL MESSAGES + */ +struct virtio_snd_jack_hdr { + /* VIRTIO_SND_R_JACK_XXX */ + struct virtio_snd_hdr hdr; + /* 0 ... virtio_snd_config::jacks - 1 */ + __le32 jack_id; +}; + +/* supported jack features */ +enum { + VIRTIO_SND_JACK_F_REMAP = 0 +}; + +struct virtio_snd_jack_info { + /* common header */ + struct virtio_snd_info hdr; + /* supported feature bit map (1 << VIRTIO_SND_JACK_F_XXX) */ + __le32 features; + /* pin configuration (High Definition Audio Speci
[PATCH v7 1/9] uapi: virtio_ids: add a sound device type ID from OASIS spec
The OASIS virtio spec defines a sound device type ID that is not present in the header yet. Signed-off-by: Anton Yakovlev --- include/uapi/linux/virtio_ids.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h index bc1c0621f5ed..029a2e07a7f9 100644 --- a/include/uapi/linux/virtio_ids.h +++ b/include/uapi/linux/virtio_ids.h @@ -51,6 +51,7 @@ #define VIRTIO_ID_PSTORE 22 /* virtio pstore device */ #define VIRTIO_ID_IOMMU23 /* virtio IOMMU */ #define VIRTIO_ID_MEM 24 /* virtio mem */ +#define VIRTIO_ID_SOUND25 /* virtio sound */ #define VIRTIO_ID_FS 26 /* virtio filesystem */ #define VIRTIO_ID_PMEM 27 /* virtio pmem */ #define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */ -- 2.30.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/7] x86/boot/compressed/64: Check SEV encryption in 32-bit boot-path
On Wed, Feb 10, 2021 at 11:21:34AM +0100, Joerg Roedel wrote: > + /* > + * Store the sme_me_mask as an indicator that SEV is active. It will be > + * set again in startup_64(). So why bother? Or does something needs it before that? ... > +SYM_FUNC_START(sev_startup32_cbit_check) s/sev_startup32_cbit_check/startup32_check_sev_cbit/ I guess. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v5 19/19] virtio/vsock: update trace event for SEQPACKET
On Thu, 18 Feb 2021 08:42:15 +0300 Arseny Krasnov wrote: Not sure if this was pulled in yet, but I do have a small issue with this patch. > @@ -69,14 +82,19 @@ TRACE_EVENT(virtio_transport_alloc_pkt, > __entry->type = type; > __entry->op = op; > __entry->flags = flags; > + __entry->msg_len = msg_len; > + __entry->msg_cnt = msg_cnt; > ), > - TP_printk("%u:%u -> %u:%u len=%u type=%s op=%s flags=%#x", > + TP_printk("%u:%u -> %u:%u len=%u type=%s op=%s flags=%#x " > + "msg_len=%u msg_cnt=%u", It's considered poor formatting to split strings like the above. This is one of the exceptions for the 80 character limit. Do not break strings just to keep it within 80 characters. -- Steve > __entry->src_cid, __entry->src_port, > __entry->dst_cid, __entry->dst_port, > __entry->len, > show_type(__entry->type), > show_op(__entry->op), > - __entry->flags) > + __entry->flags, > + __entry->msg_len, > + __entry->msg_cnt) > ); ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address
Hi Eli, > From: Eli Cohen > Sent: Tuesday, March 2, 2021 11:09 AM > > On Mon, Mar 01, 2021 at 11:12:33AM -0500, Michael S. Tsirkin wrote: > > On Mon, Mar 01, 2021 at 03:19:51PM +0200, Eli Cohen wrote: > > > On Mon, Mar 01, 2021 at 08:09:48AM -0500, Michael S. Tsirkin wrote: > > > > On Mon, Mar 01, 2021 at 09:08:28AM +0200, Eli Cohen wrote: > > > > > On Wed, Feb 24, 2021 at 05:11:23PM +0800, Jason Wang wrote: > > > > > > > > > > > > On 2021/2/24 2:18 下午, Parav Pandit wrote: > > > > > > > From: Eli Cohen > > > > > > > > > > > > > > Use a randomly generated MAC address to be applied in case > > > > > > > it is not configured by management tool. > > > > > > > > > > > > > > The value queried through mlx5_query_nic_vport_mac_address() > > > > > > > is not relelavnt to vdpa since it is the mac that should be > > > > > > > used by the regular NIC driver. > > > > > > > > > > > > > > Signed-off-by: Eli Cohen > > > > > > > Reviewed-by: Parav Pandit > > > > > > > > > > > > > > > > > > Acked-by: Jason Wang > > > > > > > > > > > > > > > > > > > --- > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 + > > > > > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > index b67bba581dfd..ece2183e7b20 100644 > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > @@ -2005,10 +2005,7 @@ static int mlx5_vdpa_dev_add(struct > vdpa_mgmt_dev *v_mdev, const char *name) > > > > > > > if (err) > > > > > > > goto err_mtu; > > > > > > > - err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config- > >mac); > > > > > > > - if (err) > > > > > > > - goto err_mtu; > > > > > > > - > > > > > > > + eth_random_addr(config->mac); > > > > > > > > > > I think this patch is missing setting VIRTIO_NET_F_MTU. I will > > > > > post v2 with the other fixes in this series. > > > > > > > > I don't really understand why this is a good idea. > > > > > > > > If userspace wants a random mac it can set it, with this patch it > > > > is impossible to know whether the mac is a hardware one (which > > > > will be persistent e.g. across reboots) or a random one. > > > > > > > > > > You can still get a persistent MAC set by the vdpa tool. e.g > > > > > > vdpa dev config set vdpa0 mac 00:11:22:33:44:55 > > > > > > If you don't use vdpa tool, the device assigns a random MAC. This > > > MAC is used to filter imcoming unicast traffic (done in a subsequent > patch). > > > > Well previously device learned the MAC from outgoing traffic and used > > that for the filter. > > No, was is added only in the last series that Parav send. Before that the > device did not filter and forwarded any packet that was forwarded to it buy > the eswitch. > > > Is changing that to a random value really all that useful as a > > default? Why not do the randomization in userspace? > > > > I think we want management entity to set the MAC, that's the VDPA tool. > So as things are right now (with the last series applied), the vdpa tool is > the > tool to assign MAC addresses and if it doesn't, a device randomly generated > MAC applies. Currently MAC addresses set by qemu command line are > ignored (set_config is not implementd). We can allow this but this will > override vdpa tool configuration. I believe its incorrect to always do random generated mac as of this patch. This is because, doing so ignores any admin config done by the sysadmin on the switch (ovs side) using [1]. So if user choose to configure using eswitch config, mlx5_vnet to honor that. And if user prefers to override is using vdpa tool or set_config from QEMU side, so be it. Hence, instead of reporting all zeros, mlx5 should query own vport context and publish that mac in the config layout and rx steering side. If user choose to override it, config layout and rx rules will have to updated on such config. [1] devlink port function set pci/:03:00.0// hw_addr 00:11:22:33:44:55 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero
On 2021/3/2 8:08 下午, Cornelia Huck wrote: On Mon, 1 Mar 2021 11:51:08 +0800 Jason Wang wrote: On 2021/3/1 5:25 上午, Michael S. Tsirkin wrote: On Fri, Feb 26, 2021 at 04:19:16PM +0800, Jason Wang wrote: On 2021/2/26 2:53 上午, Michael S. Tsirkin wrote: Confused. What is wrong with the above? It never reads the field unless the feature has been offered by device. So the spec said: " The following driver-read-only field, max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ is set. " If I read this correctly, there will be no max_virtqueue_pairs field if the VIRTIO_NET_F_MQ is not offered by device? If yes the offsetof() violates what spec said. Thanks I think that's a misunderstanding. This text was never intended to imply that field offsets change beased on feature bits. We had this pain with legacy and we never wanted to go back there. This merely implies that without VIRTIO_NET_F_MQ the field should not be accessed. Exists in the sense "is accessible to driver". Let's just clarify that in the spec, job done. Ok, agree. That will make things more eaiser. Yes, that makes much more sense. What about adding the following to the "Basic Facilities of a Virtio Device/Device Configuration Space" section of the spec: "If an optional configuration field does not exist, the corresponding space is still present, but reserved." This became interesting after re-reading some of the qemu codes. E.g in virtio-net.c we had: *static VirtIOFeature feature_sizes[] = { {.flags = 1ULL << VIRTIO_NET_F_MAC, .end = endof(struct virtio_net_config, mac)}, {.flags = 1ULL << VIRTIO_NET_F_STATUS, .end = endof(struct virtio_net_config, status)}, {.flags = 1ULL << VIRTIO_NET_F_MQ, .end = endof(struct virtio_net_config, max_virtqueue_pairs)}, {.flags = 1ULL << VIRTIO_NET_F_MTU, .end = endof(struct virtio_net_config, mtu)}, {.flags = 1ULL << VIRTIO_NET_F_SPEED_DUPLEX, .end = endof(struct virtio_net_config, duplex)}, {.flags = (1ULL << VIRTIO_NET_F_RSS) | (1ULL << VIRTIO_NET_F_HASH_REPORT), .end = endof(struct virtio_net_config, supported_hash_types)}, {} };* *It has a implict dependency chain. E.g MTU doesn't presnet if DUPLEX/RSS is not offered ... * (Do we need to specify what a device needs to do if the driver tries to access a non-existing field? We cannot really make assumptions about config space accesses; virtio-ccw can just copy a chunk of config space that contains non-existing fields... Right, it looks to me ccw doesn't depends on config len which is kind of interesting. I think the answer depends on the implementation of both transport and device: Let's take virtio-net-pci as an example, it fills status unconditionally in virtio_net_set_config() even if VIRTIO_NET_F_STATUS is not negotiated. So with the above feature_sizes: 1) if one of the MQ, MTU, DUPLEX or RSS is implemented, driver can still read status in this case 2) if none of the above four is negotied, driver can only read a 0xFF (virtio_config_readb()) I guess the device could ignore writes and return zeroes on read?) So consider the above, it might be too late to fix/clarify that in the spec on the expected behaviour of reading/writing non-exist fields. Thanks I've opened https://github.com/oasis-tcs/virtio-spec/issues/98 for the spec issues. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization