Re: [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe

[weiping zhang]

2017-12-04 18:24 GMT+08:00 Cornelia Huck :
> On Sat, 2 Dec 2017 01:51:40 +0800
> weiping zhang  wrote:
>
>> cleanup all resource allocated by virtio_mmio_probe.
>>
>> Signed-off-by: weiping zhang 
>> ---
>>  drivers/virtio/virtio_mmio.c | 34 ++
>>  1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
>> index 74dc717..3fd0e66 100644
>> --- a/drivers/virtio/virtio_mmio.c
>> +++ b/drivers/virtio/virtio_mmio.c
>> @@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device 
>> *pdev)
>>   return -EBUSY;
>>
>>   vm_dev = devm_kzalloc(>dev, sizeof(*vm_dev), GFP_KERNEL);
>> - if (!vm_dev)
>> - return  -ENOMEM;
>> + if (!vm_dev) {
>> + rc =  -ENOMEM;
>
> Touching this would be a good time to remove the extra space in front
> of the -ENOMEM :)
Thanks, I fix this at V2.
>
>> + goto free_mem;
>> + }
>>
>>   vm_dev->vdev.dev.parent = >dev;
>>   vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
>
> (...)
>
>> @@ -573,7 +580,18 @@ static int virtio_mmio_probe(struct platform_device 
>> *pdev)
>>
>>   platform_set_drvdata(pdev, vm_dev);
>>
>> - return register_virtio_device(_dev->vdev);
>> + rc = register_virtio_device(_dev->vdev);
>> + if (rc)
>> + goto unmap;
>> + return 0;
>> +unmap:
>> + iounmap(vm_dev->base);
>> +free_mem:
>> + devm_release_mem_region(>dev, mem->start,
>> + resource_size(mem));
>> +free_vmdev:
>> + devm_kfree(>dev, vm_dev);
>
> I think this is problematic as vm_dev embeds a struct device (via
> embedding a struct virtio_device). I think the right way to do this is
> - call this only if register_virtio_device() has not been called
> - put the devm_kfree() into the ->release callback for the
>   virtio_device (IOW, replace virtio_mmio_release_dev_empty() with a
>   function that calls this)
> - do a put_device() if register_virtio_device() failed
>
> I might be missing some interaction between the usual driver model
> handling and devm resources, though.
>
if fail in virtio_mmio_probe, we need free @mem of @pdev, vdev->base,
so I prefer clean all this resource in virtio_mmio_probe itself, keep
vdev.dev.release do nothing.
also we need release these resources at virtio_mmio_remove.
I'll send V2.

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

Re: [PATCH] virtio: release virtio index when fail to device_register

[weiping zhang]

2017-12-04 17:38 GMT+08:00 Cornelia Huck :
> On Sat, 2 Dec 2017 00:55:39 +0800
> weiping zhang  wrote:
>
>> On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote:
>
>> > We hold an extra reference to the struct device, even after a failed
>> > register, and it is the responsibility of the caller to give up that
>> > reference once no longer needed. As callers toregister_virtio_device()
>> > embed the struct virtio_device, it needs to be their responsibility.
>> > Looking at the existing callers,
>> >
>> > - ccw does a put_device
>> > - pci, mmio and remoteproc do nothing, causing a leak
>> > - vop does a free on the embedding structure, which is a big no-no
>> >
>> > Thoughts?
>> Sorry to relay late and thanks for your review.
>> Do you mean the "extra reference to the struct device" caused by the
>> following code?
>>
>> err = device_register(>dev);
>>   device_add(dev)
>>   get_device(dev)
>> If I'm understand right, I think there is no extra reference if we fail
>> virtio_register_device, because if device_register we don't get a
>> reference.
>
> The device_initialize() already gives you a reference. If device_add()
> fails, it has cleaned up any additional reference it might have
> obtained, but the initial reference is still there and needs to be
> released by the caller.

Thanks your clarify, I also notice the comments at device_register,
device_initialize, device_add,

 * 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.

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

[PULL] vhost: cleanups and fixes

[Michael S. Tsirkin]

The following changes since commit c1d0c3f623ada808904dec676da0126f5b800630:

  fw_cfg: fix the command line module name (2017-11-14 23:57:40 +0200)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus

for you to fetch changes up to d9e427f6ab8142d6868eb719e6a7851aafea56b6:

  virtio_balloon: fix increment of vb->num_pfns in fill_balloon() (2017-12-01 
16:55:45 +0200)


virtio,qemu: bugfixes

A couple of bugfixes that just became ready.

Signed-off-by: Michael S. Tsirkin 


Jan Stancek (1):
  virtio_balloon: fix increment of vb->num_pfns in fill_balloon()

Marc-André Lureau (1):
  fw_cfg: fix driver remove

weiping zhang (1):
  virtio: release virtio index when fail to device_register

 drivers/firmware/qemu_fw_cfg.c  | 3 ++-
 drivers/virtio/virtio.c | 2 ++
 drivers/virtio/virtio_balloon.c | 3 +--
 3 files changed, 5 insertions(+), 3 deletions(-)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe

[Cornelia Huck]

On Sat, 2 Dec 2017 01:51:40 +0800
weiping zhang  wrote:

> cleanup all resource allocated by virtio_mmio_probe.
> 
> Signed-off-by: weiping zhang 
> ---
>  drivers/virtio/virtio_mmio.c | 34 ++
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 74dc717..3fd0e66 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device 
> *pdev)
>   return -EBUSY;
>  
>   vm_dev = devm_kzalloc(>dev, sizeof(*vm_dev), GFP_KERNEL);
> - if (!vm_dev)
> - return  -ENOMEM;
> + if (!vm_dev) {
> + rc =  -ENOMEM;

Touching this would be a good time to remove the extra space in front
of the -ENOMEM :)

> + goto free_mem;
> + }
>  
>   vm_dev->vdev.dev.parent = >dev;
>   vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;

(...)

> @@ -573,7 +580,18 @@ static int virtio_mmio_probe(struct platform_device 
> *pdev)
>  
>   platform_set_drvdata(pdev, vm_dev);
>  
> - return register_virtio_device(_dev->vdev);
> + rc = register_virtio_device(_dev->vdev);
> + if (rc)
> + goto unmap;
> + return 0;
> +unmap:
> + iounmap(vm_dev->base);
> +free_mem:
> + devm_release_mem_region(>dev, mem->start,
> + resource_size(mem));
> +free_vmdev:
> + devm_kfree(>dev, vm_dev);

I think this is problematic as vm_dev embeds a struct device (via
embedding a struct virtio_device). I think the right way to do this is
- call this only if register_virtio_device() has not been called
- put the devm_kfree() into the ->release callback for the
  virtio_device (IOW, replace virtio_mmio_release_dev_empty() with a
  function that calls this)
- do a put_device() if register_virtio_device() failed

I might be missing some interaction between the usual driver model
handling and devm resources, though.

> + return rc;
>  }
>  
>  static int virtio_mmio_remove(struct platform_device *pdev)

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

Re: [RFC] virtio-net: help live migrate SR-IOV devices

[achiad shochat]

On 3 December 2017 at 19:35, Stephen Hemminger
 wrote:
