date:20171214

Re: [PATCH v19 2/7] xbitmap: potential improvement

2017-12-14 Thread kbuild test robot

Hi Wei,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.15-rc3 next-20171214]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Wei-Wang/Virtio-balloon-Enhancement/20171215-100525
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

Note: the linux-review/Wei-Wang/Virtio-balloon-Enhancement/20171215-100525 HEAD 
607ddba072bf7f9c9cbacedaccad7c42c5c7149c builds fine.
  It only hurts bisectibility.

All errors (new ones prefixed by >>):

>> lib/xbitmap.c:80:6: error: conflicting types for 'xb_clear_bit'
void xb_clear_bit(struct xb *xb, unsigned long bit)
 ^~~~
   In file included from lib/xbitmap.c:2:0:
   include/linux/xbitmap.h:37:5: note: previous declaration of 'xb_clear_bit' 
was here
int xb_clear_bit(struct xb *xb, unsigned long bit);
^~~~

vim +/xb_clear_bit +80 lib/xbitmap.c

71  
72  /**
73   * xb_clear_bit - clear a bit in the xbitmap
74   * @xb: the xbitmap tree used to record the bit
75   * @bit: index of the bit to clear
76   *
77   * This function is used to clear a bit in the xbitmap. If all the bits 
of the
78   * bitmap are 0, the bitmap will be freed.
79   */
  > 80  void xb_clear_bit(struct xb *xb, unsigned long bit)
81  {
82  unsigned long index = bit / IDA_BITMAP_BITS;
83  struct radix_tree_root *root = >xbrt;
84  struct radix_tree_node *node;
85  void **slot;
86  struct ida_bitmap *bitmap;
87  unsigned long ebit;
88  
89  bit %= IDA_BITMAP_BITS;
90  ebit = bit + 2;
91  
92  bitmap = __radix_tree_lookup(root, index, , );
93  if (radix_tree_exception(bitmap)) {
94  unsigned long tmp = (unsigned long)bitmap;
95  
96  if (ebit >= BITS_PER_LONG)
97  return;
98  tmp &= ~(1UL << ebit);
99  if (tmp == RADIX_TREE_EXCEPTIONAL_ENTRY)
   100  __radix_tree_delete(root, node, slot);
   101  else
   102  rcu_assign_pointer(*slot, (void *)tmp);
   103  return;
   104  }
   105  
   106  if (!bitmap)
   107  return;
   108  
   109  __clear_bit(bit, bitmap->bitmap);
   110  if (bitmap_empty(bitmap->bitmap, IDA_BITMAP_BITS)) {
   111  kfree(bitmap);
   112  __radix_tree_delete(root, node, slot);
   113  }
   114  }
   115  EXPORT_SYMBOL(xb_clear_bit);
   116  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCHv2] virtio_mmio: fix devm cleanup

2017-12-14 Thread weiping zhang

