Re: [XEN PATCH v8 12/22] xen/arm: ffa: support mapping guest RX/TX buffers

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Julien,

On Thu, Apr 13, 2023 at 10:31 PM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Jens,
>
> On 13/04/2023 08:14, Jens Wiklander wrote:
> > +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) )
> > +    {
> > +        /*
> > +         * Calls using the 32-bit calling convention must ignore the upper
> > +         * 32 bits in the argument registers.
> > +         */
> > +        tx_addr &= UINT32_MAX;
> > +        rx_addr &= UINT32_MAX;
> > +    }
> > +
> > +    if ( page_count > FFA_MAX_RXTX_PAGE_COUNT ) {
>
> Coding style:

OK

>
> if ( ... )
> {
>
> > +        printk(XENLOG_ERR "ffa: RXTX_MAP: error: %u pages requested (limit 
> > %u)\n",
> > +               page_count, FFA_MAX_RXTX_PAGE_COUNT);
> > +        return FFA_RET_NOT_SUPPORTED;
> > +    }
> > +
> > +    /* 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);
>
> I might be missing something. Here you only get the reference on one
> page. Per the value of FFA_MAX_RXTX_PAGE_COUNT, it looks like the buffer
> can be up to 32 pages.
>
> Can you clarify?

Good catch. I'll reduce FFA_MAX_RXTX_PAGE_COUNT to 1 since that's what
I've been testing with. I'll add a TODO for supporting a larger
number.

>
> > +    if ( !tx_pg )
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +    /* Only normal RAM for now */
> > +    if ( !p2m_is_ram(t) )
>
> p2m_is_ram() would allow RAM page marked read-only in stage-2. Is it
> intended?
>
> If not, then I think you want to use t != p2m_ram_rw.

Thanks, I'll update it.

>
> > +        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) )
>
> Same here.

OK

Thanks,
Jens

>
> > +        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 = page_count;
> > +    ctx->tx_is_free = true;
> > +    return FFA_RET_OK;
>
> Cheers,
>
> --
> Julien Grall