Re: [PATCH v4] drm/virtio: Add option to disable KMS support
On Wed, Mar 01, 2023 at 03:37:24AM +0300, Dmitry Osipenko wrote: > On 2/28/23 18:54, Rob Clark wrote: > > From: Rob Clark > > > > Add a build option to disable modesetting support. This is useful in > > cases where the guest only needs to use the GPU in a headless mode, or > > (such as in the CrOS usage) window surfaces are proxied to a host > > compositor. > > > > As the modesetting ioctls are a big surface area for potential security > > bugs to be found (it's happened in the past, we should assume it will > > again in the future), it makes sense to have a build option to disable > > those ioctls in cases where they serve no legitimate purpose. > > > > v2: Use more if (IS_ENABLED(...)) > > v3: Also permit the host to advertise no scanouts > > v4: Spiff out commit msg > > > > Signed-off-by: Rob Clark > > Reviewed-by: Dmitry Osipenko > > --- > > Gerd, to give you some context on the v4.. we've chatted a bit more on > the #dri-devel and concluded that config option is the most robust way > of having KMS disabled from a security stand point. We would also want > to have a per-driver option (and not global) because there are scenarios > of using passthrough GPU + virtio-gpu in a guest, hence we would only > want to toggle KMS for a particular driver. IMHO both ways options to disable the KMS bits should work the same way. With the current patch modeset_init() runs with num_scanouts == 0 but doesn't with CONFIG_KMS=n. There are also two different ways to tweak driver_features. Can we get rid of that please, for robustness reasons? I'd suggest to have a is_kms_enabled() helper function (probably best as inline so gcc can figure it is constant false for CONFIG_KMS=n and throw away unreachable code). Add "if (!is_kms_enabled()) return;" to modeset_init() and modeset_fini() instead of stubbing them out. Use the drm_device->driver_features override in both cases. Also the edid check can go away. As already mentioned this is about a device feature not a edid being present. take care, Gerd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 2/2] vdpasim: support doorbell mapping
在 2023/2/27 17:18, Longpeng(Mike) 写道:
From: Longpeng
Support doorbell mapping for vdpasim devices, then we can test the notify
passthrough feature even if there's no real hardware on hand.
Allocates a dummy page which is used to emulate the notify page of the device,
all VQs share the same notify register that initiated to 0x. A periodic
work will check whether there're requests need to process ( the value of the
notify register is 0x or not ).
We can test on QEMU with:
-device vhost-vdpa-device-pci,vhostdev=/dev/vhost-vdpa-0,page-per-vq=on
Signed-off-by: Longpeng
Acked-by: Jason Wang
Thanks
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 67
drivers/vdpa/vdpa_sim/vdpa_sim.h | 3 ++
2 files changed, 70 insertions(+)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index cb88891b44a8..5a8c820b179f 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -39,6 +39,8 @@ MODULE_PARM_DESC(max_iotlb_entries,
#define VDPASIM_QUEUE_ALIGN PAGE_SIZE
#define VDPASIM_QUEUE_MAX 256
#define VDPASIM_VENDOR_ID 0
+#define VDPASIM_VRING_POLL_PERIOD 100 /* ms */
+#define VDPASIM_NOTIFY_DEFVAL 0x
static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
{
@@ -245,6 +247,28 @@ static const struct dma_map_ops vdpasim_dma_ops = {
static const struct vdpa_config_ops vdpasim_config_ops;
static const struct vdpa_config_ops vdpasim_batch_config_ops;
+static void vdpasim_notify_work(struct work_struct *work)
+{
+ struct vdpasim *vdpasim;
+ u16 *val;
+
+ vdpasim = container_of(work, struct vdpasim, notify_work.work);
+
+ if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
+ goto out;
+
+ if (!vdpasim->running)
+ goto out;
+
+ val = (u16 *)vdpasim->notify;
+ if (xchg(val, VDPASIM_NOTIFY_DEFVAL) != VDPASIM_NOTIFY_DEFVAL)
+ schedule_work(>work);
+
+out:
+ schedule_delayed_work(>notify_work,
+ msecs_to_jiffies(VDPASIM_VRING_POLL_PERIOD));
+}
+
struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr,
const struct vdpa_dev_set_config *config)
{
@@ -286,6 +310,13 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr
*dev_attr,
set_dma_ops(dev, _dma_ops);
vdpasim->vdpa.mdev = dev_attr->mgmt_dev;
+ INIT_DELAYED_WORK(>notify_work, vdpasim_notify_work);
+
+ vdpasim->notify = (u16 *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
+ if (!vdpasim->notify)
+ goto err_iommu;
+ WRITE_ONCE(*vdpasim->notify, VDPASIM_NOTIFY_DEFVAL);
+
vdpasim->config = kzalloc(dev_attr->config_size, GFP_KERNEL);
if (!vdpasim->config)
goto err_iommu;
@@ -320,6 +351,13 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr
*dev_attr,
vdpasim->vdpa.dma_dev = dev;
+ /*
+* Start periodic (every 100ms) notify work, it won't introduce
+* any overhead if the device is not started.
+*/
+ schedule_delayed_work(>notify_work,
+ msecs_to_jiffies(VDPASIM_VRING_POLL_PERIOD));
+
return vdpasim;
err_iommu:
@@ -671,11 +709,34 @@ static int vdpasim_dma_unmap(struct vdpa_device *vdpa,
unsigned int asid,
return 0;
}
+static pgprot_t vdpasim_get_vq_notification_pgprot(struct vdpa_device *vdpa,
+ u16 qid, pgprot_t prot)
+{
+ /*
+* We use normal RAM pages to emulate the vq notification area, so
+* just keep the pgprot as it mmaped.
+*/
+ return prot;
+}
+
+static struct vdpa_notification_area
+vdpasim_get_vq_notification(struct vdpa_device *vdpa, u16 qid)
+{
+ struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+ struct vdpa_notification_area notify;
+
+ notify.addr = virt_to_phys((void *)vdpasim->notify);
+ notify.size = PAGE_SIZE;
+
+ return notify;
+}
+
static void vdpasim_free(struct vdpa_device *vdpa)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
int i;
+ cancel_delayed_work_sync(>notify_work);
cancel_work_sync(>work);
for (i = 0; i < vdpasim->dev_attr.nvqs; i++) {
@@ -694,6 +755,8 @@ static void vdpasim_free(struct vdpa_device *vdpa)
kfree(vdpasim->iommu);
kfree(vdpasim->vqs);
kfree(vdpasim->config);
+ if (vdpasim->notify)
+ free_page((unsigned long)vdpasim->notify);
}
static const struct vdpa_config_ops vdpasim_config_ops = {
@@ -705,6 +768,8 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
.get_vq_ready = vdpasim_get_vq_ready,
.set_vq_state = vdpasim_set_vq_state,
.get_vq_state = vdpasim_get_vq_state,
+ .get_vq_notification= vdpasim_get_vq_notification,
+ .get_vq_notification_pgprot = vdpasim_get_vq_notification_pgprot,
Re: [PATCH v4 07/15] vdpa: add vhost_vdpa_suspend
On 2/24/2023 7:54 AM, Eugenio Pérez wrote:
The function vhost.c:vhost_dev_stop fetches the vring base so the vq
state can be migrated to other devices. However, this is unreliable in
vdpa, since we didn't signal the device to suspend the queues, making
the value fetched useless.
Suspend the device if possible before fetching first and subsequent
vring bases.
Moreover, vdpa totally reset and wipes the device at the last device
before fetch its vrings base, making that operation useless in the last
device. This will be fixed in later patches of this series.
Signed-off-by: Eugenio Pérez
---
v4:
* Look for _F_SUSPEND at vhost_dev->backend_cap, not backend_features
* Fall back on reset & fetch used idx from guest's memory
---
hw/virtio/vhost-vdpa.c | 25 +
hw/virtio/trace-events | 1 +
2 files changed, 26 insertions(+)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 228677895a..f542960a64 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -712,6 +712,7 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, );
trace_vhost_vdpa_reset_device(dev, status);
+v->suspended = false;
return ret;
}
@@ -1109,6 +1110,29 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev)
}
}
+static void vhost_vdpa_suspend(struct vhost_dev *dev)
+{
+struct vhost_vdpa *v = dev->opaque;
+int r;
+
+if (!vhost_vdpa_first_dev(dev)) {
+return;
+}
+
+if (!(dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) {
Polarity reversed. This ends up device getting reset always even if the
backend offers _F_SUSPEND.
-Siwei
+trace_vhost_vdpa_suspend(dev);
+r = ioctl(v->device_fd, VHOST_VDPA_SUSPEND);
+if (unlikely(r)) {
+error_report("Cannot suspend: %s(%d)", g_strerror(errno), errno);
+} else {
+v->suspended = true;
+return;
+}
+}
+
+vhost_vdpa_reset_device(dev);
+}
+
static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
{
struct vhost_vdpa *v = dev->opaque;
@@ -1123,6 +1147,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev,
bool started)
}
vhost_vdpa_set_vring_ready(dev);
} else {
+vhost_vdpa_suspend(dev);
vhost_vdpa_svqs_stop(dev);
vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
}
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index a87c5f39a2..8f8d05cf9b 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -50,6 +50,7 @@ vhost_vdpa_set_vring_ready(void *dev) "dev: %p"
vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t flags) "dev: %p offset:
%"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32
vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: %p
config: %p config_len: %"PRIu32
+vhost_vdpa_suspend(void *dev) "dev: %p"
vhost_vdpa_dev_start(void *dev, bool started) "dev: %p started: %d"
vhost_vdpa_set_log_base(void *dev, uint64_t base, unsigned long long size, int refcnt, int fd,
void *log) "dev: %p base: 0x%"PRIx64" size: %llu refcnt: %d fd: %d log: %p"
vhost_vdpa_set_vring_addr(void *dev, unsigned int index, unsigned int flags, uint64_t desc_user_addr, uint64_t
used_user_addr, uint64_t avail_user_addr, uint64_t log_guest_addr) "dev: %p index: %u flags: 0x%x desc_user_addr:
0x%"PRIx64" used_user_addr: 0x%"PRIx64" avail_user_addr: 0x%"PRIx64" log_guest_addr:
0x%"PRIx64
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 05/11] vduse: Support automatic irq callback affinity
Hi Xie, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on tip/irq/core] [also build test WARNING on linus/master next-20230228] [cannot apply to mst-vhost/linux-next hch-configfs/for-next v6.2] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Xie-Yongji/lib-group_cpus-Export-group_cpus_evenly/20230228-174438 patch link: https://lore.kernel.org/r/20230228094110.37-6-xieyongji%40bytedance.com patch subject: [PATCH v3 05/11] vduse: Support automatic irq callback affinity config: x86_64-randconfig-s021 (https://download.01.org/0day-ci/archive/20230301/202303010802.fygx4t0d-...@intel.com/config) compiler: gcc-11 (Debian 11.3.0-8) 11.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/6c15cc28cb814c0e6cb80955bc59517e80c15ae2 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Xie-Yongji/lib-group_cpus-Export-group_cpus_evenly/20230228-174438 git checkout 6c15cc28cb814c0e6cb80955bc59517e80c15ae2 # save the config file mkdir build_dir && cp config build_dir/.config make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 olddefconfig make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/vdpa/vdpa_user/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202303010802.fygx4t0d-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/vdpa/vdpa_user/vduse_dev.c:724:16: sparse: sparse: symbol >> 'create_affinity_masks' was not declared. Should it be static? -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v3 1/3] vsock: support sockmap
On Tue, Feb 28, 2023 at 07:04:34PM +, Bobby Eshleman wrote: > @@ -1241,19 +1252,34 @@ static int vsock_dgram_connect(struct socket *sock, > > memcpy(>remote_addr, remote_addr, sizeof(vsk->remote_addr)); > sock->state = SS_CONNECTED; > + sk->sk_state = TCP_ESTABLISHED; > > out: > release_sock(sk); > return err; > } How is this related? Maybe add a comment to explain? Does TCP_ESTABLISHED make sense for all types of sockets? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v4] drm/virtio: Add option to disable KMS support
From: Rob Clark
Add a build option to disable modesetting support. This is useful in
cases where the guest only needs to use the GPU in a headless mode, or
(such as in the CrOS usage) window surfaces are proxied to a host
compositor.
As the modesetting ioctls are a big surface area for potential security
bugs to be found (it's happened in the past, we should assume it will
again in the future), it makes sense to have a build option to disable
those ioctls in cases where they serve no legitimate purpose.
v2: Use more if (IS_ENABLED(...))
v3: Also permit the host to advertise no scanouts
v4: Spiff out commit msg
Signed-off-by: Rob Clark
Reviewed-by: Dmitry Osipenko
---
drivers/gpu/drm/virtio/Kconfig | 11 +++
drivers/gpu/drm/virtio/Makefile | 5 +++-
drivers/gpu/drm/virtio/virtgpu_drv.c | 6 +++-
drivers/gpu/drm/virtio/virtgpu_drv.h | 10 +++
drivers/gpu/drm/virtio/virtgpu_kms.c | 44 ++--
5 files changed, 59 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
QEMU based VMMs (like KVM or Xen).
If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+ bool "Virtio GPU driver modesetting support"
+ depends on DRM_VIRTIO_GPU
+ default y
+ help
+ Enable modesetting support for virtio GPU driver. This can be
+ disabled in cases where only "headless" usage of the GPU is
+ required.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..24c7ebe87032 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -4,8 +4,11 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
- virtgpu_display.o virtgpu_vq.o \
+ virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
+ virtgpu_display.o
+
obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..9cb7d6dd3da6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = {
- .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
DRIVER_ATOMIC,
+ .driver_features =
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
+ DRIVER_MODESET | DRIVER_ATOMIC |
+#endif
+ DRIVER_GEM | DRIVER_RENDER,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..ffe8faf67247 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device
*vgdev,
uint32_t x, uint32_t y);
/* virtgpu_display.c */
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
+#else
+static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
+{
+ return 0;
+}
+static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
+{
+}
+#endif
/* virtgpu_plane.c */
uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..1d888e309d6b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -161,7 +161,8 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_VIRGL))
vgdev->has_virgl_3d = true;
#endif
- if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS) &&
+ virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
vgdev->has_edid = true;
}
if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
@@ -218,17 +219,28 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
goto err_vbufs;
}
- /* get display info */
- virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
- num_scanouts, _scanouts);
- vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
-
Re: [PATCH v3] drm/virtio: Add option to disable KMS support
On Tue, Feb 28, 2023 at 4:34 AM Thomas Zimmermann wrote:
>
> Hi
>
> Am 27.02.23 um 19:15 schrieb Rob Clark:
> > On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
> > wrote:
> >>
> >> On 2/27/23 20:38, Rob Clark wrote:
> >> ...
> >>> + if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS)) {
> >>> + /* get display info */
> >>> + virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
> >>> + num_scanouts, _scanouts);
> >>> + vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
> >>> + VIRTIO_GPU_MAX_SCANOUTS);
> >>> + if (!vgdev->num_scanouts) {
> >>> + /*
> >>> + * Having an EDID but no scanouts is non-sensical,
> >>> + * but it is permitted to have no scanouts and no
> >>> + * EDID (in which case DRIVER_MODESET and
> >>> + * DRIVER_ATOMIC are not advertised)
> >>> + */
> >>> + if (vgdev->has_edid) {
> >>> + DRM_ERROR("num_scanouts is zero\n");
> >>> + ret = -EINVAL;
> >>> + goto err_scanouts;
> >>> + }
> >>> + dev->driver_features &= ~(DRIVER_MODESET |
> >>> DRIVER_ATOMIC);
> >>
> >> If it's now configurable by host, why do we need the
> >> CONFIG_DRM_VIRTIO_GPU_KMS?
> >
> > Because a kernel config option makes it more obvious that
> > modeset/atomic ioctls are blocked. Which makes it more obvious about
> > where any potential security issues apply and where fixes need to get
> > backported to. The config option is the only thing _I_ want,
> > everything else is just a bonus to help other people's use-cases.
>
> I find this very vague. What's the security thread?
The modeset ioctls are a big potential attack surface area. Which in
the case of CrOS VM guests serves no legitimate purpose. (kms is
unused in the guest, instead guest window surfaces are proxied to host
for composition alongside host window surfaces.)
There have been in the past potential security bugs (use-after-free,
etc) found in the kms ioctls. We should assume that there will be
more in the future. So it seems like simple common sense to want to
block unused ioctls.
> And if the config option is useful, shouldn't it be DRM-wide? The
> modesetting ioctl calls are shared among all drivers.
Maybe, if there is a use? The situation of compositing guest windows
in the host seems a bit unique to virtgpu, which is why I went with a
config option specific to virtgpu.
BR,
-R
> Best regards
> Thomas
>
> >
> > BR,
> > -R
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3] drm/virtio: Add option to disable KMS support
Am 28.02.23 um 13:34 schrieb Thomas Zimmermann:
Hi
Am 27.02.23 um 19:15 schrieb Rob Clark:
On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
wrote:
On 2/27/23 20:38, Rob Clark wrote:
...
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS)) {
+ /* get display info */
+ virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
+ num_scanouts, _scanouts);
+ vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
+ VIRTIO_GPU_MAX_SCANOUTS);
+ if (!vgdev->num_scanouts) {
+ /*
+ * Having an EDID but no scanouts is
non-sensical,
+ * but it is permitted to have no scanouts and no
+ * EDID (in which case DRIVER_MODESET and
+ * DRIVER_ATOMIC are not advertised)
+ */
+ if (vgdev->has_edid) {
+ DRM_ERROR("num_scanouts is zero\n");
+ ret = -EINVAL;
+ goto err_scanouts;
+ }
+ dev->driver_features &= ~(DRIVER_MODESET |
DRIVER_ATOMIC);
If it's now configurable by host, why do we need the
CONFIG_DRM_VIRTIO_GPU_KMS?
Because a kernel config option makes it more obvious that
modeset/atomic ioctls are blocked. Which makes it more obvious about
where any potential security issues apply and where fixes need to get
backported to. The config option is the only thing _I_ want,
everything else is just a bonus to help other people's use-cases.
I find this very vague. What's the security thread?
And if the config option is useful, shouldn't it be DRM-wide? The
modesetting ioctl calls are shared among all drivers.
For reference, here's an older discussion about render-only devices.
https://lore.kernel.org/dri-devel/2022100437.15258-1-christian.koe...@amd.com/
Best regards
Thomas
BR,
-R
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3] drm/virtio: Add option to disable KMS support
Hi,
> + if (!vgdev->num_scanouts) {
> + /*
> + * Having an EDID but no scanouts is non-sensical,
> + * but it is permitted to have no scanouts and no
> + * EDID (in which case DRIVER_MODESET and
> + * DRIVER_ATOMIC are not advertised)
> + */
> + if (vgdev->has_edid) {
> + DRM_ERROR("num_scanouts is zero\n");
That error message isn't very clear.
Also I'd suggest to just drop the edid check. It's about commands
being supported by the device, not about the actual presence of an EDID
blob, so the check doesn't look very useful to me.
take care,
Gerd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3] drm/virtio: Add option to disable KMS support
Hi
Am 27.02.23 um 19:15 schrieb Rob Clark:
On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
wrote:
On 2/27/23 20:38, Rob Clark wrote:
...
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS)) {
+ /* get display info */
+ virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
+ num_scanouts, _scanouts);
+ vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
+ VIRTIO_GPU_MAX_SCANOUTS);
+ if (!vgdev->num_scanouts) {
+ /*
+ * Having an EDID but no scanouts is non-sensical,
+ * but it is permitted to have no scanouts and no
+ * EDID (in which case DRIVER_MODESET and
+ * DRIVER_ATOMIC are not advertised)
+ */
+ if (vgdev->has_edid) {
+ DRM_ERROR("num_scanouts is zero\n");
+ ret = -EINVAL;
+ goto err_scanouts;
+ }
+ dev->driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
If it's now configurable by host, why do we need the
CONFIG_DRM_VIRTIO_GPU_KMS?
Because a kernel config option makes it more obvious that
modeset/atomic ioctls are blocked. Which makes it more obvious about
where any potential security issues apply and where fixes need to get
backported to. The config option is the only thing _I_ want,
everything else is just a bonus to help other people's use-cases.
I find this very vague. What's the security thread?
And if the config option is useful, shouldn't it be DRM-wide? The
modesetting ioctl calls are shared among all drivers.
Best regards
Thomas
BR,
-R
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drm/virtio: Add option to disable KMS support
Hi
Am 24.02.23 um 19:02 schrieb Rob Clark:
From: Rob Clark
Add a build option to disable modesetting support. This is useful in
cases where the guest only needs to use the GPU in a headless mode, or
(such as in the CrOS usage) window surfaces are proxied to a host
compositor.
We've just been discussing this on IRC, but failed to see the practical
benefit. It's not like the modesetting code takes up lots of memory.
What's the real-world use case?
Best regards
Thomas
Signed-off-by: Rob Clark
---
drivers/gpu/drm/virtio/Kconfig | 11 +++
drivers/gpu/drm/virtio/Makefile | 5 -
drivers/gpu/drm/virtio/virtgpu_drv.c | 6 +-
drivers/gpu/drm/virtio/virtgpu_drv.h | 10 ++
drivers/gpu/drm/virtio/virtgpu_kms.c | 6 ++
5 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
QEMU based VMMs (like KVM or Xen).
If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+ bool "Virtio GPU driver modesetting support"
+ depends on DRM_VIRTIO_GPU
+ default y
+ help
+ Enable modesetting support for virtio GPU driver. This can be
+ disabled in cases where only "headless" usage of the GPU is
+ required.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..24c7ebe87032 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -4,8 +4,11 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
- virtgpu_display.o virtgpu_vq.o \
+ virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
+ virtgpu_display.o
+
obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..9cb7d6dd3da6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = {
- .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
DRIVER_ATOMIC,
+ .driver_features =
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
+ DRIVER_MODESET | DRIVER_ATOMIC |
+#endif
+ DRIVER_GEM | DRIVER_RENDER,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..ffe8faf67247 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device
*vgdev,
uint32_t x, uint32_t y);
/* virtgpu_display.c */
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
+#else
+static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
+{
+ return 0;
+}
+static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
+{
+}
+#endif
/* virtgpu_plane.c */
uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..293e6f0bf133 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -161,9 +161,11 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_VIRGL))
vgdev->has_virgl_3d = true;
#endif
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
vgdev->has_edid = true;
}
+#endif
if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
vgdev->has_indirect = true;
}
@@ -218,6 +220,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
goto err_vbufs;
}
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
/* get display info */
virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
num_scanouts, _scanouts);
@@ -229,6 +232,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
goto err_scanouts;
}
DRM_INFO("number of scanouts: %d\n", num_scanouts);
+#endif
Re: [PATCH v2] vdpa_sim: set last_used_idx as last_avail_idx in vdpasim_queue_ready
On Fri, Feb 03, 2023 at 03:25:01PM +0100, Eugenio Pérez wrote:
> Starting from an used_idx different than 0 is needed in use cases like
> virtual machine migration. Not doing so and letting the caller set an
> avail idx different than 0 causes destination device to try to use old
> buffers that source driver already recover and are not available
> anymore.
>
> Since vdpa_sim does not support receive inflight descriptors as a
> destination of a migration, let's set both avail_idx and used_idx the
> same at vq start. This is how vhost-user works in a
> VHOST_SET_VRING_BASE call.
>
> Although the simple fix is to set last_used_idx at vdpasim_set_vq_state,
> it would be reset at vdpasim_queue_ready. The last_avail_idx case is
> fixed with commit a09f493c ("vdpa_sim: not reset state in
> vdpasim_queue_ready"). Since the only option is to make it equal to
> last_avail_idx, adding the only change needed here.
>
> This was discovered and tested live migrating the vdpa_sim_net device.
>
> Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
> Signed-off-by: Eugenio Pérez
> ---
> Cherry-picked from patch 2/2 of the series [1]. Differences are:
> * Set the value of used_idx at vdpasim_queue_ready instead of fetching
> from the guest vring like vhost-kernel.
>
> v2: Actually update last_used_idx only at vdpasim_queue_ready.
>
> Note that commit id present in the patch text is not in master but in
> git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git.
>
> [1] https://lkml.org/lkml/2023/1/18/1041
Can you post with a fixed hash please?
> ---
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index 6a0a65814626..79ac585e40b9 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -68,6 +68,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim,
> unsigned int idx)
> (uintptr_t)vq->device_addr);
>
> vq->vring.last_avail_idx = last_avail_idx;
> + vq->vring.last_used_idx = last_avail_idx;
> vq->vring.notify = vdpasim_vq_notify;
> }
>
> --
> 2.31.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drm/virtio: Add option to disable KMS support
Hi
Am 28.02.23 um 10:19 schrieb Javier Martinez Canillas:
Gerd Hoffmann writes:
[...]
I think it is a bad idea to make that a compile time option, I'd suggest
a runtime switch instead, for example a module parameter to ask the
driver to ignore any scanouts.
I don't think there's a need for a new module parameter, there's already
the virtio-gpu 'modeset' module parameter to enable/disable modsetting
and the global 'nomodeset' kernel cmdline parameter to do it for all DRM
drivers.
Currently, many drivers just fail to probe when 'nomodeset' is present,
but others only disable modsetting but keep the rendering part. In fact,
most DRM only drivers just ignore the 'nomodeset' parameter.
Do you have a list of these drivers? Maybe we need to adjust semantics
slightly. Please see my comment below.
We could make virtio-gpu driver to only disable KMS with these params,
something like the following (untested) patch:
From 9cddee7f696f37c34d80d6160e87827f3d7a0237 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas
Date: Tue, 28 Feb 2023 10:09:11 +0100
Subject: [PATCH] drm/virtio: Only disable KMS with nomodeset
The virtio-gpu driver currently fails to probe if either the "nomodeset"
kernel cmdline parameter is used or the module "modeset" parameter used.
But there may be cases where the rendering part of the driver is needed
and only the mode setting part needs to be disabled. So let's change the
logic to only disable the KMS part but still keep the DRM side of it.
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/virtio/virtgpu_display.c | 16 +++
drivers/gpu/drm/virtio/virtgpu_drv.c | 23 ++
drivers/gpu/drm/virtio/virtgpu_kms.c | 25 +---
3 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c
b/drivers/gpu/drm/virtio/virtgpu_display.c
index 9ea7611a9e0f..e176e5e8c1a0 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -335,6 +335,22 @@ static const struct drm_mode_config_funcs
virtio_gpu_mode_funcs = {
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
{
int i, ret;
+ u32 num_scanouts;
+
+ if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
+ vgdev->has_edid = true;
+ }
+
+ /* get display info */
+ virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
+ num_scanouts, _scanouts);
+ vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
+ VIRTIO_GPU_MAX_SCANOUTS);
+ if (!vgdev->num_scanouts) {
+ DRM_ERROR("num_scanouts is zero\n");
+ return -EINVAL;
+ }
+ DRM_INFO("number of scanouts: %d\n", num_scanouts);
ret = drmm_mode_config_init(vgdev->ddev);
if (ret)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..979b5b177f49 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -40,7 +40,7 @@
#include "virtgpu_drv.h"
-static const struct drm_driver driver;
+static struct drm_driver driver;
static int virtio_gpu_modeset = -1;
@@ -69,13 +69,12 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)
static int virtio_gpu_probe(struct virtio_device *vdev)
{
struct drm_device *dev;
+ struct virtio_gpu_device *vgdev;
int ret;
- if (drm_firmware_drivers_only() && virtio_gpu_modeset == -1)
- return -EINVAL;
-
- if (virtio_gpu_modeset == 0)
- return -EINVAL;
+ if ((drm_firmware_drivers_only() && virtio_gpu_modeset == -1) ||
+ (virtio_gpu_modeset == 0))
+ driver.driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
The kernel-wide option 'nomodeset' affects system behavior. It's a
misnomer, as it actually means 'don't replace the firmware-provided
framebuffer.' So if you just set these flags here, virtio-gpu would
later remove the firmware driver via aperture helpers. Therefore, if
drm_formware_drivers_only() returns true, we should fail probing with
-ENODEV.
But we could try to repurpose the module's 'modeset' option. It's
already obsoleted by nomodeset anyway. I'd try to make modeset it a
boolean that controls modesetting vs render-only. It will then be about
the driver's feature set, rather than system behavior.
Best regards
Thomas
/*
* The virtio-gpu device is a virtual device that doesn't have DMA
@@ -98,11 +97,19 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
if (ret)
goto err_free;
+ if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+ vgdev = dev->dev_private;
+ ret = virtio_gpu_modeset_init(vgdev);
+ if (ret)
+ goto err_deinit;
+ }
+
ret = drm_dev_register(dev, 0);
Re: [PATCH vhost 07/10] virtio_ring: add api virtio_dma_map() for advance dma
On Tue, 21 Feb 2023 09:51:07 +0800, Jason Wang wrote:
> On Mon, Feb 20, 2023 at 3:02 PM Xuan Zhuo wrote:
> >
> > On Mon, 20 Feb 2023 13:38:24 +0800, Jason Wang wrote:
> > > On Tue, Feb 14, 2023 at 3:27 PM Xuan Zhuo
> > > wrote:
> > > >
> > > > Added virtio_dma_map() to map DMA addresses for virtual memory in
> > > > advance. The purpose is to keep memory mapped across multiple add/get
> > > > buf operations.
> > >
> > > I wonder if instead of exporting helpers like this, it might be simple
> > > to just export dma_dev then the upper layer can use DMA API at will?
> >
> >
> > The reason for not doing this, Virtio is not just using DMA_DEV to mapp, but
> > also check whether DMA is used.
>
> We should let the DMA API decide by exporting a correct dma_dev. E.g
> when ACCESS_PLATFORM is not negotiated, advertising a DMA dev without
> dma_ops.
Do you mean we provide this API?
virtio_get_dma_dev()
If it returns NULL, the caller will use the physical memory address directly. If
this func return a dma_dev, the caller should use DMA API.
Thanks.
>
> Thanks
>
> >
> >
> > >
> > > (Otherwise the DMA helpers need to grow/shrink as the DMA API evolves?)
> > >
> > > >
> > > > Added virtio_dma_unmap() for unmap DMA address.
> > > >
> > > > Signed-off-by: Xuan Zhuo
> > > > ---
> > > > drivers/virtio/virtio_ring.c | 92
> > > > include/linux/virtio.h | 9
> > > > 2 files changed, 101 insertions(+)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index cd9364eb2345..855338609c7f 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -3172,4 +3172,96 @@ const struct vring *virtqueue_get_vring(struct
> > > > virtqueue *vq)
> > > > }
> > > > EXPORT_SYMBOL_GPL(virtqueue_get_vring);
> > > >
> > > > +/**
> > > > + * virtio_dma_map_page - get the DMA addr of the memory for virtio
> > > > device
> > > > + * @dev: virtio device
> > > > + * @page: the page of the memory to DMA
> > > > + * @offset: the offset of the memory inside page
> > > > + * @length: memory length
> > > > + * @dir: DMA direction
> > > > + *
> > > > + * This API is only for pre-mapped buffers, for non premapped buffers
> > > > virtio
> > > > + * core handles DMA API internally.
> > > > + *
> > > > + * Returns the DMA addr. DMA_MAPPING_ERROR means error.
> > > > + */
> > > > +dma_addr_t virtio_dma_map_page(struct device *dev, struct page *page,
> > > > size_t offset,
> > > > + unsigned int length, enum
> > > > dma_data_direction dir)
> > > > +{
> > >
> > > This (and the reset) needs to be done per virtqueue instead per device
> > > after b0e504e5505d184b0be248b7dcdbe50b79f03758 ("virtio_ring: per
> > > virtqueue dma device").
> >
> >
> > YES.
> >
> >
> > >
> > > > + struct virtio_device *vdev = dev_to_virtio(dev);
> > > > +
> > > > + if (!vring_use_dma_api(vdev))
> > > > + return page_to_phys(page) + offset;
> > > > +
> > > > + return dma_map_page(vdev->dev.parent, page, offset, length,
> > > > dir);
> > > > +}
> > >
> > > Need either inline or EXPORT_SYMBOL_GPL() here.
> >
> > Because I did not use this interface, I did not export it.
> >
> > Thanks.
> >
> >
> > >
> > > Thanks
> > >
> > >
> > > > +
> > > > +/**
> > > > + * virtio_dma_map - get the DMA addr of the memory for virtio device
> > > > + * @dev: virtio device
> > > > + * @addr: the addr to DMA
> > > > + * @length: memory length
> > > > + * @dir: DMA direction
> > > > + *
> > > > + * This API is only for pre-mapped buffers, for non premapped buffers
> > > > virtio
> > > > + * core handles DMA API internally.
> > > > + *
> > > > + * Returns the DMA addr.
> > > > + */
> > > > +dma_addr_t virtio_dma_map(struct device *dev, void *addr, unsigned int
> > > > length,
> > > > + enum dma_data_direction dir)
> > > > +{
> > > > + struct page *page;
> > > > + size_t offset;
> > > > +
> > > > + page = virt_to_page(addr);
> > > > + offset = offset_in_page(addr);
> > > > +
> > > > + return virtio_dma_map_page(dev, page, offset, length, dir);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(virtio_dma_map);
> > > > +
> > > > +/**
> > > > + * virtio_dma_mapping_error - check dma address
> > > > + * @dev: virtio device
> > > > + * @addr: DMA address
> > > > + *
> > > > + * This API is only for pre-mapped buffers, for non premapped buffers
> > > > virtio
> > > > + * core handles DMA API internally.
> > > > + *
> > > > + * Returns 0 means dma valid. Other means invalid dma address.
> > > > + */
> > > > +int virtio_dma_mapping_error(struct device *dev, dma_addr_t addr)
> > > > +{
> > > > + struct virtio_device *vdev = dev_to_virtio(dev);
> > > > +
> > > > + if (!vring_use_dma_api(vdev))
> > > > + return 0;
> > > > +
> > > > + return dma_mapping_error(vdev->dev.parent, addr);
> > > > +}
> > > >
Re: [PATCH v3 05/11] vduse: Support automatic irq callback affinity
Hi Xie,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tip/irq/core]
[also build test WARNING on linus/master next-20230228]
[cannot apply to mst-vhost/linux-next v6.2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Xie-Yongji/lib-group_cpus-Export-group_cpus_evenly/20230228-174438
patch link:
https://lore.kernel.org/r/20230228094110.37-6-xieyongji%40bytedance.com
patch subject: [PATCH v3 05/11] vduse: Support automatic irq callback affinity
config: m68k-allyesconfig
(https://download.01.org/0day-ci/archive/20230228/202302281954.jra7qzq4-...@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/intel-lab-lkp/linux/commit/6c15cc28cb814c0e6cb80955bc59517e80c15ae2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review
Xie-Yongji/lib-group_cpus-Export-group_cpus_evenly/20230228-174438
git checkout 6c15cc28cb814c0e6cb80955bc59517e80c15ae2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1
O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1
O=build_dir ARCH=m68k SHELL=/bin/bash drivers/vdpa/vdpa_user/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot
| Link:
https://lore.kernel.org/oe-kbuild-all/202302281954.jra7qzq4-...@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/vdpa/vdpa_user/vduse_dev.c:725:1: warning: no previous prototype for
>> 'create_affinity_masks' [-Wmissing-prototypes]
725 | create_affinity_masks(unsigned int nvecs, struct irq_affinity *affd)
| ^
vim +/create_affinity_masks +725 drivers/vdpa/vdpa_user/vduse_dev.c
723
724 struct cpumask *
> 725 create_affinity_masks(unsigned int nvecs, struct irq_affinity *affd)
726 {
727 unsigned int affvecs = 0, curvec, usedvecs, i;
728 struct cpumask *masks = NULL;
729
730 if (nvecs > affd->pre_vectors + affd->post_vectors)
731 affvecs = nvecs - affd->pre_vectors -
affd->post_vectors;
732
733 if (!affd->calc_sets)
734 affd->calc_sets = default_calc_sets;
735
736 affd->calc_sets(affd, affvecs);
737
738 if (!affvecs)
739 return NULL;
740
741 masks = kcalloc(nvecs, sizeof(*masks), GFP_KERNEL);
742 if (!masks)
743 return NULL;
744
745 /* Fill out vectors at the beginning that don't need affinity */
746 for (curvec = 0; curvec < affd->pre_vectors; curvec++)
747 cpumask_setall([curvec]);
748
749 for (i = 0, usedvecs = 0; i < affd->nr_sets; i++) {
750 unsigned int this_vecs = affd->set_size[i];
751 int j;
752 struct cpumask *result = group_cpus_evenly(this_vecs);
753
754 if (!result) {
755 kfree(masks);
756 return NULL;
757 }
758
759 for (j = 0; j < this_vecs; j++)
760 cpumask_copy([curvec + j], [j]);
761 kfree(result);
762
763 curvec += this_vecs;
764 usedvecs += this_vecs;
765 }
766
767 /* Fill out vectors at the end that don't need affinity */
768 if (usedvecs >= affvecs)
769 curvec = affd->pre_vectors + affvecs;
770 else
771 curvec = affd->pre_vectors + usedvecs;
772 for (; curvec < nvecs; curvec++)
773 cpumask_setall([curvec]);
774
775 return masks;
776 }
777
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] vdpa_sim: set last_used_idx as last_avail_idx in vdpasim_queue_ready
On Fri, Feb 03, 2023 at 03:25:01PM +0100, Eugenio Pérez wrote:
Starting from an used_idx different than 0 is needed in use cases like
virtual machine migration. Not doing so and letting the caller set an
avail idx different than 0 causes destination device to try to use old
buffers that source driver already recover and are not available
anymore.
Since vdpa_sim does not support receive inflight descriptors as a
destination of a migration, let's set both avail_idx and used_idx the
same at vq start. This is how vhost-user works in a
VHOST_SET_VRING_BASE call.
Although the simple fix is to set last_used_idx at vdpasim_set_vq_state,
it would be reset at vdpasim_queue_ready. The last_avail_idx case is
fixed with commit a09f493c ("vdpa_sim: not reset state in
vdpasim_queue_ready"). Since the only option is to make it equal to
last_avail_idx, adding the only change needed here.
This was discovered and tested live migrating the vdpa_sim_net device.
Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
Signed-off-by: Eugenio Pérez
---
Cherry-picked from patch 2/2 of the series [1]. Differences are:
* Set the value of used_idx at vdpasim_queue_ready instead of fetching
from the guest vring like vhost-kernel.
v2: Actually update last_used_idx only at vdpasim_queue_ready.
Note that commit id present in the patch text is not in master but in
git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git.
[1] https://lkml.org/lkml/2023/1/18/1041
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 6a0a65814626..79ac585e40b9 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -68,6 +68,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim,
unsigned int idx)
(uintptr_t)vq->device_addr);
vq->vring.last_avail_idx = last_avail_idx;
+ vq->vring.last_used_idx = last_avail_idx;
vq->vring.notify = vdpasim_vq_notify;
}
-- 2.31.1
If you need to resend, I'd add a comment in the code following the
commit description.
Anyway, the patch LGTM:
Reviewed-by: Stefano Garzarella
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v1 12/12] test/vsock: MSG_ZEROCOPY support for vsock_perf
On Mon, Feb 20, 2023 at 09:05:12AM +, Krasnov Arseniy wrote: On 16.02.2023 18:29, Stefano Garzarella wrote: On Mon, Feb 06, 2023 at 07:06:32AM +, Arseniy Krasnov wrote: To use this option pass '--zc' parameter: --zerocopy or --zero-copy maybe better follow what we did with the other parameters :-) ./vsock_perf --zc --sender --port --bytes With this option MSG_ZEROCOPY flag will be passed to the 'send()' call. Signed-off-by: Arseniy Krasnov --- tools/testing/vsock/vsock_perf.c | 127 +-- 1 file changed, 120 insertions(+), 7 deletions(-) diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c index a72520338f84..1d435be9b48e 100644 --- a/tools/testing/vsock/vsock_perf.c +++ b/tools/testing/vsock/vsock_perf.c @@ -18,6 +18,8 @@ #include #include #include +#include +#include #define DEFAULT_BUF_SIZE_BYTES (128 * 1024) #define DEFAULT_TO_SEND_BYTES (64 * 1024) @@ -28,9 +30,14 @@ #define BYTES_PER_GB (1024 * 1024 * 1024ULL) #define NSEC_PER_SEC (10ULL) +#ifndef SOL_VSOCK +#define SOL_VSOCK 287 +#endif I thought we use the current kernel headers when we compile the tests, do we need to fix something in the makefile? Not sure, of course we are using uapi. But i see, that defines like SOL_XXX is not defined in uapi headers. For example SOL_IP is defined in include/linux/socket.h, but userspace app uses SOL_IP from in.h (at least on my machine). E.g. SOL_XXX is not exported to user. Right, I see only few SOL_* in the uapi, e.g. SOL_TIPC in uapi/linux/tipc.h So it's fine for now, otherwise we can define it in uapi/linux/vm_sockets.h Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v1 07/12] vsock/virtio: MGS_ZEROCOPY flag support
On Mon, Feb 20, 2023 at 09:04:04AM +, Krasnov Arseniy wrote:
On 16.02.2023 18:16, Stefano Garzarella wrote:
On Mon, Feb 06, 2023 at 07:00:35AM +, Arseniy Krasnov wrote:
This adds main logic of MSG_ZEROCOPY flag processing for packet
creation. When this flag is set and user's iov iterator fits for
zerocopy transmission, call 'get_user_pages()' and add returned
pages to the newly created skb.
Signed-off-by: Arseniy Krasnov
---
net/vmw_vsock/virtio_transport_common.c | 212 ++--
1 file changed, 195 insertions(+), 17 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c
b/net/vmw_vsock/virtio_transport_common.c
index 05ce97b967ad..69e37f8a68a6 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -37,6 +37,169 @@ virtio_transport_get_ops(struct vsock_sock *vsk)
return container_of(t, struct virtio_transport, transport);
}
I'd use bool if we don't need to return an error value in the following
new functions.
+static int virtio_transport_can_zcopy(struct iov_iter *iov_iter,
+ size_t free_space)
+{
+ size_t pages;
+ int i;
+
+ if (!iter_is_iovec(iov_iter))
+ return -1;
+
+ if (iov_iter->iov_offset)
+ return -1;
+
+ /* We can't send whole iov. */
+ if (free_space < iov_iter->count)
+ return -1;
+
+ for (pages = 0, i = 0; i < iov_iter->nr_segs; i++) {
+ const struct iovec *iovec;
+ int pages_in_elem;
+
+ iovec = _iter->iov[i];
+
+ /* Base must be page aligned. */
+ if (offset_in_page(iovec->iov_base))
+ return -1;
+
+ /* Only last element could have not page aligned size. */
+ if (i != (iov_iter->nr_segs - 1)) {
+ if (offset_in_page(iovec->iov_len))
+ return -1;
+
+ pages_in_elem = iovec->iov_len >> PAGE_SHIFT;
+ } else {
+ pages_in_elem = round_up(iovec->iov_len, PAGE_SIZE);
+ pages_in_elem >>= PAGE_SHIFT;
+ }
+
+ /* In case of user's pages - one page is one frag. */
+ if (pages + pages_in_elem > MAX_SKB_FRAGS)
+ return -1;
+
+ pages += pages_in_elem;
+ }
+
+ return 0;
+}
+
+static int virtio_transport_init_zcopy_skb(struct vsock_sock *vsk,
+ struct sk_buff *skb,
+ struct iov_iter *iter,
+ bool zerocopy)
+{
+ struct ubuf_info_msgzc *uarg_zc;
+ struct ubuf_info *uarg;
+
+ uarg = msg_zerocopy_realloc(sk_vsock(vsk),
+ iov_length(iter->iov, iter->nr_segs),
+ NULL);
+
+ if (!uarg)
+ return -1;
+
+ uarg_zc = uarg_to_msgzc(uarg);
+ uarg_zc->zerocopy = zerocopy ? 1 : 0;
+
+ skb_zcopy_init(skb, uarg);
+
+ return 0;
+}
+
+static int virtio_transport_fill_nonlinear_skb(struct sk_buff *skb,
+ struct vsock_sock *vsk,
+ struct virtio_vsock_pkt_info *info)
+{
+ struct iov_iter *iter;
+ int frag_idx;
+ int seg_idx;
+
+ iter = >msg->msg_iter;
+ frag_idx = 0;
+ VIRTIO_VSOCK_SKB_CB(skb)->curr_frag = 0;
+ VIRTIO_VSOCK_SKB_CB(skb)->frag_off = 0;
+
+ /* At this moment:
+ * 1) 'iov_offset' is zero.
+ * 2) Every 'iov_base' and 'iov_len' are also page aligned
+ * (except length of the last element).
+ * 3) Number of pages in this iov <= MAX_SKB_FRAGS.
+ * 4) Length of the data fits in current credit space.
+ */
+ for (seg_idx = 0; seg_idx < iter->nr_segs; seg_idx++) {
+ struct page *user_pages[MAX_SKB_FRAGS];
+ const struct iovec *iovec;
+ size_t last_frag_len;
+ size_t pages_in_seg;
+ int page_idx;
+
+ iovec = >iov[seg_idx];
+ pages_in_seg = iovec->iov_len >> PAGE_SHIFT;
+
+ if (iovec->iov_len % PAGE_SIZE) {
+ last_frag_len = iovec->iov_len % PAGE_SIZE;
+ pages_in_seg++;
+ } else {
+ last_frag_len = PAGE_SIZE;
+ }
+
+ if (get_user_pages((unsigned long)iovec->iov_base,
+ pages_in_seg, FOLL_GET, user_pages,
+ NULL) != pages_in_seg)
+ return -1;
Reading the get_user_pages() documentation, this should pin the user
pages, so we should be fine if we then expose them in the virtqueue.
But reading Documentation/core-api/pin_user_pages.rst it seems that
drivers should use "pin_user_pages*() for DMA-pinned pages", so I'm not
sure what we should do.
That is really interesting question for me too. IIUC 'pin_user_pages()'
sets special value to ref counter of page, so we can distinguish such
pages from the others. I've grepped for pinned pages check and found,
the it is used in mm/vmscan.c by calling 'folio_maybe_dma_pinned()' during
page lists processing. Seems 'pin_user_pages()' is more strict version of
'get_user_pages()' and it is recommended to use 'pin_' when data on these
pages will be accessed.
I think, i'll check
Re: [RFC PATCH v1 00/12] vsock: MSG_ZEROCOPY flag support
On Mon, Feb 20, 2023 at 08:59:32AM +, Krasnov Arseniy wrote: On 16.02.2023 16:33, Stefano Garzarella wrote: Hi Arseniy, sorry for the delay, but I was offline. Hello! Sure no problem!, i was also offline a little bit so i'm replying just now On Mon, Feb 06, 2023 at 06:51:55AM +, Arseniy Krasnov wrote: Hello, DESCRIPTION this is MSG_ZEROCOPY feature support for virtio/vsock. I tried to follow current implementation for TCP as much as possible: 1) Sender must enable SO_ZEROCOPY flag to use this feature. Without this flag, data will be sent in "classic" copy manner and MSG_ZEROCOPY flag will be ignored (e.g. without completion). 2) Kernel uses completions from socket's error queue. Single completion for single tx syscall (or it can merge several completions to single one). I used already implemented logic for MSG_ZEROCOPY support: 'msg_zerocopy_realloc()' etc. I will review for the vsock point of view. Hope some net maintainers can comment about SO_ZEROCOPY. Anyway I think is a good idea to keep it as close as possible to the TCP implementation. Difference with copy way is not significant. During packet allocation, non-linear skb is created, then I call 'get_user_pages()' for each page from user's iov iterator (I think i don't need 'pin_user_pages()' as Are these pages exposed to the host via virtqueues? If so, I think we should pin them. What happens if the host accesses them but these pages have been unmapped? Yes, user pages with data will be used by the virtio device. 'pin' - You mean use 'pin_user_pages()'? Unmapped - You mean guest will unmap it, while host must access it to copy packet? Such pages have incremented refcount by 'get_user_pages()', so page is locked and memory and will be locked until host finishes copying data. Yep, I got it while reviewing patch 7 ;-) I think it is better to discuss things related to 'get/pin_user_pages()' in one place, for example in 07/12 where this function is called. Agree! there is no backing storage for these pages) and add each returned page to the skb as fragment. There are also some updates for vhost and guest parts of transport - in both cases i've added handling of non-linear skb for virtio part. vhost copies data from such skb to the guest's rx virtio buffers. In the guest, virtio transport fills virtio queue with pages from skb. I think doc in Documentation/networking/msg_zerocopy.rst could be also updated in next versions. Yep, good idea. Ack, i'll do it in v2. This version has several limits/problems: 1) As this feature totally depends on transport, there is no way (or it is difficult) to check whether transport is able to handle it or not during SO_ZEROCOPY setting. Seems I need to call AF_VSOCK specific setsockopt callback from setsockopt callback for SOL_SOCKET, but this leads to lock problem, because both AF_VSOCK and SOL_SOCKET callback are not considered to be called from each other. So in current version SO_ZEROCOPY is set successfully to any type (e.g. transport) of AF_VSOCK socket, but if transport does not support MSG_ZEROCOPY, tx routine will fail with EOPNOTSUPP. I'll take a look, but if we have no alternative, I think it's okay to make tx fail.> Thanks 2) When MSG_ZEROCOPY is used, for each tx system call we need to enqueue one completion. In each completion there is flag which shows how tx was performed: zerocopy or copy. This leads that whole message must be send in zerocopy or copy way - we can't send part of message with copying and rest of message with zerocopy mode (or vice versa). Now, we need to account vsock credit logic, e.g. we can't send whole data once - only allowed number of bytes could sent at any moment. In case of copying way there is no problem as in worst case we can send single bytes, but zerocopy is more complex because smallest transmission unit is single page. So if there is not enough space at peer's side to send integer number of pages (at least one) - we will wait, thus stalling tx side. To overcome this problem i've added simple rule - zerocopy is possible only when there is enough space at another side for whole message (to check, that current 'msghdr' was already used in previous tx iterations i use 'iov_offset' field of it's iov iter). I see the problem and I think your approach is the right one. 3) loopback transport is not supported, because it requires to implement non-linear skb handling in dequeue logic (as we "send" fragged skb and "receive" it from the same queue). I'm going to implement it in next versions. loopback is useful for testing and debugging, so it would be great to have the support, but if it's too complicated, we can do it later. Ok, i'll implement it in v2. 4) Current implementation sets max length of packet to 64KB. IIUC this is due to 'kmalloc()' allocated data buffers. I think, in case of Yep, I think so. When I started
Re: [PATCH v2] virtio-net: Fix probe of virtio-net on kvmtool
On Tue, 28 Feb 2023 18:06:38 +0800, Xuan Zhuo
wrote:
> On Fri, 24 Feb 2023 11:11:37 +0800, Jason Wang wrote:
> > On Fri, Feb 24, 2023 at 3:38 AM Rob Bradford via B4 Relay
> > wrote:
> > >
> > > From: Rob Bradford
> > >
> > > kvmtool does not support the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature
> > > but does advertise the VIRTIO_NET_F_GUEST_TSO{4,6} features. Check that
> > > the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature is present before setting
> > > the NETIF_F_GRO_HW feature bit as otherwise
>
> Here are settings for dev->features and dev->hw_features.
What I want to say is that in normal circumstances, ethtool will identify it and
will not directly modify the backend, if there is no
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
Thanks.
>
>
> > > an attempt will be made to
> > > program the virtio-net device using the ctrl queue which will fail.
> > >
> > > This resolves the following error when running on kvmtool:
>
> Can you talk about it in detail what it did?
>
> Thanks.
>
> > >
> > > [1.865992] net eth0: Fail to set guest offload.
> > > [1.872491] virtio_net virtio2 eth0: set_features() failed (-22);
> > > wanted 0x00134829, left 0x008000134829
> > >
> > > Signed-off-by: Rob Bradford
> > > ---
> > > Changes in v2:
> > > - Use parentheses to group logical OR of features
> > > - Link to v1:
> > >
> > > https://lore.kernel.org/r/20230223-virtio-net-kvmtool-v1-1-fc23d29b9...@rivosinc.com
> > > ---
> > > drivers/net/virtio_net.c | 7 +++
> > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 61e33e4dd0cd..f8341d1a4ccd 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -3780,10 +3780,9 @@ static int virtnet_probe(struct virtio_device
> > > *vdev)
> > > }
> > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> > > dev->features |= NETIF_F_RXCSUM;
> > > - if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> > > - virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
> > > - dev->features |= NETIF_F_GRO_HW;
> > > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > + if ((virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> > > + virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) &&
> > > + virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > dev->hw_features |= NETIF_F_GRO_HW;
> >
> > Does this mean we won't have NETIF_F_GRO_HW when only TSO4/TSO6 are
> > supported but not GUEST_OFFLOADS?
> >
> > Is this intended?
> >
> > Thanks
> >
> > >
> > > dev->vlan_features = dev->features;
> > >
> > > ---
> > > base-commit: c39cea6f38eefe356d64d0bc1e1f2267e282cdd3
> > > change-id: 20230223-virtio-net-kvmtool-87f37515be22
> > >
> > > Best regards,
> > > --
> > > Rob Bradford
> > >
> >
> > ___
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio-net: Fix probe of virtio-net on kvmtool
On Fri, 24 Feb 2023 11:11:37 +0800, Jason Wang wrote:
> On Fri, Feb 24, 2023 at 3:38 AM Rob Bradford via B4 Relay
> wrote:
> >
> > From: Rob Bradford
> >
> > kvmtool does not support the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature
> > but does advertise the VIRTIO_NET_F_GUEST_TSO{4,6} features. Check that
> > the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature is present before setting
> > the NETIF_F_GRO_HW feature bit as otherwise
Here are settings for dev->features and dev->hw_features.
> > an attempt will be made to
> > program the virtio-net device using the ctrl queue which will fail.
> >
> > This resolves the following error when running on kvmtool:
Can you talk about it in detail what it did?
Thanks.
> >
> > [1.865992] net eth0: Fail to set guest offload.
> > [1.872491] virtio_net virtio2 eth0: set_features() failed (-22); wanted
> > 0x00134829, left 0x008000134829
> >
> > Signed-off-by: Rob Bradford
> > ---
> > Changes in v2:
> > - Use parentheses to group logical OR of features
> > - Link to v1:
> >
> > https://lore.kernel.org/r/20230223-virtio-net-kvmtool-v1-1-fc23d29b9...@rivosinc.com
> > ---
> > drivers/net/virtio_net.c | 7 +++
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 61e33e4dd0cd..f8341d1a4ccd 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -3780,10 +3780,9 @@ static int virtnet_probe(struct virtio_device *vdev)
> > }
> > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> > dev->features |= NETIF_F_RXCSUM;
> > - if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> > - virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
> > - dev->features |= NETIF_F_GRO_HW;
> > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > + if ((virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> > + virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) &&
> > + virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > dev->hw_features |= NETIF_F_GRO_HW;
>
> Does this mean we won't have NETIF_F_GRO_HW when only TSO4/TSO6 are
> supported but not GUEST_OFFLOADS?
>
> Is this intended?
>
> Thanks
>
> >
> > dev->vlan_features = dev->features;
> >
> > ---
> > base-commit: c39cea6f38eefe356d64d0bc1e1f2267e282cdd3
> > change-id: 20230223-virtio-net-kvmtool-87f37515be22
> >
> > Best regards,
> > --
> > Rob Bradford
> >
>
> ___
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
