From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:57775) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ggy5I-0002i6-Ky for qemu-devel@nongnu.org; Tue, 08 Jan 2019 15:34:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ggy5G-0001r9-KW for qemu-devel@nongnu.org; Tue, 08 Jan 2019 15:34:48 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:42442) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ggy5G-0001pX-9T for qemu-devel@nongnu.org; Tue, 08 Jan 2019 15:34:46 -0500 Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id x08KNgV4074370 for ; Tue, 8 Jan 2019 15:34:44 -0500 Received: from e16.ny.us.ibm.com (e16.ny.us.ibm.com [129.33.205.206]) by mx0a-001b2d01.pphosted.com with ESMTP id 2pw2kj11fn-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 08 Jan 2019 15:34:43 -0500 Received: from localhost by e16.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 8 Jan 2019 20:34:42 -0000 References: <1545062250-7573-1-git-send-email-akrowiak@linux.ibm.com> <112a03e6-3d6c-1c14-10ab-c9cea09ad388@linux.ibm.com> <20190108173113.3c2d830c.cohuck@redhat.com> <20190108175021.003deab9@oc2783563651> <20190108180615.7e48cf13.cohuck@redhat.com> From: Tony Krowiak Date: Tue, 8 Jan 2019 15:34:37 -0500 MIME-Version: 1.0 In-Reply-To: <20190108180615.7e48cf13.cohuck@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Message-Id: <4876194e-1a87-1491-a8b3-eed5b0a9afe6@linux.ibm.com> Subject: Re: [Qemu-devel] [PATCH v3] qdev/core: fix qbus_is_full() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck , Halil Pasic Cc: imammedo@redhat.com, qemu-devel@nongnu.org, Christian Borntraeger , Peter Maydell On 1/8/19 12:06 PM, Cornelia Huck wrote: > On Tue, 8 Jan 2019 17:50:21 +0100 > Halil Pasic wrote: > >> On Tue, 8 Jan 2019 17:31:13 +0100 >> Cornelia Huck wrote: >> >>> On Tue, 8 Jan 2019 11:08:56 -0500 >>> Tony Krowiak wrote: >>> >>>> On 12/17/18 10:57 AM, Tony Krowiak wrote: >>>>> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index >>>>> value of the BusState structure with the max_dev value of the BusClass structure >>>>> to determine whether the maximum number of children has been reached for the >>>>> bus. The problem is, the max_index field of the BusState structure does not >>>>> necessarily reflect the number of devices that have been plugged into >>>>> the bus. >>>>> >>>>> Whenever a child device is plugged into the bus, the bus's max_index value is >>>>> assigned to the child device and then incremented. If the child is subsequently >>>>> unplugged, the value of the max_index does not change and no longer reflects the >>>>> number of children. >>>>> >>>>> When the bus's max_index value reaches the maximum number of devices >>>>> allowed for the bus (i.e., the max_dev field in the BusClass structure), >>>>> attempts to plug another device will be rejected claiming that the bus is >>>>> full -- even if the bus is actually empty. >>>>> >>>>> To resolve the problem, a new 'num_children' field is being added to the >>>>> BusState structure to keep track of the number of children plugged into the >>>>> bus. It will be incremented when a child is plugged, and decremented when a >>>>> child is unplugged. >>>>> >>>>> Signed-off-by: Tony Krowiak >>>>> Reviewed-by: Pierre Morel >>>>> Reviewed-by: Halil Pasic >>>>> --- >>>>> hw/core/qdev.c | 3 +++ >>>>> include/hw/qdev-core.h | 1 + >>>>> qdev-monitor.c | 2 +- >>>>> 3 files changed, 5 insertions(+), 1 deletion(-) >>>> >>>> Ping ... >>>> I could not determine who the maintainer is for the three files >>>> listed above. I checked the MAINTAINERS file and the prologue of each >>>> individual file. Can someone please tell me who is responsible >>>> for merging these changes? Any additional review comments? >>>> >>>>> >>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>>>> index 6b3cc55b27c2..956923f33520 100644 >>>>> --- a/hw/core/qdev.c >>>>> +++ b/hw/core/qdev.c >>>>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child) >>>>> snprintf(name, sizeof(name), "child[%d]", kid->index); >>>>> QTAILQ_REMOVE(&bus->children, kid, sibling); >>>>> >>>>> + bus->num_children--; >>>>> + >>>>> /* This gives back ownership of kid->child back to us. */ >>>>> object_property_del(OBJECT(bus), name, NULL); >>>>> object_unref(OBJECT(kid->child)); >>>>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child) >>>>> char name[32]; >>>>> BusChild *kid = g_malloc0(sizeof(*kid)); >>>>> >>>>> + bus->num_children++; >>>>> kid->index = bus->max_index++; >>> >>> Hm... I'm wondering what happens for insane numbers of hotplugging >>> operations here? >>> >>> (Preexisting problem for busses without limit, but busses with a limit >>> could now run into that as well.) >>> >> >> How does this patch change things? I mean bus->num_children gets >> decremented on unplug. > > We don't stop anymore if max_index >= max_dev, which means that we can > now trigger that even if max_dev != 0. I guess I am missing your point. If max_dev == 0, then there is nothing stopping an insane number of hot plug operations; either before this patch, or with this patch. With the patch, once the number of children hot plugged reaches max_dev, the qbus_is_full function will return false and no more children can be plugged. If a child device is unplugged, the num_children - which counts the number of children attached to the bus - will be decremented, so it always reflects the number of children added to the bus. Besides, checking max_index against max_dev is erroneous, because max_index is incremented every time a child device is plugged and is never decremented. It really operates as more of a uinique identifier than a counter and is also used to create a unique property name when the child device is linked to the bus as a property (see bus_add_child function in hw/core/qdev.c). > >> >> Regards, >> Halil >> >>>>> kid->child = child; >>>>> object_ref(OBJECT(kid->child)); >>>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >>>>> index a24d0dd566e3..521f0a947ead 100644 >>>>> --- a/include/hw/qdev-core.h >>>>> +++ b/include/hw/qdev-core.h >>>>> @@ -206,6 +206,7 @@ struct BusState { >>>>> HotplugHandler *hotplug_handler; >>>>> int max_index; >>>>> bool realized; >>>>> + int num_children; >>>>> QTAILQ_HEAD(ChildrenHead, BusChild) children; >>>>> QLIST_ENTRY(BusState) sibling; >>>>> }; >>>>> diff --git a/qdev-monitor.c b/qdev-monitor.c >>>>> index 07147c63bf8b..45a8ba49644c 100644 >>>>> --- a/qdev-monitor.c >>>>> +++ b/qdev-monitor.c >>>>> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem) >>>>> static inline bool qbus_is_full(BusState *bus) >>>>> { >>>>> BusClass *bus_class = BUS_GET_CLASS(bus); >>>>> - return bus_class->max_dev && bus->max_index >= bus_class->max_dev; >>>>> + return bus_class->max_dev && bus->num_children >= bus_class->max_dev; >>>>> } >>>>> >>>>> /* >>>>> >>> >>> The approach the patch takes looks sane to me. >>> >> >