| Message ID | 1470700619-7521-1-git-send-email-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/09/16 02:56, Tamas K Lengyel wrote: > From: Tamas K Lengyel <tamas@tklengyel.com> > > Use __get_gfn_type_access instead of get_gfn_type_access when checking > the hostp2m entries during altp2m mem_access setting and gfn remapping > to avoid a lock conflict which can make dom0 freeze. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > --- > Cc: George Dunlap <george.dunlap@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > --- > xen/arch/x86/mm/p2m.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com> Thanks, Razvan
>>> On 09.08.16 at 01:56, <tamas.lengyel@zentific.com> wrote: > Use __get_gfn_type_access instead of get_gfn_type_access when checking > the hostp2m entries during altp2m mem_access setting and gfn remapping > to avoid a lock conflict which can make dom0 freeze. You fail to mention why the resulting code is nevertheless correct: > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1787,8 +1787,8 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > if ( !mfn_valid(mfn) ) > { > > - mfn = get_gfn_type_access(hp2m, gfn_l, &t, &old_a, > - P2M_ALLOC | P2M_UNSHARE, &page_order); > + mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, > + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); In this case the respective p2m lock is already being held. > @@ -2563,8 +2563,8 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, > /* Check host p2m if no valid entry in alternate */ > if ( !mfn_valid(mfn) ) > { > - mfn = get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a, > - P2M_ALLOC | P2M_UNSHARE, &page_order); > + mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a, > + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); In this case, however, only ap2m's lock is being held, yet you query hp2m, so it's not immediately obvious whether the change is correct, and it would seem to me that you'd want to hold the gfn lock until after the subsequent ap2m->set_entry() (to make sure the two don't go out of sync). Since taking hp2m's lock can't nest inside any of its ap2m-s' ones, that'd be a slightly more intrusive adjustment. Jan
On Tue, Aug 9, 2016 at 1:55 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 09.08.16 at 01:56, <tamas.lengyel@zentific.com> wrote: >> Use __get_gfn_type_access instead of get_gfn_type_access when checking >> the hostp2m entries during altp2m mem_access setting and gfn remapping >> to avoid a lock conflict which can make dom0 freeze. > > You fail to mention why the resulting code is nevertheless correct: > >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -1787,8 +1787,8 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, >> if ( !mfn_valid(mfn) ) >> { >> >> - mfn = get_gfn_type_access(hp2m, gfn_l, &t, &old_a, >> - P2M_ALLOC | P2M_UNSHARE, &page_order); >> + mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, >> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > > In this case the respective p2m lock is already being held. > >> @@ -2563,8 +2563,8 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, >> /* Check host p2m if no valid entry in alternate */ >> if ( !mfn_valid(mfn) ) >> { >> - mfn = get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a, >> - P2M_ALLOC | P2M_UNSHARE, &page_order); >> + mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a, >> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > > In this case, however, only ap2m's lock is being held, yet you query > hp2m, so it's not immediately obvious whether the change is correct, > and it would seem to me that you'd want to hold the gfn lock until > after the subsequent ap2m->set_entry() (to make sure the two don't > go out of sync). Since taking hp2m's lock can't nest inside any of its > ap2m-s' ones, that'd be a slightly more intrusive adjustment. > True, we should lock the hp2m just before we lock the ap2m here, similar to the mem_access path, as we can't do gfn_lock after the altp2m is already locked. Tamas
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 812dbf6..fb1fda4 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1787,8 +1787,8 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, if ( !mfn_valid(mfn) ) { - mfn = get_gfn_type_access(hp2m, gfn_l, &t, &old_a, - P2M_ALLOC | P2M_UNSHARE, &page_order); + mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); rc = -ESRCH; if ( !mfn_valid(mfn) || t != p2m_ram_rw ) @@ -2563,8 +2563,8 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, /* Check host p2m if no valid entry in alternate */ if ( !mfn_valid(mfn) ) { - mfn = get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a, - P2M_ALLOC | P2M_UNSHARE, &page_order); + mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a, + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); if ( !mfn_valid(mfn) || t != p2m_ram_rw ) goto out;