Re: [PATCH 2/2] efi_selftest: check for deleted handles in the event notification list

[Heinrich Schuchardt]

Am 1. Juni 2023 21:32:04 MESZ schrieb Ilias Apalodimas <ilias.apalodimas at linaro.org>:
>Hi Heinrich, 
>
>[...]
>> > + * @event	notified event
>> > + * @context	pointer to the notification count
>> > + */
>> > +static void EFIAPI test_uninstall_notify(struct efi_event *event, void *context)
>> > +{
>> > +	return;
>> > +}
>> > +
>> >   /*
>> >    * Setup unit test.
>> >    *
>> > @@ -91,6 +108,21 @@ static int setup(const efi_handle_t img_handle,
>> >   		return EFI_ST_FAILURE;
>> >   	}
>> > 
>> > +	ret = boottime->create_event(EVT_NOTIFY_SIGNAL,
>> 
>> You could use the existing notify function.
>> 
>
>The reason I created a new one is that the current one calls
>locate_handle_buffer().  See below 
>
>> > +				     TPL_CALLBACK, test_uninstall_notify, NULL,
>> > +				     &test_uninstall_event);
>> > +	if (ret != EFI_SUCCESS) {
>> > +		efi_st_error("could not create event\n");
>> > +		return EFI_ST_FAILURE;
>> > +	}
>> > +
>> > +	ret = boottime->register_protocol_notify(&guid3, test_uninstall_event,
>> > +						 &test_uninstall_key);
>> 
>> I would prefer to use the same guid1 protocol. Deleting one handle
>> should not remove the other handle from the handle queue if both have
>> the same protocol installed.
>> 
>
>Unless I am missing something, that would not work. The notifier function
>will call locate_handle_buffer and will retrieve all handles.  As a result

This would not match the specification.

LocateHandle(ByRegistrationKey) must only retrieve at maximum a single handle per call irrespective of the queue size. LocateHandleBuffer() should do the same.

Regards

Heinrich 

>the subsequent call will return EFI_NOT_FOUND.  You've already sent a ptch
>covering that, the purpose of this one is check that the handler will be
>clearer from the events list if the protocol gets uninstalled. 
>We could use guid2 but I do prefer habing distinct protocols for every
>test.
>
>> > +	if (ret != EFI_SUCCESS) {
>> > +		efi_st_error("could not register event\n");
>> > +		return EFI_ST_FAILURE;
>> > +	}
>> > +
>> >   	return EFI_ST_SUCCESS;
>> >   }
>> > 
>> > @@ -121,8 +153,10 @@ static int teardown(void)
>> >   static int execute(void)
>> >   {
>> >   	efi_status_t ret;
>> > -	efi_handle_t handle1 = NULL, handle2 = NULL;
>> > -	struct interface interface1, interface2;
>> > +	efi_handle_t handle1 = NULL, handle2 = NULL, handle3 = NULL;
>> > +	struct interface interface1, interface2, interface3;
>> > +	efi_uintn_t handle_count;
>> > +	efi_handle_t *handles;
>> > 
>> >   	ret = boottime->install_protocol_interface(&handle1, &guid1,
>> >   						   EFI_NATIVE_INTERFACE,
>> > @@ -224,6 +258,29 @@ static int execute(void)
>> >   		return EFI_ST_FAILURE;
>> >   	}
>> > 
>> > +	ret = boottime->install_protocol_interface(&handle3, &guid3,
>> > +						   EFI_NATIVE_INTERFACE,
>> > +						   &interface3);
>> > +	if (ret != EFI_SUCCESS) {
>> > +		efi_st_error("could not install interface\n");
>> > +		return EFI_ST_FAILURE;
>> > +	}
>> 
>> Please, check the number of invocations of the notify function.
>> 
>
>I can add that, but the tests already check that case in detail. 
>
>> > +
>> > +	ret = boottime->uninstall_multiple_protocol_interfaces
>> > +			(handle3, &guid3, &interface3, NULL);
>> > +	if (ret != EFI_SUCCESS) {
>> > +		efi_st_error("UninstallMultipleProtocolInterfaces failed\n");
>> > +		return EFI_ST_FAILURE;
>> > +	}
>> 
>> And here again. We don't expect UninstallMultipleProtocolInterfaces() to
>> trigger the event.
>> 
>
>ditto
>
>Thanks
>/Ilias
>> Best regards
>> 
>> Heinrich
>> 
>> > +
>> > +	ret = boottime->locate_handle_buffer(BY_REGISTER_NOTIFY, NULL,
>> > +					     test_uninstall_key,
>> > +					     &handle_count, &handles);
>> > +	if (ret != EFI_NOT_FOUND) {
>> > +		efi_st_error("UninstallMultipleProtocolInterfaces failed to delete event handles\n");
>> > +		return EFI_ST_FAILURE;
>> > +	}
>> > +
>> >   	return EFI_ST_SUCCESS;
>> >   }
>> > 
>> > --
>> > 2.39.2
>> > 
>>