[Qemu-devel] [PATCH v2 0/4] ppc: adding some RTAS calls in tests/libqos

[Qemu-devel] [PATCH v2 0/4] ppc: adding some RTAS calls in tests/libqos

[Daniel Henrique Barboza]

v2:
- added a new patch to fix a Travis build issue with Mac OS

This series implements a few RTAS hypercalls in tests/libqos
that, used together, implement the DRC state transition described
in PAPR 2.7+, 13.4.

This started as an attempt of implementing hot unplug qtests for the
sPAPR machine but I've found a few issues that will require more time
solving:

- CPU hot unplug: for some reason the machine freezes after the
callback is returned.

- LMB hot unplug: not supported by the sPAPR machine if not
set in CAS.

I have a feeling that the CPU hot unplug  issue might be related
with the lack of CAS negotiation step as well, but only way to be
sure is to further understanding how the CAS negotation interfere
with the device hot unplug. If needed we'll have to implement the
client architecture support hypercall as well in the future.

Until then, I believe these hypercalls have a value of their own and
are worth being pushed upstream.


Daniel Henrique Barboza (4):
  tests: adding 'check_exception' RTAS implementation
  tests: adding 'set_indicator' RTAS call
  tests: ibm,configure-connector RTAS call implementation
  tests/rtas-test.c: fix Apple endian.h include

 tests/libqos/rtas.c | 105 ++++++++++++++++++++++++
 tests/libqos/rtas.h |   5 ++
 tests/rtas-test.c   | 229 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 339 insertions(+)

-- 
2.13.6

[Qemu-devel] [PATCH v2 1/4] tests: adding 'check_exception' RTAS implementation

[Daniel Henrique Barboza]

In the sPAPR guest, events such as device hotplug/unplug are
retrieved by the check_exception RTAS call after the guest
receives an IRQ pulse. For both hotplug and unplug operations,
guest intervention is required to transit the DRC state of
the attached device to the configured state.

Without guest intervention, we are still able of qtesting hotplug
devices in the same manner that we support device hotplug in
early (pre-CAS) stages.

Unfortunately, hot unplugs qtests relies on callbacks that demands
guest intervention to complete - otherwise we risk leaving the guest
in an inconsistent state that might impact other hotplug/unplug
operations later on. If we want to make hot unplug qtests we'll
need to simulate the guest behavior in the scenario in which a
hot unplug is received, allowing the hot unplug process to go
as intended.

This patch is the first step towards hot unplug qtests in sPAPR,
implementing the check_exception RTAS hypercall in libqos. This
hypercall is used to fetch events such as hotplug/hot unplug from
the sPAPR machine after the guest receives an IRQ pulse (or,
in the case of the test implemented here, we simply know when
there is/isn't an event to be retrieved).

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 tests/libqos/rtas.c | 37 +++++++++++++++++++++++++
 tests/libqos/rtas.h |  2 ++
 tests/rtas-test.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)

diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
index 0269803ce0..fdeab448f7 100644
--- a/tests/libqos/rtas.c
+++ b/tests/libqos/rtas.c
@@ -114,3 +114,40 @@ int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
 
     return 0;
 }
+
+/*
+ * check_exception as defined by PAPR 2.7+, 7.3.3.2
+ *
+ * nargs = 7 (with Extended Information)
+ * nrets = 1
+ *
+ * arg[2] = mask of event classes to process
+ * arg[4] = real address of error log
+ * arg[5] = length of error log
+ *
+ * arg[0] (Vector Offset), arg[1] and arg[6] (Additional information)
+ * and arg[3] (Critical) aren't used in the logic of check_exception
+ * in hw/ppc/spapr_events.c and can be ignored.
+ *
+ * If there is an event that matches the given mask, check-exception writes
+ * it in buf_addr up to a max of buf_len bytes.
+ *
+ */
+int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
+                          uint32_t buf_addr, uint32_t buf_len)
+{
+    uint32_t args[7], ret[1];
+    int res;
+
+    args[0] = args[1] = args[3] = args[6] = 0;
+    args[2] = mask;
+    args[4] = buf_addr;
+    args[5] = buf_len;
+
+    res = qrtas_call(alloc, "check-exception", 7, args, 1, ret);
+    if (res != 0) {
+        return -1;
+    }
+
+    return ret[0];
+}
diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
index 498eb19230..330ecfd397 100644
--- a/tests/libqos/rtas.h
+++ b/tests/libqos/rtas.h
@@ -12,4 +12,6 @@ uint32_t qrtas_ibm_read_pci_config(QGuestAllocator *alloc, uint64_t buid,
                                    uint32_t addr, uint32_t size);
 int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
                                uint32_t addr, uint32_t size, uint32_t val);
+int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
+                          uint32_t buf_addr, uint32_t buf_len);
 #endif /* LIBQOS_RTAS_H */
diff --git a/tests/rtas-test.c b/tests/rtas-test.c
index 276c87ef84..c5a6080043 100644
--- a/tests/rtas-test.c
+++ b/tests/rtas-test.c
@@ -5,6 +5,9 @@
 #include "libqos/libqos-spapr.h"
 #include "libqos/rtas.h"
 
+#define EVENT_MASK_EPOW (1 << 30)
+#define EVENT_LOG_LEN 2048
+
 static void test_rtas_get_time_of_day(void)
 {
     QOSState *qs;
@@ -24,6 +27,76 @@ static void test_rtas_get_time_of_day(void)
     qtest_shutdown(qs);
 }
 
+static void test_rtas_check_exception_no_events(void)
+{
+    QOSState *qs;
+    uint64_t ret;
+    uintptr_t guest_buf_addr;
+    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
+
+    qs = qtest_spapr_boot("-machine pseries");
+    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
+
+    /*
+     * mask = 0 should return no events, returning
+     * RTAS_OUT_NO_ERRORS_FOUND (1).
+     */
+    ret = qrtas_check_exception(qs->alloc, 0, guest_buf_addr, EVENT_LOG_LEN);
+    g_assert_cmpint(ret, ==, 1);
+
+    /*
+     * Using a proper event mask should also return
+     * no events since no hotplugs happened.
+     */
+    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW, guest_buf_addr,
+                                EVENT_LOG_LEN);
+    g_assert_cmpint(ret, ==, 1);
+
+    guest_free(qs->alloc, guest_buf_addr);
+    g_free(buf);
+
+    qtest_shutdown(qs);
+}
+
+static void test_rtas_check_exception_hotplug_event(void)
+{
+    QOSState *qs;
+    uint64_t ret;
+    uintptr_t guest_buf_addr;
+    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
+    uint8_t *zero_buf = g_malloc0(EVENT_LOG_LEN);
+
+    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
+                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
+
+    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
+
+    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
+                         "'core-id':'1'");
+    /*
+     * We use EPOW mask instead of HOTPLUG because the code defaults
+     * the hotplug interrupt source to EPOW if the guest didn't change
+     * OV5_HP_EVT during CAS.
+     */
+    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
+                                guest_buf_addr, EVENT_LOG_LEN);
+
+    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
+    guest_free(qs->alloc, guest_buf_addr);
+
+    /*
+     * Calling check_exception after a hotplug needs to return
+     * RTAS_OUT_SUCCESS (0) and a non-zero error_log.
+     */
+    g_assert_cmpint(ret, ==, 0);
+    g_assert(memcmp(buf, zero_buf, EVENT_LOG_LEN) != 0);
+
+    g_free(buf);
+    g_free(zero_buf);
+
+    qtest_shutdown(qs);
+}
+
 int main(int argc, char *argv[])
 {
     const char *arch = qtest_get_arch();
@@ -35,6 +108,10 @@ int main(int argc, char *argv[])
         exit(EXIT_FAILURE);
     }
     qtest_add_func("rtas/get-time-of-day", test_rtas_get_time_of_day);
+    qtest_add_func("rtas/rtas-check-exception-no-events",
+                   test_rtas_check_exception_no_events);
+    qtest_add_func("rtas/rtas-check-exception-hotplug-event",
+                   test_rtas_check_exception_hotplug_event);
 
     return g_test_run();
 }
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 1/4] tests: adding 'check_exception' RTAS implementation

[Laurent Vivier]

On 31/10/2017 21:43, Daniel Henrique Barboza wrote:
> In the sPAPR guest, events such as device hotplug/unplug are
> retrieved by the check_exception RTAS call after the guest
> receives an IRQ pulse. For both hotplug and unplug operations,
> guest intervention is required to transit the DRC state of
> the attached device to the configured state.
> 
> Without guest intervention, we are still able of qtesting hotplug
> devices in the same manner that we support device hotplug in
> early (pre-CAS) stages.
> 
> Unfortunately, hot unplugs qtests relies on callbacks that demands
> guest intervention to complete - otherwise we risk leaving the guest
> in an inconsistent state that might impact other hotplug/unplug
> operations later on. If we want to make hot unplug qtests we'll
> need to simulate the guest behavior in the scenario in which a
> hot unplug is received, allowing the hot unplug process to go
> as intended.
> 
> This patch is the first step towards hot unplug qtests in sPAPR,
> implementing the check_exception RTAS hypercall in libqos. This
> hypercall is used to fetch events such as hotplug/hot unplug from
> the sPAPR machine after the guest receives an IRQ pulse (or,
> in the case of the test implemented here, we simply know when
> there is/isn't an event to be retrieved).
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> ---
>  tests/libqos/rtas.c | 37 +++++++++++++++++++++++++
>  tests/libqos/rtas.h |  2 ++
>  tests/rtas-test.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 116 insertions(+)
> 
> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
> index 0269803ce0..fdeab448f7 100644
> --- a/tests/libqos/rtas.c
> +++ b/tests/libqos/rtas.c
> @@ -114,3 +114,40 @@ int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
>  
>      return 0;
>  }
> +
> +/*
> + * check_exception as defined by PAPR 2.7+, 7.3.3.2
> + *
> + * nargs = 7 (with Extended Information)
> + * nrets = 1
> + *
> + * arg[2] = mask of event classes to process
> + * arg[4] = real address of error log
> + * arg[5] = length of error log
> + *
> + * arg[0] (Vector Offset), arg[1] and arg[6] (Additional information)
> + * and arg[3] (Critical) aren't used in the logic of check_exception
> + * in hw/ppc/spapr_events.c and can be ignored.
> + *
> + * If there is an event that matches the given mask, check-exception writes
> + * it in buf_addr up to a max of buf_len bytes.
> + *
> + */
> +int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
> +                          uint32_t buf_addr, uint32_t buf_len)
> +{
> +    uint32_t args[7], ret[1];
> +    int res;
> +
> +    args[0] = args[1] = args[3] = args[6] = 0;
> +    args[2] = mask;
> +    args[4] = buf_addr;
> +    args[5] = buf_len;
> +
> +    res = qrtas_call(alloc, "check-exception", 7, args, 1, ret);
> +    if (res != 0) {
> +        return -1;
> +    }
> +
> +    return ret[0];
> +}

I think you should map the qrtas_check_exception() prototype to
"check-exception" parameters, and let the caller to provide vector
offset, additional info, critical if it wants.

> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
> index 498eb19230..330ecfd397 100644
> --- a/tests/libqos/rtas.h
> +++ b/tests/libqos/rtas.h
> @@ -12,4 +12,6 @@ uint32_t qrtas_ibm_read_pci_config(QGuestAllocator *alloc, uint64_t buid,
>                                     uint32_t addr, uint32_t size);
>  int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
>                                 uint32_t addr, uint32_t size, uint32_t val);
> +int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
> +                          uint32_t buf_addr, uint32_t buf_len);
>  #endif /* LIBQOS_RTAS_H */
> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
> index 276c87ef84..c5a6080043 100644
> --- a/tests/rtas-test.c
> +++ b/tests/rtas-test.c
> @@ -5,6 +5,9 @@
>  #include "libqos/libqos-spapr.h"
>  #include "libqos/rtas.h"
>  
> +#define EVENT_MASK_EPOW (1 << 30)
> +#define EVENT_LOG_LEN 2048
> +
>  static void test_rtas_get_time_of_day(void)
>  {
>      QOSState *qs;
> @@ -24,6 +27,76 @@ static void test_rtas_get_time_of_day(void)
>      qtest_shutdown(qs);
>  }
>  
> +static void test_rtas_check_exception_no_events(void)
> +{
> +    QOSState *qs;
> +    uint64_t ret;
> +    uintptr_t guest_buf_addr;
> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
> +
> +    qs = qtest_spapr_boot("-machine pseries");
> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));

