Re: [Xen-devel] [PATCH v02 2/7] arm: omap: introduce iommu translation for IPU remoteproc

[Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>]

Hi Julien,


On Fri, Jul 4, 2014 at 5:30 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Andrii,
>
>
> On 26/06/14 12:07, Andrii Tseglytskyi wrote:
>>
>> +static u32 mmu_pte_table_alloc(struct mmu_info *mmu, u32 pgd, u32
>> sect_num,
>> +                               struct mmu_pagetable *pgt, u32 hyp_addr)
>> +{
>> +    u32 *pte = NULL;
>> +    u32 i;
>> +
>> +    /* allocate pte table once */
>> +    if ( 0 == hyp_addr )
>> +    {
>> +        pte = xzalloc_bytes(PAGE_SIZE);
>> +        if ( !pte )
>> +        {
>> +            pr_mmu("failed to alloc 2nd level table");
>> +            return 0;
>> +        }
>> +    }
>> +    else
>> +    {
>> +        pte = __va(hyp_addr & MMU_SECTION_MASK(mmu->pg_data->pte_shift));
>> +    }
>> +
>> +    ASSERT(256 == MMU_PTRS_PER_PTE(mmu));
>> +
>> +    for ( i = 0; i < MMU_PTRS_PER_PTE(mmu); i++ )
>> +    {
>> +        u32 paddr, maddr;
>> +
>> +        paddr = pgd + (i * PAGE_SIZE);
>> +        maddr = p2m_lookup(current->domain, paddr, NULL);
>> +        ASSERT(maddr != INVALID_PADDR);
>
>
> Hmmm, what prevents the guest to pass an invalid IPA? On debug build you
> will hit the assert, but on non-debug build you will screw the IPU page
> table.
>

Agree. Will replace ASSERTS with proper if - else logic.

> [..]
>
>> +static u32 mmu_ipu_translate_pagetable(struct mmu_info *mmu, struct
>> mmu_pagetable *pgt)
>> +{
>
>
> [..]
>
>
>> +        /* first level pointers have different formats, depending on
>> their type */
>> +        if ( ipu_pgd_is_super(pgd) )
>> +            pd_mask = MMU_SECTION_MASK(MMU_OMAP_SUPER_SHIFT);
>> +        else if ( ipu_pgd_is_section(pgd) )
>> +            pd_mask = MMU_SECTION_MASK(MMU_OMAP_SECTION_SHIFT);
>> +        else if ( ipu_pgd_is_table(pgd) )
>> +            pd_mask = MMU_SECTION_MASK(MMU_OMAP_SECOND_LEVEL_SHIFT);
>> +
>> +        pd_paddr = pgd & pd_mask;
>> +        pd_flags = pgd & ~pd_mask;
>> +        pd_maddr = p2m_lookup(current->domain, pd_paddr, NULL);
>> +        ASSERT(pd_maddr != INVALID_PADDR);
>
>
> Same remark here.
>

OK

> --
> Julien Grall
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



-- 

Andrii Tseglytskyi | Embedded Dev
GlobalLogic
www.globallogic.com

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel