Re: [Xen-devel] [PATCH v3] dm_op: Add xendevicemodel_modified_memory_bulk.

[Jennifer Herbert]

On 29/03/17 15:54, Jan Beulich wrote:

On 28.03.17 at 15:18,  wrote:

@@ -441,13 +481,8 @@ static int dm_op(domid_t domid,
   struct xen_dm_op_modified_memory *data =
   _memory;
   
-const_op = false;

-
-rc = -EINVAL;
-if ( data->pad )
-break;
-
-rc = modified_memory(d, data);
+rc = modified_memory(d, data, [1]);
+const_op = (rc != 0);

Isn't this wrong now, i.e. don't you need to copy back the
header now in all cases?

I only define what I'll set nr_extents to in case of error, and of
course opaque
is opaque.

Well, but you do need the opaque value for the continuation,
don't you? In which case you need to also write back on
-ERESTART. And as you say you need to write back in case
of error. So I'd expect

const_op = !rc;



Quite right, see you point now - I didn't notice I'd inverted the logic.

-jenny


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

Re: [Xen-devel] [PATCH v3] dm_op: Add xendevicemodel_modified_memory_bulk.

[Jan Beulich]

>>> On 29.03.17 at 16:35,  wrote:
> On 29/03/17 11:38, Jan Beulich wrote:
> On 28.03.17 at 15:18,  wrote:
>>> @@ -441,13 +481,8 @@ static int dm_op(domid_t domid,
>>>   struct xen_dm_op_modified_memory *data =
>>>   _memory;
>>>   
>>> -const_op = false;
>>> -
>>> -rc = -EINVAL;
>>> -if ( data->pad )
>>> -break;
>>> -
>>> -rc = modified_memory(d, data);
>>> +rc = modified_memory(d, data, [1]);
>>> +const_op = (rc != 0);
>> Isn't this wrong now, i.e. don't you need to copy back the
>> header now in all cases?
> 
> I only define what I'll set nr_extents to in case of error, and of 
> course opaque
> is opaque.

Well, but you do need the opaque value for the continuation,
don't you? In which case you need to also write back on
-ERESTART. And as you say you need to write back in case
of error. So I'd expect

   const_op = !rc;

> By only writing back on error, I hoped to improve efficiency for the 
> common case,
> (especially for existing use with calls of one extent).  (I know its 
> only a small difference)
> If you want me to write back - what do you want me to write back for 
> success?

Right, avoiding to write something useless is sensible. If anything,
the original value of nr_extents would make sense to be written
back, but that value was long lost by that time.

Jan


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

Re: [Xen-devel] [PATCH v3] dm_op: Add xendevicemodel_modified_memory_bulk.

[Jennifer Herbert]

On 29/03/17 11:38, Jan Beulich wrote:

On 28.03.17 at 15:18,  wrote:
Perhaps drop "already"? Personally I also wouldn't mind you dropping 
the variable altogether and using header->opaque directly, but I 
guess that's too "opaque" for your taste? 


It would make the code too opaque for my taste, so I'll just drop the 
'already' bit.



@@ -441,13 +481,8 @@ static int dm_op(domid_t domid,
  struct xen_dm_op_modified_memory *data =
  _memory;
  
-const_op = false;

-
-rc = -EINVAL;
-if ( data->pad )
-break;
-
-rc = modified_memory(d, data);
+rc = modified_memory(d, data, [1]);
+const_op = (rc != 0);

Isn't this wrong now, i.e. don't you need to copy back the
header now in all cases?


I only define what I'll set nr_extents to in case of error, and of 
course opaque

is opaque.
If I where to write back, I'd be writing back 0 to nr_extents - which 
wouldn’t really
mean anything since I’m not defining the order for which I’m processing 
them.
In fact the only thing it tells you is that extent 0 is the last one 
processed, which

I don't think its all that useful.

Ideally I'd prefer to leave it untouched on success, but the original 
value is lost on

continuation, this would be more involved.

By only writing back on error, I hoped to improve efficiency for the 
common case,
(especially for existing use with calls of one extent).  (I know its 
only a small difference)
If you want me to write back - what do you want me to write back for 
success?



Cheers,

-jenny


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

[Xen-devel] [PATCH v3] dm_op: Add xendevicemodel_modified_memory_bulk.

[Jennifer Herbert]

From: Jennifer Herbert 

This new lib devicemodel call allows multiple extents of pages to be
marked as modified in a single call.  This is something needed for a
usecase I'm working on.

The xen side of the modified_memory call has been modified to accept
an array of extents.  The devicemodle library either provides an array
of length 1, to support the original library function, or a second
function allows an array to be provided.

Signed-off-by: Jennifer Herbert 
---
Cc: Jan Beulich 
Cc: Ian Jackson 
Cc: Wei Liu 
Cc: Andrew Cooper 
Cc: Paul Durrant 
---
Changes as discussed on thread.

 tools/libs/devicemodel/core.c   |   30 --
 tools/libs/devicemodel/include/xendevicemodel.h |   19 +++-
 xen/arch/x86/hvm/dm.c   |  115 +++
 xen/include/public/hvm/dm_op.h  |   22 -
 4 files changed, 133 insertions(+), 53 deletions(-)

diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index a85cb49..f9e37a5 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -434,22 +434,36 @@ int xendevicemodel_track_dirty_vram(
  dirty_bitmap, (size_t)(nr + 7) / 8);
 }
 
-int xendevicemodel_modified_memory(
-xendevicemodel_handle *dmod, domid_t domid, uint64_t first_pfn,
-uint32_t nr)
+int xendevicemodel_modified_memory_bulk(
+xendevicemodel_handle *dmod, domid_t domid,
+struct xen_dm_op_modified_memory_extent *extents, uint32_t nr)
 {
 struct xen_dm_op op;
-struct xen_dm_op_modified_memory *data;
+struct xen_dm_op_modified_memory *header;
+size_t extents_size = nr * sizeof(struct xen_dm_op_modified_memory_extent);
 
 memset(, 0, sizeof(op));
 
 op.op = XEN_DMOP_modified_memory;
-data = _memory;
+header = _memory;
 
-data->first_pfn = first_pfn;
-data->nr = nr;
+header->nr_extents = nr;
+header->pfns_processed = 0;
 
-return xendevicemodel_op(dmod, domid, 1, , sizeof(op));
+return xendevicemodel_op(dmod, domid, 2, , sizeof(op),
+ extents, extents_size);
+}
+
+int xendevicemodel_modified_memory(
+xendevicemodel_handle *dmod, domid_t domid, uint64_t first_pfn,
+uint32_t nr)
+{
+struct xen_dm_op_modified_memory_extent extent;
+
+extent.first_pfn = first_pfn;
+extent.nr = nr;
+
+return xendevicemodel_modified_memory_bulk(dmod, domid, , 1);
 }
 
 int xendevicemodel_set_mem_type(
diff --git a/tools/libs/devicemodel/include/xendevicemodel.h 
b/tools/libs/devicemodel/include/xendevicemodel.h
index b3f600e..9c62bf9 100644
--- a/tools/libs/devicemodel/include/xendevicemodel.h
+++ b/tools/libs/devicemodel/include/xendevicemodel.h
@@ -236,8 +236,8 @@ int xendevicemodel_track_dirty_vram(
 uint32_t nr, unsigned long *dirty_bitmap);
 
 /**
- * This function notifies the hypervisor that a set of domain pages
- * have been modified.
+ * This function notifies the hypervisor that a set of contiguous
+ * domain pages have been modified.
  *
  * @parm dmod a handle to an open devicemodel interface.
  * @parm domid the domain id to be serviced
@@ -250,6 +250,21 @@ int xendevicemodel_modified_memory(
 uint32_t nr);
 
 /**
+ * This function notifies the hypervisor that a set of discontiguous
+ * domain pages have been modified.
+ *
+ * @parm dmod a handle to an open devicemodel interface.
+ * @parm domid the domain id to be serviced
+ * @parm extents an array of extent structs, which each hold
+ a start_pfn and nr (number of pfns).
+ * @parm nr the number of extents in the array
+ * @return 0 on success, -1 on failure.
+ */
+int xendevicemodel_modified_memory_bulk(
+xendevicemodel_handle *dmod, domid_t domid,
+struct xen_dm_op_modified_memory_extent extents[], uint32_t nr);
+
+/**
  * This function notifies the hypervisor that a set of domain pages
  * are to be treated in a specific way. (See the definition of
  * hvmmem_type_t).
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 2122c45..b5031ce 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -119,56 +119,96 @@ static int set_isa_irq_level(struct domain *d, uint8_t 
isa_irq,
 }
 
 static int modified_memory(struct domain *d,
-   struct xen_dm_op_modified_memory *data)
+   struct xen_dm_op_modified_memory *header,
+   xen_dm_op_buf_t* buf)
 {
-xen_pfn_t last_pfn = data->first_pfn + data->nr - 1;
-unsigned int iter = 0;
-int rc = 0;
-
-if ( (data->first_pfn > last_pfn) ||
- (last_pfn > domain_get_maximum_gpfn(d)) )
-return -EINVAL;
+/* Process maximum of 256 pfns before checking for continuation */
+const unsigned int cont_check_interval = 0x100;
+unsigned int rem_extents =