Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Aug 12, 2019 at 03:24:36PM +0100, George Dunlap wrote:
> On 8/12/19 2:56 PM, Roger Pau Monné wrote:
> > On Mon, Aug 12, 2019 at 02:17:53PM +0100, George Dunlap wrote:
> >> On 8/2/19 10:22 AM, Roger Pau Monne wrote:
> >>> Switch rmrr_identity_mapping to use iommu_{un}map in order to
> >>> establish RMRR mappings for PV domains, like it's done in
> >>> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
> >>> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
> >>> not properly updating the iommu page tables.
> >>
> >> Sorry, I think this description is somewhat backwards: you're saying
> >> what you're doing first, and then saying what the problematic behavior
> >> was, but not actually saying what was causing the problematic behavior.
> >>
> >> Why was {set,clear}_identity_p2m not updating the page tables?
> >>
> >> I agree with Jan, that figuring that out is a prerequisite for any kind
> >> of fix: if `need_iommu_pt_sync()` is false at that point for the
> >> hardware domain, then there's a bigger problem than RMRRs not being
> >> adjusted properly.
> > 
> > need_iommu_pt_sync is indeed false for a PV hardware domain not
> > running in strict mode, see:
> > 
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/passthrough/iommu.c;h=f8c3bf53bd1793df93d7ddea6564dc929f40c156;hb=HEAD#l192
> > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01547.html
> > 
> > This is fine for a non-strict PV hardware domain, since it has all the
> > host memory (minus memory used by Xen) mapped in the iommu page tables
> > except for RMRR regions not marked as reserved in the e820 memory map,
> > which are added using set_identity_p2m_entry.
> > 
> > The issue here is that this patch alone doesn't fix the issue for the
> > reporter, and there seems to be an additional flush required in order
> > to get the issue solved on his end:
> > 
> > https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00526.html
> > 
> > My theory is that the system Roman is using is booting with DMA
> > remapping enabled in the iommu, and somehow the call to
> > iommu_flush_all in intel_iommu_hwdom_init doesn't seem to work
> > properly, while calling iommu_iotlb_flush_all does seem to do the
> > right thing. I'm still waiting for Roman to come back with the result
> > of my last debug patches in order to figure out whether my hypothesis
> > above is true.
> > 
> > This however won't still explain the weird behaviour of
> > iommu_flush_all, and further debugging will still be required.
> 
> OK; so this patch has four effects, it looks like:
> 
> 1. iommu_legacy_[un]map -> iommu_[un]map
> 2. Move iommu ops out of {set,clear}_identity_p2m for PV guests;
> open-code them in rmrr_identity_mapping
> 3. For non-translated guests, do the operation unconditionally
> 4. Add a flush

There's already a flush in iommu_legacy_[un]map, so the flush is also
done for both patches, just in different places. The legacy interface
does the flush on every call, while the new interface allows to
postpone the flush until all iommu page-table operations are done.

> 
> Regarding #3, the previous patch changed it from "if
> iommu_needs_pt_sync" to "if has_iommu_pt"; this one changes it to
> "always".  Is that what you intended?

Well, #3 is not actually correct. The code in intel_iommu_hwdom_init
and hence setup_hwdom_rmrr will only be called if the domain has an
iommu, so has_iommu_pt will always be true when adding RMRR regions.
Note rmrr_identity_mapping is the only caller of
{set,clear}_identity_p2m against PV guests.

> I don't really see the point of #2: from the device's perspective, the
> "physmap" is the IOMMU for PV guests, and IOMMU+p2m for HVM guests; it
> makes sense to have a single place to call for either type of guest,
> rather than open-coding every location.

OK, that's fine, but note that {set/clear}_identity_p2m_entry is
AFAICT the only p2m function allowed against PV guests, the rest will
return some kind of error, which IMO makes it the outlier, hence my
proposal to make it available to translated guests only, like the rest
of the p2m functions in that file. I just find it confusing that some
p2m functions can be used against PV guests, but not others.

Thanks, Roger.

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