Re: [PATCH v4 06/21] IOMMU/x86: perform PV Dom0 mappings in batches

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.05.2022 13:20, Roger Pau Monné wrote:
> On Wed, May 04, 2022 at 11:46:37AM +0200, Jan Beulich wrote:
>> On 03.05.2022 16:49, Roger Pau Monné wrote:
>>> On Mon, Apr 25, 2022 at 10:34:59AM +0200, Jan Beulich wrote:
>>>> For large page mappings to be easily usable (i.e. in particular without
>>>> un-shattering of smaller page mappings) and for mapping operations to
>>>> then also be more efficient, pass batches of Dom0 memory to iommu_map().
>>>> In dom0_construct_pv() and its helpers (covering strict mode) this
>>>> additionally requires establishing the type of those pages (albeit with
>>>> zero type references).
>>>
>>> I think it's possible I've already asked this.  Would it make sense to
>>> add the IOMMU mappings in alloc_domheap_pages(), maybe by passing a
>>> specific flag?
>>
>> I don't think you did ask, but now that you do: This would look like a
>> layering violation to me. I don't think allocation should ever have
>> mapping (into the IOMMU or elsewhere) as a "side effect", no matter
>> that ...
> 
> Hm, I'm certainly not that familiar with PV itself to likely be able
> to make a proper argument here.  I fully agree with you for translated
> guests using a p2m.
> 
> For PV we currently establish/teardown IOMMU mappings in
> _get_page_type(), which already looks like a layering violation to me,
> hence also doing so in alloc_domheap_pages() wouldn't seem that bad if
> it allows to simplify the resulting code overall.

That's a layering violation too, I agree, but at least it's one central
place.

>>> It would seem to me that doing it that way would also allow the
>>> mappings to get established in blocks for domUs.
>>
>> ... then this would perhaps be possible.
>>
>>>> The installing of zero-ref writable types has in fact shown (observed
>>>> while putting together the change) that despite the intention by the
>>>> XSA-288 changes (affecting DomU-s only) for Dom0 a number of
>>>> sufficiently ordinary pages (at the very least initrd and P2M ones as
>>>> well as pages that are part of the initial allocation but not part of
>>>> the initial mapping) still have been starting out as PGT_none, meaning
>>>> that they would have gained IOMMU mappings only the first time these
>>>> pages would get mapped writably. Consequently an open question is
>>>> whether iommu_memory_setup() should set the pages to PGT_writable_page
>>>> independent of need_iommu_pt_sync().
>>>
>>> I think I'm confused, doesn't the setting of PGT_writable_page happen
>>> as a result of need_iommu_pt_sync() and having those pages added to
>>> the IOMMU page tables? (so they can be properly tracked and IOMMU
>>> mappings are removed if thte page is also removed)
>>
>> In principle yes - in guest_physmap_add_page(). But this function isn't
>> called for the pages I did enumerate in the remark. XSA-288 really only
>> cared about getting this right for DomU-s.
> 
> Would it make sense to change guest_physmap_add_page() to be able to
> pass the page_order parameter down to iommu_map(), and then use it for
> dom0 build instead of introducing iommu_memory_setup()?

To be quite frank: This is something that I might have been willing to
do months ago, when this series was still fresh. If I was to start
re-doing all of this code now, it would take far more time than it
would have taken back then. Hence I'd like to avoid a full re-work here
unless entirely unacceptable in the way currently done (which largely
fits with how we have been doing Dom0 setup).

Furthermore, guest_physmap_add_page() doesn't itself call iommu_map().
What you're suggesting would require get_page_and_type() to be able to
work on higher-order pages. I view adjustments like this as well out
of scope for this series.

> I think guest_physmap_add_page() will need to be adjusted at some
> point for domUs, and hence it could be unified with dom0 usage
> also?

As an optimization - perhaps. I view it as more important to have HVM
guests work reasonably well (which includes the performance aspect of
setting them up).

