From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59501) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVZIR-0006We-SX for qemu-devel@nongnu.org; Wed, 11 Mar 2015 01:35:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVZIL-0006s8-Pw for qemu-devel@nongnu.org; Wed, 11 Mar 2015 01:35:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37368) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVZIL-0006qd-Ii for qemu-devel@nongnu.org; Wed, 11 Mar 2015 01:35:01 -0400 Date: Wed, 11 Mar 2015 16:35:41 +1100 From: David Gibson Message-ID: <20150311163541.6ec72541@voom.fritz.box> In-Reply-To: <20150304191231.GB7092@redhat.com> References: <1425399495-13664-1-git-send-email-imammedo@redhat.com> <1425399495-13664-3-git-send-email-imammedo@redhat.com> <20150303173539.GA21824@redhat.com> <20150303213351.43e8333e@igors-macbook-pro.local> <20150304121148.GA27667@redhat.com> <20150304141232.0ab69867@nial.brq.redhat.com> <20150304134900.GA29478@redhat.com> <20150304161444.46407c29@nial.brq.redhat.com> <20150304153139.GA4020@redhat.com> <20150304173342.681ca7f8@nial.brq.redhat.com> <20150304191231.GB7092@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/8rt/No8SgNFly_H=vkpHOAO"; protocol="application/pgp-signature" Subject: Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: ghammer@redhat.com, Igor Mammedov , qemu-devel@nongnu.org, afaerber@suse.de --Sig_/8rt/No8SgNFly_H=vkpHOAO Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Wed, 4 Mar 2015 20:12:31 +0100 "Michael S. Tsirkin" wrote: > On Wed, Mar 04, 2015 at 05:33:42PM +0100, Igor Mammedov wrote: > > On Wed, 4 Mar 2015 16:31:39 +0100 > > "Michael S. Tsirkin" wrote: > >=20 > > > On Wed, Mar 04, 2015 at 04:14:44PM +0100, Igor Mammedov wrote: > > > > On Wed, 4 Mar 2015 14:49:00 +0100 > > > > "Michael S. Tsirkin" wrote: > > > >=20 > > > > > On Wed, Mar 04, 2015 at 02:12:32PM +0100, Igor Mammedov wrote: > > > > > > On Wed, 4 Mar 2015 13:11:48 +0100 > > > > > > "Michael S. Tsirkin" wrote: > > > > > >=20 > > > > > > > On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote: > > > > > > > > On Tue, 3 Mar 2015 18:35:39 +0100 > > > > > > > > "Michael S. Tsirkin" wrote: > > > > > > > >=20 > > > > > > > > > On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov w= rote: > > > > > > > > > > Based on Microsoft's sepecifications (paper can be dowl= oaded from > > > > > > > > > > http://go.microsoft.com/fwlink/?LinkId=3D260709), add a= device > > > > > > > > > > description to the SSDT ACPI table and its implementati= on. > > > > > > > > > >=20 > > > > > > > > > > The GUID is set using "vmgenid.uuid" property. > > > > > > > > > >=20 > > > > > > > > > > Example of using vmgenid device: > > > > > > > > > > -device vmgenid,id=3DFOO,uuid=3D"324e6eaf-d1d1-4bf6-bf= 41-b9bb6c91fb87" > > [...] > >=20 > > > > >=20 > > > > > > >=20 > > > > > > >=20 > > > > > > > BTW, why do we need to stick vmgen_buf_paddr in the info? > > > > > > Because according to MS spec device should have ADDR object > > > > > > with physical buffer address packed in Package(2). So that > > > > > > Windows could read value from there. > > > > > >=20 > > > > > > [...] > > > > >=20 > > > > > Yes but why not read the property when and where we > > > > > need it? > > > > It's basically to fit the style used in acpi-build.c > > > > where we collect info by reading properties in > > > > acpi_get_pm_info(), acpi_get_misc_info(), acpi_get_pci_info() ... > > > > and then just use pm, misc, pci in build_ssdt() > > > > should we drop all above and just inline it in build_ssdt() ? > > >=20 > > > The issue is you have two items to track here: > > > - addr - you stick that in the info struct > > > - full object address - you don't > > > an inconsistency that I dislike. > > What is "full object address"? >=20 > where you look up the vmgen id pci device. >=20 > >=20 > > > > > > > > > > + name =3D g_strdup_printf("PCI0%s.S%.02X_", name ? = name : "", > > > > > > > > > > pdev->devfn); > > > > > > > > > > + g_free(last); > > > > > > > > > > + return name; > > > > > > > > > > +} > > > > > > > > >=20 > > > > > > > > > Looks tricky, and duplicates logic for device naming. > > > > > > > > > All this won't be necessary if you just add this as child > > > > > > > > > of the correct device, without playing with scope. > > > > > > > > > Why not do it? > > > > > > > > since vmgenid PCI device is located somewhere on PCI bus we= don't have > > > > > > > > fixed PATH to it and we need full path to it to send Notivy= from > > > > > > > > "\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_pa= th)" below. > > > > > > >=20 > > > > > > > I see. Still - can't this function return the full aml_name? > > > > > > it's possible but I'd prefer to return back to 2 ACPI devices a= s it was > > > > > > in v13 since Windows sees 2 devices anyway, even if they merged= into one > > > > > > PCI device description (which probably wrong but windows handle= s it because > > > > > > PCI Standard RAM controller is driver less) and get rid of > > > > > > acpi_get_pci_dev_scope_name() thing. > > > > >=20 > > > > > OK but I think it should be under PCI0 at least, > > > > > since that one claims the relevant resource in its CRS. > > > > vmgenid device doesn't claim any resource if we use PCI for its > > > > implementation since corresponding PCI device claims its BAR. > > > > But I don't see any problem in putting VGID device into PCI0 scope. > > > >=20 > > > > >=20 > > > > > > It will also help if vmgenid will be a part of multifunction de= vice, > > > > > > which current build_append_pci_bus_devices() ignores for now (i= .e. it > > > > > > describes only function 0 devices on slot). > > > > > >=20 > > > > > > [...] > > > > >=20 > > > > > OK, though we might need to add the description for the pci devic= e anyway > > > > > e.g. in order to mark it hidden. > > > > Experiments show that Windows ignores _STA for PCI devices, > > > > it looks like it completely ignores SXX devices in ACPI for present= at boot > > > > devices except of _EJ(). > > > > BTW: I've already tried it, it doesn't hide anything. > > > > =20 > > > > [...] > > >=20 > > > So it boils down to the fact that windows thinks it's RAM, > > It thinks it's PCI Standard RAM Controller not RAM itself. > >=20 > > > so it binds a generic driver to it, but then we get > > According to device manager no driver is bound to it and neither needed. > >=20 > > > lucky and it does not try to use it as RAM. > > Video cards also use a bunch of "PCI Standard RAM Controller" > > devices I guess to expose additional VRAM, > > That doesn't mean that BARs are to be used by OS as conventional RAM > > it's rather for usage by vendor's driver. > > Same goes for ivshmem which is also expose RAM bar and has the same CLA= SS ID, > > BAR's RAM is used only by means of ivshmem driver. > >=20 > > But yes we get lucky that Windows has stub device description. >=20 > OK. So if you are going to rely on this, > I think it's a good idea to get ack from David to confirm > this is solvable for pseries. I've looked into this a bit more. We've confirmed it's definitely a bug in SLOF - but fixing it is a bit more subtle than I thought. Basically, SLOF is setting the device_type property for all PCI devices based on the PCI class code - it's device_type =3D "memory" that causes the kernel to erroneously pick up the PCI device as regular RAM. In fact, device_type is supposed to indicate the capabilities of the OF driver attached to the device, so it should only be set by an actual OF driver binding to the device, not generically in the PCI code. The catch is whether we'll break any existing SLOF supported devices is we remove setting of the device_type. This will need some testing. --=20 David Gibson Senior Software Engineer, Virtualization, Red Hat --Sig_/8rt/No8SgNFly_H=vkpHOAO Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJU/9QtAAoJEGw4ysog2bOSPsoP/jaNAPPblK3hpY2g54MC8XMh VwmGzOoZKY+N7gWkbAv6JpnNdflGoIehAT45FtnTvDXrc7yBhQnIl39IbJPkR9hB 7Dneb9bQUi7BM2mRdB+Ec0yrkeO5eJnL+Of7MLwPALi+67QNiGi5O7XWfLYfU1BQ U6FI7PSjzWEZwZwmEHD2Huws8tAySWssHqlyloqghKi4HWtpbkXyPH5IhOUT9uam Ve/YUgIhmXUekQNVEOH0cJMQRNng6qfIPcmc14fA3BWJEhEyjdxgvTTCYwt4xl5Y JMK1tG2Cnz0Ovl7kez8htmTx2p2UnYgs1emRGpejAoBzuvblp7+Dx3oG2wNdgBeh nc5ifqEM7Am9pF4wPoKRFijzupgadhewm6aYLPaEwxkvbwqWyB/9sGGg8GSk/e4I jKnMITD7SlraO8pqz5G7lml3knZ3wciZgOLilOYeOgVp8yhTU1XasMFYJdh0BMbf +xEFoSqgkR2AA0cJiNSfUHz9mGStaZNHO5eyahGPYPVcYLvh2PD0Lz0GwNt8GWjK NyJGyp2cFyyz6vzLvWw1avN9hEpCssTlhEgbADfZ+6PP4I9F8AxFTO5Q2nrdHVVU SJlr/M9WlynCSVqEnQOvdaTMMHResRnD3W8+y7nwTY6RQRKDle4iIZq9lBIzBNWJ rb6KFkEDD15LWhzU/TeP =U0cb -----END PGP SIGNATURE----- --Sig_/8rt/No8SgNFly_H=vkpHOAO--