Re: [Xen-devel] [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.04.14 at 16:40, <roger.pau@xxxxxxxxxx> wrote:
> On 07/04/14 14:04, Jan Beulich wrote:
>>>>> On 07.04.14 at 13:51, <roger.pau@xxxxxxxxxx> wrote:
>>> On 04/04/14 17:35, Jan Beulich wrote:
>>>>>>> On 04.04.14 at 16:37, <roger.pau@xxxxxxxxxx> wrote:
>>>>> --- a/xen/common/grant_table.c
>>>>> +++ b/xen/common/grant_table.c
>>>>> @@ -749,6 +749,22 @@ __gnttab_map_grant_ref(
>>>>>              goto undo_out;
>>>>>          }
>>>>>      }
>>>>> +    else if ( need_iommu(ld) )
>>>>> +    {
>>>>> +        int err;
>>>>> +
>>>>> +        BUG_ON(!paging_mode_translate(ld));
>>>>> +        err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame,
>>>>> +                             op->flags & GNTMAP_readonly ?
>>>>> +                                IOMMUF_readable :
>>>>> +                                IOMMUF_readable|IOMMUF_writable);
>>>>> +        if ( err )
>>>>> +        {
>>>>> +            double_gt_unlock(lgt, rgt);
>>>>> +            rc = GNTST_general_error;
>>>>> +            goto undo_out;
>>>>> +        }
>>>>> +    }
>>>>
>>>> As much of this as possible should be folded with the if() branch.
>>>> And looking at the PV code, I think it makes no sense for the
>>>> conditions whether to map the page r/o or r/w to be different
>>>> between PV and non-PV.
>>>>
>>>> Plus - wouldn't it be better to have this taken care of via
>>>> create_grant_host_mapping(), by not blindly calling
>>>> iommu_unmap_page() on anything other than p2m_ram_rw in
>>>> ept_set_entry() and p2m_set_entry(), the more that this should
>>>> already be taken care of in the iommu_hap_pt_share case.
>>>
>>> Thanks for the comment, it indeed makes much more sense to fix this in 
>>> ept_set_entry/p2m_set_entry, the following patch adds IOMMU support for 
>>> all page types except p2m_mmio_direct (when the hap memory map is not 
>>> shared with IOMMU):
>> 
>> This seems to be going a little too far - I'm not sure we want to include
>> all types here: p2m_mmio_dm, p2m_ram_paging_*, p2m_ram_paged,
>> and p2m_ram_broken all might require different treatment. Please do
>> this via some sort of switch() setting the permissions instead, calling
>> iommu_unmap_page() when the permissions remain zero.
> 
> Right, I was looking at the wrong p2m.h header (the ARM one), which has 
> a more limited set of p2m types.
> 
> I'm not sure if the type p2m_ram_shared should be given an IOMMU 
> read-only entry, that's what I've done in the patch below.

I'm not sure about the right behavior there too, hence copying Tim
and Andres. However, I'm told page sharing doesn't work when an
IOMMU is in use by a domain anyway, so not handling this case
properly wouldn't have any bad consequences for now.

> Also, there's a comment on the top of p2m.h that worries me:
> 
> /*
>  * AMD IOMMU: When we share p2m table with iommu, bit 52 -bit 58 in pte 
>  * cannot be non-zero, otherwise, hardware generates io page faults when 
>  * device access those pages. Therefore, p2m_ram_rw has to be defined as 0.
>  */
> 
> Does this mean that on AMD if the map is shared between HAP and IOMMU, 
> the only page type accessible from the IOMMU would be p2m_ram_rw? If 

Yes, that does mean exactly that.

> so, that should be fixed also (probably by setting iommu_hap_pt_share = 
> 0 on AMD hardware).

We're considering to drop the sharing altogether anyway, and are
anxiously awaiting Intel to fix their VT-d code for this to not have an
otherwise expected performance impact.

Jan


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