Messages in this thread | | | Date | Thu, 19 Jan 2023 18:19:22 +0100 | Subject | Re: [PATCH 3/4] firmware: Add support for Qualcomm UEFI Secure Application | From | Maximilian Luz <> |
| |
On 1/19/23 17:47, Johan Hovold wrote: > On Wed, Jan 18, 2023 at 09:45:18PM +0100, Maximilian Luz wrote: >> On 1/17/23 09:24, Johan Hovold wrote: >>> On Sun, Jul 24, 2022 at 12:49:48AM +0200, Maximilian Luz wrote: > >>>> +module_platform_driver(qcom_uefisecapp_driver); >>> >>> I noticed that for efivarfs to work, you're currently relying on having >>> the firmware still claim that the variable services are supported in the >>> RT_PROP table so that efi core registers the default ops at subsys init >>> time (which are later overridden by this driver). >>> >>> Otherwise efivarfs may fail to initialise when built in: >>> >>> static __init int efivarfs_init(void) >>> { >>> if (!efivars_kobject()) >>> return -ENODEV; >>> >>> return register_filesystem(&efivarfs_type); >>> } >>> >>> module_init(efivarfs_init); >>> >>> With recent X13s firmware the corresponding bit in the RT_PROP table has >>> been cleared so that efivarfs would fail to initialise. Similar problem >>> when booting with 'efi=noruntime'. >>> >>> One way to handle this is to register also the qcom_uefisecapp_driver at >>> subsys init time and prevent it from being built as a module (e.g. as is >>> done for the SCM driver). I'm using the below patch for this currently. >> >> So I've had another look and I'm not sure this will work reliably: >> >> First, you are correct in case the RT_PROP table is cleared. In that >> case, using subsys_initcall() will move the efivar registration before >> the efivarfs_init() call. >> >> However, in case EFI indicates support for variables, we will then have >> generic_ops_register() and the uefisecapp's driver call running both in >> subsys_initcall(). So if I'm not mistaken, this could cause the generic >> ops to be registered after the uefisecapp ones, which we want to avoid. > > Good catch, I was using 'efi=noruntime' on the CRD so I did not notice > that race. > >> One solution is bumping uefisecapp to fs_initcall(). Or do you have any >> other suggestions? > > I think it would be best to avoid that if we can, but that should work. > > The problem here is that the firmware claims to support the EFI variable > services even when it clearly does not and the corresponding callbacks > just return EFI_UNSUPPORTED. As far as I understand, this is still spec > compliant though so we just need to handle that. > > One way to address this could be to have efi core not register the > default efivars ops in this case. That would require checking that the > services are indeed available by making one of those calls during > initialisation. > > This would however expose the fact that the Google SMI implementation > implicitly relies on overriding the default ops, but I think that's a > good thing as what we have now is racy in multiple ways. > > Instead I think we should move the efivarfs availability check from > module init to mount time. That should allow the Google driver, and your > SCM implementation, to continue to be built as modules. > > Any consumers (e.g. the Qualcomm RTC driver) would instead need to > check if efivars is available or else defer probe. > > Alternatively, it seems all efivars implementation would need to be > always-built in which is not ideal for generic kernels. > > I just posted a series here as food for thought: > > https://lore.kernel.org/r/20230119164255.28091-1-johan+linaro@kernel.org
Thanks, I agree that those checks are probably the better option.
Regards, Max
| |