Re: [PATCH v02 1/7] arm: introduce remoteprocessor iommu module

[Julien Grall <julien.grall@linaro.org>]

Hi Andrii,

On 26/06/14 12:07, Andrii Tseglytskyi wrote:
> This is a fisrst patch from patch series which was

s/firsrst/first/

Although I don't think you have to say that this is the first patch :). 
This is not useful for the commit message.

> developed to handle remote (external) processors
> memory management units. Remote processors are
> typically used for graphic rendering (GPUs) and
> high quality video decoding (IPUs). They are typically
> installed on such multimedia SoCs as OMAP4 / OMAP5.
>
> As soon as remoteprocessor MMU typically works with
> pagetables filled by physical addresses, which are
> allocated by domU kernel, it is almost impossible to
> use them under Xen, intermediate physical addresses
> allocated by kernel, need to be translated to machine
> addresses.
>
> This patch introduces a simple framework to perform
> pfn -> mfn translation for external MMUs.
> It introduces basic data structures and algorithms
> needed for translation.
>
> Typically, when MMU is configured, some it registers
> are updated by new values. Introduced frameworks
> uses traps as starting point of remoteproc MMUs
> pagetables translation.
>
> Change-Id: Ia4d311a40289df46a003f5ae8706c150bee1885d
> Signed-off-by: Andrii Tseglytskyi <andrii.tseglytskyi@globallogic.com>

[..]

> +static struct mmu_info *mmu_list[] = {
> +};
> +
> +#define mmu_for_each(pfunc, data)                       \
> +({                                                      \
> +    u32 __i;                                            \
> +    int __res = 0;                                      \
> +                                                        \
> +    for ( __i = 0; __i < ARRAY_SIZE(mmu_list); __i++ )  \
> +    {                                                   \
> +        __res |= pfunc(mmu_list[__i], data);            \

Using _res |= will result to a wrong errno at the end.
See the usage in iommu mmu_init_all.

I would use

__res = pfunc(...)
if ( !__res )
   break;

And therefore modify mmu_check to return 1 if failing, 0 otherwise.

This will also avoid to continue to browse all the MMU for nothing.

> +    }                                                   \
> +    __res;                                              \
> +})
> +
> +static int mmu_check_mem_range(struct mmu_info *mmu, paddr_t addr)
> +{
> +    if ( (addr >= mmu->mem_start) && (addr < (mmu->mem_start + mmu->mem_size)) )
> +        return 1;
> +
> +    return 0;
> +}
> +
> +static inline struct mmu_info *mmu_lookup(u32 addr)
> +{
> +    u32 i;
> +
> +    /* enumerate all registered MMU's and check is address in range */
> +    for ( i = 0; i < ARRAY_SIZE(mmu_list); i++ )
> +    {
> +        if ( mmu_check_mem_range(mmu_list[i], addr) )
> +            return mmu_list[i];
> +    }
> +
> +    return NULL;
> +}
> +
> +static int mmu_mmio_check(struct vcpu *v, paddr_t addr)
> +{
> +    return mmu_for_each(mmu_check_mem_range, addr);
> +}

This solution leads any guest to access to the MMU and therefore program 
it. If you plan to use for passthrough, you have to find a way to say 
that a specific domain is able to use the MMU, maybe an hypercall. 
Otherwise this is a security issue.

IIRC I have already raised this concern on V1 :). It would be nice if 
you resolve it ASAP, because I suspect it will need some rework in the 
way you handle MMNU.

