Re: [PATCH v4 2/8] firmware: smccc: Add support for Live Firmware Activation (LFA)

[Andre Przywara <andre.przywara@arm.com>]

Hi Mark,

thanks for the reply, your input is much appreciated!

On 10/13/26 16:48, Mark Rutland wrote:
> On Mon, Sep 21, 2026 at 05:25:25PM +0200, Andre Przywara wrote:
>> On 9/18/26 17:24, Mark Rutland wrote:
>>> On Fri, Sep 18, 2026 at 04:11:05PM +0200, Andre Przywara wrote:
>>>> From: Salman Nabi <salman.nabi@arm.com>
>>>>
>>>> The Arm Live Firmware Activation (LFA) is a specification [1] to describe
>>>> activating firmware components without a reboot. Those components
>>>> (like TF-A's BL31, EDK-II, TF-RMM, secure paylods) would be updated the
>>>> usual way: via fwupd, FF-A or other secure storage methods, or via some
>>>> IMPDEF Out-Of-Bound method. The user can then activate this new firmware,
>>>> at system runtime, without requiring a reboot.
>>>> The specification covers the SMCCC interface to list and query available
>>>> components and eventually trigger the activation.
>>>
>>> It'd be helpful to have a few basic concepts covered here or in the
>>> code. For example, I have no idea what a "rendezvous" is, and it's not
>>> at all obvious from the code.
>>
>> OK, I can give a brief summary of the concepts, but I guess doing this a top
>> comment in the source file is more useful than here in the commit message?
> 
>  From my PoV, even placing that in the cover letter would be fine. The
> key thing is just to get enough context to be able to review this
> without having to ingest the entire LFA spec.
> 
> In particular with "rendezvous", I have no idea why the admin should
> have any choice at all, and without some context I can't tell if there's
> some sensible use-case I'm missing.

The admin should indeed not have a say, and if the firmware component 
requires rendezvous, it will always be used.
But for firmware that claims to not need any rendezvous, we default to 
using it anyway, because it's the safest way to guarantee continuity for 
the kernel. We don't know which firmware services might be affected, and 
if they respond with something like -EAGAIN or just block for a longer 
period during the activation. The kernel seems to rely on the firmware 
services (TRNG, SoC-ID, SCMI, ...) replying, once they have been 
initialised, so them suddenly returning an (even transient) error will 
probably confuse the drivers.
So the safe way to do this is to send all cores into the firmware 
regardless, there they cannot do any harm.

Now as users asked for it on the list, if the admin knows exactly that 
this firmware component does not provide any services to the kernel (say 
some fan controller), they can override this policy and allow a single 
CPU to trigger the activation. This was meant as a compromise to keep 
the kernel side sane, but still allow less disruptive updates. The idea 
would be that some user space component sets this attribute at boot 
time, if it matches some UUID that is known to be safe.

 > [...]>
>>>> +static const char *get_image_name(const struct fw_image *image)
>>>> +{
>>>> +	if (image->image_name && image->image_name[0] != '\0')
>>>> +		return image->image_name;
>>>> +
>>>> +	return kobject_name(&image->kobj);
>>>> +}
>>>
>>> As before, please just use the UUID.
>>>
>>> I *assume* that we cannot have more than one image for a given
>>> component, and hence we cannot have two entries for the same UUID? Is
>>> that mandated by the LFA spec?
>>
>> The LFA spec speaks about "firmware components" as its area of interest, by
>> design all the real firmware *image* related parts, for instance how the
>> firmware update image lands on the system, is left implementation defined.
>> So yes, each firmware component, identified by its UUID, appears only once
>> in the system, by design.
> 
> Ok; does the spec *clearly* mandate that?

Well, there is only one logical bit (somewhat confusingly named 
"activation pending") to say whether an update image has been provided 
for a specific component or not. If someone pushes another image, it 
would replace the already "pending" one (depending on firmware policy).

> 
>> So I guess using "image" everywhere is a bit imprecise, but I am bit
>> reluctant to change all identifiers now. Not even sure "firmware component"
>> is a well recognised term in the first place.
>>
>> Let me know what you think about that.
> 
> Using "component" sounds fine by me. Then we can clearly describe
> loading an image *for* a component.
> 
> ... unless I'm still misunderstanding the terminology here? I assume
> "component" is a logical entity, and "image" is a specific binary blob.

Yes, that's correct.

> 
> [...]
> 
>>>> +static int activate_fw_image(struct fw_image *image)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +retry:
>>>> +	/*
>>>> +	 * cpu_rendezvous_forced is set by the administrator, via sysfs,
>>>> +	 * cpu_rendezvous is dictated by each firmware component.
>>>> +	 */
>>>> +	image->use_cpu_rendezvous = image->cpu_rendezvous_forced ||
>>>> +				    image->cpu_rendezvous;
>>>> +	if (image->use_cpu_rendezvous)
>>>> +		ret = stop_machine(call_lfa_activate, image, cpu_online_mask);
>>>
>>> What prevents cpu_online_mask changing under your feet here?
>>
>> Right, I see. Is cpus_read_lock() safe to hold around stop_machine()?
> 
> Looking at the core code, the locking is handled within stop_machine(),
> and since cpu_online_mask is a pointer, we can safely pass that around
> without extra locking.
> 
> So this is fine as-is. Sorry for the noise.
> 
> [...]
> 
>>> This looks odd given call_lfa_activate() doesn't seem to distinguish
>>> secondaries from one another. Does LFA expect all callers to pass the
>>> exact same arguments for a "rendezvous"?
>>
>> Yes, even though the spec doesn't explicitly mention this. But there aren't
>> many arguments to deviate anyway: the FID and sequence ID are naturally the
>> same, and the flags just indicate whether we skip the rendezvous, so that
>> doesn't really apply here (and inconsistencies are checked and handled by
>> TF-A).
>> The LFA agent (the firmware side) just elects one primary core doing the
>> work, I think in TF-A it's the first one doing the SMC. The others just wait
>> in EL3, to not mess with the update process.
> 
> I think i'd be worth describing this somewhere. Even if just the level
> of detail above, with a reference ot the relevant part of the spec.

