Re: [Qemu-devel] [RFC PATCH 11/34] hyperv: add synic message delivery

[Roman Kagan]

On Wed, Feb 07, 2018 at 11:58:08AM +0100, Paolo Bonzini wrote:
> On 06/02/2018 21:30, Roman Kagan wrote:
> > +
> > +    HvSintMsgCb msg_cb;
> > +    void *msg_cb_data;
> > +    struct hyperv_message *msg;
> > +    /*
> > +     * the state of the message staged in .msg:
> > +     * 0        - the staging area is not in use (after init or message
> > +     *            successfully delivered to guest)
> > +     * -EBUSY   - the staging area is being used in vcpu thread
> > +     * -EAGAIN  - delivery attempt failed due to slot being busy, retry
> > +     * -EXXXX   - error
> > +     */
> > +    int msg_status;
> > +
> 
> Access to these fields needs to be protected by a mutex (the refcount is
> okay because it is only released in the bottom half).

Hmm, I'll double-check, but the original idea was that this stuff is
only used/ref-d/unref-d in the main thread so no mutex was necessary.

> Please also add
> comments regarding which fields are read-only, which are accessed under
> BQL, etc.
> 
> Also, msg_status's handling of EBUSY/EAGAIN is a bit strange, like:
> 
> +    if (ret == -EBUSY) {
> +        return -EAGAIN;
> +    }
> +    if (ret) {
> +        return ret;
> +    }
> 
> I wonder if it would be better to change -EBUSY to 1, or to split
> msg_status into two fields, such as msg_status (filled by the vCPU
> thread) and msg_state which can be HYPERV_MSG_STATE_{FREE,BUSY,POSTED}.
> msg_status is only valid if the state is POSTED, and sint_msg_bh then
> moves the state back to FREE.

Fair enough, that code isn't easy to follow.  I'll give this your
suggestion a try.

Thanks,
Roman.