|
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().
Right.
+ +#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); pcie_port_init_reg(d); rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err); 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> Thanks, Marcel
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. Thanks, Marcel
[Prev in Thread] | Current Thread | [Next in Thread] |