| Message ID | 851d095cd457109e4a22a2e5ecd36ccbdacbf48b.1526916378.git.simon@ruderich.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/21/2018 10:36 AM, Simon Ruderich wrote: > Adapted patch from Baojun Wang [1] with the following commit message: > > I found this could be useful to have qemu-softmmu as a cross > debugger (launch with -s -S command line option), then if we can > have a command to load guest physical memory, we can use cross gdb > to do some target debug which gdb cannot do directly. > > pmemload is necessary to directly write physical memory which is not > possible with gdb alone as it uses only logical addresses. > > The QAPI for pmemload uses "val" as parameter name for the physical > address. This name is not very descriptive but is consistent with the > existing pmemsave. Changing the parameter name of pmemsave is not > possible without breaking the existing API. > > [1]: https://lists.gnu.org/archive/html/qemu-trivial/2014-04/msg00074.html > > Based-on-patch-by: Baojun Wang <wangbj@gmail.com> > Signed-off-by: Simon Ruderich <simon@ruderich.org> > --- > cpus.c | 41 +++++++++++++++++++++++++++++++++++++++++ > hmp-commands.hx | 14 ++++++++++++++ > hmp.c | 12 ++++++++++++ > hmp.h | 1 + > qapi/misc.json | 20 ++++++++++++++++++++ > 5 files changed, 88 insertions(+) I haven't closely reviewed this, but a quick note: > +++ b/qapi/misc.json > @@ -1219,6 +1219,26 @@ > { 'command': 'pmemsave', > 'data': {'val': 'int', 'size': 'int', 'filename': 'str'} } > > +## > +# @pmemload: > +# > +# Load a portion of guest physical memory from a file. > +# > +# @val: the physical address of the guest to start from > +# > +# @size: the size of memory region to load > +# > +# @offset: the offset in the file to start from > +# > +# @filename: the file to load the memory from as binary data > +# > +# Returns: Nothing on success > +# > +# Since: 2.13 There was no 2.13 release, and this feature has missed the 3.0 deadlines, so it would need to say 'Since: 3.1' > +## > +{ 'command': 'pmemload', > + 'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} } > + > ## > # @cont: > # >
* Simon Ruderich (simon@ruderich.org) wrote: > Adapted patch from Baojun Wang [1] with the following commit message: > > I found this could be useful to have qemu-softmmu as a cross > debugger (launch with -s -S command line option), then if we can > have a command to load guest physical memory, we can use cross gdb > to do some target debug which gdb cannot do directly. > > pmemload is necessary to directly write physical memory which is not > possible with gdb alone as it uses only logical addresses. > > The QAPI for pmemload uses "val" as parameter name for the physical > address. This name is not very descriptive but is consistent with the > existing pmemsave. Changing the parameter name of pmemsave is not > possible without breaking the existing API. > > [1]: https://lists.gnu.org/archive/html/qemu-trivial/2014-04/msg00074.html > > Based-on-patch-by: Baojun Wang <wangbj@gmail.com> > Signed-off-by: Simon Ruderich <simon@ruderich.org> > --- > cpus.c | 41 +++++++++++++++++++++++++++++++++++++++++ > hmp-commands.hx | 14 ++++++++++++++ > hmp.c | 12 ++++++++++++ > hmp.h | 1 + > qapi/misc.json | 20 ++++++++++++++++++++ > 5 files changed, 88 insertions(+) > > diff --git a/cpus.c b/cpus.c > index 49d4d44916..9b105336af 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -2367,6 +2367,47 @@ exit: > qemu_close(fd); > } > > +void qmp_pmemload(int64_t addr, int64_t size, int64_t offset, > + const char *filename, Error **errp) > +{ > + int fd; > + size_t l; > + ssize_t r; > + uint8_t buf[1024]; > + > + fd = qemu_open(filename, O_RDONLY | O_BINARY); > + if (fd < 0) { > + error_setg_file_open(errp, errno, filename); > + return; > + } > + if (offset > 0) { > + if (lseek(fd, offset, SEEK_SET) != offset) { > + error_setg_errno(errp, errno, > + "could not seek to offset %" PRIx64, offset); > + goto exit; > + } > + } > + > + while (size != 0) { > + l = sizeof(buf); > + if (l > size) { > + l = size; > + } > + r = read(fd, buf, l); > + if (r <= 0) { > + error_setg(errp, QERR_IO_ERROR); > + goto exit; > + } > + l = r; /* in case of short read */ > + cpu_physical_memory_write(addr, buf, l); > + addr += l; > + size -= l; > + } > + > +exit: > + qemu_close(fd); > +} > + > void qmp_inject_nmi(Error **errp) > { > nmi_monitor_handle(monitor_get_cpu_index(), errp); > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 0734fea931..84647c7c1d 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -822,6 +822,20 @@ STEXI > @item pmemsave @var{addr} @var{size} @var{file} > @findex pmemsave > save to disk physical memory dump starting at @var{addr} of size @var{size}. > +ETEXI > + > + { > + .name = "pmemload", > + .args_type = "val:l,size:i,offset:i,filename:s", > + .params = "addr size offset file", > + .help = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'", > + .cmd = hmp_pmemload, > + }, > + I'm guessing that size and offset should be 'l' to allow large sizes and offsets, and there's an 'F' type for filenames (see monitor.c which has a big comment table near the start). Also, had you considered rearranging and making them optional, for example if you do: val:l,filename:F,offset:i?,size:i? I think that would mean you can do the fairly obvious: pmemload addr "myfile" with the assumption that loads the whole file. Dave > +STEXI > +@item pmemload @var{addr} @var{size} @var{offset} @var{file} > +@findex pmemload > +load from disk physical memory dump starting at @var{addr} of size @var{size} at file offset @var{offset}. > ETEXI > > { > diff --git a/hmp.c b/hmp.c > index a4d28913bb..b85c943b63 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1105,6 +1105,18 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict) > hmp_handle_error(mon, &err); > } > > +void hmp_pmemload(Monitor *mon, const QDict *qdict) > +{ > + uint64_t size = qdict_get_int(qdict, "size"); > + uint64_t offset = qdict_get_int(qdict, "offset"); > + const char *filename = qdict_get_str(qdict, "filename"); > + uint64_t addr = qdict_get_int(qdict, "val"); > + Error *err = NULL; > + > + qmp_pmemload(addr, size, offset, filename, &err); > + hmp_handle_error(mon, &err); > +} > + > void hmp_ringbuf_write(Monitor *mon, const QDict *qdict) > { > const char *chardev = qdict_get_str(qdict, "device"); > diff --git a/hmp.h b/hmp.h > index 20f27439d3..31767ea4a8 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -47,6 +47,7 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict); > void hmp_cpu(Monitor *mon, const QDict *qdict); > void hmp_memsave(Monitor *mon, const QDict *qdict); > void hmp_pmemsave(Monitor *mon, const QDict *qdict); > +void hmp_pmemload(Monitor *mon, const QDict *qdict); > void hmp_ringbuf_write(Monitor *mon, const QDict *qdict); > void hmp_ringbuf_read(Monitor *mon, const QDict *qdict); > void hmp_cont(Monitor *mon, const QDict *qdict); > diff --git a/qapi/misc.json b/qapi/misc.json > index f5988cc0b5..b4c0065b02 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -1219,6 +1219,26 @@ > { 'command': 'pmemsave', > 'data': {'val': 'int', 'size': 'int', 'filename': 'str'} } > > +## > +# @pmemload: > +# > +# Load a portion of guest physical memory from a file. > +# > +# @val: the physical address of the guest to start from > +# > +# @size: the size of memory region to load > +# > +# @offset: the offset in the file to start from > +# > +# @filename: the file to load the memory from as binary data > +# > +# Returns: Nothing on success > +# > +# Since: 2.13 > +## > +{ 'command': 'pmemload', > + 'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} } > + > ## > # @cont: > # > -- > 2.15.0 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Aug 09, 2018 at 11:46:50AM -0500, Eric Blake wrote: > There was no 2.13 release, and this feature has missed the 3.0 deadlines, so > it would need to say 'Since: 3.1' Will do. Regards Simon
On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -822,6 +822,20 @@ STEXI >> @item pmemsave @var{addr} @var{size} @var{file} >> @findex pmemsave >> save to disk physical memory dump starting at @var{addr} of size @var{size}. >> +ETEXI >> + >> + { >> + .name = "pmemload", >> + .args_type = "val:l,size:i,offset:i,filename:s", >> + .params = "addr size offset file", >> + .help = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'", >> + .cmd = hmp_pmemload, >> + }, >> + > > I'm guessing that size and offset should be 'l' to allow large > sizes and offsets, and there's an 'F' type for filenames I've copied that from "pmemsave" and "memsave" which use 'i' for size. I'll add patches which will adapt them to use both 'l' and 'F' and adapt my pmemload patch as well. qapi/misc.json seems to always use 'int' for integer types. Is this value large enough on 64-bit architectures? > (see monitor.c which has a big comment table near the start). Thanks. Just curious, what is the difference between 's' and 'F'. Is that only for documentation purposes (and maybe tab completion) or is the usage different? I noticed existing code uses qdict_get_str() for both 's' and 'F'. > Also, had you considered rearranging and making them optional, > for example if you do: > > val:l,filename:F,offset:i?,size:i? > > I think that would mean you can do the fairly obvious: > pmemload addr "myfile" > > with the assumption that loads the whole file. I tried to keep it as similar to the existing functions "memsave"/"pmemsave", only adding "offset". Eric Blake already raised this issue in the thread, here's my response for reference: On Mon, Apr 23, 2018 at 02:46:57PM -0500, Eric Blake wrote: > Back-compat in the QMP interface matters. The HMP interface, however, > exists to serve humans not machines, and we can break it at will to > something that makes more sense to humans. So don't let HMP concerns > hold you back from a sane interface. I see. However I don't like breaking existing interfaces unless I have to. In this case I think not having the optional parameters is fine and the consistency between the existing memsave/pmemsave functions is more important. > Optional parameters are listed as '*name':'type' in the .json file, > which tells the generator to create a 'has_name' bool parameter > alongside the 'name' parameter in the C code for the QMP interface. The > HMP interface should then call into the QMP interface. > > Recent HMP patches that I've authored may offer some inspiration: commit > 08fb10a added a new command, and commit dba4932 added an optional > parameter to an existing command. Thank you for the explanation, this looks straight-forward. Do you have strong opinions regarding the optional parameters or would you accept the patch as is (minus possible implementation issues)? I like the symmetry to the existing functions (I noticed that size can only be optional for pmemload because saving the complete memory doesn't sound useful) and having to specify size/offset doesn't hurt the caller too much. Thanks for your review! Regards Simon PS: Diff between v3 and my current local version follows: diff --git a/hmp-commands.hx b/hmp-commands.hx index 5a43dae133..c39d745a22 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -818,7 +818,7 @@ ETEXI { .name = "memsave", - .args_type = "val:l,size:i,filename:s", + .args_type = "val:l,size:l,filename:F", .params = "addr size file", .help = "save to disk virtual memory dump starting at 'addr' of size 'size'", .cmd = hmp_memsave, @@ -832,7 +832,7 @@ ETEXI { .name = "pmemsave", - .args_type = "val:l,size:i,filename:s", + .args_type = "val:l,size:l,filename:F", .params = "addr size file", .help = "save to disk physical memory dump starting at 'addr' of size 'size'", .cmd = hmp_pmemsave, @@ -846,7 +846,7 @@ ETEXI { .name = "pmemload", - .args_type = "val:l,size:i,offset:i,filename:s", + .args_type = "val:l,size:l,offset:l,filename:F", .params = "addr size offset file", .help = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'", .cmd = hmp_pmemload, diff --git a/qapi/misc.json b/qapi/misc.json index 6c34b2ff8b..becc257a76 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -1196,7 +1196,7 @@ # # Returns: Nothing on success # -# Since: 2.13 +# Since: 3.1 ## { 'command': 'pmemload', 'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }
Simon Ruderich <simon@ruderich.org> writes: > On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: >>> --- a/hmp-commands.hx >>> +++ b/hmp-commands.hx Subject claims "qmp: add", but the patch also adds to hmp. Recommend to split the patch into QMP and HMP part. >>> @@ -822,6 +822,20 @@ STEXI >>> @item pmemsave @var{addr} @var{size} @var{file} >>> @findex pmemsave >>> save to disk physical memory dump starting at @var{addr} of size @var{size}. >>> +ETEXI >>> + >>> + { >>> + .name = "pmemload", >>> + .args_type = "val:l,size:i,offset:i,filename:s", >>> + .params = "addr size offset file", >>> + .help = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'", >>> + .cmd = hmp_pmemload, >>> + }, >>> + >> >> I'm guessing that size and offset should be 'l' to allow large >> sizes and offsets, and there's an 'F' type for filenames > > I've copied that from "pmemsave" and "memsave" which use 'i' for > size. I'll add patches which will adapt them to use both 'l' and > 'F' and adapt my pmemload patch as well. > > qapi/misc.json seems to always use 'int' for integer types. Is > this value large enough on 64-bit architectures? Yes. QAPI's int translates to int64_t. >> (see monitor.c which has a big comment table near the start). > > Thanks. > > Just curious, what is the difference between 's' and 'F'. Is that > only for documentation purposes (and maybe tab completion) or is > the usage different? I noticed existing code uses qdict_get_str() > for both 's' and 'F'. The main behavioral difference is completion. >> Also, had you considered rearranging and making them optional, >> for example if you do: >> >> val:l,filename:F,offset:i?,size:i? >> >> I think that would mean you can do the fairly obvious: >> pmemload addr "myfile" >> >> with the assumption that loads the whole file. > > I tried to keep it as similar to the existing functions > "memsave"/"pmemsave", only adding "offset". Eric Blake already > raised this issue in the thread, here's my response for > reference: > > On Mon, Apr 23, 2018 at 02:46:57PM -0500, Eric Blake wrote: >> Back-compat in the QMP interface matters. The HMP interface, however, >> exists to serve humans not machines, and we can break it at will to >> something that makes more sense to humans. So don't let HMP concerns >> hold you back from a sane interface. > > I see. However I don't like breaking existing interfaces unless I > have to. In this case I think not having the optional parameters > is fine and the consistency between the existing memsave/pmemsave > functions is more important. > >> Optional parameters are listed as '*name':'type' in the .json file, >> which tells the generator to create a 'has_name' bool parameter >> alongside the 'name' parameter in the C code for the QMP interface. The >> HMP interface should then call into the QMP interface. >> >> Recent HMP patches that I've authored may offer some inspiration: commit >> 08fb10a added a new command, and commit dba4932 added an optional >> parameter to an existing command. > > Thank you for the explanation, this looks straight-forward. > > Do you have strong opinions regarding the optional parameters or > would you accept the patch as is (minus possible implementation > issues)? I like the symmetry to the existing functions (I noticed > that size can only be optional for pmemload because saving the > complete memory doesn't sound useful) and having to specify > size/offset doesn't hurt the caller too much. I recommend to start with the QMP interface. Parameters are unordered there. memsave and pmemsave both take mandatory @val, @size, @filename. memsave additionally takes optional @cpu-index. Your pmemload has pmemsave's arguments plus and mandatory @offset. Rationale for adding @offset? You may have answered this question already; pointer to that answer would be fine. Once we got the QMP interface nailed down, we can move to the HMP interface. > Thanks for your review! > > Regards > Simon > > PS: Diff between v3 and my current local version follows: > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 5a43dae133..c39d745a22 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -818,7 +818,7 @@ ETEXI > > { > .name = "memsave", > - .args_type = "val:l,size:i,filename:s", > + .args_type = "val:l,size:l,filename:F", > .params = "addr size file", > .help = "save to disk virtual memory dump starting at 'addr' of size 'size'", > .cmd = hmp_memsave, > @@ -832,7 +832,7 @@ ETEXI > > { > .name = "pmemsave", > - .args_type = "val:l,size:i,filename:s", > + .args_type = "val:l,size:l,filename:F", > .params = "addr size file", > .help = "save to disk physical memory dump starting at 'addr' of size 'size'", > .cmd = hmp_pmemsave, These two should become a separate bug fix patch. The bug being fixed is completion. > @@ -846,7 +846,7 @@ ETEXI > > { > .name = "pmemload", > - .args_type = "val:l,size:i,offset:i,filename:s", > + .args_type = "val:l,size:l,offset:l,filename:F", > .params = "addr size offset file", > .help = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'", > .cmd = hmp_pmemload, > diff --git a/qapi/misc.json b/qapi/misc.json > index 6c34b2ff8b..becc257a76 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -1196,7 +1196,7 @@ > # > # Returns: Nothing on success > # > -# Since: 2.13 > +# Since: 3.1 > ## > { 'command': 'pmemload', > 'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }
On Tue, Aug 14, 2018 at 05:49:12PM +0200, Markus Armbruster wrote: >> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: >>>> --- a/hmp-commands.hx >>>> +++ b/hmp-commands.hx > > Subject claims "qmp: add", but the patch also adds to hmp. Recommend to > split the patch into QMP and HMP part. Hello, Sure, I can do that. >> qapi/misc.json seems to always use 'int' for integer types. Is >> this value large enough on 64-bit architectures? > > Yes. QAPI's int translates to int64_t. Thanks. >> Just curious, what is the difference between 's' and 'F'. Is that >> only for documentation purposes (and maybe tab completion) or is >> the usage different? I noticed existing code uses qdict_get_str() >> for both 's' and 'F'. > > The main behavioral difference is completion. Good to know, thanks. > I recommend to start with the QMP interface. Parameters are unordered > there. memsave and pmemsave both take mandatory @val, @size, @filename. > memsave additionally takes optional @cpu-index. Yes. > Your pmemload has pmemsave's arguments plus and mandatory @offset. > Rationale for adding @offset? You may have answered this question > already; pointer to that answer would be fine. My initial patch didn't have the offset. It was suggested by Eric Blake in <0a960aa8-2a3f-8667-3d46-cecf8e65e482@redhat.com>: On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote: > Do you additionally need an offset where to start reading from within > the file (that is, since you already have the 'size' parameter to avoid > reading the entire file, and the 'val' parameter to target anywhere in > physical memory, how do I start reading anywhere from the file)? It sounded useful to me so I added it. > Once we got the QMP interface nailed down, we can move to the HMP > interface. Good point. > These two should become a separate bug fix patch. The bug being fixed > is completion. Sure, they are in separate patches. Just wanted to show the general changes I applied from the reviews. Thanks for the review. Regards Simon
* Simon Ruderich (simon@ruderich.org) wrote: > On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: > >> --- a/hmp-commands.hx > >> +++ b/hmp-commands.hx > >> @@ -822,6 +822,20 @@ STEXI > >> @item pmemsave @var{addr} @var{size} @var{file} > >> @findex pmemsave > >> save to disk physical memory dump starting at @var{addr} of size @var{size}. > >> +ETEXI > >> + > >> + { > >> + .name = "pmemload", > >> + .args_type = "val:l,size:i,offset:i,filename:s", > >> + .params = "addr size offset file", > >> + .help = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'", > >> + .cmd = hmp_pmemload, > >> + }, > >> + > > > > I'm guessing that size and offset should be 'l' to allow large > > sizes and offsets, and there's an 'F' type for filenames > > I've copied that from "pmemsave" and "memsave" which use 'i' for > size. I'll add patches which will adapt them to use both 'l' and > 'F' and adapt my pmemload patch as well. > > qapi/misc.json seems to always use 'int' for integer types. Is > this value large enough on 64-bit architectures? > > > (see monitor.c which has a big comment table near the start). > > Thanks. > > Just curious, what is the difference between 's' and 'F'. Is that > only for documentation purposes (and maybe tab completion) or is > the usage different? I noticed existing code uses qdict_get_str() > for both 's' and 'F'. > > > Also, had you considered rearranging and making them optional, > > for example if you do: > > > > val:l,filename:F,offset:i?,size:i? > > > > I think that would mean you can do the fairly obvious: > > pmemload addr "myfile" > > > > with the assumption that loads the whole file. > > I tried to keep it as similar to the existing functions > "memsave"/"pmemsave", only adding "offset". Eric Blake already > raised this issue in the thread, here's my response for > reference: > > On Mon, Apr 23, 2018 at 02:46:57PM -0500, Eric Blake wrote: > > Back-compat in the QMP interface matters. The HMP interface, however, > > exists to serve humans not machines, and we can break it at will to > > something that makes more sense to humans. So don't let HMP concerns > > hold you back from a sane interface. > > I see. However I don't like breaking existing interfaces unless I > have to. In this case I think not having the optional parameters > is fine and the consistency between the existing memsave/pmemsave > functions is more important. > > > Optional parameters are listed as '*name':'type' in the .json file, > > which tells the generator to create a 'has_name' bool parameter > > alongside the 'name' parameter in the C code for the QMP interface. The > > HMP interface should then call into the QMP interface. > > > > Recent HMP patches that I've authored may offer some inspiration: commit > > 08fb10a added a new command, and commit dba4932 added an optional > > parameter to an existing command. > > Thank you for the explanation, this looks straight-forward. > > Do you have strong opinions regarding the optional parameters or > would you accept the patch as is (minus possible implementation > issues)? I like the symmetry to the existing functions (I noticed > that size can only be optional for pmemload because saving the > complete memory doesn't sound useful) and having to specify > size/offset doesn't hurt the caller too much. No strong feeling either way; that was just a suggestion. Dave > > Thanks for your review! > > Regards > Simon > > PS: Diff between v3 and my current local version follows: > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 5a43dae133..c39d745a22 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -818,7 +818,7 @@ ETEXI > > { > .name = "memsave", > - .args_type = "val:l,size:i,filename:s", > + .args_type = "val:l,size:l,filename:F", > .params = "addr size file", > .help = "save to disk virtual memory dump starting at 'addr' of size 'size'", > .cmd = hmp_memsave, > @@ -832,7 +832,7 @@ ETEXI > > { > .name = "pmemsave", > - .args_type = "val:l,size:i,filename:s", > + .args_type = "val:l,size:l,filename:F", > .params = "addr size file", > .help = "save to disk physical memory dump starting at 'addr' of size 'size'", > .cmd = hmp_pmemsave, > @@ -846,7 +846,7 @@ ETEXI > > { > .name = "pmemload", > - .args_type = "val:l,size:i,offset:i,filename:s", > + .args_type = "val:l,size:l,offset:l,filename:F", > .params = "addr size offset file", > .help = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'", > .cmd = hmp_pmemload, > diff --git a/qapi/misc.json b/qapi/misc.json > index 6c34b2ff8b..becc257a76 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -1196,7 +1196,7 @@ > # > # Returns: Nothing on success > # > -# Since: 2.13 > +# Since: 3.1 > ## > { 'command': 'pmemload', > 'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} } > > -- > + privacy is necessary > + using gnupg http://gnupg.org > + public key id: 0x92FEFDB7E44C32F9 -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Simon Ruderich <simon@ruderich.org> writes: > On Tue, Aug 14, 2018 at 05:49:12PM +0200, Markus Armbruster wrote: >>> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: >>>>> --- a/hmp-commands.hx >>>>> +++ b/hmp-commands.hx >> >> Subject claims "qmp: add", but the patch also adds to hmp. Recommend to >> split the patch into QMP and HMP part. > > Hello, > > Sure, I can do that. > >>> qapi/misc.json seems to always use 'int' for integer types. Is >>> this value large enough on 64-bit architectures? >> >> Yes. QAPI's int translates to int64_t. > > Thanks. > >>> Just curious, what is the difference between 's' and 'F'. Is that >>> only for documentation purposes (and maybe tab completion) or is >>> the usage different? I noticed existing code uses qdict_get_str() >>> for both 's' and 'F'. >> >> The main behavioral difference is completion. > > Good to know, thanks. > >> I recommend to start with the QMP interface. Parameters are unordered >> there. memsave and pmemsave both take mandatory @val, @size, @filename. >> memsave additionally takes optional @cpu-index. > > Yes. > >> Your pmemload has pmemsave's arguments plus and mandatory @offset. >> Rationale for adding @offset? You may have answered this question >> already; pointer to that answer would be fine. > > My initial patch didn't have the offset. It was suggested by Eric > Blake in <0a960aa8-2a3f-8667-3d46-cecf8e65e482@redhat.com>: > > On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote: >> Do you additionally need an offset where to start reading from within >> the file (that is, since you already have the 'size' parameter to avoid >> reading the entire file, and the 'val' parameter to target anywhere in >> physical memory, how do I start reading anywhere from the file)? > > It sounded useful to me so I added it. Feels like an optional parameter to me. >> Once we got the QMP interface nailed down, we can move to the HMP >> interface. > > Good point. > >> These two should become a separate bug fix patch. The bug being fixed >> is completion. > > Sure, they are in separate patches. Just wanted to show the > general changes I applied from the reviews. > > Thanks for the review. > > Regards > Simon
On Wed, Aug 15, 2018 at 06:22:51AM +0200, Markus Armbruster wrote: >> My initial patch didn't have the offset. It was suggested by Eric >> Blake in <0a960aa8-2a3f-8667-3d46-cecf8e65e482@redhat.com>: >> >> On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote: >>> Do you additionally need an offset where to start reading from within >>> the file (that is, since you already have the 'size' parameter to avoid >>> reading the entire file, and the 'val' parameter to target anywhere in >>> physical memory, how do I start reading anywhere from the file)? >> >> It sounded useful to me so I added it. > > Feels like an optional parameter to me. For the HMP or the QMP interface? If you think 'offset' is not necessary I can also drop it completely. Regards Simon
Simon Ruderich <simon@ruderich.org> writes: > On Wed, Aug 15, 2018 at 06:22:51AM +0200, Markus Armbruster wrote: >>> My initial patch didn't have the offset. It was suggested by Eric >>> Blake in <0a960aa8-2a3f-8667-3d46-cecf8e65e482@redhat.com>: >>> >>> On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote: >>>> Do you additionally need an offset where to start reading from within >>>> the file (that is, since you already have the 'size' parameter to avoid >>>> reading the entire file, and the 'val' parameter to target anywhere in >>>> physical memory, how do I start reading anywhere from the file)? >>> >>> It sounded useful to me so I added it. >> >> Feels like an optional parameter to me. > > For the HMP or the QMP interface? Both. > If you think 'offset' is not necessary I can also drop it > completely. I think it's a reasonable feature, and since you already coded it up...
On Wed, Aug 15, 2018 at 04:29:25PM +0200, Markus Armbruster wrote: >> For the HMP or the QMP interface? > > Both. Ok. >> If you think 'offset' is not necessary I can also drop it >> completely. > > I think it's a reasonable feature, and since you already coded it up... In that case, should size also become optional? As already suggested in this thread (and similar for QMP): On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: > Also, had you considered rearranging and making them optional, > for example if you do: > > val:l,filename:F,offset:i?,size:i? > > I think that would mean you can do the fairly obvious: > pmemload addr "myfile" > > with the assumption that loads the whole file. This would deviate from pmemsave/memsave, but feels more natural. How are multiple optional parameters handled? Filled from left-to-right? Regards Simon
Simon Ruderich <simon@ruderich.org> writes: > On Wed, Aug 15, 2018 at 04:29:25PM +0200, Markus Armbruster wrote: >>> For the HMP or the QMP interface? >> >> Both. > > Ok. > >>> If you think 'offset' is not necessary I can also drop it >>> completely. >> >> I think it's a reasonable feature, and since you already coded it up... > > In that case, should size also become optional? As already > suggested in this thread (and similar for QMP): > > On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: >> Also, had you considered rearranging and making them optional, >> for example if you do: >> >> val:l,filename:F,offset:i?,size:i? >> >> I think that would mean you can do the fairly obvious: >> pmemload addr "myfile" >> >> with the assumption that loads the whole file. > > This would deviate from pmemsave/memsave, but feels more natural. The different order or arguments in HMP is somewhat ugly. Okay if it makes the command more pleasant to use. Up to you and Dave to decide. If you decide to deviate, consider filename:F,address:l,size:i?,offset:i? > How are multiple optional parameters handled? Filled from > left-to-right? HMP: yes. QMP has only named parameters.
On Thu, Aug 16, 2018 at 07:43:41AM +0200, Markus Armbruster wrote: >> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: >>> Also, had you considered rearranging and making them optional, >>> for example if you do: >>> >>> val:l,filename:F,offset:i?,size:i? >>> >>> I think that would mean you can do the fairly obvious: >>> pmemload addr "myfile" >>> >>> with the assumption that loads the whole file. >> >> This would deviate from pmemsave/memsave, but feels more natural. > > The different order or arguments in HMP is somewhat ugly. Okay if it > makes the command more pleasant to use. Up to you and Dave to decide. > > If you decide to deviate, consider > > filename:F,address:l,size:i?,offset:i? From what I understand we can't have optional arguments in the middle. Would you prefer mandatory size/offset parameters in HMP or optional parameters but inconsistent with pmemsave/memsave? Both works for me. Regards Simon
Simon Ruderich <simon@ruderich.org> writes: > On Thu, Aug 16, 2018 at 07:43:41AM +0200, Markus Armbruster wrote: >>> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: >>>> Also, had you considered rearranging and making them optional, >>>> for example if you do: >>>> >>>> val:l,filename:F,offset:i?,size:i? >>>> >>>> I think that would mean you can do the fairly obvious: >>>> pmemload addr "myfile" >>>> >>>> with the assumption that loads the whole file. >>> >>> This would deviate from pmemsave/memsave, but feels more natural. >> >> The different order or arguments in HMP is somewhat ugly. Okay if it >> makes the command more pleasant to use. Up to you and Dave to decide. >> >> If you decide to deviate, consider >> >> filename:F,address:l,size:i?,offset:i? > > From what I understand we can't have optional arguments in the > middle. Would you prefer mandatory size/offset parameters in HMP > or optional parameters but inconsistent with pmemsave/memsave? If you guys decide you want consistency with memsave, that's fine with me. If you decide you want to rearrange arguments for better usability, that's also fine. I just wanted to throw in another rearrangement for you to consider.
On Thu, Aug 16, 2018 at 10:26:18AM +0200, Markus Armbruster wrote: >> On Thu, Aug 16, 2018 at 07:43:41AM +0200, Markus Armbruster wrote: >>>> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: >>>>> Also, had you considered rearranging and making them optional, >>>>> for example if you do: >>>>> >>>>> val:l,filename:F,offset:i?,size:i? >>>>> >>>>> I think that would mean you can do the fairly obvious: >>>>> pmemload addr "myfile" >>>>> >>>>> with the assumption that loads the whole file. >>>> >>>> This would deviate from pmemsave/memsave, but feels more natural. >>> >>> The different order or arguments in HMP is somewhat ugly. Okay if it >>> makes the command more pleasant to use. Up to you and Dave to decide. >>> >>> If you decide to deviate, consider >>> >>> filename:F,address:l,size:i?,offset:i? >> >> From what I understand we can't have optional arguments in the >> middle. Would you prefer mandatory size/offset parameters in HMP >> or optional parameters but inconsistent with pmemsave/memsave? > > If you guys decide you want consistency with memsave, that's fine with > me. If you decide you want to rearrange arguments for better usability, > that's also fine. I just wanted to throw in another rearrangement for > you to consider. Thanks for your suggestions. Then I'll go with "val:l,size:l,offset:l,filename:F" for now. I'll send the revised patch series soon. Regards Simon
diff --git a/cpus.c b/cpus.c index 49d4d44916..9b105336af 100644 --- a/cpus.c +++ b/cpus.c @@ -2367,6 +2367,47 @@ exit: qemu_close(fd); } +void qmp_pmemload(int64_t addr, int64_t size, int64_t offset, + const char *filename, Error **errp) +{ + int fd; + size_t l; + ssize_t r; + uint8_t buf[1024]; + + fd = qemu_open(filename, O_RDONLY | O_BINARY); + if (fd < 0) { + error_setg_file_open(errp, errno, filename); + return; + } + if (offset > 0) { + if (lseek(fd, offset, SEEK_SET) != offset) { + error_setg_errno(errp, errno, + "could not seek to offset %" PRIx64, offset); + goto exit; + } + } + + while (size != 0) { + l = sizeof(buf); + if (l > size) { + l = size; + } + r = read(fd, buf, l); + if (r <= 0) { + error_setg(errp, QERR_IO_ERROR); + goto exit; + } + l = r; /* in case of short read */ + cpu_physical_memory_write(addr, buf, l); + addr += l; + size -= l; + } + +exit: + qemu_close(fd); +} + void qmp_inject_nmi(Error **errp) { nmi_monitor_handle(monitor_get_cpu_index(), errp); diff --git a/hmp-commands.hx b/hmp-commands.hx index 0734fea931..84647c7c1d 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -822,6 +822,20 @@ STEXI @item pmemsave @var{addr} @var{size} @var{file} @findex pmemsave save to disk physical memory dump starting at @var{addr} of size @var{size}. +ETEXI + + { + .name = "pmemload", + .args_type = "val:l,size:i,offset:i,filename:s", + .params = "addr size offset file", + .help = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'", + .cmd = hmp_pmemload, + }, + +STEXI +@item pmemload @var{addr} @var{size} @var{offset} @var{file} +@findex pmemload +load from disk physical memory dump starting at @var{addr} of size @var{size} at file offset @var{offset}. ETEXI { diff --git a/hmp.c b/hmp.c index a4d28913bb..b85c943b63 100644 --- a/hmp.c +++ b/hmp.c @@ -1105,6 +1105,18 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } +void hmp_pmemload(Monitor *mon, const QDict *qdict) +{ + uint64_t size = qdict_get_int(qdict, "size"); + uint64_t offset = qdict_get_int(qdict, "offset"); + const char *filename = qdict_get_str(qdict, "filename"); + uint64_t addr = qdict_get_int(qdict, "val"); + Error *err = NULL; + + qmp_pmemload(addr, size, offset, filename, &err); + hmp_handle_error(mon, &err); +} + void hmp_ringbuf_write(Monitor *mon, const QDict *qdict) { const char *chardev = qdict_get_str(qdict, "device"); diff --git a/hmp.h b/hmp.h index 20f27439d3..31767ea4a8 100644 --- a/hmp.h +++ b/hmp.h @@ -47,6 +47,7 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict); void hmp_cpu(Monitor *mon, const QDict *qdict); void hmp_memsave(Monitor *mon, const QDict *qdict); void hmp_pmemsave(Monitor *mon, const QDict *qdict); +void hmp_pmemload(Monitor *mon, const QDict *qdict); void hmp_ringbuf_write(Monitor *mon, const QDict *qdict); void hmp_ringbuf_read(Monitor *mon, const QDict *qdict); void hmp_cont(Monitor *mon, const QDict *qdict); diff --git a/qapi/misc.json b/qapi/misc.json index f5988cc0b5..b4c0065b02 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -1219,6 +1219,26 @@ { 'command': 'pmemsave', 'data': {'val': 'int', 'size': 'int', 'filename': 'str'} } +## +# @pmemload: +# +# Load a portion of guest physical memory from a file. +# +# @val: the physical address of the guest to start from +# +# @size: the size of memory region to load +# +# @offset: the offset in the file to start from +# +# @filename: the file to load the memory from as binary data +# +# Returns: Nothing on success +# +# Since: 2.13 +## +{ 'command': 'pmemload', + 'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} } + ## # @cont: #
Adapted patch from Baojun Wang [1] with the following commit message: I found this could be useful to have qemu-softmmu as a cross debugger (launch with -s -S command line option), then if we can have a command to load guest physical memory, we can use cross gdb to do some target debug which gdb cannot do directly. pmemload is necessary to directly write physical memory which is not possible with gdb alone as it uses only logical addresses. The QAPI for pmemload uses "val" as parameter name for the physical address. This name is not very descriptive but is consistent with the existing pmemsave. Changing the parameter name of pmemsave is not possible without breaking the existing API. [1]: https://lists.gnu.org/archive/html/qemu-trivial/2014-04/msg00074.html Based-on-patch-by: Baojun Wang <wangbj@gmail.com> Signed-off-by: Simon Ruderich <simon@ruderich.org> --- cpus.c | 41 +++++++++++++++++++++++++++++++++++++++++ hmp-commands.hx | 14 ++++++++++++++ hmp.c | 12 ++++++++++++ hmp.h | 1 + qapi/misc.json | 20 ++++++++++++++++++++ 5 files changed, 88 insertions(+)