why "EVENT_LOG_LEN * sizeof(uint8_t)" and not only EVENT_LOG_LEN?

> +
> +    /*
> +     * mask = 0 should return no events, returning
> +     * RTAS_OUT_NO_ERRORS_FOUND (1).
> +     */
> +    ret = qrtas_check_exception(qs->alloc, 0, guest_buf_addr, EVENT_LOG_LEN);
> +    g_assert_cmpint(ret, ==, 1);
> +
> +    /*
> +     * Using a proper event mask should also return
> +     * no events since no hotplugs happened.
> +     */
> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW, guest_buf_addr,
> +                                EVENT_LOG_LEN);
> +    g_assert_cmpint(ret, ==, 1);
> +
> +    guest_free(qs->alloc, guest_buf_addr);
> +    g_free(buf);
> +
> +    qtest_shutdown(qs);
> +}
> +
> +static void test_rtas_check_exception_hotplug_event(void)
> +{
> +    QOSState *qs;
> +    uint64_t ret;
> +    uintptr_t guest_buf_addr;
> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
> +    uint8_t *zero_buf = g_malloc0(EVENT_LOG_LEN);
> +
> +    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
> +                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
> +
> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
> +
> +    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
> +                         "'core-id':'1'");
> +    /*
> +     * We use EPOW mask instead of HOTPLUG because the code defaults
> +     * the hotplug interrupt source to EPOW if the guest didn't change
> +     * OV5_HP_EVT during CAS.
> +     */
> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
> +                                guest_buf_addr, EVENT_LOG_LEN);
> +
> +    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
> +    guest_free(qs->alloc, guest_buf_addr);
> +
> +    /*
> +     * Calling check_exception after a hotplug needs to return
> +     * RTAS_OUT_SUCCESS (0) and a non-zero error_log.
> +     */
> +    g_assert_cmpint(ret, ==, 0);
> +    g_assert(memcmp(buf, zero_buf, EVENT_LOG_LEN) != 0);

I think you should check the content of the event buffer (at least the
fixed part).

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/4] tests: adding 'check_exception' RTAS implementation

[Daniel Henrique Barboza]

Hi Laurent,

On 11/06/2017 01:12 PM, Laurent Vivier wrote:
> On 31/10/2017 21:43, Daniel Henrique Barboza wrote:
>> In the sPAPR guest, events such as device hotplug/unplug are
>> retrieved by the check_exception RTAS call after the guest
>> receives an IRQ pulse. For both hotplug and unplug operations,
>> guest intervention is required to transit the DRC state of
>> the attached device to the configured state.
>>
>> Without guest intervention, we are still able of qtesting hotplug
>> devices in the same manner that we support device hotplug in
>> early (pre-CAS) stages.
>>
>> Unfortunately, hot unplugs qtests relies on callbacks that demands
>> guest intervention to complete - otherwise we risk leaving the guest
>> in an inconsistent state that might impact other hotplug/unplug
>> operations later on. If we want to make hot unplug qtests we'll
>> need to simulate the guest behavior in the scenario in which a
>> hot unplug is received, allowing the hot unplug process to go
>> as intended.
>>
>> This patch is the first step towards hot unplug qtests in sPAPR,
>> implementing the check_exception RTAS hypercall in libqos. This
>> hypercall is used to fetch events such as hotplug/hot unplug from
>> the sPAPR machine after the guest receives an IRQ pulse (or,
>> in the case of the test implemented here, we simply know when
>> there is/isn't an event to be retrieved).
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
>> ---
>>   tests/libqos/rtas.c | 37 +++++++++++++++++++++++++
>>   tests/libqos/rtas.h |  2 ++
>>   tests/rtas-test.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 116 insertions(+)
>>
>> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
>> index 0269803ce0..fdeab448f7 100644
>> --- a/tests/libqos/rtas.c
>> +++ b/tests/libqos/rtas.c
>> @@ -114,3 +114,40 @@ int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
>>   
>>       return 0;
>>   }
>> +
>> +/*
>> + * check_exception as defined by PAPR 2.7+, 7.3.3.2
>> + *
>> + * nargs = 7 (with Extended Information)
>> + * nrets = 1
>> + *
>> + * arg[2] = mask of event classes to process
>> + * arg[4] = real address of error log
>> + * arg[5] = length of error log
>> + *
>> + * arg[0] (Vector Offset), arg[1] and arg[6] (Additional information)
>> + * and arg[3] (Critical) aren't used in the logic of check_exception
>> + * in hw/ppc/spapr_events.c and can be ignored.
>> + *
>> + * If there is an event that matches the given mask, check-exception writes
>> + * it in buf_addr up to a max of buf_len bytes.
>> + *
>> + */
>> +int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>> +                          uint32_t buf_addr, uint32_t buf_len)
>> +{
>> +    uint32_t args[7], ret[1];
>> +    int res;
>> +
>> +    args[0] = args[1] = args[3] = args[6] = 0;
>> +    args[2] = mask;
>> +    args[4] = buf_addr;
>> +    args[5] = buf_len;
>> +
>> +    res = qrtas_call(alloc, "check-exception", 7, args, 1, ret);
>> +    if (res != 0) {
>> +        return -1;
>> +    }
>> +
>> +    return ret[0];
>> +}
> I think you should map the qrtas_check_exception() prototype to
> "check-exception" parameters, and let the caller to provide vector
> offset, additional info, critical if it wants.
>
>> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
>> index 498eb19230..330ecfd397 100644
>> --- a/tests/libqos/rtas.h
>> +++ b/tests/libqos/rtas.h
>> @@ -12,4 +12,6 @@ uint32_t qrtas_ibm_read_pci_config(QGuestAllocator *alloc, uint64_t buid,
>>                                      uint32_t addr, uint32_t size);
>>   int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
>>                                  uint32_t addr, uint32_t size, uint32_t val);
>> +int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>> +                          uint32_t buf_addr, uint32_t buf_len);
>>   #endif /* LIBQOS_RTAS_H */
>> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
>> index 276c87ef84..c5a6080043 100644
>> --- a/tests/rtas-test.c
>> +++ b/tests/rtas-test.c
>> @@ -5,6 +5,9 @@
>>   #include "libqos/libqos-spapr.h"
>>   #include "libqos/rtas.h"
>>   
>> +#define EVENT_MASK_EPOW (1 << 30)
>> +#define EVENT_LOG_LEN 2048
>> +
>>   static void test_rtas_get_time_of_day(void)
>>   {
>>       QOSState *qs;
>> @@ -24,6 +27,76 @@ static void test_rtas_get_time_of_day(void)
>>       qtest_shutdown(qs);
>>   }
>>   
>> +static void test_rtas_check_exception_no_events(void)
>> +{
>> +    QOSState *qs;
>> +    uint64_t ret;
>> +    uintptr_t guest_buf_addr;
>> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
>> +
>> +    qs = qtest_spapr_boot("-machine pseries");
>> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
> why "EVENT_LOG_LEN * sizeof(uint8_t)" and not only EVENT_LOG_LEN?

Hmpf, EVENT_LEN was being expressed in uint32_t at first, then I've 
changed it to
uint8_t and forgot to remove the sizeof. I'll change it in the next spin.
>
>> +
>> +    /*
>> +     * mask = 0 should return no events, returning
>> +     * RTAS_OUT_NO_ERRORS_FOUND (1).
>> +     */
>> +    ret = qrtas_check_exception(qs->alloc, 0, guest_buf_addr, EVENT_LOG_LEN);
>> +    g_assert_cmpint(ret, ==, 1);
>> +
>> +    /*
>> +     * Using a proper event mask should also return
>> +     * no events since no hotplugs happened.
>> +     */
>> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW, guest_buf_addr,
>> +                                EVENT_LOG_LEN);
>> +    g_assert_cmpint(ret, ==, 1);
>> +
>> +    guest_free(qs->alloc, guest_buf_addr);
>> +    g_free(buf);
>> +
>> +    qtest_shutdown(qs);
>> +}
>> +
>> +static void test_rtas_check_exception_hotplug_event(void)
>> +{
>> +    QOSState *qs;
>> +    uint64_t ret;
>> +    uintptr_t guest_buf_addr;
>> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
>> +    uint8_t *zero_buf = g_malloc0(EVENT_LOG_LEN);
>> +
>> +    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
>> +                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
>> +
>> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
>> +
>> +    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
>> +                         "'core-id':'1'");
>> +    /*
>> +     * We use EPOW mask instead of HOTPLUG because the code defaults
>> +     * the hotplug interrupt source to EPOW if the guest didn't change
>> +     * OV5_HP_EVT during CAS.
>> +     */
>> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
>> +                                guest_buf_addr, EVENT_LOG_LEN);
>> +
>> +    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
>> +    guest_free(qs->alloc, guest_buf_addr);
>> +
>> +    /*
>> +     * Calling check_exception after a hotplug needs to return
>> +     * RTAS_OUT_SUCCESS (0) and a non-zero error_log.
>> +     */
>> +    g_assert_cmpint(ret, ==, 0);
>> +    g_assert(memcmp(buf, zero_buf, EVENT_LOG_LEN) != 0);
> I think you should check the content of the event buffer (at least the
> fixed part).
>
> Thanks,
> Laurent
>

Re: [Qemu-devel] [PATCH v2 1/4] tests: adding 'check_exception' RTAS implementation

[Michael Roth]

Quoting Daniel Henrique Barboza (2017-10-31 15:43:27)
> In the sPAPR guest, events such as device hotplug/unplug are
> retrieved by the check_exception RTAS call after the guest
> receives an IRQ pulse. For both hotplug and unplug operations,
> guest intervention is required to transit the DRC state of
> the attached device to the configured state.
> 
> Without guest intervention, we are still able of qtesting hotplug
> devices in the same manner that we support device hotplug in
> early (pre-CAS) stages.
> 
> Unfortunately, hot unplugs qtests relies on callbacks that demands
> guest intervention to complete - otherwise we risk leaving the guest
> in an inconsistent state that might impact other hotplug/unplug
> operations later on. If we want to make hot unplug qtests we'll
> need to simulate the guest behavior in the scenario in which a
> hot unplug is received, allowing the hot unplug process to go
> as intended.
> 
> This patch is the first step towards hot unplug qtests in sPAPR,
> implementing the check_exception RTAS hypercall in libqos. This
> hypercall is used to fetch events such as hotplug/hot unplug from
> the sPAPR machine after the guest receives an IRQ pulse (or,
> in the case of the test implemented here, we simply know when
> there is/isn't an event to be retrieved).
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> ---
>  tests/libqos/rtas.c | 37 +++++++++++++++++++++++++
>  tests/libqos/rtas.h |  2 ++
>  tests/rtas-test.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 116 insertions(+)
> 
> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
> index 0269803ce0..fdeab448f7 100644
> --- a/tests/libqos/rtas.c
> +++ b/tests/libqos/rtas.c
> @@ -114,3 +114,40 @@ int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
> 
>      return 0;
>  }
> +
> +/*
> + * check_exception as defined by PAPR 2.7+, 7.3.3.2

Now that we have a public LoPAPR I think it's better to reference that
when possible.

[Qemu-devel] [PATCH v2 2/4] tests: adding 'set_indicator' RTAS call

[Daniel Henrique Barboza]

'set_indicator' is a RTAS hypercall that is used to change
the state of both physical and logical devices DRCs by setting
allocation-state, isolation-state and dr-indicator.

This patch implements the set_indicator RTAS call in
tests/libqos/rtas.c, adding its test in tests/rtas-test.c.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 tests/libqos/rtas.c | 33 ++++++++++++++++++++++++
 tests/libqos/rtas.h |  2 ++
 tests/rtas-test.c   | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 108 insertions(+)

diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
index fdeab448f7..ade572a84f 100644
--- a/tests/libqos/rtas.c
+++ b/tests/libqos/rtas.c
@@ -151,3 +151,36 @@ int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
 
     return ret[0];
 }
+
+/*
+ * set_indicator as defined by PAPR 2.7+, 7.3.5.4
+ *
+ * nargs = 3
+ * nrets = 1
+ *
+ * arg[0] = the type of the indicator
+ * arg[1] = index of the specific indicator
+ * arg[2] = desired new state
+ *
+ * Depending on the input, set_indicator will call set_isolation_state,
+ * set_allocation_state or set_dr_indicator in hw/ppc/spapr_drc.c.
+ * These functions allows the guest to control the state of hotplugged
+ * and hot unplugged devices.
+ */
+int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
+                        uint32_t new_state)
+{
+    uint32_t args[3], ret[1];
+    int res;
+
+    args[0] = type;
+    args[1] = idx;
+    args[2] = new_state;
+
+    res = qrtas_call(alloc, "set-indicator", 3, args, 1, ret);
+    if (res != 0) {
+        return -1;
+    }
+
+    return ret[0];
+}
diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
index 330ecfd397..9dfa18f32b 100644
--- a/tests/libqos/rtas.h
+++ b/tests/libqos/rtas.h
@@ -14,4 +14,6 @@ int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
                                uint32_t addr, uint32_t size, uint32_t val);
 int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
                           uint32_t buf_addr, uint32_t buf_len);
