From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40862) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aDuOB-00039w-Az for qemu-devel@nongnu.org; Tue, 29 Dec 2015 08:32:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aDuO8-0002rW-4G for qemu-devel@nongnu.org; Tue, 29 Dec 2015 08:32:35 -0500 Received: from [59.151.112.132] (port=48395 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aDuO6-0002r6-MD for qemu-devel@nongnu.org; Tue, 29 Dec 2015 08:32:32 -0500 References: <1451378763-25398-1-git-send-email-caoj.fnst@cn.fujitsu.com> <20151229130853-mutt-send-email-mst@redhat.com> From: Cao jin Message-ID: <56828A5E.8060202@cn.fujitsu.com> Date: Tue, 29 Dec 2015 21:27:58 +0800 MIME-Version: 1.0 In-Reply-To: <20151229130853-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4] igd-passthrough-i440FX: convert to realize() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, stefano.stabellini@eu.citrix.com Agree with your review point. Since can`t get contact with author tiejun.chen@intel.com, maybe only can ask for test help from Stefano, I will send next version when we get in touch with stefano. On 12/29/2015 07:25 PM, Michael S. Tsirkin wrote: > On Tue, Dec 29, 2015 at 04:46:03PM +0800, Cao jin wrote: >> Signed-off-by: Cao jin >> --- >> changelog v4: >> 1. strip the bugfix code, but I guess this patch should be applied after >> that bugfix patch. >> 2. Other little fix as per previous review >> >> Some explanation to previous review: >> 1. error_setg_errno() I use in this patch will print the strerror(errno), so I >> guess it will be informative enough? example: >> >> error_setg_errno(errp, errno, "open %s fail", path); will print: >> "open bluhbluh fail: string_from_strerrer(errno)" > > I'd prefer some mention of why this is attempted too. > E.g. "igd passthrough : open bluhbluh fail" > >> 2. snprintf() has tiny tiny chance to fail, > > It just formats a string into a buffer. the buffer > is pre-allocated, and we know it's large enough for the data. > How can it fail? > > >> I don`t assert here, because this >> device is on-board, it init during machine initialization, in case it fails, >> the errp we set will results in error_report(err) and exit(1), at least can >> let user know why fail. see the call-chain: >> >> pc_xen_hvm_init_pci->pc_init1->i440fx_init->pci_create_simple-> pci_create_simple_multifunction->qdev_init_nofail > > unlike open failing, there's nothing user can do, this means there's > a coding bug somewhere, and assert is easier to debug. > >> About test: >> 1. Compiled >> 2. Did a dirty hack to force create/realize this device in pc_init1(), prove >> that the realizing process is ok, I will reply this mail later to attch the >> screenshot evidence > > Good job, thanks! > You should also Cc Tiejun Chen who wrote this > code, presumably with access to hardware to test on. > > >> hw/pci-host/piix.c | 30 +++++++++++++++--------------- >> 1 file changed, 15 insertions(+), 15 deletions(-) >> >> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c >> index a9cb983..e91570f 100644 >> --- a/hw/pci-host/piix.c >> +++ b/hw/pci-host/piix.c >> @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = { >> {0xa8, 4}, /* SNB: base of GTT stolen memory */ >> }; >> >> -static int host_pci_config_read(int pos, int len, uint32_t *val) >> +static void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp) >> { >> char path[PATH_MAX]; >> int config_fd; >> @@ -769,19 +769,19 @@ static int host_pci_config_read(int pos, int len, uint32_t *val) >> /* Access real host bridge. */ >> int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s", >> 0, 0, 0, 0, "config"); >> - int ret = 0; >> >> if (rc >= size || rc < 0) { >> - return -ENODEV; >> + error_setg_errno(errp, errno, "snprintf err"); >> } >> >> config_fd = open(path, O_RDWR); >> if (config_fd < 0) { >> - return -ENODEV; >> + error_setg_errno(errp, errno, "open %s fail", path); >> + return; >> } >> >> if (lseek(config_fd, pos, SEEK_SET) != pos) { >> - ret = -errno; >> + error_setg_errno(errp, errno, "lseek err"); >> goto out; >> } >> >> @@ -789,32 +789,32 @@ static int host_pci_config_read(int pos, int len, uint32_t *val) >> rc = read(config_fd, (uint8_t *)val, len); >> } while (rc < 0 && (errno == EINTR || errno == EAGAIN)); >> if (rc != len) { >> - ret = -errno; >> + error_setg_errno(errp, errno, "read err"); >> } >> >> out: >> close(config_fd); >> - return ret; >> } >> >> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev) >> +static void igd_pt_i440fx_realize(struct PCIDevice *pci_dev, Error **errp) >> { >> uint32_t val = 0; >> - int rc, i, num; >> + int i, num; >> int pos, len; >> + Error *local_err = NULL; >> >> num = ARRAY_SIZE(igd_host_bridge_infos); >> for (i = 0; i < num; i++) { >> pos = igd_host_bridge_infos[i].offset; >> len = igd_host_bridge_infos[i].len; >> - rc = host_pci_config_read(pos, len, &val); >> - if (rc) { >> - return -ENODEV; >> + >> + host_pci_config_read(pos, len, &val, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; >> } >> pci_default_write_config(pci_dev, pos, cpu_to_le32(val), len); >> } >> - >> - return 0; >> } >> >> static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data) >> @@ -822,7 +822,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data) >> DeviceClass *dc = DEVICE_CLASS(klass); >> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> >> - k->init = igd_pt_i440fx_initfn; >> + k->realize = igd_pt_i440fx_realize; >> dc->desc = "IGD Passthrough Host bridge"; >> } >> >> -- >> 2.1.0 >> >> > > > . > -- Yours Sincerely, Cao Jin