[Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Paolo Bonzini]

These commands are useful when testing machine-check passthrough.
gpa2hva is useful to inject a MADV_HWPOISON madvise from gdb, while
gpa2hpa is useful to inject an error with the mce-inject kernel
module.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hmp-commands.hx |  32 ++++++++++++++++++
 monitor.c       | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8819281..0aca984 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -526,6 +526,38 @@ Dump 80 16 bit values at the start of the video memory.
 ETEXI
 
     {
+        .name       = "gpa2hva",
+        .args_type  = "addr:l",
+        .params     = "addr",
+        .help       = "print the host virtual address corresponding to a guest physical address",
+        .cmd        = hmp_gpa2hva,
+    },
+
+STEXI
+@item gpa2hva @var{addr}
+@findex gpa2hva
+Print the host virtual address at which the guest's physical address @var{addr}
+is mapped.
+ETEXI
+
+#ifdef CONFIG_LINUX
+    {
+        .name       = "gpa2hpa",
+        .args_type  = "addr:l",
+        .params     = "addr",
+        .help       = "print the host physical address corresponding to a guest physical address",
+        .cmd        = hmp_gpa2hpa,
+    },
+#endif
+
+STEXI
+@item gpa2hpa @var{addr}
+@findex gpa2hpa
+Print the host physical address at which the guest's physical address @var{addr}
+is mapped.
+ETEXI
+
+    {
         .name       = "p|print",
         .args_type  = "fmt:/,val:l",
         .params     = "/fmt expr",
diff --git a/monitor.c b/monitor.c
index be282ec..216a97a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1421,6 +1421,107 @@ static void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict)
     memory_dump(mon, count, format, size, addr, 1);
 }
 
+static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
+{
+    MemoryRegionSection mrs = memory_region_find(get_system_memory(),
+                                                 addr, 1);
+
+    if (!mrs.mr) {
+        error_setg(errp, "No memory is mapped at address 0x%" HWADDR_PRIx, addr);
+        return NULL;
+    }
+
+    if (!memory_region_is_ram(mrs.mr) && !memory_region_is_romd(mrs.mr)) {
+        error_setg(errp, "Memory at address 0x%" HWADDR_PRIx "is not RAM", addr);
+        memory_region_unref(mrs.mr);
+        return NULL;
+    }
+
+    *p_mr = mrs.mr;
+    return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_region);
+}
+
+static void hmp_gpa2hva(Monitor *mon, const QDict *qdict)
+{
+    hwaddr addr = qdict_get_int(qdict, "addr");
+    Error *local_err = NULL;
+    MemoryRegion *mr;
+    void *ptr;
+
+    ptr = gpa2hva(&mr, addr, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        return;
+    }
+
+    monitor_printf(mon, "Host virtual address for 0x%" HWADDR_PRIx
+                   " (%s) is %p\n",
+                   addr, mr->name, ptr);
+
+    memory_region_unref(mr);
+}
+
+#ifdef CONFIG_LINUX
+static uint64_t vtop(void *ptr, Error **errp)
+{
+    uint64_t pinfo;
+    uint64_t ret = -1;
+    uintptr_t addr = (uintptr_t) ptr;
+    uintptr_t pagesize = getpagesize();
+    off_t offset = addr / pagesize * sizeof(pinfo);
+    int fd;
+
+    fd = open("/proc/self/pagemap", O_RDONLY);
+    if (fd == -1) {
+        error_setg_errno(errp, errno, "Cannot open /proc/self/pagemap");
+        return -1;
+    }
+
+    /* Force copy-on-write if necessary.  */
+    atomic_add((uint8_t *)ptr, 0);
+
+    if (pread(fd, &pinfo, sizeof(pinfo), offset) != sizeof(pinfo)) {
+        error_setg_errno(errp, errno, "Cannot read pagemap");
+        goto out;
+    }
+    if ((pinfo & (1ull << 63)) == 0) {
+        error_setg(errp, "Page not present");
+        goto out;
+    }
+    ret = ((pinfo & 0x007fffffffffffffull) * pagesize) | (addr & (pagesize - 1));
+
+out:
+    close(fd);
+    return ret;
+}
+
+static void hmp_gpa2hpa(Monitor *mon, const QDict *qdict)
+{
+    hwaddr addr = qdict_get_int(qdict, "addr");
+    Error *local_err = NULL;
+    MemoryRegion *mr;
+    void *ptr;
+    uint64_t physaddr;
+
+    ptr = gpa2hva(&mr, addr, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        return;
+    }
+
+    physaddr = vtop(ptr, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+    } else {
+        monitor_printf(mon, "Host physical address for 0x%" HWADDR_PRIx
+                       " (%s) is 0x%" PRIx64 "\n",
+                       addr, mr->name, (uint64_t) physaddr);
+    }
+
+    memory_region_unref(mr);
+}
+#endif
+
 static void do_print(Monitor *mon, const QDict *qdict)
 {
     int format = qdict_get_int(qdict, "format");
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Peter Maydell]

On 20 March 2017 at 14:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
> These commands are useful when testing machine-check passthrough.
> gpa2hva is useful to inject a MADV_HWPOISON madvise from gdb, while
> gpa2hpa is useful to inject an error with the mce-inject kernel
> module.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hmp-commands.hx |  32 ++++++++++++++++++
>  monitor.c       | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 133 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8819281..0aca984 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -526,6 +526,38 @@ Dump 80 16 bit values at the start of the video memory.
>  ETEXI

I have some comments which feel kind of nit-picky, but since this
is a public-facing HMP API I think they need attention since we only
get one chance to get it right.

>      {
> +        .name       = "gpa2hva",
> +        .args_type  = "addr:l",
> +        .params     = "addr",
> +        .help       = "print the host virtual address corresponding to a guest physical address",
> +        .cmd        = hmp_gpa2hva,
> +    },

How does this work for guest CPUs which have more than one physical
address space (eg ARM TrustZone)? There's no ability here to specify
the secure/nonsecure transaction attribute that you need to distinguish
which kind of guest physical address you're talking about.

The command also doesn't let you specify which CPU you care about,
which is bad because they don't all have to have the same address map.

The documentation should also say what happens if the guest physaddr
doesn't correspond to RAM.

> +#ifdef CONFIG_LINUX
> +    {
> +        .name       = "gpa2hpa",
> +        .args_type  = "addr:l",
> +        .params     = "addr",
> +        .help       = "print the host physical address corresponding to a guest physical address",
> +        .cmd        = hmp_gpa2hpa,
> +    },
> +#endif
> +
> +STEXI
> +@item gpa2hpa @var{addr}
> +@findex gpa2hpa
> +Print the host physical address at which the guest's physical address @var{addr}
> +is mapped.

