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 04:15:38PM +0100, George Dunlap wrote:
> On 8/12/19 3:55 PM, Roger Pau Monné wrote:
> > 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.
> 
> But if iommu is set the same in both cases, and if the flush happens in
> both cases, then why did this patch work and the previous patch didn't?

iommu_legacy_[un]map uses iommu_iotlb_flush while my patch uses
iommu_iotlb_flush_all. I'm still not sure why this difference in
behaviour, similar to what happens with iommu_flush_all.

> >> 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.
> 
> Right, but that's because set_identity_p2m_entry() isn't really about
> the p2m.  It's about making sure that both devices and SMI handlers (or
> whatever) can access certain bits of memory.
> 
> Lots of functions in p2m.c "tolerate" calls for PV guests; and
> guest_physmap_* calls used to actually to IOMMU operations on PV guests
> before XSA 288.
> 
> We could change that to set_identity_physmap_entry() or something if you
> prefer.

OK, let me try to first figure out exactly what's going on and then I
can propose something sensible :).

Thanks, Roger.

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