Re: [Qemu-devel] [PATCH 1/3] qdev: fix the order compat and global properties are applied

[Halil Pasic]

On 07/13/2017 06:15 PM, Eduardo Habkost wrote:
> On Thu, Jul 13, 2017 at 01:54:01PM +0200, Halil Pasic wrote:
>>
>>
>> On 07/12/2017 08:29 PM, Eduardo Habkost wrote:
>>> On Wed, Jul 12, 2017 at 07:33:15PM +0200, Halil Pasic wrote:
>>>>
>>>>
>>>> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
>>>>> From: Greg Kurz <address@hidden>
[..]
>>>> Code looks good to me. Given that high profile people are happy with the
>>>> patch I won't object on the behavior aspect which I don't understand fully.
>>>> Thus:
>>>>
>>>> Reviewed-by: Halil Pasic <address@hidden>
>>>>
>>>>
>>>> Now a couple of questions for keeping  my clear conscience:
>>>> * What did we test? Since this patch is fixing a problem it
>>>> changes behavior. Did we test if there is something that breaks?
>>>>
>>>> * The previous version seems to establish a (somewhat strange)
>>>> precedence for the case the same device property (storage object)
>>>> is set via multiple super-classes (e.g. set both by parent and
>>>> parents parent). This seems to have at least been possible,
>>>> and technically it still is I guess. Now instead of most general
>>>> (super class) wins we have last added property wins. I assume it
>>>> isn't a problem, because we don't have something obscure like that.
>>>> Or am I wrong? This obviously connects to the first question.
>>>> (By the way, most specialized wins would not have been that
>>>> surprising given how inheritance and OO usually works. My assumption
>>>> that nobody used this obscure mechanism is largely based on it's
>>>> strangeness).
>>>
>>> Note that we are not changing the behavior when the classes
>>> themselves set different defaults.  Subclasses are still free to
>>> override defaults set by superclasses inside QEMU code, and they
>>> will be unaffected by this series.  What we are changing here are
>>> the semantics of the -global command-line option when applied to
>>> superclasses.
>>
>> I was not referring to this.
> 
> Good.  :)
> 
>>
>>>
>>> The main sources of global properties we have are:
>>> * MachineClass::compat_props> * -global command-line option
>>
>> I was thinking about these two.
> 
> Good, this is what we're really trying to address, so let's
> review that:
> 
>>
>>> * -cpu command-line option
>>>
>>> The behavior on the compat_props case was addressed by the hack
>>> in commit 0bcba41f "machine: Convert abstract typename on
>>> compat_props to subclass names".  This means compat_props was
>>> already following the order in which properties were registered.
>>
>> I disagree. Commit 0bcba41f affects only *abstract* classes. So
>> your statement is true if a non-abstract class inheriting form
>> device can only have abstract ancestor classes. I'm not aware
>> such rule exists in QEMU, and according to your reply to my comment
>> on patch 2 there are even people using non-abstract superclasses
>> for devices.
> 
> Good catch!  You are correct, and your experiment below is
> correct.  But I thought no non-abstract superclasses where used
> on compat_props on any machine-type (then this patch wouldn't
> have any visible effect in the current tree).
> 
> However, commit 0bcba41f has this note, which I had forgot about:
> 
>     Note that there's one case that won't be fixed by this hack:
>     "-global spapr-pci-vfio-host-bridge.<option>=<value>" won't be
>     able to override compat_props, because spapr-pci-host-bridge is
>     not an abstract class.
> 
> We really have a
> spapr-pci-host-bridge.dynamic-reconfiguration=off entry in
> compat_props for pseries-2.3.
> 
> This means this series will also fix the ordering between
> compat_props and -global if "-global
> spapr-pci-vfio-host-bridge.dynamic-reconfiguration=..." is used
> with machine-type pseries <= 2.3.
> 
> 
>>
>> Since I don't tend to trust myself with constructing proofs
>> for such stuff in my head, I've tried out my hypothesis before
>> making my review.
>>
>> This is the patch I used to verify my hypothesis:
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 41ca666..d524cd0 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -445,6 +445,10 @@ static const TypeInfo ccw_machine_info = {
>>              .property = "max_revision",\
>>              .value    = "0",\
>>          },{\
>> +            .driver   = "virtio-ccw-device",\
>> +            .property = "max_revision",\
>> +            .value    = "1",\
>> +        },{\
>>              .driver   = "virtio-balloon-ccw",\
>>              .property = "max_revision",\
>>              .value    = "0",\
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index e18fd26..6992697 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -1666,7 +1666,7 @@ static const TypeInfo virtio_ccw_device_info = {
>>      .instance_size = sizeof(VirtioCcwDevice),
>>      .class_init = virtio_ccw_device_class_init,
>>      .class_size = sizeof(VirtIOCCWDeviceClass),
>> -    .abstract = true,
>> +    .abstract = false,
>>  };
>>  
>>  /* virtio-ccw-bus */
>>
>> Unfortunately it's virtio-ccw because I'm most familiar with that,
>> and I knew immediately how can I construct the situation I have in
>> mind there.
>>
>> What we observe as the effect of this patch before your patch
>> both my virtio-ccw-blk and virtio-ccw-net were revision 1
>> when running a s390-ccw-virtio-2.4 (more general takes precedence).
>> After your patch series, since  virtio-ccw-net is further down in
>> the list, it ended up being revision 0 (virtio-ccw-blk remained
>> 1 as my change was inserted after the property for virtio-ccw-blk
>> but before the property for virtio-ccw-net (in the array of
>> compat_prpos).
>>
>> Do you agree, or should I re-check my experiment and maybe also
>> look for some example you can run on amd64.
> 
> I think pseries and spapr-pci-vfio-host-bridge is an existing
> example that doesn't require changing the source.
> 
>>
>>> In this case there should be no behavior change, and we have
>>> something to test: check if the original bug[1] (where -global
>>> was unable to override compat_props) is still fixed.
> 
> So, correcting myself: the only behavior change regarding
> compat_props introduced by this patch should be when "-global
> spapr-pci-vfio-host-bridge.dynamic-reconfiguration=on" is used
> with machine-type pseries <= 2.3.  Now -global will correctly
> override the entry in compat_props.
> 
> I didn't confirm if we have other non-abstract superclasses in
> compat_props added to qemu.git after commit 0bcba41f, though.
> 
>

I don't understand the 'added after commit 0bcba41' as this commit
affects only abstract superclasses. But it does not really matter
for me (see below).

To sum it up form my perspective:
1) My r-b still stands for the same reasons I've stated at the
beginning.
2) The changes for sure affect an external interface (command
line) and IMHO also an internal API. Both changes are for me
without a doubt for the better. 
3) For the external one, updating the documentation would in my opinion
really be a good idea. (We discussed this in the other thread).
4) Regarding the internal API change, there are two ways of thinking
about it: in the abstract forgetting about the "internal" from 
"internal API", and by looking at the interaction with all the client
code. My little analysis was restricted to the first, because I wanted
to understand the changes, and wanted to be sure we understand the
changes. I wanted to avoid doing the second part too. I  do think we
just have to make sure no client code is adversely affected by it. Now 
I've double checked, the stuff supported on s390x is not (adversely)
affected. For the non s390x stuff I will just take your word that
nobody is adversely affected.
5) I think a sentence or two in the commit message about why is the
external change OK, and why doesn't the internal API change require
any client code changes would not have hurt.

In general, I think we are in agreement.

Halil
 
>>>
>>> However, the behavior of -global will change if the user
>>> specifies command-line options that contradict each other.  I
>>> don't believe users rely on that behavior, and the old behavior
>>> was a bug and not a feature.  In this case we can test it, but
>>> the behavior change is intentional.
>>
>> I don't think old behavior was sane, that's why I gave my r-b
>> without insisting on the for me open questions.
>>
>> Btw. I would not call that contradicting. But it certainly
>> is not something our users should rely on because (as we concluded
>> in the discussion at patch 2), using -global for a superclass is
>> not documented.
>>
>>>
>>> [1] https://www.mail-archive.com/address@hidden/msg416670.html
>>>     https://www.mail-archive.com/address@hidden/msg416985.html
>>>
>>
>