| Message ID | 5CCA94F9020000780022B02A@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry() | expand |
Hi Jan, On 5/2/19 7:58 AM, Jan Beulich wrote: > This is what both callers of guest_physmap_add_page() in memory.c want > (for the !paging_mode_translate() case), and it is also what both > callers in gnttab_transfer() need (but have been lacking). The other > (x86-specific) callers are all HVM-only, and hence unaffected by this > change. > > Sadly this isn't enough yet to drop Arm's dummy macro, as there's one > more use in page_alloc.c. I think we can live with that. I have now sent a patch to move the dummy implementation in common code (see <20190507151458.29350-15-julien.grall@arm.com>). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> The common code looks fine to me. I will leave Andrew commenting for the x86 parts: Acked-by: Julien Grall <julien.grall@arm.com> Cheers,
On 5/2/19 7:58 AM, Jan Beulich wrote: > This is what both callers of guest_physmap_add_page() in memory.c want > (for the !paging_mode_translate() case), and it is also what both > callers in gnttab_transfer() need (but have been lacking). The other > (x86-specific) callers are all HVM-only, and hence unaffected by this > change. Sorry, what's going on here? -George
>>> On 07.05.19 at 18:15, <george.dunlap@citrix.com> wrote: > On 5/2/19 7:58 AM, Jan Beulich wrote: >> This is what both callers of guest_physmap_add_page() in memory.c want >> (for the !paging_mode_translate() case), and it is also what both >> callers in gnttab_transfer() need (but have been lacking). The other >> (x86-specific) callers are all HVM-only, and hence unaffected by this >> change. > > Sorry, what's going on here? I guess that's a Jan-wrote-an-unparsable-description-once-again question? 1) The two callers in common/memory.c currently call set_gpfn_from_mfn() themselves, so moving the call into guest_physmap_add_page() helps tidy their code. 2) The two callers in common/grant_table.c fail to make that call alongside the one to guest_physmap_add_page(), so will actually get fixed by the change. 3) Other callers are HVM only and are hence unaffected by a change to the function's !paging_mode_translate() part. Jan
On 5/8/19 2:57 PM, Jan Beulich wrote: >>>> On 07.05.19 at 18:15, <george.dunlap@citrix.com> wrote: >> On 5/2/19 7:58 AM, Jan Beulich wrote: >>> This is what both callers of guest_physmap_add_page() in memory.c want >>> (for the !paging_mode_translate() case), and it is also what both >>> callers in gnttab_transfer() need (but have been lacking). The other >>> (x86-specific) callers are all HVM-only, and hence unaffected by this >>> change. >> >> Sorry, what's going on here? > > I guess that's a Jan-wrote-an-unparsable-description-once-again > question? Sort of, yes. :-) It's not unparsable; it's just missing some information. > 1) The two callers in common/memory.c currently call set_gpfn_from_mfn() > themselves, so moving the call into guest_physmap_add_page() helps > tidy their code. > > 2) The two callers in common/grant_table.c fail to make that call alongside > the one to guest_physmap_add_page(), so will actually get fixed by the > change. > > 3) Other callers are HVM only and are hence unaffected by a change to > the function's !paging_mode_translate() part. Right. I think I would have written something like this: 8<--- When giving ownership of a page to a PV guest, a number of things need to be done, including adding the page into the IOMMU if appropriate, and updating the m2p. At the moment this is done by calling guest_physmap_add_entry() and set_gpfn_from_mfn() separately. Unfortunately, this duplication leads to mistakes: gnttab_transfer() calls guest_physmap_add_entry() but fails to call set_gpfn_from_mfn(). Since guest_physmap_add_entry() is already special-casing PV guests, move set_gpfn_from_mfn() into that function. --->8 I have a technical question about the patch; I'll reply inline to the patch itself. -George
On 5/2/19 7:58 AM, Jan Beulich wrote: > This is what both callers of guest_physmap_add_page() in memory.c want > (for the !paging_mode_translate() case), and it is also what both > callers in gnttab_transfer() need (but have been lacking). The other > (x86-specific) callers are all HVM-only, and hence unaffected by this > change. > > Sadly this isn't enough yet to drop Arm's dummy macro, as there's one > more use in page_alloc.c. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d > * any guest-requested type changes succeed and remove the IOMMU > * entry). > */ > - if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw ) > + if ( t != p2m_ram_rw ) > return 0; So, you seem to be claiming that the only way to get here is via guest_physmap_add_page(), which will always call this function with p2m_ram_rw. So then what's the point of this conditional at all anymore? Would it be better to add an ASSERT(t == p2m_ram_rw) here? And if we ever *do* get here with t == p2m_ram_rw, do we really not want to call set_gpfn_from_mfn()? -George
>>> On 08.05.19 at 16:59, <george.dunlap@citrix.com> wrote: > On 5/8/19 2:57 PM, Jan Beulich wrote: >>>>> On 07.05.19 at 18:15, <george.dunlap@citrix.com> wrote: >>> On 5/2/19 7:58 AM, Jan Beulich wrote: >>>> This is what both callers of guest_physmap_add_page() in memory.c want >>>> (for the !paging_mode_translate() case), and it is also what both >>>> callers in gnttab_transfer() need (but have been lacking). The other >>>> (x86-specific) callers are all HVM-only, and hence unaffected by this >>>> change. >>> >>> Sorry, what's going on here? >> >> I guess that's a Jan-wrote-an-unparsable-description-once-again >> question? > > Sort of, yes. :-) It's not unparsable; it's just missing some information. > >> 1) The two callers in common/memory.c currently call set_gpfn_from_mfn() >> themselves, so moving the call into guest_physmap_add_page() helps >> tidy their code. >> >> 2) The two callers in common/grant_table.c fail to make that call alongside >> the one to guest_physmap_add_page(), so will actually get fixed by the >> change. >> >> 3) Other callers are HVM only and are hence unaffected by a change to >> the function's !paging_mode_translate() part. > > Right. I think I would have written something like this: Well, I can certainly use your suggested text, but for now I was meaning to perhaps use the text from my earlier reply as replacement, if that's deemed better than the original. Just let me know. Jan > 8<--- > > When giving ownership of a page to a PV guest, a number of things need > to be done, including adding the page into the IOMMU if appropriate, and > updating the m2p. At the moment this is done by calling > guest_physmap_add_entry() and set_gpfn_from_mfn() separately. > > Unfortunately, this duplication leads to mistakes: gnttab_transfer() > calls guest_physmap_add_entry() but fails to call set_gpfn_from_mfn(). > > Since guest_physmap_add_entry() is already special-casing PV guests, > move set_gpfn_from_mfn() into that function. > > --->8 > > I have a technical question about the patch; I'll reply inline to the > patch itself. > > -George
>>> On 08.05.19 at 17:08, <george.dunlap@citrix.com> wrote: > On 5/2/19 7:58 AM, Jan Beulich wrote: >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d >> * any guest-requested type changes succeed and remove the IOMMU >> * entry). >> */ >> - if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw ) >> + if ( t != p2m_ram_rw ) >> return 0; > > So, you seem to be claiming that the only way to get here is via > guest_physmap_add_page(), Well, I'm not "claiming" anything here, I'm just modifying existing code (and no more than what fits under this patch's title). > which will always call this function with > p2m_ram_rw. So then what's the point of this conditional at all > anymore? Would it be better to add an ASSERT(t == p2m_ram_rw) here? > > And if we ever *do* get here with t == p2m_ram_rw, do we really not want > to call set_gpfn_from_mfn()? Thinking about e.g. p2m_grant_map_* I wouldn't want to add the suggested ASSERT(), and the M2P doesn't want updating in that case either. Jan
On 5/8/19 4:16 PM, Jan Beulich wrote: >>>> On 08.05.19 at 17:08, <george.dunlap@citrix.com> wrote: >> On 5/2/19 7:58 AM, Jan Beulich wrote: >>> --- a/xen/arch/x86/mm/p2m.c >>> +++ b/xen/arch/x86/mm/p2m.c >>> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d >>> * any guest-requested type changes succeed and remove the IOMMU >>> * entry). >>> */ >>> - if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw ) >>> + if ( t != p2m_ram_rw ) >>> return 0; >> >> So, you seem to be claiming that the only way to get here is via >> guest_physmap_add_page(), > > Well, I'm not "claiming" anything here, I'm just modifying existing > code (and no more than what fits under this patch's title). Not here, but in the other email. But looking at it -- it looks like on x86, guest_physmap_add_entry() is actually called from *exactly* two locations: hvm/grant_table.c:create_grant_p2m_mapping(), and p2m.h:guest_physmap_add_page(). Which sort of makes me wonder if it might not be better to add the PV conditional to guest_physmap_add_page() instead, and leave guest_physmap_add_entry() as entirely HVM. > >> which will always call this function with >> p2m_ram_rw. So then what's the point of this conditional at all >> anymore? Would it be better to add an ASSERT(t == p2m_ram_rw) here? >> >> And if we ever *do* get here with t == p2m_ram_rw, do we really not want >> to call set_gpfn_from_mfn()? > > Thinking about e.g. p2m_grant_map_* I wouldn't want to add the > suggested ASSERT(), and the M2P doesn't want updating in that > case either. Sorry, do you think p2m_grant_map_* is more likely somehow than p2m_ram_ro? It looks very much like neither one should ever happen. The purpose of having an ASSERT() there is to alert developers making such a fundamental change to the fact that they need to think carefully about what should happen in that case. (For safety sake, because ASSERT is only a debugging aid, we certainly need to keep some sort of conditional here to make sure non-writable pages don't end up with writable mappings in the IOMMU.) If we don't have an ASSERT(), then we need to think carefully about whether we should call set_gp2n_from_mfn() if t is, for instance, p2m_ram_ro. -George
>>> On 08.05.19 at 17:45, <george.dunlap@citrix.com> wrote: > On 5/8/19 4:16 PM, Jan Beulich wrote: >>>>> On 08.05.19 at 17:08, <george.dunlap@citrix.com> wrote: >>> On 5/2/19 7:58 AM, Jan Beulich wrote: >>>> --- a/xen/arch/x86/mm/p2m.c >>>> +++ b/xen/arch/x86/mm/p2m.c >>>> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d >>>> * any guest-requested type changes succeed and remove the IOMMU >>>> * entry). >>>> */ >>>> - if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw ) >>>> + if ( t != p2m_ram_rw ) >>>> return 0; >>> >>> So, you seem to be claiming that the only way to get here is via >>> guest_physmap_add_page(), >> >> Well, I'm not "claiming" anything here, I'm just modifying existing >> code (and no more than what fits under this patch's title). > > Not here, but in the other email. > > But looking at it -- it looks like on x86, guest_physmap_add_entry() is > actually called from *exactly* two locations: > hvm/grant_table.c:create_grant_p2m_mapping(), and > p2m.h:guest_physmap_add_page(). > > Which sort of makes me wonder if it might not be better to add the PV > conditional to guest_physmap_add_page() instead, and leave > guest_physmap_add_entry() as entirely HVM. Yes, I think I'll do this. >>> which will always call this function with >>> p2m_ram_rw. So then what's the point of this conditional at all >>> anymore? Would it be better to add an ASSERT(t == p2m_ram_rw) here? >>> >>> And if we ever *do* get here with t == p2m_ram_rw, do we really not want >>> to call set_gpfn_from_mfn()? >> >> Thinking about e.g. p2m_grant_map_* I wouldn't want to add the >> suggested ASSERT(), and the M2P doesn't want updating in that >> case either. > > Sorry, do you think p2m_grant_map_* is more likely somehow than > p2m_ram_ro? It looks very much like neither one should ever happen. The > purpose of having an ASSERT() there is to alert developers making such a > fundamental change to the fact that they need to think carefully about > what should happen in that case. Let's face it - p2m types are a HVM concept only anyway. But this discussion becomes moot (afaict) with the change above anyway. Jan
--- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d * any guest-requested type changes succeed and remove the IOMMU * entry). */ - if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw ) + if ( t != p2m_ram_rw ) return 0; for ( i = 0; i < (1UL << page_order); ++i, ++page ) { - if ( get_page_and_type(page, d, PGT_writable_page) ) + if ( !need_iommu_pt_sync(d) ) + /* nothing */; + else if ( get_page_and_type(page, d, PGT_writable_page) ) put_page_and_type(page); else return -EINVAL; + + set_gpfn_from_mfn(mfn_x(mfn) + i, gfn_x(gfn) + i); } return 0; --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -270,16 +270,10 @@ static void populate_physmap(struct memo guest_physmap_add_page(d, _gfn(gpfn), mfn, a->extent_order); - if ( !paging_mode_translate(d) ) - { - for ( j = 0; j < (1U << a->extent_order); j++ ) - set_gpfn_from_mfn(mfn_x(mfn_add(mfn, j)), gpfn + j); - - /* Inform the domain of the new page's machine address. */ - if ( unlikely(__copy_mfn_to_guest_offset(a->extent_list, i, - mfn)) ) - goto out; - } + if ( !paging_mode_translate(d) && + /* Inform the domain of the new page's machine address. */ + unlikely(__copy_mfn_to_guest_offset(a->extent_list, i, mfn)) ) + goto out; } } @@ -755,15 +749,11 @@ static long memory_exchange(XEN_GUEST_HA guest_physmap_add_page(d, _gfn(gpfn), mfn, exch.out.extent_order); - if ( !paging_mode_translate(d) ) - { - for ( k = 0; k < (1UL << exch.out.extent_order); k++ ) - set_gpfn_from_mfn(mfn_x(mfn_add(mfn, k)), gpfn + k); - if ( __copy_mfn_to_guest_offset(exch.out.extent_start, - (i << out_chunk_order) + j, - mfn) ) - rc = -EFAULT; - } + if ( !paging_mode_translate(d) && + __copy_mfn_to_guest_offset(exch.out.extent_start, + (i << out_chunk_order) + j, + mfn) ) + rc = -EFAULT; } BUG_ON( !(d->is_dying) && (j != (1UL << out_chunk_order)) );
This is what both callers of guest_physmap_add_page() in memory.c want (for the !paging_mode_translate() case), and it is also what both callers in gnttab_transfer() need (but have been lacking). The other (x86-specific) callers are all HVM-only, and hence unaffected by this change. Sadly this isn't enough yet to drop Arm's dummy macro, as there's one more use in page_alloc.c. Signed-off-by: Jan Beulich <jbeulich@suse.com>