Re: [Qemu-devel] [PATCH 05/10] hyperv: add synic message delivery

[Roman Kagan]

On Wed, Oct 03, 2018 at 01:06:58PM +0200, Paolo Bonzini wrote:
> On 21/09/2018 10:22, Roman Kagan wrote:
> > +typedef struct HvSintStagedMesage {
> > +    /* message content staged by hyperv_post_msg */
> > +    struct hyperv_message msg;
> > +    /* callback + data (r/o) to complete the processing in a BH */
> > +    HvSintMsgCb cb;
> > +    void *cb_data;
> > +    /* message posting status filled by cpu_post_msg */
> > +    int status;
> > +    /* passing the buck: */
> > +    enum {
> > +        /* initial state */
> > +        HV_STAGED_MSG_FREE,
> > +        /*
> > +         * hyperv_post_msg (e.g. in main loop) grabs the staged area (FREE 
> > ->
> > +         * BUSY), copies msg, and schedules cpu_post_msg on the assigned 
> > cpu
> > +         */
> > +        HV_STAGED_MSG_BUSY,
> > +        /*
> > +         * cpu_post_msg (vcpu thread) tries to copy staged msg to msg slot,
> > +         * notify the guest, records the status, marks the posting done 
> > (BUSY
> > +         * -> POSTED), and schedules sint_msg_bh BH
> > +         */
> > +        HV_STAGED_MSG_POSTED,
> > +        /*
> > +         * sint_msg_bh (BH) verifies that the posting is done, runs the
> > +         * callback, and starts over (POSTED -> FREE)
> > +         */
> > +    } state;
> > +} HvSintStagedMesage;
> 
> s/Mesage/Message/

:facepalm:

> > +    if (atomic_read(&staged_msg->state) != HV_STAGED_MSG_POSTED) {
> > +        /* status nor ready yet (spurious ack from guest?), ignore */
> > +        return;
> > +    }
> > +
> 
> Can this actually happen?  It seems scary...

I don't see off-hand what can stop the guest from wrmsr HV_X64_MSR_EOM
at an arbitrary moment, which AFAICS will result in this eventfd being
signaled.  I haven't seen this codepath being taken in real life but
I think it provides enough protection for the internal state from being
screwed up by such an unexpected signal.

Thanks,
Roman.