+int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
+                        uint32_t new_state);
 #endif /* LIBQOS_RTAS_H */
diff --git a/tests/rtas-test.c b/tests/rtas-test.c
index c5a6080043..2c34b6e83c 100644
--- a/tests/rtas-test.c
+++ b/tests/rtas-test.c
@@ -8,6 +8,13 @@
 #define EVENT_MASK_EPOW (1 << 30)
 #define EVENT_LOG_LEN 2048
 
+#define RTAS_SENSOR_TYPE_ISOLATION_STATE 9001
+#define RTAS_SENSOR_TYPE_ALLOCATION_STATE 9003
+#define SPAPR_DR_ISOLATION_STATE_ISOLATED 0
+#define SPAPR_DR_ALLOCATION_STATE_UNUSABLE 0
+#define SPAPR_DR_ALLOCATION_STATE_USABLE  1
+#define SPAPR_DR_ISOLATION_STATE_UNISOLATED 1
+
 static void test_rtas_get_time_of_day(void)
 {
     QOSState *qs;
@@ -97,6 +104,71 @@ static void test_rtas_check_exception_hotplug_event(void)
     qtest_shutdown(qs);
 }
 
+/*
+ * To test the 'set-indicator' RTAS call we will hotplug a device
+ * (in this case a CPU) and then make its DRC state go from
+ * the starting state UNUSABLE(1) to UNISOLATE(3). These DRC
+ * states transitions are described in further detail in
+ * PAPR 2.7+ 13.4.
+ */
+static void test_rtas_set_indicator(void)
+{
+    QOSState *qs;
+    uint64_t ret;
+    uintptr_t guest_buf_addr;
+    uint32_t drc_index;
+    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
+
+    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
+                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
+
+    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
+    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
+                         "'core-id':'1'");
+
+    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
+                                guest_buf_addr, EVENT_LOG_LEN);
+
+    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
+    guest_free(qs->alloc, guest_buf_addr);
+
+    g_assert_cmpint(ret, ==, 0);
+
+    /*
+     * This time we can't ignore the event log written in the
+     * check_exception call - we need the DRC index of the
+     * recently added CPU to make the state changes using set_indicator.
+     *
+     * A bit of magic to go straight to the DRC index by checking the
+     * error log format in hw/ppc/spapr_events.c:
+     *
+     * - rtas_error_log size = 8 bytes
+     * - all other structures until the hotplug event log = 88 bytes
+     * - inside the hotplug event log, skip 8 + 4 bytes to get to
+     *   the drc_id union.
+     *
+     * This gives us a 108 bytes skip to get the drc info, which is
+     * written in be32.
+     */
+    drc_index = be32toh(*((uint32_t *)(buf + 108)));
+    g_free(buf);
+
+    /*
+     * According to the DRC state diagram, the guest first sets a device
+     * to USABLE (2), then UNISOLATED (3). Both should return
+     * RTAS_OUT_SUCCESS(0).
+     */
+    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ALLOCATION_STATE,
+                              drc_index, SPAPR_DR_ALLOCATION_STATE_USABLE);
+    g_assert_cmpint(ret, ==, 0);
+
+    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ISOLATION_STATE,
+                              drc_index, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
+    g_assert_cmpint(ret, ==, 0);
+
+    qtest_shutdown(qs);
+}
+
 int main(int argc, char *argv[])
 {
     const char *arch = qtest_get_arch();
@@ -112,6 +184,7 @@ int main(int argc, char *argv[])
                    test_rtas_check_exception_no_events);
     qtest_add_func("rtas/rtas-check-exception-hotplug-event",
                    test_rtas_check_exception_hotplug_event);
+    qtest_add_func("rtas/test_rtas_set_indicator", test_rtas_set_indicator);
 
     return g_test_run();
 }
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 2/4] tests: adding 'set_indicator' RTAS call

[Laurent Vivier]

On 31/10/2017 21:43, Daniel Henrique Barboza wrote:
> 'set_indicator' is a RTAS hypercall that is used to change
> the state of both physical and logical devices DRCs by setting
> allocation-state, isolation-state and dr-indicator.
> 
> This patch implements the set_indicator RTAS call in
> tests/libqos/rtas.c, adding its test in tests/rtas-test.c.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> ---
>  tests/libqos/rtas.c | 33 ++++++++++++++++++++++++
>  tests/libqos/rtas.h |  2 ++
>  tests/rtas-test.c   | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 108 insertions(+)
> 
> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
> index fdeab448f7..ade572a84f 100644
> --- a/tests/libqos/rtas.c
> +++ b/tests/libqos/rtas.c
> @@ -151,3 +151,36 @@ int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>  
>      return ret[0];
>  }
> +
> +/*
> + * set_indicator as defined by PAPR 2.7+, 7.3.5.4
> + *
> + * nargs = 3
> + * nrets = 1
> + *
> + * arg[0] = the type of the indicator
> + * arg[1] = index of the specific indicator
> + * arg[2] = desired new state
> + *
> + * Depending on the input, set_indicator will call set_isolation_state,
> + * set_allocation_state or set_dr_indicator in hw/ppc/spapr_drc.c.
> + * These functions allows the guest to control the state of hotplugged
> + * and hot unplugged devices.
> + */
> +int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
> +                        uint32_t new_state)
> +{
> +    uint32_t args[3], ret[1];
> +    int res;
> +
> +    args[0] = type;
> +    args[1] = idx;
> +    args[2] = new_state;
> +
> +    res = qrtas_call(alloc, "set-indicator", 3, args, 1, ret);
> +    if (res != 0) {
> +        return -1;
> +    }
> +
> +    return ret[0];
> +}
> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
> index 330ecfd397..9dfa18f32b 100644
> --- a/tests/libqos/rtas.h
> +++ b/tests/libqos/rtas.h
> @@ -14,4 +14,6 @@ int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
>                                 uint32_t addr, uint32_t size, uint32_t val);
>  int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>                            uint32_t buf_addr, uint32_t buf_len);
> +int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
> +                        uint32_t new_state);
>  #endif /* LIBQOS_RTAS_H */
> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
> index c5a6080043..2c34b6e83c 100644
> --- a/tests/rtas-test.c
> +++ b/tests/rtas-test.c
> @@ -8,6 +8,13 @@
>  #define EVENT_MASK_EPOW (1 << 30)
>  #define EVENT_LOG_LEN 2048
>  
> +#define RTAS_SENSOR_TYPE_ISOLATION_STATE 9001
> +#define RTAS_SENSOR_TYPE_ALLOCATION_STATE 9003
> +#define SPAPR_DR_ISOLATION_STATE_ISOLATED 0
> +#define SPAPR_DR_ALLOCATION_STATE_UNUSABLE 0
> +#define SPAPR_DR_ALLOCATION_STATE_USABLE  1
> +#define SPAPR_DR_ISOLATION_STATE_UNISOLATED 1
> +
>  static void test_rtas_get_time_of_day(void)
>  {
>      QOSState *qs;
> @@ -97,6 +104,71 @@ static void test_rtas_check_exception_hotplug_event(void)
>      qtest_shutdown(qs);
>  }
>  
> +/*
> + * To test the 'set-indicator' RTAS call we will hotplug a device
> + * (in this case a CPU) and then make its DRC state go from
> + * the starting state UNUSABLE(1) to UNISOLATE(3). These DRC
> + * states transitions are described in further detail in
> + * PAPR 2.7+ 13.4.
> + */
> +static void test_rtas_set_indicator(void)
> +{
> +    QOSState *qs;
> +    uint64_t ret;
> +    uintptr_t guest_buf_addr;
> +    uint32_t drc_index;
> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
> +
> +    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
> +                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
> +
> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
> +    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
> +                         "'core-id':'1'");
> +
> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
> +                                guest_buf_addr, EVENT_LOG_LEN);
> +
> +    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
> +    guest_free(qs->alloc, guest_buf_addr);
> +
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    /*
> +     * This time we can't ignore the event log written in the
> +     * check_exception call - we need the DRC index of the
> +     * recently added CPU to make the state changes using set_indicator.
> +     *
> +     * A bit of magic to go straight to the DRC index by checking the
> +     * error log format in hw/ppc/spapr_events.c:
> +     *
> +     * - rtas_error_log size = 8 bytes
> +     * - all other structures until the hotplug event log = 88 bytes
> +     * - inside the hotplug event log, skip 8 + 4 bytes to get to
> +     *   the drc_id union.
> +     *
> +     * This gives us a 108 bytes skip to get the drc info, which is
> +     * written in be32.
> +     */
> +    drc_index = be32toh(*((uint32_t *)(buf + 108)));
> +    g_free(buf);

It's test program, so I think you should really check the content of
rtas_error_log (initiator field), the content of the extended (check Log
Valid, Log Format identifier, Company identifier, ...) and the content
of the Platform Event Log (Logical Resource Identitifcation: Section Id,
Resource Type). Then you can get Logical CPU Id. You should also check
you can hotplug memory (perhaps in the previous patch too).
> +
> +    /*
> +     * According to the DRC state diagram, the guest first sets a device
> +     * to USABLE (2), then UNISOLATED (3). Both should return
> +     * RTAS_OUT_SUCCESS(0).
> +     */
> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ALLOCATION_STATE,
> +                              drc_index, SPAPR_DR_ALLOCATION_STATE_USABLE);
> +    g_assert_cmpint(ret, ==, 0);

According to the diagram, you need get-sensor-state to check the initial
state (UNUSABLE).

> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ISOLATION_STATE,
> +                              drc_index, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> +    g_assert_cmpint(ret, ==, 0);
> +

And the you can also check it has moved to UNISOLATE.

> +    qtest_shutdown(qs);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      const char *arch = qtest_get_arch();
> @@ -112,6 +184,7 @@ int main(int argc, char *argv[])
>                     test_rtas_check_exception_no_events);
>      qtest_add_func("rtas/rtas-check-exception-hotplug-event",
>                     test_rtas_check_exception_hotplug_event);
> +    qtest_add_func("rtas/test_rtas_set_indicator", test_rtas_set_indicator);
>  
>      return g_test_run();
>  }
> 

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/4] tests: adding 'set_indicator' RTAS call

[Daniel Henrique Barboza]

Hi Laurent,

