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

[Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>]

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
>