From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38158) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gSlBn-0003xr-MV for qemu-devel@nongnu.org; Fri, 30 Nov 2018 10:58:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gSlBk-0002Kf-8r for qemu-devel@nongnu.org; Fri, 30 Nov 2018 10:58:47 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:42006) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gSlBj-0002HZ-RO for qemu-devel@nongnu.org; Fri, 30 Nov 2018 10:58:44 -0500 Received: from pps.filterd (m0098404.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id wAUFqeCI037312 for ; Fri, 30 Nov 2018 10:58:40 -0500 Received: from e13.ny.us.ibm.com (e13.ny.us.ibm.com [129.33.205.203]) by mx0a-001b2d01.pphosted.com with ESMTP id 2p36j3w95e-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 30 Nov 2018 10:58:40 -0500 Received: from localhost by e13.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 30 Nov 2018 15:58:38 -0000 References: <1542904555-1136-1-git-send-email-pmorel@linux.ibm.com> <1542904555-1136-3-git-send-email-pmorel@linux.ibm.com> <07102a0f-d37e-b9a6-047e-370bc3209686@linux.ibm.com> From: Tony Krowiak Date: Fri, 30 Nov 2018 10:58:32 -0500 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Message-Id: <49c28e28-9380-474b-b5d3-5ab61dd0af9e@linux.ibm.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: pmorel@linux.ibm.com, borntraeger@de.ibm.com Cc: cohuck@redhat.com, agraf@suse.de, rth@twiddle.net, david@redhat.com, qemu-s390x@nongnu.org, qemu-devel@nongnu.org, peter.maydell@linaro.org, pbonzini@redhat.com, mst@redhat.com, eric.auger@redhat.com, pasic@linux.ibm.com On 11/30/18 4:31 AM, Pierre Morel wrote: > On 29/11/2018 21:42, Tony Krowiak wrote: >> On 11/22/18 11:35 AM, Pierre Morel wrote: >>> Two good reasons to use the base device as a child of the >>> AP BUS: >>> - We can easily find the device without traversing the qtree. >>> - In case we have different APdevice instantiation, VFIO with >>> =C2=A0=C2=A0 interception or emulation, we will need the APDevice as >>> =C2=A0=C2=A0 a parent device. >>> >>> Signed-off-by: Pierre Morel >>> --- >>> =C2=A0 hw/s390x/ap-device.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 | 22 ++++++++++++++++++++++ >>> =C2=A0 hw/vfio/ap.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 16 ++++++---------- >>> =C2=A0 include/hw/s390x/ap-device.h |=C2=A0 2 ++ >>> =C2=A0 3 files changed, 30 insertions(+), 10 deletions(-) >>> >>> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c >>> index f5ac8db..554d5aa 100644 >>> --- a/hw/s390x/ap-device.c >>> +++ b/hw/s390x/ap-device.c >>> @@ -11,13 +11,35 @@ >>> =C2=A0 #include "qemu/module.h" >>> =C2=A0 #include "qapi/error.h" >>> =C2=A0 #include "hw/qdev.h" >>> +#include "hw/s390x/ap-bridge.h" >>> =C2=A0 #include "hw/s390x/ap-device.h" >>> +APDevice *s390_get_ap(void) >> >> How about ap_get_device(void)? >=20 > Yes, keep same conventions. >=20 >> >>> +{ >>> +=C2=A0=C2=A0=C2=A0 static DeviceState *apb; >> >> Why static if you call s390_get_ap_bridge() >> to retrieve it without first checking for NULL? >=20 > static are initialized as NULL. Yes, but down below, you call s390_get_ap_bridge() to set apb regardless of whether apb is NULL or not. It makes no sense to declare is as static here. It is also redundant because the=20 s390_get_ap_bridge() function already caches it in a static variable. >=20 >> >>> +=C2=A0=C2=A0=C2=A0 BusState *bus; >>> +=C2=A0=C2=A0=C2=A0 BusChild *child; >>> +=C2=A0=C2=A0=C2=A0 static APDevice *ap; >>> + >>> +=C2=A0=C2=A0=C2=A0 if (ap) { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ap; >>> +=C2=A0=C2=A0=C2=A0 } >>> + >>> +=C2=A0=C2=A0=C2=A0 apb =3D s390_get_ap_bridge(); >>> +=C2=A0=C2=A0=C2=A0 /* We have only a single child on the BUS */ >>> +=C2=A0=C2=A0=C2=A0 bus =3D qdev_get_child_bus(apb, TYPE_AP_BUS >>> +=C2=A0=C2=A0=C2=A0 child =3D QTAILQ_FIRST(&bus->children); >>> +=C2=A0=C2=A0=C2=A0 assert(child !=3D NULL); >>> +=C2=A0=C2=A0=C2=A0 ap =3D DO_UPCAST(APDevice, parent_obj, child->chi= ld); >> >> I received a comment from Thomas Huth in Message ID >> <2291104a-4cbf-e4fd-3496-fa0910beb96a@redhat.com> >> that DO_UPCAST should be avoided in new code. You should >> create an AP_DEVICE macro for this in ap-device.h >> >=20 > Thanks I will do. >=20 > Regards, > Pierre >=20 >=20