Re: [XEN PATCH v7 13/20] xen/arm: ffa: support mapping guest RX/TX buffers

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 3 Mar 2023, at 08:41, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Thu, Mar 2, 2023 at 4:05 PM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> Hi Jens,
>> 
>>> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
>>> 
>>> Adds support in the mediator to map and unmap the RX and TX buffers
>>> provided by the guest using the two FF-A functions FFA_RXTX_MAP and
>>> FFA_RXTX_UNMAP.
>>> 
>>> These buffer are later used to to transmit data that cannot be passed in
>>> registers only.
>>> 
>>> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
>>> ---
>>> xen/arch/arm/tee/ffa.c | 127 +++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 127 insertions(+)
>>> 
>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>>> index f1b014b6c7f4..953b6dfd5eca 100644
>>> --- a/xen/arch/arm/tee/ffa.c
>>> +++ b/xen/arch/arm/tee/ffa.c
>>> @@ -149,10 +149,17 @@ struct ffa_partition_info_1_1 {
>>> };
>>> 
>>> struct ffa_ctx {
>>> +    void *rx;
>>> +    const void *tx;
>>> +    struct page_info *rx_pg;
>>> +    struct page_info *tx_pg;
>>> +    unsigned int page_count;
>>>    uint32_t guest_vers;
>>> +    bool tx_is_mine;
>>>    bool interrupted;
>>> };
>>> 
>>> +
>> Newline probably added by mistake.
> 
> Yes, I'll remove it.
> 
>> 
>>> /* Negotiated FF-A version to use with the SPMC */
>>> static uint32_t ffa_version __ro_after_init;
>>> 
>>> @@ -337,6 +344,11 @@ static void set_regs(struct cpu_user_regs *regs, 
>>> register_t v0, register_t v1,
>>>        set_user_reg(regs, 7, v7);
>>> }
>>> 
>>> +static void set_regs_error(struct cpu_user_regs *regs, uint32_t error_code)
>>> +{
>>> +    set_regs(regs, FFA_ERROR, 0, error_code, 0, 0, 0, 0, 0);
>>> +}
>>> +
>>> static void set_regs_success(struct cpu_user_regs *regs, uint32_t w2,
>>>                             uint32_t w3)
>>> {
>>> @@ -358,6 +370,99 @@ static void handle_version(struct cpu_user_regs *regs)
>>>    set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
>>> }
>>> 
>>> +static uint32_t handle_rxtx_map(uint32_t fid, register_t tx_addr,
>>> +                                register_t rx_addr, uint32_t page_count)
>>> +{
>>> +    uint32_t ret = FFA_RET_INVALID_PARAMETERS;
>>> +    struct domain *d = current->domain;
>>> +    struct ffa_ctx *ctx = d->arch.tee;
>>> +    struct page_info *tx_pg;
>>> +    struct page_info *rx_pg;
>>> +    p2m_type_t t;
>>> +    void *rx;
>>> +    void *tx;
>>> +
>>> +    if ( !smccc_is_conv_64(fid) )
>>> +    {
>>> +        tx_addr &= UINT32_MAX;
>>> +        rx_addr &= UINT32_MAX;
>>> +    }
>> 
>> I am bit wondering here what we should do:
>> - we could just say that 32bit version of the call is not allowed for non 
>> 32bit guests
>> - we could check that the highest bits are 0 for 64bit guests and return an 
>> error if not
>> - we can just mask hopping that if there was a mistake the address after the 
>> mask
>> does not exist in the guest space
>> 
>> At the end nothing in the spec is preventing a 64bit guest to use the 32bit 
>> so it might
>> be a good idea to return an error if the highest 32bit are not 0 here ?
> 
> The SMC Calling Convention says:
> When an SMC32/HVC32 call is made from AArch64:
> - A Function Identifier is passed in register W0.
> - Arguments are passed in registers W1-W7.
> 
> So masking off the higher bits is all that should be done.

Please put a comment saying that in 32 bit convention higher bits should be 
ignored.

> 
>> 
>>> +
>>> +    /* For now to keep things simple, only deal with a single page */
>>> +    if ( page_count != 1 )
>>> +        return FFA_RET_NOT_SUPPORTED;
>> 
>> Please add a TODO here and a print as this is a limitation we will probably 
>> have to
>> work on soon.
> 
> I'll add an arbitrary upper limit and a print if it's exceeded.

thanks

> 
>> 
>> 
>>> +
>>> +    /* Already mapped */
>>> +    if ( ctx->rx )
>>> +        return FFA_RET_DENIED;
>>> +
>>> +    tx_pg = get_page_from_gfn(d, gfn_x(gaddr_to_gfn(tx_addr)), &t, 
>>> P2M_ALLOC);
>>> +    if ( !tx_pg )
>>> +        return FFA_RET_INVALID_PARAMETERS;
>>> +    /* Only normal RAM for now */
>>> +    if ( !p2m_is_ram(t) )
>>> +        goto err_put_tx_pg;
>>> +
>>> +    rx_pg = get_page_from_gfn(d, gfn_x(gaddr_to_gfn(rx_addr)), &t, 
>>> P2M_ALLOC);
>>> +    if ( !tx_pg )
>>> +        goto err_put_tx_pg;
>>> +    /* Only normal RAM for now */
>>> +    if ( !p2m_is_ram(t) )
>>> +        goto err_put_rx_pg;
>>> +
>>> +    tx = __map_domain_page_global(tx_pg);
>>> +    if ( !tx )
>>> +        goto err_put_rx_pg;
>>> +
>>> +    rx = __map_domain_page_global(rx_pg);
>>> +    if ( !rx )
>>> +        goto err_unmap_tx;
>>> +
>>> +    ctx->rx = rx;
>>> +    ctx->tx = tx;
>>> +    ctx->rx_pg = rx_pg;
>>> +    ctx->tx_pg = tx_pg;
>>> +    ctx->page_count = 1;
>> 
>> please use page_count here instead of 1 so that this is not forgotten once
>> we add support for more pages.
> 
> OK

Cheers
Bertrand

> 
> Cheers,
> Jens
> 
>> 
>> 
>> Cheers
>> Bertrand
>> 
>>> +    ctx->tx_is_mine = true;
>>> +    return FFA_RET_OK;
>>> +
>>> +err_unmap_tx:
>>> +    unmap_domain_page_global(tx);
>>> +err_put_rx_pg:
>>> +    put_page(rx_pg);
>>> +err_put_tx_pg:
>>> +    put_page(tx_pg);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static void rxtx_unmap(struct ffa_ctx *ctx)
>>> +{
>>> +    unmap_domain_page_global(ctx->rx);
>>> +    unmap_domain_page_global(ctx->tx);
>>> +    put_page(ctx->rx_pg);
>>> +    put_page(ctx->tx_pg);
>>> +    ctx->rx = NULL;
>>> +    ctx->tx = NULL;
>>> +    ctx->rx_pg = NULL;
>>> +    ctx->tx_pg = NULL;
>>> +    ctx->page_count = 0;
>>> +    ctx->tx_is_mine = false;
>>> +}
>>> +
>>> +static uint32_t handle_rxtx_unmap(void)
>>> +{
>>> +    struct domain *d = current->domain;
>>> +    struct ffa_ctx *ctx = d->arch.tee;
>>> +
>>> +    if ( !ctx->rx )
>>> +        return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> +    rxtx_unmap(ctx);
>>> +
>>> +    return FFA_RET_OK;
>>> +}
>>> +
>>> static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t 
>>> fid)
>>> {
>>>    struct arm_smccc_1_2_regs arg = { .a0 = fid, };
>>> @@ -423,6 +528,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>>    uint32_t fid = get_user_reg(regs, 0);
>>>    struct domain *d = current->domain;
>>>    struct ffa_ctx *ctx = d->arch.tee;
>>> +    int e;
>>> 
>>>    if ( !ctx )
>>>        return false;
>>> @@ -435,6 +541,24 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>>    case FFA_ID_GET:
>>>        set_regs_success(regs, get_vm_id(d), 0);
>>>        return true;
>>> +    case FFA_RXTX_MAP_32:
>>> +#ifdef CONFIG_ARM_64
>>> +    case FFA_RXTX_MAP_64:
>>> +#endif
>>> +        e = handle_rxtx_map(fid, get_user_reg(regs, 1), get_user_reg(regs, 
>>> 2),
>>> +                            get_user_reg(regs, 3));
>>> +        if ( e )
>>> +            set_regs_error(regs, e);
>>> +        else
>>> +            set_regs_success(regs, 0, 0);
>>> +        return true;
>>> +    case FFA_RXTX_UNMAP:
>>> +        e = handle_rxtx_unmap();
>>> +        if ( e )
>>> +            set_regs_error(regs, e);
>>> +        else
>>> +            set_regs_success(regs, 0, 0);
>>> +        return true;
>>>    case FFA_MSG_SEND_DIRECT_REQ_32:
>>> #ifdef CONFIG_ARM_64
>>>    case FFA_MSG_SEND_DIRECT_REQ_64:
>>> @@ -515,6 +639,9 @@ static int ffa_relinquish_resources(struct domain *d)
>>>                   get_vm_id(d), subscr_vm_destroyed[n], res);
>>>    }
>>> 
>>> +    if ( ctx->rx )
>>> +        rxtx_unmap(ctx);
>>> +
>>>    XFREE(d->arch.tee);
>>> 
>>>    return 0;
>>> --
>>> 2.34.1