...what if you're on a system where host RAM exists at multiple host
physical addresses? What if the RAM happens to be paged out?
(Plus the remarks for gpa2hva apply.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> These commands are useful when testing machine-check passthrough.
> gpa2hva is useful to inject a MADV_HWPOISON madvise from gdb, while
> gpa2hpa is useful to inject an error with the mce-inject kernel
> module.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hmp-commands.hx |  32 ++++++++++++++++++
>  monitor.c       | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 133 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8819281..0aca984 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -526,6 +526,38 @@ Dump 80 16 bit values at the start of the video memory.
>  ETEXI
>  
>      {
> +        .name       = "gpa2hva",
> +        .args_type  = "addr:l",
> +        .params     = "addr",
> +        .help       = "print the host virtual address corresponding to a guest physical address",
> +        .cmd        = hmp_gpa2hva,
> +    },
> +
> +STEXI
> +@item gpa2hva @var{addr}
> +@findex gpa2hva
> +Print the host virtual address at which the guest's physical address @var{addr}
> +is mapped.
> +ETEXI
> +
> +#ifdef CONFIG_LINUX
> +    {
> +        .name       = "gpa2hpa",
> +        .args_type  = "addr:l",
> +        .params     = "addr",
> +        .help       = "print the host physical address corresponding to a guest physical address",
> +        .cmd        = hmp_gpa2hpa,
> +    },
> +#endif
> +
> +STEXI
> +@item gpa2hpa @var{addr}
> +@findex gpa2hpa
> +Print the host physical address at which the guest's physical address @var{addr}
> +is mapped.
> +ETEXI
> +
> +    {
>          .name       = "p|print",
>          .args_type  = "fmt:/,val:l",
>          .params     = "/fmt expr",
> diff --git a/monitor.c b/monitor.c
> index be282ec..216a97a 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1421,6 +1421,107 @@ static void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict)
>      memory_dump(mon, count, format, size, addr, 1);
>  }
>  
> +static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
> +{
> +    MemoryRegionSection mrs = memory_region_find(get_system_memory(),
> +                                                 addr, 1);
> +
> +    if (!mrs.mr) {
> +        error_setg(errp, "No memory is mapped at address 0x%" HWADDR_PRIx, addr);
> +        return NULL;
> +    }
> +
> +    if (!memory_region_is_ram(mrs.mr) && !memory_region_is_romd(mrs.mr)) {
> +        error_setg(errp, "Memory at address 0x%" HWADDR_PRIx "is not RAM", addr);
> +        memory_region_unref(mrs.mr);
> +        return NULL;
> +    }
> +
> +    *p_mr = mrs.mr;
> +    return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_region);
> +}
> +
> +static void hmp_gpa2hva(Monitor *mon, const QDict *qdict)
> +{
> +    hwaddr addr = qdict_get_int(qdict, "addr");
> +    Error *local_err = NULL;
> +    MemoryRegion *mr;
> +    void *ptr;
> +
> +    ptr = gpa2hva(&mr, addr, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Host virtual address for 0x%" HWADDR_PRIx
> +                   " (%s) is %p\n",
> +                   addr, mr->name, ptr);
> +
> +    memory_region_unref(mr);
> +}
> +
> +#ifdef CONFIG_LINUX
> +static uint64_t vtop(void *ptr, Error **errp)
> +{
> +    uint64_t pinfo;
> +    uint64_t ret = -1;
> +    uintptr_t addr = (uintptr_t) ptr;
> +    uintptr_t pagesize = getpagesize();
> +    off_t offset = addr / pagesize * sizeof(pinfo);
> +    int fd;
> +
> +    fd = open("/proc/self/pagemap", O_RDONLY);
> +    if (fd == -1) {
> +        error_setg_errno(errp, errno, "Cannot open /proc/self/pagemap");
> +        return -1;
> +    }
> +
> +    /* Force copy-on-write if necessary.  */
> +    atomic_add((uint8_t *)ptr, 0);
> +
> +    if (pread(fd, &pinfo, sizeof(pinfo), offset) != sizeof(pinfo)) {
> +        error_setg_errno(errp, errno, "Cannot read pagemap");
> +        goto out;
> +    }
> +    if ((pinfo & (1ull << 63)) == 0) {
> +        error_setg(errp, "Page not present");
> +        goto out;
> +    }
> +    ret = ((pinfo & 0x007fffffffffffffull) * pagesize) | (addr & (pagesize - 1));
> +
> +out:
> +    close(fd);
> +    return ret;
> +}
> +
> +static void hmp_gpa2hpa(Monitor *mon, const QDict *qdict)
> +{
> +    hwaddr addr = qdict_get_int(qdict, "addr");
> +    Error *local_err = NULL;
> +    MemoryRegion *mr;
> +    void *ptr;
> +    uint64_t physaddr;
> +
> +    ptr = gpa2hva(&mr, addr, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        return;
> +    }
> +
> +    physaddr = vtop(ptr, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +    } else {
> +        monitor_printf(mon, "Host physical address for 0x%" HWADDR_PRIx
> +                       " (%s) is 0x%" PRIx64 "\n",
> +                       addr, mr->name, (uint64_t) physaddr);
> +    }
> +
> +    memory_region_unref(mr);
> +}
> +#endif
> +
>  static void do_print(Monitor *mon, const QDict *qdict)
>  {
>      int format = qdict_get_int(qdict, "format");
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Paolo Bonzini]

On 20/03/2017 15:56, Peter Maydell wrote:
> On 20 March 2017 at 14:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> These commands are useful when testing machine-check passthrough.
>> gpa2hva is useful to inject a MADV_HWPOISON madvise from gdb, while
>> gpa2hpa is useful to inject an error with the mce-inject kernel
>> module.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hmp-commands.hx |  32 ++++++++++++++++++
>>  monitor.c       | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 133 insertions(+)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 8819281..0aca984 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -526,6 +526,38 @@ Dump 80 16 bit values at the start of the video memory.
>>  ETEXI
> 
> I have some comments which feel kind of nit-picky, but since this
> is a public-facing HMP API I think they need attention since we only
> get one chance to get it right.
> 
>>      {
>> +        .name       = "gpa2hva",
>> +        .args_type  = "addr:l",
>> +        .params     = "addr",
>> +        .help       = "print the host virtual address corresponding to a guest physical address",
>> +        .cmd        = hmp_gpa2hva,
>> +    },
> 
> How does this work for guest CPUs which have more than one physical
> address space (eg ARM TrustZone)? There's no ability here to specify
> the secure/nonsecure transaction attribute that you need to distinguish
> which kind of guest physical address you're talking about.
> 
> The command also doesn't let you specify which CPU you care about,
> which is bad because they don't all have to have the same address map.

It just uses address_space_memory currently.  I can make it use the
current CPU address space too, similar to x and xp; it's the same for my
own use.

> The documentation should also say what happens if the guest physaddr
> doesn't correspond to RAM.

An error? :)

>> +#ifdef CONFIG_LINUX
>> +    {
>> +        .name       = "gpa2hpa",
>> +        .args_type  = "addr:l",
>> +        .params     = "addr",
>> +        .help       = "print the host physical address corresponding to a guest physical address",
>> +        .cmd        = hmp_gpa2hpa,
>> +    },
>> +#endif
>> +
>> +STEXI
>> +@item gpa2hpa @var{addr}
>> +@findex gpa2hpa
>> +Print the host physical address at which the guest's physical address @var{addr}
>> +is mapped.
> 
> ...what if you're on a system where host RAM exists at multiple host
> physical addresses?

It gives whatever the host OS thinks it's the corresponding host
physical address.  It's what happens to be in the page tables for the HVA.

> What if the RAM happens to be paged out?
> (Plus the remarks for gpa2hva apply.)

Another error? :)

Paolo

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 20 March 2017 at 14:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> These commands are useful when testing machine-check passthrough.
>> gpa2hva is useful to inject a MADV_HWPOISON madvise from gdb, while
>> gpa2hpa is useful to inject an error with the mce-inject kernel
>> module.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hmp-commands.hx |  32 ++++++++++++++++++
>>  monitor.c       | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 133 insertions(+)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 8819281..0aca984 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -526,6 +526,38 @@ Dump 80 16 bit values at the start of the video memory.
>>  ETEXI
>
> I have some comments which feel kind of nit-picky, but since this
> is a public-facing HMP API I think they need attention since we only
> get one chance to get it right.

HMP is not a stable interface.  If we get it wrong, we change it.  If
our change breaks your usage, you get to keep the pieces.

That said, we prefer not to change HMP commands.  It's certainly better
to get it right from the start.

[...]

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Peter Maydell]

On 20 March 2017 at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> I have some comments which feel kind of nit-picky, but since this
>> is a public-facing HMP API I think they need attention since we only
>> get one chance to get it right.
>
> HMP is not a stable interface.  If we get it wrong, we change it.  If
> our change breaks your usage, you get to keep the pieces.

Oh yes, I had my HMP and QMP the wrong way round.

...which reminds me that I thought we preferred all HMP commands
to be implemented in terms of their QMP equivalent ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Paolo Bonzini]

On 20/03/2017 17:32, Peter Maydell wrote:
> On 20 March 2017 at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> I have some comments which feel kind of nit-picky, but since this
>>> is a public-facing HMP API I think they need attention since we only
>>> get one chance to get it right.
>>
>> HMP is not a stable interface.  If we get it wrong, we change it.  If
>> our change breaks your usage, you get to keep the pieces.
> 
> Oh yes, I had my HMP and QMP the wrong way round.
> 
> ...which reminds me that I thought we preferred all HMP commands
> to be implemented in terms of their QMP equivalent ?

