On Thu, Nov 26, 2020 at 01:54:55AM -0600, Shivaprasad G Bhat wrote:
> The patch adds support for async hcalls at the DRC level for the
> spapr devices. To be used by spapr-scm devices in the patch/es to follow.
>
> Signed-off-by: Shivaprasad G Bhat <sb...@linux.ibm.com>
> ---
> hw/ppc/spapr_drc.c | 146
> ++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_drc.h | 25 ++++++++
> 2 files changed, 171 insertions(+)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 77718cde1f..2cecccf701 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -15,6 +15,7 @@
> #include "qapi/qmp/qnull.h"
> #include "cpu.h"
> #include "qemu/cutils.h"
> +#include "qemu/guest-random.h"
> #include "hw/ppc/spapr_drc.h"
> #include "qom/object.h"
> #include "migration/vmstate.h"
> @@ -421,6 +422,145 @@ void spapr_drc_detach(SpaprDrc *drc)
> spapr_drc_release(drc);
> }
>
> +
> +/*
> + * @drc : device DRC targetting which the async hcalls to be made.
> + *
> + * All subsequent requests to run/query the status should use the
> + * unique token returned here.
> + */
> +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> +{
> + Error *err = NULL;
> + uint64_t token;
> + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> +
> + state = g_malloc0(sizeof(*state));
> + state->pending = true;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> +retry:
> + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> + error_report_err(err);
> + g_free(state);
> + return 0;
> + }
Returning w/o releasing the lock.
> +
> + if (!token) /* Token should be non-zero */
> + goto retry;
> +
> + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> + if (tmp->continue_token == token) {
> + /* If the token already in use, get a new one */
> + goto retry;
> + }
> + }
> + }
> +
> + state->continue_token = token;
> + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> +
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +
> + return state->continue_token;
> +}
> +
> +static void *spapr_drc_async_hcall_runner(void *opaque)
> +{
> + int response = -1;
> + SpaprDrcDeviceAsyncHCallState *state = opaque;
> +
> + /*
> + * state is freed only after this thread finishes(after pthread_join()),
> + * don't worry about it becoming NULL.
> + */
> +
> + response = state->func(state->data);
> +
> + state->hcall_ret = response;
> + state->pending = 0;
> +
> + return NULL;
> +}
> +
> +/*
> + * @drc : device DRC targetting which the async hcalls to be made.
> + * token : The continue token to be used for tracking as recived from
> + * spapr_drc_get_new_async_hcall_token
> + * @func() : the worker function which needs to be executed asynchronously
> + * @data : data to be passed to the asynchronous function. Worker is supposed
> + * to free/cleanup the data that is passed here
> + */
> +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> + SpaprDrcAsyncHcallWorkerFunc *func, void
> *data)
> +{
> + SpaprDrcDeviceAsyncHCallState *state, *next;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> + if (state->continue_token == token) {
> + state->func = func;
> + state->data = data;
> + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> + spapr_drc_async_hcall_runner, state,
> + QEMU_THREAD_JOINABLE);
> + break;
> + }
> + }
Looks like QLIST_FOREACH should be enough here as you don't
seem to be removing any list entry in this path.
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +}
> +
> +/*
> + * spapr_drc_finish_async_hcalls
> + * Waits for all pending async requests to complete
> + * thier execution and free the states
> + */
> +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> +{
> + SpaprDrcDeviceAsyncHCallState *state, *next;
> +
> + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> + return;
> + }
> +
> + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> + qemu_thread_join(&state->thread);
> + QLIST_REMOVE(state, node);
> + g_free(state);
> + }
> +}
Why is it safe to iterate the list here w/o the lock?
Regards,
Bharata.
