From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35288) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a65yg-0004si-0o for qemu-devel@nongnu.org; Mon, 07 Dec 2015 19:17:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a65yb-0007hJ-PX for qemu-devel@nongnu.org; Mon, 07 Dec 2015 19:17:57 -0500 Date: Tue, 8 Dec 2015 11:18:02 +1100 From: David Gibson Message-ID: <20151208001802.GF20139@voom.fritz.box> References: <87bna7v9bc.fsf@blackfin.pond.sub.org> <20151204000157.GF9559@voom.redhat.com> <87mvtqy6xs.fsf@blackfin.pond.sub.org> <20151204093554.GJ9559@voom.redhat.com> <87io4ettf0.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="eDB11BtaWSyaBkpc" Content-Disposition: inline In-Reply-To: <87io4ettf0.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[] List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, agraf@suse.de --eDB11BtaWSyaBkpc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Dec 04, 2015 at 11:11:31AM +0100, Markus Armbruster wrote: > David Gibson writes: >=20 > > On Fri, Dec 04, 2015 at 09:05:51AM +0100, Markus Armbruster wrote: > >> David Gibson writes: > >>=20 > >> > On Thu, Dec 03, 2015 at 04:30:31PM +0100, Markus Armbruster wrote: > >> >> 1. Before commit 94649d4 "spapr: Don't use QOM [*] syntax for DR > >> >> connectors", the indexes were small integers: > >> >>=20 > >> >> (qemu) info qom-tree > >> >> /machine (pseries-2.4-machine) > >> >> /unattached (container) > >> >> [...] > >> >> /device[5] (spapr-pci-host-bridge) > >> >> /pci@800000020000000.mmio[0] (qemu:memory-region) > >> >> /pci@800000020000000.mmio-alias[0] (qemu:memory-region) > >> >> /pci@800000020000000.io[0] (qemu:memory-region) > >> >> /pci@800000020000000.io-alias[0] (qemu:memory-region) > >> >> /pci.0 (PCI) > >> >> /pci@800000020000000.iommu-root[0] (qemu:memory-region) > >> >> /dr-connector[0] (spapr-dr-connector) > >> >> /dr-connector[1] (spapr-dr-connector) > >> >> /dr-connector[2] (spapr-dr-connector) > >> >> [...] > >> >>=20 > >> >> Since then, they're big ones: > >> >>=20 > >> >> /dr-connector[1073741824] (spapr-dr-connector) > >> >> /dr-connector[1073741825] (spapr-dr-connector) > >> >> /dr-connector[1073741826] (spapr-dr-connector) > >> >>=20 > >> >> The commit message doesn't quite spell out this change, and I'm > >> >> therefore double-checkint it's intentional. Is it? > >> > > >> > Yes, it's intentional. The small integers were arbitrarily allocated > >> > by the QOM magic [*] code, whereas the big integers are actually > >> > meaningful values (essentially the DRC's global ID for the dynamic > >> > reconfiguration hypervisor interfaces). > >>=20 > >> Good. > >>=20 > >> >> 2. Before commit 6c2f9a1 "qapi: Make output visitor return qnull() > >> >> instead of NULL", qom-get returned {}: > >> >>=20 > >> >> Since then, it returns null: > >> >>=20 > >> >> QMP> { "execute": "qom-get", "arguments": { "path": "/machin= e/unattached/device[5]/dr-connector[1073741950]", "property": "fdt" } } > >> >> {"return": null} > >> >>=20 > >> >> Does anyone care? > >> > > >> > Hm, I'm guessing this is a case where fdt is NULL internally. Which= I > >>=20 > >> Yes. > >>=20 > >> > think will happen before a device gets hotplugged into the DRC. In > >> > that case null seems more correct to me than {}, since {} would also > >> > be what's shown for a present-but-empty device tree. > >>=20 > >> It was {} in 2.4. Changing it to null so we can distingish "nothing" > >> from "empty" is an incompatible change. May make sense anyway, but I > >> can't judge it. > > > > Strictly speaking it's an incompatible change, yes. But I find it > > hard to imagine anything would be relying on the {} behaviour. This > > property is essentially a debugging interface to start with, and the > > missing / empty case is examining it in a state that's unlikely to be > > interesting. >=20 > I'm not against changing it, I just want it changed intentionally rather > than by accidental side effect :) >=20 > If you tell me you want null here going forward, I'll make sure it gets > changed to null in the next development cycle, with a nice commit > message. I would like it to be null (or simply missing) in future. > If you want it to be null in 2.5, NAK "[PATCH for-2.5 2/3] spapr_drc: > Change value of property "fdt" from null back to {}". It'll remain an > implicit change then, not documented in commit messages. I expect we'll > eventually get a patch similar to the NAKed one regardless, because > we'll tighten up the visitor contracts, and returning without visiting > anything should become a programming error then. But leaving it as is for 2.5 is fine. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --eDB11BtaWSyaBkpc Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWZiG6AAoJEGw4ysog2bOSQH4QAKm46c9WNhO2k+Ym6lHqHo+M daom94fJmFYdAQB6/XLvYL+xhL87EN9z4H+OQOgEkqw9s+uuwLMCNHAUAvsfZpmI NtmYyw0gGJ1UT3nBYUex6l+xG5UB++hKRYzRFfzX5NV0riN63hPYt0jsZIUYIXw4 2Vn55/EbfNLBE2D1sAqRaFWHxLhKQL89kD0WBWvfe6jV20MkQ9znUUwkJIvFe9Nw e+PLdVhNOl4+MJB2MY9w0qtNp3ZZMFl6ABZ84OOkyxMSRZOfeHvsWVmuvAbWGo5w 0GnsVN65BVD/KOLJRkXMrYBNSD1sRUNo59oB+jPxPsy69aznaLduzuSxe0GHJJGu pT6fNb+MiJDnOfBfHuPe+3og3RmcNlO97/tWJujVBu7CnJBKSO7I42zhNY/olXd4 NwhtCmtsAYTw1nn0GJogopqKcMkoU+hnSF6xKTw3rwvjnVH/lWoQ1dZIrHB0D7iN 9iEvyUY5cS8B57dueMq9swTEMbjYBBEKDCo8i7DENMb/7NiVuUwFoACG2BaybcG5 L+ftMNnqNx1G6t5WnKioFhhkfP5kxAWXVmEi1S2/LisyNeXWw6KoHY3LM1EtBFQ/ vonArp+WjWe/PTQcJPKlV0PGDvN80hqrd7z1ooBgH1isyinA8Ly2P2wlM6Bpog4/ v84nG6Ms1UE6Pc9IakGL =vFTG -----END PGP SIGNATURE----- --eDB11BtaWSyaBkpc--