> +static int mmu_copy_pagetable(struct mmu_info *mmu, struct mmu_pagetable *pgt)
> +{
> +    void __iomem *pagetable = NULL;
> +    u32 maddr, i;
> +
> +    ASSERT(mmu);
> +    ASSERT(pgt);
> +
> +    if ( !pgt->paddr )
> +        return -EINVAL;
> +
> +    /* pagetable size can be more than one page */
> +    for ( i = 0; i < MMU_PGD_TABLE_SIZE(mmu) / PAGE_SIZE; i++ )
> +    {
> +        /* lookup address where remoteproc pagetable is stored by kernel */
> +        maddr = p2m_lookup(current->domain, pgt->paddr + i * PAGE_SIZE, NULL);
> +        if ( !maddr )
> +        {
> +            pr_mmu("failed to translate 0x%08lx to maddr", pgt->paddr + i * PAGE_SIZE);
> +            return -EINVAL;
> +        }
> +
> +        pagetable = ioremap_nocache(maddr, MMU_PGD_TABLE_SIZE(mmu));

ioremap_* should only be used to map device memory. For the guest memory 
you have to use copy_from_guest helper.

> +struct mmu_pagetable *mmu_pagetable_lookup(struct mmu_info *mmu, u32 addr, bool is_maddr)

you have to use paddr_t for addr. The number of bit for the physical 
address can be greater than 40 bits.

The remark is the same everywhere within this file.

[..]

> +static u32 mmu_translate_pagetable(struct mmu_info *mmu, u32 paddr)
> +{
> +    struct mmu_pagetable *pgt;
> +    int res;
> +
> +    /* lookup using machine address first */
> +    pgt = mmu_pagetable_lookup(mmu, paddr, true);
> +    if ( !pgt )
> +    {
> +        /* lookup using kernel physical address */
> +        pgt = mmu_pagetable_lookup(mmu, paddr, false);
> +        if ( !pgt )
> +        {
> +            /* if pagetable doesn't exists in lookup list - allocate it */
> +            pgt = mmu_alloc_pagetable(mmu, paddr);

The function mmu_alloc_pagetable can return NULL. But you never check 
the return value and dereference it just below.

> +        }
> +    }
> +
> +    pgt->maddr = MMU_INVALID_ADDRESS;

[..]

> +static int mmu_mmio_read(struct vcpu *v, mmio_info_t *info)
> +{
> +    struct mmu_info *mmu = NULL;
> +    unsigned long flags;
> +    register_t *r;
> +
> +    r = select_user_reg(guest_cpu_user_regs(), info->dabt.reg);
> +
> +    ASSERT(r);

The ASSERT is pointless, select_user_reg will never return NULL.

[..]

> +static int mmu_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{
> +    struct mmu_info *mmu = NULL;
> +    unsigned long flags;
> +    register_t *r;
> +    u32 new_addr, val;
> +
> +    r = select_user_reg(guest_cpu_user_regs(), info->dabt.reg);
> +
> +    ASSERT(r);

Same here.

> +    /* dom0 should not access remoteproc MMU */
> +    if ( 0 == current->domain->domain_id )
> +        return 1;

Why this restriction?

> +    /* find corresponding MMU */
> +    mmu = mmu_lookup(info->gpa);
> +    if ( !mmu )
> +    {
> +        pr_mmu("can't get mmu for addr 0x%08x", (u32)info->gpa);
> +        return -EINVAL;
> +    }
> +
> +    ASSERT(v->domain == current->domain);

I guess this restriction is because you are using current in 
mmu_trap_translate_pagetable?

So, the iohandler is usually call on the current VCPU, there is no need 
to worry about it. Futhermore, I would pass the vcpu/domain in argument 
of the next function.

> +static int mmu_init(struct mmu_info *mmu, u32 data)
> +{
> +    ASSERT(mmu);
> +    ASSERT(!mmu->mem_map);
> +
> +    INIT_LIST_HEAD(&mmu->pagetables_list);
> +
> +    /* map MMU memory */
> +    mmu->mem_map = ioremap_nocache(mmu->mem_start, mmu->mem_size);
> +	if ( !mmu->mem_map )

It looks like there is a hard tab here.

[..]

> +static int mmu_init_all(void)
> +{
> +    int res;
> +
> +    res = mmu_for_each(mmu_init, 0);
> +    if ( res )
> +    {
> +        printk("%s error during init %d\n", __func__, res);
> +        return res;
> +    }

Hmmm... do_initcalls doesn't check the return value. How your code 
behave we one of the MMU has not been initialized?

I think do_initcalls & co should check the return, but as it's the 
common code I don't know how x86 respect this convention to return 0 if 
succeded. Ian, Stefano, any thoughs?

[..]

> diff --git a/xen/include/xen/remoteproc_iommu.h b/xen/include/xen/remoteproc_iommu.h

I think this file as

> new file mode 100644
> index 0000000..22e2951
> --- /dev/null
> +++ b/xen/include/xen/remoteproc_iommu.h

The remoteproc things is ARM specific, right? If so, this header should 
be moved in include/asm-arm.

> +
> +#define MMU_INVALID_ADDRESS ((u32)(-1))

You should not assume that the MMU ADDRESS is always 32 bits. Please use 
paddr_t here.

> +#define pr_mmu(fmt, ...) \
> +	printk("%s: %s: "fmt"\n", __func__, ((mmu) ? (mmu)->name : ""), ##__VA_ARGS__)

Hmmm, you are assuming that mmu is existing within the function. You 
should pass the mmu in parameter of this macro.

Also, most of the usage are for an error (except the one in 
mmu_page_alloc). I would prefix it by XENLOG_ERROR.

> +struct pagetable_data {
> +	/* 1st level translation */
> +	u32 pgd_shift;
> +	u32 pte_shift;
> +	u32 super_shift;
> +	u32 section_shift;
> +	/* 2nd level translation */
> +	u32 pte_large_shift;
> +};

No hard tab in Xen. Please remove them.

> +
> +struct mmu_pagetable {
> +	u32					*hyp_pagetable;
> +	u32					*kern_pagetable;
> +	u32					paddr;
> +	u32					maddr;
> +	struct list_head	link_node;
> +	u32					page_counter;
> +};

Same here.

> +
> +struct mmu_info {
> +	const char				*name;
> +	const struct pagetable_data *pg_data;
> +	/* register where phys pointer to pagetable is stored */
> +	u32					*trap_offsets;
> +	paddr_t				mem_start;
> +	u32					mem_size;
> +	spinlock_t			lock;
> +	struct list_head	pagetables_list;
> +	u32					num_traps;
> +	void __iomem		*mem_map;
> +	u32	(*translate_pfunc)(struct mmu_info *, struct mmu_pagetable *);
> +	void (*print_pagetable_pfunc)(struct mmu_info *);
> +};

Same here.

Regards,

-- 
Julien Grall