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

From: Yu Zhang <yu.c.zhang@linux.intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xen.org, Paul Durrant <paul.durrant@citrix.com>,
	zhiyuan.lv@intel.com, Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: [PATCH v5 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
Date: Tue, 9 Aug 2016 17:25:05 +0800	[thread overview]
Message-ID: <57A9A171.7070401@linux.intel.com> (raw)
In-Reply-To: <57A9ACB20200007800104173@prv-mh.provo.novell.com>

On 8/9/2016 4:13 PM, Jan Beulich wrote:
>>>> On 09.08.16 at 09:39, <yu.c.zhang@linux.intel.com> wrote:
>> 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?
> 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.

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.

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.

Do you think this analysis reasonable?
If so, we may only use read_atomic() to get the value of entry_count in 
hap_enable_log_dirty() and keep
other places as it is.

Of course, some explanation comments in entry_count's definition would 
be necessary. :-)

Yu

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