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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>>> +                    {
>>>> +                        put_page_and_type(page);
>>>> +                        flush_flags |= IOMMUF_readable | IOMMUF_writable;
>>>> +                    }
>>>> +                    else if ( !rc )
>>>> +                        rc = -EBUSY;
>>>
>>> Is it fine to return an error here? AFAICT you could have a RO page
>>> shared with Xen with PGT_none, and not having an iommu mapping for it
>>> would be expected, hence returning an error seems wrong?
>>
>> No, pages shared with Xen don't live on d->page_list (which is what the
>> loop iterates over).
> 
> So then there should be no PGT_none pages in d->page_list?
> 
> The only user I can find of PGT_none is share_xen_page_with_guest.

Plus the implicit use when a page gets first added to a domain (by
setting ->u.inuse.type_info to zero).

Jan

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