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

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

On 8/9/2016 12:29 AM, Jan Beulich wrote:
> > > > On 12.07.16 at 11:02, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> > @@ -5512,6 +5513,12 @@ static int hvmop_set_mem_type(
> >           if ( rc )
> >               goto out;
> >   
> > +        if ( t == p2m_ram_rw && memtype[a.hvmmem_type] == p2m_ioreq_server )
> > +            p2m->ioreq.entry_count++;
> > +
> > +        if ( t == p2m_ioreq_server && memtype[a.hvmmem_type] == p2m_ram_rw )
> > +            p2m->ioreq.entry_count--;
> > +
> These (and others below) happen, afaict, outside of any lock, which
> can't be right.
How about we make this entry_count as atomic_t and use atomic_inc/dec 
instead?
> > @@ -5530,11 +5537,13 @@ static int hvmop_set_mem_type(
> >   }
> >   
> >   static int hvmop_map_mem_type_to_ioreq_server(
> > -    XEN_GUEST_HANDLE_PARAM(xen_hvm_map_mem_type_to_ioreq_server_t) uop)
> > +    XEN_GUEST_HANDLE_PARAM(xen_hvm_map_mem_type_to_ioreq_server_t) uop,
> > +    unsigned long *iter)
> >   {
> >       xen_hvm_map_mem_type_to_ioreq_server_t op;
> >       struct domain *d;
> >       int rc;
> > +    unsigned long gfn = *iter;
> >   
> >       if ( copy_from_guest(&op, uop, 1) )
> >           return -EFAULT;
> > @@ -5559,7 +5568,42 @@ 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 || op.flags != 0 )
> > +        rc = hvm_map_mem_type_to_ioreq_server(d, op.id, op.type, op.flags);
> Couldn't you omit the right side of the || in the if()?
Oh right, it is redundant. Thanks! :-)

> > +    /*
> > +     * 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 ( gfn <= p2m->max_mapped_pfn )
> > +        {
> > +            p2m_type_t t;
> > +
> > +            if ( p2m->ioreq.entry_count == 0 )
> > +                break;
> Perhaps better to be moved up into the while() expression?
OK. Another thing is maybe we can use _atomic_read() to get value of 
entry_count if
the counter is defined as atomic_t?

> > +            get_gfn_unshare(d, gfn, &t);
> > +
> > +            if ( (t == p2m_ioreq_server) &&
> > +                 !(p2m_change_type_one(d, gfn, t, p2m_ram_rw)) )
> Stray parentheses.
Got it. Thanks!

> > +                p2m->ioreq.entry_count--;
> > +
> > +            put_gfn(d, gfn);
> > +
> > +            /* Check for continuation if it's not the last iteration. */
> > +            if ( ++gfn <= p2m->max_mapped_pfn &&
> > +                 !(gfn & HVMOP_op_mask) &&
> > +                 hypercall_preempt_check() )
> > +            {
> > +                rc = -ERESTART;
> > +                goto out;
> I think you need to write gfn back to *iter.
Yes, thanks for pointing out.

Yu

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