Message ID | 20201022050638.29641-3-sherry.sun@nxp.com |
---|---|
State | New |
Headers | show |
Series | Change vring space from nomal memory to dma coherent memory | expand |
Hi Sherry, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on char-misc/char-misc-testing] [also build test WARNING on soc/for-next linus/master v5.9 next-20201022] [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/0day-ci/linux/commits/Sherry-Sun/Change-vring-space-from-nomal-memory-to-dma-coherent-memory/20201022-131008 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git f3277cbfba763cd2826396521b9296de67cf1bbc config: i386-randconfig-s002-20201022 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-dirty # https://github.com/0day-ci/linux/commit/6ae9d6d36b63c7bb8170f4c0409470d8e7101880 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Sherry-Sun/Change-vring-space-from-nomal-memory-to-dma-coherent-memory/20201022-131008 git checkout 6ae9d6d36b63c7bb8170f4c0409470d8e7101880 # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> "sparse warnings: (new ones prefixed by >>)" >> drivers/misc/mic/vop/vop_main.c:339:14: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected void *used @@ got void [noderef] __iomem * @@ >> drivers/misc/mic/vop/vop_main.c:339:14: sparse: expected void *used >> drivers/misc/mic/vop/vop_main.c:339:14: sparse: got void [noderef] __iomem * >> drivers/misc/mic/vop/vop_main.c:357:35: sparse: sparse: restricted __le64 degrades to integer vim +339 drivers/misc/mic/vop/vop_main.c 289 290 /* 291 * This routine will assign vring's allocated in host/io memory. Code in 292 * virtio_ring.c however continues to access this io memory as if it were local 293 * memory without io accessors. 294 */ 295 static struct virtqueue *vop_find_vq(struct virtio_device *dev, 296 unsigned index, 297 void (*callback)(struct virtqueue *vq), 298 const char *name, bool ctx) 299 { 300 struct _vop_vdev *vdev = to_vopvdev(dev); 301 struct vop_device *vpdev = vdev->vpdev; 302 struct mic_vqconfig __iomem *vqconfig; 303 struct mic_vqconfig config; 304 struct virtqueue *vq; 305 void __iomem *va; 306 struct _mic_vring_info __iomem *info; 307 void *used; 308 int vr_size, _vr_size, err, magic; 309 u8 type = ioread8(&vdev->desc->type); 310 311 if (index >= ioread8(&vdev->desc->num_vq)) 312 return ERR_PTR(-ENOENT); 313 314 if (!name) 315 return ERR_PTR(-ENOENT); 316 317 /* First assign the vring's allocated in host memory */ 318 vqconfig = _vop_vq_config(vdev->desc) + index; 319 memcpy_fromio(&config, vqconfig, sizeof(config)); 320 _vr_size = round_up(vring_size(le16_to_cpu(config.num), MIC_VIRTIO_RING_ALIGN), 4); 321 vr_size = PAGE_ALIGN(_vr_size + sizeof(struct _mic_vring_info)); 322 va = vpdev->hw_ops->remap(vpdev, le64_to_cpu(config.address), vr_size); 323 if (!va) 324 return ERR_PTR(-ENOMEM); 325 vdev->vr[index] = va; 326 memset_io(va, 0x0, _vr_size); 327 328 info = va + _vr_size; 329 magic = ioread32(&info->magic); 330 331 if (WARN(magic != MIC_MAGIC + type + index, "magic mismatch")) { 332 err = -EIO; 333 goto unmap; 334 } 335 336 vdev->used_size[index] = PAGE_ALIGN(sizeof(__u16) * 3 + 337 sizeof(struct vring_used_elem) * 338 le16_to_cpu(config.num)); > 339 used = va + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) + 340 sizeof(__u16) * (3 + le16_to_cpu(config.num))); 341 vdev->used_virt[index] = used; 342 if (!used) { 343 err = -ENOMEM; 344 dev_err(_vop_dev(vdev), "%s %d err %d\n", 345 __func__, __LINE__, err); 346 goto unmap; 347 } 348 349 vq = vop_new_virtqueue(index, le16_to_cpu(config.num), dev, ctx, 350 (void __force *)va, vop_notify, callback, 351 name, used); 352 if (!vq) { 353 err = -ENOMEM; 354 goto unmap; 355 } 356 > 357 vdev->used[index] = config.address + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) + 358 sizeof(__u16) * (3 + le16_to_cpu(config.num))); 359 writeq(vdev->used[index], &vqconfig->used_address); 360 361 vq->priv = vdev; 362 return vq; 363 unmap: 364 vpdev->hw_ops->unmap(vpdev, vdev->vr[index]); 365 return ERR_PTR(err); 366 } 367 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, Oct 22, 2020 at 01:06:36PM +0800, Sherry Sun wrote: > We don't need to allocate and reassign the used ring here and remove the > used_address_updated flag.Since RC has allocated the entire vring, > including the used ring. Simply add the corresponding offset can get the > used ring address. Someone needs to verify this vs the existing intel implementations. > - used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, > - get_order(vdev->used_size[index])); > + used = va + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) + This adds an over 80 char line. > + vdev->used[index] = config.address + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) + Again.
Hi Greg & Christoph, > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > On Thu, Oct 22, 2020 at 01:06:36PM +0800, Sherry Sun wrote: > > We don't need to allocate and reassign the used ring here and remove > > the used_address_updated flag.Since RC has allocated the entire vring, > > including the used ring. Simply add the corresponding offset can get > > the used ring address. > > Someone needs to verify this vs the existing intel implementations. Hi Greg, @gregkh@linuxfoundation.org, sorry I don't have the Intel MIC devices so cannot test it, do you know anyone who can help test this patch changes on the Intel MIC platform? Thanks. > > > - used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, > > - get_order(vdev->used_size[index])); > > + used = va + PAGE_ALIGN(sizeof(struct vring_desc) * > > +le16_to_cpu(config.num) + > > This adds an over 80 char line. Hi Christoph, will fix it in V4, thanks. > > > + vdev->used[index] = config.address + PAGE_ALIGN(sizeof(struct > > +vring_desc) * le16_to_cpu(config.num) + > > Again. Best regards Sherry
On Mon, Oct 26, 2020 at 03:04:45AM +0000, Sherry Sun wrote: > Hi Greg & Christoph, > > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > > ring > > > > On Thu, Oct 22, 2020 at 01:06:36PM +0800, Sherry Sun wrote: > > > We don't need to allocate and reassign the used ring here and remove > > > the used_address_updated flag.Since RC has allocated the entire vring, > > > including the used ring. Simply add the corresponding offset can get > > > the used ring address. > > > > Someone needs to verify this vs the existing intel implementations. > > Hi Greg, @gregkh@linuxfoundation.org, sorry I don't have the Intel MIC devices so cannot test it, do you know anyone who can help test this patch changes on the Intel MIC platform? Thanks. Why not ask the authors/maintainers of that code? greg k-h
diff --git a/drivers/misc/mic/vop/vop_debugfs.c b/drivers/misc/mic/vop/vop_debugfs.c index 9d4f175f4dd1..05eca4a98585 100644 --- a/drivers/misc/mic/vop/vop_debugfs.c +++ b/drivers/misc/mic/vop/vop_debugfs.c @@ -79,8 +79,6 @@ static int vop_dp_show(struct seq_file *s, void *pos) seq_printf(s, "Vdev reset %d\n", dc->vdev_reset); seq_printf(s, "Guest Ack %d ", dc->guest_ack); seq_printf(s, "Host ack %d\n", dc->host_ack); - seq_printf(s, "Used address updated %d ", - dc->used_address_updated); seq_printf(s, "Vdev 0x%llx\n", dc->vdev); seq_printf(s, "c2h doorbell %d ", dc->c2h_vdev_db); seq_printf(s, "h2c doorbell %d\n", dc->h2c_vdev_db); diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c index 714b94f42d38..1ccc94dfd6ac 100644 --- a/drivers/misc/mic/vop/vop_main.c +++ b/drivers/misc/mic/vop/vop_main.c @@ -250,10 +250,6 @@ static void vop_del_vq(struct virtqueue *vq, int n) struct _vop_vdev *vdev = to_vopvdev(vq->vdev); struct vop_device *vpdev = vdev->vpdev; - dma_unmap_single(&vpdev->dev, vdev->used[n], - vdev->used_size[n], DMA_BIDIRECTIONAL); - free_pages((unsigned long)vdev->used_virt[n], - get_order(vdev->used_size[n])); vring_del_virtqueue(vq); vpdev->hw_ops->unmap(vpdev, vdev->vr[n]); vdev->vr[n] = NULL; @@ -340,8 +336,8 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev, vdev->used_size[index] = PAGE_ALIGN(sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * le16_to_cpu(config.num)); - used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, - get_order(vdev->used_size[index])); + used = va + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) + + sizeof(__u16) * (3 + le16_to_cpu(config.num))); vdev->used_virt[index] = used; if (!used) { err = -ENOMEM; @@ -355,27 +351,15 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev, name, used); if (!vq) { err = -ENOMEM; - goto free_used; + goto unmap; } - vdev->used[index] = dma_map_single(&vpdev->dev, used, - vdev->used_size[index], - DMA_BIDIRECTIONAL); - if (dma_mapping_error(&vpdev->dev, vdev->used[index])) { - err = -ENOMEM; - dev_err(_vop_dev(vdev), "%s %d err %d\n", - __func__, __LINE__, err); - goto del_vq; - } + vdev->used[index] = config.address + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) + + sizeof(__u16) * (3 + le16_to_cpu(config.num))); writeq(vdev->used[index], &vqconfig->used_address); vq->priv = vdev; return vq; -del_vq: - vring_del_virtqueue(vq); -free_used: - free_pages((unsigned long)used, - get_order(vdev->used_size[index])); unmap: vpdev->hw_ops->unmap(vpdev, vdev->vr[index]); return ERR_PTR(err); @@ -388,9 +372,7 @@ static int vop_find_vqs(struct virtio_device *dev, unsigned nvqs, struct irq_affinity *desc) { struct _vop_vdev *vdev = to_vopvdev(dev); - struct vop_device *vpdev = vdev->vpdev; - struct mic_device_ctrl __iomem *dc = vdev->dc; - int i, err, retry, queue_idx = 0; + int i, err, queue_idx = 0; /* We must have this many virtqueues. */ if (nvqs > ioread8(&vdev->desc->num_vq)) @@ -412,24 +394,6 @@ static int vop_find_vqs(struct virtio_device *dev, unsigned nvqs, } } - iowrite8(1, &dc->used_address_updated); - /* - * Send an interrupt to the host to inform it that used - * rings have been re-assigned. - */ - vpdev->hw_ops->send_intr(vpdev, vdev->c2h_vdev_db); - for (retry = 100; --retry;) { - if (!ioread8(&dc->used_address_updated)) - break; - msleep(100); - } - - dev_dbg(_vop_dev(vdev), "%s: retry: %d\n", __func__, retry); - if (!retry) { - err = -ENODEV; - goto error; - } - return 0; error: vop_del_vqs(dev); diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c index 2cc3c22482b5..cc928d45af5a 100644 --- a/drivers/misc/mic/vop/vop_vringh.c +++ b/drivers/misc/mic/vop/vop_vringh.c @@ -53,33 +53,6 @@ static void _vop_notify(struct vringh *vrh) vpdev->hw_ops->send_intr(vpdev, db); } -static void vop_virtio_init_post(struct vop_vdev *vdev) -{ - struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd); - struct vop_device *vpdev = vdev->vpdev; - int i, used_size; - - for (i = 0; i < vdev->dd->num_vq; i++) { - used_size = PAGE_ALIGN(sizeof(u16) * 3 + - sizeof(struct vring_used_elem) * - le16_to_cpu(vqconfig->num)); - if (!le64_to_cpu(vqconfig[i].used_address)) { - dev_warn(vop_dev(vdev), "used_address zero??\n"); - continue; - } - vdev->vvr[i].vrh.vring.used = - (void __force *)vpdev->hw_ops->remap( - vpdev, - le64_to_cpu(vqconfig[i].used_address), - used_size); - } - - vdev->dc->used_address_updated = 0; - - dev_info(vop_dev(vdev), "%s: device type %d LINKUP\n", - __func__, vdev->virtio_id); -} - static inline void vop_virtio_device_reset(struct vop_vdev *vdev) { int i; @@ -130,9 +103,6 @@ static void vop_bh_handler(struct work_struct *work) struct vop_vdev *vdev = container_of(work, struct vop_vdev, virtio_bh_work); - if (vdev->dc->used_address_updated) - vop_virtio_init_post(vdev); - if (vdev->dc->vdev_reset) vop_virtio_device_reset(vdev); @@ -250,7 +220,6 @@ static void vop_init_device_ctrl(struct vop_vdev *vdev, dc->guest_ack = 0; dc->vdev_reset = 0; dc->host_ack = 0; - dc->used_address_updated = 0; dc->c2h_vdev_db = -1; dc->h2c_vdev_db = -1; vdev->dc = dc; diff --git a/include/uapi/linux/mic_common.h b/include/uapi/linux/mic_common.h index 504e523f702c..fe660d486b20 100644 --- a/include/uapi/linux/mic_common.h +++ b/include/uapi/linux/mic_common.h @@ -56,8 +56,7 @@ struct mic_device_desc { * @vdev_reset: Set to 1 by guest to indicate virtio device has been reset. * @guest_ack: Set to 1 by guest to ack a command. * @host_ack: Set to 1 by host to ack a command. - * @used_address_updated: Set to 1 by guest when the used address should be - * updated. + * @must_be_zero: Reserved because this bit is no longer needed. * @c2h_vdev_db: The doorbell number to be used by guest. Set by host. * @h2c_vdev_db: The doorbell number to be used by host. Set by guest. */ @@ -67,7 +66,7 @@ struct mic_device_ctrl { __u8 vdev_reset; __u8 guest_ack; __u8 host_ack; - __u8 used_address_updated; + __u8 must_be_zero; __s8 c2h_vdev_db; __s8 h2c_vdev_db; } __attribute__ ((aligned(8)));