Re: [Xen-devel] [PATCH] xen_pt: fix failure of attaching & detaching a PCI device to VM repeatedly

["Chang, JianzhongX" <jianzhongx.chang@xxxxxxxxx>]

>> An error message will be reported like this:
>> "libxl: error: libxl_qmp.c:287:qmp_handle_error_response: received an 
>> error message from QMP server: Duplicate ID 'pci-pt-07_10.1' for device"
>> 
>> When xen_pt_region_add/del() is called, MemoryRegion may not belong to 
>> the XenPCIPassthroughState.
>> xen_pt_region_update() checks it but memory_region_ref/unref() does not.
>> This case causes obj->ref issue and affects the release of related objects.
>> So, the detection operation is moved from xen_pt_region_update to 
>> xen_pt_region_add/del.
>> 
>> Signed-off-by: Jianzhong,Chang <jianzhongx.chang@xxxxxxxxx>
>> ---
>>  hw/xen/xen_pt.c |   23 ++++++++++++++++++++---
>>  1 files changed, 20 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index aa96288..95b4970 
>> 100644
>> --- a/hw/xen/xen_pt.c
>> +++ b/hw/xen/xen_pt.c
>> @@ -523,6 +523,14 @@ static int 
>> xen_pt_bar_from_region(XenPCIPassthroughState *s, MemoryRegion *mr)
>>      }
>>      return -1;
>>  }
>> +static bool xen_pt_region_in_state(XenPCIPassthroughState *s, 
>> +MemoryRegion *mr) {
>> +    int bar = xen_pt_bar_from_region(s, mr);
>> +    if (-1 == bar && (!s->msix || &s->msix->mmio != mr)) {
>> +        return false;
>> +    }
>> +    return true;
>> +}
>>  
>>  /*
>>   * This function checks if an io_region overlaps an io_region from 
>> another @@ -587,9 +595,6 @@ static void 
>> xen_pt_region_update(XenPCIPassthroughState *s,
>>      };
>>  
>>      bar = xen_pt_bar_from_region(s, mr);
>> -    if (bar == -1 && (!s->msix || &s->msix->mmio != mr)) {
>> -        return;
>> -    }
>>  
>>      if (s->msix && &s->msix->mmio == mr) {
>>          if (adding) {
>> @@ -642,6 +647,9 @@ static void xen_pt_region_add(MemoryListener *l, 
>> MemoryRegionSection *sec)
>>      XenPCIPassthroughState *s = container_of(l, XenPCIPassthroughState,
>>                                               memory_listener);
>>  
>> +    if (!xen_pt_region_in_state(s, sec->mr)) {
>> +        return;
>> +    }
>>      memory_region_ref(sec->mr);
>>      xen_pt_region_update(s, sec, true);  } @@ -651,6 +659,9 @@ static 
>> void xen_pt_region_del(MemoryListener *l, MemoryRegionSection *sec)
>>      XenPCIPassthroughState *s = container_of(l, XenPCIPassthroughState,
>>                                               memory_listener);
>>  
>> +    if (!xen_pt_region_in_state(s, sec->mr)) {
>> +        return;
>> +    }
>>      xen_pt_region_update(s, sec, false);
>>      memory_region_unref(sec->mr);
>>  }
>> @@ -660,6 +671,9 @@ static void xen_pt_io_region_add(MemoryListener *l, 
>> MemoryRegionSection *sec)
>>      XenPCIPassthroughState *s = container_of(l, XenPCIPassthroughState,
>>                                               io_listener);
>>  
>> +    if (!xen_pt_region_in_state(s, sec->mr)) {
>> +        return;
>> +    }
>>      memory_region_ref(sec->mr);
>>      xen_pt_region_update(s, sec, true);  } @@ -669,6 +683,9 @@ static 
>> void xen_pt_io_region_del(MemoryListener *l, MemoryRegionSection *sec)
>>      XenPCIPassthroughState *s = container_of(l, XenPCIPassthroughState,
>>                                               io_listener);
>>  
>> +    if (!xen_pt_region_in_state(s, sec->mr)) {
>> +        return;
>> +    }
>>      xen_pt_region_update(s, sec, false);
>>      memory_region_unref(sec->mr);
>>  }
>
>Wouldn't it make more sense to move the
>memory_region_ref/memory_region_unref calls inside xen_pt_region_update?

Move the operation  of mr->ref inside xen_pt_region_update can also fix this 
bug. 
For the convenience of maintenance, another patch can be created.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel