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.