Right, but we make exceptions for debug commands such as x or xp (and
these ones).

Paolo

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 20 March 2017 at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> I have some comments which feel kind of nit-picky, but since this
>>> is a public-facing HMP API I think they need attention since we only
>>> get one chance to get it right.
>>
>> HMP is not a stable interface.  If we get it wrong, we change it.  If
>> our change breaks your usage, you get to keep the pieces.
>
> Oh yes, I had my HMP and QMP the wrong way round.
>
> ...which reminds me that I thought we preferred all HMP commands
> to be implemented in terms of their QMP equivalent ?

Yes.  We make exceptions for commands we believe have no QMP use, such
as "print".  I figure the rationale for these is "just for testing".
Paolo, can you confirm?

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Paolo Bonzini]

On 20/03/2017 18:01, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On 20 March 2017 at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>> I have some comments which feel kind of nit-picky, but since this
>>>> is a public-facing HMP API I think they need attention since we only
>>>> get one chance to get it right.
>>>
>>> HMP is not a stable interface.  If we get it wrong, we change it.  If
>>> our change breaks your usage, you get to keep the pieces.
>>
>> Oh yes, I had my HMP and QMP the wrong way round.
>>
>> ...which reminds me that I thought we preferred all HMP commands
>> to be implemented in terms of their QMP equivalent ?
> 
> Yes.  We make exceptions for commands we believe have no QMP use, such
> as "print".  I figure the rationale for these is "just for testing".
> Paolo, can you confirm?

Yes.  If somebody comes up with a non-testing use, I suppose we can
always add a QMP variant.  I'll send v3 which uses the current monitor
CPU's address space according to Peter's review.

Paolo

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Paolo Bonzini]

On 20/03/2017 18:16, Paolo Bonzini wrote:
> 
> 
> On 20/03/2017 18:01, Markus Armbruster wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 20 March 2017 at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>> I have some comments which feel kind of nit-picky, but since this
>>>>> is a public-facing HMP API I think they need attention since we only
>>>>> get one chance to get it right.
>>>>
>>>> HMP is not a stable interface.  If we get it wrong, we change it.  If
>>>> our change breaks your usage, you get to keep the pieces.
>>>
>>> Oh yes, I had my HMP and QMP the wrong way round.
>>>
>>> ...which reminds me that I thought we preferred all HMP commands
>>> to be implemented in terms of their QMP equivalent ?
>>
>> Yes.  We make exceptions for commands we believe have no QMP use, such
>> as "print".  I figure the rationale for these is "just for testing".
>> Paolo, can you confirm?
> 
> Yes.  If somebody comes up with a non-testing use, I suppose we can
> always add a QMP variant.  I'll send v3 which uses the current monitor
> CPU's address space according to Peter's review.

Since there is no CPU method to transform a CPU state into a MemTxAttrs
value, and the MemTxAttrs are needed to get the right address space, v2
is the best I can do for 2.9.

Changing it to take the CPU state into account (e.g. include secure
memory when in secure mode) can be done in later releases if necessary.

David, are you queuing the patch?

Paolo

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Dr. David Alan Gilbert]

* Paolo Bonzini (bonzini@gnu.org) wrote:
> 
> 
> On 20/03/2017 18:16, Paolo Bonzini wrote:
> > 
> > 
> > On 20/03/2017 18:01, Markus Armbruster wrote:
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>
> >>> On 20 March 2017 at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
> >>>> Peter Maydell <peter.maydell@linaro.org> writes:
> >>>>> I have some comments which feel kind of nit-picky, but since this
> >>>>> is a public-facing HMP API I think they need attention since we only
> >>>>> get one chance to get it right.
> >>>>
> >>>> HMP is not a stable interface.  If we get it wrong, we change it.  If
> >>>> our change breaks your usage, you get to keep the pieces.
> >>>
> >>> Oh yes, I had my HMP and QMP the wrong way round.
> >>>
> >>> ...which reminds me that I thought we preferred all HMP commands
> >>> to be implemented in terms of their QMP equivalent ?
> >>
> >> Yes.  We make exceptions for commands we believe have no QMP use, such
> >> as "print".  I figure the rationale for these is "just for testing".
> >> Paolo, can you confirm?
> > 
> > Yes.  If somebody comes up with a non-testing use, I suppose we can
> > always add a QMP variant.  I'll send v3 which uses the current monitor
> > CPU's address space according to Peter's review.
> 
> Since there is no CPU method to transform a CPU state into a MemTxAttrs
> value, and the MemTxAttrs are needed to get the right address space, v2
> is the best I can do for 2.9.
> 
> Changing it to take the CPU state into account (e.g. include secure
> memory when in secure mode) can be done in later releases if necessary.
> 
> David, are you queuing the patch?

