[libvirt] [PATCH] Support physical memory in virDomainMemoryPeek()

[Nguyen Anh Quynh]

On Wed, Jul 22, 2009 at 4:19 PM, Nguyen Anh Quynh<aquynh at gmail.com> wrote:
> On Wed, Jul 22, 2009 at 4:02 PM, Daniel Veillard<veillard at redhat.com> wrote:
>> On Wed, Jul 22, 2009 at 12:36:10PM +0900, Nguyen Anh Quynh wrote:
>>> Hi,
>>>
>>> This patch provides support for physical memory in
>>> virDomainMemoryPeek(). Please consider applying.
>>>
>>> Signed-off-by: Nguyen Anh Quynh <aquynh at gmail.com>
>>
>>  okay a couple of comments:
>>
>>> # diffstat pmemsave3.diff
>>>  docs/libvirt-api.xml         |    2 +-
>>>  include/libvirt/libvirt.h.in |    1 +
>>>  src/libvirt.c                |   14 +++++---------
>>>  src/qemu_driver.c            |   17 ++++++++---------
>>>  4 files changed, 15 insertions(+), 19 deletions(-)
>>
>>> diff --git a/docs/libvirt-api.xml b/docs/libvirt-api.xml
>>> index 8ded57a..04b1108 100644
>>> --- a/docs/libvirt-api.xml
>>> +++ b/docs/libvirt-api.xml
>>
>>  This is generated, once the comment in libvirt.c is updated this is
>> extracted automatically (cd docs ; make rebuild), no need to add it to
>> the patch
>>
>>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>>> index ba2b6f0..e6536c7 100644
>>> --- a/include/libvirt/libvirt.h.in
>>> +++ b/include/libvirt/libvirt.h.in
>>> @@ -619,6 +619,7 @@ int                     virDomainBlockPeek (virDomainPtr dom,
>>>  /* Memory peeking flags. */
>>>  typedef enum {
>>>    VIR_MEMORY_VIRTUAL              = 1, /* addresses are virtual addresses */
>>> +  VIR_MEMORY_PHYSICAL             = 2, /* addresses are physical addresses */
>>>  } virDomainMemoryFlags;
>>>
>>>  int                     virDomainMemoryPeek (virDomainPtr dom,
>>> diff --git a/src/libvirt.c b/src/libvirt.c
>>> index f4a7fa7..393d0c1 100644
>>> --- a/src/libvirt.c
>>> +++ b/src/libvirt.c
>>> @@ -3815,19 +3815,20 @@ virDomainMemoryPeek (virDomainPtr dom,
>>>          goto error;
>>>      }
>>>
>>> -    /* Flags must be VIR_MEMORY_VIRTUAL at the moment.
>>> -     *
>>> -     * Note on access to physical memory: A VIR_MEMORY_PHYSICAL flag is
>>> +    /* Note on access to physical memory: A VIR_MEMORY_PHYSICAL flag is
>>>       * a possibility.  However it isn't really useful unless the caller
>>>       * can also access registers, particularly CR3 on x86 in order to
>>>       * get the Page Table Directory.  Since registers are different on
>>>       * every architecture, that would imply another call to get the
>>>       * machine registers.
>>>       *
>>> -     * The QEMU driver handles only VIR_MEMORY_VIRTUAL, mapping it
>>> +     * The QEMU driver handles VIR_MEMORY_VIRTUAL, mapping it
>>>       * to the qemu 'memsave' command which does the virtual to physical
>>>       * mapping inside qemu.
>>>       *
>>> +     * The QEMU driver also handles VIR_MEMORY_PHYSICAL, mapping it
>>> +     * to the qemu 'pmemsave' command.
>>> +     *
>>>       * At time of writing there is no Xen driver.  However the Xen
>>>       * hypervisor only lets you map physical pages from other domains,
>>>       * and so the Xen driver would have to do the virtual to physical
>>
>>  Okay
>>
>>> @@ -3836,11 +3837,6 @@ virDomainMemoryPeek (virDomainPtr dom,
>>>       * which does this, although we cannot copy this code directly
>>>       * because of incompatible licensing.
>>>       */
>>> -    if (flags != VIR_MEMORY_VIRTUAL) {
>>> -        virLibDomainError (dom, VIR_ERR_INVALID_ARG,
>>> -                           _("flags parameter must be VIR_MEMORY_VIRTUAL"));
>>> -        goto error;
>>> -    }
>>
>>  The check should be preserved as
>>    if ((flags != VIR_MEMORY_VIRTUAL) && (flags != VIR_MEMORY_PHYSICAL)) {
>> and update the error message
>>
>
> Yes, this makes sense.
>
>>>      /* Allow size == 0 as an access test. */
>>>      if (size > 0 && !buffer) {
>>> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
>>> index 00dc6e5..995cbee 100644
>>> --- a/src/qemu_driver.c
>>> +++ b/src/qemu_driver.c
>>> @@ -5181,12 +5181,6 @@ qemudDomainMemoryPeek (virDomainPtr dom,
>>>          goto cleanup;
>>>      }
>>>
>>> -    if (flags != VIR_MEMORY_VIRTUAL) {
>>> -        qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
>>> -                          "%s", _("QEMU driver only supports virtual memory addrs"));
>>> -        goto cleanup;
>>> -    }
>>> -
>>
>>  I tend to think the check should be modified similary here
>>
>>>      if (!virDomainIsActive(vm)) {
>>>          qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_INVALID,
>>>                           "%s", _("domain is not running"));
>>> @@ -5200,15 +5194,20 @@ qemudDomainMemoryPeek (virDomainPtr dom,
>>>          goto cleanup;
>>>      }
>>>
>>> -    /* Issue the memsave command. */
>>> -    snprintf (cmd, sizeof cmd, "memsave %llu %zi \"%s\"", offset, size, tmp);
>>> +    if (flags == VIR_MEMORY_VIRTUAL)
>>> +        /* Issue the memsave command. */
>>> +        snprintf (cmd, sizeof cmd, "memsave %llu %zi \"%s\"", offset, size, tmp);
>>> +    else
>>> +        /* flags == VIR_MEMORY_PHYSICAL, issue the pmemsave command. */
>>> +        snprintf (cmd, sizeof cmd, "pmemsave %llu %zi \"%s\"", offset, size, tmp);
>>> +
>>
>>  Then having no error handling here would make sense, but currently
>> if you pass a wrong argument you would just silently assume flags ==
>> VIR_MEMORY_PHYSICAL, so the patch as-is is not correct.
>
> Actually I dont think it is necessary to do sanity check inside the
> driver, as it is already checked outside, inside virDomainMemoryPeek()
> (I will fix the patch, as agreed above).
>
> So do you really want to check it again here?

Acutally, to avoid all those ugly sanity checks, it is best to define
VIR_MEMORY_* as an enum type, then redefine virDomainMemoryCheck() as
(note the last param is changed):

int virDomainMemoryPeek (virDomainPtr dom,
                                             unsigned long long start,
                                             size_t size,
                                             void *buffer,
                                             enum virDomainMemoryFlags flags);


I didnt do that in my patch because that might cause some
incompatibility to those applications using this function. But
technically, that is nicer, and correct way to implement this API.

Let me know your idea about this.

Thanks,
Quynh