On 11/06/2017 02:54 PM, Laurent Vivier wrote:
> On 31/10/2017 21:43, Daniel Henrique Barboza wrote:
>> 'set_indicator' is a RTAS hypercall that is used to change
>> the state of both physical and logical devices DRCs by setting
>> allocation-state, isolation-state and dr-indicator.
>>
>> This patch implements the set_indicator RTAS call in
>> tests/libqos/rtas.c, adding its test in tests/rtas-test.c.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
>> ---
>>   tests/libqos/rtas.c | 33 ++++++++++++++++++++++++
>>   tests/libqos/rtas.h |  2 ++
>>   tests/rtas-test.c   | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 108 insertions(+)
>>
>> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
>> index fdeab448f7..ade572a84f 100644
>> --- a/tests/libqos/rtas.c
>> +++ b/tests/libqos/rtas.c
>> @@ -151,3 +151,36 @@ int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>>   
>>       return ret[0];
>>   }
>> +
>> +/*
>> + * set_indicator as defined by PAPR 2.7+, 7.3.5.4
>> + *
>> + * nargs = 3
>> + * nrets = 1
>> + *
>> + * arg[0] = the type of the indicator
>> + * arg[1] = index of the specific indicator
>> + * arg[2] = desired new state
>> + *
>> + * Depending on the input, set_indicator will call set_isolation_state,
>> + * set_allocation_state or set_dr_indicator in hw/ppc/spapr_drc.c.
>> + * These functions allows the guest to control the state of hotplugged
>> + * and hot unplugged devices.
>> + */
>> +int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
>> +                        uint32_t new_state)
>> +{
>> +    uint32_t args[3], ret[1];
>> +    int res;
>> +
>> +    args[0] = type;
>> +    args[1] = idx;
>> +    args[2] = new_state;
>> +
>> +    res = qrtas_call(alloc, "set-indicator", 3, args, 1, ret);
>> +    if (res != 0) {
>> +        return -1;
>> +    }
>> +
>> +    return ret[0];
>> +}
>> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
>> index 330ecfd397..9dfa18f32b 100644
>> --- a/tests/libqos/rtas.h
>> +++ b/tests/libqos/rtas.h
>> @@ -14,4 +14,6 @@ int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
>>                                  uint32_t addr, uint32_t size, uint32_t val);
>>   int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>>                             uint32_t buf_addr, uint32_t buf_len);
>> +int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
>> +                        uint32_t new_state);
>>   #endif /* LIBQOS_RTAS_H */
>> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
>> index c5a6080043..2c34b6e83c 100644
>> --- a/tests/rtas-test.c
>> +++ b/tests/rtas-test.c
>> @@ -8,6 +8,13 @@
>>   #define EVENT_MASK_EPOW (1 << 30)
>>   #define EVENT_LOG_LEN 2048
>>   
>> +#define RTAS_SENSOR_TYPE_ISOLATION_STATE 9001
>> +#define RTAS_SENSOR_TYPE_ALLOCATION_STATE 9003
>> +#define SPAPR_DR_ISOLATION_STATE_ISOLATED 0
>> +#define SPAPR_DR_ALLOCATION_STATE_UNUSABLE 0
>> +#define SPAPR_DR_ALLOCATION_STATE_USABLE  1
>> +#define SPAPR_DR_ISOLATION_STATE_UNISOLATED 1
>> +
>>   static void test_rtas_get_time_of_day(void)
>>   {
>>       QOSState *qs;
>> @@ -97,6 +104,71 @@ static void test_rtas_check_exception_hotplug_event(void)
>>       qtest_shutdown(qs);
>>   }
>>   
>> +/*
>> + * To test the 'set-indicator' RTAS call we will hotplug a device
>> + * (in this case a CPU) and then make its DRC state go from
>> + * the starting state UNUSABLE(1) to UNISOLATE(3). These DRC
>> + * states transitions are described in further detail in
>> + * PAPR 2.7+ 13.4.
>> + */
>> +static void test_rtas_set_indicator(void)
>> +{
>> +    QOSState *qs;
>> +    uint64_t ret;
>> +    uintptr_t guest_buf_addr;
>> +    uint32_t drc_index;
>> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
>> +
>> +    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
>> +                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
>> +
>> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
>> +    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
>> +                         "'core-id':'1'");
>> +
>> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
>> +                                guest_buf_addr, EVENT_LOG_LEN);
>> +
>> +    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
>> +    guest_free(qs->alloc, guest_buf_addr);
>> +
>> +    g_assert_cmpint(ret, ==, 0);
>> +
>> +    /*
>> +     * This time we can't ignore the event log written in the
>> +     * check_exception call - we need the DRC index of the
>> +     * recently added CPU to make the state changes using set_indicator.
>> +     *
>> +     * A bit of magic to go straight to the DRC index by checking the
>> +     * error log format in hw/ppc/spapr_events.c:
>> +     *
>> +     * - rtas_error_log size = 8 bytes
>> +     * - all other structures until the hotplug event log = 88 bytes
>> +     * - inside the hotplug event log, skip 8 + 4 bytes to get to
>> +     *   the drc_id union.
>> +     *
>> +     * This gives us a 108 bytes skip to get the drc info, which is
>> +     * written in be32.
>> +     */
>> +    drc_index = be32toh(*((uint32_t *)(buf + 108)));
>> +    g_free(buf);
> It's test program, so I think you should really check the content of
> rtas_error_log (initiator field), the content of the extended (check Log
> Valid, Log Format identifier, Company identifier, ...) and the content
> of the Platform Event Log (Logical Resource Identitifcation: Section Id,
> Resource Type). Then you can get Logical CPU Id. You should also check
> you can hotplug memory (perhaps in the previous patch too).
Yeah, after reading your reviews in the 3 patches I realized that I need to
do more checks to fully assert the hypercall behavior. My mindset when 
making
this was "what do I need to make device hotplug tests" and I ended up
overlooking/ignoring the checks that makes sense for the hypercalls but
not necessarily related to the device hotplug process.

About hotplugging memory: I tried making a LMB hotplug test and seeing
an error "Memory hotplug not supported for this machine". At the time I 
thought
it had something to do with not going through CAS before but now I realize
this doesn't make sense because we can do LMB hotplug when the machine
is started with -S. I'll revisit this behavior in the next spin too.

>> +
>> +    /*
>> +     * According to the DRC state diagram, the guest first sets a device
>> +     * to USABLE (2), then UNISOLATED (3). Both should return
>> +     * RTAS_OUT_SUCCESS(0).
>> +     */
>> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ALLOCATION_STATE,
>> +                              drc_index, SPAPR_DR_ALLOCATION_STATE_USABLE);
>> +    g_assert_cmpint(ret, ==, 0);
> According to the diagram, you need get-sensor-state to check the initial
> state (UNUSABLE).

RIght, we would need to implement the libqos version of get-sensor-state 
to do
that. I'll get it done in v3.


Thanks,


Daniel

>> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ISOLATION_STATE,
>> +                              drc_index, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
>> +    g_assert_cmpint(ret, ==, 0);
>> +
> And the you can also check it has moved to UNISOLATE.
>
>> +    qtest_shutdown(qs);
>> +}
>> +
>>   int main(int argc, char *argv[])
>>   {
>>       const char *arch = qtest_get_arch();
>> @@ -112,6 +184,7 @@ int main(int argc, char *argv[])
>>                      test_rtas_check_exception_no_events);
>>       qtest_add_func("rtas/rtas-check-exception-hotplug-event",
>>                      test_rtas_check_exception_hotplug_event);
>> +    qtest_add_func("rtas/test_rtas_set_indicator", test_rtas_set_indicator);
>>   
>>       return g_test_run();
>>   }
>>
> Thanks,
> Laurent
>
>

Re: [Qemu-devel] [PATCH v2 2/4] tests: adding 'set_indicator' RTAS call

[Michael Roth]

Quoting Daniel Henrique Barboza (2017-10-31 15:43:28)
> 'set_indicator' is a RTAS hypercall that is used to change
> the state of both physical and logical devices DRCs by setting
> allocation-state, isolation-state and dr-indicator.
> 
> This patch implements the set_indicator RTAS call in
> tests/libqos/rtas.c, adding its test in tests/rtas-test.c.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> ---
>  tests/libqos/rtas.c | 33 ++++++++++++++++++++++++
>  tests/libqos/rtas.h |  2 ++
>  tests/rtas-test.c   | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 108 insertions(+)
> 
> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
> index fdeab448f7..ade572a84f 100644
> --- a/tests/libqos/rtas.c
> +++ b/tests/libqos/rtas.c
> @@ -151,3 +151,36 @@ int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
> 
>      return ret[0];
>  }
> +
> +/*
> + * set_indicator as defined by PAPR 2.7+, 7.3.5.4
> + *
> + * nargs = 3
> + * nrets = 1
> + *
> + * arg[0] = the type of the indicator
> + * arg[1] = index of the specific indicator
> + * arg[2] = desired new state
> + *
> + * Depending on the input, set_indicator will call set_isolation_state,
> + * set_allocation_state or set_dr_indicator in hw/ppc/spapr_drc.c.
> + * These functions allows the guest to control the state of hotplugged
> + * and hot unplugged devices.
> + */
> +int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
> +                        uint32_t new_state)
> +{
> +    uint32_t args[3], ret[1];
> +    int res;
> +
> +    args[0] = type;
> +    args[1] = idx;
> +    args[2] = new_state;
> +
> +    res = qrtas_call(alloc, "set-indicator", 3, args, 1, ret);
> +    if (res != 0) {
> +        return -1;
> +    }
> +
> +    return ret[0];
> +}
> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
> index 330ecfd397..9dfa18f32b 100644
> --- a/tests/libqos/rtas.h
> +++ b/tests/libqos/rtas.h
> @@ -14,4 +14,6 @@ int qrtas_ibm_write_pci_config(QGuestAllocator *alloc, uint64_t buid,
>                                 uint32_t addr, uint32_t size, uint32_t val);
>  int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>                            uint32_t buf_addr, uint32_t buf_len);
> +int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
> +                        uint32_t new_state);
>  #endif /* LIBQOS_RTAS_H */
> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
> index c5a6080043..2c34b6e83c 100644
> --- a/tests/rtas-test.c
> +++ b/tests/rtas-test.c
> @@ -8,6 +8,13 @@
>  #define EVENT_MASK_EPOW (1 << 30)
>  #define EVENT_LOG_LEN 2048
> 
> +#define RTAS_SENSOR_TYPE_ISOLATION_STATE 9001
> +#define RTAS_SENSOR_TYPE_ALLOCATION_STATE 9003
> +#define SPAPR_DR_ISOLATION_STATE_ISOLATED 0
> +#define SPAPR_DR_ALLOCATION_STATE_UNUSABLE 0
> +#define SPAPR_DR_ALLOCATION_STATE_USABLE  1
> +#define SPAPR_DR_ISOLATION_STATE_UNISOLATED 1
> +
>  static void test_rtas_get_time_of_day(void)
>  {
>      QOSState *qs;
> @@ -97,6 +104,71 @@ static void test_rtas_check_exception_hotplug_event(void)
>      qtest_shutdown(qs);
>  }
> 
> +/*
> + * To test the 'set-indicator' RTAS call we will hotplug a device
> + * (in this case a CPU) and then make its DRC state go from
> + * the starting state UNUSABLE(1) to UNISOLATE(3). These DRC
> + * states transitions are described in further detail in
> + * PAPR 2.7+ 13.4.
> + */
> +static void test_rtas_set_indicator(void)
> +{
> +    QOSState *qs;
> +    uint64_t ret;
> +    uintptr_t guest_buf_addr;
> +    uint32_t drc_index;
> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
> +
> +    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
> +                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
> +
> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
> +    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
> +                         "'core-id':'1'");
> +
> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
> +                                guest_buf_addr, EVENT_LOG_LEN);
> +
> +    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
> +    guest_free(qs->alloc, guest_buf_addr);
> +
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    /*
> +     * This time we can't ignore the event log written in the
> +     * check_exception call - we need the DRC index of the
> +     * recently added CPU to make the state changes using set_indicator.
> +     *
> +     * A bit of magic to go straight to the DRC index by checking the
> +     * error log format in hw/ppc/spapr_events.c:
> +     *
> +     * - rtas_error_log size = 8 bytes
> +     * - all other structures until the hotplug event log = 88 bytes
> +     * - inside the hotplug event log, skip 8 + 4 bytes to get to
> +     *   the drc_id union.
> +     *
> +     * This gives us a 108 bytes skip to get the drc info, which is
> +     * written in be32.
> +     */
> +    drc_index = be32toh(*((uint32_t *)(buf + 108)));

Definitely agree some helpers to parse/validate the event log structure would
be helpful here. I think you can pretty much cast to a struct epow_extended_log
if you break out some of the definitions in spapr_events.c.

> +    g_free(buf);
> +
> +    /*
> +     * According to the DRC state diagram, the guest first sets a device
> +     * to USABLE (2), then UNISOLATED (3). Both should return
> +     * RTAS_OUT_SUCCESS(0).
> +     */
> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ALLOCATION_STATE,
> +                              drc_index, SPAPR_DR_ALLOCATION_STATE_USABLE);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ISOLATION_STATE,
> +                              drc_index, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> +    g_assert_cmpint(ret, ==, 0);

Would be good to also validate handling for some of the invalid transitions
too, like unisolating a device that's in an unusable state, eventually
at least.

> +
> +    qtest_shutdown(qs);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      const char *arch = qtest_get_arch();
> @@ -112,6 +184,7 @@ int main(int argc, char *argv[])
>                     test_rtas_check_exception_no_events);
>      qtest_add_func("rtas/rtas-check-exception-hotplug-event",
>                     test_rtas_check_exception_hotplug_event);
> +    qtest_add_func("rtas/test_rtas_set_indicator", test_rtas_set_indicator);
> 
>      return g_test_run();
>  }
> -- 
> 2.13.6
> 

[Qemu-devel] [PATCH v2 3/4] tests: ibm, configure-connector RTAS call implementation

[Daniel Henrique Barboza]

'ibm,configure-connector' hypercall is used by the guest OS
to start the configuration of a given device, where the
machine configures the said device and all its sub-devices,
giving back FDTs to the caller for each sub-device.

This hypercall is supposed to be called multiple times by the
guest OS until it returns RTAS_OUT_SUCESS (code 0), indicating
that the device is now properly configured and ready
to be used, or a return value < 0 when an error occurs.

This patch implements the 'ibm,configure-connector' RTAS
hypercall in tests/libqos/rtas.c, with an extra test
case for it inside tests/rtas-tests.c.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 tests/libqos/rtas.c | 35 +++++++++++++++++++++++++++
 tests/libqos/rtas.h |  1 +
 tests/rtas-test.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+)

diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
index ade572a84f..1cb9e2b495 100644
--- a/tests/libqos/rtas.c
+++ b/tests/libqos/rtas.c
@@ -184,3 +184,38 @@ int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
 
     return ret[0];
 }
+
+/*
+ * ibm,configure-connector as defined by PAPR 2.7+, 13.5.3.5
+ *
+ * nargs = 2
+ * nrets = 1
+ *
+ * args[0] and args[1] compose the 64 bit Work Area address.
+ *
+ * This call will configure not only the device reported in the first
+ * offset of the Work Area but all of its siblingis as well, returning
+ * the FDT of each configured sub-device as well as a return code
+ * 'Next child', 'Next property' or 'Previous parent'. When the whole
+ * configuration is done, 'Configuration completed' (0) is returned.
+ *
+ * configure-connector will always reply with status code 'Next child'(2)
+ * on the first successful call. The DRC configuration will be
+ * completed when configure-connector returns status 0. Any return
+ * status < 0 indicates an error.
+ */
+int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr)
+{
+    uint32_t args[2], ret[1];
+    int res;
+
+    args[0] = (uint32_t)(wa_addr);
+    args[1] = (uint32_t)(wa_addr >> 32);
+
+    res = qrtas_call(alloc, "ibm,configure-connector", 2, args, 1, ret);
+    if (res != 0) {
+        return -1;
+    }
+
+    return ret[0];
+}
diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
index 9dfa18f32b..35c44fb967 100644
--- a/tests/libqos/rtas.h
+++ b/tests/libqos/rtas.h
@@ -16,4 +16,5 @@ int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
                           uint32_t buf_addr, uint32_t buf_len);
 int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
                         uint32_t new_state);
+int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr);
 #endif /* LIBQOS_RTAS_H */
diff --git a/tests/rtas-test.c b/tests/rtas-test.c
index 2c34b6e83c..b3538bf878 100644
--- a/tests/rtas-test.c
+++ b/tests/rtas-test.c
@@ -15,6 +15,8 @@
 #define SPAPR_DR_ALLOCATION_STATE_USABLE  1
 #define SPAPR_DR_ISOLATION_STATE_UNISOLATED 1
 
+#define CC_WA_LEN 4096
+
 static void test_rtas_get_time_of_day(void)
 {
     QOSState *qs;
@@ -169,6 +171,70 @@ static void test_rtas_set_indicator(void)
     qtest_shutdown(qs);
 }
 
+static void test_rtas_ibm_configure_connector(void)
+{
+    QOSState *qs;
+    uint64_t ret;
+    uintptr_t guest_buf_addr, guest_drc_addr;
+    uint32_t drc_index;
+    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
+
+    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
+                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
+
+    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
+    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
+                         "'core-id':'1'");
+
+    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
+                                guest_buf_addr, EVENT_LOG_LEN);
+
+    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
+    guest_free(qs->alloc, guest_buf_addr);
+
+    g_assert_cmpint(ret, ==, 0);
+
+    /*
+     * Same 108 bytes offset magic used and explained in
+     * test_rtas_set_indicator.
+     */
+    drc_index = be32toh(*((uint32_t *)(buf + 108)));
+    g_free(buf);
+
+    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ALLOCATION_STATE,
+                              drc_index, SPAPR_DR_ALLOCATION_STATE_USABLE);
+    g_assert_cmpint(ret, ==, 0);
+
+    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ISOLATION_STATE,
+                              drc_index, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
+    g_assert_cmpint(ret, ==, 0);
+
+    /*
+     * Call ibm,configure-connector to finish the hotplugged device
+     * configuration, putting its DRC into 'ready' state.
+     *
+     * We're not interested in the generated FDTs during the config
+     * process, thus we simply keep calling configure-connector
+     * until it returns SUCCESS(0) or an error.
+     *
+     * The full explanation logic behind this process can be found
+     * at PAPR 2.7+, 13.5.3.5.
+     */
+    guest_drc_addr = guest_alloc(qs->alloc, CC_WA_LEN * sizeof(uint32_t));
+    writel(guest_drc_addr, drc_index);
+    writel(guest_drc_addr + sizeof(uint32_t), 0);
+
+    do {
+        ret = qrtas_ibm_configure_connector(qs->alloc, guest_drc_addr);
+    } while (ret > 0);
+
+    guest_free(qs->alloc, guest_drc_addr);
+
+    g_assert_cmpint(ret, ==, 0);
+
+    qtest_shutdown(qs);
+}
+
 int main(int argc, char *argv[])
 {
     const char *arch = qtest_get_arch();
@@ -185,6 +251,8 @@ int main(int argc, char *argv[])
     qtest_add_func("rtas/rtas-check-exception-hotplug-event",
                    test_rtas_check_exception_hotplug_event);
     qtest_add_func("rtas/test_rtas_set_indicator", test_rtas_set_indicator);
+    qtest_add_func("rtas/test_rtas_ibm_configure_connector",
+                   test_rtas_ibm_configure_connector);
 
     return g_test_run();
 }
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 3/4] tests: ibm, configure-connector RTAS call implementation

[Laurent Vivier]

On 31/10/2017 21:43, Daniel Henrique Barboza wrote:
> 'ibm,configure-connector' hypercall is used by the guest OS
> to start the configuration of a given device, where the
> machine configures the said device and all its sub-devices,
> giving back FDTs to the caller for each sub-device.
> 
> This hypercall is supposed to be called multiple times by the
> guest OS until it returns RTAS_OUT_SUCESS (code 0), indicating
> that the device is now properly configured and ready
> to be used, or a return value < 0 when an error occurs.
> 
> This patch implements the 'ibm,configure-connector' RTAS
> hypercall in tests/libqos/rtas.c, with an extra test
> case for it inside tests/rtas-tests.c.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> ---
>  tests/libqos/rtas.c | 35 +++++++++++++++++++++++++++
>  tests/libqos/rtas.h |  1 +
>  tests/rtas-test.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+)
> 
> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
> index ade572a84f..1cb9e2b495 100644
> --- a/tests/libqos/rtas.c
> +++ b/tests/libqos/rtas.c
> @@ -184,3 +184,38 @@ int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
>  
>      return ret[0];
>  }
> +
> +/*
> + * ibm,configure-connector as defined by PAPR 2.7+, 13.5.3.5
> + *
> + * nargs = 2
> + * nrets = 1
> + *
> + * args[0] and args[1] compose the 64 bit Work Area address.
> + *
> + * This call will configure not only the device reported in the first
> + * offset of the Work Area but all of its siblingis as well, returning
> + * the FDT of each configured sub-device as well as a return code
> + * 'Next child', 'Next property' or 'Previous parent'. When the whole
> + * configuration is done, 'Configuration completed' (0) is returned.
> + *
> + * configure-connector will always reply with status code 'Next child'(2)
> + * on the first successful call. The DRC configuration will be
> + * completed when configure-connector returns status 0. Any return
> + * status < 0 indicates an error.
> + */
> +int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr)
> +{
> +    uint32_t args[2], ret[1];
> +    int res;
> +
> +    args[0] = (uint32_t)(wa_addr);
> +    args[1] = (uint32_t)(wa_addr >> 32);

This part looks strange to me. I agree it's what qemu does, but
according to spec args[0] is "Address of work area" and args[1] "0 or
address of additional page":

Linux on Power Architecture Platform Reference
Version 1.1
24 March 2016

13.5.3.5 ibm,configure-connector RTAS Call
The “need more memory” status code, is similar in semantics to the “call
again” status. However, on the next ibm,configure-connector call, the OS
will supply, via the Memory extent parameter, the address of another
page of memory for RTAS to add to the work area in order for
configuration to continue. On all other calls to ibm,configure-connector
the contents of the Memory extent parameter should be 0.

> +    res = qrtas_call(alloc, "ibm,configure-connector", 2, args, 1, ret);
> +    if (res != 0) {
> +        return -1;
> +    }
> +
> +    return ret[0];
> +}
> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
> index 9dfa18f32b..35c44fb967 100644
> --- a/tests/libqos/rtas.h
> +++ b/tests/libqos/rtas.h
> @@ -16,4 +16,5 @@ int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>                            uint32_t buf_addr, uint32_t buf_len);
>  int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
>                          uint32_t new_state);
> +int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr);
>  #endif /* LIBQOS_RTAS_H */
> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
> index 2c34b6e83c..b3538bf878 100644
> --- a/tests/rtas-test.c
> +++ b/tests/rtas-test.c
> @@ -15,6 +15,8 @@
>  #define SPAPR_DR_ALLOCATION_STATE_USABLE  1
>  #define SPAPR_DR_ISOLATION_STATE_UNISOLATED 1
>  
> +#define CC_WA_LEN 4096
> +
>  static void test_rtas_get_time_of_day(void)
>  {
>      QOSState *qs;
> @@ -169,6 +171,70 @@ static void test_rtas_set_indicator(void)
>      qtest_shutdown(qs);
>  }
>  
> +static void test_rtas_ibm_configure_connector(void)
> +{
> +    QOSState *qs;
> +    uint64_t ret;
> +    uintptr_t guest_buf_addr, guest_drc_addr;
> +    uint32_t drc_index;
> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
> +
> +    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
> +                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
> +
> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
> +    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
> +                         "'core-id':'1'");
> +
> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
> +                                guest_buf_addr, EVENT_LOG_LEN);
> +
> +    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
> +    guest_free(qs->alloc, guest_buf_addr);
> +
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    /*
> +     * Same 108 bytes offset magic used and explained in
> +     * test_rtas_set_indicator.
> +     */
> +    drc_index = be32toh(*((uint32_t *)(buf + 108)));
> +    g_free(buf);
> +
> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ALLOCATION_STATE,
> +                              drc_index, SPAPR_DR_ALLOCATION_STATE_USABLE);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ISOLATION_STATE,
> +                              drc_index, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    /*
> +     * Call ibm,configure-connector to finish the hotplugged device
> +     * configuration, putting its DRC into 'ready' state.
> +     *
> +     * We're not interested in the generated FDTs during the config
> +     * process, thus we simply keep calling configure-connector
> +     * until it returns SUCCESS(0) or an error.
> +     *
> +     * The full explanation logic behind this process can be found
> +     * at PAPR 2.7+, 13.5.3.5.
> +     */
> +    guest_drc_addr = guest_alloc(qs->alloc, CC_WA_LEN * sizeof(uint32_t));

CC_WA_LEN is already 4096, why "* sizeof(uint32_t"?
It must be aligned to a 4096 bytes boundaries.

> +    writel(guest_drc_addr, drc_index);
> +    writel(guest_drc_addr + sizeof(uint32_t), 0)> +
> +    do {
> +        ret = qrtas_ibm_configure_connector(qs->alloc, guest_drc_addr);
> +    } while (ret > 0);

I think it would be interesting to check the result of the call (return
value, node name, property name,...)

And you should also exit with error in the case of ret == 5 (need more
memory).

> +    guest_free(qs->alloc, guest_drc_addr);
> +
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    qtest_shutdown(qs);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      const char *arch = qtest_get_arch();
> @@ -185,6 +251,8 @@ int main(int argc, char *argv[])
>      qtest_add_func("rtas/rtas-check-exception-hotplug-event",
>                     test_rtas_check_exception_hotplug_event);
>      qtest_add_func("rtas/test_rtas_set_indicator", test_rtas_set_indicator);
> +    qtest_add_func("rtas/test_rtas_ibm_configure_connector",
> +                   test_rtas_ibm_configure_connector);
>  
>      return g_test_run();
>  }
> 

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 3/4] tests: ibm, configure-connector RTAS call implementation

[Daniel Henrique Barboza]

On 11/06/2017 03:46 PM, Laurent Vivier wrote:
> On 31/10/2017 21:43, Daniel Henrique Barboza wrote:
>> 'ibm,configure-connector' hypercall is used by the guest OS
>> to start the configuration of a given device, where the
>> machine configures the said device and all its sub-devices,
>> giving back FDTs to the caller for each sub-device.
>>
>> This hypercall is supposed to be called multiple times by the
>> guest OS until it returns RTAS_OUT_SUCESS (code 0), indicating
>> that the device is now properly configured and ready
>> to be used, or a return value < 0 when an error occurs.
>>
>> This patch implements the 'ibm,configure-connector' RTAS
>> hypercall in tests/libqos/rtas.c, with an extra test
>> case for it inside tests/rtas-tests.c.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
>> ---
>>   tests/libqos/rtas.c | 35 +++++++++++++++++++++++++++
>>   tests/libqos/rtas.h |  1 +
>>   tests/rtas-test.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 104 insertions(+)
>>
>> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
>> index ade572a84f..1cb9e2b495 100644
>> --- a/tests/libqos/rtas.c
>> +++ b/tests/libqos/rtas.c
>> @@ -184,3 +184,38 @@ int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
>>   
>>       return ret[0];
>>   }
>> +
>> +/*
>> + * ibm,configure-connector as defined by PAPR 2.7+, 13.5.3.5
>> + *
>> + * nargs = 2
>> + * nrets = 1
>> + *
>> + * args[0] and args[1] compose the 64 bit Work Area address.
>> + *
>> + * This call will configure not only the device reported in the first
>> + * offset of the Work Area but all of its siblingis as well, returning
>> + * the FDT of each configured sub-device as well as a return code
>> + * 'Next child', 'Next property' or 'Previous parent'. When the whole
>> + * configuration is done, 'Configuration completed' (0) is returned.
>> + *
>> + * configure-connector will always reply with status code 'Next child'(2)
>> + * on the first successful call. The DRC configuration will be
>> + * completed when configure-connector returns status 0. Any return
>> + * status < 0 indicates an error.
>> + */
>> +int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr)
>> +{
>> +    uint32_t args[2], ret[1];
>> +    int res;
>> +
>> +    args[0] = (uint32_t)(wa_addr);
>> +    args[1] = (uint32_t)(wa_addr >> 32);
> This part looks strange to me. I agree it's what qemu does, but
> according to spec args[0] is "Address of work area" and args[1] "0 or
> address of additional page":
>
> Linux on Power Architecture Platform Reference
> Version 1.1
> 24 March 2016
>
> 13.5.3.5 ibm,configure-connector RTAS Call
> The “need more memory” status code, is similar in semantics to the “call
> again” status. However, on the next ibm,configure-connector call, the OS
> will supply, via the Memory extent parameter, the address of another
> page of memory for RTAS to add to the work area in order for
> configuration to continue. On all other calls to ibm,configure-connector
> the contents of the Memory extent parameter should be 0.

Hmmmmm thinking about it, the reason why it works in QEMU is because
wa_addr >> 32 will always be zero, perhaps?

At any rate, In this test, makes sense to leave args[1] to be set by the 
caller,
setting it to 0 in the non "need more memory" scenario.


>
>> +    res = qrtas_call(alloc, "ibm,configure-connector", 2, args, 1, ret);
>> +    if (res != 0) {
>> +        return -1;
>> +    }
>> +
>> +    return ret[0];
>> +}
>> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
>> index 9dfa18f32b..35c44fb967 100644
>> --- a/tests/libqos/rtas.h
>> +++ b/tests/libqos/rtas.h
>> @@ -16,4 +16,5 @@ int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>>                             uint32_t buf_addr, uint32_t buf_len);
>>   int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
>>                           uint32_t new_state);
>> +int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr);
>>   #endif /* LIBQOS_RTAS_H */
>> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
>> index 2c34b6e83c..b3538bf878 100644
>> --- a/tests/rtas-test.c
>> +++ b/tests/rtas-test.c
>> @@ -15,6 +15,8 @@
>>   #define SPAPR_DR_ALLOCATION_STATE_USABLE  1
>>   #define SPAPR_DR_ISOLATION_STATE_UNISOLATED 1
>>   
>> +#define CC_WA_LEN 4096
>> +
>>   static void test_rtas_get_time_of_day(void)
>>   {
>>       QOSState *qs;
>> @@ -169,6 +171,70 @@ static void test_rtas_set_indicator(void)
>>       qtest_shutdown(qs);
>>   }
>>   
>> +static void test_rtas_ibm_configure_connector(void)
>> +{
>> +    QOSState *qs;
>> +    uint64_t ret;
>> +    uintptr_t guest_buf_addr, guest_drc_addr;
>> +    uint32_t drc_index;
>> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
>> +
>> +    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
>> +                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
>> +
>> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
>> +    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
>> +                         "'core-id':'1'");
>> +
>> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
>> +                                guest_buf_addr, EVENT_LOG_LEN);
>> +
>> +    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
>> +    guest_free(qs->alloc, guest_buf_addr);
>> +
>> +    g_assert_cmpint(ret, ==, 0);
>> +
>> +    /*
>> +     * Same 108 bytes offset magic used and explained in
>> +     * test_rtas_set_indicator.
>> +     */
>> +    drc_index = be32toh(*((uint32_t *)(buf + 108)));
>> +    g_free(buf);
>> +
>> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ALLOCATION_STATE,
>> +                              drc_index, SPAPR_DR_ALLOCATION_STATE_USABLE);
>> +    g_assert_cmpint(ret, ==, 0);
>> +
>> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ISOLATION_STATE,
>> +                              drc_index, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
>> +    g_assert_cmpint(ret, ==, 0);
>> +
>> +    /*
>> +     * Call ibm,configure-connector to finish the hotplugged device
>> +     * configuration, putting its DRC into 'ready' state.
>> +     *
>> +     * We're not interested in the generated FDTs during the config
>> +     * process, thus we simply keep calling configure-connector
>> +     * until it returns SUCCESS(0) or an error.
>> +     *
>> +     * The full explanation logic behind this process can be found
>> +     * at PAPR 2.7+, 13.5.3.5.
>> +     */
>> +    guest_drc_addr = guest_alloc(qs->alloc, CC_WA_LEN * sizeof(uint32_t));
> CC_WA_LEN is already 4096, why "* sizeof(uint32_t"?
> It must be aligned to a 4096 bytes boundaries.

Good catch. I'll fix it.

>
>> +    writel(guest_drc_addr, drc_index);
>> +    writel(guest_drc_addr + sizeof(uint32_t), 0)> +
>> +    do {
>> +        ret = qrtas_ibm_configure_connector(qs->alloc, guest_drc_addr);
>> +    } while (ret > 0);
> I think it would be interesting to check the result of the call (return
> value, node name, property name,...)
>
> And you should also exit with error in the case of ret == 5 (need more
> memory).
>
>> +    guest_free(qs->alloc, guest_drc_addr);
>> +
>> +    g_assert_cmpint(ret, ==, 0);
>> +
>> +    qtest_shutdown(qs);
>> +}
>> +
>>   int main(int argc, char *argv[])
>>   {
>>       const char *arch = qtest_get_arch();
>> @@ -185,6 +251,8 @@ int main(int argc, char *argv[])
>>       qtest_add_func("rtas/rtas-check-exception-hotplug-event",
>>                      test_rtas_check_exception_hotplug_event);
>>       qtest_add_func("rtas/test_rtas_set_indicator", test_rtas_set_indicator);
>> +    qtest_add_func("rtas/test_rtas_ibm_configure_connector",
>> +                   test_rtas_ibm_configure_connector);
>>   
>>       return g_test_run();
>>   }
>>
> Thanks,
> Laurent
>

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 3/4] tests: ibm, configure-connector RTAS call implementation

[Michael Roth]

