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 09.08.16 at 11:25, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:

> 
> On 8/9/2016 4:13 PM, Jan Beulich wrote:
>>>>> On 09.08.16 at 09:39, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>>> 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?
>> That's certainly one of the options, but beware of overflow.
> 
> Oh, thanks for your remind, Jan. I just found that atomic_t is defined 
> as  "typedef struct { int counter; } atomic_t;"
> which do have overflow issues if entry_count is supposed to be a uint32 
> or uint64.
> 
> Now I have another thought: the entry_count is referenced in 3 
> different  scenarios:
> 1> the hvmop handlers -  hvmop_set_mem_type() and 
> hvmop_map_mem_type_to_ioreq_server(),
> which  are triggered by device model, and will not be concurrent. And 
> hvmop_set_mem_type() will
> permit the mem type change only when an ioreq server has already been 
> mapped to this type.

You shouldn't rely on a well behaved dm, and that's already
leaving aside the question whether there couldn't even be well
behaved use cases of parallel invocations of this op.

> 2> the misconfiguration handlers - resolve_misconfig() or do_recalc(),  
> which are triggered by HVM's
> vm exit, yet this will only happen after the ioreq server has already 
> been unmapped. This means the
> accesses to the entry_count will not be concurrent with the above hvmop 
> handlers.

This case may be fine, but not for (just) the named reason:
Multiple misconfig invocations can happen at the same time, but
presumably your addition is inside the p2m-locked region (you'd
have to double check that).

> 3> routine hap_enable_log_dirty() - this can be triggered by tool stack 
> at any time, but only by read
> access to this entry_count, so a read_atomic() would work.

A read access may be fine, but only if the value can't get incremented
in a racy way.

Jan


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