[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.