Quoting Daniel Henrique Barboza (2017-11-09 06:35:30)
> 
> 
> On 11/06/2017 03:46 PM, Laurent Vivier wrote:
> > On 31/10/2017 21:43, Daniel Henrique Barboza wrote:
> >> 'ibm,configure-connector' hypercall is used by the guest OS
> >> to start the configuration of a given device, where the
> >> machine configures the said device and all its sub-devices,
> >> giving back FDTs to the caller for each sub-device.
> >>
> >> This hypercall is supposed to be called multiple times by the
> >> guest OS until it returns RTAS_OUT_SUCESS (code 0), indicating
> >> that the device is now properly configured and ready
> >> to be used, or a return value < 0 when an error occurs.
> >>
> >> This patch implements the 'ibm,configure-connector' RTAS
> >> hypercall in tests/libqos/rtas.c, with an extra test
> >> case for it inside tests/rtas-tests.c.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> >> ---
> >>   tests/libqos/rtas.c | 35 +++++++++++++++++++++++++++
> >>   tests/libqos/rtas.h |  1 +
> >>   tests/rtas-test.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 104 insertions(+)
> >>
> >> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
> >> index ade572a84f..1cb9e2b495 100644
> >> --- a/tests/libqos/rtas.c
> >> +++ b/tests/libqos/rtas.c
> >> @@ -184,3 +184,38 @@ int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
> >>   
> >>       return ret[0];
> >>   }
> >> +
> >> +/*
> >> + * ibm,configure-connector as defined by PAPR 2.7+, 13.5.3.5
> >> + *
> >> + * nargs = 2
> >> + * nrets = 1
> >> + *
> >> + * args[0] and args[1] compose the 64 bit Work Area address.
> >> + *
> >> + * This call will configure not only the device reported in the first
> >> + * offset of the Work Area but all of its siblingis as well, returning
> >> + * the FDT of each configured sub-device as well as a return code
> >> + * 'Next child', 'Next property' or 'Previous parent'. When the whole
> >> + * configuration is done, 'Configuration completed' (0) is returned.
> >> + *
> >> + * configure-connector will always reply with status code 'Next child'(2)
> >> + * on the first successful call. The DRC configuration will be
> >> + * completed when configure-connector returns status 0. Any return
> >> + * status < 0 indicates an error.
> >> + */
> >> +int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr)
> >> +{
> >> +    uint32_t args[2], ret[1];
> >> +    int res;
> >> +
> >> +    args[0] = (uint32_t)(wa_addr);
> >> +    args[1] = (uint32_t)(wa_addr >> 32);
> > This part looks strange to me. I agree it's what qemu does, but
> > according to spec args[0] is "Address of work area" and args[1] "0 or
> > address of additional page":
> >
> > Linux on Power Architecture Platform Reference
> > Version 1.1
> > 24 March 2016
> >
> > 13.5.3.5 ibm,configure-connector RTAS Call
> > The “need more memory” status code, is similar in semantics to the “call
> > again” status. However, on the next ibm,configure-connector call, the OS
> > will supply, via the Memory extent parameter, the address of another
> > page of memory for RTAS to add to the work area in order for
> > configuration to continue. On all other calls to ibm,configure-connector
> > the contents of the Memory extent parameter should be 0.
> 
> Hmmmmm thinking about it, the reason why it works in QEMU is because
> wa_addr >> 32 will always be zero, perhaps?

It looks that way. At least with linux guests the "need more memory"
handling remains unimplemented, so we only ever end up dealing with:

  rc = rtas_call(cc_token, 2, 1, NULL, rtas_data_buf, NULL);

which effectively sets args[1] to 0. Still worth fixing though of
course.

> 
> At any rate, In this test, makes sense to leave args[1] to be set by the 
> caller,
> setting it to 0 in the non "need more memory" scenario.
> 
> 
> >
> >> +    res = qrtas_call(alloc, "ibm,configure-connector", 2, args, 1, ret);
> >> +    if (res != 0) {
> >> +        return -1;
> >> +    }
> >> +
> >> +    return ret[0];
> >> +}
> >> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
> >> index 9dfa18f32b..35c44fb967 100644
> >> --- a/tests/libqos/rtas.h
> >> +++ b/tests/libqos/rtas.h
> >> @@ -16,4 +16,5 @@ int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
> >>                             uint32_t buf_addr, uint32_t buf_len);
> >>   int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
> >>                           uint32_t new_state);
> >> +int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr);
> >>   #endif /* LIBQOS_RTAS_H */
> >> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
> >> index 2c34b6e83c..b3538bf878 100644
> >> --- a/tests/rtas-test.c
> >> +++ b/tests/rtas-test.c
> >> @@ -15,6 +15,8 @@
> >>   #define SPAPR_DR_ALLOCATION_STATE_USABLE  1
> >>   #define SPAPR_DR_ISOLATION_STATE_UNISOLATED 1
> >>   
> >> +#define CC_WA_LEN 4096
> >> +
> >>   static void test_rtas_get_time_of_day(void)
> >>   {
> >>       QOSState *qs;
> >> @@ -169,6 +171,70 @@ static void test_rtas_set_indicator(void)
> >>       qtest_shutdown(qs);
> >>   }
> >>   
> >> +static void test_rtas_ibm_configure_connector(void)
> >> +{
> >> +    QOSState *qs;
> >> +    uint64_t ret;
> >> +    uintptr_t guest_buf_addr, guest_drc_addr;
> >> +    uint32_t drc_index;
> >> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
> >> +
> >> +    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
> >> +                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
> >> +
> >> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
> >> +    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
> >> +                         "'core-id':'1'");
> >> +
> >> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
> >> +                                guest_buf_addr, EVENT_LOG_LEN);
> >> +
> >> +    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
> >> +    guest_free(qs->alloc, guest_buf_addr);
> >> +
> >> +    g_assert_cmpint(ret, ==, 0);
> >> +
> >> +    /*
> >> +     * Same 108 bytes offset magic used and explained in
> >> +     * test_rtas_set_indicator.
> >> +     */
> >> +    drc_index = be32toh(*((uint32_t *)(buf + 108)));
> >> +    g_free(buf);
> >> +
> >> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ALLOCATION_STATE,
> >> +                              drc_index, SPAPR_DR_ALLOCATION_STATE_USABLE);
> >> +    g_assert_cmpint(ret, ==, 0);
> >> +
> >> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ISOLATION_STATE,
> >> +                              drc_index, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> >> +    g_assert_cmpint(ret, ==, 0);
> >> +
> >> +    /*
> >> +     * Call ibm,configure-connector to finish the hotplugged device
> >> +     * configuration, putting its DRC into 'ready' state.
> >> +     *
> >> +     * We're not interested in the generated FDTs during the config
> >> +     * process, thus we simply keep calling configure-connector
> >> +     * until it returns SUCCESS(0) or an error.
> >> +     *
> >> +     * The full explanation logic behind this process can be found
> >> +     * at PAPR 2.7+, 13.5.3.5.
> >> +     */
> >> +    guest_drc_addr = guest_alloc(qs->alloc, CC_WA_LEN * sizeof(uint32_t));
> > CC_WA_LEN is already 4096, why "* sizeof(uint32_t"?
> > It must be aligned to a 4096 bytes boundaries.
> 
> Good catch. I'll fix it.
> 
> >
> >> +    writel(guest_drc_addr, drc_index);
> >> +    writel(guest_drc_addr + sizeof(uint32_t), 0)> +
> >> +    do {
> >> +        ret = qrtas_ibm_configure_connector(qs->alloc, guest_drc_addr);
> >> +    } while (ret > 0);
> > I think it would be interesting to check the result of the call (return
> > value, node name, property name,...)
> >
> > And you should also exit with error in the case of ret == 5 (need more
> > memory).
> >
> >> +    guest_free(qs->alloc, guest_drc_addr);
> >> +
> >> +    g_assert_cmpint(ret, ==, 0);
> >> +
> >> +    qtest_shutdown(qs);
> >> +}
> >> +
> >>   int main(int argc, char *argv[])
> >>   {
> >>       const char *arch = qtest_get_arch();
> >> @@ -185,6 +251,8 @@ int main(int argc, char *argv[])
> >>       qtest_add_func("rtas/rtas-check-exception-hotplug-event",
> >>                      test_rtas_check_exception_hotplug_event);
> >>       qtest_add_func("rtas/test_rtas_set_indicator", test_rtas_set_indicator);
> >> +    qtest_add_func("rtas/test_rtas_ibm_configure_connector",
> >> +                   test_rtas_ibm_configure_connector);
> >>   
> >>       return g_test_run();
> >>   }
> >>
> > Thanks,
> > Laurent
> >
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 3/4] tests: ibm, configure-connector RTAS call implementation

[Daniel Henrique Barboza]

On 11/28/2017 10:02 PM, Michael Roth wrote:
> Quoting Daniel Henrique Barboza (2017-11-09 06:35:30)
>>
>> On 11/06/2017 03:46 PM, Laurent Vivier wrote:
>>> On 31/10/2017 21:43, Daniel Henrique Barboza wrote:
>>>> 'ibm,configure-connector' hypercall is used by the guest OS
>>>> to start the configuration of a given device, where the
>>>> machine configures the said device and all its sub-devices,
>>>> giving back FDTs to the caller for each sub-device.
>>>>
>>>> This hypercall is supposed to be called multiple times by the
>>>> guest OS until it returns RTAS_OUT_SUCESS (code 0), indicating
>>>> that the device is now properly configured and ready
>>>> to be used, or a return value < 0 when an error occurs.
>>>>
>>>> This patch implements the 'ibm,configure-connector' RTAS
>>>> hypercall in tests/libqos/rtas.c, with an extra test
>>>> case for it inside tests/rtas-tests.c.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
>>>> ---
>>>>    tests/libqos/rtas.c | 35 +++++++++++++++++++++++++++
>>>>    tests/libqos/rtas.h |  1 +
>>>>    tests/rtas-test.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 104 insertions(+)
>>>>
>>>> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
>>>> index ade572a84f..1cb9e2b495 100644
>>>> --- a/tests/libqos/rtas.c
>>>> +++ b/tests/libqos/rtas.c
>>>> @@ -184,3 +184,38 @@ int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
>>>>    
>>>>        return ret[0];
>>>>    }
>>>> +
>>>> +/*
>>>> + * ibm,configure-connector as defined by PAPR 2.7+, 13.5.3.5
>>>> + *
>>>> + * nargs = 2
>>>> + * nrets = 1
>>>> + *
>>>> + * args[0] and args[1] compose the 64 bit Work Area address.
>>>> + *
>>>> + * This call will configure not only the device reported in the first
>>>> + * offset of the Work Area but all of its siblingis as well, returning
>>>> + * the FDT of each configured sub-device as well as a return code
>>>> + * 'Next child', 'Next property' or 'Previous parent'. When the whole
>>>> + * configuration is done, 'Configuration completed' (0) is returned.
>>>> + *
>>>> + * configure-connector will always reply with status code 'Next child'(2)
>>>> + * on the first successful call. The DRC configuration will be
>>>> + * completed when configure-connector returns status 0. Any return
>>>> + * status < 0 indicates an error.
>>>> + */
>>>> +int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr)
>>>> +{
>>>> +    uint32_t args[2], ret[1];
>>>> +    int res;
>>>> +
>>>> +    args[0] = (uint32_t)(wa_addr);
>>>> +    args[1] = (uint32_t)(wa_addr >> 32);
>>> This part looks strange to me. I agree it's what qemu does, but
>>> according to spec args[0] is "Address of work area" and args[1] "0 or
>>> address of additional page":
>>>
>>> Linux on Power Architecture Platform Reference
>>> Version 1.1
>>> 24 March 2016
>>>
>>> 13.5.3.5 ibm,configure-connector RTAS Call
>>> The “need more memory” status code, is similar in semantics to the “call
>>> again” status. However, on the next ibm,configure-connector call, the OS
>>> will supply, via the Memory extent parameter, the address of another
>>> page of memory for RTAS to add to the work area in order for
>>> configuration to continue. On all other calls to ibm,configure-connector
>>> the contents of the Memory extent parameter should be 0.
>> Hmmmmm thinking about it, the reason why it works in QEMU is because
>> wa_addr >> 32 will always be zero, perhaps?
> It looks that way. At least with linux guests the "need more memory"
> handling remains unimplemented, so we only ever end up dealing with:
>
>    rc = rtas_call(cc_token, 2, 1, NULL, rtas_data_buf, NULL);
>
> which effectively sets args[1] to 0. Still worth fixing though of
> course.

Thanks for the info Mike. Yeah, this is a good TODO.


Daniel
>
>> At any rate, In this test, makes sense to leave args[1] to be set by the
>> caller,
>> setting it to 0 in the non "need more memory" scenario.
>>
>>
>>>> +    res = qrtas_call(alloc, "ibm,configure-connector", 2, args, 1, ret);
>>>> +    if (res != 0) {
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    return ret[0];
>>>> +}
>>>> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
>>>> index 9dfa18f32b..35c44fb967 100644
>>>> --- a/tests/libqos/rtas.h
>>>> +++ b/tests/libqos/rtas.h
>>>> @@ -16,4 +16,5 @@ int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>>>>                              uint32_t buf_addr, uint32_t buf_len);
>>>>    int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
>>>>                            uint32_t new_state);
>>>> +int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr);
>>>>    #endif /* LIBQOS_RTAS_H */
>>>> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
>>>> index 2c34b6e83c..b3538bf878 100644
>>>> --- a/tests/rtas-test.c
>>>> +++ b/tests/rtas-test.c
>>>> @@ -15,6 +15,8 @@
>>>>    #define SPAPR_DR_ALLOCATION_STATE_USABLE  1
>>>>    #define SPAPR_DR_ISOLATION_STATE_UNISOLATED 1
>>>>    
>>>> +#define CC_WA_LEN 4096
>>>> +
>>>>    static void test_rtas_get_time_of_day(void)
>>>>    {
>>>>        QOSState *qs;
>>>> @@ -169,6 +171,70 @@ static void test_rtas_set_indicator(void)
>>>>        qtest_shutdown(qs);
>>>>    }
>>>>    
>>>> +static void test_rtas_ibm_configure_connector(void)
>>>> +{
>>>> +    QOSState *qs;
>>>> +    uint64_t ret;
>>>> +    uintptr_t guest_buf_addr, guest_drc_addr;
>>>> +    uint32_t drc_index;
>>>> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
>>>> +
>>>> +    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
>>>> +                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
>>>> +
>>>> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
>>>> +    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
>>>> +                         "'core-id':'1'");
>>>> +
>>>> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
>>>> +                                guest_buf_addr, EVENT_LOG_LEN);
>>>> +
>>>> +    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
>>>> +    guest_free(qs->alloc, guest_buf_addr);
>>>> +
>>>> +    g_assert_cmpint(ret, ==, 0);
>>>> +
>>>> +    /*
>>>> +     * Same 108 bytes offset magic used and explained in
>>>> +     * test_rtas_set_indicator.
>>>> +     */
>>>> +    drc_index = be32toh(*((uint32_t *)(buf + 108)));
>>>> +    g_free(buf);
>>>> +
>>>> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ALLOCATION_STATE,
>>>> +                              drc_index, SPAPR_DR_ALLOCATION_STATE_USABLE);
>>>> +    g_assert_cmpint(ret, ==, 0);
>>>> +
>>>> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ISOLATION_STATE,
>>>> +                              drc_index, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
>>>> +    g_assert_cmpint(ret, ==, 0);
>>>> +
>>>> +    /*
>>>> +     * Call ibm,configure-connector to finish the hotplugged device
>>>> +     * configuration, putting its DRC into 'ready' state.
>>>> +     *
>>>> +     * We're not interested in the generated FDTs during the config
>>>> +     * process, thus we simply keep calling configure-connector
>>>> +     * until it returns SUCCESS(0) or an error.
>>>> +     *
>>>> +     * The full explanation logic behind this process can be found
>>>> +     * at PAPR 2.7+, 13.5.3.5.
>>>> +     */
>>>> +    guest_drc_addr = guest_alloc(qs->alloc, CC_WA_LEN * sizeof(uint32_t));
>>> CC_WA_LEN is already 4096, why "* sizeof(uint32_t"?
>>> It must be aligned to a 4096 bytes boundaries.
>> Good catch. I'll fix it.
>>
>>>> +    writel(guest_drc_addr, drc_index);
>>>> +    writel(guest_drc_addr + sizeof(uint32_t), 0)> +
>>>> +    do {
>>>> +        ret = qrtas_ibm_configure_connector(qs->alloc, guest_drc_addr);
>>>> +    } while (ret > 0);
>>> I think it would be interesting to check the result of the call (return
>>> value, node name, property name,...)
>>>
>>> And you should also exit with error in the case of ret == 5 (need more
>>> memory).
>>>
>>>> +    guest_free(qs->alloc, guest_drc_addr);
>>>> +
>>>> +    g_assert_cmpint(ret, ==, 0);
>>>> +
>>>> +    qtest_shutdown(qs);
>>>> +}
>>>> +
>>>>    int main(int argc, char *argv[])
>>>>    {
>>>>        const char *arch = qtest_get_arch();
>>>> @@ -185,6 +251,8 @@ int main(int argc, char *argv[])
>>>>        qtest_add_func("rtas/rtas-check-exception-hotplug-event",
>>>>                       test_rtas_check_exception_hotplug_event);
>>>>        qtest_add_func("rtas/test_rtas_set_indicator", test_rtas_set_indicator);
>>>> +    qtest_add_func("rtas/test_rtas_ibm_configure_connector",
>>>> +                   test_rtas_ibm_configure_connector);
>>>>    
>>>>        return g_test_run();
>>>>    }
>>>>
>>> Thanks,
>>> Laurent
>>>
>