>>> If the pages are not added here (because dom0 is not running in strict
>>> mode) then setting PGT_writable_page is not required?
>>
>> Correct - in that case we skip fiddling with IOMMU mappings on
>> transitions to/from PGT_writable_page, and hence putting this type in
>> place would be benign (but improve consistency).
>>
>>>> Note also that strictly speaking the iommu_iotlb_flush_all() here (as
>>>> well as the pre-existing one in arch_iommu_hwdom_init()) shouldn't be
>>>> needed: Actual hooking up (AMD) or enabling of translation (VT-d)
>>>> occurs only afterwards anyway, so nothing can have made it into TLBs
>>>> just yet.
>>>
>>> Hm, indeed. I think the one in arch_iommu_hwdom_init can surely go
>>> away, as we must strictly do the hwdom init before enabling the iommu
>>> itself.
>>
>> Why would that be? That's imo as much of an implementation detail as
>> ...
> 
> Well, you want to have the reserved/inclusive options applied (and
> mappings created) before enabling the IOMMU, because at that point
> devices have already been assigned.  So it depends more on a
> combination of devices assigned & IOMMU enabled rather than just IOMMU
> being enabled.
> 
>>> The one in dom0 build I'm less convinced, just to be on the safe side
>>> if we ever change the order of IOMMU init and memory setup.
>>
>> ... this. Just like we populate tables with the IOMMU already enabled
>> for DomU-s, I think the same would be valid to do for Dom0.
>>
>>> I would expect flushing an empty TLB to not be very expensive?
>>
>> I wouldn't "expect" this - it might be this way, but it surely depends
>> on whether an implementation can easily tell whether the TLB is empty,
>> and whether its emptiness actually makes a difference for the latency
>> of a flush operation.
>>
>>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>>> @@ -347,8 +347,8 @@ static unsigned int __hwdom_init hwdom_i
>>>>  
>>>>  void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>>>>  {
>>>> -    unsigned long i, top, max_pfn;
>>>> -    unsigned int flush_flags = 0;
>>>> +    unsigned long i, top, max_pfn, start, count;
>>>> +    unsigned int flush_flags = 0, start_perms = 0;
>>>>  
>>>>      BUG_ON(!is_hardware_domain(d));
>>>>  
>>>> @@ -379,9 +379,9 @@ void __hwdom_init arch_iommu_hwdom_init(
>>>>       * First Mb will get mapped in one go by pvh_populate_p2m(). Avoid
>>>>       * setting up potentially conflicting mappings here.
>>>>       */
>>>> -    i = paging_mode_translate(d) ? PFN_DOWN(MB(1)) : 0;
>>>> +    start = paging_mode_translate(d) ? PFN_DOWN(MB(1)) : 0;
>>>>  
>>>> -    for ( ; i < top; i++ )
>>>> +    for ( i = start, count = 0; i < top; )
>>>>      {
>>>>          unsigned long pfn = pdx_to_pfn(i);
>>>>          unsigned int perms = hwdom_iommu_map(d, pfn, max_pfn);
>>>> @@ -390,20 +390,41 @@ void __hwdom_init arch_iommu_hwdom_init(
>>>>          if ( !perms )
>>>>              rc = 0;
>>>>          else if ( paging_mode_translate(d) )
>>>> +        {
>>>>              rc = p2m_add_identity_entry(d, pfn,
>>>>                                          perms & IOMMUF_writable ? 
>>>> p2m_access_rw
>>>>                                                                  : 
>>>> p2m_access_r,
>>>>                                          0);
>>>> +            if ( rc )
>>>> +                printk(XENLOG_WARNING
>>>> +                       "%pd: identity mapping of %lx failed: %d\n",
>>>> +                       d, pfn, rc);
>>>> +        }
>>>> +        else if ( pfn != start + count || perms != start_perms )
>>>> +        {
>>>> +        commit:
>>>> +            rc = iommu_map(d, _dfn(start), _mfn(start), count, 
>>>> start_perms,
>>>> +                           &flush_flags);
>>>> +            if ( rc )
>>>> +                printk(XENLOG_WARNING
>>>> +                       "%pd: IOMMU identity mapping of [%lx,%lx) failed: 
>>>> %d\n",
>>>> +                       d, pfn, pfn + count, rc);
>>>> +            SWAP(start, pfn);
>>>> +            start_perms = perms;
>>>> +            count = 1;
>>>> +        }
>>>>          else
>>>> -            rc = iommu_map(d, _dfn(pfn), _mfn(pfn), 1ul << PAGE_ORDER_4K,
>>>> -                           perms, &flush_flags);
>>>> +        {
>>>> +            ++count;
>>>> +            rc = 0;
>>>
>>> Seeing as we want to process this in blocks now, I wonder whether it
>>> would make sense to take a different approach, and use a rangeset to
>>> track which regions need to be mapped.  What gets added would be based
>>> on the host e820 plus the options
>>> iommu_hwdom_{strict,inclusive,reserved}.  We would then punch holes
>>> based on the logic in hwdom_iommu_map() and finally we could iterate
>>> over the regions afterwards using rangeset_consume_ranges().
>>>
>>> Not that you strictly need to do it here, just think the end result
>>> would be clearer.
>>
>> The end result might indeed be, but it would be more of a change right
>> here. Hence I'd prefer to leave that out of the series for now.
> 
> OK.  I think it might be nice to add a comment in that regard, mostly
> because I tend to forget myself.

Sure, I've added another post-commit-message remark.

Jan