From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55419) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCQTD-0006AD-Fd for qemu-devel@nongnu.org; Mon, 13 Jun 2016 07:55:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCQT7-0007pF-Ho for qemu-devel@nongnu.org; Mon, 13 Jun 2016 07:55:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41461) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCQT7-0007p9-98 for qemu-devel@nongnu.org; Mon, 13 Jun 2016 07:55:49 -0400 References: <1465552478-5540-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1465552478-5540-13-git-send-email-caoj.fnst@cn.fujitsu.com> <575E8814.8090505@redhat.com> <575E944E.6030707@cn.fujitsu.com> From: Marcel Apfelbaum Message-ID: <575E9F40.7020009@redhat.com> Date: Mon, 13 Jun 2016 14:55:44 +0300 MIME-Version: 1.0 In-Reply-To: <575E944E.6030707@cn.fujitsu.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin , qemu-devel@nongnu.org Cc: Gerd Hoffmann , John Snow , Dmitry Fleytman , Jason Wang , "Michael S. Tsirkin" , Hannes Reinecke , Paolo Bonzini , Alex Williamson , Markus Armbruster 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 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 >> >