From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55847) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0n7P-0005NO-QN for qemu-devel@nongnu.org; Fri, 05 Jun 2015 04:36:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z0n7L-0002B9-8u for qemu-devel@nongnu.org; Fri, 05 Jun 2015 04:36:47 -0400 Received: from mga02.intel.com ([134.134.136.20]:18488) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0n7K-0002As-Sk for qemu-devel@nongnu.org; Fri, 05 Jun 2015 04:36:43 -0400 Message-ID: <55715F97.8060801@intel.com> Date: Fri, 05 Jun 2015 16:36:39 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1426669601-13891-1-git-send-email-tiejun.chen@intel.com> <1426669601-13891-4-git-send-email-tiejun.chen@intel.com> <1426674083.32192.44.camel@nilsson.home.kraxel.org> <550A1FE7.2050602@intel.com> <20150531181100.GD5268@redhat.com> <556CFDF2.80103@intel.com> <20150602111611-mutt-send-email-mst@redhat.com> In-Reply-To: <20150602111611-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v7][PATCH 03/10] piix: create host bridge to passthrough List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, allen.m.kay@intel.com, Gerd Hoffmann , rth@twiddle.net On 2015/6/2 17:17, Michael S. Tsirkin wrote: > On Tue, Jun 02, 2015 at 08:50:58AM +0800, Chen, Tiejun wrote: >> On 2015/6/1 2:11, Michael S. Tsirkin wrote: >>> On Thu, Mar 19, 2015 at 09:01:27AM +0800, Chen, Tiejun wrote: >>>> On 2015/3/18 18:21, Gerd Hoffmann wrote: >>>>> On Mi, 2015-03-18 at 17:06 +0800, Tiejun Chen wrote: >>>>>> Implement a pci host bridge specific to passthrough. Actually >>>>>> this just inherits the standard one. And we also just expose >>>>>> a minimal real host bridge pci configuration subset. >>>>> >>>>>> +/* Here we just expose minimal host bridge offset subset. */ >>>>>> +static const IGDHostInfo igd_host_bridge_infos[] = { >>>>>> + {0x08, 2}, /* revision id */ >>>>>> + {0x2c, 2}, /* sybsystem vendor id */ >>>>>> + {0x2e, 2}, /* sybsystem id */ >>>>>> + {0x50, 2}, /* SNB: processor graphics control register */ >>>>>> + {0x52, 2}, /* processor graphics control register */ >>>>>> + {0xa4, 4}, /* SNB: graphics base of stolen memory */ >>>>>> + {0xa8, 4}, /* SNB: base of GTT stolen memory */ >>>>>> +}; >>>>> >>>>> Hmm, no vendor/device id here? Will the idg guest drivers happily read >>>> >>>> These two emulated values are already fine. >>>> >>>> And this is a long story. At the beginning, we were initially trying to >>>> expose more, >>>> >>>> + case 0x00: /* vendor id */ >>>> + case 0x02: /* device id */ >>>> + case 0x08: /* revision id */ >>>> + case 0x2c: /* sybsystem vendor id */ >>>> + case 0x2e: /* sybsystem id */ >>>> + case 0x50: /* SNB: processor graphics control register */ >>>> + case 0x52: /* processor graphics control register */ >>>> + case 0xa0: /* top of memory */ >>>> + case 0xb0: /* ILK: BSM: should read from dev 2 offset 0x5c */ >>>> + case 0x58: /* SNB: PAVPC Offset */ >>>> + case 0xa4: /* SNB: graphics base of stolen memory */ >>>> + case 0xa8: /* SNB: base of GTT stolen memory */ >>>> >>>> But this brought some discussions with Paolo and Michael, and then plus our >>>> further experiment, now as everyone expect, this is a minimal real host >>>> bridge pci configuration subset which we need to expose. >>>> >>>>> graphics control registers from i440fx even though this chipset never >>>>> existed in a igd variant? >>>>> >>>>> [ just wondering, if it works that way fine, it certainly makes things >>>> >>>> Yes, it works fine. >>>> >>>>> easier for the firmware which uses host bridge pci id to figure >>>>> whenever it is running @ i440fx or q35 ]. >>>>> >>>>>> +static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev) >>>>>> +{ >>>>>> + uint32_t val = 0; >>>>>> + int rc, i, num; >>>>>> + int pos, len; >>>>>> + >>>>>> + 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; >>>>>> + } >>>>>> + pci_default_write_config(pci_dev, pos, val, len); >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>> >>>>> Nothing i440fx specific in here, just copying some host bridge pci >>>>> config space bits. I guess we can sub-class the q35 host bridge the >>>>> same way and even reuse the init function? >>>> >>>> This is our another discussion long time ago :) >>>> >>>> Currently Xen don't run with q35. If I remember those discussions correctly, >>>> something is still restricted to Windows. >>>> >>>>> >>>>>> +#define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "xen-igd-passthrough-i440FX" >>>>> >>>>> One xen leftover slipped through here. >>>> >>>> Fixed. >>> >>> So should I expect v8 then? >>> >> >> For this revision we just had only this valuable comment, and that is just >> about to rename a macro, so I think its not worth resend this, right? >> >> If I'm wrong let me know :) >> >> Thanks >> Tiejun > > I didn't follow closely so I have no idea how valuable was the last > round of feedback, but when you say "Fixed" don't you mean "will be It should be, but in this revision, I just received this sole comment until now so I mean its not worth resending a new review just with this fix :) Anyway, its not big deal, just let me send this out as you expect. Thanks Tiejun > fixed in the next revision of the patchset"? >