From: "Roger Pau Monné" <roger.pau@citrix.com>
To: George Dunlap <george.dunlap@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Wei Liu <wl@xen.org>,
George Dunlap <george.dunlap@eu.citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Roman Shaposhnik <roman@zededa.com>,
Paul Durrant <paul.durrant@citrix.com>,
Jan Beulich <jbeulich@suse.com>,
xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
Date: Mon, 12 Aug 2019 17:30:26 +0200 [thread overview]
Message-ID: <20190812153026.mubd3o5mlyisggxl@Air-de-Roger> (raw)
In-Reply-To: <93b5ce4e-f841-cf41-8ae4-e5986afe7aae@citrix.com>
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-08-12 15:31 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-02 9:22 [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains Roger Pau Monne
2019-08-02 13:09 ` Jan Beulich
2019-08-12 10:17 ` Paul Durrant
2019-08-12 13:17 ` George Dunlap
2019-08-12 13:56 ` Roger Pau Monné
2019-08-12 14:24 ` George Dunlap
2019-08-12 14:55 ` Roger Pau Monné
2019-08-12 15:15 ` George Dunlap
2019-08-12 15:30 ` Roger Pau Monné [this message]
2019-12-05 11:24 ` Jan Beulich
2019-12-09 10:59 ` Roger Pau Monné
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190812153026.mubd3o5mlyisggxl@Air-de-Roger \
--to=roger.pau@citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=george.dunlap@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=kevin.tian@intel.com \
--cc=paul.durrant@citrix.com \
--cc=roman@zededa.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).