[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI


  • To: Luca Fancellu <luca.fancellu@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 29 Sep 2021 10:00:53 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=kNCv2JfxAmGrCKSm1zBbx09ONKsK59pscIEd/URrupE=; b=jrvzm6QtJ5Z6h76QtSrkHhOfbTnELpWq2GjWBglqCBGJyANkN14e1yV4SdIqFNuQFyfrKkdo6MRAF3Dg+lqXc3NnHmMLjDcv94kTOx9/kIFYTZY8aSUww+y9Oye9Cem4qQ1c8Zfjh6JyE8JcEXZRXlzGLhNEULduv0kEIXTgbZ3bWMWSfenmL1yohQUDkaPYnBkfAsBbVT5j0BMANgYKgnoQWIrYnPmALf/fwsaMeuPoFrZX5ZoUEZgADBaZrD/bj4WrSkNT8mrxh/5JrSFVsC8sdSEkzYXhs6ligIVeqgYwa8qJ5xzNAqUaqygTWcKTTnQ17krN4cRcfvwwuc/V/g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fRskDfeSb3hwMq/cMbYH5pvdR0YRa1OdI83JLMzIA7f+6J+C09jOqggOXLwJZftQNHaAibMCZN17eZSEWefW+XtDSWDgqwVYLjgm/OFMQz+nsXvf7WiE7bEkdE8l4b8TBwDhekw0rdKhDPdutYFmOuLmWwvefFHStcLIF30tnC8O3JTZGoJ4Rcp2r2zPlyPP16Tw3B9qqIblMKT07+DkG6kOJMZF+bOnXlBHWCQIWuRSGA4SnYq3I3vuOqq9bsBoGj3LOdKADnAt2K1x3Nybj1QNCNJOriyz81o8nwCBtQCg0Ig5Vpi/PFwGiJDgAxZcjyYZqGu1e/w5XfFyYYsPMg==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: bertrand.marquis@xxxxxxx, wei.chen@xxxxxxx, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 29 Sep 2021 08:01:09 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 28.09.2021 18:32, Luca Fancellu wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>          {
>              read_file(dir_handle, s2w(&name), &kernel, option_str);
>              efi_bs->FreePool(name.w);
> -
> -            if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> -                            (void **)&shim_lock)) &&
> -                 (status = shim_lock->Verify(kernel.ptr, kernel.size)) != 
> EFI_SUCCESS )
> -                PrintErrMesg(L"Dom0 kernel image could not be verified", 
> status);
>          }
>  
>          if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
> @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>      if (dt_module_found < 0)
>          /* efi_arch_check_dt_boot throws some error */
>          blexit(L"Error processing boot modules on DT.");
> +
> +    /* If Dom0 is specified, verify it */
> +    if ( kernel.ptr &&
> +         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> +                                           (void **)&shim_lock)) &&
> +        (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS 
> )
> +        PrintErrMesg(L"Dom0 kernel image could not be verified", status);

Why does this code need moving in the first place? That's (to me at least)
not obvious from looking just at the common (i.e. non-Arm-specific) part.
Hence I would wish for the comment to be slightly extended to indicate
that the kernel image may also have been loaded by <whichever function>.

Also, two nits: You've broken indentation here (you've lost a space too
much compared to the original code) and ...

>      /*
>       * Check if a proper configuration is provided to start Xen:
>       *  - Dom0 specified (minimum required)
> 

... you will want to insert a blank line not only before, but also after
your insertion (or, imo less desirable, neither of the two blank lines).

Further I wonder whether your addition wouldn't better be after the code
following this comment.

And finally I wonder (looking back at the earlier patch) why you use
kernel.addr there when kernel.ptr is being used here. Unless there's a
reason for the difference, being consistent would be better.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.