Re: [Xen-devel] [RFC PATCH 4/9] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Oleksandr,
Hi, Julien

>
> On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> Update IOMMU mapping if the IOMMU doesn't share page table with the CPU.
>> The best place to do so on ARM is p2m_set_entry(). Use mfn as an indicator
>> of the required action. If mfn is valid call iommu_map_pages(),
>> otherwise - iommu_unmap_pages().
>
>
> Can you explain in the commit message why you do this change in
> p2m_set_entry and not __p2m_set_entry?
ok

>
>
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> ---
>>  xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 1fc6ca3..84d3a09 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>>      if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
>>          p2m_free_entry(p2m, orig_pte, level);
>>
>> -    if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) ||
>> p2m_valid(*entry)) )
>> +    if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) &&
>> +         (p2m_valid(orig_pte) || p2m_valid(*entry)) )
>>          rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL <<
>> page_order);
>>      else
>>          rc = 0;
>> @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m,
>>                    p2m_type_t t,
>>                    p2m_access_t a)
>>  {
>> +    unsigned long orig_nr = nr;
>> +    gfn_t orig_sgfn = sgfn;
>> +    mfn_t orig_smfn = smfn;
>>      int rc = 0;
>>
>>      while ( nr )
>> @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m,
>>          nr -= (1 << order);
>>      }
>>
>> +    if ( likely(!rc) )
>
>
> I would do
>
> if ( unlikely(rc) )
>   return rc;
>
> /* IOMMU map/unmap ... */
>
> This would remove one indentation layer of if.
Agree.

>
>> +    {
>> +        /*
>> +         * It's time to update IOMMU mapping if the latter doesn't
>> +         * share page table with the CPU. Pass the whole memory block to
>> let
>> +         * the IOMMU code decide what to do with it.
>> +         * The reason to update IOMMU mapping outside "while loop" is
>> that
>> +         * the IOMMU might not support the pages/superpages the CPU can
>> deal
>> +         * with (4K, 2M, 1G) and as result this will lead to non-optimal
>> +         * mapping.
>
>
> My worry with this solution is someone may decide to use __p2m_set_entry
> (see relinquish) directly because he knows the correct order. So the IOMMU
> would be correctly handled when page table are shared and not when they are
> "unshared".
As for relinquish_p2m_mapping(), I was thinking about it, but I
decided not to remove IOMMU mapping here since
the whole IOMMU page table would be removed during complete_domain_destroy().
But, I agree that nothing prevent someone to use __p2m_set_entry
directly in future.

>
> I would probably extend __p2m_get_entry with a new parameter indication
> whether we want to map the page in the IOMMU directly or not.
Sorry, I didn't get your point here. Did you mean __p2m_set_entry?

>
> Also, do you expect the IOMMU to support a page granularity bigger than the
> one supported by Xen? If not, we should probably have a check somewhere, to
> prevent potential security issue as we would map more than expected.
At the moment I keep in mind IPMMU only. And it supports the same page
granularity as the CPU
(4K, 2M, 1G). Could you, please, explain what a proposed check should check?
With the current solution we pass the whole memory block to the IOMMU
code and the IOMMU driver should handle this properly.
If we pass, for example 1,1 GB, but the IOMMU driver supports 4K pages
only then it has to split the memory block into 4K pages and process
them.

>
>
>> +         * Also we assume that the IOMMU mapping should be updated only
>> +         * if CPU mapping passed successfully.
>> +         */
>> +        if ( need_iommu(p2m->domain) && !iommu_use_hap_pt(p2m->domain) )
>> +        {
>> +            if ( !mfn_eq(orig_smfn, INVALID_MFN) )
>> +            {
>> +                unsigned int flags = p2m_get_iommu_flags(t);
>> +
>> +                rc = iommu_map_pages(p2m->domain,
>> +                                     gfn_x(orig_sgfn),
>> +                                     mfn_x(orig_smfn),
>> +                                     orig_nr,
>> +                                     flags);
>> +            }
>> +            else
>> +            {
>> +                rc = iommu_unmap_pages(p2m->domain,
>> +                                       gfn_x(orig_sgfn),
>> +                                       orig_nr);
>> +            }
>> +        }
>> +    }
>> +
>>      return rc;
>>  }
>>
>>
>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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