From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 01 Jul 2019 07:40:12 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5DB893086216; Mon, 1 Jul 2019 14:39:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.14]) by smtp.corp.redhat.com (Postfix) with ESMTP id 140D263B83; Mon, 1 Jul 2019 14:39:42 +0000 (UTC) Subject: Re: [PATCH] OvmfPkg/XenBusDxe: Don't call DisconnectController in Stop() To: Anthony PERARD , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel , Julien Grall References: <20190701111403.7007-1-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: <9f384863-60e4-efec-0f95-e61434469292@redhat.com> Date: Mon, 1 Jul 2019 16:39:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190701111403.7007-1-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Mon, 01 Jul 2019 14:39:59 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/01/19 13:14, Anthony PERARD wrote: > Calling DisconnectController() on children isn't part of the job of > EFI_DRIVER_BINDING_PROTOCOL.Stop() as it only needs to deallocate > resources allocated in Start(). The disconnection will happen when > both DevicePath and XenBus protocols gets uninstalled. Correct. In the spec, UninstallMultipleProtocolInterfaces() refers to UninstallProtocolInterface(), and the latter says, [...] Before the protocol interface is removed, an attempt is made to force all the drivers that are consuming the protocol interface to stop consuming that protocol interface. This is done by calling the boot service EFI_BOOT_SERVICES.DisconnectController() for the driver that currently have the protocol interface open with an attribute of EFI_OPEN_PROTOCOL_BY_DRIVER or EFI_OPEN_PROTOCOL_BY_DRIVER | EFI_OPEN_PROTOCOL_EXCLUSIVE. [...] And, the Driver Writer's Guide states, in a Note, When an attempt is made to remove a protocol interface from a handle in the handle database, the UEFI core firmware checks to see if any other UEFI drivers are currently using the services of the protocol to be removed. If UEFI drivers are using that protocol interface, the UEFI core firmware attempts to stop those UEFI drivers with a call to DisconnectController(). This is a quick, legal, and safe way to shut down any protocols associated with this driver's stack. > > Reported-by: Laszlo Ersek > Signed-off-by: Anthony PERARD > --- > > Notes: > Please apply this patch after: > "OvmfPkg/XenBusDxe: Close XenIoProtocol openned by children" > > OvmfPkg/XenBusDxe/XenBusDxe.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c > index 7c07a96650..634c7b71eb 100644 > --- a/OvmfPkg/XenBusDxe/XenBusDxe.c > +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c > @@ -446,12 +446,6 @@ XenBusDxeDriverBindingStop ( > continue; > } > ChildData = XENBUS_PRIVATE_DATA_FROM_THIS (XenBusIo); > - Status = gBS->DisconnectController (ChildData->Handle, NULL, NULL); > - if (EFI_ERROR (Status)) { > - DEBUG ((EFI_D_ERROR, "XenBusDxe: error disconnecting child: %r\n", > - Status)); > - continue; > - } > > Status = gBS->CloseProtocol (Dev->ControllerHandle, &gXenIoProtocolGuid, > Dev->This->DriverBindingHandle, ChildData->Handle); > Reviewed-by: Laszlo Ersek