| Message ID | 6f775e11a75a2faa1c66a86e6d23a97f695c2ca1.1523537181.git.simon@ruderich.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/5] cpus: correct coding style in qmp_memsave/qmp_pmemsave | expand |
On 04/12/2018 07:50 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> > --- Focusing on just the interface: > +++ b/qapi/misc.json > @@ -1185,6 +1185,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 Should size be an optional parameter (default read to the end of the file)? > +# > +# @offset: the offset in the file to start from Should offset be an optional parameter (default start reading from offset 0 of the file)? > +# > +# @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: > # >
On Tue, Apr 17, 2018 at 04:18:43PM -0500, Eric Blake wrote: > Focusing on just the interface: > >> +++ b/qapi/misc.json >> @@ -1185,6 +1185,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 > > Should size be an optional parameter (default read to the end of the file)? > >> +# >> +# @offset: the offset in the file to start from > > Should offset be an optional parameter (default start reading from > offset 0 of the file)? From the QAPI point-of-view making both optional seems like a good idea, however then pmemsave should also mark its "size" parameter optional for consistency (this is backwards compatible as existing callers are not affected). On the monitor API side this is more problematic as the order of the pmemsave/pmemload parameters make optional parameters impossible. Personally I'd keep it as is because it's simple and consistent with the existing interface (and can be changed in the future without breaking compatibility). In case you prefer those parameters to be optional I'll require some help to implement that: I don't understand how qapi-schema.json, hmp-commands.hx and hmp.h interact (I've to admit I just copied that from the original patch and didn't give it much thought as it worked fine) and would require some pointers on how to implement optional arguments for qapi-schema.json. Regards Simon
On 04/22/2018 04:47 AM, Simon Ruderich wrote: > On Tue, Apr 17, 2018 at 04:18:43PM -0500, Eric Blake wrote: >> Focusing on just the interface: >> >>> +++ b/qapi/misc.json >>> @@ -1185,6 +1185,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 >> >> Should size be an optional parameter (default read to the end of the file)? >> >>> +# >>> +# @offset: the offset in the file to start from >> >> Should offset be an optional parameter (default start reading from >> offset 0 of the file)? > >>From the QAPI point-of-view making both optional seems like a > good idea, however then pmemsave should also mark its "size" > parameter optional for consistency (this is backwards compatible > as existing callers are not affected). Correct. > > On the monitor API side this is more problematic as the order of > the pmemsave/pmemload parameters make optional parameters > impossible. 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. > > Personally I'd keep it as is because it's simple and consistent > with the existing interface (and can be changed in the future > without breaking compatibility). > > In case you prefer those parameters to be optional I'll require > some help to implement that: I don't understand how > qapi-schema.json, hmp-commands.hx and hmp.h interact (I've to > admit I just copied that from the original patch and didn't give > it much thought as it worked fine) and would require some > pointers on how to implement optional arguments for > qapi-schema.json. 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.
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. Regards Simon
diff --git a/cpus.c b/cpus.c index d256d8e9b4..c690dc205f 100644 --- a/cpus.c +++ b/cpus.c @@ -2314,6 +2314,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 35d862a5d2..a392d0e379 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 1e392055f7..c9b37dbfbd 100644 --- a/hmp.c +++ b/hmp.c @@ -1090,6 +1090,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 4e2ec375b0..0a69e371ca 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 5636f4a149..2255d219fa 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -1185,6 +1185,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(+)