From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37906) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1PuT-0008W7-GN for qemu-devel@nongnu.org; Sun, 29 Jun 2014 20:57:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X1PuN-0001Ep-Ax for qemu-devel@nongnu.org; Sun, 29 Jun 2014 20:57:29 -0400 Received: from mga02.intel.com ([134.134.136.20]:53981) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1PuN-0001Ei-4P for qemu-devel@nongnu.org; Sun, 29 Jun 2014 20:57:23 -0400 Message-ID: <53B0B5ED.7020604@intel.com> Date: Mon, 30 Jun 2014 08:57:17 +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> <53AD37CA.3040800@intel.com> <20140629114358.GB26161@redhat.com> In-Reply-To: <20140629114358.GB26161@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/29 19:43, Michael S. Tsirkin wrote: > 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. Okay. > > >>> >>> >> >> [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. Understood. > 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. I think its fine enough to add a comment. Thanks Tiejun > > >>> 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 >