Re: [RFC PATCH post-8.1] hw/xen: Clean up event channel 'type

qemu-devel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH post-8.1] hw/xen: Clean up event channel 'type_val' handl

From:	Paul Durrant
Subject:	Re: [RFC PATCH post-8.1] hw/xen: Clean up event channel 'type_val' handling to use union
Date:	Mon, 14 Aug 2023 13:31:59 +0100
User-agent:	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0

On 03/08/2023 16:28, David Woodhouse wrote:

From: David Woodhouse <dwmw@amazon.co.uk>

A previous implementation of this stuff used a 64-bit field for all of
the port information (vcpu/type/type_val) and did atomic exchanges on
them. When I implemented that in Qemu I regretted my life choices and
just kept it simple with locking instead.

So there's no need for the XenEvtchnPort to be so simplistic. We can
use a union for the pirq/virq/interdomain information, which lets us
keep a separate bit for the 'remote domain' in interdomain ports. A
single bit is enough since the only possible targets are loopback or
qemu itself.

So now we can ditch PORT_INFO_TYPEVAL_REMOTE_QEMU and the horrid
manual masking, although the in-memory representation is identical
so there's no change in the saved state ABI.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
Thought this would be a nice cleanup to avoid abusing `type_val` for
various different purposes, and especially the top bit of it for
interdomain ports. But having done it I find myself fairly ambivalent
about it. Does anyone feel strongly either way?

  hw/i386/kvm/xen_evtchn.c | 124 ++++++++++++++++++++-------------------
  1 file changed, 64 insertions(+), 60 deletions(-)

I don't feel that strongly, but using the union+bitfield approach is alittle nicer to read and only makes the code 4 lines longer.

diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index a731738411..446ae46022 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -58,7 +58,15 @@ OBJECT_DECLARE_SIMPLE_TYPE(XenEvtchnState, XEN_EVTCHN)
  typedef struct XenEvtchnPort {
      uint32_t vcpu;      /* Xen/ACPI vcpu_id */
      uint16_t type;      /* EVTCHNSTAT_xxxx */
-    uint16_t type_val;  /* pirq# / virq# / remote port according to type */
+    union {
+        uint16_t type_val;  /* pirq# / virq# / remote port according to type */

Not sure the comment is that valuable any more... and maybe just 'val'now rather than 'type_val'?

+        uint16_t pirq;
+        uint16_t virq;
+        struct {
+            uint16_t port:15;
+            uint16_t to_qemu:1; /* Only two targets; qemu or loopback */

I'd have switch the sense and called this 'loopback'... since it's themore unlikely case.

+        } interdomain;
+    } u;
  } XenEvtchnPort;

/* 32-bit compatibility definitions, also used natively in 32-bit build */

@@ -210,16 +218,16 @@ static int xen_evtchn_post_load(void *opaque, int 
version_id)
          XenEvtchnPort *p = &s->port_table[i];

if (p->type == EVTCHNSTAT_pirq) {

-            assert(p->type_val);
-            assert(p->type_val < s->nr_pirqs);
+            assert(p->u.pirq);
+            assert(p->u.pirq < s->nr_pirqs);

/*

               * Set the gsi to IRQ_UNBOUND; it may be changed to an actual
               * GSI# below, or to IRQ_MSI_EMU when the MSI table snooping
               * catches up with it.
               */
-            s->pirq[p->type_val].gsi = IRQ_UNBOUND;
-            s->pirq[p->type_val].port = i;
+            s->pirq[p->u.pirq].gsi = IRQ_UNBOUND;
+            s->pirq[p->u.pirq].port = i;
          }
      }
      /* Rebuild s->pirq[].gsi mapping */
@@ -243,7 +251,7 @@ static const VMStateDescription xen_evtchn_port_vmstate = {
      .fields = (VMStateField[]) {
          VMSTATE_UINT32(vcpu, XenEvtchnPort),
          VMSTATE_UINT16(type, XenEvtchnPort),
-        VMSTATE_UINT16(type_val, XenEvtchnPort),
+        VMSTATE_UINT16(u.type_val, XenEvtchnPort),
          VMSTATE_END_OF_LIST()
      }
  };
@@ -599,14 +607,13 @@ static void unbind_backend_ports(XenEvtchnState *s)

for (i = 1; i < s->nr_ports; i++) {

          p = &s->port_table[i];
-        if (p->type == EVTCHNSTAT_interdomain &&
-            (p->type_val & PORT_INFO_TYPEVAL_REMOTE_QEMU)) {
-            evtchn_port_t be_port = p->type_val & 
PORT_INFO_TYPEVAL_REMOTE_PORT_MASK;
+        if (p->type == EVTCHNSTAT_interdomain && p->u.interdomain.to_qemu) {
+            evtchn_port_t be_port = p->u.interdomain.port;

if (s->be_handles[be_port]) {

                  /* This part will be overwritten on the load anyway. */
                  p->type = EVTCHNSTAT_unbound;
-                p->type_val = PORT_INFO_TYPEVAL_REMOTE_QEMU;
+                p->u.interdomain.port = 0;

/* Leave the backend port open and unbound too. */

                  if (kvm_xen_has_cap(EVTCHN_SEND)) {
@@ -644,7 +651,7 @@ int xen_evtchn_status_op(struct evtchn_status *status)

switch (p->type) {

      case EVTCHNSTAT_unbound:
-        if (p->type_val & PORT_INFO_TYPEVAL_REMOTE_QEMU) {
+        if (p->u.interdomain.to_qemu) {
              status->u.unbound.dom = DOMID_QEMU;
          } else {
              status->u.unbound.dom = xen_domid;
@@ -652,22 +659,21 @@ int xen_evtchn_status_op(struct evtchn_status *status)
          break;

case EVTCHNSTAT_interdomain:

-        if (p->type_val & PORT_INFO_TYPEVAL_REMOTE_QEMU) {
+        if (p->u.interdomain.to_qemu) {
              status->u.interdomain.dom = DOMID_QEMU;
          } else {
              status->u.interdomain.dom = xen_domid;
          }


Possibly neater as a ternary now you're switching on a simple boolean.

- status->u.interdomain.port = p->type_val &

-            PORT_INFO_TYPEVAL_REMOTE_PORT_MASK;
+        status->u.interdomain.port = p->u.interdomain.port;
          break;

case EVTCHNSTAT_pirq:

-        status->u.pirq = p->type_val;
+        status->u.pirq = p->u.pirq;
          break;

case EVTCHNSTAT_virq:

-        status->u.virq = p->type_val;
+        status->u.virq = p->u.virq;
          break;
      }

@@ -983,7 +989,7 @@ static int clear_port_pending(XenEvtchnState *s, evtchn_port_t port)

  static void free_port(XenEvtchnState *s, evtchn_port_t port)
  {
      s->port_table[port].type = EVTCHNSTAT_closed;
-    s->port_table[port].type_val = 0;
+    s->port_table[port].u.type_val = 0;
      s->port_table[port].vcpu = 0;

if (s->nr_ports == port + 1) {

@@ -1006,7 +1012,7 @@ static int allocate_port(XenEvtchnState *s, uint32_t 
vcpu, uint16_t type,
          if (s->port_table[p].type == EVTCHNSTAT_closed) {
              s->port_table[p].vcpu = vcpu;
              s->port_table[p].type = type;
-            s->port_table[p].type_val = val;
+            s->port_table[p].u.type_val = val;

*port = p;@@ -1047,15 +1053,15 @@ static int close_port(XenEvtchnState *s, evtchn_port_t port,

          return -ENOENT;

case EVTCHNSTAT_pirq:

-        s->pirq[p->type_val].port = 0;
-        if (s->pirq[p->type_val].is_translated) {
+        s->pirq[p->u.pirq].port = 0;
+        if (s->pirq[p->u.pirq].is_translated) {
              *flush_kvm_routes = true;
          }
          break;

case EVTCHNSTAT_virq:

-        kvm_xen_set_vcpu_virq(virq_is_global(p->type_val) ? 0 : p->vcpu,
-                              p->type_val, 0);
+        kvm_xen_set_vcpu_virq(virq_is_global(p->u.virq) ? 0 : p->vcpu,
+                              p->u.virq, 0);
          break;

case EVTCHNSTAT_ipi:

@@ -1065,8 +1071,8 @@ static int close_port(XenEvtchnState *s, evtchn_port_t 
port,
          break;

case EVTCHNSTAT_interdomain:

-        if (p->type_val & PORT_INFO_TYPEVAL_REMOTE_QEMU) {
-            uint16_t be_port = p->type_val & ~PORT_INFO_TYPEVAL_REMOTE_QEMU;
+        if (p->u.interdomain.to_qemu) {
+            uint16_t be_port = p->u.interdomain.port;
              struct xenevtchn_handle *xc = s->be_handles[be_port];
              if (xc) {
                  if (kvm_xen_has_cap(EVTCHN_SEND)) {
@@ -1076,14 +1082,15 @@ static int close_port(XenEvtchnState *s, evtchn_port_t 
port,
              }
          } else {
              /* Loopback interdomain */
-            XenEvtchnPort *rp = &s->port_table[p->type_val];
-            if (!valid_port(p->type_val) || rp->type_val != port ||
+            XenEvtchnPort *rp = &s->port_table[p->u.interdomain.port];
+            if (!valid_port(p->u.interdomain.port) ||
+                rp->u.interdomain.port != port ||
                  rp->type != EVTCHNSTAT_interdomain) {
                  error_report("Inconsistent state for interdomain unbind");
              } else {
                  /* Set the other end back to unbound */
                  rp->type = EVTCHNSTAT_unbound;
-                rp->type_val = 0;
+                rp->u.interdomain.port = 0;
              }
          }
          break;
@@ -1207,7 +1214,7 @@ int xen_evtchn_bind_vcpu_op(struct evtchn_bind_vcpu *vcpu)
      if (p->type == EVTCHNSTAT_interdomain ||
          p->type == EVTCHNSTAT_unbound ||
          p->type == EVTCHNSTAT_pirq ||
-        (p->type == EVTCHNSTAT_virq && virq_is_global(p->type_val))) {
+        (p->type == EVTCHNSTAT_virq && virq_is_global(p->u.virq))) {
          /*
           * unmask_port() with do_unmask==false will just raise the event
           * on the new vCPU if the port was already pending.
@@ -1352,19 +1359,15 @@ int xen_evtchn_bind_ipi_op(struct evtchn_bind_ipi *ipi)
  int xen_evtchn_bind_interdomain_op(struct evtchn_bind_interdomain 
*interdomain)
  {
      XenEvtchnState *s = xen_evtchn_singleton;
-    uint16_t type_val;
      int ret;

if (!s) {

          return -ENOTSUP;
      }

- if (interdomain->remote_dom == DOMID_QEMU) {

-        type_val = PORT_INFO_TYPEVAL_REMOTE_QEMU;
-    } else if (interdomain->remote_dom == DOMID_SELF ||
-               interdomain->remote_dom == xen_domid) {
-        type_val = 0;
-    } else {
+    if (interdomain->remote_dom != DOMID_QEMU &&
+        interdomain->remote_dom != DOMID_SELF &&
+        interdomain->remote_dom != xen_domid) {
          return -ESRCH;
      }

@@ -1375,8 +1378,8 @@ int xen_evtchn_bind_interdomain_op(struct evtchn_bind_interdomain *interdomain)

      qemu_mutex_lock(&s->port_lock);

/* The newly allocated port starts out as unbound */

-    ret = allocate_port(s, 0, EVTCHNSTAT_unbound, type_val,
-                        &interdomain->local_port);
+    ret = allocate_port(s, 0, EVTCHNSTAT_unbound, 0, &interdomain->local_port);
+
      if (ret) {
          goto out;
      }
@@ -1401,7 +1404,8 @@ int xen_evtchn_bind_interdomain_op(struct 
evtchn_bind_interdomain *interdomain)
              assign_kernel_eventfd(lp->type, xc->guest_port, xc->fd);
          }
          lp->type = EVTCHNSTAT_interdomain;
-        lp->type_val = PORT_INFO_TYPEVAL_REMOTE_QEMU | 
interdomain->remote_port;
+        lp->u.interdomain.to_qemu = 1;
+        lp->u.interdomain.port = interdomain->remote_port;
          ret = 0;
      } else {
          /* Loopback */
@@ -1415,13 +1419,13 @@ int xen_evtchn_bind_interdomain_op(struct 
evtchn_bind_interdomain *interdomain)
           * the port that was just allocated for the local end.
           */
          if (interdomain->local_port != interdomain->remote_port &&
-            rp->type == EVTCHNSTAT_unbound && rp->type_val == 0) {
+            rp->type == EVTCHNSTAT_unbound && !rp->u.interdomain.to_qemu) {

rp->type = EVTCHNSTAT_interdomain;

-            rp->type_val = interdomain->local_port;
+            rp->u.interdomain.port = interdomain->local_port;

lp->type = EVTCHNSTAT_interdomain;

-            lp->type_val = interdomain->remote_port;
+            lp->u.interdomain.port = interdomain->remote_port;
          } else {
              ret = -EINVAL;
          }
@@ -1440,7 +1444,6 @@ int xen_evtchn_bind_interdomain_op(struct 
evtchn_bind_interdomain *interdomain)
  int xen_evtchn_alloc_unbound_op(struct evtchn_alloc_unbound *alloc)
  {
      XenEvtchnState *s = xen_evtchn_singleton;
-    uint16_t type_val;
      int ret;

if (!s) {

@@ -1451,18 +1454,19 @@ int xen_evtchn_alloc_unbound_op(struct 
evtchn_alloc_unbound *alloc)
          return -ESRCH;
      }

- if (alloc->remote_dom == DOMID_QEMU) {

-        type_val = PORT_INFO_TYPEVAL_REMOTE_QEMU;
-    } else if (alloc->remote_dom == DOMID_SELF ||
-               alloc->remote_dom == xen_domid) {
-        type_val = 0;
-    } else {
+    if (alloc->remote_dom != DOMID_QEMU && alloc->remote_dom != DOMID_SELF &&
+        alloc->remote_dom != xen_domid) {

Maybe vertically align the clauses here as in the 'if' a couple of hunksback?

          return -EPERM;
      }


Since all the above are nits...

Reviewed-by: Paul Durrant <paul@xen.org>

[Prev in Thread]

Current Thread

[Next in Thread]

[RFC PATCH post-8.1] hw/xen: Clean up event channel 'type_val' handling to use union, David Woodhouse, 2023/08/03
- Re: [RFC PATCH post-8.1] hw/xen: Clean up event channel 'type_val' handling to use union, Paul Durrant <=
  - Re: [RFC PATCH post-8.1] hw/xen: Clean up event channel 'type_val' handling to use union, David Woodhouse, 2023/08/23

Prev by Date: Re: [PATCH 2/2] vhost: Add Error parameter to vhost_scsi_common_start()
Next by Date: Re: [PATCH] i386/xen: Don't advertise XENFEAT_supervisor_mode_kernel
Previous by thread: [RFC PATCH post-8.1] hw/xen: Clean up event channel 'type_val' handling to use union
Next by thread: Re: [RFC PATCH post-8.1] hw/xen: Clean up event channel 'type_val' handling to use union
Index(es):
- Date
- Thread