Re: [Qemu-devel] [PATCH v2 3/4] tests: ibm, configure-connector RTAS call implementation

[Michael Roth]

Quoting Daniel Henrique Barboza (2017-10-31 15:43:29)
> 'ibm,configure-connector' hypercall is used by the guest OS
> to start the configuration of a given device, where the
> machine configures the said device and all its sub-devices,
> giving back FDTs to the caller for each sub-device.
> 
> This hypercall is supposed to be called multiple times by the
> guest OS until it returns RTAS_OUT_SUCESS (code 0), indicating
> that the device is now properly configured and ready
> to be used, or a return value < 0 when an error occurs.
> 
> This patch implements the 'ibm,configure-connector' RTAS
> hypercall in tests/libqos/rtas.c, with an extra test
> case for it inside tests/rtas-tests.c.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> ---
>  tests/libqos/rtas.c | 35 +++++++++++++++++++++++++++
>  tests/libqos/rtas.h |  1 +
>  tests/rtas-test.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+)
> 
> diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
> index ade572a84f..1cb9e2b495 100644
> --- a/tests/libqos/rtas.c
> +++ b/tests/libqos/rtas.c
> @@ -184,3 +184,38 @@ int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
> 
>      return ret[0];
>  }
> +
> +/*
> + * ibm,configure-connector as defined by PAPR 2.7+, 13.5.3.5
> + *
> + * nargs = 2
> + * nrets = 1
> + *
> + * args[0] and args[1] compose the 64 bit Work Area address.
> + *
> + * This call will configure not only the device reported in the first
> + * offset of the Work Area but all of its siblingis as well, returning
> + * the FDT of each configured sub-device as well as a return code
> + * 'Next child', 'Next property' or 'Previous parent'. When the whole
> + * configuration is done, 'Configuration completed' (0) is returned.
> + *
> + * configure-connector will always reply with status code 'Next child'(2)
> + * on the first successful call. The DRC configuration will be
> + * completed when configure-connector returns status 0. Any return
> + * status < 0 indicates an error.
> + */
> +int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr)
> +{
> +    uint32_t args[2], ret[1];
> +    int res;
> +
> +    args[0] = (uint32_t)(wa_addr);
> +    args[1] = (uint32_t)(wa_addr >> 32);
> +
> +    res = qrtas_call(alloc, "ibm,configure-connector", 2, args, 1, ret);
> +    if (res != 0) {
> +        return -1;
> +    }
> +
> +    return ret[0];
> +}
> diff --git a/tests/libqos/rtas.h b/tests/libqos/rtas.h
> index 9dfa18f32b..35c44fb967 100644
> --- a/tests/libqos/rtas.h
> +++ b/tests/libqos/rtas.h
> @@ -16,4 +16,5 @@ int qrtas_check_exception(QGuestAllocator *alloc, uint32_t mask,
>                            uint32_t buf_addr, uint32_t buf_len);
>  int qrtas_set_indicator(QGuestAllocator *alloc, uint32_t type, uint32_t idx,
>                          uint32_t new_state);
> +int qrtas_ibm_configure_connector(QGuestAllocator *alloc, uintptr_t wa_addr);
>  #endif /* LIBQOS_RTAS_H */
> diff --git a/tests/rtas-test.c b/tests/rtas-test.c
> index 2c34b6e83c..b3538bf878 100644
> --- a/tests/rtas-test.c
> +++ b/tests/rtas-test.c
> @@ -15,6 +15,8 @@
>  #define SPAPR_DR_ALLOCATION_STATE_USABLE  1
>  #define SPAPR_DR_ISOLATION_STATE_UNISOLATED 1
> 
> +#define CC_WA_LEN 4096
> +
>  static void test_rtas_get_time_of_day(void)
>  {
>      QOSState *qs;
> @@ -169,6 +171,70 @@ static void test_rtas_set_indicator(void)
>      qtest_shutdown(qs);
>  }
> 
> +static void test_rtas_ibm_configure_connector(void)
> +{
> +    QOSState *qs;
> +    uint64_t ret;
> +    uintptr_t guest_buf_addr, guest_drc_addr;
> +    uint32_t drc_index;
> +    uint8_t *buf = g_malloc0(EVENT_LOG_LEN);
> +
> +    qs = qtest_spapr_boot("-machine pseries -cpu POWER8_v2.0 "
> +                          "-smp 1,sockets=4,cores=1,threads=1,maxcpus=4");
> +
> +    guest_buf_addr = guest_alloc(qs->alloc, EVENT_LOG_LEN * sizeof(uint8_t));
> +    qtest_qmp_device_add("power8_v2.0-spapr-cpu-core", "id-1",
> +                         "'core-id':'1'");
> +
> +    ret = qrtas_check_exception(qs->alloc, EVENT_MASK_EPOW,
> +                                guest_buf_addr, EVENT_LOG_LEN);
> +
> +    memread(guest_buf_addr, buf, EVENT_LOG_LEN);
> +    guest_free(qs->alloc, guest_buf_addr);
> +
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    /*
> +     * Same 108 bytes offset magic used and explained in
> +     * test_rtas_set_indicator.
> +     */
> +    drc_index = be32toh(*((uint32_t *)(buf + 108)));
> +    g_free(buf);
> +
> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ALLOCATION_STATE,
> +                              drc_index, SPAPR_DR_ALLOCATION_STATE_USABLE);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    ret = qrtas_set_indicator(qs->alloc, RTAS_SENSOR_TYPE_ISOLATION_STATE,
> +                              drc_index, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    /*
> +     * Call ibm,configure-connector to finish the hotplugged device
> +     * configuration, putting its DRC into 'ready' state.
> +     *
> +     * We're not interested in the generated FDTs during the config
> +     * process, thus we simply keep calling configure-connector
> +     * until it returns SUCCESS(0) or an error.
> +     *
> +     * The full explanation logic behind this process can be found
> +     * at PAPR 2.7+, 13.5.3.5.
> +     */
> +    guest_drc_addr = guest_alloc(qs->alloc, CC_WA_LEN * sizeof(uint32_t));
> +    writel(guest_drc_addr, drc_index);
> +    writel(guest_drc_addr + sizeof(uint32_t), 0);
> +
> +    do {
> +        ret = qrtas_ibm_configure_connector(qs->alloc, guest_drc_addr);
> +    } while (ret > 0);

This should result in a transition to a 'configured' state, but it's
hard to probe that sort of stuff using the rtas interfaces since it's
sort of an internal state that the guest doesn't set/read directly. I
think it makes sense focusing on making sure everything is coherant
from a guest perspective here, but maybe it would be good to eventually
add a set of drc/* tests to validate things like the drc->state
transitions themselves under various normal/edge cases.

> +
> +    guest_free(qs->alloc, guest_drc_addr);
> +
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    qtest_shutdown(qs);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      const char *arch = qtest_get_arch();
> @@ -185,6 +251,8 @@ int main(int argc, char *argv[])
>      qtest_add_func("rtas/rtas-check-exception-hotplug-event",
>                     test_rtas_check_exception_hotplug_event);
>      qtest_add_func("rtas/test_rtas_set_indicator", test_rtas_set_indicator);
> +    qtest_add_func("rtas/test_rtas_ibm_configure_connector",
> +                   test_rtas_ibm_configure_connector);
> 
>      return g_test_run();
>  }
> -- 
> 2.13.6
> 

[Qemu-devel] [PATCH v2 4/4] tests/rtas-test.c: fix Apple endian.h include

[Daniel Henrique Barboza]

Recent rtas-test.c changes added a call to 'be32toh', a function
from the header 'endian.h' in Linux. This caused QEMU Travis build
to break because be32toh is undefined in Mac OS (even using the
machine/endian.h header).

This patch makes a conditional code to include the proper header
file if we're compiling in an __APPLE__ env.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 tests/rtas-test.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tests/rtas-test.c b/tests/rtas-test.c
index b3538bf878..511980ad17 100644
--- a/tests/rtas-test.c
+++ b/tests/rtas-test.c
@@ -5,6 +5,17 @@
 #include "libqos/libqos-spapr.h"
 #include "libqos/rtas.h"
 
+#if defined(__APPLE__)
+#    include <libkern/OSByteOrder.h>
+
+#    define be32toh(x) OSSwapBigToHostInt32(x)
+#    define __BYTE_ORDER    BYTE_ORDER
+#    define __BIG_ENDIAN    BIG_ENDIAN
+#    define __LITTLE_ENDIAN LITTLE_ENDIAN
+#else
+#    include <endian.h>
+#endif
+
 #define EVENT_MASK_EPOW (1 << 30)
 #define EVENT_LOG_LEN 2048
 
-- 
2.13.6