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 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.
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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel