Re: [Xen-devel] [PATCH v3 06/11] optee: add std call handling

[Julien Grall <julien.grall@xxxxxxx>]

Hi Volodymyr,

On 17/01/2019 15:21, Volodymyr Babchuk wrote:
> Julien Grall writes:
>
> > Hi Volodymyr,
> >
> > On 18/12/2018 21:11, Volodymyr Babchuk wrote:
> > > From: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>
> > >
> > > The main way to communicate with OP-TEE is to issue standard SMCCC
> > > call. "Standard" is a SMCCC term and it means that call can be
> > > interrupted and OP-TEE can return control to NW before completing
> > > the call.
> > >
> > > In contrast with fast calls, where arguments and return values
> > > are passed in registers, standard calls use shared memory. Register
> > > pair a1,a2 holds 64-bit PA of command buffer, where all arguments
> > > are stored and which is used to return data. OP-TEE internally
> > > copies contents of this buffer into own secure memory before accessing
> > > and validating any data in command buffer. This is done to make sure
> > > that NW will not change contents of the validated parameters.
> > >
> > > Mediator needs to do the same for number of reasons:
> > >
> > > 1. To make sure that guest will not change data after validation.
> > > 2. To translate IPAs to PAs in the command buffer (this is not done
> > >      in this patch).
> > > 3. To hide translated address from guest, so it will not be able
> > >      to do IPA->PA translation by misusing mediator.
> > >
> > > During standard call OP-TEE can issue multiple "RPC returns", asking
> > > NW to do some work for OP-TEE. NW then issues special call
> > > OPTEE_SMC_CALL_RETURN_FROM_RPC to resume handling of the original call.
> > > Thus, mediator needs to maintain context for original standard call
> > > during multiple SMCCC calls.
> > >
> > > Standard call is considered complete, when returned value is
> > > not a RPC request.
> > >
> > > Signed-off-by: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>
> > > ---
> > >
> > >    Changes from v2:
> > >     - renamed struct domain_ctx to struct optee_domain
> > >     - fixed coding style
> > >     - Now I use access_guest_memory_by_ipa() instead of mappings
> > >       to read command buffer
> > >     - Added tracking for in flight calls, so guest can't resume
> > >       the same call from two CPUs simultaniously
> > >
> > >    xen/arch/arm/tee/optee.c     | 319 ++++++++++++++++++++++++++++++++++-
> > >    xen/include/asm-arm/domain.h |   3 +
> > >    2 files changed, 320 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> > > index 584241b03a..dc90e2ed8e 100644
> > > --- a/xen/arch/arm/tee/optee.c
> > > +++ b/xen/arch/arm/tee/optee.c
> > > @@ -12,6 +12,8 @@
> > >     */
> > >      #include <xen/device_tree.h>
> > > +#include <xen/domain_page.h>
> > > +#include <xen/guest_access.h>
> > >    #include <xen/sched.h>
> > >    #include <asm/smccc.h>
> > >    #include <asm/tee/tee.h>
> > > @@ -22,11 +24,38 @@
> > >    /* Client ID 0 is reserved for hypervisor itself */
> > >    #define OPTEE_CLIENT_ID(domain) (domain->domain_id + 1)
> > >    +/*
> > > + * Maximal number of concurrent standard calls from one guest. This
> > > + * corresponds to OPTEE configuration option CFG_NUM_THREADS, because
> > > + * OP-TEE spawns a thread for every standard call.
> > Looking at OP-TEE, CFG_NUM_THREADS will vary depending on the
> > platform. Is there any way to probe that number of threads from Xen?
> Yes, this is per-platform configuration.
> Easiest way is to add Fast SMC to OP-TEE that will report this
> parameter.
> Jens, what do you think about adding additional call?
>
> > In any case, I think we should update the comment to reflect that this
> > seems to be the maximum CFG_NUM_THREADS supported by any upstream
> > platform.
> Actually, OP-TEE protocol have possibility to handle limited number of
> threads correctly. OP-TEE can report that all threads are busy and
> client will wait for a free one. XEN can do the same, although this is not
> implemented in this patch series. But I can add this.
Could you expand by wait? Will it block in OP-TEE/Xen or does it return to the 
guest?
> Basically this means that all can work correctly even with
> MAX_STD_CALLS==1. It just will be not so efficient.
Given the OS is not aware of the number of threads, The problem would be the 
same without Xen between. Am I right?
[...]

> > > +
> > > +    /*
> > > +     * Command buffer should start at page boundary.
> > > +     * This is OP-TEE ABI requirement.
> > > +     */
> > > +    if ( call->guest_arg_ipa & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1) )
> > > +        return false;
> > > +
> > > +    call->xen_arg = _xmalloc(OPTEE_MSG_NONCONTIG_PAGE_SIZE,
> > > +                             OPTEE_MSG_NONCONTIG_PAGE_SIZE);
> > > +    if ( !call->xen_arg )
> > > +        return false;
> > > +
> > > +    BUILD_BUG_ON(OPTEE_MSG_NONCONTIG_PAGE_SIZE > PAGE_SIZE);
> > As you use _xmalloc, you should not need this. This is only necessary
> > if you use alloc_xenheap_page.
> Yes, right. This is leftover from time when I mapped guest page into
> XEN. I'll move it down, to place where I map that page.
>
> > I am wondering whether it is wise to allocate the memory from xenheap
> > and not domheap. While on Arm64 (for now) xenheap and domheap are the
> > same, on Arm32 they are different. The xenheap is at most 1GB, so
> > pretty limited.
> Honestly, I have no opinion there. What are limitations of domheap in
> this case?
domheap pages may not always be mapped in Xen page-tables. So you have to call 
map_domain_page/unmap_domain_page at every use.
In practice, on Arm64, those operations are today a NOP because the memory is 
always mapped. On Arm32, domheap is never mapped so those operations will 
require to modify the page-tables.
There would be potentially ways to optimize the Arm32 case. So I think this is 
not a big concern as it would allow to account the memory to the domain and take 
advantage of potential new safety feature around domheap.
BTW, I am not asking to implement the accounting :). You can still allocate 
domheap memory without a domain assigned. I am only giving the advantages of 
using domheap over xenheap :).
> > > +
> > > +    xen_addr = virt_to_maddr(call->xen_arg);
> > > +
> > > +    set_user_reg(regs, 1, xen_addr >> 32);
> > > +    set_user_reg(regs, 2, xen_addr & 0xFFFFFFFF);
> > > +
> > > +    return true;
> > > +}
> > > +
> > > +static void copy_std_request_back(struct optee_domain *ctx,
> > > +                                  struct cpu_user_regs *regs,
> > > +                                  struct optee_std_call *call)
> > Can you add a comment on top of the function explaining what it does?
> Yes, sure.
>
> "Copy OP-TEE response back into guest's request buffer" will be sufficient?
See more below.

> > > +{
> > > +    struct optee_msg_arg *guest_arg;
> > > +    struct page_info *page;
> > > +    unsigned int i;
> > > +    uint32_t attr;
> > > +
> > > +    /* copy_std_request() validated IPA for us */
> > Not really, the guest is free to modify the stage-2 mapping on another
> > vCPU while this is happening. I agree that the guest will shoot
> > himself, but we at least need to not have weird behavior happening.
> >
> > In that case, I would check that the type is p2m_ram_rw as you don't
> > want to write in read-only or foreign mapping.
> How I can do this atomically? I.e. what if guest will mangle p2m right
> after the check?
What you want to prevent is Xen writing to the wrong page. The guest should not 
play with page that are shared with an higher exception level.
get_page_from_gfn() takes a reference on the current page, that will be release 
by put_page(). Between that you are sure the page can not disappear under your feet.
Furthermore, AFAIK, there are no way for an Arm guest to modify the p2m type of 
a page once inserted. It can only remove or replace with a newly allocated page 
the mapping. If the guest instructs to
        - remove the page, as you have a reference that page will not disappear.
	- replace the page with a new one, then the guest will not be able to see the 
result. Tough luck, but it was not meant to do that :).
> > Also, as copy_std_request() and copy_std_request_back may not be
> > called in the same "thread" it would be useful if you specify a bit
> > more the interaction.
> I not sure what do you mean there.
What I meant is the following can happen:

     guest vCPU A         |   Xen
                        
     Initiate call 1
                              copy_std_request()
                              call OP-TEE
                                -> Call "preempted"
                              return to guest
     Resume call 1
                              resume call in OP-TEE
                              copy_std_back_request()

AFAICT, the call could even resume from a different vCPU. It is not entirely 
trivial to understand this from just reading the code and the comment 
"copy_std_request() validated IPA for us" leads to think copy_std_request() was 
called right before. This may not be the case. So can you detail a bit more the 
interaction in the code?
> > > +    page = get_page_from_gfn(current->domain, 
> > > paddr_to_pfn(call->guest_arg_ipa),
> > Please use gfn_x(gaddr_to_gfn(...)) to clarify this is a gfn. The
> > gfn_x will be unnecessary soon with a cleanup that is currently under
> > review.
> So there are chances, that it will be removed when time will come to
> merge this patch series? :)
There is a chance to be merged. Even if it is wasn't, I would prefer to use 
gaddr_to_gfn(..) as your are dealing with a guest physical address. Ideally I 
would like paddr_to_pfn completely disappear in Arm code as the return is not 
typesafe (GFN vs MFN).
[...]

> > > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > > index 175de44927..88b48697bd 100644
> > > --- a/xen/include/asm-arm/domain.h
> > > +++ b/xen/include/asm-arm/domain.h
> > > @@ -97,6 +97,9 @@ struct arch_domain
> > >        struct vpl011 vpl011;
> > >    #endif
> > >    +#ifdef CONFIG_TEE
> > > +    void *tee;
> > > +#endif
> > Did you look whether there are any hole in arch_domain that could be re-used?
> I thought about that. But what are chances to find 64bit-wide,
> 64bit-aligned hole in a structure? If I remember C standard correctly,
> there are no reasons for compiler to leave such holes.
It depends on the alignment requested for each structure. Have a look at pahole 
to see the number (and size) of holes we have in some structures ;).
Anyway, I can't see anything promising in p2m_domain so far.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel