Re: [PATCH v6 9/9] ALSA: virtio: introduce device suspend/resume support

2021-03-02 Thread Anton Yakovlev

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

2021-03-02 Thread Jie Deng


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

2021-03-02 Thread Jason Wang
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

2021-03-02 Thread Michael S. Tsirkin
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

2021-03-02 Thread Michael S. Tsirkin
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

2021-03-02 Thread Stefan Hajnoczi
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

2021-03-02 Thread Jason Wang
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

2021-03-02 Thread Jason Wang


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

2021-03-02 Thread Jason Wang


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

2021-03-02 Thread Arnd Bergmann
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

2021-03-02 Thread Stefan Hajnoczi
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

2021-03-02 Thread Michael S. Tsirkin
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

2021-03-02 Thread Cornelia Huck
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

2021-03-02 Thread Stefano Garzarella

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

2021-03-02 Thread Stefano Garzarella

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

2021-03-02 Thread Anton Yakovlev

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

2021-03-02 Thread Anton Yakovlev
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

2021-03-02 Thread Anton Yakovlev
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

2021-03-02 Thread Anton Yakovlev
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

2021-03-02 Thread Anton Yakovlev
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

2021-03-02 Thread Anton Yakovlev
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

2021-03-02 Thread Anton Yakovlev
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

2021-03-02 Thread Anton Yakovlev
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

2021-03-02 Thread Anton Yakovlev
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

2021-03-02 Thread Anton Yakovlev
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

2021-03-02 Thread Anton Yakovlev
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

2021-03-02 Thread Borislav Petkov
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

2021-03-02 Thread Steven Rostedt
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

2021-03-02 Thread Parav Pandit
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

2021-03-02 Thread Jason Wang


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