* [PATCH] xen: mark local pages as FOREIGN in the m2p_override @ 2012-05-14 11:23 Stefano Stabellini 2012-05-23 14:20 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 3+ messages in thread From: Stefano Stabellini @ 2012-05-14 11:23 UTC (permalink / raw) To: konrad.wilk Cc: xen-devel, linux-kernel, Stefano.Stabellini, Stefano Stabellini When the frontend and the backend reside on the same domain, even if we add pages to the m2p_override, these pages will never be returned by mfn_to_pfn because the check "get_phys_to_machine(pfn) != mfn" will always fail, so the pfn of the frontend will be returned instead (resulting in a deadlock because the frontend pages are already locked). However m2p_add_override can easily find out whether another pfn corresponding to the mfn exists in the m2p, and can set the FOREIGN bit in the p2m, making sure that mfn_to_pfn returns the pfn of the backend. This allows the backend to perform direct_IO on these pages, but as a side effect prevents the frontend from using get_user_pages_fast on them while they are being shared with the backend. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- arch/x86/xen/p2m.c | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-) diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c index 7ece122..c62ae5c 100644 --- a/arch/x86/xen/p2m.c +++ b/arch/x86/xen/p2m.c @@ -687,6 +687,7 @@ int m2p_add_override(unsigned long mfn, struct page *page, unsigned long uninitialized_var(address); unsigned level; pte_t *ptep = NULL; + int ret = 0; pfn = page_to_pfn(page); if (!PageHighMem(page)) { @@ -722,6 +723,16 @@ int m2p_add_override(unsigned long mfn, struct page *page, list_add(&page->lru, &m2p_overrides[mfn_hash(mfn)]); spin_unlock_irqrestore(&m2p_override_lock, flags); + /* p2m(m2p(mfn)) == mfn: the mfn is already present somewhere in + * this domain. Set the FOREIGN_FRAME_BIT in the p2m for the other + * pfn so that the following mfn_to_pfn(mfn) calls will return the + * pfn from the m2p_override (the backend pfn) instead. + * As a side effect GUPF might not be safe on the frontend pages + * while they are being shared with the backend. */ + ret = __get_user(pfn, &machine_to_phys_mapping[mfn]); + if (ret >= 0 && get_phys_to_machine(pfn) == mfn) + set_phys_to_machine(pfn, FOREIGN_FRAME(mfn)); + return 0; } EXPORT_SYMBOL_GPL(m2p_add_override); @@ -733,6 +744,7 @@ int m2p_remove_override(struct page *page, bool clear_pte) unsigned long uninitialized_var(address); unsigned level; pte_t *ptep = NULL; + int ret = 0; pfn = page_to_pfn(page); mfn = get_phys_to_machine(pfn); @@ -802,6 +814,12 @@ int m2p_remove_override(struct page *page, bool clear_pte) } else set_phys_to_machine(pfn, page->index); + mfn &= ~FOREIGN_FRAME_BIT; + ret = __get_user(pfn, &machine_to_phys_mapping[mfn]); + if (ret >= 0 && get_phys_to_machine(pfn) == FOREIGN_FRAME(mfn) && + m2p_find_override(mfn) == NULL) + set_phys_to_machine(pfn, mfn); + return 0; } EXPORT_SYMBOL_GPL(m2p_remove_override); -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xen: mark local pages as FOREIGN in the m2p_override 2012-05-14 11:23 [PATCH] xen: mark local pages as FOREIGN in the m2p_override Stefano Stabellini @ 2012-05-23 14:20 ` Konrad Rzeszutek Wilk 2012-05-23 17:37 ` Stefano Stabellini 0 siblings, 1 reply; 3+ messages in thread From: Konrad Rzeszutek Wilk @ 2012-05-23 14:20 UTC (permalink / raw) To: Stefano Stabellini; +Cc: xen-devel, linux-kernel On Mon, May 14, 2012 at 12:23:07PM +0100, Stefano Stabellini wrote: > When the frontend and the backend reside on the same domain, even if we > add pages to the m2p_override, these pages will never be returned by > mfn_to_pfn because the check "get_phys_to_machine(pfn) != mfn" will > always fail, so the pfn of the frontend will be returned instead > (resulting in a deadlock because the frontend pages are already locked). > > However m2p_add_override can easily find out whether another pfn > corresponding to the mfn exists in the m2p, and can set the FOREIGN bit > in the p2m, making sure that mfn_to_pfn returns the pfn of the backend. > > This allows the backend to perform direct_IO on these pages, but as a > side effect prevents the frontend from using get_user_pages_fast on > them while they are being shared with the backend. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > arch/x86/xen/p2m.c | 18 ++++++++++++++++++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c > index 7ece122..c62ae5c 100644 > --- a/arch/x86/xen/p2m.c > +++ b/arch/x86/xen/p2m.c > @@ -687,6 +687,7 @@ int m2p_add_override(unsigned long mfn, struct page *page, > unsigned long uninitialized_var(address); > unsigned level; > pte_t *ptep = NULL; > + int ret = 0; > > pfn = page_to_pfn(page); > if (!PageHighMem(page)) { > @@ -722,6 +723,16 @@ int m2p_add_override(unsigned long mfn, struct page *page, > list_add(&page->lru, &m2p_overrides[mfn_hash(mfn)]); > spin_unlock_irqrestore(&m2p_override_lock, flags); > > + /* p2m(m2p(mfn)) == mfn: the mfn is already present somewhere in > + * this domain. Set the FOREIGN_FRAME_BIT in the p2m for the other <nods> In other words, the MFN is local. And you want it to be forced to be !local - foreign right. > + * pfn so that the following mfn_to_pfn(mfn) calls will return the .. the other pfn being. So we want to override p2m(m2p(mfn)) == mfn[frontend] so that it becomes p2m(m2p(mfn)) == mfn[backend]? (nods) What happens if multiple m2p_add_override are called on the same page? This would be possible if the xen-blkfront is setup shared for the same disk, right? Won't we loose the old frontend PFN -> backend MFN information then as we would overwrite the old P2M relationship? > + * pfn from the m2p_override (the backend pfn) instead. Can you explain in the comment why we want to do that. I think I know, but I am not going to remember it in a month. > + * As a side effect GUPF might not be safe on the frontend pages > + * while they are being shared with the backend. */ How is it not safe? > + ret = __get_user(pfn, &machine_to_phys_mapping[mfn]); > + if (ret >= 0 && get_phys_to_machine(pfn) == mfn) if (ret == 0) [get_user only provides -EFAULT or 0] > + set_phys_to_machine(pfn, FOREIGN_FRAME(mfn)); > + > return 0; > } > EXPORT_SYMBOL_GPL(m2p_add_override); > @@ -733,6 +744,7 @@ int m2p_remove_override(struct page *page, bool clear_pte) > unsigned long uninitialized_var(address); > unsigned level; > pte_t *ptep = NULL; > + int ret = 0; > > pfn = page_to_pfn(page); > mfn = get_phys_to_machine(pfn); > @@ -802,6 +814,12 @@ int m2p_remove_override(struct page *page, bool clear_pte) > } else > set_phys_to_machine(pfn, page->index); > You also need a comment here. > + mfn &= ~FOREIGN_FRAME_BIT; > + ret = __get_user(pfn, &machine_to_phys_mapping[mfn]); > + if (ret >= 0 && get_phys_to_machine(pfn) == FOREIGN_FRAME(mfn) && ret == 0 > + m2p_find_override(mfn) == NULL) > + set_phys_to_machine(pfn, mfn); > + > return 0; > } > EXPORT_SYMBOL_GPL(m2p_remove_override); > -- > 1.7.2.5 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] xen: mark local pages as FOREIGN in the m2p_override 2012-05-23 14:20 ` Konrad Rzeszutek Wilk @ 2012-05-23 17:37 ` Stefano Stabellini 0 siblings, 0 replies; 3+ messages in thread From: Stefano Stabellini @ 2012-05-23 17:37 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Stefano Stabellini, xen-devel@lists.xensource.com, linux-kernel@vger.kernel.org On Wed, 23 May 2012, Konrad Rzeszutek Wilk wrote: > On Mon, May 14, 2012 at 12:23:07PM +0100, Stefano Stabellini wrote: > > When the frontend and the backend reside on the same domain, even if we > > add pages to the m2p_override, these pages will never be returned by > > mfn_to_pfn because the check "get_phys_to_machine(pfn) != mfn" will > > always fail, so the pfn of the frontend will be returned instead > > (resulting in a deadlock because the frontend pages are already locked). > > > > However m2p_add_override can easily find out whether another pfn > > corresponding to the mfn exists in the m2p, and can set the FOREIGN bit > > in the p2m, making sure that mfn_to_pfn returns the pfn of the backend. > > > > This allows the backend to perform direct_IO on these pages, but as a > > side effect prevents the frontend from using get_user_pages_fast on > > them while they are being shared with the backend. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > arch/x86/xen/p2m.c | 18 ++++++++++++++++++ > > 1 files changed, 18 insertions(+), 0 deletions(-) > > > > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c > > index 7ece122..c62ae5c 100644 > > --- a/arch/x86/xen/p2m.c > > +++ b/arch/x86/xen/p2m.c > > @@ -687,6 +687,7 @@ int m2p_add_override(unsigned long mfn, struct page *page, > > unsigned long uninitialized_var(address); > > unsigned level; > > pte_t *ptep = NULL; > > + int ret = 0; > > > > pfn = page_to_pfn(page); > > if (!PageHighMem(page)) { > > @@ -722,6 +723,16 @@ int m2p_add_override(unsigned long mfn, struct page *page, > > list_add(&page->lru, &m2p_overrides[mfn_hash(mfn)]); > > spin_unlock_irqrestore(&m2p_override_lock, flags); > > > > + /* p2m(m2p(mfn)) == mfn: the mfn is already present somewhere in > > + * this domain. Set the FOREIGN_FRAME_BIT in the p2m for the other > > <nods> In other words, the MFN is local. And you want it to be forced > to be !local - foreign right. Yes > > + * pfn so that the following mfn_to_pfn(mfn) calls will return the > > .. the other pfn being. So we want to override p2m(m2p(mfn)) == mfn[frontend] > so that it becomes p2m(m2p(mfn)) == mfn[backend]? (nods) Exactly > What happens if multiple m2p_add_override are called on the same page? > This would be possible if the xen-blkfront is setup shared for the same > disk, right? It is possible even with a single disk, I can see the frontend sharing three times the same page, before the first request has completed, when mounting an ext4 partition. But it is completely safe: m2p_add_override is going to set the page as FOREIGN the first time and m2p_remove_override is going to remove the bit only when the last page is unshared (it does so by checking m2p_find_override(mfn) == NULL). > Won't we loose the old frontend PFN -> backend MFN information then as > we would overwrite the old P2M relationship? No, we are just changing it to be: old frontend PFN -> (backend MFN | FOREIGN_FRAME_BIT) > > + * pfn from the m2p_override (the backend pfn) instead. > > Can you explain in the comment why we want to do that. I think > I know, but I am not going to remember it in a month. Good point. The reason is that the pages that have been shared by xen-blkfront are locked (lock_page, called by do_read_cache_page). When the userspace backend tries to use them with direct_IO, mfn_to_pfn returns the pfn of the frontend, so do_blockdev_direct_IO is going to try to lock the same pages again. I'll add this comment to the commit message. > > + * As a side effect GUPF might not be safe on the frontend pages > > + * while they are being shared with the backend. */ > > How is it not safe? Because get_user_pages_fast ends up calling mfn_to_pfn that would now return the backend pfn rather than the frontend pfn. > > + ret = __get_user(pfn, &machine_to_phys_mapping[mfn]); > > + if (ret >= 0 && get_phys_to_machine(pfn) == mfn) > > if (ret == 0) > > [get_user only provides -EFAULT or 0] OK, I'll change the check. > > + set_phys_to_machine(pfn, FOREIGN_FRAME(mfn)); > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(m2p_add_override); > > @@ -733,6 +744,7 @@ int m2p_remove_override(struct page *page, bool clear_pte) > > unsigned long uninitialized_var(address); > > unsigned level; > > pte_t *ptep = NULL; > > + int ret = 0; > > > > pfn = page_to_pfn(page); > > mfn = get_phys_to_machine(pfn); > > @@ -802,6 +814,12 @@ int m2p_remove_override(struct page *page, bool clear_pte) > > } else > > set_phys_to_machine(pfn, page->index); > > > > You also need a comment here. OK, I'll add one. > > + mfn &= ~FOREIGN_FRAME_BIT; > > + ret = __get_user(pfn, &machine_to_phys_mapping[mfn]); > > + if (ret >= 0 && get_phys_to_machine(pfn) == FOREIGN_FRAME(mfn) && > > ret == 0 OK > > + m2p_find_override(mfn) == NULL) > > + set_phys_to_machine(pfn, mfn); > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(m2p_remove_override); > > -- > > 1.7.2.5 > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-05-23 17:37 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-14 11:23 [PATCH] xen: mark local pages as FOREIGN in the m2p_override Stefano Stabellini 2012-05-23 14:20 ` Konrad Rzeszutek Wilk 2012-05-23 17:37 ` Stefano Stabellini
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).