Re: [PATCH v2 07/18] IOMMU/x86: perform PV Dom0 mappings in batches

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Dec 10, 2021 at 12:41:31PM +0100, Jan Beulich wrote:
> On 10.12.2021 10:36, Roger Pau Monné wrote:
> > On Fri, Dec 03, 2021 at 01:38:48PM +0100, Jan Beulich wrote:
> >> On 02.12.2021 15:10, Roger Pau Monné wrote:
> >>> On Fri, Sep 24, 2021 at 11:47:41AM +0200, Jan Beulich wrote:
> >>>> @@ -720,6 +795,17 @@ int __init dom0_construct_pv(struct doma
> >>>>      /* Pages that are part of page tables must be read only. */
> >>>>      mark_pv_pt_pages_rdonly(d, l4start, vpt_start, nr_pt_pages);
> >>>>  
> >>>> +    /*
> >>>> +     * This needs to come after all potentially excess
> >>>> +     * get_page_and_type(..., PGT_writable_page) invocations; see the loop a
> >>>> +     * few lines further up, where the effect of calling that function in an
> >>>> +     * earlier loop iteration may get overwritten by a later one.
> >>>> +     */
> >>>> +    if ( need_iommu_pt_sync(d) &&
> >>>> +         iommu_unmap(d, _dfn(PFN_DOWN(mpt_alloc) - nr_pt_pages), nr_pt_pages,
> >>>> +                     &flush_flags) )
> >>>> +        BUG();
> >>>
> >>> Wouldn't such unmap better happen as part of changing the types of the
> >>> pages that become part of the guest page tables?
> >>
> >> Not sure - it's a single call here, but would be a call per page when
> >> e.g. moved into mark_pv_pt_pages_rdonly().
> > 
> > I see. So this would result in multiple calls when moved, plus all the
> > involved page shattering and aggregation logic. Overall it would be
> > less error prone, as the iommu unmap would happen next to the type
> > change, but I'm not going to insist if you think it's not worth it.
> > The page table structure pages shouldn't be that many anyway?
> 
> Typically it wouldn't be that many, true. I'm not sure about "less
> error prone", though: We'd have more problems if the range unmapped
> here wasn't properly representing the set of page tables used.

I have to admit I'm biased regarding the PV dom0 building code because
I find it utterly hard to follow, so IMO pairing the unmap call with
the code that marks the pages as read-only seemed less error prone and
less likely to go out of sync with regards to future changes.

That said, if you still feel it's better to do it in a block here I
won't argue anymore.

> >>>> @@ -372,16 +372,30 @@ void __hwdom_init arch_iommu_hwdom_init(
> >>>>                                          perms & IOMMUF_writable ? p2m_access_rw
> >>>>                                                                  : p2m_access_r,
> >>>>                                          0);
> >>>> +        else if ( pfn != start + count || perms != start_perms )
> >>>> +        {
> >>>> +        commit:
> >>>> +            rc = iommu_map(d, _dfn(start), _mfn(start), count,
> >>>> +                           start_perms, &flush_flags);
> >>>> +            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;
> >>>> +        }
> >>>>  
> >>>>          if ( rc )
> >>>>              printk(XENLOG_WARNING "%pd: identity %smapping of %lx failed: %d\n",
> >>>>                     d, !paging_mode_translate(d) ? "IOMMU " : "", pfn, rc);
> >>>
> >>> Would be nice to print the count (or end pfn) in case it's a range.
> >>
> >> I can do so if you think it's worth further extra code. I can't use
> >> "count" here in particular, as that was updated already (in context
> >> above). The most reasonable change towards this would perhaps be to
> >> duplicate the printk() into both the "if()" and the "else if()" bodies.
> > 
> > Maybe. The current message gives the impression that a single pfn has
> > been added and failed, but without printing the range that failed the
> > message will not be that helpful in diagnosing further issues that
> > might arise due to the mapping failure.
> 
> I guess I'll make the change then. I'm still not really convinced though,
> as the presence of the message should be far more concerning than whether
> it's a single page or a range. As middle ground, would
> 
>              printk(XENLOG_WARNING "%pd: identity %smapping of %lx... failed: %d\n",
> 
> be indicative enough of this perhaps not having been just a single page?

Let's go with that last suggestion then.

I would like to attempt to simplify part of the logic here, at which
point it might be easier to print a unified message for both the
translated and non-translated guests.

Thanks, Roger.