Yes, will do.

Dave

> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> These commands are useful when testing machine-check passthrough.
> gpa2hva is useful to inject a MADV_HWPOISON madvise from gdb, while
> gpa2hpa is useful to inject an error with the mce-inject kernel
> module.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

And queued.

Dave
> ---
>  hmp-commands.hx |  32 ++++++++++++++++++
>  monitor.c       | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 133 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8819281..0aca984 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -526,6 +526,38 @@ Dump 80 16 bit values at the start of the video memory.
>  ETEXI
>  
>      {
> +        .name       = "gpa2hva",
> +        .args_type  = "addr:l",
> +        .params     = "addr",
> +        .help       = "print the host virtual address corresponding to a guest physical address",
> +        .cmd        = hmp_gpa2hva,
> +    },
> +
> +STEXI
> +@item gpa2hva @var{addr}
> +@findex gpa2hva
> +Print the host virtual address at which the guest's physical address @var{addr}
> +is mapped.
> +ETEXI
> +
> +#ifdef CONFIG_LINUX
> +    {
> +        .name       = "gpa2hpa",
> +        .args_type  = "addr:l",
> +        .params     = "addr",
> +        .help       = "print the host physical address corresponding to a guest physical address",
> +        .cmd        = hmp_gpa2hpa,
> +    },
> +#endif
> +
> +STEXI
> +@item gpa2hpa @var{addr}
> +@findex gpa2hpa
> +Print the host physical address at which the guest's physical address @var{addr}
> +is mapped.
> +ETEXI
> +
> +    {
>          .name       = "p|print",
>          .args_type  = "fmt:/,val:l",
>          .params     = "/fmt expr",
> diff --git a/monitor.c b/monitor.c
> index be282ec..216a97a 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1421,6 +1421,107 @@ static void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict)
>      memory_dump(mon, count, format, size, addr, 1);
>  }
>  
> +static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
> +{
> +    MemoryRegionSection mrs = memory_region_find(get_system_memory(),
> +                                                 addr, 1);
> +
> +    if (!mrs.mr) {
> +        error_setg(errp, "No memory is mapped at address 0x%" HWADDR_PRIx, addr);
> +        return NULL;
> +    }
> +
> +    if (!memory_region_is_ram(mrs.mr) && !memory_region_is_romd(mrs.mr)) {
> +        error_setg(errp, "Memory at address 0x%" HWADDR_PRIx "is not RAM", addr);
> +        memory_region_unref(mrs.mr);
> +        return NULL;
> +    }
> +
> +    *p_mr = mrs.mr;
> +    return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_region);
> +}
> +
> +static void hmp_gpa2hva(Monitor *mon, const QDict *qdict)
> +{
> +    hwaddr addr = qdict_get_int(qdict, "addr");
> +    Error *local_err = NULL;
> +    MemoryRegion *mr;
> +    void *ptr;
> +
> +    ptr = gpa2hva(&mr, addr, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Host virtual address for 0x%" HWADDR_PRIx
> +                   " (%s) is %p\n",
> +                   addr, mr->name, ptr);
> +
> +    memory_region_unref(mr);
> +}
> +
> +#ifdef CONFIG_LINUX
> +static uint64_t vtop(void *ptr, Error **errp)
> +{
> +    uint64_t pinfo;
> +    uint64_t ret = -1;
> +    uintptr_t addr = (uintptr_t) ptr;
> +    uintptr_t pagesize = getpagesize();
> +    off_t offset = addr / pagesize * sizeof(pinfo);
> +    int fd;
> +
> +    fd = open("/proc/self/pagemap", O_RDONLY);
> +    if (fd == -1) {
> +        error_setg_errno(errp, errno, "Cannot open /proc/self/pagemap");
> +        return -1;
> +    }
> +
> +    /* Force copy-on-write if necessary.  */
> +    atomic_add((uint8_t *)ptr, 0);
> +
> +    if (pread(fd, &pinfo, sizeof(pinfo), offset) != sizeof(pinfo)) {
> +        error_setg_errno(errp, errno, "Cannot read pagemap");
> +        goto out;
> +    }
> +    if ((pinfo & (1ull << 63)) == 0) {
> +        error_setg(errp, "Page not present");
> +        goto out;
> +    }
> +    ret = ((pinfo & 0x007fffffffffffffull) * pagesize) | (addr & (pagesize - 1));
> +
> +out:
> +    close(fd);
> +    return ret;
> +}
> +
> +static void hmp_gpa2hpa(Monitor *mon, const QDict *qdict)
> +{
> +    hwaddr addr = qdict_get_int(qdict, "addr");
> +    Error *local_err = NULL;
> +    MemoryRegion *mr;
> +    void *ptr;
> +    uint64_t physaddr;
> +
> +    ptr = gpa2hva(&mr, addr, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        return;
> +    }
> +
> +    physaddr = vtop(ptr, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +    } else {
> +        monitor_printf(mon, "Host physical address for 0x%" HWADDR_PRIx
> +                       " (%s) is 0x%" PRIx64 "\n",
> +                       addr, mr->name, (uint64_t) physaddr);
> +    }
> +
> +    memory_region_unref(mr);
> +}
> +#endif
> +
>  static void do_print(Monitor *mon, const QDict *qdict)
>  {
>      int format = qdict_get_int(qdict, "format");
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] hmp: gpa2hva and gpa2hpa hostaddr command

