From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43416) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1DWZ-0004vB-Tu for qemu-devel@nongnu.org; Sun, 29 Jun 2014 07:44:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X1DWT-0000p9-PA for qemu-devel@nongnu.org; Sun, 29 Jun 2014 07:43:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21765) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1DWT-0000p4-Gy for qemu-devel@nongnu.org; Sun, 29 Jun 2014 07:43:53 -0400 Date: Sun, 29 Jun 2014 14:43:58 +0300 From: "Michael S. Tsirkin" Message-ID: <20140629114358.GB26161@redhat.com> References: <1403662641-28526-1-git-send-email-tiejun.chen@intel.com> <1403662641-28526-6-git-send-email-tiejun.chen@intel.com> <20140625071321.GD25563@redhat.com> <53AD37CA.3040800@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53AD37CA.3040800@intel.com> Subject: Re: [Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Chen, Tiejun" Cc: peter.maydell@linaro.org, xen-devel@lists.xensource.com, stefano.stabellini@eu.citrix.com, allen.m.kay@intel.com, qemu-devel@nongnu.org, Kelly.Zytaruk@amd.com, yang.z.zhang@intel.com, anthony@codemonkey.ws, anthony.perard@citrix.com, pbonzini@redhat.com On Fri, Jun 27, 2014 at 05:22:18PM +0800, Chen, Tiejun wrote: > On 2014/6/25 15:13, Michael S. Tsirkin wrote: > >On Wed, Jun 25, 2014 at 10:17:21AM +0800, Tiejun Chen wrote: > > [snip] > > >>diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h > >>index 507165c..25147cf 100644 > >>--- a/hw/xen/xen_pt.h > >>+++ b/hw/xen/xen_pt.h > >>@@ -63,7 +63,7 @@ typedef int (*xen_pt_conf_byte_read) > >> #define XEN_PT_BAR_UNMAPPED (-1) > >> > >> #define PCI_CAP_MAX 48 > >>- > >>+#define PCI_INTEL_OPREGION 0xfc > >> > > > >XEN_.... please > > > >PCI_CAP_MAX should be fixed too. > > They are specific to PCI, not XEN. They are? Where in the PCI spec does it say 48? Same for PCI_INTEL_OPREGION. > Why should we add such a prefix? So that people working on core pci do not have to worry about breaking your devices by adding a symbol in the global header. > > > > > > [snip] > > >> > >>+ if (igd_guest_opregion) { > >>+ ret = xc_domain_memory_mapping(xen_xc, xen_domid, > >>+ (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT), > >>+ (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), > > > >don't spread casts all around. > >Should be a last resort. > > Okay. > > > > >>+ 3, > >>+ DPCI_REMOVE_MAPPING); > >>+ if (ret) { > >>+ return ret; > >>+ } > >>+ } > >>+ > >> return 0; > >> } > >> > >>@@ -447,3 +462,52 @@ err_out: > >> XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); > >> return -1; > >> } > >>+ > >>+uint32_t igd_read_opregion(XenPCIPassthroughState *s) > >>+{ > >>+ uint32_t val = 0; > >>+ > >>+ if (igd_guest_opregion == 0) { > > > >!igd_guest_opregion is shorter and does the same, > > Okay. > > > > >>+ return val; > >>+ } > >>+ > >>+ val = igd_guest_opregion; > >>+ > >>+ XEN_PT_LOG(&s->dev, "Read opregion val=%x\n", val); > >>+ return val; > >>+} > >>+ > >>+void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val) > >>+{ > >>+ int ret; > >>+ > >>+ if (igd_guest_opregion) { > >>+ XEN_PT_LOG(&s->dev, "opregion register already been set, ignoring %x\n", > >>+ val); > >>+ return; > >>+ } > >>+ > >>+ xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION, > >>+ (uint8_t *)&igd_host_opregion, 4); > >>+ igd_guest_opregion = (unsigned long)(val & ~0xfff) > >>+ | (igd_host_opregion & 0xfff); > >>+ > > > >Clearly broken on BE. > > I still can't understand why we need to address this in BE case. So code is clean and reusable. Copy and paste is a fact of life, you don't want people to inherit bugs. If some code absolutely must be LE specific, it needs a comment that explains this and cautions people against trying to use it elsewhere in QEMU. > >Maybe not important here but writing clean code is > >just as easy. > >uint8_t igd_host_opregion[4]; > > > >... > > > > xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION, > > igd_host_opregion, sizeof igd_host_opregion); > > > > igd_guest_opregion = (val & ~0xfff) | > > (pci_get_word(igd_host_opregion) & 0xfff); > > > >0xfff should be a macro too to avoid duplication. > > > > Okay. > > Thanks > Tiejun From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping Date: Sun, 29 Jun 2014 14:43:58 +0300 Message-ID: <20140629114358.GB26161@redhat.com> References: <1403662641-28526-1-git-send-email-tiejun.chen@intel.com> <1403662641-28526-6-git-send-email-tiejun.chen@intel.com> <20140625071321.GD25563@redhat.com> <53AD37CA.3040800@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <53AD37CA.3040800@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: "Chen, Tiejun" Cc: peter.maydell@linaro.org, xen-devel@lists.xensource.com, stefano.stabellini@eu.citrix.com, allen.m.kay@intel.com, qemu-devel@nongnu.org, Kelly.Zytaruk@amd.com, yang.z.zhang@intel.com, anthony@codemonkey.ws, anthony.perard@citrix.com, pbonzini@redhat.com List-Id: xen-devel@lists.xenproject.org On Fri, Jun 27, 2014 at 05:22:18PM +0800, Chen, Tiejun wrote: > On 2014/6/25 15:13, Michael S. Tsirkin wrote: > >On Wed, Jun 25, 2014 at 10:17:21AM +0800, Tiejun Chen wrote: > > [snip] > > >>diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h > >>index 507165c..25147cf 100644 > >>--- a/hw/xen/xen_pt.h > >>+++ b/hw/xen/xen_pt.h > >>@@ -63,7 +63,7 @@ typedef int (*xen_pt_conf_byte_read) > >> #define XEN_PT_BAR_UNMAPPED (-1) > >> > >> #define PCI_CAP_MAX 48 > >>- > >>+#define PCI_INTEL_OPREGION 0xfc > >> > > > >XEN_.... please > > > >PCI_CAP_MAX should be fixed too. > > They are specific to PCI, not XEN. They are? Where in the PCI spec does it say 48? Same for PCI_INTEL_OPREGION. > Why should we add such a prefix? So that people working on core pci do not have to worry about breaking your devices by adding a symbol in the global header. > > > > > > [snip] > > >> > >>+ if (igd_guest_opregion) { > >>+ ret = xc_domain_memory_mapping(xen_xc, xen_domid, > >>+ (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT), > >>+ (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), > > > >don't spread casts all around. > >Should be a last resort. > > Okay. > > > > >>+ 3, > >>+ DPCI_REMOVE_MAPPING); > >>+ if (ret) { > >>+ return ret; > >>+ } > >>+ } > >>+ > >> return 0; > >> } > >> > >>@@ -447,3 +462,52 @@ err_out: > >> XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n"); > >> return -1; > >> } > >>+ > >>+uint32_t igd_read_opregion(XenPCIPassthroughState *s) > >>+{ > >>+ uint32_t val = 0; > >>+ > >>+ if (igd_guest_opregion == 0) { > > > >!igd_guest_opregion is shorter and does the same, > > Okay. > > > > >>+ return val; > >>+ } > >>+ > >>+ val = igd_guest_opregion; > >>+ > >>+ XEN_PT_LOG(&s->dev, "Read opregion val=%x\n", val); > >>+ return val; > >>+} > >>+ > >>+void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val) > >>+{ > >>+ int ret; > >>+ > >>+ if (igd_guest_opregion) { > >>+ XEN_PT_LOG(&s->dev, "opregion register already been set, ignoring %x\n", > >>+ val); > >>+ return; > >>+ } > >>+ > >>+ xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION, > >>+ (uint8_t *)&igd_host_opregion, 4); > >>+ igd_guest_opregion = (unsigned long)(val & ~0xfff) > >>+ | (igd_host_opregion & 0xfff); > >>+ > > > >Clearly broken on BE. > > I still can't understand why we need to address this in BE case. So code is clean and reusable. Copy and paste is a fact of life, you don't want people to inherit bugs. If some code absolutely must be LE specific, it needs a comment that explains this and cautions people against trying to use it elsewhere in QEMU. > >Maybe not important here but writing clean code is > >just as easy. > >uint8_t igd_host_opregion[4]; > > > >... > > > > xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION, > > igd_host_opregion, sizeof igd_host_opregion); > > > > igd_guest_opregion = (val & ~0xfff) | > > (pci_get_word(igd_host_opregion) & 0xfff); > > > >0xfff should be a macro too to avoid duplication. > > > > Okay. > > Thanks > Tiejun