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 Fri, Apr 21, 2017 at 7:27 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> On 21/04/17 15:18, Oleksandr Tyshchenko wrote:
>>
>> On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@xxxxxxx>
>> wrote:
>>>
>>> Hi Oleksandr,
>>
>> Hi, Julien
>
>
> 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?
>
>
> Yes sorry.
>
>>
>>>
>>> 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?
>
>
> We should check that the smallest granularity supported by the IOMMU is
> inferior or equal to PAGE_SIZE. If not, then there will be more rework
> required in Xen to support correctly those IOMMUs.
>
> For instance, Xen PAGE_SIZE is currently 4KB. If the IOMMUs only support
> 64KB, then you will end up to map a bigger chunk than expected leading a
> guest device to access memory it should not access.
>
> Supporting such IOMMU will require much more work in Xen than this small
> changes.

Aha. Seems, I understand what you meant. I already have a check in IPMMU driver:

/*
* both the virtual address and the physical one, as well as
* the size of the mapping, must be aligned (at least) to the
* size of the smallest page supported by the hardware
*/
if (!IS_ALIGNED(iova | paddr | size, min_pagesz)) {
printk("unaligned: iova 0x%lx pa 0x%"PRIx64" size 0x%zx min_pagesz 0x%x\n",
      iova, paddr, size, min_pagesz);
return -EINVAL;
}

where min_pagesz - is a minimum page size supported by hardware.
Hope, that this check can catch such case.

>
>
>> 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.
>
>
> The IOMMU driver will likely duplicate the same logic as in p2m_set_entry
> and today does not seem to be necessary. By that I mean splitting into
> smaller chunk.
>
> You may need to split again those chunks if the IOMMU does not support the
> granularity. But this is less likely on current hardware.
>
> My main concern is to make sure __p2m_get_entry works as expected with all
> the configurations. Currently, what you describe will require much more work
> than this series. I will be ok whether you rework __p2m_get_entry or not.
As we have already found common ground:
I will rework the patch by adding updating IOMMU mapping to
__p2m_set_entry and add new argument to it to
to show whether we have to update IOMMU mapping or not.
So, if we call  __p2m_set_entry directly, we have to force it to
update IOMMU mapping.

>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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