Re: [Xen-devel] [PATCH v4 3/4] x86/mm: handle foreign mappings in p2m_entry_modify

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Roger Pau Monné [mailto:roger.pau@xxxxxxxxxx]
> Sent: Tuesday, February 19, 2019 10:01 PM
> 
> On Tue, Feb 19, 2019 at 06:14:00AM +0000, Tian, Kevin wrote:
> > > From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx]
> > > Sent: Tuesday, February 19, 2019 1:27 AM
> > > diff --git a/xen/arch/x86/mm/shadow/common.c
> > > b/xen/arch/x86/mm/shadow/common.c
> > > index fe48c4a02b..ad670de515 100644
> > > --- a/xen/arch/x86/mm/shadow/common.c
> > > +++ b/xen/arch/x86/mm/shadow/common.c
> > > @@ -3189,7 +3189,8 @@ shadow_write_p2m_entry(struct domain *d,
> > > unsigned long gfn,
> > >           sh_unshadow_for_p2m_change(d, gfn, p, new, level);
> > >
> > >      p2m_entry_modify(p2m_get_hostp2m(d),
> > > p2m_flags_to_type(l1e_get_flags(new)),
> > > -                     p2m_flags_to_type(l1e_get_flags(*p)), level);
> > > +                     p2m_flags_to_type(l1e_get_flags(*p)), 
> > > l1e_get_mfn(new),
> > > +                     l1e_get_mfn(*p), level);
> > >
> > >      /* Update the entry with new content */
> > >      safe_write_pte(p, new);
> > > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> > > index f4ec2becbd..2801a8ccca 100644
> > > --- a/xen/include/asm-x86/p2m.h
> > > +++ b/xen/include/asm-x86/p2m.h
> > > @@ -932,11 +932,14 @@ int p2m_set_ioreq_server(struct domain *d,
> > > unsigned int flags,
> > >  struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d,
> > >                                                unsigned int *flags);
> > >
> > > -static inline void p2m_entry_modify(struct p2m_domain *p2m,
> > > p2m_type_t nt,
> > > -                                    p2m_type_t ot, unsigned int level)
> > > +static inline int p2m_entry_modify(struct p2m_domain *p2m,
> p2m_type_t
> > > nt,
> > > +                                   p2m_type_t ot, mfn_t nfn, mfn_t ofn,
> > > +                                   unsigned int level)
> > >  {
> > > -    if ( level != 1 || nt == ot )
> > > -        return;
> > > +    BUG_ON(level > 1 && (nt == p2m_ioreq_server || nt ==
> > > p2m_map_foreign));
> > > +
> > > +    if ( level != 1 || (nt == ot && mfn_eq(nfn, ofn)) )
> > > +        return 0;
> >
> > could also return here in case of nt==ot==p2m_ioreq_server,
> > otherwise p2m->ioreq.entry_count might be unnecessarily
> > inc/dec if mfn changes while type stays as p2m_ioreq_server...
> 
> The original code that open-coded the handling of p2m_ioreq_server
> didn't have this optimization, and as said by George I'm not sure the
> extra check is going to be worth it. I expect changing the mfn of an
> entry with type p2m_ioreq_server is not something very common.
> 

OK. Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

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