diff mbox

[v3,4/4] Xen PCI passthru: convert to realize()

Message ID 1452047951-17429-5-git-send-email-caoj.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Cao jin Jan. 6, 2016, 2:39 a.m. UTC
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen/xen_pt.c | 53 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

Comments

Eric Blake Jan. 6, 2016, 4:07 p.m. UTC | #1
On 01/05/2016 07:39 PM, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/xen/xen_pt.c | 53 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 25 deletions(-)
> 

> @@ -801,19 +801,19 @@ static int xen_pt_initfn(PCIDevice *d)
>      if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
>          (s->real_device.dev == 2) && (s->real_device.func == 0)) {
>          if (!is_igd_vga_passthrough(&s->real_device)) {
> -            XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
> -                       " to passthrough IGD GFX.\n");
> +            error_setg(errp, "Need to enable igd-passthru if you're trying"
> +                    " to passthrough IGD GFX.");

No trailing '.' in error_setg() messages.


> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
>      xen_pt_config_init(s, &local_err);
>      if (local_err) {
>          error_append_hint(&local_err, "PCI Config space initialisation failed");
> -        rc = -1;
> +        error_propagate(errp, local_err);
>          goto err_out;
>      }

Looks like this fixes a memory leak in an earlier patch; maybe you need
to shuffle hunks around?

>  
>      /* Bind interrupt */
>      rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
>      if (rc) {
> -        XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
> +        error_setg_errno(errp, errno, "Failed to read PCI_INTERRUPT_PIN!");

No trailing '!'


> @@ -891,14 +890,14 @@ out:
>  
>          rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
>          if (rc) {
> -            XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
> +            error_setg_errno(errp, errno, "Failed to read PCI_COMMAND!");

and again


> @@ -911,12 +910,16 @@ out:
>                 "Real physical device %02x:%02x.%d registered successfully!\n",
>                 s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
>  
> -    return 0;
> +    return;
>  
>  err_out:
> +    for (i = 0; i < PCI_ROM_SLOT; i++) {
> +        object_unparent(OBJECT(&s->bar[i]));
> +    }
> +    object_unparent(OBJECT(&s->rom));
> +
>      xen_pt_destroy(d);
>      assert(rc);
> -    return rc;

Is the assertion still needed?
Cao jin Jan. 7, 2016, 7:16 a.m. UTC | #2
On 01/07/2016 12:07 AM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
[...]
>
>> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
>>       xen_pt_config_init(s, &local_err);
>>       if (local_err) {
>>           error_append_hint(&local_err, "PCI Config space initialisation failed");
>> -        rc = -1;
>> +        error_propagate(errp, local_err);
>>           goto err_out;
>>       }
>
> Looks like this fixes a memory leak in an earlier patch; maybe you need
> to shuffle hunks around?
>

Sorry, don`t quite undertand what "shuffle hunks around" means, could 
you detail it?

[...]
>>
>>   err_out:
>> +    for (i = 0; i < PCI_ROM_SLOT; i++) {
>> +        object_unparent(OBJECT(&s->bar[i]));
>> +    }
>> +    object_unparent(OBJECT(&s->rom));
>> +
>>       xen_pt_destroy(d);
>>       assert(rc);
>> -    return rc;
>
> Is the assertion still needed?
>

Actually I think in the original code, the assertion isn`t necessary 
too, but I guess it is a kind of defensive coding? You can see, not very 
if (rc) equals TRUE will goto err_out. So I prefer not to touch it when 
I am not sure about the author`s intent.
Eric Blake Jan. 7, 2016, 4:53 p.m. UTC | #3
On 01/07/2016 12:16 AM, Cao jin wrote:
> 
> 
> On 01/07/2016 12:07 AM, Eric Blake wrote:
>> On 01/05/2016 07:39 PM, Cao jin wrote:
> [...]
>>
>>> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
>>>       xen_pt_config_init(s, &local_err);
>>>       if (local_err) {
>>>           error_append_hint(&local_err, "PCI Config space
>>> initialisation failed");
>>> -        rc = -1;
>>> +        error_propagate(errp, local_err);
>>>           goto err_out;
>>>       }
>>
>> Looks like this fixes a memory leak in an earlier patch; maybe you need
>> to shuffle hunks around?
>>
> 
> Sorry, don`t quite undertand what "shuffle hunks around" means, could
> you detail it?

It means this change should occur in an earlier patch (probably 2/4?).
I shuffle hunks by using 'git rebase -i'.
diff mbox

Patch

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 3787c26..b058f61 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -760,10 +760,10 @@  static void xen_pt_destroy(PCIDevice *d) {
 }
 /* init */
 
-static int xen_pt_initfn(PCIDevice *d)
+static void xen_pt_realize(PCIDevice *d, Error **errp)
 {
     XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
-    int rc = 0;
+    int i, rc = 0;
     uint8_t machine_irq = 0, scratch;
     uint16_t cmd = 0;
     int pirq = XEN_PT_UNASSIGNED_PIRQ;
@@ -780,8 +780,8 @@  static int xen_pt_initfn(PCIDevice *d)
                             s->hostaddr.slot, s->hostaddr.function,
                             &local_err);
     if (local_err) {
-        XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
-        return -1;
+        error_propagate(errp, local_err);
+        return;
     }
 
     s->is_virtfn = s->real_device.is_virtfn;
@@ -801,19 +801,19 @@  static int xen_pt_initfn(PCIDevice *d)
     if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
         (s->real_device.dev == 2) && (s->real_device.func == 0)) {
         if (!is_igd_vga_passthrough(&s->real_device)) {
-            XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
-                       " to passthrough IGD GFX.\n");
+            error_setg(errp, "Need to enable igd-passthru if you're trying"
+                    " to passthrough IGD GFX.");
             xen_host_pci_device_put(&s->real_device);
-            return -1;
+            return;
         }
 
         xen_pt_setup_vga(s, &s->real_device, &local_err);
         if (local_err) {
             error_append_hint(&local_err, "Setup VGA BIOS of passthrough"
                     " GFX failed!");
-            XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
+            error_propagate(errp, local_err);
             xen_host_pci_device_put(&s->real_device);
-            return -1;
+            return;
         }
 
         /* Register ISA bridge for passthrough GFX. */
@@ -827,27 +827,26 @@  static int xen_pt_initfn(PCIDevice *d)
     xen_pt_config_init(s, &local_err);
     if (local_err) {
         error_append_hint(&local_err, "PCI Config space initialisation failed");
-        rc = -1;
+        error_propagate(errp, local_err);
         goto err_out;
     }
 
     /* Bind interrupt */
     rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
     if (rc) {
-        XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
+        error_setg_errno(errp, errno, "Failed to read PCI_INTERRUPT_PIN!");
         goto err_out;
     }
     if (!scratch) {
-        XEN_PT_LOG(d, "no pin interrupt\n");
+        error_setg(errp, "no pin interrupt");
         goto out;
     }
 
     machine_irq = s->real_device.irq;
     rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
-
     if (rc < 0) {
-        XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
-                   machine_irq, pirq, errno);
+        error_setg_errno(errp, errno, "Mapping machine irq %u to"
+                         " pirq %i failed", machine_irq, pirq);
 
         /* Disable PCI intx assertion (turn on bit10 of devctl) */
         cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -868,8 +867,8 @@  static int xen_pt_initfn(PCIDevice *d)
                                        PCI_SLOT(d->devfn),
                                        e_intx);
         if (rc < 0) {
-            XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
-                       e_intx, errno);
+            error_setg_errno(errp, errno, "Binding of interrupt %i failed!",
+                             e_intx);
 
             /* Disable PCI intx assertion (turn on bit10 of devctl) */
             cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -877,8 +876,8 @@  static int xen_pt_initfn(PCIDevice *d)
 
             if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
                 if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
-                    XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
-                               " (err: %d)\n", machine_irq, errno);
+                    error_setg_errno(errp, errno, "Unmapping of machine"
+                            " interrupt %i failed!", machine_irq);
                 }
             }
             s->machine_irq = 0;