OK, I added a sentence to that effect.
And btw: TF-A uses the last core to call in to do the activation, but 
this is an implementation detail.

> 
> [...]
> 
>> We had it like this originally in v1, I don't remember why we changed that,
>> and it wasn't mentioned in any of the reply emails.
>> Maybe I thought it looked neater to explicitly mark this as a retry, as we
>> don't expect this to happen, normally.
>> Will try to change it back and see what happens ;-)
>>
>>>> +
>>>> +	lfa_cancel(image);
>>>> +
>>>> +	pr_err("LFA_ACTIVATE for image %s failed\n", get_image_name(image));
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int prime_fw_image(struct fw_image *image)
>>>> +{
>>>> +	struct arm_smccc_1_2_regs reg = { 0 }, res;
>>>
>>> Do you need separate input and output strutctures?
>>
>> As mentioned above: in this case yes, since we call LFA_PRIME potentially
>> multiple times, and then the function ID would be overwritten by the error
>> return code, at least.
> 
> Ok, but you reinitialise 'a1' every time round the loop, so can you just
> reinitialise the few registers you need each time around?
> 
> See below for a more concrete example.
> 
>>>
>>>> +	int ret;
>>>> +
>>>> +	if (image->may_reset_cpu) {
>>>> +		pr_err("CPU reset not supported by kernel driver\n");
>>>> +
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> Unnecessary newline again.
>>>
>>>> +
>>>> +	reg.a0 = ARM_SMCCC_LFA_PRIME;
>>>> +retry:
>>>> +	/*
>>>> +	 * LFA_PRIME will return 1 in reg.a1 if the firmware priming
>>>> +	 * is still in progress. In that case LFA_PRIME will need to
>>>> +	 * be called again.
>>>> +	 * reg.a1 will become 0 once the prime process completes.
>>>> +	 */
>>>> +	reg.a1 = image->fw_seq_id;
>>>> +	arm_smccc_1_2_invoke(&reg, &res);
>>>> +	if ((long)res.a0 < 0) {
>>>> +		pr_err("LFA_PRIME for image %s failed: %s\n",
>>>> +		       get_image_name(image),
>>>> +		       lfa_error_string((long)res.a0));
>>>> +
>>>> +		return lfa_to_linux_errno((long)res.a0);
>>>> +	}
>>>> +
>>>> +	if (res.a1 & LFA_PRIME_CALL_AGAIN)
>>>> +		goto retry;
>>>
>>> Should this be a loop?
> 
> 
> As above, if this were a loop you could do something like:
> 
> 	bool retry;
> 
> 	do {
> 		struct arm_smccc_1_2_regs regs = {
> 			.a0 = ARM_SMCCC_LFA_PRIME,
> 			.a1 = image->fw_seq_id,
> 		};
> 
> 		arm_smccc_1_2_invoke(&regs, &regs);
> 
> 		if ((long)args.a0 < 0) {
> 			...
> 		}
> 		
> 		retry = res.a1 & LFA_PRIME_CALL_AGAIN;
> 	} while (retry)
> 
> ... and the initialization of the registers isn't a special case.

OK, I had the loop quite similar to this meanwhile, but can add the regs 
declaration inside, to avoid the "res" variable.

> [...]
> 
>>>> +static ssize_t pending_version_show(struct kobject *kobj,
>>>> +				    struct kobj_attribute *attr, char *buf)
>>>> +{
>>>> +	struct fw_image *image = kobj_to_fw_image(kobj);
>>>> +	struct arm_smccc_1_2_regs reg = { 0 };
>>>> +
>>>> +	/*
>>>> +	 * Similar to activation pending, this value can change following an
>>>> +	 * update, we need to retrieve fresh info instead of stale information.
>>>> +	 */
>>>> +	reg.a0 = ARM_SMCCC_LFA_GET_INVENTORY;
>>>> +	reg.a1 = image->fw_seq_id;
>>>> +	arm_smccc_1_2_invoke(&reg, &reg);
>>>> +	if (reg.a0 == LFA_SUCCESS) {
>>>> +		if (reg.a5 != 0 && image->activation_pending) {
>>>> +			u32 maj, min;
>>>> +
>>>> +			image->pending_version = reg.a5;
>>>> +			maj = reg.a5 >> 32;
>>>> +			min = reg.a5 & 0xffffffff;
>>>
>>> You could use upper_32_bits() and lower_32_bits() here.
>>>
>>>> +
>>>> +			return sysfs_emit(buf, "%u.%u\n", maj, min);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return sysfs_emit(buf, "N/A\n");
>>>
>>> Why a string rather than an error code?
>>
>> This value is somewhat optional, it's only valid if there is an "activation
>> pending" (read: an update image has been provided and waits for its
>> activation).
> 
> I'm confused. This function is called pending_version_show(); is that a
> different thing that's "pending"?

I meant that there is naturally no *update* version number to report 
until an update image has been provided. Maybe the confusion is about 
the word "pending" here, which is a bit of an overloaded term, I think.

>> But it's not really the firmware's or the user's fault here if
>> that's the case (in fact it's the common case), so I am not sure if an error
>> return is appropriate, since in the current state the value is simply not
>> applicable. And the "cat" command returning with an error looks ugly. Since
>> it's a string, it's easy to distinguish from any actual version number from
>> userland, in scripts.
>>
>> Any suggestion how to handle this otherwise? Just return -ENXIO?
> 
> If this is a legitimate state, returning a string is fine so long as it
> is clearly documented.

I chose to return 0 from this function, as in: zero bytes read. This 
should be distinguishable for userland from a real error or any other 
version string. Hope that's fine, if not, feel free to comment on v5.

> 
>>> [...]
>>>
>>>> +static int lfa_smccc_probe(struct arm_smccc_device *sdev)
>>>> +{
> 
>>>> +}
>>>
>>> There was an interrupt in the binding, not detected or used here. What's
>>> going on?
>>
>> That's implemented in patch 6/8.
>> That patch here is already quite large, also different parts (ACPI, timeout,
>> DT, locking) were contributed by different authors, so I kept them as
>> separate patches, to give credit and simplify review.
>> I can of course squash them all in one patch, maybe it's time to do this
>> now, but this makes attribution a bit fuzzy.
> 
>  From my perspsecitve, if the interrupt is expected, it's odd to add the

The interrupt is completely optional, and really only useful if you do 
out-of-band provisioning of the update image. And the automatic 
activation feature comes in a separate patch anyway.

> probing support later. At this stage inthe series, the LFA driver will
> probe even without an interrupt, but then suddenly stops later in the
> series.

Well, this would only be the case if the interrupt would be wrongly 
configured, not sure that is something that it that critical from a 
regression point of view, especially if we really only talk about like 
four patches in between?

I would rather keep at least the ACPI patch separate, and be it for 
given Vedashree her commit.

Cheers,
Andre

> 
> Mark.