Re: [Xen-devel] [PATCH v4 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.

[Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>]

On 6/15/2016 6:21 PM, Jan Beulich wrote:
> > > > On 15.06.16 at 11:50, <george.dunlap@xxxxxxxxxx> wrote:
> > On 14/06/16 14:31, Jan Beulich wrote:
> > > > > > On 14.06.16 at 15:13, <george.dunlap@xxxxxxxxxx> wrote:
> > > > On 14/06/16 11:45, Jan Beulich wrote:
> > > > > Locking is somewhat strange here: You protect against the "set"
> > > > > counterpart altering state while you retrieve it, but you don't
> > > > > protect against the returned data becoming stale by the time
> > > > > the caller can consume it. Is that not a problem? (The most
> > > > > concerning case would seem to be a race of hvmop_set_mem_type()
> > > > > with de-registration of the type.)
> > > > How is that different than calling set_mem_type() first, and then
> > > > de-registering without first unmapping all the types?
> > > Didn't we all agree this is something that should be disallowed
> > > anyway (not that I've seen this implemented, i.e. just being
> > > reminded of it by your reply)?
> > I think I suggested it as a good idea, but Paul and Yang both thought it
> > wasn't necessary.  Do you think it should be a requirement?
> I think things shouldn't be left in a half-adjusted state.
>
> > We could have the de-registering operation fail in those circumstances;
> > but probably a more robust thing to do would be to have Xen go change
> > all the ioreq_server entires back to ram_rw (since if the caller just
> > ignores the failure, things are in an even worse state).
> If that's reasonable to do without undue delay (e.g. by using
> the usual "recalculate everything" forced to trickle down through
> the page table levels, then that's as good.
Thanks for your advices, Jan & George.

Previously in the 2nd version, I used p2m_change_entry_type_global() to 
reset the
outstanding p2m_ioreq_server entries back to p2m_ram_rw asynchronously after
the de-registration. But we realized later that this approach means we 
can not support
live migration. And to recalculate the whole p2m table forcefully when 
de-registration
happens means too much cost.

And further discussion with Paul was that we can leave the 
responsibility to reset p2m type
to the device model side, and even a device model fails to do so, the 
affected one will only
be the current VM, neither other VM nor hypervisor will get hurt.

I thought we have reached agreement in the review process of version 2, 
so I removed
this part from version 3.

> > > > > > +    uint32_t flags;     /* IN - types of accesses to be forwarded to the
> > > > > > +                           ioreq server. flags with 0 means to unmap the
> > > > > > +                           ioreq server */
> > > > > > +#define _HVMOP_IOREQ_MEM_ACCESS_READ 0
> > > > > > +#define HVMOP_IOREQ_MEM_ACCESS_READ \
> > > > > > +    (1u << _HVMOP_IOREQ_MEM_ACCESS_READ)
> > > > > > +
> > > > > > +#define _HVMOP_IOREQ_MEM_ACCESS_WRITE 1
> > > > > > +#define HVMOP_IOREQ_MEM_ACCESS_WRITE \
> > > > > > +    (1u << _HVMOP_IOREQ_MEM_ACCESS_WRITE)
> > > > > Is there any use for these _HVMOP_* values? The more that they
> > > > > violate standard C name space rules?
> > > > I assume he's just going along with what he sees in params.h.
> > > > "Violating standard C name space rules" by having #defines which start
> > > > with a single _ seems to be a well-established policy for Xen. :-)
> > > Sadly, and I'm trying to prevent matters becoming worse.
> > > Speaking of which - there are XEN_ prefixes missing here too.
> > Right, so in that case I think I would have said, "I realize that lots
> > of other places in the Xen interface use this sort of template for
> > flags, but I think it's a bad idea and I'm trying to stop it expanding.
> >   Is there any actual need to have the bit numbers defined separately?
> > If not, please just define each flag as (1u << 0), &c."
> Actually my coding style related comment wasn't for these two
> stage definitions - for those I simply questioned whether they're
> needed. My style complaint was for the <underscore><uppercase>
> name pattern (which would simply be avoided by not having the
> individual bit number #define-s).
>
> > I think you've tripped over "changing coding styles" in unfamiliar code
> > before too, so you know how frustrating it is to try to follow the
> > existing coding style only to be told that you did it wrong. :-)
> Agreed, you caught me on this one. Albeit with the slight
> difference that in the public interface we can't as easily correct
> old mistakes to aid people who simply clone surrounding code
> when adding new bits (the possibility of adding #ifdef-ery doesn't
> seem very attractive to me there, unless we got reports of actual
> name space collisions).
Hah, I guess these 2 #defines are just cloned from similar ones, and I 
did not expected
they would receive so much comments. Anyway, I admire your preciseness 
and thanks
for pointing this out. :)

Since the bit number #defines have no special meaning, I'd like to just 
define the flags
directly:

#define HVMOP_IOREQ_MEM_ACCESS_READ (1u << 0)
#define HVMOP_IOREQ_MEM_ACCESS_WRITE (1u << 1)


B.R.
Yu

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