@@ -891,14 +890,14 @@  out:
 
         rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
         if (rc) {
-            XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
+            error_setg_errno(errp, errno, "Failed to read PCI_COMMAND!");
             goto err_out;
         } else {
             val |= cmd;
             rc = xen_host_pci_set_word(&s->real_device, PCI_COMMAND, val);
             if (rc) {
-                XEN_PT_ERR(d, "Failed to write PCI_COMMAND val=0x%x!(rc: %d)\n",
-                           val, rc);
+                error_setg_errno(errp, errno, "Failed to write PCI_COMMAND"
+                                 " val=0x%x!", val);
                 goto err_out;
             }
         }
@@ -911,12 +910,16 @@  out:
                "Real physical device %02x:%02x.%d registered successfully!\n",
                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
 
-    return 0;
+    return;
 
 err_out:
+    for (i = 0; i < PCI_ROM_SLOT; i++) {
+        object_unparent(OBJECT(&s->bar[i]));
+    }
+    object_unparent(OBJECT(&s->rom));
+
     xen_pt_destroy(d);
     assert(rc);
-    return rc;
 }
 
 static void xen_pt_unregister_device(PCIDevice *d)
@@ -935,7 +938,7 @@  static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = xen_pt_initfn;
+    k->realize = xen_pt_realize;
     k->exit = xen_pt_unregister_device;
     k->config_read = xen_pt_pci_read_config;
     k->config_write = xen_pt_pci_write_config;