> On Sun, 3 Dec 2017 11:14:37 +0200
> achiad shochat  wrote:
>
>> On 3 December 2017 at 07:05, Michael S. Tsirkin  wrote:
>> > On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
>> >> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
>> >> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
>> >> > > Re. problem #2:
>> >> > > Indeed the best way to address it seems to be to enslave the VF driver
>> >> > > netdev under a persistent anchor netdev.
>> >> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
>> >> > > netdev to work in conjunction.
>> >> > > And it's indeed desired that this enslavement logic work out-of-the 
>> >> > > box.
>> >> > > But in case of PV+VF some configurable policies must be in place (and
>> >> > > they'd better be generic rather than differ per PV technology).
>> >> > > For example - based on which characteristics should the PV+VF coupling
>> >> > > be done? netvsc uses MAC address, but that might not always be the
>> >> > > desire.
>> >> >
>> >> > It's a policy but not guest userspace policy.
>> >> >
>> >> > The hypervisor certainly knows.
>> >> >
>> >> > Are you concerned that someone might want to create two devices with the
>> >> > same MAC for an unrelated reason?  If so, hypervisor could easily set a
>> >> > flag in the virtio device to say "this is a backup, use MAC to find
>> >> > another device".
>> >>
>> >> This is something I was going to suggest: a flag or other configuration on
>> >> the virtio device to help control how this new feature is used.  I can
>> >> imagine this might be useful to control from either the hypervisor side or
>> >> the VM side.
>> >>
>> >> The hypervisor might want to (1) disable it (force it off), (2) enable it
>> >> for VM choice, or (3) force it on for the VM.  In case (2), the VM might 
>> >> be
>> >> able to chose whether it wants to make use of the feature, or stick with 
>> >> the
>> >> bonding solution.
>> >>
>> >> Either way, the kernel is making a feature available, and the user (VM or
>> >> hypervisor) is able to control it by selecting the feature based on the
>> >> policy desired.
>> >>
>> >> sln
>> >
>> > I'm not sure what's the feature that is available here.
>> >
>> > I saw this as a flag that says "this device shares backend with another
>> > network device which can be found using MAC, and that backend should be
>> > preferred".  kernel then forces configuration which uses that other
>> > backend - as long as it exists.
>> >
>> > However, please Cc virtio-dev mailing list if we are doing this since
>> > this is a spec extension.
>> >
>> > --
>> > MST
>>
>>
>> Can someone please explain why assume a virtio device is there at all??
>> I specified a case where there isn't any.
>>
>> I second Jacob - having a netdev of one device driver enslave a netdev
>> of another device driver is an awkward a-symmetric model.
>> Regardless of whether they share the same backend device.
>> Only I am not sure the Linux Bond is the right choice.
>> e.g one may well want to use the virtio device also when the
>> pass-through device is available, e.g for multicasts, east-west
>> traffic, etc.
>> I'm not sure the Linux Bond fits that functionality.
>> And, as I hear in this thread, it is hard to make it work out of the box.
>> So I think the right thing would be to write a new dedicated module
>> for this purpose.
>>
>> Re policy -
>> Indeed the HV can request a policy from the guest but that's not a
>> claim for the virtio device enslaving the pass-through device.
>> Any policy can be queried by the upper enslaving device.
>>
>> Bottom line - I do not see a single reason to have the virtio netdev
>> (nor netvsc or any other PV netdev) enslave another netdev by itself.
>> If we'd do it right with netvsc from the beginning we wouldn't need
>> this discussion at all...
>
> There are several issues with transparent migration.
> The first is that the SR-IOV device needs to be shut off for earlier
> in the migration process.

That's not a given fact.
It's due to the DMA and it should be solve anyway.
Please read my first reply in this thread.

> Next, the SR-IOV device in the migrated go guest environment maybe different.
> It might not exist at all, it might be at a different PCI address, or it
> could even be a different vendor/speed/model.
> Keeping a virtual network device around allows persisting the connectivity,
> during the process.

Right, but that virtual device must not relate to any para-virt
specific technology (not netvsc, nor virtio).
Again, it seems you did not read my first reply.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio: release virtio index when fail to device_register

[Cornelia Huck]

On Sat, 2 Dec 2017 00:55:39 +0800
weiping zhang  wrote:

> On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote:

> > We hold an extra reference to the struct device, even after a failed
> > register, and it is the responsibility of the caller to give up that
> > reference once no longer needed. As callers toregister_virtio_device()
> > embed the struct virtio_device, it needs to be their responsibility.
> > Looking at the existing callers,
> > 
> > - ccw does a put_device
> > - pci, mmio and remoteproc do nothing, causing a leak
> > - vop does a free on the embedding structure, which is a big no-no
> > 
> > Thoughts?  
> Sorry to relay late and thanks for your review.
> Do you mean the "extra reference to the struct device" caused by the
> following code?
>  
> err = device_register(>dev);
>   device_add(dev)
>   get_device(dev)
> If I'm understand right, I think there is no extra reference if we fail
> virtio_register_device, because if device_register we don't get a
> reference.

The device_initialize() already gives you a reference. If device_add()
fails, it has cleaned up any additional reference it might have
obtained, but the initial reference is still there and needs to be
released by the caller.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization