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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.05.2022 15:55, Roger Pau Monné wrote:
> On Wed, May 04, 2022 at 02:27:14PM +0200, Jan Beulich wrote:
>> 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:
>>>>>> @@ -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.
> 
> Sorry for being confused, but are those reflected in the final commit
> message, or in the code itself?

Neither - I didn't think we have any code comments anywhere which outline
future plans, including reasons why not doing so right away. When writing
that new remark I didn't even think it would belong in the commit message.

Jan