diff mbox series

[RFC,12/34] hyperv: add synic event flag signaling

Message ID 20180206203048.11096-13-rkagan@virtuozzo.com
State New
Headers show
Series Hyper-V / VMBus | expand

Commit Message

Roman Kagan Feb. 6, 2018, 8:30 p.m. UTC
Add infrastructure to signal SynIC event flags by atomically setting the
corresponding bit in the event flags page and firing a SINT if
necessary.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 target/i386/hyperv.h |  2 ++
 target/i386/hyperv.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

Comments

Paolo Bonzini Feb. 7, 2018, 10:58 a.m. UTC | #1
On 06/02/2018 21:30, Roman Kagan wrote:
> +/*
> + * Set given event flag for a given sint on a given vcpu, and signal the sint.
> + */
> +int hyperv_set_evt_flag(HvSintRoute *sint_route, unsigned evtno)

Any reason (e.g. something from the spec) not to spell "event" and
"eventno" in full?

Thanks,

Paolo


> +{
> +    int ret;
> +    SynICState *synic = sint_route->synic;
> +    unsigned long *flags, set_mask;
> +    unsigned set_idx;
> +
> +    if (evtno > HV_EVENT_FLAGS_COUNT) {
> +        return -EINVAL;
> +    }
> +    if (!synic->enabled || !synic->evt_page_addr) {
Roman Kagan Feb. 7, 2018, 7:11 p.m. UTC | #2
On Wed, Feb 07, 2018 at 11:58:58AM +0100, Paolo Bonzini wrote:
> On 06/02/2018 21:30, Roman Kagan wrote:
> > +/*
> > + * Set given event flag for a given sint on a given vcpu, and signal the sint.
> > + */
> > +int hyperv_set_evt_flag(HvSintRoute *sint_route, unsigned evtno)
> 
> Any reason (e.g. something from the spec) not to spell "event" and
> "eventno" in full?

None beyond me being on the "shortest-comprehensible-name" camp :)

I thought "evt" was unambiguously parsed as "event", and "msg" as
"message".  Isn't it?

Thanks,
Roman.
Paolo Bonzini Feb. 8, 2018, 3:02 p.m. UTC | #3
On 07/02/2018 20:11, Roman Kagan wrote:
>>> +/*
>>> + * Set given event flag for a given sint on a given vcpu, and signal the sint.
>>> + */
>>> +int hyperv_set_evt_flag(HvSintRoute *sint_route, unsigned evtno)
>> Any reason (e.g. something from the spec) not to spell "event" and
>> "eventno" in full?
> None beyond me being on the "shortest-comprehensible-name" camp :)
> 
> I thought "evt" was unambiguously parsed as "event", and "msg" as
> "message".  Isn't it?

Yes, it's understandable, however my guess would have been that "msg" is
way more common than "evt".  grep seems to agree

  $ git grep '\Bmessage\|message\B' | wc -l
  451
  $ git grep '\Bmsg\|msg\B' | wc -l
  2504

  $ git grep '\Bevent\|event\B' | wc -l
  4998
  $ git grep '\Bevt\|evt\B' | wc -l
  483

where quite a few "evt" instances are matching "devtree" or "devtype".

("\Bfoo|foo\B" avoids the cases where "foo" is an entire word, which
would probably favor "message" and "event" when they appear as English
text).

Paolo
diff mbox series

Patch

diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index df17d9c3b7..3d942e5524 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -41,4 +41,6 @@  bool hyperv_synic_usable(void);
 
 int hyperv_post_msg(HvSintRoute *sint_route, struct hyperv_message *msg);
 
+int hyperv_set_evt_flag(HvSintRoute *sint_route, unsigned evtno);
+
 #endif
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index 918ba26849..b557cd5d5d 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -18,6 +18,7 @@ 
 #include "hw/qdev-properties.h"
 #include "exec/address-spaces.h"
 #include "sysemu/cpus.h"
+#include "qemu/bitops.h"
 #include "migration/vmstate.h"
 #include "hyperv.h"
 #include "hyperv-proto.h"
@@ -209,6 +210,37 @@  int hyperv_post_msg(HvSintRoute *sint_route, struct hyperv_message *src_msg)
     return 0;
 }
 
+/*
+ * Set given event flag for a given sint on a given vcpu, and signal the sint.
+ */
+int hyperv_set_evt_flag(HvSintRoute *sint_route, unsigned evtno)
+{
+    int ret;
+    SynICState *synic = sint_route->synic;
+    unsigned long *flags, set_mask;
+    unsigned set_idx;
+
+    if (evtno > HV_EVENT_FLAGS_COUNT) {
+        return -EINVAL;
+    }
+    if (!synic->enabled || !synic->evt_page_addr) {
+        return -ENXIO;
+    }
+
+    set_idx = BIT_WORD(evtno);
+    set_mask = BIT_MASK(evtno);
+    flags = synic->evt_page->slot[sint_route->sint].flags;
+
+    if ((atomic_fetch_or(&flags[set_idx], set_mask) & set_mask) != set_mask) {
+        memory_region_set_dirty(&synic->evt_page_mr, 0,
+                                sizeof(*synic->evt_page));
+        ret = kvm_hv_sint_route_set_sint(sint_route);
+    } else {
+        ret = 0;
+    }
+    return ret;
+}
+
 static void async_synic_update(CPUState *cs, run_on_cpu_data data)
 {
     SynICState *synic = data.host_ptr;