[Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[]

[Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[]

[Markus Armbruster]

1. Before commit 94649d4 "spapr: Don't use QOM [*] syntax for DR
   connectors", the indexes were small integers:

       (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)
             [...]

   Since then, they're big ones:

             /dr-connector[1073741824] (spapr-dr-connector)
             /dr-connector[1073741825] (spapr-dr-connector)
             /dr-connector[1073741826] (spapr-dr-connector)

   The commit message doesn't quite spell out this change, and I'm
   therefore double-checkint it's intentional.  Is it?

2. Before commit 6c2f9a1 "qapi: Make output visitor return qnull()
   instead of NULL", qom-get returned {}:

   Since then, it returns null:

       QMP> { "execute": "qom-get", "arguments": { "path": "/machine/unattached/device[5]/dr-connector[1073741950]", "property": "fdt" } }
       {"return": null}

   Does anyone care?

Re: [Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[]

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2359 bytes --]

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:
> 
>        (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)
>              [...]
> 
>    Since then, they're big ones:
> 
>              /dr-connector[1073741824] (spapr-dr-connector)
>              /dr-connector[1073741825] (spapr-dr-connector)
>              /dr-connector[1073741826] (spapr-dr-connector)
> 
>    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).

> 2. Before commit 6c2f9a1 "qapi: Make output visitor return qnull()
>    instead of NULL", qom-get returned {}:
> 
>    Since then, it returns null:
> 
>        QMP> { "execute": "qom-get", "arguments": { "path": "/machine/unattached/device[5]/dr-connector[1073741950]", "property": "fdt" } }
>        {"return": null}
> 
>    Does anyone care?

Hm, I'm guessing this is a case where fdt is NULL internally.  Which I
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.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[]

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> 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:
>> 
>>        (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)
>>              [...]
>> 
>>    Since then, they're big ones:
>> 
>>              /dr-connector[1073741824] (spapr-dr-connector)
>>              /dr-connector[1073741825] (spapr-dr-connector)
>>              /dr-connector[1073741826] (spapr-dr-connector)
>> 
>>    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).

Good.

>> 2. Before commit 6c2f9a1 "qapi: Make output visitor return qnull()
>>    instead of NULL", qom-get returned {}:
>> 
>>    Since then, it returns null:
>> 
>>        QMP> { "execute": "qom-get", "arguments": { "path": "/machine/unattached/device[5]/dr-connector[1073741950]", "property": "fdt" } }
>>        {"return": null}
>> 
>>    Does anyone care?
>
> Hm, I'm guessing this is a case where fdt is NULL internally.  Which I

Yes.

> 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.

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.

Thanks!

Re: [Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[]

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3130 bytes --]

On Fri, Dec 04, 2015 at 09:05:51AM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > 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:
> >> 
> >>        (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)
> >>              [...]
> >> 
> >>    Since then, they're big ones:
> >> 
> >>              /dr-connector[1073741824] (spapr-dr-connector)
> >>              /dr-connector[1073741825] (spapr-dr-connector)
> >>              /dr-connector[1073741826] (spapr-dr-connector)
> >> 
> >>    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).
> 
> Good.
> 
> >> 2. Before commit 6c2f9a1 "qapi: Make output visitor return qnull()
> >>    instead of NULL", qom-get returned {}:
> >> 
> >>    Since then, it returns null:
> >> 
> >>        QMP> { "execute": "qom-get", "arguments": { "path": "/machine/unattached/device[5]/dr-connector[1073741950]", "property": "fdt" } }
> >>        {"return": null}
> >> 
> >>    Does anyone care?
> >
> > Hm, I'm guessing this is a case where fdt is NULL internally.  Which I
> 
> Yes.
> 
> > 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.
> 
> 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.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[]

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Fri, Dec 04, 2015 at 09:05:51AM +0100, Markus Armbruster wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > 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:
>> >> 
>> >>        (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)
>> >>              [...]
>> >> 
>> >>    Since then, they're big ones:
>> >> 
>> >>              /dr-connector[1073741824] (spapr-dr-connector)
>> >>              /dr-connector[1073741825] (spapr-dr-connector)
>> >>              /dr-connector[1073741826] (spapr-dr-connector)
>> >> 
>> >>    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).
>> 
>> Good.
>> 
>> >> 2. Before commit 6c2f9a1 "qapi: Make output visitor return qnull()
>> >>    instead of NULL", qom-get returned {}:
>> >> 
>> >>    Since then, it returns null:
>> >> 
>> >>        QMP> { "execute": "qom-get", "arguments": { "path": "/machine/unattached/device[5]/dr-connector[1073741950]", "property": "fdt" } }
>> >>        {"return": null}
>> >> 
>> >>    Does anyone care?
>> >
>> > Hm, I'm guessing this is a case where fdt is NULL internally.  Which I
>> 
>> Yes.
>> 
>> > 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.
>> 
>> 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.

I'm not against changing it, I just want it changed intentionally rather
than by accidental side effect :)

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.

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.

Re: [Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[]

[Peter Maydell]

On 4 December 2015 at 10:11, Markus Armbruster <armbru@redhat.com> wrote:
> 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 {}".

That commit is now in master, so if you don't want it you need to
actively send a revert for it.

thanks
-- PMM

Re: [Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[]

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4263 bytes --]

On Fri, Dec 04, 2015 at 11:11:31AM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Fri, Dec 04, 2015 at 09:05:51AM +0100, Markus Armbruster wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> 
> >> > 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:
> >> >> 
> >> >>        (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)
> >> >>              [...]
> >> >> 
> >> >>    Since then, they're big ones:
> >> >> 
> >> >>              /dr-connector[1073741824] (spapr-dr-connector)
> >> >>              /dr-connector[1073741825] (spapr-dr-connector)
> >> >>              /dr-connector[1073741826] (spapr-dr-connector)
> >> >> 
> >> >>    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).
> >> 
> >> Good.
> >> 
> >> >> 2. Before commit 6c2f9a1 "qapi: Make output visitor return qnull()
> >> >>    instead of NULL", qom-get returned {}:
> >> >> 
> >> >>    Since then, it returns null:
> >> >> 
> >> >>        QMP> { "execute": "qom-get", "arguments": { "path": "/machine/unattached/device[5]/dr-connector[1073741950]", "property": "fdt" } }
> >> >>        {"return": null}
> >> >> 
> >> >>    Does anyone care?
> >> >
> >> > Hm, I'm guessing this is a case where fdt is NULL internally.  Which I
> >> 
> >> Yes.
> >> 
> >> > 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.
> >> 
> >> 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.
> 
> I'm not against changing it, I just want it changed intentionally rather
> than by accidental side effect :)
> 
> 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.


-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[]

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

On 12/07/2015 05:18 PM, David Gibson wrote:
> On Fri, Dec 04, 2015 at 11:11:31AM +0100, Markus Armbruster wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>>

>>>> 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.
>>
>> I'm not against changing it, I just want it changed intentionally rather
>> than by accidental side effect :)
>>
>> 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.

Okay, I'm about to post the patch for just that, for the 2.6 timeframe.

> But leaving it as is for 2.5 is fine.

Good.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]