From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41989) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0SMY-0000bs-1j for qemu-devel@nongnu.org; Fri, 27 Jun 2014 05:22:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X0SMR-0000Hd-SZ for qemu-devel@nongnu.org; Fri, 27 Jun 2014 05:22:29 -0400 Received: from mga09.intel.com ([134.134.136.24]:59109) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0SMR-0000HZ-N8 for qemu-devel@nongnu.org; Fri, 27 Jun 2014 05:22:23 -0400 Message-ID: <53AD37CA.3040800@intel.com> Date: Fri, 27 Jun 2014 17:22:18 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 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> In-Reply-To: <20140625071321.GD25563@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" 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 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. Why should we add such a prefix? > > [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. > 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: "Chen, Tiejun" Subject: Re: [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping Date: Fri, 27 Jun 2014 17:22:18 +0800 Message-ID: <53AD37CA.3040800@intel.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> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140625071321.GD25563@redhat.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: "Michael S. Tsirkin" 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 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. Why should we add such a prefix? > > [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. > 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