Re: [PATCH 3/6] drm/qxl: Create mouse hotspot properties on cursor planes
Hi Zack,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-exynos/exynos-drm-next drm-intel/for-linux-next
drm-tip/drm-tip v5.18 next-20220602]
[cannot apply to airlied/drm-next tegra-drm/drm/tegra/for-next]
[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]
url:
https://github.com/intel-lab-lkp/linux/commits/Zack-Rusin/drm-Add-mouse-cursor-hotspot-support-to-atomic-KMS/20220602-234633
base: git://anongit.freedesktop.org/drm/drm drm-next
config: i386-randconfig-a013
(https://download.01.org/0day-ci/archive/20220603/202206030750.8hv8vdba-...@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project
b364c76683f8ef241025a9556300778c07b590c2)
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/0bf2395ee17bd25ae6411c560de883496256195d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review
Zack-Rusin/drm-Add-mouse-cursor-hotspot-support-to-atomic-KMS/20220602-234633
git checkout 0bf2395ee17bd25ae6411c560de883496256195d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/qxl/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/qxl/qxl_display.c:486:26: warning: unused variable 'fb'
>> [-Wunused-variable]
struct drm_framebuffer *fb = plane_state->fb;
^
drivers/gpu/drm/qxl/qxl_display.c:532:26: warning: unused variable 'fb'
[-Wunused-variable]
struct drm_framebuffer *fb = plane_state->fb;
^
2 warnings generated.
vim +/fb +486 drivers/gpu/drm/qxl/qxl_display.c
c2ff663260fee3 Gabriel Krisman Bertazi 2017-02-27 482
b4b27f08f9f96d Gerd Hoffmann 2021-02-17 483 static int
qxl_primary_apply_cursor(struct qxl_device *qdev,
b4b27f08f9f96d Gerd Hoffmann 2021-02-17 484
struct drm_plane_state *plane_state)
9428088c90b6f7 Ray Strode 2017-11-27 485 {
b4b27f08f9f96d Gerd Hoffmann 2021-02-17 @486 struct
drm_framebuffer *fb = plane_state->fb;
b4b27f08f9f96d Gerd Hoffmann 2021-02-17 487 struct qxl_crtc
*qcrtc = to_qxl_crtc(plane_state->crtc);
9428088c90b6f7 Ray Strode 2017-11-27 488 struct
qxl_cursor_cmd *cmd;
9428088c90b6f7 Ray Strode 2017-11-27 489 struct
qxl_release *release;
9428088c90b6f7 Ray Strode 2017-11-27 490 int ret = 0;
9428088c90b6f7 Ray Strode 2017-11-27 491
9428088c90b6f7 Ray Strode 2017-11-27 492 if
(!qcrtc->cursor_bo)
9428088c90b6f7 Ray Strode 2017-11-27 493 return
0;
9428088c90b6f7 Ray Strode 2017-11-27 494
9428088c90b6f7 Ray Strode 2017-11-27 495 ret =
qxl_alloc_release_reserved(qdev, sizeof(*cmd),
9428088c90b6f7 Ray Strode 2017-11-27 496
QXL_RELEASE_CURSOR_CMD,
9428088c90b6f7 Ray Strode 2017-11-27 497
&release, NULL);
9428088c90b6f7 Ray Strode 2017-11-27 498 if (ret)
9428088c90b6f7 Ray Strode 2017-11-27 499 return
ret;
9428088c90b6f7 Ray Strode 2017-11-27 500
9428088c90b6f7 Ray Strode 2017-11-27 501 ret =
qxl_release_list_add(release, qcrtc->cursor_bo);
9428088c90b6f7 Ray Strode 2017-11-27 502 if (ret)
9428088c90b6f7 Ray Strode 2017-11-27 503 goto
out_free_release;
9428088c90b6f7 Ray Strode 2017-11-27 504
9428088c90b6f7 Ray Strode 2017-11-27 505 ret =
qxl_release_reserve_list(release, false);
9428088c90b6f7 Ray Strode 2017-11-27 506 if (ret)
9428088c90b6f7 Ray Strode 2017-11-27 507 goto
out_free_release;
9428088c90b6f7 Ray Strode 2017-11-27 508
9428088c90b6f7 Ray Strode 2017-11-27 509 cmd = (struct
qxl_cursor_cmd *)qxl_release_map(qdev, release);
9428088c90b6f7 Ray Strode 2017-11-27 510 cmd->type =
QXL_CURSOR_SET;
0bf2395ee17bd2 Zack Rusin 2022-06-02 5
Re: [PATCH 3/6] drm/qxl: Create mouse hotspot properties on cursor planes
Hi Zack,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-exynos/exynos-drm-next drm-intel/for-linux-next
drm-tip/drm-tip v5.18 next-20220602]
[cannot apply to airlied/drm-next tegra-drm/drm/tegra/for-next]
[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]
url:
https://github.com/intel-lab-lkp/linux/commits/Zack-Rusin/drm-Add-mouse-cursor-hotspot-support-to-atomic-KMS/20220602-234633
base: git://anongit.freedesktop.org/drm/drm drm-next
config: x86_64-randconfig-a011
(https://download.01.org/0day-ci/archive/20220603/202206030624.tdmarys5-...@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
#
https://github.com/intel-lab-lkp/linux/commit/0bf2395ee17bd25ae6411c560de883496256195d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review
Zack-Rusin/drm-Add-mouse-cursor-hotspot-support-to-atomic-KMS/20220602-234633
git checkout 0bf2395ee17bd25ae6411c560de883496256195d
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/qxl/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
drivers/gpu/drm/qxl/qxl_display.c: In function 'qxl_primary_apply_cursor':
>> drivers/gpu/drm/qxl/qxl_display.c:486:33: warning: unused variable 'fb'
>> [-Wunused-variable]
486 | struct drm_framebuffer *fb = plane_state->fb;
| ^~
drivers/gpu/drm/qxl/qxl_display.c: In function 'qxl_primary_move_cursor':
drivers/gpu/drm/qxl/qxl_display.c:532:33: warning: unused variable 'fb'
[-Wunused-variable]
532 | struct drm_framebuffer *fb = plane_state->fb;
| ^~
vim +/fb +486 drivers/gpu/drm/qxl/qxl_display.c
c2ff663260fee3 Gabriel Krisman Bertazi 2017-02-27 482
b4b27f08f9f96d Gerd Hoffmann 2021-02-17 483 static int
qxl_primary_apply_cursor(struct qxl_device *qdev,
b4b27f08f9f96d Gerd Hoffmann 2021-02-17 484
struct drm_plane_state *plane_state)
9428088c90b6f7 Ray Strode 2017-11-27 485 {
b4b27f08f9f96d Gerd Hoffmann 2021-02-17 @486 struct
drm_framebuffer *fb = plane_state->fb;
b4b27f08f9f96d Gerd Hoffmann 2021-02-17 487 struct qxl_crtc
*qcrtc = to_qxl_crtc(plane_state->crtc);
9428088c90b6f7 Ray Strode 2017-11-27 488 struct
qxl_cursor_cmd *cmd;
9428088c90b6f7 Ray Strode 2017-11-27 489 struct
qxl_release *release;
9428088c90b6f7 Ray Strode 2017-11-27 490 int ret = 0;
9428088c90b6f7 Ray Strode 2017-11-27 491
9428088c90b6f7 Ray Strode 2017-11-27 492 if
(!qcrtc->cursor_bo)
9428088c90b6f7 Ray Strode 2017-11-27 493 return
0;
9428088c90b6f7 Ray Strode 2017-11-27 494
9428088c90b6f7 Ray Strode 2017-11-27 495 ret =
qxl_alloc_release_reserved(qdev, sizeof(*cmd),
9428088c90b6f7 Ray Strode 2017-11-27 496
QXL_RELEASE_CURSOR_CMD,
9428088c90b6f7 Ray Strode 2017-11-27 497
&release, NULL);
9428088c90b6f7 Ray Strode 2017-11-27 498 if (ret)
9428088c90b6f7 Ray Strode 2017-11-27 499 return
ret;
9428088c90b6f7 Ray Strode 2017-11-27 500
9428088c90b6f7 Ray Strode 2017-11-27 501 ret =
qxl_release_list_add(release, qcrtc->cursor_bo);
9428088c90b6f7 Ray Strode 2017-11-27 502 if (ret)
9428088c90b6f7 Ray Strode 2017-11-27 503 goto
out_free_release;
9428088c90b6f7 Ray Strode 2017-11-27 504
9428088c90b6f7 Ray Strode 2017-11-27 505 ret =
qxl_release_reserve_list(release, false);
9428088c90b6f7 Ray Strode 2017-11-27 506 if (ret)
9428088c90b6f7 Ray Strode 2017-11-27 507 goto
out_free_release;
9428088c90b6f7 Ray Strode 2017-11-27 508
9428088c90b6f7 Ray Strode 2017-11-27 509 cmd = (struct
qxl_cursor_cmd *)qxl_release_map(qdev, release);
9428088c90b6f7 Ray Strode 2017-11-27 510 cmd->type =
QXL_CURSOR_SET;
0bf2395ee17bd2 Zack Rusin 2022-06-02 511
cmd->u.set.position.x = plane_state->crtc_x + plane_state->h
[GIT PULL] vhost,virtio,vdpa: features, fixes, cleanups
The following changes since commit 8ab2afa23bd197df47819a87f0265c0ac95c5b6a: Merge tag 'for-5.19/fbdev-1' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/linux-fbdev (2022-05-30 12:46:49 -0700) 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 bd8bb9aed56b1814784a975e2dfea12a9adcee92: vdpa: ifcvf: set pci driver data in probe (2022-06-01 02:16:38 -0400) vhost,virtio,vdpa: features, fixes, cleanups mac vlan filter and stats support in mlx5 vdpa irq hardening in virtio performance improvements in virtio crypto polling i/o support in virtio blk ASID support in vhost fixes, cleanups all over the place Signed-off-by: Michael S. Tsirkin Andrey Ryabinin (4): vhost: get rid of vhost_poll_flush() wrapper vhost_net: get rid of vhost_net_flush_vq() and extra flush calls vhost_test: remove vhost_test_flush_vq() vhost_vsock: simplify vhost_vsock_flush() Christophe JAILLET (1): virtio: pci: Fix an error handling path in vp_modern_probe() Cindy Lu (1): vdpa/vp_vdpa : add vdpa tool support in vp_vdpa Dan Carpenter (2): vdpasim: Off by one in vdpasim_set_group_asid() vhost-vdpa: return -EFAULT on copy_to_user() failure Eli Cohen (8): vdpa: Fix error logic in vdpa_nl_cmd_dev_get_doit vdpa: Add support for querying vendor statistics net/vdpa: Use readers/writers semaphore instead of vdpa_dev_mutex net/vdpa: Use readers/writers semaphore instead of cf_mutex vdpa/mlx5: Add support for reading descriptor statistics vdpa/mlx5: Use readers/writers semaphore instead of mutex vdpa/mlx5: Remove flow counter from steering vdpa/mlx5: Add RX MAC VLAN filter support Eugenio Pérez (1): vdpasim: allow to enable a vq repeatedly Gautam Dawar (19): vhost: move the backend feature bits to vhost_types.h virtio-vdpa: don't set callback if virtio doesn't need it vhost-vdpa: passing iotlb to IOMMU mapping helpers vhost-vdpa: switch to use vhost-vdpa specific IOTLB vdpa: introduce virtqueue groups vdpa: multiple address spaces support vdpa: introduce config operations for associating ASID to a virtqueue group vhost_iotlb: split out IOTLB initialization vhost: support ASID in IOTLB API vhost-vdpa: introduce asid based IOTLB vhost-vdpa: introduce uAPI to get the number of virtqueue groups vhost-vdpa: introduce uAPI to get the number of address spaces vhost-vdpa: uAPI to get virtqueue group id vhost-vdpa: introduce uAPI to set group ASID vhost-vdpa: support ASID based IOTLB API vdpa_sim: advertise VIRTIO_NET_F_MTU vdpa_sim: factor out buffer completion logic vdpa_sim: filter destination mac address vdpasim: control virtqueue support Jason Wang (9): virtio: use virtio_reset_device() when possible virtio: introduce config op to synchronize vring callbacks virtio-pci: implement synchronize_cbs() virtio-mmio: implement synchronize_cbs() virtio-ccw: implement synchronize_cbs() virtio: allow to unbreak virtqueue virtio: harden vring IRQ virtio: use WARN_ON() to warning illegal status value vdpa: ifcvf: set pci driver data in probe Mike Christie (4): vhost: flush dev once during vhost_dev_stop vhost-scsi: drop flush after vhost_dev_cleanup vhost-test: drop flush after vhost_dev_cleanup vhost: rename vhost_work_dev_flush Murilo Opsfelder Araujo (1): virtio-pci: Remove wrong address verification in vp_del_vqs() Solomon Tan (2): virtio: Replace unsigned with unsigned int virtio: Replace long long int with long long Stefano Garzarella (1): virtio: use virtio_device_ready() in virtio_device_restore() Suwan Kim (2): virtio-blk: support polling I/O virtio-blk: support mq_ops->queue_rqs() Xianting Tian (2): virtio_ring: remove unnecessary to_vvq call in vring hot path virtio_ring: add unlikely annotation for free descs check Zhu Lingshan (1): vDPA/ifcvf: fix uninitialized config_vector warning keliu (1): virtio: Directly use ida_alloc()/free() lei he (2): virtio-crypto: adjust dst_len at ops callback virtio-crypto: enable retry for virtio-crypto-dev zhenwei pi (3): virtio-crypto: change code style virtio-crypto: use private buffer for control request virtio-crypto: wait ctrl queue instead of busy polling drivers/block/virtio_blk.c | 224 +- .../crypto/virtio/virtio_crypto_akcipher_algs.c| 95 ++-- drivers/crypto/virtio/virtio_crypto_common.h | 21 +- drivers/crypto/virtio/virtio_crypto_core.c | 55 ++- .../crypto/virtio/virtio_crypto_skcipher_algs.c| 138 +++--- d
Re: [PATCH v4] x86/vmware: use BIT() macro for shifting
On 6/1/22 3:18 AM, Shreenidhi Shedi wrote:
> From: Shreenidhi Shedi
>
> Using BIT() macro improves readability & it uses unsigned long for
> shifting which is an added advantage.
>
> Kernel builds with -fno-strict-overflow CFLAG hence shifting a signed
> integer by 31 bits is not an issue in this case.
>
> Signed-off-by: Shreenidhi Shedi
> ---
Looks good to me.
Reviewed-by: Srivatsa S. Bhat (VMware)
> arch/x86/kernel/cpu/vmware.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> index c04b933f4..02039ec35 100644
> --- a/arch/x86/kernel/cpu/vmware.c
> +++ b/arch/x86/kernel/cpu/vmware.c
> @@ -476,8 +476,8 @@ static bool __init vmware_legacy_x2apic_available(void)
> {
> uint32_t eax, ebx, ecx, edx;
> VMWARE_CMD(GETVCPU_INFO, eax, ebx, ecx, edx);
> - return (eax & (1 << VMWARE_CMD_VCPU_RESERVED)) == 0 &&
> -(eax & (1 << VMWARE_CMD_LEGACY_X2APIC)) != 0;
> + return !(eax & BIT(VMWARE_CMD_VCPU_RESERVED)) &&
> + (eax & BIT(VMWARE_CMD_LEGACY_X2APIC));
> }
>
> #ifdef CONFIG_AMD_MEM_ENCRYPT
> --
> 2.36.1
>
Regards,
Srivatsa
VMware Photon OS
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] vringh: Fix loop descriptors check in the indirect cases
On Thu, Jun 02, 2022 at 12:55:50PM +0800, Yongji Xie wrote:
> Ping.
Thanks for the reminder!
Will queue for rc2, rc1 has too much stuff already.
> On Tue, May 10, 2022 at 3:56 PM Jason Wang wrote:
> >
> > On Tue, May 10, 2022 at 3:54 PM Yongji Xie wrote:
> > >
> > > On Tue, May 10, 2022 at 3:44 PM Jason Wang wrote:
> > > >
> > > > On Thu, May 5, 2022 at 6:08 PM Xie Yongji
> > > > wrote:
> > > > >
> > > > > We should use size of descriptor chain to test loop condition
> > > > > in the indirect case. And another statistical count is also introduced
> > > > > for indirect descriptors to avoid conflict with the statistical count
> > > > > of direct descriptors.
> > > > >
> > > > > Fixes: f87d0fbb5798 ("vringh: host-side implementation of virtio
> > > > > rings.")
> > > > > Signed-off-by: Xie Yongji
> > > > > Signed-off-by: Fam Zheng
> > > > > ---
> > > > > drivers/vhost/vringh.c | 10 --
> > > > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > > > > index 14e2043d7685..eab55accf381 100644
> > > > > --- a/drivers/vhost/vringh.c
> > > > > +++ b/drivers/vhost/vringh.c
> > > > > @@ -292,7 +292,7 @@ __vringh_iov(struct vringh *vrh, u16 i,
> > > > > int (*copy)(const struct vringh *vrh,
> > > > > void *dst, const void *src, size_t len))
> > > > > {
> > > > > - int err, count = 0, up_next, desc_max;
> > > > > + int err, count = 0, indirect_count = 0, up_next, desc_max;
> > > > > struct vring_desc desc, *descs;
> > > > > struct vringh_range range = { -1ULL, 0 }, slowrange;
> > > > > bool slow = false;
> > > > > @@ -349,7 +349,12 @@ __vringh_iov(struct vringh *vrh, u16 i,
> > > > > continue;
> > > > > }
> > > > >
> > > > > - if (count++ == vrh->vring.num) {
> > > > > + if (up_next == -1)
> > > > > + count++;
> > > > > + else
> > > > > + indirect_count++;
> > > > > +
> > > > > + if (count > vrh->vring.num || indirect_count >
> > > > > desc_max) {
> > > > > vringh_bad("Descriptor loop in %p", descs);
> > > > > err = -ELOOP;
> > > > > goto fail;
> > > > > @@ -411,6 +416,7 @@ __vringh_iov(struct vringh *vrh, u16 i,
> > > > > i = return_from_indirect(vrh,
> > > > > &up_next,
> > > > > &descs,
> > > > > &desc_max);
> > > > > slow = false;
> > > > > + indirect_count = 0;
> > > >
> > > > Do we need to reset up_next to -1 here?
> > > >
> > >
> > > It will be reset to -1 in return_from_indirect().
> >
> > Right. Then
> >
> > Acked-by: Jason Wang
> >
> > Thanks
> >
> > >
> > > Thanks,
> > > Yongji
> > >
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
On Thu, Jun 02, 2022 at 12:55:02PM +0800, Yongji Xie wrote:
> Ping.
Thanks for the reminder!
Will queue for rc2, rc1 has too much stuff already.
> On Tue, Apr 26, 2022 at 3:36 PM Xie Yongji wrote:
> >
> > The control device has no drvdata. So we will get a
> > NULL pointer dereference when accessing control
> > device's msg_timeout attribute via sysfs:
> >
> > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address:
> > 00f8
> > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show
> > (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45
> > fs/kernel_read_file.c:186)
> > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112
> > arch/x86/entry/common.c:132)
> > [ 132.881847][ T3644] entry_INT80_compat
> > (arch/x86/entry/entry_64_compat.S:419)
> >
> > To fix it, don't create the unneeded attribute for
> > control device anymore.
> >
> > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > Reported-by: kernel test robot
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Xie Yongji
> > ---
> > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> > b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index f85d1a08ed87..160e40d03084 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config
> > *config,
> >
> > dev->minor = ret;
> > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > - dev->dev = device_create(vduse_class, NULL,
> > -MKDEV(MAJOR(vduse_major), dev->minor),
> > -dev, "%s", config->name);
> > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > + MKDEV(MAJOR(vduse_major), dev->minor),
> > + dev, vduse_dev_groups, "%s", config->name);
> > if (IS_ERR(dev->dev)) {
> > ret = PTR_ERR(dev->dev);
> > goto err_dev;
> > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > return PTR_ERR(vduse_class);
> >
> > vduse_class->devnode = vduse_devnode;
> > - vduse_class->dev_groups = vduse_dev_groups;
> >
> > ret = alloc_chrdev_region(&vduse_major, 0, VDUSE_DEV_MAX, "vduse");
> > if (ret)
> > --
> > 2.20.1
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/3] recover hardware corrupted page by virtio balloon
On 02.06.22 11:28, zhenwei pi wrote: > On 6/1/22 15:59, David Hildenbrand wrote: >> On 01.06.22 04:17, zhenwei pi wrote: >>> On 5/31/22 12:08, Jue Wang wrote: On Mon, May 30, 2022 at 8:49 AM Peter Xu wrote: > > On Mon, May 30, 2022 at 07:33:35PM +0800, zhenwei pi wrote: >> A VM uses RAM of 2M huge page. Once a MCE(@HVAy in [HVAx,HVAz)) occurs, >> the >> 2M([HVAx,HVAz)) of hypervisor becomes unaccessible, but the guest >> poisons 4K >> (@GPAy in [GPAx, GPAz)) only, it may hit another 511 MCE ([GPAx, GPAz) >> except GPAy). This is the worse case, so I want to add >>'__le32 corrupted_pages' in struct virtio_balloon_config, it is used >> in the >> next step: reporting 512 * 4K 'corrupted_pages' to the guest, the guest >> has >> a chance to isolate the other 511 pages ahead of time. And the guest >> actually loses 2M, fixing 512*4K seems to help significantly. > > It sounds hackish to teach a virtio device to assume one page will always > be poisoned in huge page granule. That's only a limitation to host kernel > not virtio itself. > > E.g. there're upstream effort ongoing with enabling doublemap on hugetlbfs > pages so hugetlb pages can be mapped in 4k with it. It provides potential > possibility to do page poisoning with huge pages in 4k too. When that'll > be ready the assumption can go away, and that does sound like a better > approach towards this problem. +1. A hypervisor should always strive to minimize the guest memory loss. The HugeTLB double mapping enlightened memory poisoning behavior (only poison 4K out of a 2MB huge page and 4K in guest) is a much better solution here. To be completely transparent, it's not _strictly_ required to poison the page (whatever the granularity it is) on the host side, as long as the following are true: 1. A hypervisor can emulate the _minimized_ (e.g., 4K) the poison to the guest. 2. The host page with the UC error is "isolated" (could be PG_HWPOISON or in some other way) and prevented from being reused by other processes. For #2, PG_HWPOISON and HugeTLB double mapping enlightened memory poisoning is a good solution. > >> >>> >>> I assume when talking about "the performance memory drops a lot", you >>> imply that this patch set can mitigate that performance drop? >>> >>> But why do you see a performance drop? Because we might lose some >>> possible THP candidates (in the host or the guest) and you want to plug >>> does holes? I assume you'll see a performance drop simply because >>> poisoning memory is expensive, including migrating pages around on CE. >>> >>> If you have some numbers to share, especially before/after this change, >>> that would be great. >>> >> >> The CE storm leads 2 problems I have even seen: >> 1, the memory bandwidth slows down to 10%~20%, and the cycles per >> instruction of CPU increases a lot. >> 2, the THR (/proc/interrupts) interrupts frequently, the CPU has to use a >> lot time to handle IRQ. > > Totally no good knowledge on CMCI, but if 2) is true then I'm wondering > whether it's necessary to handle the interrupts that frequently. When I > was reading the Intel CMCI vector handler I stumbled over this comment: > > /* >* The interrupt handler. This is called on every event. >* Just call the poller directly to log any events. >* This could in theory increase the threshold under high load, >* but doesn't for now. >*/ > static void intel_threshold_interrupt(void) > > I think that matches with what I was thinking.. I mean for 2) not sure > whether it can be seen as a CMCI problem and potentially can be optimized > by adjust the cmci threshold dynamically. The CE storm caused performance drop is caused by the extra cycles spent by the ECC steps in memory controller, not in CMCI handling. This is observed in the Google fleet as well. A good solution is to monitor the CE rate closely in user space via /dev/mcelog and migrate all VMs to another host once the CE rate exceeds some threshold. CMCI is a _background_ interrupt that is not handled in the process execution context and its handler is setup to switch to poll (1 / 5 min) mode if there are more than ~ a dozen CEs reported via CMCI per second. > > -- > Peter Xu > >>> >>> Hi, Andrew, David, Naoya >>> >>> According to the suggestions, I'd give up the improvement of memory >>> failure on huge page in this series. >>> >>> Is it worth recovering corrupted pages for the guest kernel? I'd follow >>> your decision. >> >> Well, as I said, I am not sure if we really need/want this for a handful >> of 4k poisoned pages in a VM. As I suspected, doing so might prim
Re: Re: [PATCH 0/3] recover hardware corrupted page by virtio balloon
On 6/1/22 15:59, David Hildenbrand wrote: On 01.06.22 04:17, zhenwei pi wrote: On 5/31/22 12:08, Jue Wang wrote: On Mon, May 30, 2022 at 8:49 AM Peter Xu wrote: On Mon, May 30, 2022 at 07:33:35PM +0800, zhenwei pi wrote: A VM uses RAM of 2M huge page. Once a MCE(@HVAy in [HVAx,HVAz)) occurs, the 2M([HVAx,HVAz)) of hypervisor becomes unaccessible, but the guest poisons 4K (@GPAy in [GPAx, GPAz)) only, it may hit another 511 MCE ([GPAx, GPAz) except GPAy). This is the worse case, so I want to add '__le32 corrupted_pages' in struct virtio_balloon_config, it is used in the next step: reporting 512 * 4K 'corrupted_pages' to the guest, the guest has a chance to isolate the other 511 pages ahead of time. And the guest actually loses 2M, fixing 512*4K seems to help significantly. It sounds hackish to teach a virtio device to assume one page will always be poisoned in huge page granule. That's only a limitation to host kernel not virtio itself. E.g. there're upstream effort ongoing with enabling doublemap on hugetlbfs pages so hugetlb pages can be mapped in 4k with it. It provides potential possibility to do page poisoning with huge pages in 4k too. When that'll be ready the assumption can go away, and that does sound like a better approach towards this problem. +1. A hypervisor should always strive to minimize the guest memory loss. The HugeTLB double mapping enlightened memory poisoning behavior (only poison 4K out of a 2MB huge page and 4K in guest) is a much better solution here. To be completely transparent, it's not _strictly_ required to poison the page (whatever the granularity it is) on the host side, as long as the following are true: 1. A hypervisor can emulate the _minimized_ (e.g., 4K) the poison to the guest. 2. The host page with the UC error is "isolated" (could be PG_HWPOISON or in some other way) and prevented from being reused by other processes. For #2, PG_HWPOISON and HugeTLB double mapping enlightened memory poisoning is a good solution. I assume when talking about "the performance memory drops a lot", you imply that this patch set can mitigate that performance drop? But why do you see a performance drop? Because we might lose some possible THP candidates (in the host or the guest) and you want to plug does holes? I assume you'll see a performance drop simply because poisoning memory is expensive, including migrating pages around on CE. If you have some numbers to share, especially before/after this change, that would be great. The CE storm leads 2 problems I have even seen: 1, the memory bandwidth slows down to 10%~20%, and the cycles per instruction of CPU increases a lot. 2, the THR (/proc/interrupts) interrupts frequently, the CPU has to use a lot time to handle IRQ. Totally no good knowledge on CMCI, but if 2) is true then I'm wondering whether it's necessary to handle the interrupts that frequently. When I was reading the Intel CMCI vector handler I stumbled over this comment: /* * The interrupt handler. This is called on every event. * Just call the poller directly to log any events. * This could in theory increase the threshold under high load, * but doesn't for now. */ static void intel_threshold_interrupt(void) I think that matches with what I was thinking.. I mean for 2) not sure whether it can be seen as a CMCI problem and potentially can be optimized by adjust the cmci threshold dynamically. The CE storm caused performance drop is caused by the extra cycles spent by the ECC steps in memory controller, not in CMCI handling. This is observed in the Google fleet as well. A good solution is to monitor the CE rate closely in user space via /dev/mcelog and migrate all VMs to another host once the CE rate exceeds some threshold. CMCI is a _background_ interrupt that is not handled in the process execution context and its handler is setup to switch to poll (1 / 5 min) mode if there are more than ~ a dozen CEs reported via CMCI per second. -- Peter Xu Hi, Andrew, David, Naoya According to the suggestions, I'd give up the improvement of memory failure on huge page in this series. Is it worth recovering corrupted pages for the guest kernel? I'd follow your decision. Well, as I said, I am not sure if we really need/want this for a handful of 4k poisoned pages in a VM. As I suspected, doing so might primarily be interesting for some sort of de-fragmentation (allow again a higher order page to be placed at the affected PFNs), not because of the slight reduction of available memory. A simple VM reboot would get the job similarly done. Sure, Let's drop this idea. Thanks to all for the suggestions. Hi, Naoya It seems that memory failure notifier is not required currently, so I'll not push the next version of: [PATCH 1/3] memory-failure: Introduce memory failure notifier [PATCH 2/3] mm/memory-failure.c: support reset PTE during unpoison Thanks you for review work! As the poisoning refcount code is already a bit shaky as I
Re: [PATCH 6/6] vDPA: fix 'cast to restricted le16' warnings in vdpa_dev_net_config_fill()
On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan wrote: > > This commit fixes spars warnings: cast to restricted __le16 > in function vdpa_dev_net_config_fill() > > Signed-off-by: Zhu Lingshan > --- > drivers/vdpa/vdpa.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index 50a11ece603e..2719ce9962fc 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -837,11 +837,11 @@ static int vdpa_dev_net_config_fill(struct vdpa_device > *vdev, struct sk_buff *ms > config.mac)) > return -EMSGSIZE; > > - val_u16 = le16_to_cpu(config.status); > + val_u16 = le16_to_cpu((__force __le16)config.status); Can we use virtio accessors like virtio16_to_cpu()? Thanks > if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16)) > return -EMSGSIZE; > > - val_u16 = le16_to_cpu(config.mtu); > + val_u16 = le16_to_cpu((__force __le16)config.mtu); > if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) > return -EMSGSIZE; > > -- > 2.31.1 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0
On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan wrote: > > If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, > so when userspace querying queue pair numbers, it should return mq=1 > than zero Spec said: "max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ is set" So we are probably fine. Thanks > > Signed-off-by: Zhu Lingshan > --- > drivers/vdpa/vdpa.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index 030d96bdeed2..50a11ece603e 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -818,9 +818,10 @@ static int vdpa_dev_net_mq_config_fill(struct > vdpa_device *vdev, > u16 val_u16; > > if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) > - return 0; > + val_u16 = 1; > + else > + val_u16 = le16_to_cpu((__force > __le16)config->max_virtqueue_pairs); > > - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); > return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, val_u16); > } > > -- > 2.31.1 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] vDPA: !FEATURES_OK should not block querying device config space
On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan wrote:
>
> Users may want to query the config space of a vDPA device,
> to choose a appropriate one for a certain guest. This means the
> users need to read the config space before FEATURES_OK, and
> the existence of config space contents does not depend on
> FEATURES_OK.
Quotes from the spec:
"The device MUST allow reading of any device-specific configuration
field before FEATURES_OK is set by the driver. This includes fields
which are conditional on feature bits, as long as those feature bits
are offered by the device."
>
> This commit removes FEATURES_OK blocker in vdpa_dev_config_fill()
> which calls vdpa_dev_net_config_fill() for virtio-net
>
> Signed-off-by: Zhu Lingshan
> ---
> drivers/vdpa/vdpa.c | 8
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index c820dd2b0307..030d96bdeed2 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -863,17 +863,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct
> sk_buff *msg, u32 portid,
> {
> u32 device_id;
> void *hdr;
> - u8 status;
> int err;
>
> mutex_lock(&vdev->cf_mutex);
> - status = vdev->config->get_status(vdev);
> - if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> - NL_SET_ERR_MSG_MOD(extack, "Features negotiation not
> completed");
> - err = -EAGAIN;
> - goto out;
> - }
So we had the following in vdpa_dev_net_config_fill():
features = vdev->config->get_driver_features(vdev);
if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
VDPA_ATTR_PAD))
return -EMSGSIZE;
It looks to me we need to switch to using get_device_features() instead.
Thanks
> -
> hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> VDPA_CMD_DEV_CONFIG_GET);
> if (!hdr) {
> --
> 2.31.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/6] vDPA/ifcvf: support userspace to query device feature bits
On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan wrote:
>
> This commit supports userspace to query device feature bits
> by filling the relevant netlink attribute.
>
> There are two types of netlink attributes:
> VDPA_ATTR_DEV_ work for virtio devices config space, and
> VDPA_ATTR_MGMTDEV_ work for the management devices.
>
> This commit fixes a misuse of VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> this attr is for a virtio device, not management devices.
>
> Thus VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES is introduced for
> reporting management device features, and VDPA_ATTR_DEV_SUPPORTED_FEATURES
> for virtio devices feature bits.
>
> Signed-off-by: Zhu Lingshan
> ---
> drivers/vdpa/vdpa.c | 15 ++-
> include/uapi/linux/vdpa.h | 1 +
> 2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 2b75c00b1005..c820dd2b0307 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -508,7 +508,7 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev
> *mdev, struct sk_buff *m
> err = -EMSGSIZE;
> goto msg_err;
> }
> - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> + if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,
Adding Eli and Parav.
If I understand correctly, we can't provision virtio features right
now. This means, the vDPA instance should have the same features as
its parent (management device).
And it seems to me if we can do things like this, we need first allow
the features to be provisioned. (And this change breaks uABI)
Thanks
> mdev->supported_features, VDPA_ATTR_PAD)) {
> err = -EMSGSIZE;
> goto msg_err;
> @@ -827,7 +827,7 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device
> *vdev,
> static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff
> *msg)
> {
> struct virtio_net_config config = {};
> - u64 features;
> + u64 features_device, features_driver;
> u16 val_u16;
>
> vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));
> @@ -844,12 +844,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device
> *vdev, struct sk_buff *ms
> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
> return -EMSGSIZE;
>
> - features = vdev->config->get_driver_features(vdev);
> - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES,
> features,
> + features_driver = vdev->config->get_driver_features(vdev);
> + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES,
> features_driver,
> VDPA_ATTR_PAD))
> return -EMSGSIZE;
>
> - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> + features_device = vdev->config->get_device_features(vdev);
> + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> features_device,
> + VDPA_ATTR_PAD))
> + return -EMSGSIZE;
> +
> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device,
> &config);
> }
>
> static int
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 1061d8d2d09d..70a3672c288f 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -30,6 +30,7 @@ enum vdpa_attr {
> VDPA_ATTR_MGMTDEV_BUS_NAME, /* string */
> VDPA_ATTR_MGMTDEV_DEV_NAME, /* string */
> VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,/* u64 */
> + VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES, /* u64 */
>
> VDPA_ATTR_DEV_NAME, /* string */
> VDPA_ATTR_DEV_ID, /* u32 */
> --
> 2.31.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/6] vDPA/ifcvf: support userspace to query features and MQ of a management device
On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan wrote:
>
> Adapting to current netlink interfaces, this commit allows userspace
> to query feature bits and MQ capability of a management device.
>
> Signed-off-by: Zhu Lingshan
> ---
> drivers/vdpa/ifcvf/ifcvf_base.c | 12
> drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
> drivers/vdpa/ifcvf/ifcvf_main.c | 3 +++
> 3 files changed, 16 insertions(+)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 6bccc8291c26..7be703b5d1f4 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -341,6 +341,18 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16
> num)
> return 0;
> }
>
> +u16 ifcvf_get_max_vq_pairs(struct ifcvf_hw *hw)
> +{
> + struct virtio_net_config __iomem *config;
> + u16 val, mq;
> +
> + config = (struct virtio_net_config __iomem *)hw->dev_cfg;
Any reason we need the cast here? (cast from void * seems not necessary).
> + val = vp_ioread16((__le16 __iomem *)&config->max_virtqueue_pairs);
I don't see any __le16 cast for the callers of vp_ioread16, anything
make max_virtqueue_pairs different here?
Thanks
> + mq = le16_to_cpu((__force __le16)val);
> +
> + return mq;
> +}
> +
> static int ifcvf_hw_enable(struct ifcvf_hw *hw)
> {
> struct virtio_pci_common_cfg __iomem *cfg;
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index f5563f665cc6..d54a1bed212e 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -130,6 +130,7 @@ u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
> int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features);
> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
> +u16 ifcvf_get_max_vq_pairs(struct ifcvf_hw *hw);
> struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
> int ifcvf_probed_virtio_net(struct ifcvf_hw *hw);
> u32 ifcvf_get_config_size(struct ifcvf_hw *hw);
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 4366320fb68d..0c3af30b297e 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -786,6 +786,9 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev,
> const char *name,
> vf->hw_features = ifcvf_get_hw_features(vf);
> vf->config_size = ifcvf_get_config_size(vf);
>
> + ifcvf_mgmt_dev->mdev.max_supported_vqs = ifcvf_get_max_vq_pairs(vf);
Btw, I think current IFCVF doesn't support the provisioning of a
$max_qps that is smaller than what hardware did.
Then I wonder if we need a min_supported_vqs attribute or doing
mediation in the ifcvf parent.
Thanks
> + ifcvf_mgmt_dev->mdev.supported_features = vf->hw_features;
> +
> adapter->vdpa.mdev = &ifcvf_mgmt_dev->mdev;
> ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring);
> if (ret) {
> --
> 2.31.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/6] vDPA/ifcvf: get_config_size should return a value no greater than dev implementation
On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan wrote:
>
> ifcvf_get_config_size() should return a virtio device type specific value,
> however the ret_value should not be greater than the onboard size of
> the device implementation. E.g., for virtio_net, config_size should be
> the minimum value of sizeof(struct virtio_net_config) and the onboard
> cap size.
>
> Signed-off-by: Zhu Lingshan
> ---
> drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++--
> drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 48c4dadb0c7c..6bccc8291c26 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev
> *pdev)
> break;
> case VIRTIO_PCI_CAP_DEVICE_CFG:
> hw->dev_cfg = get_cap_addr(hw, &cap);
> + hw->cap_dev_config_size = le32_to_cpu(cap.length);
One possible issue here is that, if the hardware have more size than
spec, we may end up with a migration compatibility issue.
It looks to me we'd better build the config size based on the
features, e.g it looks to me for net, we should probably use
offset_of(struct virtio_net_config, mtu)?
> IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg);
> break;
> }
> @@ -233,15 +234,18 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64
> features)
> u32 ifcvf_get_config_size(struct ifcvf_hw *hw)
> {
> struct ifcvf_adapter *adapter;
> + u32 net_config_size = sizeof(struct virtio_net_config);
> + u32 blk_config_size = sizeof(struct virtio_blk_config);
> + u32 cap_size = hw->cap_dev_config_size;
> u32 config_size;
>
> adapter = vf_to_adapter(hw);
> switch (hw->dev_type) {
> case VIRTIO_ID_NET:
> - config_size = sizeof(struct virtio_net_config);
> + config_size = cap_size >= net_config_size ? net_config_size :
> cap_size;
I don't get the code here, any chance that net_config_size could be zero?
Thanks
> break;
> case VIRTIO_ID_BLOCK:
> - config_size = sizeof(struct virtio_blk_config);
> + config_size = cap_size >= blk_config_size ? blk_config_size :
> cap_size;
> break;
> default:
> config_size = 0;
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index 115b61f4924b..f5563f665cc6 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -87,6 +87,8 @@ struct ifcvf_hw {
> int config_irq;
> int vqs_reused_irq;
> u16 nr_vring;
> + /* VIRTIO_PCI_CAP_DEVICE_CFG size */
> + u32 cap_dev_config_size;
> };
>
> struct ifcvf_adapter {
> --
> 2.31.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
