[Qemu-devel] [PATCH] virtio-pci: fix memory MR cleanup for modern

2015-07-27 Thread Michael S. Tsirkin 
Each memory_region_add_subregion must be paired with
memory_region_del_subregion.

Signed-off-by: Michael S. Tsirkin 
---
 hw/virtio/virtio-pci.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index db8f27c..c024161 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1414,6 +1414,13 @@ static void virtio_pci_modern_region_map(VirtIOPCIProxy 
*proxy,
 virtio_pci_add_mem_cap(proxy, cap);
 }
 
+static void virtio_pci_modern_region_unmap(VirtIOPCIProxy *proxy,
+   VirtIOPCIRegion *region)
+{
+memory_region_del_subregion(&proxy->modern_bar,
+®ion->mr);
+}
+
 /* This is called by virtio-bus just after the device is plugged. */
 static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
 {
@@ -1520,8 +1527,16 @@ static void virtio_pci_device_plugged(DeviceState *d, 
Error **errp)
 static void virtio_pci_device_unplugged(DeviceState *d)
 {
 VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
+bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
 
 virtio_pci_stop_ioeventfd(proxy);
+
+if (modern) {
+virtio_pci_modern_region_unmap(proxy, &proxy->common);
+virtio_pci_modern_region_unmap(proxy, &proxy->isr);
+virtio_pci_modern_region_unmap(proxy, &proxy->device);
+virtio_pci_modern_region_unmap(proxy, &proxy->notify);
+}
 }
 
 static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
-- 
MST

Re: [Qemu-devel] [PATCH] virtio-pci: fix memory MR cleanup for modern

2015-07-27 Thread Paolo Bonzini 


On 27/07/2015 15:24, Michael S. Tsirkin wrote:
> +static void virtio_pci_modern_region_unmap(VirtIOPCIProxy *proxy,
> +   VirtIOPCIRegion *region)
> +{
> +memory_region_del_subregion(&proxy->modern_bar,
> +®ion->mr);
> +}
> +
>  /* This is called by virtio-bus just after the device is plugged. */
>  static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>  {
> @@ -1520,8 +1527,16 @@ static void virtio_pci_device_plugged(DeviceState *d, 
> Error **errp)
>  static void virtio_pci_device_unplugged(DeviceState *d)
>  {
>  VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> +bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
>  
>  virtio_pci_stop_ioeventfd(proxy);
> +
> +if (modern) {
> +virtio_pci_modern_region_unmap(proxy, &proxy->common);
> +virtio_pci_modern_region_unmap(proxy, &proxy->isr);
> +virtio_pci_modern_region_unmap(proxy, &proxy->device);
> +virtio_pci_modern_region_unmap(proxy, &proxy->notify);
> +}
>  }

Actually this is not necessary.  memory_region_del_subregion is only
needed inasmuch as it prevents further guest access to the region, so
it's enough that the toplevel region (the modern_bar itself) is
unmapped.  The PCI core does that automatically.

That said, it's polite to unmap everything, so if you want this patch:

Reviewed-by: Paolo Bonzini 

Paolo

Re: [Qemu-devel] [PATCH] virtio-pci: fix memory MR cleanup for modern

2015-07-27 Thread Paolo Bonzini 


On 27/07/2015 15:30, Paolo Bonzini wrote:
> 
> 
> On 27/07/2015 15:24, Michael S. Tsirkin wrote:
>> +static void virtio_pci_modern_region_unmap(VirtIOPCIProxy *proxy,
>> +   VirtIOPCIRegion *region)
>> +{
>> +memory_region_del_subregion(&proxy->modern_bar,
>> +®ion->mr);
>> +}
>> +
>>  /* This is called by virtio-bus just after the device is plugged. */
>>  static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>>  {
>> @@ -1520,8 +1527,16 @@ static void virtio_pci_device_plugged(DeviceState *d, 
>> Error **errp)
>>  static void virtio_pci_device_unplugged(DeviceState *d)
>>  {
>>  VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>> +bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
>>  
>>  virtio_pci_stop_ioeventfd(proxy);
>> +
>> +if (modern) {
>> +virtio_pci_modern_region_unmap(proxy, &proxy->common);
>> +virtio_pci_modern_region_unmap(proxy, &proxy->isr);
>> +virtio_pci_modern_region_unmap(proxy, &proxy->device);
>> +virtio_pci_modern_region_unmap(proxy, &proxy->notify);
>> +}
>>  }
> 
> Actually this is not necessary.  memory_region_del_subregion is only
> needed inasmuch as it prevents further guest access to the region, so
> it's enough that the toplevel region (the modern_bar itself) is
> unmapped.  The PCI core does that automatically.
> 
> That said, it's polite to unmap everything, so if you want this patch:
> 
> Reviewed-by: Paolo Bonzini 

After discussing on IRC the problem (which is that
memory_region_add_subregion add a reference to the VirtioPCIProxy
object, and you want to remove it), this is indeed the correct fix.

Paolo