2017-12-15 2:48 GMT+08:00 Michael S. Tsirkin :
> On Wed, Dec 13, 2017 at 02:34:14PM +, Mark Rutland wrote:
>> On Tue, Dec 12, 2017 at 06:02:23PM +0100, Cornelia Huck wrote:
>> > On Tue, 12 Dec 2017 13:45:50 +
>> > Mark Rutland  wrote:
>> >
>> > > Recent rework of the virtio_mmio probe/remove paths balanced a
>> > > devm_ioremap() with an iounmap() rather than its devm variant. This ends
>> > > up corrupting the devm datastructures, and results in the following
>> > > boot-time splat on arm64 under QEMU 2.9.0:
>> > >
>> > > [3.450397] [ cut here ]
>> > > [3.453822] Trying to vfree() nonexistent vm area (c05b4844)
>> > > [3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 
>> > > __vunmap+0x1b8/0x220
>> > > [3.475898] Kernel panic - not syncing: panic_on_warn set ...
>> > > [3.475898]
>> > > [3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
>> > > [3.513109] Hardware name: linux,dummy-virt (DT)
>> > > [3.525382] Call trace:
>> > > [3.531683]  dump_backtrace+0x0/0x368
>> > > [3.543921]  show_stack+0x20/0x30
>> > > [3.547767]  dump_stack+0x108/0x164
>> > > [3.559584]  panic+0x25c/0x51c
>> > > [3.569184]  __warn+0x29c/0x31c
>> > > [3.576023]  report_bug+0x1d4/0x290
>> > > [3.586069]  bug_handler.part.2+0x40/0x100
>> > > [3.597820]  bug_handler+0x4c/0x88
>> > > [3.608400]  brk_handler+0x11c/0x218
>> > > [3.613430]  do_debug_exception+0xe8/0x318
>> > > [3.627370]  el1_dbg+0x18/0x78
>> > > [3.634037]  __vunmap+0x1b8/0x220
>> > > [3.648747]  vunmap+0x6c/0xc0
>> > > [3.653864]  __iounmap+0x44/0x58
>> > > [3.659771]  devm_ioremap_release+0x34/0x68
>> > > [3.672983]  release_nodes+0x404/0x880
>> > > [3.683543]  devres_release_all+0x6c/0xe8
>> > > [3.695692]  driver_probe_device+0x250/0x828
>> > > [3.706187]  __driver_attach+0x190/0x210
>> > > [3.717645]  bus_for_each_dev+0x14c/0x1f0
>> > > [3.728633]  driver_attach+0x48/0x78
>> > > [3.740249]  bus_add_driver+0x26c/0x5b8
>> > > [3.752248]  driver_register+0x16c/0x398
>> > > [3.757211]  __platform_driver_register+0xd8/0x128
>> > > [3.770860]  virtio_mmio_init+0x1c/0x24
>> > > [3.782671]  do_one_initcall+0xe0/0x398
>> > > [3.791890]  kernel_init_freeable+0x594/0x660
>> > > [3.798514]  kernel_init+0x18/0x190
>> > > [3.810220]  ret_from_fork+0x10/0x18
>> > >
>> > > To fix this, we can simply rip out the explicit cleanup that the devm
>> > > infrastructure will do for us when our probe function returns an error
>> > > code, or when our remove function returns.
>> > >
>> > > We only need to ensure that we call put_device() if a call to
>> > > register_virtio_device() fails in the probe path.
>> > >
>> > > Signed-off-by: Mark Rutland 
>> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for 
>> > > virtio_mmio_probe")
>> > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for 
>> > > virtio_mmio_remove")
>> > > Cc: Cornelia Huck 
>> > > Cc: Michael S. Tsirkin 
>> > > Cc: weiping zhang 
>> > > Cc: virtualization@lists.linux-foundation.org
>> > > ---
>> > >  drivers/virtio/virtio_mmio.c | 43 
>> > > +--
>> > >  1 file changed, 9 insertions(+), 34 deletions(-)
>> >
>> > In the hope that I have grokked the devm_* interface by now,
>> >
>> > Reviewed-by: Cornelia Huck 
>>
>> Thanks!
>>
>> Michael, could you please queue this as a fix for v4.15?
>>
>> This regressed arm64 VMs booting between v4.15-rc1 and v4-15-rc2,
>> impacting our automated regression testing, and I'd very much like to
>> get back to testing pure mainline rather than mainline + local fixes.
>>
>> Thanks,
>> Mark.
>
> Yep, plan to.
> Thanks!

Sorry to bother again,
As we know if we call device_register we should keep vdev alive until
dev.release be called. As discuss with Cornelia before,

> - return register_virtio_device(_dev->vdev);
> + rc = register_virtio_device(_dev->vdev);
> + if (rc)
> + goto put_dev;
> + return 0;
> +put_dev:
> + put_device(_dev->vdev.dev);

> Here you give up the extra reference from device_initialize(), which
> may or may not be the last reference (since you don't know if
> device_add() had already exposed the struct device to other code that
> might have acquired a reference). As the device has an empty release
> function, touching the device structure after that is not a real
> problem, but...

cuase devm_ interface will free vdev if probe fail, even dev.ref count not
dev to 0. So devm_ interface, may be not very suitable for device_register.

__
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___

Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree

2017-12-14 Thread Michael S. Tsirkin

On Fri, Dec 15, 2017 at 09:38:42AM +0800, weiping zhang wrote:
> 2017-12-15 3:13 GMT+08:00 Michael S. Tsirkin :
> > On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang wrote:
> >> As mentioned at drivers/base/core.c:
> >> /*
> >>  * NOTE: _Never_ directly free @dev after calling this function, even
> >>  * if it returned an error! Always use put_device() to give up the
> >>  * reference initialized in this function instead.
> >>  */
> >> so we don't free vp_dev until vp_dev->vdev.dev.release be called.
> >
> > seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
> > annotations went to stable, should this go there too?
> >
> just let people know the detail reason of using put_device.
> >> Signed-off-by: weiping zhang 
> >> Reviewed-by: Cornelia Huck 
> >
> > OK but this relies on users knowing that register_virtio_device
> > calls device_register. I think we want to add a comment
> > to register_virtio_device.
> >
> > Also the cleanup is uglified.
> >
> > I really think the right thing would be to change device_register making
> > it safe to kfree. People have the right to expect register on failure to
> > have no effect.
> >
> > That just might be too hard to implement though.
> >
> > For now, my suggestion - add a variable.
> >
> >> ---
> >>  drivers/virtio/virtio_pci_common.c | 17 +
> >>  1 file changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/virtio/virtio_pci_common.c 
> >> b/drivers/virtio/virtio_pci_common.c
> >> index 1c4797e..91d20f7 100644
> >> --- a/drivers/virtio/virtio_pci_common.c
> >> +++ b/drivers/virtio/virtio_pci_common.c
> >> @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> >>   pci_set_master(pci_dev);
> >>
> >>   rc = register_virtio_device(_dev->vdev);
> >> - if (rc)
> >> - goto err_register;
> >> + if (rc) {
> >> + if (vp_dev->ioaddr)
> >> +  virtio_pci_legacy_remove(vp_dev);
> >> + else
> >> +  virtio_pci_modern_remove(vp_dev);
> >> + pci_disable_device(pci_dev);
> >> + put_device(_dev->vdev.dev);
> >> + }
> >>
> >> - return 0;
> >> + return rc;
> >>
> >> -err_register:
> >> - if (vp_dev->ioaddr)
> >> -  virtio_pci_legacy_remove(vp_dev);
> >> - else
> >> -  virtio_pci_modern_remove(vp_dev);
> >>  err_probe:
> >>   pci_disable_device(pci_dev);
> >>  err_enable_device:
> >> --
> >> 2.9.4
> >
> > I'd prefer something like the below.
> >
> > --->
> >
> > virtio_pci: don't kfree device on register failure
> >
> > Signed-off-by: Michael S. Tsirkin 
> >
> > ---
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c 
> > b/drivers/virtio/virtio_pci_common.c
> > index 1c4797e..995ab03 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
> >  static int virtio_pci_probe(struct pci_dev *pci_dev,
> > const struct pci_device_id *id)
> >  {
> > -   struct virtio_pci_device *vp_dev;
> > +   struct virtio_pci_device *vp_dev, *reg_dev = NULL;
> > int rc;
> >
> > /* allocate our structure and fill it out */
> > @@ -551,6 +551,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> > pci_set_master(pci_dev);
> >
> > rc = register_virtio_device(_dev->vdev);
> > +   /* NOTE: device is considered registered even if register failed. */
> > +   reg_dev = vp_dev;
> > if (rc)
> > goto err_register;
> >
> > @@ -564,7 +566,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> >  err_probe:
> > pci_disable_device(pci_dev);
> >  err_enable_device:
> > -   kfree(vp_dev);
> > +   if (reg_dev)
> > +   put_device(dev);
> > +   else
> > +   kfree(vp_dev);
> > return rc;
> >  }
> looks more cleaner and same coding style.
> Need I send V3 or apply your patch directly ?

Pls post v3 updating all patches to this style.

Also just to make sure, none of this is a regression and none
of this causes actual known issues right?

I think it's preferrable to defer to next merge cycle unless this
is a regression.

> > ___
> > 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 1/3] virtio_pci: use put_device instead of kfree

2017-12-14 Thread weiping zhang

2017-12-15 3:13 GMT+08:00 Michael S. Tsirkin :
> On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang wrote:
>> As mentioned at drivers/base/core.c:
>> /*
>>  * NOTE: _Never_ directly free @dev after calling this function, even
>>  * if it returned an error! Always use put_device() to give up the
>>  * reference initialized in this function instead.
>>  */
>> so we don't free vp_dev until vp_dev->vdev.dev.release be called.
>
> seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
> annotations went to stable, should this go there too?
>
just let people know the detail reason of using put_device.
>> Signed-off-by: weiping zhang 
>> Reviewed-by: Cornelia Huck 
>
> OK but this relies on users knowing that register_virtio_device
> calls device_register. I think we want to add a comment
> to register_virtio_device.
>
> Also the cleanup is uglified.
>
> I really think the right thing would be to change device_register making
> it safe to kfree. People have the right to expect register on failure to
> have no effect.
>
> That just might be too hard to implement though.
>
> For now, my suggestion - add a variable.
>
>> ---
>>  drivers/virtio/virtio_pci_common.c | 17 +
>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_pci_common.c 
>> b/drivers/virtio/virtio_pci_common.c
>> index 1c4797e..91d20f7 100644
>> --- a/drivers/virtio/virtio_pci_common.c
>> +++ b/drivers/virtio/virtio_pci_common.c
>> @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>>   pci_set_master(pci_dev);
>>
>>   rc = register_virtio_device(_dev->vdev);
>> - if (rc)
>> - goto err_register;
>> + if (rc) {
>> + if (vp_dev->ioaddr)
>> +  virtio_pci_legacy_remove(vp_dev);
>> + else
>> +  virtio_pci_modern_remove(vp_dev);
>> + pci_disable_device(pci_dev);
>> + put_device(_dev->vdev.dev);
>> + }
>>
>> - return 0;
>> + return rc;
>>
>> -err_register:
>> - if (vp_dev->ioaddr)
>> -  virtio_pci_legacy_remove(vp_dev);
>> - else
>> -  virtio_pci_modern_remove(vp_dev);
>>  err_probe:
>>   pci_disable_device(pci_dev);
>>  err_enable_device:
>> --
>> 2.9.4
>
> I'd prefer something like the below.
>
> --->
>
> virtio_pci: don't kfree device on register failure
>
> Signed-off-by: Michael S. Tsirkin 
>
> ---
>
> diff --git a/drivers/virtio/virtio_pci_common.c 
> b/drivers/virtio/virtio_pci_common.c
> index 1c4797e..995ab03 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
>  static int virtio_pci_probe(struct pci_dev *pci_dev,
> const struct pci_device_id *id)
>  {
> -   struct virtio_pci_device *vp_dev;
> +   struct virtio_pci_device *vp_dev, *reg_dev = NULL;
> int rc;
>
> /* allocate our structure and fill it out */
> @@ -551,6 +551,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> pci_set_master(pci_dev);
>
> rc = register_virtio_device(_dev->vdev);
> +   /* NOTE: device is considered registered even if register failed. */
> +   reg_dev = vp_dev;
> if (rc)
> goto err_register;
>
> @@ -564,7 +566,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  err_probe:
> pci_disable_device(pci_dev);
>  err_enable_device:
> -   kfree(vp_dev);
> +   if (reg_dev)
> +   put_device(dev);
> +   else
> +   kfree(vp_dev);
> return rc;
>  }
looks more cleaner and same coding style.
Need I send V3 or apply your patch directly ?
> ___
> 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 1/3] virtio_pci: use put_device instead of kfree

2017-12-14 Thread Michael S. Tsirkin

On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang wrote:
> As mentioned at drivers/base/core.c:
> /*
>  * NOTE: _Never_ directly free @dev after calling this function, even
>  * if it returned an error! Always use put_device() to give up the
>  * reference initialized in this function instead.
>  */
> so we don't free vp_dev until vp_dev->vdev.dev.release be called.

seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
annotations went to stable, should this go there too?

> Signed-off-by: weiping zhang 
> Reviewed-by: Cornelia Huck 

OK but this relies on users knowing that register_virtio_device
calls device_register. I think we want to add a comment
to register_virtio_device.

Also the cleanup is uglified.

I really think the right thing would be to change device_register making
it safe to kfree. People have the right to expect register on failure to
have no effect.

That just might be too hard to implement though.

For now, my suggestion - add a variable.

> ---
>  drivers/virtio/virtio_pci_common.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c 
> b/drivers/virtio/virtio_pci_common.c
> index 1c4797e..91d20f7 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>   pci_set_master(pci_dev);
>  
>   rc = register_virtio_device(_dev->vdev);
> - if (rc)
> - goto err_register;
> + if (rc) {
> + if (vp_dev->ioaddr)
> +  virtio_pci_legacy_remove(vp_dev);
> + else
> +  virtio_pci_modern_remove(vp_dev);
> + pci_disable_device(pci_dev);
> + put_device(_dev->vdev.dev);
> + }
>  
> - return 0;
> + return rc;
>  
> -err_register:
> - if (vp_dev->ioaddr)
> -  virtio_pci_legacy_remove(vp_dev);
> - else
> -  virtio_pci_modern_remove(vp_dev);
>  err_probe:
>   pci_disable_device(pci_dev);
>  err_enable_device:
> -- 
> 2.9.4

I'd prefer something like the below.

--->

virtio_pci: don't kfree device on register failure

Signed-off-by: Michael S. Tsirkin 

---

diff --git a/drivers/virtio/virtio_pci_common.c 
b/drivers/virtio/virtio_pci_common.c
index 1c4797e..995ab03 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
 static int virtio_pci_probe(struct pci_dev *pci_dev,
const struct pci_device_id *id)
 {
-   struct virtio_pci_device *vp_dev;
+   struct virtio_pci_device *vp_dev, *reg_dev = NULL;
int rc;
 
/* allocate our structure and fill it out */
@@ -551,6 +551,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
pci_set_master(pci_dev);
 
rc = register_virtio_device(_dev->vdev);
+   /* NOTE: device is considered registered even if register failed. */
+   reg_dev = vp_dev;
if (rc)
goto err_register;
 
@@ -564,7 +566,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 err_probe:
pci_disable_device(pci_dev);
 err_enable_device:
-   kfree(vp_dev);
+   if (reg_dev)
+   put_device(dev);
+   else
+   kfree(vp_dev);
return rc;
 }
 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCHv2] virtio_mmio: fix devm cleanup

2017-12-14 Thread Michael S. Tsirkin

On Wed, Dec 13, 2017 at 02:34:14PM +, Mark Rutland wrote:
> On Tue, Dec 12, 2017 at 06:02:23PM +0100, Cornelia Huck wrote:
> > On Tue, 12 Dec 2017 13:45:50 +
> > Mark Rutland  wrote:
> > 
> > > Recent rework of the virtio_mmio probe/remove paths balanced a
> > > devm_ioremap() with an iounmap() rather than its devm variant. This ends
> > > up corrupting the devm datastructures, and results in the following
> > > boot-time splat on arm64 under QEMU 2.9.0:
> > > 
> > > [3.450397] [ cut here ]
> > > [3.453822] Trying to vfree() nonexistent vm area (c05b4844)
> > > [3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 
> > > __vunmap+0x1b8/0x220
> > > [3.475898] Kernel panic - not syncing: panic_on_warn set ...
> > > [3.475898]
> > > [3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
> > > [3.513109] Hardware name: linux,dummy-virt (DT)
> > > [3.525382] Call trace:
> > > [3.531683]  dump_backtrace+0x0/0x368
> > > [3.543921]  show_stack+0x20/0x30
> > > [3.547767]  dump_stack+0x108/0x164
> > > [3.559584]  panic+0x25c/0x51c
> > > [3.569184]  __warn+0x29c/0x31c
> > > [3.576023]  report_bug+0x1d4/0x290
> > > [3.586069]  bug_handler.part.2+0x40/0x100
> > > [3.597820]  bug_handler+0x4c/0x88
> > > [3.608400]  brk_handler+0x11c/0x218
> > > [3.613430]  do_debug_exception+0xe8/0x318
> > > [3.627370]  el1_dbg+0x18/0x78
> > > [3.634037]  __vunmap+0x1b8/0x220
> > > [3.648747]  vunmap+0x6c/0xc0
> > > [3.653864]  __iounmap+0x44/0x58
> > > [3.659771]  devm_ioremap_release+0x34/0x68
> > > [3.672983]  release_nodes+0x404/0x880
> > > [3.683543]  devres_release_all+0x6c/0xe8
> > > [3.695692]  driver_probe_device+0x250/0x828
> > > [3.706187]  __driver_attach+0x190/0x210
> > > [3.717645]  bus_for_each_dev+0x14c/0x1f0
> > > [3.728633]  driver_attach+0x48/0x78
> > > [3.740249]  bus_add_driver+0x26c/0x5b8
> > > [3.752248]  driver_register+0x16c/0x398
> > > [3.757211]  __platform_driver_register+0xd8/0x128
> > > [3.770860]  virtio_mmio_init+0x1c/0x24
> > > [3.782671]  do_one_initcall+0xe0/0x398
> > > [3.791890]  kernel_init_freeable+0x594/0x660
> > > [3.798514]  kernel_init+0x18/0x190
> > > [3.810220]  ret_from_fork+0x10/0x18
> > > 
> > > To fix this, we can simply rip out the explicit cleanup that the devm
> > > infrastructure will do for us when our probe function returns an error
> > > code, or when our remove function returns.
> > > 
> > > We only need to ensure that we call put_device() if a call to
> > > register_virtio_device() fails in the probe path.
> > > 
> > > Signed-off-by: Mark Rutland 
> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for 
> > > virtio_mmio_remove")
> > > Cc: Cornelia Huck 
> > > Cc: Michael S. Tsirkin 
> > > Cc: weiping zhang 
> > > Cc: virtualization@lists.linux-foundation.org
> > > ---
> > >  drivers/virtio/virtio_mmio.c | 43 
> > > +--
> > >  1 file changed, 9 insertions(+), 34 deletions(-)
> > 
> > In the hope that I have grokked the devm_* interface by now,
> > 
> > Reviewed-by: Cornelia Huck 
> 
> Thanks!
> 
> Michael, could you please queue this as a fix for v4.15?
> 
> This regressed arm64 VMs booting between v4.15-rc1 and v4-15-rc2,
> impacting our automated regression testing, and I'd very much like to
> get back to testing pure mainline rather than mainline + local fixes.
> 
> Thanks,
> Mark.

Yep, plan to.
Thanks!
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v19 3/7] xbitmap: add more operations

2017-12-14 Thread Matthew Wilcox

On Fri, Dec 15, 2017 at 01:29:45AM +0900, Tetsuo Handa wrote:
> > > Also, one more thing you need to check. Have you checked how long does
> > > xb_find_next_set_bit(xb, 0, ULONG_MAX) on an empty xbitmap takes?
> > > If it causes soft lockup warning, should we add cond_resched() ?
> > > If yes, you have to document that this API might sleep. If no, you
> > > have to document that the caller of this API is responsible for
> > > not to pass such a large value range.
> > 
> > Yes, that will take too long time. Probably we can document some 
> > comments as a reminder for the callers.
> 
> Then, I feel that API is poorly implemented. There is no need to brute-force
> when scanning [0, ULONG_MAX] range. If you eliminate exception path and
> redesign the data structure, xbitmap will become as simple as a sample
> implementation shown below. Not tested yet, but I think that this will be
> sufficient for what virtio-baloon wants to do; i.e. find consecutive "1" bits
> quickly. I didn't test whether finding "struct ulong_list_data" using radix
> tree can improve performance.

find_next_set_bit() is just badly implemented.  There is no need to
redesign the data structure.  It should be a simple matter of:

 - look at ->head, see it is NULL, return false.

If bit 100 is set and you call find_next_set_bit(101, ULONG_MAX), it
should look at block 0, see there is a pointer to it, scan the block,
see there are no bits set above 100, then realise we're at the end of
the tree and stop.

If bit 2000 is set, and you call find_next_set_bit(2001, ULONG_MAX)
tit should look at block 1, see there's no bit set after bit 2001, then
look at the other blocks in the node, see that all the pointers are NULL
and stop.

This isn't rocket science, we already do something like this in the radix
tree and it'll be even easier to do in the XArray.  Which I'm going back
to working on now.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[no subject]

2017-12-14 Thread Solen win

solen...@freshdesk.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] drm/virtio: Add window server support

2017-12-14 Thread Tomeu Vizoso

This is to allow clients running within VMs to be able to communicate
with a compositor in the host. Clients will use the communication
protocol that the compositor supports, and virtio-gpu will assist with
making buffers available in both sides, and copying content as needed.

It is expected that a service in the guest will act as a proxy,
interacting with virtio-gpu to support unmodified clients. For some
features of the protocol, the hypervisor might have to intervene and
also parse the protocol data to properly bridge resources. The following
IOCTLs have been added to this effect:

*_WINSRV_CONNECT: Opens a connection to the compositor in the host,
returns a FD that represents this connection and on which the following
IOCTLs can be used. Callers are expected to poll this FD for new
messages from the compositor.

*_WINSRV_TX: Asks the hypervisor to forward a message to the compositor

*_WINSRV_RX: Returns all queued messages

Alongside protocol data that is opaque to the kernel, the client can
send file descriptors that reference GEM buffers allocated by
virtio-gpu. The guest proxy is expected to figure out when a client is
passing a FD that refers to shared memory in the guest and allocate a
virtio-gpu buffer of the same size with DRM_VIRTGPU_RESOURCE_CREATE.

When the client notifies the compositor that it can read from that buffer,
the proxy should copy the contents from the SHM region to the virtio-gpu
resource and call DRM_VIRTGPU_TRANSFER_TO_HOST.

This has been tested with Wayland clients that make use of wl_shm to
pass buffers to the compositor, but is expected to work similarly for X
clients that make use of MIT-SHM with FD passing.

Signed-off-by: Tomeu Vizoso 
Cc: Zach Reizner 

---

Hi,

this work is based on the virtio_wl driver in the ChromeOS kernel by
Zach Reizner, currently at:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/virtio/virtio_wl.c

There's two features missing in this patch when compared with virtio_wl:

* Allow the guest access directly host memory, without having to resort
to TRANSFER_TO_HOST

* Pass FDs from host to guest (Wayland specifies that the compositor
shares keyboard data with the guest via a shared buffer)

I plan to work on this next, but I would like to get some comments on
the general approach so I can better choose which patch to follow.

Thanks,

Tomeu
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  39 -
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 168 +++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  58 +--
 drivers/gpu/drm/virtio/virtgpu_vq.c| 283 +
 include/uapi/drm/virtgpu_drm.h |  29 
 include/uapi/linux/virtio_gpu.h|  39 +
 6 files changed, 602 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index da2fb585fea4..268b386e1232 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -178,6 +178,8 @@ struct virtio_gpu_device {
 
struct virtio_gpu_queue ctrlq;
struct virtio_gpu_queue cursorq;
+   struct virtio_gpu_queue winsrv_rxq;
+   struct virtio_gpu_queue winsrv_txq;
struct kmem_cache *vbufs;
bool vqs_ready;
 
@@ -205,10 +207,32 @@ struct virtio_gpu_device {
 
 struct virtio_gpu_fpriv {
uint32_t ctx_id;
+
+   struct list_head winsrv_conns; /* list of virtio_gpu_winsrv_conn */
+   spinlock_t winsrv_lock;
+};
+
+struct virtio_gpu_winsrv_rx_qentry {
+   struct virtio_gpu_winsrv_rx *cmd;
+   struct list_head next;
+};
+
+struct virtio_gpu_winsrv_conn {
+   struct virtio_gpu_device *vgdev;
+
+   spinlock_t lock;
+
+   int fd;
+   struct drm_file *drm_file;
+
+   struct list_head cmdq; /* queue of virtio_gpu_winsrv_rx_qentry */
+   wait_queue_head_t cmdwq;
+
+   struct list_head next;
 };
 
 /* virtio_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 10
+#define DRM_VIRTIO_NUM_IOCTLS 11
 extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
 
 /* virtio_kms.c */
@@ -318,9 +342,22 @@ virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device 
*vgdev,
 void virtio_gpu_ctrl_ack(struct virtqueue *vq);
 void virtio_gpu_cursor_ack(struct virtqueue *vq);
 void virtio_gpu_fence_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_tx_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_rx_read(struct virtqueue *vq);
 void virtio_gpu_dequeue_ctrl_func(struct work_struct *work);
 void virtio_gpu_dequeue_cursor_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_rx_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_tx_func(struct work_struct *work);
 void virtio_gpu_dequeue_fence_func(struct work_struct *work);
+void virtio_gpu_fill_winsrv_rx(struct virtio_gpu_device *vgdev);
+void virtio_gpu_queue_winsrv_rx_in(struct virtio_gpu_device *vgdev,
+

Re: [PATCH v19 3/7] xbitmap: add more operations

2017-12-14 Thread Matthew Wilcox

On Wed, Dec 13, 2017 at 08:26:06PM +0800, Wei Wang wrote:
> On 12/12/2017 09:20 PM, Tetsuo Handa wrote:
> > Can you eliminate exception path and fold all xbitmap patches into one, and
> > post only one xbitmap patch without virtio-baloon changes? If exception path
> > is valuable, you can add exception path after minimum version is merged.
> > This series is too difficult for me to close corner cases.
> 
> That exception path is claimed to save memory, and I don't have a strong
> reason to remove that part.
> Matthew, could we get your feedback on this?

Sure.  This code is derived from the IDA code in lib/idr.c.  Eventually,
I intend to reunite them.  For IDA, it clearly makes sense; the first 62
entries result in allocating no memory at all, which is going to be 99%
of users.  After that, we allocate 128 bytes which will serve the first
1024 users.

The xbitmap, as used by Wei's patches here is going to be used somewhat
differently from that.  I understand why Tetsuo wants the exceptional
path removed; I'm not sure the gains will be as important.  But if we're
going to rebuild the IDA on top of the xbitmap, we need to keep them.

I really want to pay more attention to this, but I need to focus on
getting the XArray finished.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-dev] Re: [PATCH v19 3/7] xbitmap: add more operations

2017-12-14 Thread Wei Wang


On 12/14/2017 11:47 AM, Wei Wang wrote:

On 12/13/2017 10:16 PM, Tetsuo Handa wrote:





  if (set)
  ret = find_next_bit(,
BITS_PER_LONG, ebit);
  else
  ret = find_next_zero_bit(,
BITS_PER_LONG,
ebit);
  if (ret < BITS_PER_LONG)
  return ret - 2 + ida_start;
  } else if (bitmap) {
  if (set)
  ret = find_next_bit(bitmap->bitmap,
IDA_BITMAP_BITS, bit);
  else
  ret = 
find_next_zero_bit(bitmap->bitmap,

IDA_BITMAP_BITS, bit);

"bit" may not be 0 for the first round and "bit" is always 0 afterwords.
But where is the guaranteed that "end" is a multiple of 
IDA_BITMAP_BITS ?

Please explain why it is correct to use IDA_BITMAP_BITS unconditionally
for the last round.


There missed something here, it will be:

nbits = min(end - ida_start + 1, IDA_BITMAP_BITS - bit);



captured a bug here, should be:
nbits = min(end - ida_start + 1, (unsigned long)IDA_BITMAP_BITS);



if (set)
ret = find_next_bit(bitmap->bitmap, nbits, bit);
else
ret = find_next_zero_bit(bitmap->bitmap,
   nbits, bit);
if (ret < nbits)
return ret + ida_start;




Best,
Wei




___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PULL 1/1] virtio/s390: implement PM operations for virtio_ccw

2017-12-14 Thread Cornelia Huck

From: Christian Borntraeger 

Suspend/Resume to/from disk currently fails. Let us wire
up the necessary callbacks. This is mostly just forwarding
the requests to the virtio drivers. The only thing that
has to be done in virtio_ccw itself is to re-set the
virtio revision.

Suggested-by: Thomas Huth 
Signed-off-by: Christian Borntraeger 
Message-Id: <20171207141102.70190-2-borntrae...@de.ibm.com>
Reviewed-by: David Hildenbrand 
Signed-off-by: Cornelia Huck 
---
 drivers/s390/virtio/virtio_ccw.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index b18fe2014cf2..330b3fa3430a 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -1300,6 +1300,9 @@ static int virtio_ccw_cio_notify(struct ccw_device *cdev, 
int event)
vcdev->device_lost = true;
rc = NOTIFY_DONE;
break;
+   case CIO_OPER:
+   rc = NOTIFY_OK;
+   break;
default:
rc = NOTIFY_DONE;
break;
@@ -1312,6 +1315,25 @@ static struct ccw_device_id virtio_ids[] = {
{},
 };
 
+static int virtio_ccw_freeze(struct ccw_device *cdev)
+{
+   struct virtio_ccw_device *vcdev = dev_get_drvdata(>dev);
+
+   return virtio_device_freeze(>vdev);
+}
+
+static int virtio_ccw_restore(struct ccw_device *cdev)
+{
+   struct virtio_ccw_device *vcdev = dev_get_drvdata(>dev);
+   int ret;
+
+   ret = virtio_ccw_set_transport_rev(vcdev);
+   if (ret)
+   return ret;
+
+   return virtio_device_restore(>vdev);
+}
+
 static struct ccw_driver virtio_ccw_driver = {
.driver = {
.owner = THIS_MODULE,
@@ -1324,6 +1346,9 @@ static struct ccw_driver virtio_ccw_driver = {
.set_online = virtio_ccw_online,
.notify = virtio_ccw_cio_notify,
.int_class = IRQIO_VIR,
+   .freeze = virtio_ccw_freeze,
+   .thaw = virtio_ccw_restore,
+   .restore = virtio_ccw_restore,
 };
 
 static int __init pure_hex(char **cp, unsigned int *val, int min_digit,
-- 
2.13.6

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PULL 0/1] s390/virtio update

2017-12-14 Thread Cornelia Huck

The following changes since commit e073f74a5a39c6dc45f28a5006c21aa94490d9b8:

  Merge branch 'this' into vhost (2017-12-07 18:39:24 +0200)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git 
tags/virtio-s390-20171214

for you to fetch changes up to 619b4b0ba832144d4be899640a2047f9675df849:

  virtio/s390: implement PM operations for virtio_ccw (2017-12-14 10:32:21 
+0100)


Hibernation support for virtio-ccw.



Christian Borntraeger (1):
  virtio/s390: implement PM operations for virtio_ccw

 drivers/s390/virtio/virtio_ccw.c | 25 +
 1 file changed, 25 insertions(+)

-- 
2.13.6

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v19 2/7] xbitmap: potential improvement

Re: [PATCHv2] virtio_mmio: fix devm cleanup

Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree

Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree

Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree

Re: [PATCHv2] virtio_mmio: fix devm cleanup

Re: [PATCH v19 3/7] xbitmap: add more operations

[no subject]

[PATCH] drm/virtio: Add window server support

Re: [PATCH v19 3/7] xbitmap: add more operations

Re: [virtio-dev] Re: [PATCH v19 3/7] xbitmap: add more operations

[PULL 1/1] virtio/s390: implement PM operations for virtio_ccw

[PULL 0/1] s390/virtio update

13 matches

Site Navigation

Mail list logo

Footer information