Re: [Xen-devel] [PATCH v3] IOMMU/x86: make page type checks consistent when mapping pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.09.2019 16:08, Roger Pau Monné  wrote:
> On Fri, Sep 06, 2019 at 02:08:09PM +0200, Jan Beulich wrote:
>> On 06.09.2019 13:45, Roger Pau Monné  wrote:
>>> On Fri, Sep 06, 2019 at 12:52:11PM +0200, Jan Beulich wrote:
>>>> On 06.09.2019 11:37, Roger Pau Monné  wrote:
>>>>> On Wed, Jul 03, 2019 at 12:18:45PM +0000, Jan Beulich wrote:
>>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>>> @@ -829,13 +829,13 @@ guest_physmap_add_page(struct domain *d,
>>>>>>            *
>>>>>>            * Retain this property by grabbing a writable type ref and 
>>>>>> then
>>>>>>            * dropping it immediately.  The result will be pages that 
>>>>>> have a
>>>>>> -         * writable type (and an IOMMU entry), but a count of 0 (such 
>>>>>> that
>>>>>> -         * any guest-requested type changes succeed and remove the IOMMU
>>>>>> -         * entry).
>>>>>> +         * writable type (and an IOMMU entry if necessary), but a count 
>>>>>> of 0
>>>>>> +         * (such that any guest-requested type changes succeed and 
>>>>>> remove the
>>>>>> +         * IOMMU entry).
>>>>>>            */
>>>>>>           for ( i = 0; i < (1UL << page_order); ++i, ++page )
>>>>>>           {
>>>>>> -            if ( !need_iommu_pt_sync(d) )
>>>>>> +            if ( !iommu_enabled )
>>>>>
>>>>> That's kind of a strong check for a domain that might never use the
>>>>> iommu at all. Isn't it fine to just rely on
>>>>> arch_iommu_populate_page_table finding non-writable pages and thus not
>>>>> adding them to the iommu page-tables?
>>>>
>>>> No - the code change here is to take care of page additions to
>>>> the domain after it has booted.
>>>
>>> Please bear with me, but AFAICT arch_iommu_populate_page_table could
>>> be used after the domain has booted if a pci device is hot plugged.
>>>
>>> If this is to deal with additions to domains having an iommu already
>>> enabled, isn't it enough to use need_iommu_pt_sync?
>>>
>>> That's going to return true for all PV domains, except for dom0 not
>>> running in strict mode, which is expected because in that case dom0
>>> already has the whole RAM mapped into the iommu page-tables?
>>
>> Well, my previous reply wasn't precise enough, I guess. The change
>> really is about both cases: If the domain is already using an IOMMU,
>> need_iommu_pt_sync() would be enough indeed. If IOMMU use _may_ be
>> enabled later on, we need to transition pages out of their initial
>> PGT_none state right away. If we deferred this until the point
>> where the IOMMU gets enabled for the domain, we'd have to watch out
>> for PGT_none pages there, which would be extra hassle.
> 
> I still think a relaxed PV dom0 should avoid the overhead of
> get_page_and_type, and the iommu flush done afterwards, as it already
> has all host RAM into it's iommu page-tables.
> 
> Ie: I think the check should be something like:
> 
> if ( !iommu_enabled ||
>      (is_hardware_domain(d) && !need_iommu_pt_sync(d) )

Ah, yes, I can certainly do this (if the patch doesn't become
unnecessary anyway).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel