[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and f

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it
Date: Mon, 13 Jun 2016 14:55:44 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

On 06/13/2016 02:09 PM, Cao jin wrote:

On 06/13/2016 06:16 PM, Marcel Apfelbaum wrote:
On 06/10/2016 12:54 PM, Cao jin wrote:

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 692283f..a06d184 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s)
      int res;

-    res = msi_init(PCI_DEVICE(s),
-                   0xD0,   /* MSI capability offset              */
-                   1,      /* MAC MSI interrupts                 */
-                   true,   /* 64-bit message addresses supported */
-                   false); /* Per vector mask supported          */
+    res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);

Why did you chose to remove author's comments?

Because msi_init() has its function comments now, which is the same is the 
author`s comments, I guess author add these comments because function has no 
comment before, remove comments also is to save
screen space:)

some macros of some devices is also unnecessary I think, because we have 
comments of msi_init().


+#define VMXNET3_USE_64BIT         (true)
+#define VMXNET3_PER_VECTOR_MASK   (false)

like these macros, but it does't take too much space as above one I feel, so I 
didn't touch them.

@@ -56,14 +57,17 @@ static int xio3130_upstream_initfn(PCIDevice *d)
      PCIEPort *p = PCIE_PORT(d);
      int rc;
+    Error *err = NULL;

      pci_bridge_initfn(d, TYPE_PCIE_BUS);

      rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
                    XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
+                  XIO3130_MSI_SUPPORTED_FLAGS &
      if (rc < 0) {
+        assert(rc == -ENOTSUP);
+        error_report_err(err);

Hi Jin, Markus

It looks a little weird to me to check for negative error code
and then assert for a specific error as the only "valid error".
Maybe we should assert inside msi_init to leave the callers "clean"?

Hi Marcel,

I think it is because: except assigned device(vfio), the emulated pci devices 
won`t have config space overlapped(-EINVAL), unless programming error.

Understood, thanks for the explanation.

For the PCI part:
Reviewed-by: Marcel Apfelbaum <address@hidden>


If implemented as you said, I guess there need a judgement (if..else..) to 
check if current device is assigned in msi_init(), or else assert the error

I am well aware a lot of time was spent for this series, but I just
spotted it and I want to be sure we are doing it right.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]