From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56296) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2Uj1-00047b-Oz for qemu-devel@nongnu.org; Thu, 03 Nov 2016 22:59:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2Uiy-0008DW-EB for qemu-devel@nongnu.org; Thu, 03 Nov 2016 22:59:27 -0400 Received: from [59.151.112.132] (port=15156 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2Uix-0008BG-Fi for qemu-devel@nongnu.org; Thu, 03 Nov 2016 22:59:24 -0400 References: <1478145997-28865-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1478145997-28865-4-git-send-email-caoj.fnst@cn.fujitsu.com> <99c7ca3e-47ac-7c35-99b8-93fbda09a48f@redhat.com> From: Cao jin Message-ID: <581BFA20.4090802@cn.fujitsu.com> Date: Fri, 4 Nov 2016 11:01:52 +0800 MIME-Version: 1.0 In-Reply-To: <99c7ca3e-47ac-7c35-99b8-93fbda09a48f@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 03/10] pci: Convert msix_init() to Error and fix callers to check it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum , qemu-devel@nongnu.org Cc: Jiri Pirko , Gerd Hoffmann , Dmitry Fleytman , Jason Wang , "Michael S. Tsirkin" , Hannes Reinecke , Paolo Bonzini , Alex Williamson , Markus Armbruster On 11/03/2016 07:38 PM, Marcel Apfelbaum wrote: > On 11/03/2016 06:06 AM, Cao jin wrote: >> > > [...] > >> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c >> index 52a4123..fada834 100644 >> --- a/hw/scsi/megasas.c >> +++ b/hw/scsi/megasas.c >> @@ -2360,9 +2360,12 @@ static void megasas_scsi_realize(PCIDevice >> *dev, Error **errp) >> >> if (megasas_use_msix(s) && >> msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, >> - &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { >> + &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) { >> + /*TODO: check msix_init's error, and should fail on msix=on */ > > Why this "TODO", can't we do something similar to other changes already > done in this patch? > The 1st version of this series handles the error in this patch. look at the comments: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg03192.html *First convert msix_init() without changing behavior, by having every caller of msix_init() immediately pass the error received to error_report_err(). Then clean up the callers one after the other.* So later, this patch looks like what it is now. I feel it also make this patch thinner, easier to review. >> + error_report_err(err); >> s->msix = ON_OFF_AUTO_OFF; >> } >> + >> if (pci_is_express(dev)) { >> pcie_endpoint_cap_init(dev, 0xa0); >> } >> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index d7dbe0e..6fbd30f 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -1432,6 +1432,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice >> *vdev, Error **errp) >> static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) >> { >> int ret; >> + Error *err = NULL; >> >> vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) * >> sizeof(unsigned long)); >> @@ -1439,7 +1440,8 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, >> int pos, Error **errp) >> vdev->bars[vdev->msix->table_bar].region.mem, >> vdev->msix->table_bar, vdev->msix->table_offset, >> vdev->bars[vdev->msix->pba_bar].region.mem, >> - vdev->msix->pba_bar, vdev->msix->pba_offset, pos); >> + vdev->msix->pba_bar, vdev->msix->pba_offset, pos, >> + &err); > > Do we pass the err pointer to msix_init, but we don't do anything with it? > > Also since we do have an *errp in the function already, I suggest > renaming err -> local_err or something. (only if the series needs a > re-spin) > yes, it maybe need a re-spin, thanks >> if (ret < 0) { >> if (ret == -ENOTSUP) { >> return 0; >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index 06831de..5acce38 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -1693,13 +1693,12 @@ static void >> virtio_pci_device_plugged(DeviceState *d, Error **errp) >> >> if (proxy->nvectors) { >> int err = msix_init_exclusive_bar(&proxy->pci_dev, >> proxy->nvectors, >> - proxy->msix_bar_idx); >> + proxy->msix_bar_idx, errp); >> + /* Any error other than -ENOTSUP(board's MSI support is broken) >> + * is a programming error. */ >> + assert(!err || err == -ENOTSUP); >> if (err) { >> - /* Notice when a system that supports MSIx can't >> initialize it. */ >> - if (err != -ENOTSUP) { >> - error_report("unable to init msix vectors to %" PRIu32, >> - proxy->nvectors); >> - } >> + error_report_err(*errp); > > Why do we report the error here and we don't let the propagation > mechanism do its thing? We can prep-end the current message, I think. > The original behaviour won't fail on msix init failure, so, report & free the Error keep the behaviour same as before, propagation will results in failing to create virtio device. > > > Other than a few questions, the patch looks good to me. > > Thanks! > Marcel > -- Yours Sincerely, Cao jin