Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.

[Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>]

On 9/9/2016 6:09 PM, Jan Beulich wrote:
> > > > On 09.09.16 at 11:56, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> > On 9/9/2016 5:44 PM, Jan Beulich wrote:
> > > > > > On 09.09.16 at 11:24, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> > > > On 9/9/2016 4:20 PM, Jan Beulich wrote:
> > > > > > > > On 09.09.16 at 09:24, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> > > > > > On 9/9/2016 1:26 PM, Yu Zhang wrote:
> > > > > > > > > > On 02.09.16 at 12:47, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> > > > > > > > @@ -965,7 +968,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
> > > > > > > >         if ( is_epte_valid(ept_entry) )
> > > > > > > >         {
> > > > > > > >             if ( (recalc || ept_entry->recalc) &&
> > > > > > > > -             p2m_is_changeable(ept_entry->sa_p2mt) )
> > > > > > > > +             p2m_is_changeable(ept_entry->sa_p2mt) &&
> > > > > > > > +             (ept_entry->sa_p2mt != p2m_ioreq_server) )
> > > > > > > >                 *t = p2m_is_logdirty_range(p2m, gfn, gfn) ?
> > > > > > > p2m_ram_logdirty
> > > > > > > >                                                           : p2m_ram_rw;
> > > > > > > >             else
> > > > > > > Considering this (and at least one more similar adjustment further
> > > > > > > down), is it really appropriate to include p2m_ioreq_server in the
> > > > > > > set of "changeable" types?
> > > > > > Well, I agree p2m_ioreq_server do have different behaviors than the
> > > > > > p2m_log_dirty, but removing p2m_ioreq_server from changeable type
> > > > > > would need more specific code for the p2m_ioreq_server in routines like
> > > > > > resolve_misconfig(), do_recalc() and p2m_change_entry_type_global() etc.
> > > > > > I've tried this approach and abandoned later. :)
> > > > > I can see that the other option might require more adjustments, in
> > > > > which case I guess this variant would be fine if you created another
> > > > > helper (well named) inline function instead of open coding this in
> > > > > several places. Of course such dissimilar handling in the various
> > > > > places p2m_is_changeable() gets used right now will additionally
> > > > > require good reasoning - after all that predicate exists to express
> > > > > the similarities between different code paths.
> > > > Thanks Jan.
> > > > Well, for the logic of p2m type recalculation, similarities between
> > > > p2m_ioreq_server
> > > > and other changeable types exceeds their differences. As to the special
> > > > cases, how
> > > > about we use a macro, i.e. p2m_is_ioreq?
> > > That'd be better than the open coded check, but would still result
> > > in (taking the above example)
> > >
> > >                p2m_is_changeable(ept_entry->sa_p2mt) &&
> > >                !p2m_is_ioreq(ept_entry->sa_p2mt) )
> > >
> > > ? What I'd prefer is a predicate that can be applied here on its own,
> > > without involving && or ||.
> > OK. I can think of 2 scenarios that p2m_ioreq_server needs special handling:
> >
> > 1> In ept_get_entry()/recal_type(), the p2m types are supposed to return
> > as it is, instead of
> > changing to p2m_log_dirty. So we can use a macro or a inline function
> > like p2m_check_changeable(),
> > which combines the
> >
> >                p2m_is_changeable(ept_entry->sa_p2mt) &&
> >                !p2m_is_ioreq(ept_entry->sa_p2mt) )
> >
> > together.
> >
> > 2> In resolve_misconfig()/do_recalc(), the entry_count gets decremented. We
> > do not need this new inline function, because they are in a separate
> > if() statement.
> >
> > Is this OK? :)
> Sounds reasonable. But please give George and others a chance to
> voice their opinions before you go that route.
Sure, thanks!

Yu

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