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

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 9/9/2016 1:26 PM, Yu Zhang wrote:
> >>> On 02.09.16 at 12:47, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> > @@ -5551,7 +5553,35 @@ static int hvmop_map_mem_type_to_ioreq_server(
> >      if ( rc != 0 )
> >          goto out;
> >
> > -    rc = hvm_map_mem_type_to_ioreq_server(d, op.id, op.type, 
> op.flags);
> > +    if ( gfn == 0 )
> > +        rc = hvm_map_mem_type_to_ioreq_server(d, op.id, op.type, 
> op.flags);
> > +
> > +    /*
> > +     * Iterate p2m table when an ioreq server unmaps from 
> p2m_ioreq_server,
> > +     * and reset the remaining p2m_ioreq_server entries back to 
> p2m_ram_rw.
> > +     */
> > +    if ( op.flags == 0 && rc == 0 )
> > +    {
> > +        struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +
> > +        while ( read_atomic(&p2m->ioreq.entry_count) &&
> > +                gfn <= p2m->max_mapped_pfn )
> > +        {
> > +            unsigned long gfn_end = gfn + HVMOP_op_mask;
> > +
> > +            p2m_finish_type_change(d, gfn, gfn_end,
> > +                                   p2m_ioreq_server, p2m_ram_rw);
> > +
> > +            /* Check for continuation if it's not the last 
> iteration. */
> > +            if ( ++gfn_end <= p2m->max_mapped_pfn &&
> > +                hypercall_preempt_check() )
> > +            {
> > +                rc = -ERESTART;
> > +                *iter = gfn_end;
> > +                goto out;
> > +            }
> > +        }
>
> "gfn" doesn't get updated, so if no preemption happens here, the
> same GFN range will get acted on a second time (and so on, until
> eventually preemption becomes necessary).

Oh, right. Thanks for pointing out. :)

>
> Also when you don't really need to use "goto", please try to avoid
> it - here you could easily use just "break" instead.

OK.

>
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -545,6 +545,9 @@ static int resolve_misconfig(struct p2m_domain 
> *p2m,
> > unsigned long gfn)
> >                      e.ipat = ipat;
> >                      if ( e.recalc && p2m_is_changeable(e.sa_p2mt) )
> >                      {
> > +                         if ( e.sa_p2mt == p2m_ioreq_server )
> > +                             p2m->ioreq.entry_count--;
>
> ISTR that George had asked you to put this accounting elsewhere.
>

Yes. You have good memory. : )

George's suggestion is to put inside atomic_write_ept_entry(), which is 
a quite core routine,
and is only for ept. And my suggestion is to put inside the 
p2m_type_change_one() and routine
resolve_misconfig()/do_recalc() as well, so that we can support both 
Intel EPT and AMD NPT -
like the p2m_change_entry_type_global().

I had given a more detailed explanation in a reply on Aug 30 in the v5 
thread. :)

> And then I'd really like you to add assertions making sure this
> count doesn't underflow.

OK.

>
> > @@ -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. :)

>
> > +void p2m_finish_type_change(struct domain *d,
> > +                           unsigned long start, unsigned long end,
> > +                           p2m_type_t ot, p2m_type_t nt)
> > +{
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +    p2m_type_t t;
> > +    unsigned long gfn = start;
> > +
> > +    ASSERT(start <= end);
> > +    ASSERT(ot != nt);
> > +    ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
> > +
> > +    p2m_lock(p2m);
> > +
> > +    end = (end > p2m->max_mapped_pfn) ? p2m->max_mapped_pfn : end;
>
> min() or an if() without else.

Got it. Thanks

>
> > +    while ( gfn <= end )
> > +    {
> > +        get_gfn_query_unlocked(d, gfn, &t);
> > +
> > +        if ( t == ot )
> > +            p2m_change_type_one(d, gfn, t, nt);
> > +
> > +        gfn++;
> > +    }
> > +
> > +    p2m_unlock(p2m);
> > +}
>
> And then I wonder why p2m_change_type_range() can't be used
> instead of adding this new function.
>

Well, like p2m_change_entry_type_global() , p2m_change_type_range() also
does the change asynchronously. And here this routine is introduced
to synchronously reset the p2m type.

> Jan

Thanks
Yu

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