Re: [PATCH v5 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 8/9/2016 12:29 AM, Jan Beulich wrote:
>>>> On 12.07.16 at 11:02, <yu.c.zhang@linux.intel.com> 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@lists.xen.org
https://lists.xen.org/xen-devel