[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2] dm_op: Add xendevicemodel_modified_memory_bulk.
On 23/03/17 15:58, Jan Beulich wrote:
On 22.03.17 at 20:55, <Jennifer.Herbert@xxxxxxxxxx> wrote:
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -119,56 +119,89 @@ static int set_isa_irq_level(struct domain *d, uint8_t
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -237,13 +237,24 @@ struct xen_dm_op_set_pci_link_route {
* XEN_DMOP_modified_memory: Notify that a set of pages were modified by
* an emulator.
*
- * NOTE: In the event of a continuation, the @first_pfn is set to the
- * value of the pfn of the remaining set of pages and @nr reduced
- * to the size of the remaining set.
+ * DMOP buf 1 contains an array of xen_dm_op_modified_memory_extent with
+ * @nr_extent entries. The value of @nr_extent will be reduced on
+ * continuation.
+ *
+ * @pfns_processed is used for continuation, and not intended to be usefull
+ * to the caller. It gives the number of pfns already processed (and can
+ * be skipped) in the last extent. This should always be set to zero.
*/
#define XEN_DMOP_modified_memory 11
struct xen_dm_op_modified_memory {
+ /* IN - number of extents. */
+ uint32_t nr_extents;
+ /* IN - should be set to 0, and is updated on continuation. */
+ uint32_t pfns_processed;
I'd prefer if the field (if to be kept) wasn't named after its current
purpose, nor should we state anything beyond it needing to be
zero when first invoking the call. These are implementation details
which callers should not rely on.
Assuming we keep it, how about calling it "reserved", with a comment in
dm.c explaining what its actually for?
Elsewhere we use "opaque", but "reserved" is probably fine too.
However, we may want to name it as an OUT value, for the
error-on-extent indication mentioned above (of course another
option would be to make nr_extent IN/OUT). As an OUT, we're
free to use it for whatever intermediate value, just so long as
upon final return to the caller it has the intended value. (It's
debatable whether it should be IN/OUT, due to the need for it
to be set to zero.)
Having an indication of which extent failed seem a sensible idea. We'd
need that
parameter to be initially set to something that can represent none of
the extents,
such that if there is an error before we get to precessing the extents,
this is clear.
I don't think this is a requirement - failure outside of any extent
processing can reasonably be attached to the first extent. The
more that the actual processing of the extent (after reading the
coordinates from guest memory) can't actually fail. With that
observation I'm in fact no longer convinced we really need such
an indication - I simply didn't pay attention to this aspect when
first suggesting it. The more that your backwards processing
would invalidate a common conclusion a caller might draw: Error
on extent N means all extents lower than N were processed
successfully.
So if you wanted to stick to providing this information I'd say
use the opaque (or whatever it's going to be named) field to
provide that status. Switch back to processing extents forwards,
having the opaque field starting out as zero point to the first
extent as the failed one initially. Initial processing during
continuation handling can't fail unless the caller has fiddled with
the hypercall arguments, so I wouldn't see anything wrong with
not providing a reliable error indicator in that case.
I was mostly considering it, from a debugging perspective. Any failure
would be due to bad
arguments, which would indicate a serious bug, and trying to continue
after such a bug
was discovered would seem most unwise.
If I copied back the buffer on event of error, then we could state that
nr_extent would point
to one after extent that had the error, but say it is not defined which
if any extents would have
succeeded. This would be a trivial change, but aid with any debugging.
I can continue to count extents backwards, saving one parameter.
I can then have an opaque, which i say nothing about, other then it
should be zero.
This i'd use for for pfns_processed.
How does that sound?
-jenny
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
