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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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...


Thanks,
Razvan

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