From patchwork Thu Jul 13 11:54:01 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Halil Pasic X-Patchwork-Id: 9838349 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 9E584602D8 for ; Thu, 13 Jul 2017 11:54:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8FED0286CA for ; Thu, 13 Jul 2017 11:54:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 845D3286D9; Thu, 13 Jul 2017 11:54:51 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 516D9286CA for ; Thu, 13 Jul 2017 11:54:50 +0000 (UTC) Received: from localhost ([::1]:59026 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dVchl-0003X3-K3 for patchwork-qemu-devel@patchwork.kernel.org; Thu, 13 Jul 2017 07:54:49 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53236) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dVchD-0003Vg-TK for qemu-devel@nongnu.org; Thu, 13 Jul 2017 07:54:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dVch9-000848-V5 for qemu-devel@nongnu.org; Thu, 13 Jul 2017 07:54:15 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:41584) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dVch9-00083f-Ka for qemu-devel@nongnu.org; Thu, 13 Jul 2017 07:54:11 -0400 Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v6DBs7xp069367 for ; Thu, 13 Jul 2017 07:54:09 -0400 Received: from e06smtp10.uk.ibm.com (e06smtp10.uk.ibm.com [195.75.94.106]) by mx0a-001b2d01.pphosted.com with ESMTP id 2bnt3rcyyw-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 13 Jul 2017 07:54:08 -0400 Received: from localhost by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 13 Jul 2017 12:54:06 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196) by e06smtp10.uk.ibm.com (192.168.101.140) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Thu, 13 Jul 2017 12:54:02 +0100 Received: from d06av24.portsmouth.uk.ibm.com (d06av24.portsmouth.uk.ibm.com [9.149.105.60]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v6DBs2lf38666260; Thu, 13 Jul 2017 11:54:02 GMT Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 18AAB42047; Thu, 13 Jul 2017 12:51:25 +0100 (BST) Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 8FD9C42045; Thu, 13 Jul 2017 12:51:24 +0100 (BST) Received: from oc3836556865.ibm.com (unknown [9.152.224.141]) by d06av24.portsmouth.uk.ibm.com (Postfix) with ESMTP; Thu, 13 Jul 2017 12:51:24 +0100 (BST) To: Eduardo Habkost References: <20170711004303.3902-1-ehabkost@redhat.com> <20170711004303.3902-2-ehabkost@redhat.com> <91940c7e-3685-26e0-e8f9-e050afcd28b5@linux.vnet.ibm.com> <20170712182929.GI6020@localhost.localdomain> From: Halil Pasic Date: Thu, 13 Jul 2017 13:54:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <20170712182929.GI6020@localhost.localdomain> Content-Language: en-US X-TM-AS-GCONF: 00 x-cbid: 17071311-0040-0000-0000-000003C398FB X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17071311-0041-0000-0000-000025BEDD7D Message-Id: <640df239-d674-747d-d6d1-692030908c43@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-07-13_06:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000 definitions=main-1707130186 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] [fuzzy] X-Received-From: 148.163.156.1 Subject: Re: [Qemu-devel] [PATCH 1/3] qdev: fix the order compat and global properties are applied X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Marcel Apfelbaum , Cornelia Huck , qemu-devel@nongnu.org, Greg Kurz Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 >>> >>> The current code recursively applies global properties from child up to >>> parent types. This can cause properties passed with the -global option to >>> be silently overridden by internal compat properties. >>> >>> This is exactly what happens with virtio-*-pci drivers since commit: >>> >>> "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour" >>> >>> Passing -device virtio-blk-pci.disable-modern=off has no effect on 2.6 >>> machine types because the internal virtio-pci.disable-modern=on compat >>> property always prevail. >>> >>> This patch fixes the issue by reversing the logic: we now go through the >>> global property list and, for each property, we check if it is applicable >>> to the device. >>> >>> This result in compat properties being applied first, in the order they >>> appear in the HW_COMPAT_* macros, followed by global properties, in they >>> order appear on the command line. >>> >>> Signed-off-by: Greg Kurz >>> Message-Id: <148103887228.22326.478406873609299999.stgit@bahia.lab.toulouse-stg.fr.ibm.com> >>> Signed-off-by: Eduardo Habkost >>> --- >>> hw/core/qdev-properties.c | 15 ++------------- >>> 1 file changed, 2 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >>> index f11d578..41cca9d 100644 >>> --- a/hw/core/qdev-properties.c >>> +++ b/hw/core/qdev-properties.c >>> @@ -1148,8 +1148,7 @@ int qdev_prop_check_globals(void) >>> return ret; >>> } >>> >>> -static void qdev_prop_set_globals_for_type(DeviceState *dev, >>> - const char *typename) >>> +void qdev_prop_set_globals(DeviceState *dev) >>> { >>> GList *l; >>> >>> @@ -1157,7 +1156,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev, >>> GlobalProperty *prop = l->data; >>> Error *err = NULL; >>> >>> - if (strcmp(typename, prop->driver) != 0) { >>> + if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) { >>> continue; >>> } >>> prop->used = true; >>> @@ -1175,16 +1174,6 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev, >>> } >>> } >>> >>> -void qdev_prop_set_globals(DeviceState *dev) >>> -{ >>> - ObjectClass *class = object_get_class(OBJECT(dev)); >>> - >>> - do { >>> - qdev_prop_set_globals_for_type(dev, object_class_get_name(class)); >>> - class = object_class_get_parent(class); >>> - } while (class); >>> -} >>> - >>> /* --- 64bit unsigned int 'size' type --- */ >>> >>> static void get_size(Object *obj, Visitor *v, const char *name, void *opaque, >>> >> >> 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 >> >> >> 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. > > The main sources of global properties we have are: > * MachineClass::compat_props> * -global command-line option I was thinking about these two. > * -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. 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: 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. > 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. > > 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/qemu-devel@nongnu.org/msg416670.html > https://www.mail-archive.com/qemu-devel@nongnu.org/msg416985.html > 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 */