[Qemu-devel] [PATCH 00/12] spapr: add qtest support and refactor vty

[Qemu-devel] [PATCH 00/12] spapr: add qtest support and refactor vty

[Anthony Liguori]

Hi,

This series introduces qtest support for spapr and adds a test case
for spapr-vty.  It then significantly refactors spapr-vty fixing among
other things a number of guest driven aborts due to improper use of
QOM cast macros.

A neat trick included in this series is a save_restore() qtest function
which does a live migration to a file and then restores it allowing for
testing vmstate code within qtest cases.

[Qemu-devel] [PATCH 01/12] chardev: ringbuf: add optional save parameter to save state

[Anthony Liguori]

It is very useful to use the ringbuf chardev for writing test
cases and even more useful if the state of the ringbuf is migrated
with the guest.  Otherwise it's hard to detect data loss in a test
case.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qapi-schema.json |  3 ++-
 qemu-char.c      | 45 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index a80ee40..90602d1 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3280,10 +3280,11 @@
 # Configuration info for memory chardevs
 #
 # @size: #optional Ringbuffer size, must be power of two, default is 65536
+# @save: #optional Register a savevm handler, default false
 #
 # Since: 1.5
 ##
-{ 'type': 'ChardevMemory', 'data': { '*size'  : 'int' } }
+{ 'type': 'ChardevMemory', 'data': { '*size'  : 'int', '*save': 'bool' } }
 
 ##
 # @ChardevBackend:
diff --git a/qemu-char.c b/qemu-char.c
index 2c3cfe6..87ba24f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2793,7 +2793,32 @@ static void ringbuf_chr_close(struct CharDriverState *chr)
     chr->opaque = NULL;
 }
 
-static CharDriverState *qemu_chr_open_memory(ChardevMemory *opts,
+static void ringbuf_save(QEMUFile *f, void *opaque)
+{
+    CharDriverState *chr = opaque;
+    RingBufCharDriver *d = chr->opaque;
+
+    qemu_put_be32(f, d->prod);
+    qemu_put_be32(f, d->cons);
+    qemu_put_be32(f, d->size);
+    qemu_put_buffer(f, d->cbuf, d->size);
+}
+
+static int ringbuf_load(QEMUFile *f, void *opaque, int version_id)
+{
+    CharDriverState *chr = opaque;
+    RingBufCharDriver *d = chr->opaque;
+
+    d->prod = qemu_get_be32(f);
+    d->cons = qemu_get_be32(f);
+    d->size = qemu_get_be32(f);
+    qemu_get_buffer(f, d->cbuf, d->size);
+
+    return 0;
+}
+
+static CharDriverState *qemu_chr_open_memory(const char *id,
+                                             ChardevMemory *opts,
                                              Error **errp)
 {
     CharDriverState *chr;
@@ -2804,6 +2829,14 @@ static CharDriverState *qemu_chr_open_memory(ChardevMemory *opts,
 
     d->size = opts->has_size ? opts->size : 65536;
 
+    if (opts->has_save && opts->save) {
+        char *idstr;
+
+        g_assert(id != NULL);
+        idstr = g_strdup_printf("memchar-%s", id);
+        register_savevm(NULL, idstr, 0, 1, ringbuf_save, ringbuf_load, chr);
+    }
+
     /* The size must be power of 2 */
     if (d->size & (d->size - 1)) {
         error_setg(errp, "size of memory chardev must be power of two");
@@ -3120,6 +3153,11 @@ static void qemu_chr_parse_memory(QemuOpts *opts, ChardevBackend *backend,
         backend->memory->has_size = true;
         backend->memory->size = val;
     }
+
+    if (qemu_opt_get_bool(opts, "save", false)) {
+        backend->memory->has_save = true;
+        backend->memory->save = true;
+    }
 }
 
 typedef struct CharDriver {
@@ -3489,6 +3527,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "size",
             .type = QEMU_OPT_SIZE,
+        }, {
+            .name = "save",
+            .type = QEMU_OPT_BOOL,
         },
         { /* end of list */ }
     },
@@ -3711,7 +3752,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         chr = vc_init(backend->vc);
         break;
     case CHARDEV_BACKEND_KIND_MEMORY:
-        chr = qemu_chr_open_memory(backend->memory, errp);
+        chr = qemu_chr_open_memory(id, backend->memory, errp);
         break;
     default:
         error_setg(errp, "unknown chardev backend (%d)", backend->kind);
-- 
1.8.0

Re: [Qemu-devel] [PATCH 01/12] chardev: ringbuf: add optional save parameter to save state

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1286 bytes --]

On 06/19/2013 09:40 PM, Anthony Liguori wrote:
> It is very useful to use the ringbuf chardev for writing test
> cases and even more useful if the state of the ringbuf is migrated
> with the guest.  Otherwise it's hard to detect data loss in a test
> case.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qapi-schema.json |  3 ++-
>  qemu-char.c      | 45 +++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a80ee40..90602d1 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3280,10 +3280,11 @@
>  # Configuration info for memory chardevs
>  #
>  # @size: #optional Ringbuffer size, must be power of two, default is 65536
> +# @save: #optional Register a savevm handler, default false

Useful to have a '(since 1.6)' notation on the added field.

>  #
>  # Since: 1.5
>  ##
> -{ 'type': 'ChardevMemory', 'data': { '*size'  : 'int' } }
> +{ 'type': 'ChardevMemory', 'data': { '*size'  : 'int', '*save': 'bool' } }

Yet another case for introspection discovering an added feature to an
existing command.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH 02/12] qtest: add spapr hypercall support

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qtest.c          | 29 +++++++++++++++++++++++++++++
 tests/libqtest.c | 18 ++++++++++++++++++
 tests/libqtest.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)

diff --git a/qtest.c b/qtest.c
index 07a9612..f8c8f44 100644
--- a/qtest.c
+++ b/qtest.c
@@ -19,6 +19,9 @@
 #include "hw/irq.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpus.h"
+#ifdef TARGET_PPC64
+#include "hw/ppc/spapr.h"
+#endif
 
 #define MAX_IRQ 256
 
@@ -141,6 +144,13 @@ static bool qtest_opened;
  * where NUM is an IRQ number.  For the PC, interrupts can be intercepted
  * simply with "irq_intercept_in ioapic" (note that IRQ0 comes out with
  * NUM=0 even though it is remapped to GSI 2).
+ *
+ * Platform specific (sPAPR):
+ *
+ *  > papr_hypercall NR ARG0 ARG1 ... ARG8
+ *  < OK RET
+ *
+ * where NR, ARG[0-8] and RET are all integers.
  */
 
 static int hex2nib(char ch)
@@ -425,6 +435,25 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
         qtest_clock_warp(ns);
         qtest_send_prefix(chr);
         qtest_send(chr, "OK %"PRIi64"\n", (int64_t)qemu_get_clock_ns(vm_clock));
+#ifdef TARGET_PPC64
+    } else if (strcmp(words[0], "papr_hypercall") == 0) {
+        uint64_t nr;
+        uint64_t args[9];
+        uint64_t ret;
+        int i;
+
+        memset(args, 0, sizeof(args));
+        g_assert(words[1]);
+        nr = strtoull(words[1], NULL, 0);
+        for (i = 0; i < 9; i++) {
+            if (words[2 + i] == NULL) {
+                break;
+            }
+            args[i] = strtoull(words[2 + i], NULL, 0);
+        }
+        ret = spapr_hypercall(ppc_env_get_cpu(first_cpu), nr, args);
+        qtest_send(chr, "OK 0x%" PRIx64 "\n", ret);
+#endif
     } else {
         qtest_send_prefix(chr);
         qtest_send(chr, "FAIL Unknown command `%s'\n", words[0]);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 879ffe9..81107cf 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -544,3 +544,21 @@ void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
     qtest_sendf(s, "\n");
     qtest_rsp(s, 0);
 }
+
+uint64_t qtest_spapr_hcall9(QTestState *s, uint64_t nr, uint64_t a0,
+                            uint64_t a1, uint64_t a2, uint64_t a3, uint64_t a4,
+                            uint64_t a5, uint64_t a6, uint64_t a7, uint64_t a8)
+{
+    gchar **args;
+    uint64_t value;
+
+    qtest_sendf(s, "papr_hypercall 0x%" PRIx64 " 0x%" PRIx64
+                " 0x%" PRIx64 " 0x%" PRIx64 " 0x%" PRIx64 " 0x%" PRIx64 
+                " 0x%" PRIx64 " 0x%" PRIx64 " 0x%" PRIx64 " 0x%" PRIx64 
+                "\n", nr, a0, a1, a2, a3, a4, a5, a6, a7, a8);
+    args = qtest_rsp(s, 2);
+    value = strtoull(args[1], NULL, 0);
+    g_strfreev(args);
+
+    return value;
+}
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 437bda3..592f035 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -286,6 +286,19 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
 int64_t qtest_clock_set(QTestState *s, int64_t val);
 
 /**
+ * qtest_spapr_hcall9:
+ * @s: QTestState instance to operate on.
+ * @nr: The hypercall index
+ * @aN: The @Nth hypercall argument
+ *
+ * Issue an sPAPR hypercall
+ *
+ * Returns: The result of the hypercall.
+ */
+uint64_t qtest_spapr_hcall9(QTestState *s, uint64_t nr, uint64_t a0,
+                            uint64_t a1, uint64_t a2, uint64_t a3, uint64_t a4,
+                            uint64_t a5, uint64_t a6, uint64_t a7, uint64_t a8);
+/**
  * qtest_get_arch:
  *
  * Returns: The architecture for the QEMU executable under test.
@@ -607,4 +620,37 @@ static inline int64_t clock_set(int64_t val)
     return qtest_clock_set(global_qtest, val);
 }
 
+static inline uint64_t spapr_hcall0(uint64_t nr)
+{
+    return qtest_spapr_hcall9(global_qtest, nr, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+}
+
+static inline uint64_t spapr_hcall1(uint64_t nr, uint64_t a0)
+{
+    return qtest_spapr_hcall9(global_qtest, nr, a0, 0, 0, 0, 0, 0, 0, 0, 0);
+}
+
+static inline uint64_t spapr_hcall2(uint64_t nr, uint64_t a0, uint64_t a1)
+{
+    return qtest_spapr_hcall9(global_qtest, nr, a0, a1, 0, 0, 0, 0, 0, 0, 0);
+}
+
+static inline uint64_t spapr_hcall3(uint64_t nr, uint64_t a0, uint64_t a1,
+                                    uint64_t a2)
+{
+    return qtest_spapr_hcall9(global_qtest, nr, a0, a1, a2, 0, 0, 0, 0, 0, 0);
+}
+
+static inline uint64_t spapr_hcall4(uint64_t nr, uint64_t a0, uint64_t a1,
+                                    uint64_t a2, uint64_t a3)
+{
+    return qtest_spapr_hcall9(global_qtest, nr, a0, a1, a2, a3, 0, 0, 0, 0, 0);
+}
+
+static inline uint64_t spapr_hcall5(uint64_t nr, uint64_t a0, uint64_t a1,
+                                    uint64_t a2, uint64_t a3, uint64_t a4)
+{
+    return qtest_spapr_hcall9(global_qtest, nr, a0, a1, a2, a3, a4, 0, 0, 0, 0);
+}
+
 #endif
-- 
1.8.0

Re: [Qemu-devel] [PATCH 02/12] qtest: add spapr hypercall support

[Andreas Färber]

Am 19.06.2013 22:40, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qtest.c          | 29 +++++++++++++++++++++++++++++
>  tests/libqtest.c | 18 ++++++++++++++++++
>  tests/libqtest.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+)
> 
> diff --git a/qtest.c b/qtest.c
> index 07a9612..f8c8f44 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -19,6 +19,9 @@
>  #include "hw/irq.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpus.h"
> +#ifdef TARGET_PPC64
> +#include "hw/ppc/spapr.h"
> +#endif
>  
>  #define MAX_IRQ 256
>  
> @@ -141,6 +144,13 @@ static bool qtest_opened;
>   * where NUM is an IRQ number.  For the PC, interrupts can be intercepted
>   * simply with "irq_intercept_in ioapic" (note that IRQ0 comes out with
>   * NUM=0 even though it is remapped to GSI 2).
> + *
> + * Platform specific (sPAPR):
> + *
> + *  > papr_hypercall NR ARG0 ARG1 ... ARG8

The functions are called spapr_hcall*() but the protocol uses
papr_hypercall?

> + *  < OK RET
> + *
> + * where NR, ARG[0-8] and RET are all integers.
>   */
>  
>  static int hex2nib(char ch)
> @@ -425,6 +435,25 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
>          qtest_clock_warp(ns);
>          qtest_send_prefix(chr);
>          qtest_send(chr, "OK %"PRIi64"\n", (int64_t)qemu_get_clock_ns(vm_clock));
> +#ifdef TARGET_PPC64
> +    } else if (strcmp(words[0], "papr_hypercall") == 0) {
> +        uint64_t nr;
> +        uint64_t args[9];
> +        uint64_t ret;
> +        int i;
> +
> +        memset(args, 0, sizeof(args));
> +        g_assert(words[1]);
> +        nr = strtoull(words[1], NULL, 0);
> +        for (i = 0; i < 9; i++) {
> +            if (words[2 + i] == NULL) {
> +                break;
> +            }
> +            args[i] = strtoull(words[2 + i], NULL, 0);
> +        }
> +        ret = spapr_hypercall(ppc_env_get_cpu(first_cpu), nr, args);
> +        qtest_send(chr, "OK 0x%" PRIx64 "\n", ret);
> +#endif
>      } else {
>          qtest_send_prefix(chr);
>          qtest_send(chr, "FAIL Unknown command `%s'\n", words[0]);
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 879ffe9..81107cf 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -544,3 +544,21 @@ void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
>      qtest_sendf(s, "\n");
>      qtest_rsp(s, 0);
>  }
> +
> +uint64_t qtest_spapr_hcall9(QTestState *s, uint64_t nr, uint64_t a0,
> +                            uint64_t a1, uint64_t a2, uint64_t a3, uint64_t a4,
> +                            uint64_t a5, uint64_t a6, uint64_t a7, uint64_t a8)
> +{
> +    gchar **args;
> +    uint64_t value;
> +
> +    qtest_sendf(s, "papr_hypercall 0x%" PRIx64 " 0x%" PRIx64
> +                " 0x%" PRIx64 " 0x%" PRIx64 " 0x%" PRIx64 " 0x%" PRIx64 
> +                " 0x%" PRIx64 " 0x%" PRIx64 " 0x%" PRIx64 " 0x%" PRIx64 
> +                "\n", nr, a0, a1, a2, a3, a4, a5, a6, a7, a8);
> +    args = qtest_rsp(s, 2);
> +    value = strtoull(args[1], NULL, 0);
> +    g_strfreev(args);
> +
> +    return value;
> +}
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 437bda3..592f035 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -286,6 +286,19 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
>  int64_t qtest_clock_set(QTestState *s, int64_t val);
>  
>  /**
> + * qtest_spapr_hcall9:
> + * @s: QTestState instance to operate on.
> + * @nr: The hypercall index
> + * @aN: The @Nth hypercall argument
> + *
> + * Issue an sPAPR hypercall
> + *
> + * Returns: The result of the hypercall.
> + */
> +uint64_t qtest_spapr_hcall9(QTestState *s, uint64_t nr, uint64_t a0,
> +                            uint64_t a1, uint64_t a2, uint64_t a3, uint64_t a4,
> +                            uint64_t a5, uint64_t a6, uint64_t a7, uint64_t a8);
> +/**
>   * qtest_get_arch:
>   *
>   * Returns: The architecture for the QEMU executable under test.
> @@ -607,4 +620,37 @@ static inline int64_t clock_set(int64_t val)
>      return qtest_clock_set(global_qtest, val);
>  }
>  
> +static inline uint64_t spapr_hcall0(uint64_t nr)
> +{
> +    return qtest_spapr_hcall9(global_qtest, nr, 0, 0, 0, 0, 0, 0, 0, 0, 0);
> +}
> +
> +static inline uint64_t spapr_hcall1(uint64_t nr, uint64_t a0)
> +{
> +    return qtest_spapr_hcall9(global_qtest, nr, a0, 0, 0, 0, 0, 0, 0, 0, 0);
> +}
> +
> +static inline uint64_t spapr_hcall2(uint64_t nr, uint64_t a0, uint64_t a1)
> +{
> +    return qtest_spapr_hcall9(global_qtest, nr, a0, a1, 0, 0, 0, 0, 0, 0, 0);
> +}
> +
> +static inline uint64_t spapr_hcall3(uint64_t nr, uint64_t a0, uint64_t a1,
> +                                    uint64_t a2)
> +{
> +    return qtest_spapr_hcall9(global_qtest, nr, a0, a1, a2, 0, 0, 0, 0, 0, 0);
> +}
> +
> +static inline uint64_t spapr_hcall4(uint64_t nr, uint64_t a0, uint64_t a1,
> +                                    uint64_t a2, uint64_t a3)
> +{
> +    return qtest_spapr_hcall9(global_qtest, nr, a0, a1, a2, a3, 0, 0, 0, 0, 0);
> +}
> +
> +static inline uint64_t spapr_hcall5(uint64_t nr, uint64_t a0, uint64_t a1,
> +                                    uint64_t a2, uint64_t a3, uint64_t a4)
> +{
> +    return qtest_spapr_hcall9(global_qtest, nr, a0, a1, a2, a3, a4, 0, 0, 0, 0);
> +}

While for a large number of almost identical helpers this certainly
sucks, I made an effort to document all functions in that file, so
please keep it that way. :)

Looks very similar to what I had proposed for s390x, so fine with me.

Regards,
Andreas

> +
>  #endif
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 02/12] qtest: add spapr hypercall support

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 19.06.2013 22:40, schrieb Anthony Liguori:
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  qtest.c          | 29 +++++++++++++++++++++++++++++
>>  tests/libqtest.c | 18 ++++++++++++++++++
>>  tests/libqtest.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 93 insertions(+)
>> 
>> diff --git a/qtest.c b/qtest.c
>> index 07a9612..f8c8f44 100644
>> --- a/qtest.c
>> +++ b/qtest.c
>> @@ -19,6 +19,9 @@
>>  #include "hw/irq.h"
>>  #include "sysemu/sysemu.h"
>>  #include "sysemu/cpus.h"
>> +#ifdef TARGET_PPC64
>> +#include "hw/ppc/spapr.h"
>> +#endif
>>  
>>  #define MAX_IRQ 256
>>  
>> @@ -141,6 +144,13 @@ static bool qtest_opened;
>>   * where NUM is an IRQ number.  For the PC, interrupts can be intercepted
>>   * simply with "irq_intercept_in ioapic" (note that IRQ0 comes out with
>>   * NUM=0 even though it is remapped to GSI 2).
>> + *
>> + * Platform specific (sPAPR):
>> + *
>> + *  > papr_hypercall NR ARG0 ARG1 ... ARG8
>
> The functions are called spapr_hcall*() but the protocol uses
> papr_hypercall?

The discrepancy is inherited in the KVM vs. QEMU interfaces.  It's
called papr_hypercall in the KVM interface vs. spapr in QEMU.

I honestly don't know what the distinction between spapr and papr is.

>> +static inline uint64_t spapr_hcall5(uint64_t nr, uint64_t a0, uint64_t a1,
>> +                                    uint64_t a2, uint64_t a3, uint64_t a4)
>> +{
>> +    return qtest_spapr_hcall9(global_qtest, nr, a0, a1, a2, a3, a4, 0, 0, 0, 0);
>> +}
>
> While for a large number of almost identical helpers this certainly
> sucks, I made an effort to document all functions in that file, so
> please keep it that way. :)

Seems a bit redundant to document every one of these but I don't mind
doing it.

Regards,

Anthony Liguori

>
> Looks very similar to what I had proposed for s390x, so fine with me.
>
> Regards,
> Andreas
>
>> +
>>  #endif
>> 
>
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 02/12] qtest: add spapr hypercall support

[Alexander Graf]

Am 20.06.2013 um 17:42 schrieb Anthony Liguori <aliguori@us.ibm.com>:

> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 19.06.2013 22:40, schrieb Anthony Liguori:
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>> ---
>>> qtest.c          | 29 +++++++++++++++++++++++++++++
>>> tests/libqtest.c | 18 ++++++++++++++++++
>>> tests/libqtest.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 93 insertions(+)
>>> 
>>> diff --git a/qtest.c b/qtest.c
>>> index 07a9612..f8c8f44 100644
>>> --- a/qtest.c
>>> +++ b/qtest.c
>>> @@ -19,6 +19,9 @@
>>> #include "hw/irq.h"
>>> #include "sysemu/sysemu.h"
>>> #include "sysemu/cpus.h"
>>> +#ifdef TARGET_PPC64
>>> +#include "hw/ppc/spapr.h"
>>> +#endif
>>> 
>>> #define MAX_IRQ 256
>>> 
>>> @@ -141,6 +144,13 @@ static bool qtest_opened;
>>>  * where NUM is an IRQ number.  For the PC, interrupts can be intercepted
>>>  * simply with "irq_intercept_in ioapic" (note that IRQ0 comes out with
>>>  * NUM=0 even though it is remapped to GSI 2).
>>> + *
>>> + * Platform specific (sPAPR):
>>> + *
>>> + *  > papr_hypercall NR ARG0 ARG1 ... ARG8
>> 
>> The functions are called spapr_hcall*() but the protocol uses
>> papr_hypercall?
> 
> The discrepancy is inherited in the KVM vs. QEMU interfaces.  It's
> called papr_hypercall in the KVM interface vs. spapr in QEMU.
> 
> I honestly don't know what the distinction between spapr and papr is.

PAPR is what PAPR calls itself. However, there is also an ePAPR for BookE, so in order to distinguish the 2 more easily, we named the server version spapr wherever we remembered to.


Alex

Re: [Qemu-devel] [PATCH 02/12] qtest: add spapr hypercall support

[Anthony Liguori]

Alexander Graf <agraf@suse.de> writes:

> Am 20.06.2013 um 17:42 schrieb Anthony Liguori <aliguori@us.ibm.com>:
>
>> Andreas Färber <afaerber@suse.de> writes:
>> 
>>> Am 19.06.2013 22:40, schrieb Anthony Liguori:
>>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>>> ---
>>>> qtest.c          | 29 +++++++++++++++++++++++++++++
>>>> tests/libqtest.c | 18 ++++++++++++++++++
>>>> tests/libqtest.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 93 insertions(+)
>>>> 
>>>> diff --git a/qtest.c b/qtest.c
>>>> index 07a9612..f8c8f44 100644
>>>> --- a/qtest.c
>>>> +++ b/qtest.c
>>>> @@ -19,6 +19,9 @@
>>>> #include "hw/irq.h"
>>>> #include "sysemu/sysemu.h"
>>>> #include "sysemu/cpus.h"
>>>> +#ifdef TARGET_PPC64
>>>> +#include "hw/ppc/spapr.h"
>>>> +#endif
>>>> 
>>>> #define MAX_IRQ 256
>>>> 
>>>> @@ -141,6 +144,13 @@ static bool qtest_opened;
>>>>  * where NUM is an IRQ number.  For the PC, interrupts can be intercepted
>>>>  * simply with "irq_intercept_in ioapic" (note that IRQ0 comes out with
>>>>  * NUM=0 even though it is remapped to GSI 2).
>>>> + *
>>>> + * Platform specific (sPAPR):
>>>> + *
>>>> + *  > papr_hypercall NR ARG0 ARG1 ... ARG8
>>> 
>>> The functions are called spapr_hcall*() but the protocol uses
>>> papr_hypercall?
>> 
>> The discrepancy is inherited in the KVM vs. QEMU interfaces.  It's
>> called papr_hypercall in the KVM interface vs. spapr in QEMU.
>> 
>> I honestly don't know what the distinction between spapr and papr is.
>
> PAPR is what PAPR calls itself. However, there is also an ePAPR for
> BookE, so in order to distinguish the 2 more easily, we named the
> server version spapr wherever we remembered to.

So does it make sense to have papr_hypercall()?  Do hypercalls exist
with the virtualization extensions on BookE?

Regards,

Anthony Liguori

>
>
> Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 02/12] qtest: add spapr hypercall support

[Scott Wood]

On 06/20/2013 01:58:55 PM, Anthony Liguori wrote:
> Alexander Graf <agraf@suse.de> writes:
> 
> > Am 20.06.2013 um 17:42 schrieb Anthony Liguori  
> <aliguori@us.ibm.com>:
> >
> >> Andreas Färber <afaerber@suse.de> writes:
> >>
> >>> The functions are called spapr_hcall*() but the protocol uses
> >>> papr_hypercall?
> >>
> >> The discrepancy is inherited in the KVM vs. QEMU interfaces.  It's
> >> called papr_hypercall in the KVM interface vs. spapr in QEMU.
> >>
> >> I honestly don't know what the distinction between spapr and papr  
> is.
> >
> > PAPR is what PAPR calls itself. However, there is also an ePAPR for
> > BookE, so in order to distinguish the 2 more easily, we named the
> > server version spapr wherever we remembered to.
> 
> So does it make sense to have papr_hypercall()?  Do hypercalls exist
> with the virtualization extensions on BookE?

Yes, there are hypercalls on booke.  Currently the few that KVM  
supports are all handled in the kernel, but that may change, especially  
since Alex is thinking about new hypercalls for guest reset/stop.

-Scott

Re: [Qemu-devel] [PATCH 02/12] qtest: add spapr hypercall support

[Alexander Graf]

On 20.06.2013, at 20:58, Anthony Liguori wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> Am 20.06.2013 um 17:42 schrieb Anthony Liguori <aliguori@us.ibm.com>:
>> 
>>> Andreas Färber <afaerber@suse.de> writes:
>>> 
>>>> Am 19.06.2013 22:40, schrieb Anthony Liguori:
>>>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>>>> ---
>>>>> qtest.c          | 29 +++++++++++++++++++++++++++++
>>>>> tests/libqtest.c | 18 ++++++++++++++++++
>>>>> tests/libqtest.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 93 insertions(+)
>>>>> 
>>>>> diff --git a/qtest.c b/qtest.c
>>>>> index 07a9612..f8c8f44 100644
>>>>> --- a/qtest.c
>>>>> +++ b/qtest.c
>>>>> @@ -19,6 +19,9 @@
>>>>> #include "hw/irq.h"
>>>>> #include "sysemu/sysemu.h"
>>>>> #include "sysemu/cpus.h"
>>>>> +#ifdef TARGET_PPC64
>>>>> +#include "hw/ppc/spapr.h"
>>>>> +#endif
>>>>> 
>>>>> #define MAX_IRQ 256
>>>>> 
>>>>> @@ -141,6 +144,13 @@ static bool qtest_opened;
>>>>> * where NUM is an IRQ number.  For the PC, interrupts can be intercepted
>>>>> * simply with "irq_intercept_in ioapic" (note that IRQ0 comes out with
>>>>> * NUM=0 even though it is remapped to GSI 2).
>>>>> + *
>>>>> + * Platform specific (sPAPR):
>>>>> + *
>>>>> + *  > papr_hypercall NR ARG0 ARG1 ... ARG8
>>>> 
>>>> The functions are called spapr_hcall*() but the protocol uses
>>>> papr_hypercall?
>>> 
>>> The discrepancy is inherited in the KVM vs. QEMU interfaces.  It's
>>> called papr_hypercall in the KVM interface vs. spapr in QEMU.
>>> 
>>> I honestly don't know what the distinction between spapr and papr is.
>> 
>> PAPR is what PAPR calls itself. However, there is also an ePAPR for
>> BookE, so in order to distinguish the 2 more easily, we named the
>> server version spapr wherever we remembered to.
> 
> So does it make sense to have papr_hypercall()?  Do hypercalls exist
> with the virtualization extensions on BookE?

papr_hypercall() really means spapr_hypercall() :). I don't think we should mangle ePAPR and sPAPR together.


Alex

[Qemu-devel] [PATCH 03/12] qtest: return string from QMP commands

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 tests/libqtest.c | 16 +++++++++++++---
 tests/libqtest.h | 14 +++++++++++---
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 81107cf..235ec62 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -287,10 +287,13 @@ redo:
     return words;
 }
 
-void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
+char *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
 {
     bool has_reply = false;
     int nesting = 0;
+    GString *ret;
+
+    ret = g_string_new("");
 
     /* Send QMP request */
     socket_sendf(s->qmp_fd, fmt, ap);
@@ -319,16 +322,23 @@ void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
             nesting--;
             break;
         }
+
+        g_string_append_c(ret, c);
     }
+
+    return g_string_free(ret, FALSE);
 }
 
-void qtest_qmp(QTestState *s, const char *fmt, ...)
+char *qtest_qmp(QTestState *s, const char *fmt, ...)
 {
     va_list ap;
+    char *ret;
 
     va_start(ap, fmt);
-    qtest_qmpv(s, fmt, ap);
+    ret = qtest_qmpv(s, fmt, ap);
     va_end(ap);
+
+    return ret;
 }
 
 const char *qtest_get_arch(void)
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 592f035..5cdcae7 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -21,6 +21,7 @@
 #include <stdbool.h>
 #include <stdarg.h>
 #include <sys/types.h>
+#include <glib.h>
 
 typedef struct QTestState QTestState;
 
@@ -48,8 +49,10 @@ void qtest_quit(QTestState *s);
  * @fmt...: QMP message to send to qemu
  *
  * Sends a QMP message to QEMU
+ *
+ * Returns: the result of the QMP command
  */
-void qtest_qmp(QTestState *s, const char *fmt, ...);
+char *qtest_qmp(QTestState *s, const char *fmt, ...);
 
 /**
  * qtest_qmpv:
@@ -58,8 +61,10 @@ void qtest_qmp(QTestState *s, const char *fmt, ...);
  * @ap: QMP message arguments
  *
  * Sends a QMP message to QEMU.
+ *
+ * Returns: the result of the QMP command
  */
-void qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
+char *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
 
 /**
  * qtest_get_irq:
@@ -340,10 +345,13 @@ static inline QTestState *qtest_start(const char *args)
 static inline void qmp(const char *fmt, ...)
 {
     va_list ap;
+    char *ret;
 
     va_start(ap, fmt);
-    qtest_qmpv(global_qtest, fmt, ap);
+    ret = qtest_qmpv(global_qtest, fmt, ap);
     va_end(ap);
+
+    g_free(ret);
 }
 
 /**
-- 
1.8.0

Re: [Qemu-devel] [PATCH 03/12] qtest: return string from QMP commands

[Andreas Färber]

Am 19.06.2013 22:40, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  tests/libqtest.c | 16 +++++++++++++---
>  tests/libqtest.h | 14 +++++++++++---
>  2 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 81107cf..235ec62 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -287,10 +287,13 @@ redo:
>      return words;
>  }
>  
> -void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
> +char *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
>  {
>      bool has_reply = false;
>      int nesting = 0;
> +    GString *ret;
> +
> +    ret = g_string_new("");
>  
>      /* Send QMP request */
>      socket_sendf(s->qmp_fd, fmt, ap);
> @@ -319,16 +322,23 @@ void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
>              nesting--;
>              break;
>          }
> +
> +        g_string_append_c(ret, c);
>      }
> +
> +    return g_string_free(ret, FALSE);
>  }
>  
> -void qtest_qmp(QTestState *s, const char *fmt, ...)
> +char *qtest_qmp(QTestState *s, const char *fmt, ...)
>  {
>      va_list ap;
> +    char *ret;
>  
>      va_start(ap, fmt);
> -    qtest_qmpv(s, fmt, ap);
> +    ret = qtest_qmpv(s, fmt, ap);
>      va_end(ap);
> +
> +    return ret;
>  }
>  
>  const char *qtest_get_arch(void)
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 592f035..5cdcae7 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -21,6 +21,7 @@
>  #include <stdbool.h>
>  #include <stdarg.h>
>  #include <sys/types.h>
> +#include <glib.h>
>  
>  typedef struct QTestState QTestState;
>  
> @@ -48,8 +49,10 @@ void qtest_quit(QTestState *s);
>   * @fmt...: QMP message to send to qemu
>   *
>   * Sends a QMP message to QEMU
> + *
> + * Returns: the result of the QMP command
>   */
> -void qtest_qmp(QTestState *s, const char *fmt, ...);
> +char *qtest_qmp(QTestState *s, const char *fmt, ...);
>  
>  /**
>   * qtest_qmpv:
> @@ -58,8 +61,10 @@ void qtest_qmp(QTestState *s, const char *fmt, ...);
>   * @ap: QMP message arguments
>   *
>   * Sends a QMP message to QEMU.
> + *
> + * Returns: the result of the QMP command
>   */
> -void qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
> +char *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
>  
>  /**
>   * qtest_get_irq:
> @@ -340,10 +345,13 @@ static inline QTestState *qtest_start(const char *args)
>  static inline void qmp(const char *fmt, ...)
>  {
>      va_list ap;
> +    char *ret;
>  
>      va_start(ap, fmt);
> -    qtest_qmpv(global_qtest, fmt, ap);
> +    ret = qtest_qmpv(global_qtest, fmt, ap);
>      va_end(ap);
> +
> +    g_free(ret);
>  }
>  
>  /**

In http://patchwork.ozlabs.org/patch/207689/ you had suggested to return
QObject?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 03/12] qtest: return string from QMP commands

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 19.06.2013 22:40, schrieb Anthony Liguori:
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  tests/libqtest.c | 16 +++++++++++++---
>>  tests/libqtest.h | 14 +++++++++++---
>>  2 files changed, 24 insertions(+), 6 deletions(-)
>> 
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index 81107cf..235ec62 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -287,10 +287,13 @@ redo:
>>      return words;
>>  }
>>  
>> -void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
>> +char *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
>>  {
>>      bool has_reply = false;
>>      int nesting = 0;
>> +    GString *ret;
>> +
>> +    ret = g_string_new("");
>>  
>>      /* Send QMP request */
>>      socket_sendf(s->qmp_fd, fmt, ap);
>> @@ -319,16 +322,23 @@ void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
>>              nesting--;
>>              break;
>>          }
>> +
>> +        g_string_append_c(ret, c);
>>      }
>> +
>> +    return g_string_free(ret, FALSE);
>>  }
>>  
>> -void qtest_qmp(QTestState *s, const char *fmt, ...)
>> +char *qtest_qmp(QTestState *s, const char *fmt, ...)
>>  {
>>      va_list ap;
>> +    char *ret;
>>  
>>      va_start(ap, fmt);
>> -    qtest_qmpv(s, fmt, ap);
>> +    ret = qtest_qmpv(s, fmt, ap);
>>      va_end(ap);
>> +
>> +    return ret;
>>  }
>>  
>>  const char *qtest_get_arch(void)
>> diff --git a/tests/libqtest.h b/tests/libqtest.h
>> index 592f035..5cdcae7 100644
>> --- a/tests/libqtest.h
>> +++ b/tests/libqtest.h
>> @@ -21,6 +21,7 @@
>>  #include <stdbool.h>
>>  #include <stdarg.h>
>>  #include <sys/types.h>
>> +#include <glib.h>
>>  
>>  typedef struct QTestState QTestState;
>>  
>> @@ -48,8 +49,10 @@ void qtest_quit(QTestState *s);
>>   * @fmt...: QMP message to send to qemu
>>   *
>>   * Sends a QMP message to QEMU
>> + *
>> + * Returns: the result of the QMP command
>>   */
>> -void qtest_qmp(QTestState *s, const char *fmt, ...);
>> +char *qtest_qmp(QTestState *s, const char *fmt, ...);
>>  
>>  /**
>>   * qtest_qmpv:
>> @@ -58,8 +61,10 @@ void qtest_qmp(QTestState *s, const char *fmt, ...);
>>   * @ap: QMP message arguments
>>   *
>>   * Sends a QMP message to QEMU.
>> + *
>> + * Returns: the result of the QMP command
>>   */
>> -void qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
>> +char *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
>>  
>>  /**
>>   * qtest_get_irq:
>> @@ -340,10 +345,13 @@ static inline QTestState *qtest_start(const char *args)
>>  static inline void qmp(const char *fmt, ...)
>>  {
>>      va_list ap;
>> +    char *ret;
>>  
>>      va_start(ap, fmt);
>> -    qtest_qmpv(global_qtest, fmt, ap);
>> +    ret = qtest_qmpv(global_qtest, fmt, ap);
>>      va_end(ap);
>> +
>> +    g_free(ret);
>>  }
>>  
>>  /**
>
> In http://patchwork.ozlabs.org/patch/207689/ you had suggested to return
> QObject?

It's the Right Thing to do but it's hard.   It's on my list todo and I
don't think gating this series on doing proper QMP integration is the
right thing to do.

Regards,

Anthony Liguori

>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 04/12] qtest: add interface to save/restore

[Anthony Liguori]

The idea here is pretty simple.  We have a synchronous interface
that when called, does a migration to a file, kills the QEMU
instance, and spawns a new one using the saved file state.

We an then sprinkle calls to qtest_save_restore() thorough test
cases to validate that we are properly saving and restoring state.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 tests/libqtest.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/libqtest.h | 46 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 235ec62..bc2e84e 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -44,6 +44,7 @@ struct QTestState
     gchar *pid_file; /* QEMU PID file */
     int child_pid;   /* Child process created to execute QEMU */
     char *socket_path, *qmp_socket_path;
+    char *extra_args;
 };
 
 #define g_assert_no_errno(ret) do { \
@@ -104,6 +105,14 @@ static pid_t qtest_qemu_pid(QTestState *s)
     return pid;
 }
 
+void qtest_qmp_wait_event(QTestState *s, const char *event)
+{
+    char *d;
+    /* This is cheating */
+    d = qtest_qmp(s, "");
+    g_free(d);
+}
+
 QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s;
@@ -118,6 +127,7 @@ QTestState *qtest_init(const char *extra_args)
 
     s = g_malloc(sizeof(*s));
 
+    s->extra_args = g_strdup(extra_args);
     s->socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
     s->qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
     pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
@@ -177,6 +187,61 @@ void qtest_quit(QTestState *s)
     g_free(s->pid_file);
     g_free(s->socket_path);
     g_free(s->qmp_socket_path);
+    g_free(s->extra_args);
+}
+
+QTestState *qtest_save_restore(QTestState *s)
+{
+    char *filename;
+    char *d, *p, *extra_args;
+    char *n;
+
+    filename = g_strdup_printf("/tmp/qtest-%d.savevm", getpid());
+
+    /* Start migration to a temporary file */
+    d = qtest_qmp(s,
+                  "{ 'execute': 'migrate', "
+                  "  'arguments': { 'uri': 'exec:dd of=%s 2>/dev/null' } }",
+                  filename);
+    g_free(d);
+
+    /* Wait for critical section to be entered */
+    qtest_qmp_wait_event(s, "STOP");
+
+    /* Not strictly needed as we can't possibly respond to this command until
+     * we've completed migration by virtue of the fact that STOP has been sent
+     * but it's good to be rigorious. */
+    do {
+        d = qtest_qmp(s, "{ 'execute': 'query-migrate' }");
+        p = strstr(d, "\"status\": \"completed\",");
+        g_free(d);
+        if (!p) {
+            g_usleep(100);
+        }
+    } while (p == NULL);
+
+    /* Save arguments to this qtest instance */
+    extra_args = s->extra_args;
+    s->extra_args = NULL;
+
+    /* Quit src instance */
+    qtest_quit(s);
+
+    /* Spawn destination */
+    n = g_strdup_printf("%s -incoming exec:\"dd if=%s 2>/dev/null\"",
+                        extra_args, filename);
+    s = qtest_init(n);
+
+    /* Wait for incoming migration to complete */
+    qtest_qmp_wait_event(s, "RESUME");
+
+    /* Fixup extra arg so we can call repeatedly */
+    g_free(s->extra_args);
+    s->extra_args = extra_args;
+
+    g_free(filename);
+
+    return s;
 }
 
 static void socket_sendf(int fd, const char *fmt, va_list ap)
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 5cdcae7..f2c6e52 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -67,6 +67,15 @@ char *qtest_qmp(QTestState *s, const char *fmt, ...);
 char *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
 
 /**
+ * qtest_qmp_wait_event:
+ * @s: #QTestState instance to operate on.
+ * @event: the event to wait for.
+ *
+ * Waits for a specific QMP event to occur.
+ */
+void qtest_qmp_wait_event(QTestState *s, const char *event);
+
+/**
  * qtest_get_irq:
  * @s: #QTestState instance to operate on.
  * @num: Interrupt to observe.
@@ -291,6 +300,19 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
 int64_t qtest_clock_set(QTestState *s, int64_t val);
 
 /**
+ * qtest_save_restore:
+ * @s: QTest instance to operate on.
+ *
+ * This function will save and restore the state of the running QEMU
+ * instance.  If the savevm code is implemented correctly for a device,
+ * this function should behave like a nop.  If a test case fails because
+ * this function is called, the savevm code for the device is broken.
+ *
+ * Returns: the new QTest instance
+ */
+QTestState *qtest_save_restore(QTestState *s);
+
+/**
  * qtest_spapr_hcall9:
  * @s: QTestState instance to operate on.
  * @nr: The hypercall index
@@ -337,6 +359,17 @@ static inline QTestState *qtest_start(const char *args)
 }
 
 /**
+ * qmp_wait_event:
+ * @event: the event to wait for.
+ *
+ * Waits for a specific QMP event to occur.
+ */
+static inline void qmp_wait_event(const char *event)
+{
+    qtest_qmp_wait_event(global_qtest, event);
+}
+
+/**
  * qmp:
  * @fmt...: QMP message to send to qemu
  *
@@ -628,6 +661,19 @@ static inline int64_t clock_set(int64_t val)
     return qtest_clock_set(global_qtest, val);
 }
 
+/**
+ * save_restore:
+ *
+ * This function will save and restore the state of the running QEMU
+ * instance.  If the savevm code is implemented correctly for a device,
+ * this function should behave like a nop.  If a test case fails because
+ * this function is called, the savevm code for the device is broken.
+ */
+static inline void save_restore(void)
+{
+    global_qtest = qtest_save_restore(global_qtest);
+}
+
 static inline uint64_t spapr_hcall0(uint64_t nr)
 {
     return qtest_spapr_hcall9(global_qtest, nr, 0, 0, 0, 0, 0, 0, 0, 0, 0);
-- 
1.8.0

Re: [Qemu-devel] [PATCH 04/12] qtest: add interface to save/restore

[Andreas Färber]

Am 19.06.2013 22:40, schrieb Anthony Liguori:
> The idea here is pretty simple.  We have a synchronous interface
> that when called, does a migration to a file, kills the QEMU
> instance, and spawns a new one using the saved file state.
> 
> We an then sprinkle calls to qtest_save_restore() thorough test
> cases to validate that we are properly saving and restoring state.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  tests/libqtest.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/libqtest.h | 46 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 111 insertions(+)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 235ec62..bc2e84e 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -44,6 +44,7 @@ struct QTestState
>      gchar *pid_file; /* QEMU PID file */
>      int child_pid;   /* Child process created to execute QEMU */
>      char *socket_path, *qmp_socket_path;
> +    char *extra_args;
>  };
>  
>  #define g_assert_no_errno(ret) do { \
> @@ -104,6 +105,14 @@ static pid_t qtest_qemu_pid(QTestState *s)
>      return pid;
>  }
>  
> +void qtest_qmp_wait_event(QTestState *s, const char *event)
> +{
> +    char *d;
> +    /* This is cheating */
> +    d = qtest_qmp(s, "");

This reminds me that I was unable to use GCC_FMT_ATTR(2, 3) on
qtest_qmp() because of the "" argument that gcc would warn about.

Otherwise code looks okay, although I'm not too familiar with the events.

Regards,
Andreas

> +    g_free(d);
> +}
> +
>  QTestState *qtest_init(const char *extra_args)
>  {
>      QTestState *s;
> @@ -118,6 +127,7 @@ QTestState *qtest_init(const char *extra_args)
>  
>      s = g_malloc(sizeof(*s));
>  
> +    s->extra_args = g_strdup(extra_args);
>      s->socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
>      s->qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
>      pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
> @@ -177,6 +187,61 @@ void qtest_quit(QTestState *s)
>      g_free(s->pid_file);
>      g_free(s->socket_path);
>      g_free(s->qmp_socket_path);
> +    g_free(s->extra_args);
> +}
> +
> +QTestState *qtest_save_restore(QTestState *s)
> +{
> +    char *filename;
> +    char *d, *p, *extra_args;
> +    char *n;
> +
> +    filename = g_strdup_printf("/tmp/qtest-%d.savevm", getpid());
> +
> +    /* Start migration to a temporary file */
> +    d = qtest_qmp(s,
> +                  "{ 'execute': 'migrate', "
> +                  "  'arguments': { 'uri': 'exec:dd of=%s 2>/dev/null' } }",
> +                  filename);
> +    g_free(d);
> +
> +    /* Wait for critical section to be entered */
> +    qtest_qmp_wait_event(s, "STOP");
> +
> +    /* Not strictly needed as we can't possibly respond to this command until
> +     * we've completed migration by virtue of the fact that STOP has been sent
> +     * but it's good to be rigorious. */
> +    do {
> +        d = qtest_qmp(s, "{ 'execute': 'query-migrate' }");
> +        p = strstr(d, "\"status\": \"completed\",");
> +        g_free(d);
> +        if (!p) {
> +            g_usleep(100);
> +        }
> +    } while (p == NULL);
> +
> +    /* Save arguments to this qtest instance */
> +    extra_args = s->extra_args;
> +    s->extra_args = NULL;
> +
> +    /* Quit src instance */
> +    qtest_quit(s);
> +
> +    /* Spawn destination */
> +    n = g_strdup_printf("%s -incoming exec:\"dd if=%s 2>/dev/null\"",
> +                        extra_args, filename);
> +    s = qtest_init(n);
> +
> +    /* Wait for incoming migration to complete */
> +    qtest_qmp_wait_event(s, "RESUME");
> +
> +    /* Fixup extra arg so we can call repeatedly */
> +    g_free(s->extra_args);
> +    s->extra_args = extra_args;
> +
> +    g_free(filename);
> +
> +    return s;
>  }
>  
>  static void socket_sendf(int fd, const char *fmt, va_list ap)
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 5cdcae7..f2c6e52 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -67,6 +67,15 @@ char *qtest_qmp(QTestState *s, const char *fmt, ...);
>  char *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
>  
>  /**
> + * qtest_qmp_wait_event:
> + * @s: #QTestState instance to operate on.
> + * @event: the event to wait for.
> + *
> + * Waits for a specific QMP event to occur.
> + */
> +void qtest_qmp_wait_event(QTestState *s, const char *event);
> +
> +/**
>   * qtest_get_irq:
>   * @s: #QTestState instance to operate on.
>   * @num: Interrupt to observe.
> @@ -291,6 +300,19 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
>  int64_t qtest_clock_set(QTestState *s, int64_t val);
>  
>  /**
> + * qtest_save_restore:
> + * @s: QTest instance to operate on.
> + *
> + * This function will save and restore the state of the running QEMU
> + * instance.  If the savevm code is implemented correctly for a device,
> + * this function should behave like a nop.  If a test case fails because
> + * this function is called, the savevm code for the device is broken.
> + *
> + * Returns: the new QTest instance
> + */
> +QTestState *qtest_save_restore(QTestState *s);
> +
> +/**
>   * qtest_spapr_hcall9:
>   * @s: QTestState instance to operate on.
>   * @nr: The hypercall index
> @@ -337,6 +359,17 @@ static inline QTestState *qtest_start(const char *args)
>  }
>  
>  /**
> + * qmp_wait_event:
> + * @event: the event to wait for.
> + *
> + * Waits for a specific QMP event to occur.
> + */
> +static inline void qmp_wait_event(const char *event)
> +{
> +    qtest_qmp_wait_event(global_qtest, event);
> +}
> +
> +/**
>   * qmp:
>   * @fmt...: QMP message to send to qemu
>   *
> @@ -628,6 +661,19 @@ static inline int64_t clock_set(int64_t val)
>      return qtest_clock_set(global_qtest, val);
>  }
>  
> +/**
> + * save_restore:
> + *
> + * This function will save and restore the state of the running QEMU
> + * instance.  If the savevm code is implemented correctly for a device,
> + * this function should behave like a nop.  If a test case fails because
> + * this function is called, the savevm code for the device is broken.
> + */
> +static inline void save_restore(void)
> +{
> +    global_qtest = qtest_save_restore(global_qtest);
> +}
> +
>  static inline uint64_t spapr_hcall0(uint64_t nr)
>  {
>      return qtest_spapr_hcall9(global_qtest, nr, 0, 0, 0, 0, 0, 0, 0, 0, 0);
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 05/12] spapr-vty: add qtest test case

[Anthony Liguori]

Pretty basic for the moment but the interface is pretty simple.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 tests/Makefile         |  3 ++
 tests/spapr-vty-test.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)
 create mode 100644 tests/spapr-vty-test.c

diff --git a/tests/Makefile b/tests/Makefile
index c107489..7c0ce1a 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -67,6 +67,8 @@ gcov-files-sparc64-y += hw/m48t59.c
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 gcov-files-arm-y += hw/tmp105.c
 
+check-qtest-ppc64-y = tests/spapr-vty-test$(EXESUF)
+
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
 
 test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
@@ -133,6 +135,7 @@ tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
 tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
 tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
 tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
+tests/spapr-vty-test$(EXESUF): tests/spapr-vty-test.o
 
 # QTest rules
 
diff --git a/tests/spapr-vty-test.c b/tests/spapr-vty-test.c
new file mode 100644
index 0000000..8e3908b
--- /dev/null
+++ b/tests/spapr-vty-test.c
@@ -0,0 +1,89 @@
+/*
+ * QTest testcase for the sPAPR VTY
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include "libqtest.h"
+
+#include <glib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#define H_GET_TERM_CHAR         0x54
+#define H_PUT_TERM_CHAR         0x58
+
+static char *qmp_ringbuf_read(const char *device, int max_len)
+{
+    size_t len;
+    char *ret;
+    char *ptr;
+
+    ret = qtest_qmp(global_qtest,
+                    "{ 'execute': 'ringbuf-read', "
+                    "  'arguments': { 'device': '%s', 'size': %d } }",
+                    device, max_len);
+    len = strlen(ret);
+
+    /* Skip pass {"return" */
+    ptr = strchr(ret, '"');
+    g_assert(ptr != NULL);
+    ptr = strchr(ptr + 1, '"');
+    g_assert(ptr != NULL);
+
+    /* Start of data */
+    ptr = strchr(ptr + 1, '"');
+    g_assert(ptr != NULL);
+    ptr += 1;
+
+    len -= ptr - ret;
+    memmove(ret, ptr, len);
+    ret[len] = 0;
+
+    ptr = strrchr(ret, '"');
+    g_assert(ptr != NULL);
+    *ptr = 0;
+
+    return ret;
+}
+
+static void vty_ping(void)
+{
+    const char *greeting = "Hello, world!";
+    char *data;
+    int i;
+
+    for (i = 0; greeting[i]; i++) {
+        spapr_hcall3(H_PUT_TERM_CHAR, 0, 1, (uint64_t)greeting[i] << 56);
+    }
+
+    save_restore();
+
+    data = qmp_ringbuf_read("ring0", 16);
+    g_assert_cmpstr(data, ==, greeting);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_start("-display none -serial chardev:ring0 "
+                "-machine pseries -chardev memory,id=ring0,save=on");
+
+    qtest_add_func("/vty/ping", vty_ping);
+    ret = g_test_run();
+
+    qtest_quit(global_qtest);
+
+    return ret;
+}
-- 
1.8.0

Re: [Qemu-devel] [PATCH 05/12] spapr-vty: add qtest test case

[Alexander Graf]

On 19.06.2013, at 22:40, Anthony Liguori wrote:

> Pretty basic for the moment but the interface is pretty simple.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> tests/Makefile         |  3 ++
> tests/spapr-vty-test.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 92 insertions(+)
> create mode 100644 tests/spapr-vty-test.c
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index c107489..7c0ce1a 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -67,6 +67,8 @@ gcov-files-sparc64-y += hw/m48t59.c
> check-qtest-arm-y = tests/tmp105-test$(EXESUF)
> gcov-files-arm-y += hw/tmp105.c
> 
> +check-qtest-ppc64-y = tests/spapr-vty-test$(EXESUF)
> +
> GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
> 
> test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
> @@ -133,6 +135,7 @@ tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
> tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
> tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
> tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
> +tests/spapr-vty-test$(EXESUF): tests/spapr-vty-test.o
> 
> # QTest rules
> 
> diff --git a/tests/spapr-vty-test.c b/tests/spapr-vty-test.c
> new file mode 100644
> index 0000000..8e3908b
> --- /dev/null
> +++ b/tests/spapr-vty-test.c
> @@ -0,0 +1,89 @@
> +/*
> + * QTest testcase for the sPAPR VTY
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +#include "libqtest.h"
> +
> +#include <glib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +#define H_GET_TERM_CHAR         0x54
> +#define H_PUT_TERM_CHAR         0x58
> +
> +static char *qmp_ringbuf_read(const char *device, int max_len)
> +{
> +    size_t len;
> +    char *ret;
> +    char *ptr;
> +
> +    ret = qtest_qmp(global_qtest,
> +                    "{ 'execute': 'ringbuf-read', "
> +                    "  'arguments': { 'device': '%s', 'size': %d } }",
> +                    device, max_len);
> +    len = strlen(ret);
> +
> +    /* Skip pass {"return" */
> +    ptr = strchr(ret, '"');
> +    g_assert(ptr != NULL);
> +    ptr = strchr(ptr + 1, '"');
> +    g_assert(ptr != NULL);
> +
> +    /* Start of data */
> +    ptr = strchr(ptr + 1, '"');
> +    g_assert(ptr != NULL);
> +    ptr += 1;
> +
> +    len -= ptr - ret;
> +    memmove(ret, ptr, len);
> +    ret[len] = 0;
> +
> +    ptr = strrchr(ret, '"');
> +    g_assert(ptr != NULL);
> +    *ptr = 0;

Can't this be moved to a more generic function? Something along the lines of

  char *qmp_execute(const char *device, int max_len, const char *func, const char *args)

and then call it like:

  return qmp_execute(device, max_len, "ringbuf-read", "");

here? Or let the caller do the argument gathering completely. But a function like surely occurs in other places too, no?

> +
> +    return ret;
> +}
> +
> +static void vty_ping(void)
> +{
> +    const char *greeting = "Hello, world!";
> +    char *data;
> +    int i;
> +
> +    for (i = 0; greeting[i]; i++) {
> +        spapr_hcall3(H_PUT_TERM_CHAR, 0, 1, (uint64_t)greeting[i] << 56);

This looks like endianness breakage? Hypercall arguments should go in native endianness. Registers don't have endianness. Only memory accesses do.


Alex

> +    }
> +
> +    save_restore();
> +
> +    data = qmp_ringbuf_read("ring0", 16);
> +    g_assert_cmpstr(data, ==, greeting);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    int ret;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    qtest_start("-display none -serial chardev:ring0 "
> +                "-machine pseries -chardev memory,id=ring0,save=on");
> +
> +    qtest_add_func("/vty/ping", vty_ping);
> +    ret = g_test_run();
> +
> +    qtest_quit(global_qtest);
> +
> +    return ret;
> +}
> -- 
> 1.8.0
> 

Re: [Qemu-devel] [PATCH 05/12] spapr-vty: add qtest test case

[Anthony Liguori]

Alexander Graf <agraf@suse.de> writes:

> On 19.06.2013, at 22:40, Anthony Liguori wrote:
>
>> Pretty basic for the moment but the interface is pretty simple.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>> tests/Makefile         |  3 ++
>> tests/spapr-vty-test.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 92 insertions(+)
>> create mode 100644 tests/spapr-vty-test.c
>> 
>> diff --git a/tests/Makefile b/tests/Makefile
>> index c107489..7c0ce1a 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -67,6 +67,8 @@ gcov-files-sparc64-y += hw/m48t59.c
>> check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>> gcov-files-arm-y += hw/tmp105.c
>> 
>> +check-qtest-ppc64-y = tests/spapr-vty-test$(EXESUF)
>> +
>> GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
>> 
>> test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
>> @@ -133,6 +135,7 @@ tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
>> tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
>> tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
>> tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
>> +tests/spapr-vty-test$(EXESUF): tests/spapr-vty-test.o
>> 
>> # QTest rules
>> 
>> diff --git a/tests/spapr-vty-test.c b/tests/spapr-vty-test.c
>> new file mode 100644
>> index 0000000..8e3908b
>> --- /dev/null
>> +++ b/tests/spapr-vty-test.c
>> @@ -0,0 +1,89 @@
>> +/*
>> + * QTest testcase for the sPAPR VTY
>> + *
>> + * Copyright IBM, Corp. 2013
>> + *
>> + * Authors:
>> + *  Anthony Liguori   <aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +#include "libqtest.h"
>> +
>> +#include <glib.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +
>> +#define H_GET_TERM_CHAR         0x54
>> +#define H_PUT_TERM_CHAR         0x58
>> +
>> +static char *qmp_ringbuf_read(const char *device, int max_len)
>> +{
>> +    size_t len;
>> +    char *ret;
>> +    char *ptr;
>> +
>> +    ret = qtest_qmp(global_qtest,
>> +                    "{ 'execute': 'ringbuf-read', "
>> +                    "  'arguments': { 'device': '%s', 'size': %d } }",
>> +                    device, max_len);
>> +    len = strlen(ret);
>> +
>> +    /* Skip pass {"return" */
>> +    ptr = strchr(ret, '"');
>> +    g_assert(ptr != NULL);
>> +    ptr = strchr(ptr + 1, '"');
>> +    g_assert(ptr != NULL);
>> +
>> +    /* Start of data */
>> +    ptr = strchr(ptr + 1, '"');
>> +    g_assert(ptr != NULL);
>> +    ptr += 1;
>> +
>> +    len -= ptr - ret;
>> +    memmove(ret, ptr, len);
>> +    ret[len] = 0;
>> +
>> +    ptr = strrchr(ret, '"');
>> +    g_assert(ptr != NULL);
>> +    *ptr = 0;
>
> Can't this be moved to a more generic function? Something along the lines of
>
>   char *qmp_execute(const char *device, int max_len, const char *func, const char *args)
>
> and then call it like:
>
>   return qmp_execute(device, max_len, "ringbuf-read", "");

Yeah, it's not that simple.  We need to pull in the JSON parsing code
etc too.

I'm going to look into that but I think it's outside the scope of this series.

> here? Or let the caller do the argument gathering completely. But a
> function like surely occurs in other places too, no?

qmp support is pretty new in qtest and not quite baked yet.  My
expectation is that this function will disappear over time.

>> +
>> +    return ret;
>> +}
>> +
>> +static void vty_ping(void)
>> +{
>> +    const char *greeting = "Hello, world!";
>> +    char *data;
>> +    int i;
>> +
>> +    for (i = 0; greeting[i]; i++) {
>> +        spapr_hcall3(H_PUT_TERM_CHAR, 0, 1, (uint64_t)greeting[i] << 56);
>
> This looks like endianness breakage? Hypercall arguments should go in
> native endianness. Registers don't have endianness. Only memory
> accesses do.

Yes, you are correct re: hcall args but it's not a breakage.

VTY is weird.  The characters are packed into the arguments.  So sending
two chars would look like:

spapr_hcall3(H_PUT_TERM_CHAR, 0, 2,
            (uint64_t)greeting[0] << 56 |
            (uint64_t)greeting[1] << 48);

It can take an additional argument too for the next 8 bytes.

So even within the Linux kernel or SLOF on big endian, the code would
look like this too.

Regards,

Anthony Liguori

>
>
> Alex
>
>> +    }
>> +
>> +    save_restore();
>> +
>> +    data = qmp_ringbuf_read("ring0", 16);
>> +    g_assert_cmpstr(data, ==, greeting);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    int ret;
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    qtest_start("-display none -serial chardev:ring0 "
>> +                "-machine pseries -chardev memory,id=ring0,save=on");
>> +
>> +    qtest_add_func("/vty/ping", vty_ping);
>> +    ret = g_test_run();
>> +
>> +    qtest_quit(global_qtest);
>> +
>> +    return ret;
>> +}
>> -- 
>> 1.8.0
>> 

Re: [Qemu-devel] [PATCH 05/12] spapr-vty: add qtest test case

[Alexander Graf]

On 19.06.2013, at 23:43, Anthony Liguori wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 19.06.2013, at 22:40, Anthony Liguori wrote:
>> 
>>> Pretty basic for the moment but the interface is pretty simple.
>>> 
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>> ---
>>> tests/Makefile         |  3 ++
>>> tests/spapr-vty-test.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 92 insertions(+)
>>> create mode 100644 tests/spapr-vty-test.c
>>> 
>>> diff --git a/tests/Makefile b/tests/Makefile
>>> index c107489..7c0ce1a 100644
>>> --- a/tests/Makefile
>>> +++ b/tests/Makefile
>>> @@ -67,6 +67,8 @@ gcov-files-sparc64-y += hw/m48t59.c
>>> check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>>> gcov-files-arm-y += hw/tmp105.c
>>> 
>>> +check-qtest-ppc64-y = tests/spapr-vty-test$(EXESUF)
>>> +
>>> GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
>>> 
>>> test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
>>> @@ -133,6 +135,7 @@ tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
>>> tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
>>> tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
>>> tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
>>> +tests/spapr-vty-test$(EXESUF): tests/spapr-vty-test.o
>>> 
>>> # QTest rules
>>> 
>>> diff --git a/tests/spapr-vty-test.c b/tests/spapr-vty-test.c
>>> new file mode 100644
>>> index 0000000..8e3908b
>>> --- /dev/null
>>> +++ b/tests/spapr-vty-test.c
>>> @@ -0,0 +1,89 @@
>>> +/*
>>> + * QTest testcase for the sPAPR VTY
>>> + *
>>> + * Copyright IBM, Corp. 2013
>>> + *
>>> + * Authors:
>>> + *  Anthony Liguori   <aliguori@us.ibm.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>> + * See the COPYING file in the top-level directory.
>>> + *
>>> + */
>>> +#include "libqtest.h"
>>> +
>>> +#include <glib.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +#include <stdlib.h>
>>> +#include <unistd.h>
>>> +
>>> +#define H_GET_TERM_CHAR         0x54
>>> +#define H_PUT_TERM_CHAR         0x58
>>> +
>>> +static char *qmp_ringbuf_read(const char *device, int max_len)
>>> +{
>>> +    size_t len;
>>> +    char *ret;
>>> +    char *ptr;
>>> +
>>> +    ret = qtest_qmp(global_qtest,
>>> +                    "{ 'execute': 'ringbuf-read', "
>>> +                    "  'arguments': { 'device': '%s', 'size': %d } }",
>>> +                    device, max_len);
>>> +    len = strlen(ret);
>>> +
>>> +    /* Skip pass {"return" */
>>> +    ptr = strchr(ret, '"');
>>> +    g_assert(ptr != NULL);
>>> +    ptr = strchr(ptr + 1, '"');
>>> +    g_assert(ptr != NULL);
>>> +
>>> +    /* Start of data */
>>> +    ptr = strchr(ptr + 1, '"');
>>> +    g_assert(ptr != NULL);
>>> +    ptr += 1;
>>> +
>>> +    len -= ptr - ret;
>>> +    memmove(ret, ptr, len);
>>> +    ret[len] = 0;
>>> +
>>> +    ptr = strrchr(ret, '"');
>>> +    g_assert(ptr != NULL);
>>> +    *ptr = 0;
>> 
>> Can't this be moved to a more generic function? Something along the lines of
>> 
>>  char *qmp_execute(const char *device, int max_len, const char *func, const char *args)
>> 
>> and then call it like:
>> 
>>  return qmp_execute(device, max_len, "ringbuf-read", "");
> 
> Yeah, it's not that simple.  We need to pull in the JSON parsing code
> etc too.
> 
> I'm going to look into that but I think it's outside the scope of this series.
> 
>> here? Or let the caller do the argument gathering completely. But a
>> function like surely occurs in other places too, no?
> 
> qmp support is pretty new in qtest and not quite baked yet.  My
> expectation is that this function will disappear over time.

Ok, works for me :). There really should be a more clever logic here eventually. Maybe you could construct a QMP call object in several steps until you send it out, to reflect the tree structure of JSON.

> 
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static void vty_ping(void)
>>> +{
>>> +    const char *greeting = "Hello, world!";
>>> +    char *data;
>>> +    int i;
>>> +
>>> +    for (i = 0; greeting[i]; i++) {
>>> +        spapr_hcall3(H_PUT_TERM_CHAR, 0, 1, (uint64_t)greeting[i] << 56);
>> 
>> This looks like endianness breakage? Hypercall arguments should go in
>> native endianness. Registers don't have endianness. Only memory
>> accesses do.
> 
> Yes, you are correct re: hcall args but it's not a breakage.
> 
> VTY is weird.  The characters are packed into the arguments.  So sending
> two chars would look like:
> 
> spapr_hcall3(H_PUT_TERM_CHAR, 0, 2,
>            (uint64_t)greeting[0] << 56 |
>            (uint64_t)greeting[1] << 48);
> 
> It can take an additional argument too for the next 8 bytes.
> 
> So even within the Linux kernel or SLOF on big endian, the code would
> look like this too.

I only realized that later in the series. I think I got confused on this one when it got introduced already. What a terrific interface ;).


Alex

[Qemu-devel] [PATCH 06/12] spapr-vty: add copyright and license

[Anthony Liguori]

If you are on CC, then please Ack this patch as you touched this
file at some point in time.

Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: Andreas Färber <afaerber@suse.de>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Michael Ellerman <michael@ellerman.id.au>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/char/spapr_vty.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index 2993848..ecc2bb5 100644
--- a/hw/char/spapr_vty.c
+++ b/hw/char/spapr_vty.c
@@ -1,3 +1,16 @@
+/*
+ * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
+ *
+ * PAPR Inter-VM Logical Lan, aka ibmveth
+ *
+ * Copyright IBM, Corp. 2010-2013
+ *
+ * Authors:
+ *   David Gibson <david@gibson.dropbear.id.au>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
 #include "hw/qdev.h"
 #include "sysemu/char.h"
 #include "hw/ppc/spapr.h"
-- 
1.8.0

Re: [Qemu-devel] [PATCH 06/12] spapr-vty: add copyright and license

[Michael Ellerman]

On Wed, 2013-06-19 at 15:40 -0500, Anthony Liguori wrote:
> If you are on CC, then please Ack this patch as you touched this
> file at some point in time.

Barely, but yes.

Acked-by: Michael Ellerman <michael@ellerman.id.au>

cheers


> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 2993848..ecc2bb5 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -1,3 +1,16 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Inter-VM Logical Lan, aka ibmveth
> + *
> + * Copyright IBM, Corp. 2010-2013
> + *
> + * Authors:
> + *   David Gibson <david@gibson.dropbear.id.au>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
>  #include "hw/qdev.h"
>  #include "sysemu/char.h"
>  #include "hw/ppc/spapr.h"

Re: [Qemu-devel] [PATCH 06/12] spapr-vty: add copyright and license

[Alexey Kardashevskiy]

Yes, touched and forgot :)

Acked-by: Alexey Kardashevskiy <aik@ozlabs.ru>


On 06/20/2013 06:40 AM, Anthony Liguori wrote:
> If you are on CC, then please Ack this patch as you touched this
> file at some point in time.
> 
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael Ellerman <michael@ellerman.id.au>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/char/spapr_vty.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 2993848..ecc2bb5 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -1,3 +1,16 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Inter-VM Logical Lan, aka ibmveth
> + *
> + * Copyright IBM, Corp. 2010-2013
> + *
> + * Authors:
> + *   David Gibson <david@gibson.dropbear.id.au>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
>  #include "hw/qdev.h"
>  #include "sysemu/char.h"
>  #include "hw/ppc/spapr.h"
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 06/12] spapr-vty: add copyright and license

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

On Wed, Jun 19, 2013 at 03:40:29PM -0500, Anthony Liguori wrote:
> If you are on CC, then please Ack this patch as you touched this
> file at some point in time.
> 
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael Ellerman <michael@ellerman.id.au>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

Sorry, should have put that banner in from day 1, but I forgot,
obviously.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH 06/12] spapr-vty: add copyright and license

[Paolo Bonzini]

Il 19/06/2013 22:40, Anthony Liguori ha scritto:
> If you are on CC, then please Ack this patch as you touched this
> file at some point in time.
> 
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael Ellerman <michael@ellerman.id.au>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/char/spapr_vty.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 2993848..ecc2bb5 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -1,3 +1,16 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Inter-VM Logical Lan, aka ibmveth
> + *
> + * Copyright IBM, Corp. 2010-2013
> + *
> + * Authors:
> + *   David Gibson <david@gibson.dropbear.id.au>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
>  #include "hw/qdev.h"
>  #include "sysemu/char.h"
>  #include "hw/ppc/spapr.h"
> 

ACK

Paolo

Re: [Qemu-devel] [PATCH 06/12] spapr-vty: add copyright and license

[Andreas Färber]

Am 19.06.2013 22:40, schrieb Anthony Liguori:
> If you are on CC, then please Ack this patch as you touched this
> file at some point in time.
> 
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael Ellerman <michael@ellerman.id.au>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/char/spapr_vty.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)

Acked-by: Andreas Färber <afaerber@suse.de>

Andreas

> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 2993848..ecc2bb5 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -1,3 +1,16 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Inter-VM Logical Lan, aka ibmveth
> + *
> + * Copyright IBM, Corp. 2010-2013
> + *
> + * Authors:
> + *   David Gibson <david@gibson.dropbear.id.au>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
>  #include "hw/qdev.h"
>  #include "sysemu/char.h"
>  #include "hw/ppc/spapr.h"
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 07/12] spapr-rtas: add CPU argument to RTAS calls

[Anthony Liguori]

RTAS is a hypervisor provided binary blob that a guest loads and
calls into to execute certain functions.  It's similar to the
vsyscall page in Linux or the short lived VMCI paravirt interface
from VMware.

The QEMU implementation of the RTAS blob is simply a passthrough
that proxies all RTAS calls to the hypervisor via an hypercall.

While we pass a CPU argument for hypercall handling in QEMU, we
don't pass it for RTAS calls.  Since some RTAs calls require
making hypercalls (normally RTAS is implemented as guest code) we
have nasty hacks to allow that.

Add a CPU argument to RTAS call handling so we can more easily
invoke hypercalls just as guest code would.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/nvram/spapr_nvram.c |  4 ++--
 hw/ppc/spapr_events.c  |  2 +-
 hw/ppc/spapr_hcall.c   |  2 +-
 hw/ppc/spapr_pci.c     | 13 +++++++------
 hw/ppc/spapr_rtas.c    | 21 +++++++++++----------
 hw/ppc/spapr_vio.c     |  6 ++++--
 hw/ppc/xics.c          | 12 ++++++++----
 include/hw/ppc/spapr.h |  5 +++--
 8 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
index 1eb05c9..eb4500e 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
@@ -44,7 +44,7 @@ typedef struct sPAPRNVRAM {
 #define DEFAULT_NVRAM_SIZE 65536
 #define MAX_NVRAM_SIZE (UINT16_MAX * 16)
 
-static void rtas_nvram_fetch(sPAPREnvironment *spapr,
+static void rtas_nvram_fetch(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs,
                              target_ulong args,
                              uint32_t nret, target_ulong rets)
@@ -91,7 +91,7 @@ static void rtas_nvram_fetch(sPAPREnvironment *spapr,
     rtas_st(rets, 1, (alen < 0) ? 0 : alen);
 }
 
-static void rtas_nvram_store(sPAPREnvironment *spapr,
+static void rtas_nvram_store(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs,
                              target_ulong args,
                              uint32_t nret, target_ulong rets)
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index c0d7e62..a69390e 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -277,7 +277,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
     qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->epow_irq));
 }
 
-static void check_exception(sPAPREnvironment *spapr,
+static void check_exception(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                             uint32_t token, uint32_t nargs,
                             target_ulong args,
                             uint32_t nret, target_ulong rets)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8f0b7e8..e6f321d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -525,7 +525,7 @@ static target_ulong h_rtas(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     uint32_t nargs = ldl_be_phys(rtas_r3 + 4);
     uint32_t nret = ldl_be_phys(rtas_r3 + 8);
 
-    return spapr_rtas_call(spapr, token, nargs, rtas_r3 + 12,
+    return spapr_rtas_call(cpu, spapr, token, nargs, rtas_r3 + 12,
                            nret, rtas_r3 + 12 + 4*nargs);
 }
 
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 62ff323..9cacdd2 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -119,7 +119,7 @@ static void finish_read_pci_config(sPAPREnvironment *spapr, uint64_t buid,
     rtas_st(rets, 1, val);
 }
 
-static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
+static void rtas_ibm_read_pci_config(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                      uint32_t token, uint32_t nargs,
                                      target_ulong args,
                                      uint32_t nret, target_ulong rets)
@@ -139,7 +139,7 @@ static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
     finish_read_pci_config(spapr, buid, addr, size, rets);
 }
 
-static void rtas_read_pci_config(sPAPREnvironment *spapr,
+static void rtas_read_pci_config(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                  uint32_t token, uint32_t nargs,
                                  target_ulong args,
                                  uint32_t nret, target_ulong rets)
@@ -185,7 +185,7 @@ static void finish_write_pci_config(sPAPREnvironment *spapr, uint64_t buid,
     rtas_st(rets, 0, 0);
 }
 
-static void rtas_ibm_write_pci_config(sPAPREnvironment *spapr,
+static void rtas_ibm_write_pci_config(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                       uint32_t token, uint32_t nargs,
                                       target_ulong args,
                                       uint32_t nret, target_ulong rets)
@@ -206,7 +206,7 @@ static void rtas_ibm_write_pci_config(sPAPREnvironment *spapr,
     finish_write_pci_config(spapr, buid, addr, size, val, rets);
 }
 
-static void rtas_write_pci_config(sPAPREnvironment *spapr,
+static void rtas_write_pci_config(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                   uint32_t token, uint32_t nargs,
                                   target_ulong args,
                                   uint32_t nret, target_ulong rets)
@@ -277,7 +277,7 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr,
     }
 }
 
-static void rtas_ibm_change_msi(sPAPREnvironment *spapr,
+static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                 uint32_t token, uint32_t nargs,
                                 target_ulong args, uint32_t nret,
                                 target_ulong rets)
@@ -374,7 +374,8 @@ static void rtas_ibm_change_msi(sPAPREnvironment *spapr,
     trace_spapr_pci_rtas_ibm_change_msi(func, req_num);
 }
 
-static void rtas_ibm_query_interrupt_source_number(sPAPREnvironment *spapr,
+static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
+                                                   sPAPREnvironment *spapr,
                                                    uint32_t token,
                                                    uint32_t nargs,
                                                    target_ulong args,
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index f4bd3c9..5887e04 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -38,7 +38,7 @@
 #define TOKEN_BASE      0x2000
 #define TOKEN_MAX       0x100
 
-static void rtas_display_character(sPAPREnvironment *spapr,
+static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    uint32_t token, uint32_t nargs,
                                    target_ulong args,
                                    uint32_t nret, target_ulong rets)
@@ -54,7 +54,7 @@ static void rtas_display_character(sPAPREnvironment *spapr,
     }
 }
 
-static void rtas_get_time_of_day(sPAPREnvironment *spapr,
+static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                  uint32_t token, uint32_t nargs,
                                  target_ulong args,
                                  uint32_t nret, target_ulong rets)
@@ -78,7 +78,7 @@ static void rtas_get_time_of_day(sPAPREnvironment *spapr,
     rtas_st(rets, 7, 0); /* we don't do nanoseconds */
 }
 
-static void rtas_set_time_of_day(sPAPREnvironment *spapr,
+static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                  uint32_t token, uint32_t nargs,
                                  target_ulong args,
                                  uint32_t nret, target_ulong rets)
@@ -99,7 +99,7 @@ static void rtas_set_time_of_day(sPAPREnvironment *spapr,
     rtas_st(rets, 0, 0); /* Success */
 }
 
-static void rtas_power_off(sPAPREnvironment *spapr,
+static void rtas_power_off(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                            uint32_t token, uint32_t nargs, target_ulong args,
                            uint32_t nret, target_ulong rets)
 {
@@ -111,7 +111,7 @@ static void rtas_power_off(sPAPREnvironment *spapr,
     rtas_st(rets, 0, 0);
 }
 
-static void rtas_system_reboot(sPAPREnvironment *spapr,
+static void rtas_system_reboot(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                uint32_t token, uint32_t nargs,
                                target_ulong args,
                                uint32_t nret, target_ulong rets)
@@ -124,7 +124,8 @@ static void rtas_system_reboot(sPAPREnvironment *spapr,
     rtas_st(rets, 0, 0);
 }
 
-static void rtas_query_cpu_stopped_state(sPAPREnvironment *spapr,
+static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
+                                         sPAPREnvironment *spapr,
                                          uint32_t token, uint32_t nargs,
                                          target_ulong args,
                                          uint32_t nret, target_ulong rets)
@@ -154,7 +155,7 @@ static void rtas_query_cpu_stopped_state(sPAPREnvironment *spapr,
     rtas_st(rets, 0, -3);
 }
 
-static void rtas_start_cpu(sPAPREnvironment *spapr,
+static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPREnvironment *spapr,
                            uint32_t token, uint32_t nargs,
                            target_ulong args,
                            uint32_t nret, target_ulong rets)
@@ -208,7 +209,7 @@ static struct rtas_call {
 
 struct rtas_call *rtas_next = rtas_table;
 
-target_ulong spapr_rtas_call(sPAPREnvironment *spapr,
+target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets)
 {
@@ -217,7 +218,7 @@ target_ulong spapr_rtas_call(sPAPREnvironment *spapr,
         struct rtas_call *call = rtas_table + (token - TOKEN_BASE);
 
         if (call->fn) {
-            call->fn(spapr, token, nargs, args, nret, rets);
+            call->fn(cpu, spapr, token, nargs, args, nret, rets);
             return H_SUCCESS;
         }
     }
@@ -227,7 +228,7 @@ target_ulong spapr_rtas_call(sPAPREnvironment *spapr,
      * machines) without looking it up in the device tree.  This
      * special case makes this work */
     if (token == 0xa) {
-        rtas_display_character(spapr, 0xa, nargs, args, nret, rets);
+        rtas_display_character(cpu, spapr, 0xa, nargs, args, nret, rets);
         return H_SUCCESS;
     }
 
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 304f316..2c5b159 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -321,7 +321,8 @@ static void spapr_vio_quiesce_one(VIOsPAPRDevice *dev)
     free_crq(dev);
 }
 
-static void rtas_set_tce_bypass(sPAPREnvironment *spapr, uint32_t token,
+static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                                uint32_t token,
                                 uint32_t nargs, target_ulong args,
                                 uint32_t nret, target_ulong rets)
 {
@@ -351,7 +352,8 @@ static void rtas_set_tce_bypass(sPAPREnvironment *spapr, uint32_t token,
     rtas_st(rets, 0, 0);
 }
 
-static void rtas_quiesce(sPAPREnvironment *spapr, uint32_t token,
+static void rtas_quiesce(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                         uint32_t token,
                          uint32_t nargs, target_ulong args,
                          uint32_t nret, target_ulong rets)
 {
diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
index 1b25075..091912e 100644
--- a/hw/ppc/xics.c
+++ b/hw/ppc/xics.c
@@ -400,7 +400,8 @@ static target_ulong h_eoi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     return H_SUCCESS;
 }
 
-static void rtas_set_xive(sPAPREnvironment *spapr, uint32_t token,
+static void rtas_set_xive(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                          uint32_t token,
                           uint32_t nargs, target_ulong args,
                           uint32_t nret, target_ulong rets)
 {
@@ -427,7 +428,8 @@ static void rtas_set_xive(sPAPREnvironment *spapr, uint32_t token,
     rtas_st(rets, 0, 0); /* Success */
 }
 
-static void rtas_get_xive(sPAPREnvironment *spapr, uint32_t token,
+static void rtas_get_xive(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                          uint32_t token,
                           uint32_t nargs, target_ulong args,
                           uint32_t nret, target_ulong rets)
 {
@@ -451,7 +453,8 @@ static void rtas_get_xive(sPAPREnvironment *spapr, uint32_t token,
     rtas_st(rets, 2, ics->irqs[nr - ics->offset].priority);
 }
 
-static void rtas_int_off(sPAPREnvironment *spapr, uint32_t token,
+static void rtas_int_off(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                         uint32_t token,
                          uint32_t nargs, target_ulong args,
                          uint32_t nret, target_ulong rets)
 {
@@ -476,7 +479,8 @@ static void rtas_int_off(sPAPREnvironment *spapr, uint32_t token,
     rtas_st(rets, 0, 0); /* Success */
 }
 
-static void rtas_int_on(sPAPREnvironment *spapr, uint32_t token,
+static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                        uint32_t token,
                         uint32_t nargs, target_ulong args,
                         uint32_t nret, target_ulong rets)
 {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 864bee9..4530d25 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -319,11 +319,12 @@ static inline void rtas_st(target_ulong phys, int n, uint32_t val)
     stl_be_phys(phys + 4*n, val);
 }
 
-typedef void (*spapr_rtas_fn)(sPAPREnvironment *spapr, uint32_t token,
+typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                              uint32_t token,
                               uint32_t nargs, target_ulong args,
                               uint32_t nret, target_ulong rets);
 int spapr_rtas_register(const char *name, spapr_rtas_fn fn);
-target_ulong spapr_rtas_call(sPAPREnvironment *spapr,
+target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-- 
1.8.0

Re: [Qemu-devel] [PATCH 07/12] spapr-rtas: add CPU argument to RTAS calls

[Alexander Graf]

On 19.06.2013, at 22:40, Anthony Liguori wrote:

> RTAS is a hypervisor provided binary blob that a guest loads and
> calls into to execute certain functions.  It's similar to the
> vsyscall page in Linux or the short lived VMCI paravirt interface
> from VMware.
> 
> The QEMU implementation of the RTAS blob is simply a passthrough
> that proxies all RTAS calls to the hypervisor via an hypercall.
> 
> While we pass a CPU argument for hypercall handling in QEMU, we
> don't pass it for RTAS calls.  Since some RTAs calls require
> making hypercalls (normally RTAS is implemented as guest code) we
> have nasty hacks to allow that.
> 
> Add a CPU argument to RTAS call handling so we can more easily
> invoke hypercalls just as guest code would.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Thanks, applied to ppc-next.


Alex

Re: [Qemu-devel] [PATCH 07/12] spapr-rtas: add CPU argument to RTAS calls

[Andreas Färber]

Am 19.06.2013 22:40, schrieb Anthony Liguori:
> RTAS is a hypervisor provided binary blob that a guest loads and
> calls into to execute certain functions.  It's similar to the
> vsyscall page in Linux or the short lived VMCI paravirt interface
> from VMware.
> 
> The QEMU implementation of the RTAS blob is simply a passthrough
> that proxies all RTAS calls to the hypervisor via an hypercall.
> 
> While we pass a CPU argument for hypercall handling in QEMU, we
> don't pass it for RTAS calls.  Since some RTAs calls require

"RTAS"

> making hypercalls (normally RTAS is implemented as guest code) we
> have nasty hacks to allow that.

Where are such nasty hacks being removed? I just see the cpu argument
propagated mostly unused throughout code.

> 
> Add a CPU argument to RTAS call handling so we can more easily
> invoke hypercalls just as guest code would.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/nvram/spapr_nvram.c |  4 ++--
>  hw/ppc/spapr_events.c  |  2 +-
>  hw/ppc/spapr_hcall.c   |  2 +-
>  hw/ppc/spapr_pci.c     | 13 +++++++------
>  hw/ppc/spapr_rtas.c    | 21 +++++++++++----------
>  hw/ppc/spapr_vio.c     |  6 ++++--
>  hw/ppc/xics.c          | 12 ++++++++----
>  include/hw/ppc/spapr.h |  5 +++--
>  8 files changed, 37 insertions(+), 28 deletions(-)

Otherwise,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 07/12] spapr-rtas: add CPU argument to RTAS calls

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 19.06.2013 22:40, schrieb Anthony Liguori:
>> RTAS is a hypervisor provided binary blob that a guest loads and
>> calls into to execute certain functions.  It's similar to the
>> vsyscall page in Linux or the short lived VMCI paravirt interface
>> from VMware.
>> 
>> The QEMU implementation of the RTAS blob is simply a passthrough
>> that proxies all RTAS calls to the hypervisor via an hypercall.
>> 
>> While we pass a CPU argument for hypercall handling in QEMU, we
>> don't pass it for RTAS calls.  Since some RTAs calls require
>
> "RTAS"
>
>> making hypercalls (normally RTAS is implemented as guest code) we
>> have nasty hacks to allow that.
>
> Where are such nasty hacks being removed? I just see the cpu argument
> propagated mostly unused throughout code.

[PATCH 08/12] spapr-rtas: use hypercall interface and remove special vty
              interfaces

Regards,

Anthony Liguori

>
>> 
>> Add a CPU argument to RTAS call handling so we can more easily
>> invoke hypercalls just as guest code would.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  hw/nvram/spapr_nvram.c |  4 ++--
>>  hw/ppc/spapr_events.c  |  2 +-
>>  hw/ppc/spapr_hcall.c   |  2 +-
>>  hw/ppc/spapr_pci.c     | 13 +++++++------
>>  hw/ppc/spapr_rtas.c    | 21 +++++++++++----------
>>  hw/ppc/spapr_vio.c     |  6 ++++--
>>  hw/ppc/xics.c          | 12 ++++++++----
>>  include/hw/ppc/spapr.h |  5 +++--
>>  8 files changed, 37 insertions(+), 28 deletions(-)
>
> Otherwise,
>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 08/12] spapr-rtas: use hypercall interface and remove special vty interfaces

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/char/spapr_vty.c        | 36 ++++++++++++++++++------------------
 hw/ppc/spapr_rtas.c        | 18 ++++++++++--------
 include/hw/ppc/spapr_vio.h |  2 --
 3 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index ecc2bb5..4bac79e 100644
--- a/hw/char/spapr_vty.c
+++ b/hw/char/spapr_vty.c
@@ -63,7 +63,7 @@ static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t *buf, int max)
     return n;
 }
 
-void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
+static void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
 {
     VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(sdev);
 
@@ -86,6 +86,23 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
     return 0;
 }
 
+static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
+{
+    VIOsPAPRDevice *sdev;
+
+    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    if (!sdev && reg == 0) {
+        /* Hack for kernel early debug, which always specifies reg==0.
+         * We search all VIO devices, and grab the vty with the lowest
+         * reg.  This attempts to mimic existing PowerVM behaviour
+         * (early debug does work there, despite having no vty with
+         * reg==0. */
+        return spapr_vty_get_default(spapr->vio_bus);
+    }
+
+    return sdev;
+}
+
 /* Forward declaration */
 static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                     target_ulong opcode, target_ulong *args)
@@ -211,23 +228,6 @@ VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
     return selected;
 }
 
-VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
-{
-    VIOsPAPRDevice *sdev;
-
-    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
-    if (!sdev && reg == 0) {
-        /* Hack for kernel early debug, which always specifies reg==0.
-         * We search all VIO devices, and grab the vty with the lowest
-         * reg.  This attempts to mimic existing PowerVM behaviour
-         * (early debug does work there, despite having no vty with
-         * reg==0. */
-        return spapr_vty_get_default(spapr->vio_bus);
-    }
-
-    return sdev;
-}
-
 static void spapr_vty_register_types(void)
 {
     spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char);
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 5887e04..019aed5 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -44,14 +44,16 @@ static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    uint32_t nret, target_ulong rets)
 {
     uint8_t c = rtas_ld(args, 0);
-    VIOsPAPRDevice *sdev = vty_lookup(spapr, 0);
-
-    if (!sdev) {
-        rtas_st(rets, 0, -1);
-    } else {
-        vty_putchars(sdev, &c, sizeof(c));
-        rtas_st(rets, 0, 0);
-    }
+    target_ulong hargs[4] = {
+        0, /* reg=0 */
+        1, /* len=1 */
+        (uint64_t)c << 56, /* data */
+        0 /* data */
+    };
+    target_ulong ret;
+
+    ret = spapr_hypercall(cpu, H_PUT_TERM_CHAR, hargs);
+    rtas_st(rets, 0, ret);
 }
 
 static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
index f98ec0a..817f5ff 100644
--- a/include/hw/ppc/spapr_vio.h
+++ b/include/hw/ppc/spapr_vio.h
@@ -123,8 +123,6 @@ static inline int spapr_vio_dma_set(VIOsPAPRDevice *dev, uint64_t taddr,
 
 int spapr_vio_send_crq(VIOsPAPRDevice *dev, uint8_t *crq);
 
-VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg);
-void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len);
 void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev);
 void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd);
 void spapr_vscsi_create(VIOsPAPRBus *bus);
-- 
1.8.0

Re: [Qemu-devel] [PATCH 08/12] spapr-rtas: use hypercall interface and remove special vty interfaces

[Alexander Graf]

On 19.06.2013, at 22:40, Anthony Liguori wrote:

> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> hw/char/spapr_vty.c        | 36 ++++++++++++++++++------------------
> hw/ppc/spapr_rtas.c        | 18 ++++++++++--------
> include/hw/ppc/spapr_vio.h |  2 --
> 3 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index ecc2bb5..4bac79e 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -63,7 +63,7 @@ static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t *buf, int max)
>     return n;
> }
> 
> -void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
> +static void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
> {
>     VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(sdev);
> 
> @@ -86,6 +86,23 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
>     return 0;
> }
> 
> +static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
> +{
> +    VIOsPAPRDevice *sdev;
> +
> +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
> +    if (!sdev && reg == 0) {
> +        /* Hack for kernel early debug, which always specifies reg==0.
> +         * We search all VIO devices, and grab the vty with the lowest
> +         * reg.  This attempts to mimic existing PowerVM behaviour
> +         * (early debug does work there, despite having no vty with
> +         * reg==0. */
> +        return spapr_vty_get_default(spapr->vio_bus);
> +    }
> +
> +    return sdev;
> +}
> +
> /* Forward declaration */
> static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                                     target_ulong opcode, target_ulong *args)
> @@ -211,23 +228,6 @@ VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
>     return selected;
> }
> 
> -VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
> -{
> -    VIOsPAPRDevice *sdev;
> -
> -    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
> -    if (!sdev && reg == 0) {
> -        /* Hack for kernel early debug, which always specifies reg==0.
> -         * We search all VIO devices, and grab the vty with the lowest
> -         * reg.  This attempts to mimic existing PowerVM behaviour
> -         * (early debug does work there, despite having no vty with
> -         * reg==0. */
> -        return spapr_vty_get_default(spapr->vio_bus);
> -    }
> -
> -    return sdev;
> -}
> -
> static void spapr_vty_register_types(void)
> {
>     spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char);
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 5887e04..019aed5 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -44,14 +44,16 @@ static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                                    uint32_t nret, target_ulong rets)
> {
>     uint8_t c = rtas_ld(args, 0);
> -    VIOsPAPRDevice *sdev = vty_lookup(spapr, 0);
> -
> -    if (!sdev) {
> -        rtas_st(rets, 0, -1);
> -    } else {
> -        vty_putchars(sdev, &c, sizeof(c));
> -        rtas_st(rets, 0, 0);
> -    }
> +    target_ulong hargs[4] = {

This is too small. I believe it works with today's code, but the hypercall ABI allows for more registers to be accessed, so we should at least pad the array to not run into potential buffer overflows:

    The general purpose registers r0 and r3-r12, the CTR and XER registers are volatile along with the condition register fields 0 and 1 plus 5-7.

> +        0, /* reg=0 */
> +        1, /* len=1 */
> +        (uint64_t)c << 56, /* data */

Ugh. So the interface really is that broken? Oh well ....


Alex

> +        0 /* data */
> +    };
> +    target_ulong ret;
> +
> +    ret = spapr_hypercall(cpu, H_PUT_TERM_CHAR, hargs);
> +    rtas_st(rets, 0, ret);
> }
> 
> static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index f98ec0a..817f5ff 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -123,8 +123,6 @@ static inline int spapr_vio_dma_set(VIOsPAPRDevice *dev, uint64_t taddr,
> 
> int spapr_vio_send_crq(VIOsPAPRDevice *dev, uint8_t *crq);
> 
> -VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg);
> -void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len);
> void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev);
> void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd);
> void spapr_vscsi_create(VIOsPAPRBus *bus);
> -- 
> 1.8.0
> 

Re: [Qemu-devel] [PATCH 08/12] spapr-rtas: use hypercall interface and remove special vty interfaces

[Anthony Liguori]

Alexander Graf <agraf@suse.de> writes:

> On 19.06.2013, at 22:40, Anthony Liguori wrote:
>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>> hw/char/spapr_vty.c        | 36 ++++++++++++++++++------------------
>> hw/ppc/spapr_rtas.c        | 18 ++++++++++--------
>> include/hw/ppc/spapr_vio.h |  2 --
>> 3 files changed, 28 insertions(+), 28 deletions(-)
>> 
>> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
>> index ecc2bb5..4bac79e 100644
>> --- a/hw/char/spapr_vty.c
>> +++ b/hw/char/spapr_vty.c
>> @@ -63,7 +63,7 @@ static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t *buf, int max)
>>     return n;
>> }
>> 
>> -void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
>> +static void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
>> {
>>     VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(sdev);
>> 
>> @@ -86,6 +86,23 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
>>     return 0;
>> }
>> 
>> +static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
>> +{
>> +    VIOsPAPRDevice *sdev;
>> +
>> +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>> +    if (!sdev && reg == 0) {
>> +        /* Hack for kernel early debug, which always specifies reg==0.
>> +         * We search all VIO devices, and grab the vty with the lowest
>> +         * reg.  This attempts to mimic existing PowerVM behaviour
>> +         * (early debug does work there, despite having no vty with
>> +         * reg==0. */
>> +        return spapr_vty_get_default(spapr->vio_bus);
>> +    }
>> +
>> +    return sdev;
>> +}
>> +
>> /* Forward declaration */
>> static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>                                     target_ulong opcode, target_ulong *args)
>> @@ -211,23 +228,6 @@ VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
>>     return selected;
>> }
>> 
>> -VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
>> -{
>> -    VIOsPAPRDevice *sdev;
>> -
>> -    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>> -    if (!sdev && reg == 0) {
>> -        /* Hack for kernel early debug, which always specifies reg==0.
>> -         * We search all VIO devices, and grab the vty with the lowest
>> -         * reg.  This attempts to mimic existing PowerVM behaviour
>> -         * (early debug does work there, despite having no vty with
>> -         * reg==0. */
>> -        return spapr_vty_get_default(spapr->vio_bus);
>> -    }
>> -
>> -    return sdev;
>> -}
>> -
>> static void spapr_vty_register_types(void)
>> {
>>     spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char);
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index 5887e04..019aed5 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -44,14 +44,16 @@ static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>                                    uint32_t nret, target_ulong rets)
>> {
>>     uint8_t c = rtas_ld(args, 0);
>> -    VIOsPAPRDevice *sdev = vty_lookup(spapr, 0);
>> -
>> -    if (!sdev) {
>> -        rtas_st(rets, 0, -1);
>> -    } else {
>> -        vty_putchars(sdev, &c, sizeof(c));
>> -        rtas_st(rets, 0, 0);
>> -    }
>> +    target_ulong hargs[4] = {
>
> This is too small. I believe it works with today's code, but the
> hypercall ABI allows for more registers to be accessed,

Not for this hypercall, but I can introduce spapr_hypercall[0-9] calls
if it makes you feel better about it :-)

> so we should at least pad the array to not run into potential buffer overflows:
>
>     The general purpose registers r0 and r3-r12, the CTR and XER registers are volatile along with the condition register fields 0 and 1 plus 5-7.
>
>> +        0, /* reg=0 */
>> +        1, /* len=1 */
>> +        (uint64_t)c << 56, /* data */
>
> Ugh. So the interface really is that broken? Oh well ....

It is.  There must have been some mighty powerful crack that the lads
that designed this interface had been smoking...

Regards,

Anthony Liguori

>
>
> Alex
>
>> +        0 /* data */
>> +    };
>> +    target_ulong ret;
>> +
>> +    ret = spapr_hypercall(cpu, H_PUT_TERM_CHAR, hargs);
>> +    rtas_st(rets, 0, ret);
>> }
>> 
>> static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
>> index f98ec0a..817f5ff 100644
>> --- a/include/hw/ppc/spapr_vio.h
>> +++ b/include/hw/ppc/spapr_vio.h
>> @@ -123,8 +123,6 @@ static inline int spapr_vio_dma_set(VIOsPAPRDevice *dev, uint64_t taddr,
>> 
>> int spapr_vio_send_crq(VIOsPAPRDevice *dev, uint8_t *crq);
>> 
>> -VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg);
>> -void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len);
>> void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev);
>> void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd);
>> void spapr_vscsi_create(VIOsPAPRBus *bus);
>> -- 
>> 1.8.0
>> 

Re: [Qemu-devel] [PATCH 08/12] spapr-rtas: use hypercall interface and remove special vty interfaces

[Alexander Graf]

On 19.06.2013, at 23:45, Anthony Liguori wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 19.06.2013, at 22:40, Anthony Liguori wrote:
>> 
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>> ---
>>> hw/char/spapr_vty.c        | 36 ++++++++++++++++++------------------
>>> hw/ppc/spapr_rtas.c        | 18 ++++++++++--------
>>> include/hw/ppc/spapr_vio.h |  2 --
>>> 3 files changed, 28 insertions(+), 28 deletions(-)
>>> 
>>> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
>>> index ecc2bb5..4bac79e 100644
>>> --- a/hw/char/spapr_vty.c
>>> +++ b/hw/char/spapr_vty.c
>>> @@ -63,7 +63,7 @@ static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t *buf, int max)
>>>    return n;
>>> }
>>> 
>>> -void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
>>> +static void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
>>> {
>>>    VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(sdev);
>>> 
>>> @@ -86,6 +86,23 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
>>>    return 0;
>>> }
>>> 
>>> +static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
>>> +{
>>> +    VIOsPAPRDevice *sdev;
>>> +
>>> +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>>> +    if (!sdev && reg == 0) {
>>> +        /* Hack for kernel early debug, which always specifies reg==0.
>>> +         * We search all VIO devices, and grab the vty with the lowest
>>> +         * reg.  This attempts to mimic existing PowerVM behaviour
>>> +         * (early debug does work there, despite having no vty with
>>> +         * reg==0. */
>>> +        return spapr_vty_get_default(spapr->vio_bus);
>>> +    }
>>> +
>>> +    return sdev;
>>> +}
>>> +
>>> /* Forward declaration */
>>> static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>                                    target_ulong opcode, target_ulong *args)
>>> @@ -211,23 +228,6 @@ VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
>>>    return selected;
>>> }
>>> 
>>> -VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
>>> -{
>>> -    VIOsPAPRDevice *sdev;
>>> -
>>> -    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>>> -    if (!sdev && reg == 0) {
>>> -        /* Hack for kernel early debug, which always specifies reg==0.
>>> -         * We search all VIO devices, and grab the vty with the lowest
>>> -         * reg.  This attempts to mimic existing PowerVM behaviour
>>> -         * (early debug does work there, despite having no vty with
>>> -         * reg==0. */
>>> -        return spapr_vty_get_default(spapr->vio_bus);
>>> -    }
>>> -
>>> -    return sdev;
>>> -}
>>> -
>>> static void spapr_vty_register_types(void)
>>> {
>>>    spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char);
>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>> index 5887e04..019aed5 100644
>>> --- a/hw/ppc/spapr_rtas.c
>>> +++ b/hw/ppc/spapr_rtas.c
>>> @@ -44,14 +44,16 @@ static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>                                   uint32_t nret, target_ulong rets)
>>> {
>>>    uint8_t c = rtas_ld(args, 0);
>>> -    VIOsPAPRDevice *sdev = vty_lookup(spapr, 0);
>>> -
>>> -    if (!sdev) {
>>> -        rtas_st(rets, 0, -1);
>>> -    } else {
>>> -        vty_putchars(sdev, &c, sizeof(c));
>>> -        rtas_st(rets, 0, 0);
>>> -    }
>>> +    target_ulong hargs[4] = {
>> 
>> This is too small. I believe it works with today's code, but the
>> hypercall ABI allows for more registers to be accessed,
> 
> Not for this hypercall, but I can introduce spapr_hypercall[0-9] calls
> if it makes you feel better about it :-)

I think it's a lot easier to merely always pass an array of 9 args into the helper function. It's what the guest facing hypercall code does too. It just passes &env->gprs[4].


Alex

[Qemu-devel] [PATCH 09/12] spapr-vio: move special case handling for reg=0 to vio

[Anthony Liguori]

The creatively named reg field is a hypervisor assigned global
identifier for a virtual device.  Despite the fact that no device
is assigned a reg of 0, guests still use it to refer to early
console.

Instead of handling this in the VTY device, handle this in the VIO
bus since this is ultimately about how devices are decoded.

This does not produce a change in behavior since reg=0 hcalls to
non-VTY devices will still fail as gloriously as they did before
just for a different reason (invalid device instead of invalid reg).

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/char/spapr_vty.c        | 58 ++--------------------------------------------
 hw/ppc/spapr_vio.c         | 46 ++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr_vio.h |  2 --
 3 files changed, 48 insertions(+), 58 deletions(-)

diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index 4bac79e..45fc3ce 100644
--- a/hw/char/spapr_vty.c
+++ b/hw/char/spapr_vty.c
@@ -86,23 +86,6 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
     return 0;
 }
 
-static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
-{
-    VIOsPAPRDevice *sdev;
-
-    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
-    if (!sdev && reg == 0) {
-        /* Hack for kernel early debug, which always specifies reg==0.
-         * We search all VIO devices, and grab the vty with the lowest
-         * reg.  This attempts to mimic existing PowerVM behaviour
-         * (early debug does work there, despite having no vty with
-         * reg==0. */
-        return spapr_vty_get_default(spapr->vio_bus);
-    }
-
-    return sdev;
-}
-
 /* Forward declaration */
 static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                     target_ulong opcode, target_ulong *args)
@@ -114,7 +97,7 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     VIOsPAPRDevice *sdev;
     uint8_t buf[16];
 
-    sdev = vty_lookup(spapr, reg);
+    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
     if (!sdev) {
         return H_PARAMETER;
     }
@@ -141,7 +124,7 @@ static target_ulong h_get_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     VIOsPAPRDevice *sdev;
     uint8_t buf[16];
 
-    sdev = vty_lookup(spapr, reg);
+    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
     if (!sdev) {
         return H_PARAMETER;
     }
@@ -191,43 +174,6 @@ static const TypeInfo spapr_vty_info = {
     .class_init    = spapr_vty_class_init,
 };
 
-VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
-{
-    VIOsPAPRDevice *sdev, *selected;
-    BusChild *kid;
-
-    /*
-     * To avoid the console bouncing around we want one VTY to be
-     * the "default". We haven't really got anything to go on, so
-     * arbitrarily choose the one with the lowest reg value.
-     */
-
-    selected = NULL;
-    QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
-        DeviceState *iter = kid->child;
-
-        /* Only look at VTY devices */
-        if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) {
-            continue;
-        }
-
-        sdev = VIO_SPAPR_DEVICE(iter);
-
-        /* First VTY we've found, so it is selected for now */
-        if (!selected) {
-            selected = sdev;
-            continue;
-        }
-
-        /* Choose VTY with lowest reg value */
-        if (sdev->reg < selected->reg) {
-            selected = sdev;
-        }
-    }
-
-    return selected;
-}
-
 static void spapr_vty_register_types(void)
 {
     spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char);
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 2c5b159..ee99eec 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -77,11 +77,57 @@ static const TypeInfo spapr_vio_bus_info = {
     .instance_size = sizeof(VIOsPAPRBus),
 };
 
+static VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
+{
+    VIOsPAPRDevice *sdev, *selected;
+    BusChild *kid;
+
+    /*
+     * To avoid the console bouncing around we want one VTY to be
+     * the "default". We haven't really got anything to go on, so
+     * arbitrarily choose the one with the lowest reg value.
+     */
+
+    selected = NULL;
+    QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
+        DeviceState *iter = kid->child;
+
+        /* Only look at VTY devices */
+        if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) {
+            continue;
+        }
+
+        sdev = VIO_SPAPR_DEVICE(iter);
+
+        /* First VTY we've found, so it is selected for now */
+        if (!selected) {
+            selected = sdev;
+            continue;
+        }
+
+        /* Choose VTY with lowest reg value */
+        if (sdev->reg < selected->reg) {
+            selected = sdev;
+        }
+    }
+
+    return selected;
+}
+
 VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg)
 {
     BusChild *kid;
     VIOsPAPRDevice *dev = NULL;
 
+    /* Hack for kernel early debug, which always specifies reg==0.
+     * We search all VIO devices, and grab the vty with the lowest
+     * reg.  This attempts to mimic existing PowerVM behaviour
+     * (early debug does work there, despite having no vty with
+     * reg==0. */
+    if (reg == 0) {
+        return spapr_vty_get_default(bus);
+    }
+
     QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
         dev = (VIOsPAPRDevice *)kid->child;
         if (dev->reg == reg) {
diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
index 817f5ff..f55eb90 100644
--- a/include/hw/ppc/spapr_vio.h
+++ b/include/hw/ppc/spapr_vio.h
@@ -127,8 +127,6 @@ void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev);
 void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd);
 void spapr_vscsi_create(VIOsPAPRBus *bus);
 
-VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus);
-
 void spapr_vio_quiesce(void);
 
 #endif /* _HW_SPAPR_VIO_H */
-- 
1.8.0

Re: [Qemu-devel] [PATCH 09/12] spapr-vio: move special case handling for reg=0 to vio

[Alexander Graf]

On 19.06.2013, at 22:40, Anthony Liguori wrote:

> The creatively named reg field is a hypervisor assigned global
> identifier for a virtual device.  Despite the fact that no device
> is assigned a reg of 0, guests still use it to refer to early
> console.
> 
> Instead of handling this in the VTY device, handle this in the VIO
> bus since this is ultimately about how devices are decoded.
> 
> This does not produce a change in behavior since reg=0 hcalls to
> non-VTY devices will still fail as gloriously as they did before
> just for a different reason (invalid device instead of invalid reg).

Ben, is it true that reg=0 is guaranteed to be free, regardless of the target call? Also, are there no other PAPR calls that could possibly refer to reg=0 but mean something different from the VTY device?


Alex

> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> hw/char/spapr_vty.c        | 58 ++--------------------------------------------
> hw/ppc/spapr_vio.c         | 46 ++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_vio.h |  2 --
> 3 files changed, 48 insertions(+), 58 deletions(-)
> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 4bac79e..45fc3ce 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -86,23 +86,6 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
>     return 0;
> }
> 
> -static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
> -{
> -    VIOsPAPRDevice *sdev;
> -
> -    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
> -    if (!sdev && reg == 0) {
> -        /* Hack for kernel early debug, which always specifies reg==0.
> -         * We search all VIO devices, and grab the vty with the lowest
> -         * reg.  This attempts to mimic existing PowerVM behaviour
> -         * (early debug does work there, despite having no vty with
> -         * reg==0. */
> -        return spapr_vty_get_default(spapr->vio_bus);
> -    }
> -
> -    return sdev;
> -}
> -
> /* Forward declaration */
> static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                                     target_ulong opcode, target_ulong *args)
> @@ -114,7 +97,7 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>     VIOsPAPRDevice *sdev;
>     uint8_t buf[16];
> 
> -    sdev = vty_lookup(spapr, reg);
> +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>     if (!sdev) {
>         return H_PARAMETER;
>     }
> @@ -141,7 +124,7 @@ static target_ulong h_get_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>     VIOsPAPRDevice *sdev;
>     uint8_t buf[16];
> 
> -    sdev = vty_lookup(spapr, reg);
> +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>     if (!sdev) {
>         return H_PARAMETER;
>     }
> @@ -191,43 +174,6 @@ static const TypeInfo spapr_vty_info = {
>     .class_init    = spapr_vty_class_init,
> };
> 
> -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
> -{
> -    VIOsPAPRDevice *sdev, *selected;
> -    BusChild *kid;
> -
> -    /*
> -     * To avoid the console bouncing around we want one VTY to be
> -     * the "default". We haven't really got anything to go on, so
> -     * arbitrarily choose the one with the lowest reg value.
> -     */
> -
> -    selected = NULL;
> -    QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
> -        DeviceState *iter = kid->child;
> -
> -        /* Only look at VTY devices */
> -        if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) {
> -            continue;
> -        }
> -
> -        sdev = VIO_SPAPR_DEVICE(iter);
> -
> -        /* First VTY we've found, so it is selected for now */
> -        if (!selected) {
> -            selected = sdev;
> -            continue;
> -        }
> -
> -        /* Choose VTY with lowest reg value */
> -        if (sdev->reg < selected->reg) {
> -            selected = sdev;
> -        }
> -    }
> -
> -    return selected;
> -}
> -
> static void spapr_vty_register_types(void)
> {
>     spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char);
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 2c5b159..ee99eec 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -77,11 +77,57 @@ static const TypeInfo spapr_vio_bus_info = {
>     .instance_size = sizeof(VIOsPAPRBus),
> };
> 
> +static VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
> +{
> +    VIOsPAPRDevice *sdev, *selected;
> +    BusChild *kid;
> +
> +    /*
> +     * To avoid the console bouncing around we want one VTY to be
> +     * the "default". We haven't really got anything to go on, so
> +     * arbitrarily choose the one with the lowest reg value.
> +     */
> +
> +    selected = NULL;
> +    QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
> +        DeviceState *iter = kid->child;
> +
> +        /* Only look at VTY devices */
> +        if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) {
> +            continue;
> +        }
> +
> +        sdev = VIO_SPAPR_DEVICE(iter);
> +
> +        /* First VTY we've found, so it is selected for now */
> +        if (!selected) {
> +            selected = sdev;
> +            continue;
> +        }
> +
> +        /* Choose VTY with lowest reg value */
> +        if (sdev->reg < selected->reg) {
> +            selected = sdev;
> +        }
> +    }
> +
> +    return selected;
> +}
> +
> VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg)
> {
>     BusChild *kid;
>     VIOsPAPRDevice *dev = NULL;
> 
> +    /* Hack for kernel early debug, which always specifies reg==0.
> +     * We search all VIO devices, and grab the vty with the lowest
> +     * reg.  This attempts to mimic existing PowerVM behaviour
> +     * (early debug does work there, despite having no vty with
> +     * reg==0. */
> +    if (reg == 0) {
> +        return spapr_vty_get_default(bus);
> +    }
> +
>     QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
>         dev = (VIOsPAPRDevice *)kid->child;
>         if (dev->reg == reg) {
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index 817f5ff..f55eb90 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -127,8 +127,6 @@ void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev);
> void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd);
> void spapr_vscsi_create(VIOsPAPRBus *bus);
> 
> -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus);
> -
> void spapr_vio_quiesce(void);
> 
> #endif /* _HW_SPAPR_VIO_H */
> -- 
> 1.8.0
> 

Re: [Qemu-devel] [PATCH 09/12] spapr-vio: move special case handling for reg=0 to vio

[Anthony Liguori]

Alexander Graf <agraf@suse.de> writes:

> On 19.06.2013, at 22:40, Anthony Liguori wrote:
>
>> The creatively named reg field is a hypervisor assigned global
>> identifier for a virtual device.  Despite the fact that no device
>> is assigned a reg of 0, guests still use it to refer to early
>> console.
>> 
>> Instead of handling this in the VTY device, handle this in the VIO
>> bus since this is ultimately about how devices are decoded.
>> 
>> This does not produce a change in behavior since reg=0 hcalls to
>> non-VTY devices will still fail as gloriously as they did before
>> just for a different reason (invalid device instead of invalid reg).
>
> Ben, is it true that reg=0 is guaranteed to be free, regardless of the
> target call?

reg is exposed to the guest via device tree.  My assumption is that the
only reason this reg=0 silliness exists is for early boot code that
hasn't gotten enough working yet to actually read the device tree to
discover the proper reg value for the VTY.

> Also, are there no other PAPR calls that could possibly refer to reg=0
> but mean something different from the VTY device?

Note that if there is, it will still fail today exactly the same way as
it does without this patch.

We can still add work arounds to the reg lookup code to return something
different for reg=0 if a different device type is being searched for.

So even if that's the case, I still think this move is the right way to
handle things.

Regards,

Anthony Liguori

>
>
> Alex
>
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>> hw/char/spapr_vty.c        | 58 ++--------------------------------------------
>> hw/ppc/spapr_vio.c         | 46 ++++++++++++++++++++++++++++++++++++
>> include/hw/ppc/spapr_vio.h |  2 --
>> 3 files changed, 48 insertions(+), 58 deletions(-)
>> 
>> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
>> index 4bac79e..45fc3ce 100644
>> --- a/hw/char/spapr_vty.c
>> +++ b/hw/char/spapr_vty.c
>> @@ -86,23 +86,6 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
>>     return 0;
>> }
>> 
>> -static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
>> -{
>> -    VIOsPAPRDevice *sdev;
>> -
>> -    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>> -    if (!sdev && reg == 0) {
>> -        /* Hack for kernel early debug, which always specifies reg==0.
>> -         * We search all VIO devices, and grab the vty with the lowest
>> -         * reg.  This attempts to mimic existing PowerVM behaviour
>> -         * (early debug does work there, despite having no vty with
>> -         * reg==0. */
>> -        return spapr_vty_get_default(spapr->vio_bus);
>> -    }
>> -
>> -    return sdev;
>> -}
>> -
>> /* Forward declaration */
>> static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>                                     target_ulong opcode, target_ulong *args)
>> @@ -114,7 +97,7 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>     VIOsPAPRDevice *sdev;
>>     uint8_t buf[16];
>> 
>> -    sdev = vty_lookup(spapr, reg);
>> +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>>     if (!sdev) {
>>         return H_PARAMETER;
>>     }
>> @@ -141,7 +124,7 @@ static target_ulong h_get_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>     VIOsPAPRDevice *sdev;
>>     uint8_t buf[16];
>> 
>> -    sdev = vty_lookup(spapr, reg);
>> +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>>     if (!sdev) {
>>         return H_PARAMETER;
>>     }
>> @@ -191,43 +174,6 @@ static const TypeInfo spapr_vty_info = {
>>     .class_init    = spapr_vty_class_init,
>> };
>> 
>> -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
>> -{
>> -    VIOsPAPRDevice *sdev, *selected;
>> -    BusChild *kid;
>> -
>> -    /*
>> -     * To avoid the console bouncing around we want one VTY to be
>> -     * the "default". We haven't really got anything to go on, so
>> -     * arbitrarily choose the one with the lowest reg value.
>> -     */
>> -
>> -    selected = NULL;
>> -    QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
>> -        DeviceState *iter = kid->child;
>> -
>> -        /* Only look at VTY devices */
>> -        if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) {
>> -            continue;
>> -        }
>> -
>> -        sdev = VIO_SPAPR_DEVICE(iter);
>> -
>> -        /* First VTY we've found, so it is selected for now */
>> -        if (!selected) {
>> -            selected = sdev;
>> -            continue;
>> -        }
>> -
>> -        /* Choose VTY with lowest reg value */
>> -        if (sdev->reg < selected->reg) {
>> -            selected = sdev;
>> -        }
>> -    }
>> -
>> -    return selected;
>> -}
>> -
>> static void spapr_vty_register_types(void)
>> {
>>     spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char);
>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>> index 2c5b159..ee99eec 100644
>> --- a/hw/ppc/spapr_vio.c
>> +++ b/hw/ppc/spapr_vio.c
>> @@ -77,11 +77,57 @@ static const TypeInfo spapr_vio_bus_info = {
>>     .instance_size = sizeof(VIOsPAPRBus),
>> };
>> 
>> +static VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
>> +{
>> +    VIOsPAPRDevice *sdev, *selected;
>> +    BusChild *kid;
>> +
>> +    /*
>> +     * To avoid the console bouncing around we want one VTY to be
>> +     * the "default". We haven't really got anything to go on, so
>> +     * arbitrarily choose the one with the lowest reg value.
>> +     */
>> +
>> +    selected = NULL;
>> +    QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
>> +        DeviceState *iter = kid->child;
>> +
>> +        /* Only look at VTY devices */
>> +        if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) {
>> +            continue;
>> +        }
>> +
>> +        sdev = VIO_SPAPR_DEVICE(iter);
>> +
>> +        /* First VTY we've found, so it is selected for now */
>> +        if (!selected) {
>> +            selected = sdev;
>> +            continue;
>> +        }
>> +
>> +        /* Choose VTY with lowest reg value */
>> +        if (sdev->reg < selected->reg) {
>> +            selected = sdev;
>> +        }
>> +    }
>> +
>> +    return selected;
>> +}
>> +
>> VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg)
>> {
>>     BusChild *kid;
>>     VIOsPAPRDevice *dev = NULL;
>> 
>> +    /* Hack for kernel early debug, which always specifies reg==0.
>> +     * We search all VIO devices, and grab the vty with the lowest
>> +     * reg.  This attempts to mimic existing PowerVM behaviour
>> +     * (early debug does work there, despite having no vty with
>> +     * reg==0. */
>> +    if (reg == 0) {
>> +        return spapr_vty_get_default(bus);
>> +    }
>> +
>>     QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
>>         dev = (VIOsPAPRDevice *)kid->child;
>>         if (dev->reg == reg) {
>> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
>> index 817f5ff..f55eb90 100644
>> --- a/include/hw/ppc/spapr_vio.h
>> +++ b/include/hw/ppc/spapr_vio.h
>> @@ -127,8 +127,6 @@ void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev);
>> void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd);
>> void spapr_vscsi_create(VIOsPAPRBus *bus);
>> 
>> -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus);
>> -
>> void spapr_vio_quiesce(void);
>> 
>> #endif /* _HW_SPAPR_VIO_H */
>> -- 
>> 1.8.0
>> 

Re: [Qemu-devel] [PATCH 09/12] spapr-vio: move special case handling for reg=0 to vio

[Alexander Graf]

On 19.06.2013, at 23:49, Anthony Liguori wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 19.06.2013, at 22:40, Anthony Liguori wrote:
>> 
>>> The creatively named reg field is a hypervisor assigned global
>>> identifier for a virtual device.  Despite the fact that no device
>>> is assigned a reg of 0, guests still use it to refer to early
>>> console.
>>> 
>>> Instead of handling this in the VTY device, handle this in the VIO
>>> bus since this is ultimately about how devices are decoded.
>>> 
>>> This does not produce a change in behavior since reg=0 hcalls to
>>> non-VTY devices will still fail as gloriously as they did before
>>> just for a different reason (invalid device instead of invalid reg).
>> 
>> Ben, is it true that reg=0 is guaranteed to be free, regardless of the
>> target call?
> 
> reg is exposed to the guest via device tree.  My assumption is that the
> only reason this reg=0 silliness exists is for early boot code that
> hasn't gotten enough working yet to actually read the device tree to
> discover the proper reg value for the VTY.

So QEMU decided the numbering scheme. Can we just use reg=0 for the default VTY? Or is reg=0 considered invalid?

> 
>> Also, are there no other PAPR calls that could possibly refer to reg=0
>> but mean something different from the VTY device?
> 
> Note that if there is, it will still fail today exactly the same way as
> it does without this patch.
> 
> We can still add work arounds to the reg lookup code to return something
> different for reg=0 if a different device type is being searched for.

As mentioned in a later patch review, that's pretty much what I think would be the best way forward, yes. Unless we can just make the default VTY be reg=0. Then there's no hack needed at all ;).

> 
> So even if that's the case, I still think this move is the right way to
> handle things.

Yes, but I want to grasp the scope of potential breakage.


Alex

Re: [Qemu-devel] [PATCH 09/12] spapr-vio: move special case handling for reg=0 to vio

[Anthony Liguori]

Alexander Graf <agraf@suse.de> writes:

> On 19.06.2013, at 23:49, Anthony Liguori wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 19.06.2013, at 22:40, Anthony Liguori wrote:
>>> 
>>>> The creatively named reg field is a hypervisor assigned global
>>>> identifier for a virtual device.  Despite the fact that no device
>>>> is assigned a reg of 0, guests still use it to refer to early
>>>> console.
>>>> 
>>>> Instead of handling this in the VTY device, handle this in the VIO
>>>> bus since this is ultimately about how devices are decoded.
>>>> 
>>>> This does not produce a change in behavior since reg=0 hcalls to
>>>> non-VTY devices will still fail as gloriously as they did before
>>>> just for a different reason (invalid device instead of invalid reg).
>>> 
>>> Ben, is it true that reg=0 is guaranteed to be free, regardless of the
>>> target call?
>> 
>> reg is exposed to the guest via device tree.  My assumption is that the
>> only reason this reg=0 silliness exists is for early boot code that
>> hasn't gotten enough working yet to actually read the device tree to
>> discover the proper reg value for the VTY.
>
> So QEMU decided the numbering scheme. Can we just use reg=0 for the
> default VTY? Or is reg=0 considered invalid?

We expose reg= on the command line too.  I think we want to exclude 0
from that interface though.  I suspect libvirt is going to want to
allocate IDs to ensure a stable guest interface.

Regardless of what the "first" VTY is assigned as it's reg, software is
going to use reg=0 to access it.

Regards,

Anthony Liguori

>>> Also, are there no other PAPR calls that could possibly refer to reg=0
>>> but mean something different from the VTY device?
>> 
>> Note that if there is, it will still fail today exactly the same way as
>> it does without this patch.
>> 
>> We can still add work arounds to the reg lookup code to return something
>> different for reg=0 if a different device type is being searched for.
>
> As mentioned in a later patch review, that's pretty much what I think would be the best way forward, yes. Unless we can just make the default VTY be reg=0. Then there's no hack needed at all ;).
>
>> 
>> So even if that's the case, I still think this move is the right way to
>> handle things.
>
> Yes, but I want to grasp the scope of potential breakage.
>
>
> Alex

Re: [Qemu-devel] [PATCH 09/12] spapr-vio: move special case handling for reg=0 to vio

[Benjamin Herrenschmidt]

On Wed, 2013-06-19 at 23:28 +0200, Alexander Graf wrote:
> On 19.06.2013, at 22:40, Anthony Liguori wrote:
> 
> > The creatively named reg field is a hypervisor assigned global
> > identifier for a virtual device.  Despite the fact that no device
> > is assigned a reg of 0, guests still use it to refer to early
> > console.
> > 
> > Instead of handling this in the VTY device, handle this in the VIO
> > bus since this is ultimately about how devices are decoded.
> > 
> > This does not produce a change in behavior since reg=0 hcalls to
> > non-VTY devices will still fail as gloriously as they did before
> > just for a different reason (invalid device instead of invalid reg).
> 
> Ben, is it true that reg=0 is guaranteed to be free, regardless of the target call?

There is no guarantee other than what qemu does... I didn't write that
code so I don't know :-) But I don't see why it can't be kept free.

> Also, are there no other PAPR calls that could possibly refer to reg=0 but
> mean something different from the VTY device?

Not that come to mind.

Ben.

> 
> Alex
> 
> > 
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > ---
> > hw/char/spapr_vty.c        | 58 ++--------------------------------------------
> > hw/ppc/spapr_vio.c         | 46 ++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_vio.h |  2 --
> > 3 files changed, 48 insertions(+), 58 deletions(-)
> > 
> > diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> > index 4bac79e..45fc3ce 100644
> > --- a/hw/char/spapr_vty.c
> > +++ b/hw/char/spapr_vty.c
> > @@ -86,23 +86,6 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
> >     return 0;
> > }
> > 
> > -static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
> > -{
> > -    VIOsPAPRDevice *sdev;
> > -
> > -    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
> > -    if (!sdev && reg == 0) {
> > -        /* Hack for kernel early debug, which always specifies reg==0.
> > -         * We search all VIO devices, and grab the vty with the lowest
> > -         * reg.  This attempts to mimic existing PowerVM behaviour
> > -         * (early debug does work there, despite having no vty with
> > -         * reg==0. */
> > -        return spapr_vty_get_default(spapr->vio_bus);
> > -    }
> > -
> > -    return sdev;
> > -}
> > -
> > /* Forward declaration */
> > static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> >                                     target_ulong opcode, target_ulong *args)
> > @@ -114,7 +97,7 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> >     VIOsPAPRDevice *sdev;
> >     uint8_t buf[16];
> > 
> > -    sdev = vty_lookup(spapr, reg);
> > +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
> >     if (!sdev) {
> >         return H_PARAMETER;
> >     }
> > @@ -141,7 +124,7 @@ static target_ulong h_get_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> >     VIOsPAPRDevice *sdev;
> >     uint8_t buf[16];
> > 
> > -    sdev = vty_lookup(spapr, reg);
> > +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
> >     if (!sdev) {
> >         return H_PARAMETER;
> >     }
> > @@ -191,43 +174,6 @@ static const TypeInfo spapr_vty_info = {
> >     .class_init    = spapr_vty_class_init,
> > };
> > 
> > -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
> > -{
> > -    VIOsPAPRDevice *sdev, *selected;
> > -    BusChild *kid;
> > -
> > -    /*
> > -     * To avoid the console bouncing around we want one VTY to be
> > -     * the "default". We haven't really got anything to go on, so
> > -     * arbitrarily choose the one with the lowest reg value.
> > -     */
> > -
> > -    selected = NULL;
> > -    QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
> > -        DeviceState *iter = kid->child;
> > -
> > -        /* Only look at VTY devices */
> > -        if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) {
> > -            continue;
> > -        }
> > -
> > -        sdev = VIO_SPAPR_DEVICE(iter);
> > -
> > -        /* First VTY we've found, so it is selected for now */
> > -        if (!selected) {
> > -            selected = sdev;
> > -            continue;
> > -        }
> > -
> > -        /* Choose VTY with lowest reg value */
> > -        if (sdev->reg < selected->reg) {
> > -            selected = sdev;
> > -        }
> > -    }
> > -
> > -    return selected;
> > -}
> > -
> > static void spapr_vty_register_types(void)
> > {
> >     spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char);
> > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> > index 2c5b159..ee99eec 100644
> > --- a/hw/ppc/spapr_vio.c
> > +++ b/hw/ppc/spapr_vio.c
> > @@ -77,11 +77,57 @@ static const TypeInfo spapr_vio_bus_info = {
> >     .instance_size = sizeof(VIOsPAPRBus),
> > };
> > 
> > +static VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
> > +{
> > +    VIOsPAPRDevice *sdev, *selected;
> > +    BusChild *kid;
> > +
> > +    /*
> > +     * To avoid the console bouncing around we want one VTY to be
> > +     * the "default". We haven't really got anything to go on, so
> > +     * arbitrarily choose the one with the lowest reg value.
> > +     */
> > +
> > +    selected = NULL;
> > +    QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
> > +        DeviceState *iter = kid->child;
> > +
> > +        /* Only look at VTY devices */
> > +        if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) {
> > +            continue;
> > +        }
> > +
> > +        sdev = VIO_SPAPR_DEVICE(iter);
> > +
> > +        /* First VTY we've found, so it is selected for now */
> > +        if (!selected) {
> > +            selected = sdev;
> > +            continue;
> > +        }
> > +
> > +        /* Choose VTY with lowest reg value */
> > +        if (sdev->reg < selected->reg) {
> > +            selected = sdev;
> > +        }
> > +    }
> > +
> > +    return selected;
> > +}
> > +
> > VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg)
> > {
> >     BusChild *kid;
> >     VIOsPAPRDevice *dev = NULL;
> > 
> > +    /* Hack for kernel early debug, which always specifies reg==0.
> > +     * We search all VIO devices, and grab the vty with the lowest
> > +     * reg.  This attempts to mimic existing PowerVM behaviour
> > +     * (early debug does work there, despite having no vty with
> > +     * reg==0. */
> > +    if (reg == 0) {
> > +        return spapr_vty_get_default(bus);
> > +    }
> > +
> >     QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
> >         dev = (VIOsPAPRDevice *)kid->child;
> >         if (dev->reg == reg) {
> > diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> > index 817f5ff..f55eb90 100644
> > --- a/include/hw/ppc/spapr_vio.h
> > +++ b/include/hw/ppc/spapr_vio.h
> > @@ -127,8 +127,6 @@ void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev);
> > void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd);
> > void spapr_vscsi_create(VIOsPAPRBus *bus);
> > 
> > -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus);
> > -
> > void spapr_vio_quiesce(void);
> > 
> > #endif /* _HW_SPAPR_VIO_H */
> > -- 
> > 1.8.0
> > 

[Qemu-devel] [PATCH 10/12] spapr-vty: refactor the code to improve consistency

[Anthony Liguori]

A few small changes folded into one cleanup:
 - Fail gracefully when passed a reg of the wrong device type
 - Don't cast uint8_t buf to uint64_t for argument marshalling
 - Use spapr_vty prefix consistently.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/char/spapr_vty.c | 57 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index 45fc3ce..bbd5ecb 100644
--- a/hw/char/spapr_vty.c
+++ b/hw/char/spapr_vty.c
@@ -29,14 +29,14 @@ typedef struct VIOsPAPRVTYDevice {
 #define VIO_SPAPR_VTY_DEVICE(obj) \
      OBJECT_CHECK(VIOsPAPRVTYDevice, (obj), TYPE_VIO_SPAPR_VTY_DEVICE)
 
-static int vty_can_receive(void *opaque)
+static int spapr_vty_can_receive(void *opaque)
 {
     VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(opaque);
 
     return (dev->in - dev->out) < VTERM_BUFSIZE;
 }
 
-static void vty_receive(void *opaque, const uint8_t *buf, int size)
+static void spapr_vty_receive(void *opaque, const uint8_t *buf, int size)
 {
     VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(opaque);
     int i;
@@ -51,9 +51,8 @@ static void vty_receive(void *opaque, const uint8_t *buf, int size)
     }
 }
 
-static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t *buf, int max)
+static int spapr_vty_getchars(VIOsPAPRVTYDevice *dev, uint8_t *buf, int max)
 {
-    VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(sdev);
     int n = 0;
 
     while ((n < max) && (dev->out != dev->in)) {
@@ -63,10 +62,9 @@ static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t *buf, int max)
     return n;
 }
 
-static void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
+static void spapr_vty_putchars(VIOsPAPRVTYDevice *dev,
+                               const uint8_t *buf, int len)
 {
-    VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(sdev);
-
     /* FIXME: should check the qemu_chr_fe_write() return value */
     qemu_chr_fe_write(dev->chardev, buf, len);
 }
@@ -80,8 +78,8 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
         exit(1);
     }
 
-    qemu_chr_add_handlers(dev->chardev, vty_can_receive,
-                          vty_receive, NULL, dev);
+    qemu_chr_add_handlers(dev->chardev, spapr_vty_can_receive,
+                          spapr_vty_receive, NULL, dev);
 
     return 0;
 }
@@ -92,24 +90,31 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 {
     target_ulong reg = args[0];
     target_ulong len = args[1];
-    target_ulong char0_7 = args[2];
-    target_ulong char8_15 = args[3];
     VIOsPAPRDevice *sdev;
     uint8_t buf[16];
+    int i;
 
     sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
     if (!sdev) {
         return H_PARAMETER;
     }
 
+    if (!object_dynamic_cast(OBJECT(sdev), TYPE_VIO_SPAPR_VTY_DEVICE)) {
+        return H_PARAMETER;
+    }
+
     if (len > 16) {
         return H_PARAMETER;
     }
 
-    *((uint64_t *)buf) = cpu_to_be64(char0_7);
-    *((uint64_t *)buf + 1) = cpu_to_be64(char8_15);
+    for (i = 0; i < 16; i++) {
+        uint64_t shift = (7 - (i % 8)) * 8;
+        int index = 2 + (i / 8);
+
+        buf[i] = args[index] >> shift;
+    }
 
-    vty_putchars(sdev, buf, len);
+    spapr_vty_putchars(VIO_SPAPR_VTY_DEVICE(sdev), buf, len);
 
     return H_SUCCESS;
 }
@@ -117,25 +122,31 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 static target_ulong h_get_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                     target_ulong opcode, target_ulong *args)
 {
-    target_ulong reg = args[0];
-    target_ulong *len = args + 0;
-    target_ulong *char0_7 = args + 1;
-    target_ulong *char8_15 = args + 2;
     VIOsPAPRDevice *sdev;
+    target_ulong reg = args[0];
+    target_ulong len;
     uint8_t buf[16];
+    int i;
 
     sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
     if (!sdev) {
         return H_PARAMETER;
     }
 
-    *len = vty_getchars(sdev, buf, sizeof(buf));
-    if (*len < 16) {
-        memset(buf + *len, 0, 16 - *len);
+    if (!object_dynamic_cast(OBJECT(sdev), TYPE_VIO_SPAPR_VTY_DEVICE)) {
+        return H_PARAMETER;
     }
 
-    *char0_7 = be64_to_cpu(*((uint64_t *)buf));
-    *char8_15 = be64_to_cpu(*((uint64_t *)buf + 1));
+    len = spapr_vty_getchars(VIO_SPAPR_VTY_DEVICE(sdev), buf, sizeof(buf));
+
+    args[0] = len;
+    args[1] = args[2] = 0;
+    for (i = 0; i < len; i++) {
+        uint64_t shift = (7 - (i % 8)) * 8;
+        int index = 1 + (i / 8);
+
+        args[index] |= (uint64_t)buf[i] << shift;
+    }
 
     return H_SUCCESS;
 }
-- 
1.8.0

Re: [Qemu-devel] [PATCH 10/12] spapr-vty: refactor the code to improve consistency

[Alexander Graf]

On 19.06.2013, at 22:40, Anthony Liguori wrote:

> A few small changes folded into one cleanup:
> - Fail gracefully when passed a reg of the wrong device type
> - Don't cast uint8_t buf to uint64_t for argument marshalling
> - Use spapr_vty prefix consistently.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

[Qemu-devel] [PATCH 11/12] spapr-vio: pass type to spapr_vio_find_by_reg()

[Anthony Liguori]

Today if an invalid hcall is made to an otherwise valid device we
abort due to QOM casting.  This adds a parameter to find_by_reg()
to indicate which type of device we're looking for.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/char/spapr_vty.c        | 14 ++++----------
 hw/net/spapr_llan.c        | 15 ++++++++++-----
 hw/ppc/spapr_vio.c         | 24 +++++++++++++++---------
 include/hw/ppc/spapr_vio.h |  3 ++-
 4 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index bbd5ecb..aa56f10 100644
--- a/hw/char/spapr_vty.c
+++ b/hw/char/spapr_vty.c
@@ -94,15 +94,12 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     uint8_t buf[16];
     int i;
 
-    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                 TYPE_VIO_SPAPR_VTY_DEVICE);
     if (!sdev) {
         return H_PARAMETER;
     }
 
-    if (!object_dynamic_cast(OBJECT(sdev), TYPE_VIO_SPAPR_VTY_DEVICE)) {
-        return H_PARAMETER;
-    }
-
     if (len > 16) {
         return H_PARAMETER;
     }
@@ -128,15 +125,12 @@ static target_ulong h_get_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     uint8_t buf[16];
     int i;
 
-    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                 TYPE_VIO_SPAPR_VTY_DEVICE);
     if (!sdev) {
         return H_PARAMETER;
     }
 
-    if (!object_dynamic_cast(OBJECT(sdev), TYPE_VIO_SPAPR_VTY_DEVICE)) {
-        return H_PARAMETER;
-    }
-
     len = spapr_vty_getchars(VIO_SPAPR_VTY_DEVICE(sdev), buf, sizeof(buf));
 
     args[0] = len;
diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index 03a09f2..429d020 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -285,7 +285,8 @@ static target_ulong h_register_logical_lan(PowerPCCPU *cpu,
     target_ulong buf_list = args[1];
     target_ulong rec_queue = args[2];
     target_ulong filter_list = args[3];
-    VIOsPAPRDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    VIOsPAPRDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                                 TYPE_VIO_SPAPR_VLAN_DEVICE);
     VIOsPAPRVLANDevice *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
     vlan_bd_t filter_list_bd;
 
@@ -346,7 +347,8 @@ static target_ulong h_free_logical_lan(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                        target_ulong opcode, target_ulong *args)
 {
     target_ulong reg = args[0];
-    VIOsPAPRDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    VIOsPAPRDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                                 TYPE_VIO_SPAPR_VLAN_DEVICE);
     VIOsPAPRVLANDevice *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
 
     if (!dev) {
@@ -370,7 +372,8 @@ static target_ulong h_add_logical_lan_buffer(PowerPCCPU *cpu,
 {
     target_ulong reg = args[0];
     target_ulong buf = args[1];
-    VIOsPAPRDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    VIOsPAPRDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                                 TYPE_VIO_SPAPR_VLAN_DEVICE);
     VIOsPAPRVLANDevice *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
     vlan_bd_t bd;
 
@@ -418,7 +421,8 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     target_ulong reg = args[0];
     target_ulong *bufs = args + 1;
     target_ulong continue_token = args[7];
-    VIOsPAPRDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    VIOsPAPRDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                                 TYPE_VIO_SPAPR_VLAN_DEVICE);
     VIOsPAPRVLANDevice *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
     unsigned total_len;
     uint8_t *lbuf, *p;
@@ -485,7 +489,8 @@ static target_ulong h_multicast_ctrl(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                      target_ulong opcode, target_ulong *args)
 {
     target_ulong reg = args[0];
-    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                                 TYPE_VIO_SPAPR_VLAN_DEVICE);
 
     if (!dev) {
         return H_PARAMETER;
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index ee99eec..b4ef48b 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -114,7 +114,8 @@ static VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
     return selected;
 }
 
-VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg)
+VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg,
+                                      const char *typename)
 {
     BusChild *kid;
     VIOsPAPRDevice *dev = NULL;
@@ -129,8 +130,8 @@ VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg)
     }
 
     QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
-        dev = (VIOsPAPRDevice *)kid->child;
-        if (dev->reg == reg) {
+        dev = VIO_SPAPR_DEVICE(kid->child);
+        if (dev->reg == reg && object_dynamic_cast(OBJECT(dev), typename)) {
             return dev;
         }
     }
@@ -212,7 +213,8 @@ static target_ulong h_reg_crq(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     target_ulong reg = args[0];
     target_ulong queue_addr = args[1];
     target_ulong queue_len = args[2];
-    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                                TYPE_VIO_SPAPR_DEVICE);
 
     if (!dev) {
         hcall_dprintf("Unit 0x" TARGET_FMT_lx " does not exist\n", reg);
@@ -268,7 +270,8 @@ static target_ulong h_free_crq(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                target_ulong opcode, target_ulong *args)
 {
     target_ulong reg = args[0];
-    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                                TYPE_VIO_SPAPR_DEVICE);
 
     if (!dev) {
         hcall_dprintf("Unit 0x" TARGET_FMT_lx " does not exist\n", reg);
@@ -284,7 +287,8 @@ static target_ulong h_send_crq(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     target_ulong reg = args[0];
     target_ulong msg_hi = args[1];
     target_ulong msg_lo = args[2];
-    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                                TYPE_VIO_SPAPR_DEVICE);
     uint64_t crq_mangle[2];
 
     if (!dev) {
@@ -305,7 +309,8 @@ static target_ulong h_enable_crq(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                  target_ulong opcode, target_ulong *args)
 {
     target_ulong reg = args[0];
-    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                                TYPE_VIO_SPAPR_DEVICE);
 
     if (!dev) {
         hcall_dprintf("Unit 0x" TARGET_FMT_lx " does not exist\n", reg);
@@ -382,7 +387,7 @@ static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     }
     unit = rtas_ld(args, 0);
     enable = rtas_ld(args, 1);
-    dev = spapr_vio_find_by_reg(bus, unit);
+    dev = spapr_vio_find_by_reg(bus, unit, TYPE_VIO_SPAPR_DEVICE);
     if (!dev) {
         rtas_st(rets, 0, -3);
         return;
@@ -513,7 +518,8 @@ static target_ulong h_vio_signal(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 {
     target_ulong reg = args[0];
     target_ulong mode = args[1];
-    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
+    VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg,
+                                                TYPE_VIO_SPAPR_DEVICE);
     VIOsPAPRDeviceClass *pc;
 
     if (!dev) {
diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
index f55eb90..e513889 100644
--- a/include/hw/ppc/spapr_vio.h
+++ b/include/hw/ppc/spapr_vio.h
@@ -77,7 +77,8 @@ struct VIOsPAPRBus {
 };
 
 extern VIOsPAPRBus *spapr_vio_bus_init(void);
-extern VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg);
+extern VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg,
+                                             const char *typename);
 extern int spapr_populate_vdevice(VIOsPAPRBus *bus, void *fdt);
 extern int spapr_populate_chosen_stdout(void *fdt, VIOsPAPRBus *bus);
 
-- 
1.8.0

Re: [Qemu-devel] [PATCH 11/12] spapr-vio: pass type to spapr_vio_find_by_reg()

[Alexander Graf]

On 19.06.2013, at 22:40, Anthony Liguori wrote:

> Today if an invalid hcall is made to an otherwise valid device we
> abort due to QOM casting.  This adds a parameter to find_by_reg()
> to indicate which type of device we're looking for.

This should also adjust the hack to only match reg=0 for TYPE_VIO_SPAPR_VTY_DEVICE, no?


Alex

Re: [Qemu-devel] [PATCH 11/12] spapr-vio: pass type to spapr_vio_find_by_reg()

[Anthony Liguori]

Alexander Graf <agraf@suse.de> writes:

> On 19.06.2013, at 22:40, Anthony Liguori wrote:
>
>> Today if an invalid hcall is made to an otherwise valid device we
>> abort due to QOM casting.  This adds a parameter to find_by_reg()
>> to indicate which type of device we're looking for.
>
> This should also adjust the hack to only match reg=0 for
> TYPE_VIO_SPAPR_VTY_DEVICE, no?

Yup, that'll result in a bad cast otherwise.  Nice catch.

I'll respin.

Regards,

Anthony Liguori

>
>
> Alex

[Qemu-devel] [PATCH 12/12] spapr-vty: remove unfixable FIXME

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/char/spapr_vty.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index aa56f10..6834fe2 100644
--- a/hw/char/spapr_vty.c
+++ b/hw/char/spapr_vty.c
@@ -65,7 +65,13 @@ static int spapr_vty_getchars(VIOsPAPRVTYDevice *dev, uint8_t *buf, int max)
 static void spapr_vty_putchars(VIOsPAPRVTYDevice *dev,
                                const uint8_t *buf, int len)
 {
-    /* FIXME: should check the qemu_chr_fe_write() return value */
+    /* There is no flow control with this interface so we can't really
+     * do anything if we are unable to write out data.  So we ignore errors
+     * here and silently drop the data.
+     *
+     * Our only option would be buffering but the kernel already has a buffer
+     * so that would only delay the inevitable.
+     */
     qemu_chr_fe_write(dev->chardev, buf, len);
 }
 
-- 
1.8.0