Re: [Xen-devel] [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.08.16 at 01:56, <tamas.lengyel@xxxxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel