Hi Julien,
> On 30 Mar 2025, at 23:26, Julien Grall <jul...@xen.org> wrote:
>
> Hi Bertrand,
>
> On 27/03/2025 08:25, Bertrand Marquis wrote:
>>> On 27 Mar 2025, at 00:14, Julien Grall <jul...@xen.org> wrote:
>>>> +static int32_t ffa_get_sp_partinfo(uint32_t *uuid, uint32_t *sp_count,
>>>> + void *dst_buf, void *end_buf,
>>>> + uint32_t dst_size)
>>>> {
>>>> int32_t ret;
>>>> + uint32_t src_size, real_sp_count;
>>>> + void *src_buf = ffa_rx;
>>>> + uint32_t count = 0;
>>>> +
>>>> + /* Do we have a RX buffer with the SPMC */
>>>> + if ( !ffa_rx )
>>>> + return FFA_RET_DENIED;
>>>> +
>>>> + /* We need to use the RX buffer to receive the list */
>>>> + spin_lock(&ffa_rx_buffer_lock);
>>>> +
>>>> + ret = ffa_partition_info_get(uuid, 0, &real_sp_count, &src_size);
>>>> + if ( ret )
>>>> + goto out;
>>>> +
>>>> + /* We now own the RX buffer */
>>>> +
>>>> + /* We only support a 1.1 firmware version */
>>>> + if ( src_size < sizeof(struct ffa_partition_info_1_0) )
>>>> + {
>>>> + ret = FFA_RET_NOT_SUPPORTED;
>>>> + goto out_release;
>>>> + }
>>>> +
>>>> + for ( uint32_t sp_num = 0; sp_num < real_sp_count; sp_num++ )
>>>
>>> What's the upper limit of real_sp_count? Asking just in case we need to
>>> handle preemption.
>> In theory that would be 32k but in practice the number of entries we can
>> receive is
>> limited by the size of the exchange area we have with the secure world.
>> This area is currently defined to be one page and each entry is 8 byte in the
>> case where firmware is 1.0 (24 bytes otherwise).
>> This is an upper limit of 500 entries.
>> Now I do think this is completely unrealistic to imagine a secure world with
>> 500 SPs
>> so If you are ok I would rather define an upper limit in Xen (I would say 64
>> SPs) that
>> can be changed in the code (through a define).
>> This call currently does not support preemption in the FF-A spec (and that
>> is something
>> i will check for future versions) so I would have no solution to "continue"
>> it.
>
>> Would the "define" solution be acceptable for now ?
>
> I think the define solution is acceptable for now and possibly longer. It is
> an easy way to avoid dealing with preemption.
I will do something with a default value of 64 which should fit for now (and
maybe longer).
>
> [...]
>
>
>>>> +static uint32_t ffa_get_vm_count(void)
>>>
>>> Is this meant to be called often? If so, can we instead have a counter that
>>> will be incremented when the vTEE is first initialized and then decremented
>>> when the VM is destroyed?
>> As of now we could have a global counter that we increase or decrease
>> when a domain version is negociated and when a domain is destroyed.
>> We could also have some kind of global save of the result to be returned
>> to a guest.
>> But I did not do that because:
>> - cpu time required to update the list would be taken out an FF-A call
>> for FFA_VERSION of a VM which would require a global lock to protect
>> the data
>
> I would have thought an atomic counter would be sufficient. Is there anything
> you had in mind?
Very good idea. I only need to increase it when an FFA_VERSION has been
negociated and decrease on VM destruction.
I will do that.
>
>> - when we will have filtering the data will be per VM (which would make
>> the initial update more complex)
>> - incoming we have a notion of UUID and filtering depending on the
>> requested UUID which will make the global value only useable in a
>> limited number of cases.
>> I have 2 pending series on top of this one which would have to remove
>> such optimisations.
>> At the end this is definitely not something supposed to call often (linux
>> driver is calling it once during init).
>
> I think it was a mistake for me to asked whether this is called often or not.
> When it comes to security, what matter is whether a malicious guest could
> indirectly call ffa_get_vm_count() and delay any work on the pCPU (Xen is not
> preemptible).
>
> We don't have to resolve this now. But this will need to be addressed before
> we can we consider FFA security supported. So we should at least add it in
> the list of issue at the top of the file.
No this is really useful and I will fix it and also the partition info part
because:
- all information is read only and known when the VM is created (VM ID, max
vcpus and 64bit or not)
- I do not need anything else than that to build the result
- If we can prevent to take a lock this will make the code better.
So I will:
- add an atomic to count the number of VMs with FF-A
- create a chain list of VM FF-A contexts
- add a context to the chain when a version is negociated
- put the infos i need in the ffa ctx structure
- just go through the list to build the partinfo result
>
>>>
>>>> +{
>>>> + struct domain *d = current->domain;
>>>> + struct domain *dom;
>>>
>>> NIT: "d" and "dom" are a bit too close. Could we rename "d" with "curr_d"?
>> i will go with curr_d dest_d to make this clearer.
>>>
>>>> + uint32_t vm_count = 0;
>>>> +
>>>> + /* Count the number of VM with FF-A support */
>>>
>>> This comment implies this is including the current VM. But ...
>>>
>>>> + rcu_read_lock(&domlist_read_lock);
>>>> + for_each_domain( dom )
>>>> + {
>>>> + struct ffa_ctx *vm = dom->arch.tee;
>>>> +
>>>> + if ( dom != d && vm != NULL && vm->guest_vers != 0 )
>>>
>>> ... here you are excluding it. Also, it sounds like this is support by the
>>> OS rather than the VM itself. Is that correct?
>> I have a comment to explain that one in a different serie that i will put
>> here.
> > > Basically before 1.2, the spec was a bit blurry on if or not the result
> > > should include the
>> calling VM and in fact Linux driver (before 1.2) was ending with an error if
>> its own data
>> was included in the result hence this filter.
>
> Thanks for the clarification. Just to clarify...
>
>> I will add a comment for that.
>
> ... will the comment be added in this patch?
Yes i will add the comment in the next version of the patch.
>
>>>
>>>> + vm_count++;> + }
>>>> + rcu_read_unlock(&domlist_read_lock);
>>>> +> + return vm_count;
>>>
>>> OOI, I guess this value is just used as an hint? Asking because the number
>>> of domains can change at any point.
>> Definitely yes. The data is what it is when called but anything could change
>> after.
>> This is mostly used as hint by callers.
>
> Does this mean we would always return a fixed number? Asking because this
> would solve nicely the preemption problem in ffa_get_vm_count().
Well the number is changing when VMs are created or destroyed but as explain
earlier i will modify the design to prevent the lock
using an atomic and a chain list.
Now the problem of the potential high number of VMs still stand so I will add a
todo for that and discuss with others to check if this could be solved in the
FF-A spec in the feature.
Thanks a lot for the comments.
I will work on v4 for when you get back :-)
Cheers
Bertrand
