RE: [PATCH linux-next 1/9] vdpa_sim: Consider read only supported features instead of current
Hi Michael, Jason,
> From: Michael S. Tsirkin
> Sent: Wednesday, February 24, 2021 12:40 PM
>
> On Wed, Feb 24, 2021 at 08:18:36AM +0200, Parav Pandit wrote:
> > To honor VIRTIO_F_VERSION_1 feature bit, during endianness detection,
> > consider the read only supported features bit instead of current
> > features bit which can be modified by the driver.
> >
> > This enables vdpa_sim_net driver to invoke cpu_to_vdpasim16() early
> > enough just after vdpasim device creation in subsequent patch.
> >
> > Signed-off-by: Parav Pandit
> > Reviewed-by: Eli Cohen
>
> Well that works for legacy and modern devices but not for transitional ones.
> Without transitional device support vendors are reluctant to add modern
> features since that will break old guests ... I suspect we need to either
> add a
> new ioctl enabling modern mode, or abuse SET_FEATURES and call it from
> qemu on first config space access.
>
>From mgmt tool kernel point of view,
(a) config space decoding depends on current feature version bit
(b) feature get/set and config read are not atomic callbacks
Mgmt. tool kernel code need to read them in single call from the vendor driver.
I need to add mgmt_dev->ops->get_features_config(struct
virtio_features_config*) calllback that returns value of both atomically in
structure like below.
struct virtio_features_config {
u64 features;
union {
struct virtio_net_config net;
struct virtio_block_config blk;
} u;
}
What do you think?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4] i2c: virtio: add a virtio i2c frontend driver
On 2021/2/26 12:21, Viresh Kumar wrote: On 26-02-21, 10:46, Jie Deng wrote: This v4 was the old version before the specification was acked by the virtio tc. Following is the latest specification. https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex I will send the v5 since the host/guest ABI changes. Okay, now it makes some sense :) I am interested in this stuff, if possible please keep me Cc'd for following versions, thanks. Sure. I will add you to the Cc list. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout
> From: Jason Wang
> Sent: Wednesday, February 24, 2021 2:18 PM
>
> On 2021/2/24 2:18 下午, Parav Pandit wrote:
> > +
> > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> > + config->max_virtqueue_pairs))
> > + return -EMSGSIZE;
>
>
> We probably need to check VIRTIO_NET_F_RSS here.
Yes. Will use it in v2.
>
>
> > + if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
> > + config->rss_max_key_size))
> > + return -EMSGSIZE;
> > +
> > + rss_field = le16_to_cpu(config->rss_max_key_size);
> > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
> rss_field))
> > + return -EMSGSIZE;
> > +
> > + hash_types = le32_to_cpu(config->supported_hash_types);
> > + if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
> > + config->supported_hash_types))
> > + return -EMSGSIZE;
> > + return 0;
> > +}
> > +
> > +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct
> > +sk_buff *msg) {
> > + struct virtio_net_config config = {};
> > +
> > + vdev->config->get_config(vdev, 0, &config, sizeof(config));
>
>
> Do we need to sync with other possible get_config calls here?
To readers of get_config() is ok. No need to sync.
However, I think set_config() and get_config() should sync otherwise get_config
can get partial value.
Will probably have to add vdpa_device->config_access_lock().
>
>
> > + if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
> sizeof(config.mac), config.mac))
> > + return -EMSGSIZE;
> > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, config.status))
> > + return -EMSGSIZE;
> > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, config.mtu))
> > + return -EMSGSIZE;
>
>
> And check VIRTIO_NET_F_SPEED_DUPLEX.
Yes, will do.
>
>
> > + if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_SPEED,
> config.speed))
> > + return -EMSGSIZE;
> > + if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX,
> config.duplex))
> > + return -EMSGSIZE;
> > +
> > + return vdpa_dev_net_mq_config_fill(vdev, msg, &config); }
> > +
> > +static int
> > +vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32
> portid, u32 seq,
> > +int flags, struct netlink_ext_ack *extack) {
> > + u32 device_id;
> > + void *hdr;
> > + int err;
> > +
> > + hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> > + VDPA_CMD_DEV_CONFIG_GET);
> > + if (!hdr)
> > + return -EMSGSIZE;
> > +
> > + if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev-
> >dev))) {
> > + err = -EMSGSIZE;
> > + goto msg_err;
> > + }
> > +
> > + device_id = vdev->config->get_device_id(vdev);
> > + if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> > + err = -EMSGSIZE;
> > + goto msg_err;
> > + }
> > +
> > + switch (device_id) {
> > + case VIRTIO_ID_NET:
> > + err = vdpa_dev_net_config_fill(vdev, msg);
> > + break;
> > + default:
> > + err = -EOPNOTSUPP;
> > + break;
> > + }
> > + if (err)
> > + goto msg_err;
> > +
> > + genlmsg_end(msg, hdr);
> > + return 0;
> > +
> > +msg_err:
> > + genlmsg_cancel(msg, hdr);
> > + return err;
> > +}
> > +
> > +static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb,
> > +struct genl_info *info) {
> > + struct vdpa_device *vdev;
> > + struct sk_buff *msg;
> > + const char *devname;
> > + struct device *dev;
> > + int err;
> > +
> > + if (!info->attrs[VDPA_ATTR_DEV_NAME])
> > + return -EINVAL;
> > + devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> > + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > + if (!msg)
> > + return -ENOMEM;
> > +
> > + mutex_lock(&vdpa_dev_mutex);
> > + dev = bus_find_device(&vdpa_bus, NULL, devname,
> vdpa_name_match);
> > + if (!dev) {
> > + NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> > + err = -ENODEV;
> > + goto dev_err;
> > + }
> > + vdev = container_of(dev, struct vdpa_device, dev);
> > + if (!vdev->mdev) {
> > + NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa
> device");
> > + err = -EINVAL;
> > + goto mdev_err;
> > + }
>
>
> Though it's fine but it looks to me mdev is not required to work here.
>
Yes, mdev is not required here. However it was little weird to allow $ vdpa dev
config show, but not allow $ vdpa dev show.
It transition phase right now. Subsequently will able to allow this as well.
After this series only ifc driver will be left to convert to user created
devices.
At that point, this checks will have chance to simplify.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH linux-next 4/9] vdpa_sim_net: Enable user to set mac address and mtu
> From: Michael S. Tsirkin
> Sent: Wednesday, February 24, 2021 12:27 PM
>
> On Wed, Feb 24, 2021 at 08:18:39AM +0200, Parav Pandit wrote:
> > Enable user to set the mac address and mtu so that each vdpa device
> > can have its own user specified mac address and mtu.
> > This is done by implementing the management device's configuration
> > layout fields setting callback routine.
> >
> > Now that user is enabled to set the mac address, remove the module
> > parameter for same.
>
> Will likely break some testing setups ...
> Not too hard to keep it around, is it?
>
Mostly not. In previous series, we moved away from default device to user
created device without an additional module param.
So this shouldn't break it.
It gets confusing which one to use and module param applies same mac to all the
vdpa devices.
So lets shift to user provided mac.
> >
> > And example of setting mac addr and mtu:
> > $ vdpa mgmtdev show
> >
> > $ vdpa dev add name bar mgmtdev vdpasim_net $ vdpa dev config set bar
> > mac 00:11:22:33:44:55 mtu 9000
> >
> > View the config after setting:
> > $ vdpa dev config show
> > bar: mac 00:11:22:33:44:55 link up link_announce false mtu 9000 speed
> > 0 duplex 0
> >
> > Signed-off-by: Parav Pandit
> > Reviewed-by: Eli Cohen
> > ---
> > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 39
> >
> > 1 file changed, 22 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > index 240a5f1306b5..6e941b0e7935 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > @@ -29,12 +29,6 @@
> >
> > #define VDPASIM_NET_VQ_NUM 2
> >
> > -static char *macaddr;
> > -module_param(macaddr, charp, 0);
> > -MODULE_PARM_DESC(macaddr, "Ethernet MAC address");
> > -
> > -static u8 macaddr_buf[ETH_ALEN];
> > -
> > static void vdpasim_net_work(struct work_struct *work) {
> > struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> > @@ -113,9 +107,7 @@ static void vdpasim_net_get_config(struct vdpasim
> *vdpasim, void *config)
> > struct virtio_net_config *net_config =
> > (struct virtio_net_config *)config;
> >
> > - net_config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
> > net_config->status = cpu_to_vdpasim16(vdpasim,
> VIRTIO_NET_S_LINK_UP);
> > - memcpy(net_config->mac, macaddr_buf, ETH_ALEN);
> > }
> >
> > static void vdpasim_net_mgmtdev_release(struct device *dev) @@ -134,6
> > +126,7 @@ static struct device vdpasim_net_mgmtdev_dummy = {
> >
> > static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char
> > *name) {
> > + struct virtio_net_config *cfg;
> > struct vdpasim_dev_attr dev_attr = {};
> > struct vdpasim *simdev;
> > int ret;
> > @@ -152,6 +145,10 @@ static int vdpasim_net_dev_add(struct
> vdpa_mgmt_dev *mdev, const char *name)
> > if (IS_ERR(simdev))
> > return PTR_ERR(simdev);
> >
> > + cfg = simdev->config;
> > + eth_random_addr(cfg->mac);
> > + cfg->mtu = cpu_to_vdpasim16(simdev, 1500);
> > +
> > ret = _vdpa_register_device(&simdev->vdpa);
> > if (ret)
> > goto reg_err;
>
> Hmm moving it here is problematic:
> this part happens before set_features so I suspect endian-ness will be wrong
> for BE hosts ...
>
Hmm. I see it. Looking for solution to it now.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4] i2c: virtio: add a virtio i2c frontend driver
On 2021/2/25 15:21, Viresh Kumar wrote:
On 12-10-20, 09:55, Jie Deng wrote:
Add an I2C bus driver for virtio para-virtualization.
The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.
This driver communicates with the backend driver through a
virtio I2C message structure which includes following parts:
- Header: i2c_msg addr, flags, len.
- Data buffer: the pointer to the I2C msg data.
- Status: the processing result from the backend.
People may implement different backend drivers to emulate
different controllers according to their needs. A backend
example can be found in the device model of the open source
project ACRN. For more information, please refer to
https://projectacrn.org.
diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
new file mode 100644
index 000..7413e45
--- /dev/null
+++ b/include/uapi/linux/virtio_i2c.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later OR BSD-3-Clause */
+/*
+ * Definitions for virtio I2C Adpter
+ *
+ * Copyright (c) 2020 Intel Corporation. All rights reserved.
+ */
+
+#ifndef _UAPI_LINUX_VIRTIO_I2C_H
+#define _UAPI_LINUX_VIRTIO_I2C_H
+
+#include
+#include
+#include
+
+/**
+ * struct virtio_i2c_hdr - the virtio I2C message header structure
+ * @addr: i2c_msg addr, the slave address
+ * @flags: i2c_msg flags
+ * @len: i2c_msg len
+ */
+struct virtio_i2c_hdr {
+ __le16 addr;
+ __le16 flags;
+ __le16 len;
+};
Hi Jie,
I am a bit confused about the header and the format in which data is being
processed here. When I look at the specification present here:
https://lists.oasis-open.org/archives/virtio-comment/202009/msg00021.html
it talks about
struct virtio_i2c_out_hdr {
le16 addr;
le16 padding;
le32 flags;
};
struct virtio_i2c_in_hdr {
u8 status;
};
struct virtio_i2c_req {
struct virtio_i2c_out_hdr out_hdr;
u8 write_buf [];
u8 read_buf [];
struct virtio_i2c_in_hdr in_hdr;
};
while what we have above is completely different. What am I missing ?
This v4 was the old version before the specification was acked by the
virtio tc.
Following is the latest specification.
https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
I will send the v5 since the host/guest ABI changes.
Thanks.
___
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
Hi Michael, Are you okay to live without this ioctl for now? I think QEMU is the one that needs to be fixed and will have to be made legacy guest aware. I think the kernel can just honor the feature negotiation result done by QEMU and do as what's told to.Will you agree? If it's fine, I would proceed to reverting commit fe36cbe067 and related code in question from the kernel. Thanks, -Siwei On 2/24/2021 10:24 AM, 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? One of the reason I asked is if this ioctl becomes a mandate for vhost-vdpa kernel. QEMU would reject initialize vhost-vdpa if doesn't see this ioctl coming? If it's optional, suppose the kernel may need it only when it becomes necessary? Thanks, -Siwei ___ 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
Hi Michael, Are you okay to live without this ioctl for now? I think QEMU is the one that needs to be fixed and will have to be made legacy guest aware. I think the kernel can just honor the feature negotiation result done by QEMU and do as what's told to.Will you agree? If it's fine, I would proceed to reverting commit fe36cbe067 and related code in question from the kernel. Thanks, -Siwei On 2/24/2021 10:24 AM, 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? One of the reason I asked is if this ioctl becomes a mandate for vhost-vdpa kernel. QEMU would reject initialize vhost-vdpa if doesn't see this ioctl coming? If it's optional, suppose the kernel may need it only when it becomes necessary? Thanks, -Siwei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 6/9] ALSA: virtio: PCM substream operators
On 25.02.2021 21:30, Takashi Iwai wrote:> On Thu, 25 Feb 2021 20:02:50
+0100,
Michael S. Tsirkin wrote:
On Thu, Feb 25, 2021 at 01:51:16PM +0100, Takashi Iwai wrote:
On Thu, 25 Feb 2021 13:14:37 +0100,
Anton Yakovlev wrote:
[snip]
Takashi given I was in my tree for a while and I planned to merge
it this merge window.
Hmm, that's too quick, I'm afraid. I see still a few rough edges in
the code. e.g. the reset work should be canceled at the driver
removal, but it's missing right now. And that'll become tricky
because the reset work itself unbinds the device, hence it'll get
stuck if calling cancel_work_sync() at remove callback.
Yes, you made a good point here! In this case, we need some external
mutex for synchronization. This is just a rough idea, but maybe
something like this might work:
struct reset_work {
struct mutex mutex;
struct work_struct work;
struct virtio_snd *snd;
bool resetting;
};
static struct reset_work reset_works[SNDRV_CARDS];
init()
// init mutexes and workers
virtsnd_probe()
snd_card_new(snd->card)
reset_works[snd->card->number].snd = snd;
virtsnd_remove()
mutex_lock(reset_works[snd->card->number].mutex)
reset_works[snd->card->number].snd = NULL;
resetting = reset_works[snd->card->number].resetting;
mutex_unlock(reset_works[snd->card->number].mutex)
if (!resetting)
// cancel worker reset_works[snd->card->number].work
// remove device
virtsnd_reset_fn(work)
mutex_lock(work->mutex)
if (!work->snd)
// do nothing and take an exit path
work->resetting = true;
mutex_unlock(work->mutex)
device_reprobe()
work->resetting = false;
interrupt_handler()
schedule_work(reset_works[snd->card->number].work);
What do you think?
--
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: [GIT PULL] virtio: features, fixes
The pull request you sent on Thu, 25 Feb 2021 14:33:33 -0500: > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/ffc1759676bed0bff046427dd7d00cb68660190d Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[GIT PULL] virtio: features, fixes
There are a couple new drivers and support for the new management interface for mlx under review now. I figured I'll send them separately if review is done in time, lots of people are waiting for the vdpa tool patches to I want to make sure they make this release. The following changes since commit f40ddce88593482919761f74910f42f4b84c004b: Linux 5.11 (2021-02-14 14:32:24 -0800) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus for you to fetch changes up to 16c10bede8b3d8594279752bf53153491f3f944f: virtio-input: add multi-touch support (2021-02-23 07:52:59 -0500) virtio: features, fixes new vdpa features to allow creation and deletion of new devices virtio-blk support per-device queue depth fixes, cleanups all over the place Signed-off-by: Michael S. Tsirkin Colin Xu (1): virtio_input: Prevent EV_MSC/MSC_TIMESTAMP loop storm for MT. Dongli Zhang (1): vhost scsi: alloc vhost_scsi with kvzalloc() to avoid delay Gustavo A. R. Silva (1): virtio_net: Fix fall-through warnings for Clang Jason Wang (17): virtio-pci: do not access iomem via struct virtio_pci_device directly virtio-pci: split out modern device virtio-pci-modern: factor out modern device initialization logic virtio-pci-modern: introduce vp_modern_remove() virtio-pci-modern: introduce helper to set config vector virtio-pci-modern: introduce helpers for setting and getting status virtio-pci-modern: introduce helpers for setting and getting features virtio-pci-modern: introduce vp_modern_generation() virtio-pci-modern: introduce vp_modern_set_queue_vector() virtio-pci-modern: introduce vp_modern_queue_address() virtio-pci-modern: introduce helper to set/get queue_enable virtio-pci-modern: introduce helper for setting/geting queue size virtio-pci-modern: introduce helper for getting queue nums virtio-pci-modern: introduce helper to get notification offset virito-pci-modern: rename map_capability() to vp_modern_map_capability() virtio-pci: introduce modern device module virtio_vdpa: don't warn when fail to disable vq Jiapeng Zhong (1): virtio-mem: Assign boolean values to a bool variable Joseph Qi (1): virtio-blk: support per-device queue depth Mathias Crombez (1): virtio-input: add multi-touch support Parav Pandit (6): vdpa_sim_net: Make mac address array static vdpa: Extend routine to accept vdpa device name vdpa: Define vdpa mgmt device, ops and a netlink interface vdpa: Enable a user to add and delete a vdpa device vdpa: Enable user to query vdpa device info vdpa_sim_net: Add support for user supported devices Stefano Garzarella (1): vdpa/mlx5: fix param validation in mlx5_vdpa_get_config() Xianting Tian (1): virtio_mmio: fix one typo drivers/block/virtio_blk.c | 11 +- drivers/net/virtio_net.c | 1 + drivers/vdpa/Kconfig | 1 + drivers/vdpa/ifcvf/ifcvf_main.c| 2 +- drivers/vdpa/mlx5/net/mlx5_vnet.c | 4 +- drivers/vdpa/vdpa.c| 503 ++- drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +- drivers/vdpa/vdpa_sim/vdpa_sim.h | 2 + drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 100 -- drivers/vhost/scsi.c | 9 +- drivers/virtio/Kconfig | 9 + drivers/virtio/Makefile| 1 + drivers/virtio/virtio_input.c | 26 +- drivers/virtio/virtio_mem.c| 2 +- drivers/virtio/virtio_mmio.c | 2 +- drivers/virtio/virtio_pci_common.h | 22 +- drivers/virtio/virtio_pci_modern.c | 504 --- drivers/virtio/virtio_pci_modern_dev.c | 599 + drivers/virtio/virtio_vdpa.c | 3 +- include/linux/vdpa.h | 44 ++- include/linux/virtio_pci_modern.h | 111 ++ include/uapi/linux/vdpa.h | 40 +++ 22 files changed, 1492 insertions(+), 507 deletions(-) create mode 100644 drivers/virtio/virtio_pci_modern_dev.c create mode 100644 include/linux/virtio_pci_modern.h create mode 100644 include/uapi/linux/vdpa.h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 6/9] ALSA: virtio: PCM substream operators
On Thu, Feb 25, 2021 at 01:51:16PM +0100, Takashi Iwai wrote:
> On Thu, 25 Feb 2021 13:14:37 +0100,
> Anton Yakovlev wrote:
> >
> > On 25.02.2021 11:55, Takashi Iwai wrote:
> > > On Mon, 22 Feb 2021 16:34:41 +0100,
> > > Anton Yakovlev wrote:
> > >> +static int virtsnd_pcm_open(struct snd_pcm_substream *substream)
> > >> +{
> > >> + struct virtio_pcm *vpcm = snd_pcm_substream_chip(substream);
> > >> + struct virtio_pcm_substream *vss = NULL;
> > >> +
> > >> + if (vpcm) {
> > >> + switch (substream->stream) {
> > >> + case SNDRV_PCM_STREAM_PLAYBACK:
> > >> + case SNDRV_PCM_STREAM_CAPTURE: {
> > >
> > > The switch() here looks superfluous. The substream->stream must be a
> > > good value in the callback. If any, you can put WARN_ON() there, but
> > > I don't think it worth.
> >
> > At least it doesn't do any harm.
>
> It does -- it makes the readability worse, and that's a very important
> point.
>
> > >> +static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
> > >> + struct snd_pcm_hw_params *hw_params)
> > >> +{
> > >
> > >> + return virtsnd_pcm_msg_alloc(vss, periods, period_bytes);
> > >
> > > We have the allocation, but...
> > >
> > >> +static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream)
> > >> +{
> > >> + return 0;
> > >
> > > ... no release at hw_free()?
> > > I know that the free is present in the allocator, but it's only for
> > > re-allocation case, I suppose.
> >
> > When the substream stops, sync_ptr 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 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 either in hw_params() or in
> > dev->release(), since there it is 100% safe.
>
> OK, then it's worth to document it about this object lifecycle.
> The buffer management of this driver is fairly unique, so otherwise it
> confuses readers.
>
>
> thanks,
>
> Takashi
Takashi given I was in my tree for a while and I planned to merge
it this merge window. I can still drop it but there are
unrelated patches behind these in the tree so that's a rebase
which will invalidate my testing, I'm just concerned about
meeting the merge window.
Would it be ok to merge this as is and then address
readability stuff by patches on top?
If yes please send acks!
If you want to merge it yourself instead, also please say so.
--
MST
___
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 Thu, Feb 25, 2021 at 12:36:07PM +0800, Jason Wang wrote:
>
> On 2021/2/24 7:12 下午, Cornelia Huck wrote:
> > On Wed, 24 Feb 2021 17:29:07 +0800
> > Jason Wang wrote:
> >
> > > On 2021/2/23 6:58 下午, Cornelia Huck wrote:
> > > > On Tue, 23 Feb 2021 18:31:07 +0800
> > > > Jason Wang wrote:
> > > > > On 2021/2/23 6:04 下午, Cornelia Huck wrote:
> > > > > > On Tue, 23 Feb 2021 17:46:20 +0800
> > > > > > Jason Wang wrote:
> > > > > > > On 2021/2/23 下午5:25, Michael S. Tsirkin wrote:
> > > > > > > > On Mon, Feb 22, 2021 at 09:09:28AM -0800, Si-Wei Liu wrote:
> > > > > > > > > On 2/21/2021 8:14 PM, Jason Wang wrote:
> > > > > > > > > > On 2021/2/19 7:54 下午, Si-Wei Liu wrote:
> > > > > > > > > > > Commit 452639a64ad8 ("vdpa: make sure set_features is
> > > > > > > > > > > invoked
> > > > > > > > > > > for legacy") made an exception for legacy guests to reset
> > > > > > > > > > > features to 0, when config space is accessed before
> > > > > > > > > > > features
> > > > > > > > > > > are set. We should relieve the verify_min_features() check
> > > > > > > > > > > and allow features reset to 0 for this case.
> > > > > > > > > > >
> > > > > > > > > > > It's worth noting that not just legacy guests could access
> > > > > > > > > > > config space before features are set. For instance, when
> > > > > > > > > > > feature VIRTIO_NET_F_MTU is advertised some modern driver
> > > > > > > > > > > will try to access and validate the MTU present in the
> > > > > > > > > > > config
> > > > > > > > > > > space before virtio features are set.
> > > > > > > > > > This looks like a spec violation:
> > > > > > > > > >
> > > > > > > > > > "
> > > > > > > > > >
> > > > > > > > > > The following driver-read-only field, mtu only exists if
> > > > > > > > > > VIRTIO_NET_F_MTU is set. This field specifies the maximum
> > > > > > > > > > MTU for the
> > > > > > > > > > driver to use.
> > > > > > > > > > "
> > > > > > > > > >
> > > > > > > > > > Do we really want to workaround this?
> > > > > > > > > Isn't the commit 452639a64ad8 itself is a workaround for
> > > > > > > > > legacy guest?
> > > > > > > > >
> > > > > > > > > I think the point is, since there's legacy guest we'd have to
> > > > > > > > > support, this
> > > > > > > > > host side workaround is unavoidable. Although I agree the
> > > > > > > > > violating driver
> > > > > > > > > should be fixed (yes, it's in today's upstream kernel which
> > > > > > > > > exists for a
> > > > > > > > > while now).
> > > > > > > > Oh you are right:
> > > > > > > >
> > > > > > > >
> > > > > > > > static int virtnet_validate(struct virtio_device *vdev)
> > > > > > > > {
> > > > > > > > if (!vdev->config->get) {
> > > > > > > > dev_err(&vdev->dev, "%s failure: config
> > > > > > > > access disabled\n",
> > > > > > > > __func__);
> > > > > > > > return -EINVAL;
> > > > > > > > }
> > > > > > > >
> > > > > > > > if (!virtnet_validate_features(vdev))
> > > > > > > > return -EINVAL;
> > > > > > > >
> > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
> > > > > > > > int mtu = virtio_cread16(vdev,
> > > > > > > > offsetof(struct
> > > > > > > > virtio_net_config,
> > > > > > > > mtu));
> > > > > > > > if (mtu < MIN_MTU)
> > > > > > > > __virtio_clear_bit(vdev,
> > > > > > > > VIRTIO_NET_F_MTU);
> > > > > > > I wonder why not simply fail here?
> > > > > > I think both failing or not accepting the feature can be argued to
> > > > > > make
> > > > > > sense: "the device presented us with a mtu size that does not make
> > > > > > sense" would point to failing, "we cannot work with the mtu size
> > > > > > that
> > > > > > the device presented us" would point to not negotiating the feature.
> > > > > > > > }
> > > > > > > >
> > > > > > > > return 0;
> > > > > > > > }
> > > > > > > >
> > > > > > > > And the spec says:
> > > > > > > >
> > > > > > > >
> > > > > > > > The driver MUST follow this sequence to initialize a device:
> > > > > > > > 1. Reset the device.
> > > > > > > > 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the
> > > > > > > > device.
> > > > > > > > 3. Set the DRIVER status bit: the guest OS knows how to drive
> > > > > > > > the device.
> > > > > > > > 4. Read device feature bits, and write the subset of feature
> > > > > > > > bits understood by the OS and driver to the
> > > > > > > > device. During this step the driver MAY read (but MUST NOT
> > > > > > > > write) the device-specific configuration
> > > > > > > > fields to check that it can support the device before accepting
> > > > > > > > it.
> > > > > > > > 5. Set the FEATURES_OK status bit. The driver MUST NOT accept
> > > > > > > > new feature bits afte
Re: [PATCH v2 net-next] virtio-net: support XDP_TX when not more queues
On Thu, 25 Feb 2021 16:22:29 +0800
Xuan Zhuo wrote:
> The number of queues implemented by many virtio backends is limited,
> especially some machines have a large number of CPUs. In this case, it
> is often impossible to allocate a separate queue for XDP_TX.
>
> This patch allows XDP_TX to run by reuse the existing SQ with
> __netif_tx_lock() hold when there are not enough queues.
>
> Signed-off-by: Xuan Zhuo
> Reviewed-by: Dust Li
> ---
> drivers/net/virtio_net.c | 48
>
> 1 file changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
[...]
> @@ -2416,12 +2441,8 @@ static int virtnet_xdp_set(struct net_device *dev,
> struct bpf_prog *prog,
> xdp_qp = nr_cpu_ids;
>
> /* XDP requires extra queues for XDP_TX */
> - if (curr_qp + xdp_qp > vi->max_queue_pairs) {
> - NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available");
> - netdev_warn(dev, "request %i queues but max is %i\n",
> - curr_qp + xdp_qp, vi->max_queue_pairs);
> - return -ENOMEM;
> - }
> + if (curr_qp + xdp_qp > vi->max_queue_pairs)
> + xdp_qp = 0;
I think we should keep a netdev_warn message, but as a warning (not
error) that this will cause XDP_TX and XDP_REDIRECT to be slower on
this device due to too few free TX rings available.
In the future, we can mark a XDP features flag that this device is
operating in a slower "locked" Tx mode.
>
> old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
> if (!prog && !old_prog)
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v5 02/19] af_vsock: separate wait data loop
> On 18 Feb 2021, at 06:36, Arseny Krasnov wrote:
>
> This moves wait loop for data to dedicated function, because later
> it will be used by SEQPACKET data receive loop.
>
> Signed-off-by: Arseny Krasnov
> ---
> net/vmw_vsock/af_vsock.c | 155 +--
> 1 file changed, 83 insertions(+), 72 deletions(-)
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 656370e11707..6cf7bb977aa1 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1832,6 +1832,68 @@ static int vsock_connectible_sendmsg(struct socket
> *sock, struct msghdr *msg,
> return err;
> }
>
> +static int vsock_wait_data(struct sock *sk, struct wait_queue_entry *wait,
> +long timeout,
> +struct vsock_transport_recv_notify_data *recv_data,
> +size_t target)
> +{
> + const struct vsock_transport *transport;
> + struct vsock_sock *vsk;
> + s64 data;
> + int err;
> +
> + vsk = vsock_sk(sk);
> + err = 0;
> + transport = vsk->transport;
> + prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE);
> +
> + while ((data = vsock_stream_has_data(vsk)) == 0) {
In the original code, the prepare_to_wait() is called for each iteration of the
while loop. In this
version, it is only called once. So if we do multiple iterations, the thread
would be in the
TASK_RUNNING state, and subsequent schedule_timeout() will return immediately.
So
looks like the prepare_to_wait() should be move here, in case we have a
spurious wake_up.
> + if (sk->sk_err != 0 ||
> + (sk->sk_shutdown & RCV_SHUTDOWN) ||
> + (vsk->peer_shutdown & SEND_SHUTDOWN)) {
> + break;
> + }
> +
> + /* Don't wait for non-blocking sockets. */
> + if (timeout == 0) {
> + err = -EAGAIN;
> + break;
> + }
> +
> + if (recv_data) {
> + err = transport->notify_recv_pre_block(vsk, target,
> recv_data);
> + if (err < 0)
> + break;
> + }
> +
> + release_sock(sk);
> + timeout = schedule_timeout(timeout);
> + lock_sock(sk);
> +
> + if (signal_pending(current)) {
> + err = sock_intr_errno(timeout);
> + break;
> + } else if (timeout == 0) {
> + err = -EAGAIN;
> + break;
> + }
> + }
> +
> + finish_wait(sk_sleep(sk), wait);
> +
> + if (err)
> + return err;
> +
> + /* Internal transport error when checking for available
> + * data. XXX This should be changed to a connection
> + * reset in a later change.
> + */
> + if (data < 0)
> + return -ENOMEM;
> +
> + return data;
> +}
> +
> static int
> vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> int flags)
> @@ -1911,85 +1973,34 @@ vsock_connectible_recvmsg(struct socket *sock, struct
> msghdr *msg, size_t len,
>
>
> while (1) {
> - s64 ready;
> -
> - prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
> - ready = vsock_stream_has_data(vsk);
> -
> - if (ready == 0) {
> - if (sk->sk_err != 0 ||
> - (sk->sk_shutdown & RCV_SHUTDOWN) ||
> - (vsk->peer_shutdown & SEND_SHUTDOWN)) {
> - finish_wait(sk_sleep(sk), &wait);
> - break;
> - }
> - /* Don't wait for non-blocking sockets. */
> - if (timeout == 0) {
> - err = -EAGAIN;
> - finish_wait(sk_sleep(sk), &wait);
> - break;
> - }
> + ssize_t read;
>
> - err = transport->notify_recv_pre_block(
> - vsk, target, &recv_data);
> - if (err < 0) {
> - finish_wait(sk_sleep(sk), &wait);
> - break;
> - }
> - release_sock(sk);
> - timeout = schedule_timeout(timeout);
> - lock_sock(sk);
> + err = vsock_wait_data(sk, &wait, timeout, &recv_data, target);
> + if (err <= 0)
> + break;
>
> - if (signal_pending(current)) {
> - err = sock_intr_errno(timeout);
> - finish_wait(sk_sleep(sk), &wait);
> - break;
> - } else if (timeout == 0) {
> - err = -EAGAIN;
> -
Re: [RFC PATCH v5 04/19] af_vsock: implement SEQPACKET receive loop
On 18 Feb 2021, at 06:37, Arseny Krasnov wrote:
>
> This adds receive loop for SEQPACKET. It looks like receive loop for
> STREAM, but there is a little bit difference:
> 1) It doesn't call notify callbacks.
> 2) It doesn't care about 'SO_SNDLOWAT' and 'SO_RCVLOWAT' values, because
> there is no sense for these values in SEQPACKET case.
> 3) It waits until whole record is received or error is found during
> receiving.
> 4) It processes and sets 'MSG_TRUNC' flag.
>
> So to avoid extra conditions for two types of socket inside one loop, two
> independent functions were created.
>
> Signed-off-by: Arseny Krasnov
> ---
> include/net/af_vsock.h | 5 +++
> net/vmw_vsock/af_vsock.c | 97 +++-
> 2 files changed, 101 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> index b1c717286993..01563338cc03 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -135,6 +135,11 @@ struct vsock_transport {
> bool (*stream_is_active)(struct vsock_sock *);
> bool (*stream_allow)(u32 cid, u32 port);
>
> + /* SEQ_PACKET. */
> + size_t (*seqpacket_seq_get_len)(struct vsock_sock *vsk);
> + int (*seqpacket_dequeue)(struct vsock_sock *vsk, struct msghdr *msg,
> + int flags, bool *msg_ready);
> +
> /* Notification. */
> int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);
> int (*notify_poll_out)(struct vsock_sock *, size_t, bool *);
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index d277dc1cdbdf..b754927a556a 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1972,6 +1972,98 @@ static int __vsock_stream_recvmsg(struct sock *sk,
> struct msghdr *msg,
> return err;
> }
>
> +static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
> + size_t len, int flags)
> +{
> + const struct vsock_transport *transport;
> + const struct iovec *orig_iov;
> + unsigned long orig_nr_segs;
> + bool msg_ready;
> + struct vsock_sock *vsk;
> + size_t record_len;
> + long timeout;
> + int err = 0;
> + DEFINE_WAIT(wait);
> +
> + vsk = vsock_sk(sk);
> + transport = vsk->transport;
> +
> + timeout = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> + orig_nr_segs = msg->msg_iter.nr_segs;
> + orig_iov = msg->msg_iter.iov;
> + msg_ready = false;
> + record_len = 0;
> +
> + while (1) {
> + err = vsock_wait_data(sk, &wait, timeout, NULL, 0);
> +
> + if (err <= 0) {
> + /* In case of any loop break(timeout, signal
> + * interrupt or shutdown), we report user that
> + * nothing was copied.
> + */
> + err = 0;
> + break;
> + }
> +
> + if (record_len == 0) {
> + record_len =
> + transport->seqpacket_seq_get_len(vsk);
> +
> + if (record_len == 0)
> + continue;
> + }
> +
> + err = transport->seqpacket_dequeue(vsk, msg,
> + flags, &msg_ready);
> +
> + if (err < 0) {
> + if (err == -EAGAIN) {
> + iov_iter_init(&msg->msg_iter, READ,
> + orig_iov, orig_nr_segs,
> + len);
> + /* Clear 'MSG_EOR' here, because dequeue
> + * callback above set it again if it was
> + * set by sender. This 'MSG_EOR' is from
> + * dropped record.
> + */
> + msg->msg_flags &= ~MSG_EOR;
> + record_len = 0;
> + continue;
> + }
So a question for my understanding of the flow here. SOCK_SEQPACKET is
reliable, so
what does it mean to drop the record? Is the transport supposed to roll back to
the
beginning of the current record? If the incoming data in the transport doesn’t
follow
the protocol, and packets need to be dropped, shouldn’t the socket be reset or
similar?
Maybe there is potential for simplifying the flow if that is the case.
> +
> + err = -ENOMEM;
> + break;
> + }
> +
> + if (msg_ready)
> + break;
> + }
> +
> + if (sk->sk_err)
> + err = -sk->sk_err;
> + else if (sk->sk_shutdown & RCV_SHUTDOWN)
> + err = 0;
> +
> + if (msg_ready) {
> + /* User sets MSG_TRUNC, so return real length of
> + * packet.
> + */
> + if (flags & MSG_TRUNC)
> + er
[bug report] vdpa: set the virtqueue num during register
Hello Jason Wang,
The patch ddd50f4495d3: "vdpa: set the virtqueue num during register"
from Feb 23, 2021, leads to the following static checker warning:
drivers/vdpa/ifcvf/ifcvf_main.c:433 ifcvf_probe()
warn: risky error pointer math: '__vdpa_alloc_device(dev,
&ifc_vdpa_ops, 2592 + (0), (0)))'
include/linux/vdpa.h
255 #define vdpa_alloc_device(dev_struct, member, parent, config, name) \
256container_of(__vdpa_alloc_device( \
257 parent, config, \
258 sizeof(dev_struct) + \
259 BUILD_BUG_ON_ZERO(offsetof( \
260 dev_struct, member)), name), \
261 dev_struct, member)
262
The __vdpa_alloc_device() returns an error pointer and if we call
container_of() on then that's a bug... (Unless the container_of() is
known to be a no-op, in which case it's sort of ugly but fine, I guess.
There is one caller where this is the case.).
drivers/vdpa/ifcvf/ifcvf_main.c
432
433 adapter = vdpa_alloc_device(struct ifcvf_adapter, vdpa,
434 dev, &ifc_vdpa_ops, NULL);
435 if (adapter == NULL) {
All the other caller check for NULL. :P
436 IFCVF_ERR(pdev, "Failed to allocate vDPA structure");
437 return -ENOMEM;
438 }
439
440 pci_set_master(pdev);
441 pci_set_drvdata(pdev, adapter);
442
regards,
dan carpenter
___
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 Thu, 25 Feb 2021 12:36:07 +0800
Jason Wang wrote:
> On 2021/2/24 7:12 下午, Cornelia Huck wrote:
> > On Wed, 24 Feb 2021 17:29:07 +0800
> > Jason Wang wrote:
> >
> >> On 2021/2/23 6:58 下午, Cornelia Huck wrote:
> >>> On Tue, 23 Feb 2021 18:31:07 +0800
> >>> Jason Wang wrote:
> The problem is the MTU description for example:
>
> "The following driver-read-only field, mtu only exists if
> VIRTIO_NET_F_MTU is set."
>
> It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".
> >>> "offered by the device"? I don't think it should 'disappear' from the
> >>> config space if the driver won't use it. (Same for other config space
> >>> fields that are tied to feature bits.)
> >>
> >> But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks
> >> to according to the spec there will be no mtu field.
> > I think so, yes.
> >
> >> And a more interesting case is VIRTIO_NET_F_MQ is not offered but
> >> VIRTIO_NET_F_MTU offered. To me, it means we don't have
> >> max_virtqueue_pairs but it's not how the driver is wrote today.
> > That would be a bug, but it seems to me that the virtio-net driver
> > reads max_virtqueue_pairs conditionally and handles absence of the
> > feature correctly?
>
>
> Yes, see the avove codes:
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
> int mtu = virtio_cread16(vdev,
> offsetof(struct virtio_net_config,
> mtu));
> if (mtu < MIN_MTU)
> __virtio_clear_bit(vdev, VIRTIO_NET_F_MTU);
> }
>
Ouch, you're right. The virtio_cread accessors all operate on offsets
into a structure, it's just more obvious here.
> So it's probably too late to fix the driver.
It is never too late to fix a driver :)
It seems involved, though. We'd need different config space structures
based upon which set of features has been negotiated. It's not too bad
when features build upon each other and add fields at the end (this
should be fine right now, if the code remembered to check for the
feature), but can become ugly if an in-between field depends upon a
feature.
I guess we've been lucky that it seems to be an extremely uncommon
configuration.
>
>
> >
> >>
> >>>
> Otherwise readers (at least for me), may think the MTU is only valid
> if driver set the bit.
> >>> I think it would still be 'valid' in the sense that it exists and has
> >>> some value in there filled in by the device, but a driver reading it
> >>> without negotiating the feature would be buggy. (Like in the kernel
> >>> code above; the kernel not liking the value does not make the field
> >>> invalid.)
> >>
> >> See Michael's reply, the spec allows read the config before setting
> >> features.
> > Yes, the period prior to finishing negotiation is obviously special.
> >
> >>
> >>> Maybe a statement covering everything would be:
> >>>
> >>> "The following driver-read-only field mtu only exists if the device
> >>> offers VIRTIO_NET_F_MTU and may be read by the driver during feature
> >>> negotiation and after VIRTIO_NET_F_MTU has been successfully
> >>> negotiated."
> >>>
>
> > Should we add a wording clarification to the spec?
> I think so.
> >>> Some clarification would be needed for each field that depends on a
> >>> feature; that would be quite verbose. Maybe we can get away with a
> >>> clarifying statement?
> >>>
> >>> "Some config space fields may depend on a certain feature. In that
> >>> case, the field exits if the device has offered the corresponding
> >>> feature,
> >>
> >> So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config
> >> will look like:
> >>
> >> struct virtio_net_config {
> >> u8 mac[6];
> >> le16 status;
> >> le16 mtu;
> >> };
> >>
> > I agree.
>
>
> So consider it's probably too late to fix the driver which assumes some
> field are always persent, it looks to me need fix the spec do declare
> the fields are always existing instead.
The problem with that is that it has been in the spec already, and a
compliant device that did not provide the fields without the features
would suddenly become non-compliant...
>
>
> >
> >>>and may be read by the driver during feature negotiation, and
> >>> accessed by the driver after the feature has been successfully
> >>> negotiated. A shorthand for this is a statement that a field only
> >>> exists if a certain feature bit is set."
> >>
> >> I'm not sure using "shorthand" is good for the spec, at least we can
> >> limit the its scope only to the configuration space part.
> > Maybe "a shorthand expression"?
>
>
> So the questions is should we use this for all over the spec or it will
> be only used in this speicifc part (device configuration).
For command structures and the like, "feature is set" should always
mean "
Re: [PATCH v5 6/9] ALSA: virtio: PCM substream operators
On 25.02.2021 11:55, Takashi Iwai wrote:
On Mon, 22 Feb 2021 16:34:41 +0100,
Anton Yakovlev wrote:
+static int virtsnd_pcm_open(struct snd_pcm_substream *substream)
+{
+ struct virtio_pcm *vpcm = snd_pcm_substream_chip(substream);
+ struct virtio_pcm_substream *vss = NULL;
+
+ if (vpcm) {
+ switch (substream->stream) {
+ case SNDRV_PCM_STREAM_PLAYBACK:
+ case SNDRV_PCM_STREAM_CAPTURE: {
The switch() here looks superfluous. The substream->stream must be a
good value in the callback. If any, you can put WARN_ON() there, but
I don't think it worth.
At least it doesn't do any harm. If something really went wrong, we can
check it right in the open callback, which is called the very first.
+static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *hw_params)
+{
+ return virtsnd_pcm_msg_alloc(vss, periods, period_bytes);
We have the allocation, but...
+static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream)
+{
+ return 0;
... no release at hw_free()?
I know that the free is present in the allocator, but it's only for
re-allocation case, I suppose.
When the substream stops, sync_ptr 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 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 either in hw_params() or in
dev->release(), since there it is 100% safe.
thanks,
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 4/7] x86/boot/compressed/64: Add 32-bit boot #VC handler
On Wed, Feb 10, 2021 at 11:21:32AM +0100, Joerg Roedel wrote: > From: Joerg Roedel > > Add a #VC exception handler which is used when the kernel still executes > in protected mode. This boot-path already uses CPUID, which will cause #VC > exceptions in an SEV-ES guest. > > Signed-off-by: Joerg Roedel > --- > arch/x86/boot/compressed/head_64.S | 6 ++ > arch/x86/boot/compressed/mem_encrypt.S | 77 +- > 2 files changed, 82 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/boot/compressed/head_64.S > b/arch/x86/boot/compressed/head_64.S > index 8deeec78cdb4..eadaa0a082b8 100644 > --- a/arch/x86/boot/compressed/head_64.S > +++ b/arch/x86/boot/compressed/head_64.S > @@ -34,6 +34,7 @@ > #include > #include > #include > +#include > #include "pgtable.h" > > /* > @@ -856,6 +857,11 @@ SYM_FUNC_START(startup32_set_idt_entry) > SYM_FUNC_END(startup32_set_idt_entry) > > SYM_FUNC_START(startup32_load_idt) > + /* #VC handler */ > + lealrva(startup32_vc_handler)(%ebp), %eax > + movl$X86_TRAP_VC, %edx > + callstartup32_set_idt_entry > + > /* Load IDT */ > lealrva(boot32_idt)(%ebp), %eax > movl%eax, rva(boot32_idt_desc+2)(%ebp) > diff --git a/arch/x86/boot/compressed/mem_encrypt.S > b/arch/x86/boot/compressed/mem_encrypt.S > index aa561795efd1..350ecb56c7e4 100644 > --- a/arch/x86/boot/compressed/mem_encrypt.S > +++ b/arch/x86/boot/compressed/mem_encrypt.S > @@ -67,10 +67,85 @@ SYM_FUNC_START(get_sev_encryption_bit) > ret > SYM_FUNC_END(get_sev_encryption_bit) > > +/* > + * Emit code to request an CPUID register from the Hypervisor using > + * the MSR-based protocol. > + * > + * fn: The register containing the CPUID function > + * reg: Register requested > + * 1 = EAX > + * 2 = EBX > + * 3 = ECX > + * 4 = EDX > + * > + * Result is in EDX. Jumps to .Lfail on error > + */ > +.macro SEV_ES_REQ_CPUID fn:req reg:req I'm wondering - instead of replicating this 4 times, can this be a function which you CALL? You do have a stack so you should be able to. > + /* Request CPUID[%ebx].EAX */ > + movl$\reg, %eax > + shll$30, %eax > + orl $0x0004, %eax > + movl\fn, %edx > + movl$MSR_AMD64_SEV_ES_GHCB, %ecx > + wrmsr > + rep; vmmcall > + rdmsr > + /* Check response code */ Before you do that, I guess you wanna check: GHCBData[29:12] – Reserved, must be zero in the HV response. Thx. -- 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: [PATCH v5 2/9] ALSA: virtio: add virtio sound driver
On 25.02.2021 11:38, Takashi Iwai wrote:
On Mon, 22 Feb 2021 16:34:37 +0100,
Anton Yakovlev wrote:
+static int virtsnd_find_vqs(struct virtio_snd *snd)
+{
+ struct virtio_device *vdev = snd->vdev;
+ vq_callback_t *callbacks[VIRTIO_SND_VQ_MAX] = {
+ [VIRTIO_SND_VQ_EVENT] = virtsnd_event_notify_cb
+ };
+ const char *names[VIRTIO_SND_VQ_MAX] = {
Shouldn't be static?
Well, yes. Although in this particular case, I do not think it is that
critical. :)
Also it's often const char * const names[] = { ... }
unless you overwrite something.
I tried to use the same type names as in the function prototype.
Otherwise the compiler or static analyzer may complain.
+/**
+ * virtsnd_reset_fn() - Kernel worker's function to reset the device.
+ * @work: Reset device work.
+ *
+ * Context: Process context.
+ */
+static void virtsnd_reset_fn(struct work_struct *work)
+{
+ struct virtio_snd *snd =
+ container_of(work, struct virtio_snd, reset_work);
+ struct virtio_device *vdev = snd->vdev;
+ struct device *dev = &vdev->dev;
+ int rc;
+
+ dev_info(dev, "sound device needs reset\n");
+
+ /*
+ * It seems that the only way to properly reset the device is to remove
+ * and re-create the ALSA sound card device.
+ */
+ rc = device_reprobe(dev);
+ if (rc)
+ dev_err(dev, "failed to reprobe sound device: %d\n", rc);
Now I'm wondering whether it's safe to do that from this place.
Basically device_reprobe() unbinds the device that releases the full
resources once including the devm_* stuff. And this work itself is in
a part of devm allocated resource, so it'll be released there. That
said, we might hit use-after-free... This needs to be verified.
It's safe. Suicide kernel workers are funny but possible things. Since
the kernel itself (AFAIU) assumes such a situation and does not access
the worker structure after the callback function call.
thanks,
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