[Dr. David Alan Gilbert]

* Paolo Bonzini (bonzini@gnu.org) wrote:
> 
> 
> On 20/03/2017 18:16, Paolo Bonzini wrote:
> > 
> > 
> > On 20/03/2017 18:01, Markus Armbruster wrote:
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>
> >>> On 20 March 2017 at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
> >>>> Peter Maydell <peter.maydell@linaro.org> writes:
> >>>>> I have some comments which feel kind of nit-picky, but since this
> >>>>> is a public-facing HMP API I think they need attention since we only
> >>>>> get one chance to get it right.
> >>>>
> >>>> HMP is not a stable interface.  If we get it wrong, we change it.  If
> >>>> our change breaks your usage, you get to keep the pieces.
> >>>
> >>> Oh yes, I had my HMP and QMP the wrong way round.
> >>>
> >>> ...which reminds me that I thought we preferred all HMP commands
> >>> to be implemented in terms of their QMP equivalent ?
> >>
> >> Yes.  We make exceptions for commands we believe have no QMP use, such
> >> as "print".  I figure the rationale for these is "just for testing".
> >> Paolo, can you confirm?
> > 
> > Yes.  If somebody comes up with a non-testing use, I suppose we can
> > always add a QMP variant.  I'll send v3 which uses the current monitor
> > CPU's address space according to Peter's review.
> 
> Since there is no CPU method to transform a CPU state into a MemTxAttrs
> value, and the MemTxAttrs are needed to get the right address space, v2
> is the best I can do for 2.9.
> 
> Changing it to take the CPU state into account (e.g. include secure
> memory when in secure mode) can be done in later releases if necessary.
> 
> David, are you queuing the patch?
> 
> Paolo

Unfortunately it's hitting a warning on the mingw build:

/home/dgilbert/git/hmp/monitor.c: In function 'hmp_gpa2hva':
/home/dgilbert/git/hmp/monitor.c:1461:5: error: 'mr' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     memory_region_unref(mr);
     ^~~~~~~~~~~~~~~~~~~~~~~

I think that's the compiler not being clever enough to spot that path
would always be initialised.

I guess it just needs an = NULL on the mr.

(i686-w64-mingw32-gcc gcc version 6.3.0 20161221 (Fedora MinGW 6.3.0-1.fc25) (GCC) )

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK