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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> @@ -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()?

> +    /*
> +     * 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?

> +            get_gfn_unshare(d, gfn, &t);
> +
> +            if ( (t == p2m_ioreq_server) &&
> +                 !(p2m_change_type_one(d, gfn, t, p2m_ram_rw)) )

Stray parentheses.

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

Jan


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