Re: [Xen-devel] [PATCH v1] Fix p2m_set_suppress_ve

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Apr 4, 2019 at 6:50 AM Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
>
> On 4/4/19 3:46 PM, Razvan Cojocaru wrote:
> > On 4/3/19 6:30 PM, Jan Beulich wrote:
> >>>>> On 03.04.19 at 17:17, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> >>> On 4/3/19 5:58 PM, Jan Beulich wrote:
> >>>>>>> On 03.04.19 at 16:29, <aisaila@xxxxxxxxxxxxxxx> wrote:
> >>>>> --- a/xen/arch/x86/mm/p2m.c
> >>>>> +++ b/xen/arch/x86/mm/p2m.c
> >>>>> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d,
> >>>>> gfn_t gfn, bool suppress_ve,
> >>>>>        mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
> >>>>>        if ( !mfn_valid(mfn) )
> >>>>>        {
> >>>>> -        rc = -ESRCH;
> >>>>> -        goto out;
> >>>>> +        unsigned int page_order;
> >>>>> +
> >>>>> +        mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
> >>>>> +                                    P2M_ALLOC | P2M_UNSHARE,
> >>>>> &page_order, 0);
> >>>>
> >>>> I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
> >>>> at least P2M_UNSHARE is too heavy: Why would you want to force
> >>>> un-sharing of a page when all you want to alter is #VE behavior?
> >>>
> >>> That logic was taken from p2m_set_altp2m_mem_access(), we thought the
> >>> two cases are very similar.
> >>
> >> I see.
> >
> > On the UNSHARE observation, we don't know why the author originally
> > requested the flag. We decided to keep it on the assumption that it
> > _probably_ handles some corner-case that somebody has come accross.
> >
> > We'll prepare a mini-series factoring out the code we've been discussing
> > in separate functions: one for getting things out of the hostp2m if the
> > entry is not present in the altp2m, and one for the special
> > page-order-dependent code (which is duplicated in
> > p2m_set_altp2m_mem_access() and p2m_change_altp2m_gfn()).
> >
> > Before going into that, are we now certain that ALLOC is sufficient? I
> > believe it should be for _our_ use-cases, but we don't want to break
> > anyone's code. Maybe Tamas knows more about this.
>
> Sorry, I forgot to mention that p2m_change_altp2m_gfn() only uses ALLOC:
>
> 2649     /* Check host p2m if no valid entry in alternate */
> 2650     if ( !mfn_valid(mfn) )
> 2651     {
> 2652         mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> 2653                                     P2M_ALLOC, &page_order, 0);
> 2654
> 2655         if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> 2656             goto out;
> 2657
> 2658         /* If this is a superpage, copy that first */
> 2659         if ( page_order != PAGE_ORDER_4K )
> 2660         {
> 2661             gfn_t gfn;
> 2662             unsigned long mask;
> 2663
> 2664             mask = ~((1UL << page_order) - 1);
> 2665             gfn = _gfn(gfn_x(old_gfn) & mask);
> 2666             mfn = _mfn(mfn_x(mfn) & mask);
> 2667
> 2668             if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> 2669                 goto out;
> 2670         }
> 2671     }
>
> Confusing...

I agree that it is confusing. It would be fine to UNSHARE here as well
to keep things consistent but otherwise it's not really an issue as
the entry type is checked later to ensure that this is a p2m_ram_rw
entry. We are simply trying to keep mem_sharing and _modified_ altp2m
entries exclusive. So it is fine to have mem_shared entries in the
hostp2m and have those entries be copied into altp2m tables lazily,
but for altp2m entries that have changed mem_access permissions or are
remapped we want the entries in the hostp2m to be of regular type.
This is not necessarily a technical requirement, it's mostly just to
reduce complexity. So it would be fine to add UNSHARE here as well, I
guess the only reason why I haven't done that is because I already
trigger the unshare and copy-to-altp2m before remapping by setting
dummy mem_access permission on the entries.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel