From: Paul Durrant <xadimgnik@gmail.com>
To: David Woodhouse <dwmw2@infradead.org>,
qemu-devel <qemu-devel@nongnu.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
Richard Henderson <richard.henderson@linaro.org>,
Eduardo Habkost <eduardo@habkost.net>,
"Michael S. Tsirkin" <mst@redhat.com>,
Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
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 [thread overview]
Message-ID: <23529a16-3a7e-287a-4da8-eebf53d5a95d@xen.org> (raw)
In-Reply-To: <2d5e23d21fdbd148d9b0d9e4c00145217c4ddd17.camel@infradead.org>
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 a
little 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 the
more 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 hunks
back?
> return -EPERM;
> }
>
Since all the above are nits...
Reviewed-by: Paul Durrant <paul@xen.org>
next prev parent reply other threads:[~2023-08-14 12:33 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-03 15:28 [RFC PATCH post-8.1] hw/xen: Clean up event channel 'type_val' handling to use union David Woodhouse
2023-08-14 12:31 ` Paul Durrant [this message]
2023-08-23 12:09 ` David Woodhouse
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=23529a16-3a7e-287a-4da8-eebf53d5a95d@xen.org \
--to=xadimgnik@gmail.com \
--cc=dwmw2@infradead.org \
--cc=eduardo@habkost.net \
--cc=marcel.apfelbaum@gmail.com \
--cc=mst@redhat.com \
--cc=paul@xen.org \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).