From: George Dunlap <george.dunlap@citrix.com> To: Jan Beulich <JBeulich@suse.com>, xen-devel <xen-devel@lists.xenproject.org> Cc: George Dunlap <George.Dunlap@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Juergen Gross <jgross@suse.com> Subject: Re: [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages Date: Mon, 13 May 2019 14:44:10 +0100 [thread overview] Message-ID: <3172ee3f-0800-99a5-e148-d10ecdfd51a8@citrix.com> (raw) In-Reply-To: <5C7E78F6020000780021BB21@prv1-mh.provo.novell.com> On 3/5/19 1:26 PM, Jan Beulich wrote: > There are currently three more or less different checks: > - _get_page_type() adjusts the IOMMU mappings according to the new type > alone, > - arch_iommu_populate_page_table() wants just the type to be > PGT_writable_page, > - iommu_hwdom_init() additionally permits all other types with a type > refcount of zero. > The canonical one is in _get_page_type(), and as of XSA-288 > arch_iommu_populate_page_table() also has no need anymore to deal with > PGT_none pages. In the PV Dom0 case, however, PGT_none pages are still > necessary to consider, since in that case pages don't get handed to > guest_physmap_add_entry(). Furthermore, the function so far also > established r/o mappings, which is not in line with the rules set forth > by the XSA-288 change. > > For arch_iommu_populate_page_table() to not encounter PGT_none pages > anymore even in cases where the IOMMU gets enabled for a domain only > when it is already running, replace the IOMMU dependency in > guest_physmap_add_entry()'s handling of PV guests to check just the > system wide state instead of the domain property. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -837,11 +837,11 @@ guest_physmap_add_entry(struct domain *d > * > * Retain this property by grabbing a writable type ref and then > * dropping it immediately. The result will be pages that have a > - * writable type (and an IOMMU entry), but a count of 0 (such that > - * any guest-requested type changes succeed and remove the IOMMU > - * entry). > + * writable type (and an IOMMU entry if necessary), but a count of 0 > + * (such that any guest-requested type changes succeed and remove the > + * IOMMU entry). > */ > - if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw ) > + if ( !iommu_enabled || t != p2m_ram_rw ) > return 0; This looks good. One question about the next one... > > for ( i = 0; i < (1UL << page_order); ++i, ++page ) > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -192,21 +192,27 @@ void __hwdom_init iommu_hwdom_init(struc > > page_list_for_each ( page, &d->page_list ) > { > - unsigned long mfn = mfn_x(page_to_mfn(page)); > - unsigned long dfn = mfn_to_gmfn(d, mfn); > - unsigned int mapping = IOMMUF_readable; > - int ret; > + if ( (page->u.inuse.type_info & PGT_type_mask) == PGT_none ) > + { > + ASSERT(!(page->u.inuse.type_info & PGT_count_mask)); > + if ( get_page_and_type(page, d, PGT_writable_page) ) > + put_page_and_type(page); > + else if ( !rc ) > + rc = -EBUSY; > + } > > - if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) || > - ((page->u.inuse.type_info & PGT_type_mask) > - == PGT_writable_page) ) > - mapping |= IOMMUF_writable; > + if ( ((page->u.inuse.type_info & PGT_type_mask) == > + PGT_writable_page) ) > + { > + unsigned long mfn = mfn_x(page_to_mfn(page)); > + unsigned long dfn = mfn_to_gmfn(d, mfn); > + int ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, > + IOMMUF_readable | IOMMUF_writable, > + &flush_flags); What's the idea behind calling iommu_map() here, rather than relying on the one in _get_page_type()? Does need_iommu_pt_sync() not work yet at this point, or do you expect there to be pages that have been marked PGT_writable without having gone through _get_page_type()? -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
WARNING: multiple messages have this Message-ID (diff)
From: George Dunlap <george.dunlap@citrix.com> To: Jan Beulich <JBeulich@suse.com>, xen-devel <xen-devel@lists.xenproject.org> Cc: George Dunlap <George.Dunlap@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Juergen Gross <jgross@suse.com> Subject: Re: [Xen-devel] [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages Date: Mon, 13 May 2019 14:44:10 +0100 [thread overview] Message-ID: <3172ee3f-0800-99a5-e148-d10ecdfd51a8@citrix.com> (raw) Message-ID: <20190513134410.9yBaJQanA4AX9jNsF-SoEaOr-fKpJkT6iijPKNTs9jQ@z> (raw) In-Reply-To: <5C7E78F6020000780021BB21@prv1-mh.provo.novell.com> On 3/5/19 1:26 PM, Jan Beulich wrote: > There are currently three more or less different checks: > - _get_page_type() adjusts the IOMMU mappings according to the new type > alone, > - arch_iommu_populate_page_table() wants just the type to be > PGT_writable_page, > - iommu_hwdom_init() additionally permits all other types with a type > refcount of zero. > The canonical one is in _get_page_type(), and as of XSA-288 > arch_iommu_populate_page_table() also has no need anymore to deal with > PGT_none pages. In the PV Dom0 case, however, PGT_none pages are still > necessary to consider, since in that case pages don't get handed to > guest_physmap_add_entry(). Furthermore, the function so far also > established r/o mappings, which is not in line with the rules set forth > by the XSA-288 change. > > For arch_iommu_populate_page_table() to not encounter PGT_none pages > anymore even in cases where the IOMMU gets enabled for a domain only > when it is already running, replace the IOMMU dependency in > guest_physmap_add_entry()'s handling of PV guests to check just the > system wide state instead of the domain property. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -837,11 +837,11 @@ guest_physmap_add_entry(struct domain *d > * > * Retain this property by grabbing a writable type ref and then > * dropping it immediately. The result will be pages that have a > - * writable type (and an IOMMU entry), but a count of 0 (such that > - * any guest-requested type changes succeed and remove the IOMMU > - * entry). > + * writable type (and an IOMMU entry if necessary), but a count of 0 > + * (such that any guest-requested type changes succeed and remove the > + * IOMMU entry). > */ > - if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw ) > + if ( !iommu_enabled || t != p2m_ram_rw ) > return 0; This looks good. One question about the next one... > > for ( i = 0; i < (1UL << page_order); ++i, ++page ) > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -192,21 +192,27 @@ void __hwdom_init iommu_hwdom_init(struc > > page_list_for_each ( page, &d->page_list ) > { > - unsigned long mfn = mfn_x(page_to_mfn(page)); > - unsigned long dfn = mfn_to_gmfn(d, mfn); > - unsigned int mapping = IOMMUF_readable; > - int ret; > + if ( (page->u.inuse.type_info & PGT_type_mask) == PGT_none ) > + { > + ASSERT(!(page->u.inuse.type_info & PGT_count_mask)); > + if ( get_page_and_type(page, d, PGT_writable_page) ) > + put_page_and_type(page); > + else if ( !rc ) > + rc = -EBUSY; > + } > > - if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) || > - ((page->u.inuse.type_info & PGT_type_mask) > - == PGT_writable_page) ) > - mapping |= IOMMUF_writable; > + if ( ((page->u.inuse.type_info & PGT_type_mask) == > + PGT_writable_page) ) > + { > + unsigned long mfn = mfn_x(page_to_mfn(page)); > + unsigned long dfn = mfn_to_gmfn(d, mfn); > + int ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, > + IOMMUF_readable | IOMMUF_writable, > + &flush_flags); What's the idea behind calling iommu_map() here, rather than relying on the one in _get_page_type()? Does need_iommu_pt_sync() not work yet at this point, or do you expect there to be pages that have been marked PGT_writable without having gone through _get_page_type()? -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-05-13 13:44 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-05 13:21 [PATCH 0/3] today's XSAs assorted 4.12 candidate follow-up Jan Beulich 2019-03-05 13:25 ` [PATCH 1/3] x86/mm: fix #GP(0) in switch_cr3_cr4() Jan Beulich 2019-03-05 13:58 ` Andrew Cooper 2019-03-05 13:26 ` [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages Jan Beulich 2019-05-13 13:44 ` George Dunlap [this message] 2019-05-13 13:44 ` [Xen-devel] " George Dunlap 2019-05-13 13:59 ` Jan Beulich 2019-05-13 13:59 ` [Xen-devel] " Jan Beulich 2019-05-14 11:17 ` Jan Beulich 2019-05-14 11:17 ` [Xen-devel] " Jan Beulich [not found] ` <5C7E78B0020000780021BB1E@suse.com> 2019-03-05 13:28 ` [PATCH 1/3] x86/mm: fix #GP(0) in switch_cr3_cr4() Juergen Gross 2019-03-05 13:28 ` [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests Jan Beulich 2019-04-02 10:26 ` Julien Grall 2019-04-02 16:10 ` Jan Beulich 2019-04-08 11:47 ` Julien Grall 2019-04-08 11:47 ` [Xen-devel] " Julien Grall 2019-04-08 14:29 ` Jan Beulich 2019-04-08 14:29 ` [Xen-devel] " Jan Beulich 2019-04-09 9:50 ` Julien Grall 2019-04-09 9:50 ` [Xen-devel] " Julien Grall 2019-04-09 12:21 ` Jan Beulich 2019-04-09 12:21 ` [Xen-devel] " Jan Beulich 2019-04-14 16:33 ` Julien Grall 2019-04-14 16:33 ` [Xen-devel] " Julien Grall 2019-04-25 10:36 ` Jan Beulich 2019-04-25 10:36 ` [Xen-devel] " Jan Beulich 2019-04-08 11:58 ` Julien Grall 2019-04-08 11:58 ` [Xen-devel] " Julien Grall 2019-04-08 14:02 ` Jan Beulich 2019-04-08 14:02 ` [Xen-devel] " Jan Beulich 2019-04-08 16:10 ` Julien Grall 2019-04-08 16:10 ` [Xen-devel] " Julien Grall 2019-05-13 8:06 ` Ping: " Jan Beulich 2019-05-13 8:06 ` [Xen-devel] " Jan Beulich 2019-05-13 14:35 ` George Dunlap 2019-05-13 14:35 ` [Xen-devel] " George Dunlap 2019-05-13 15:13 ` Jan Beulich 2019-05-13 15:13 ` [Xen-devel] " Jan Beulich [not found] ` <5C7E78F6020000780021BB21@suse.com> 2019-03-05 13:50 ` [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages Juergen Gross 2019-03-05 15:21 ` Jan Beulich [not found] ` <5C7E798E020000780021BB43@suse.com> 2019-03-05 13:53 ` [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests Juergen Gross 2019-03-05 15:22 ` Jan Beulich 2019-03-05 15:23 [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages Juergen Gross
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=3172ee3f-0800-99a5-e148-d10ecdfd51a8@citrix.com \ --to=george.dunlap@citrix.com \ --cc=George.Dunlap@eu.citrix.com \ --cc=JBeulich@suse.com \ --cc=andrew.cooper3@citrix.com \ --cc=jgross@suse.com \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.