xen_rmb, xen_wmb

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

In sys/arch/xen/include/hypervisor.h rev. 1.48 and
sys/arch/xen/include/xenring.h rev. 1.2, the macros xen_rmb and
xen_wmb were defined as follows:

#define xen_rmb() membar_producer()
#define xen_wmb() membar_consumer()

This definition doesn't make much sense on its face, because
membar_producer orders writes (and may have no effect on reads), while
membar_consumer orders reads (and may have no effect on writes).

On x86, both functions -- membar_producer and membar_consumer -- are
no-ops, though -- because, on normal memory (wb), x86 never reorders
loads with loads or stores with stores (nor in wp, wt, or uc memory),
and a mere compiler instruction barrier suffices.

So by accident, on x86, these functions are interchangeable and the
definitions aren't exactly wrong -- unless Xen uses these for
operations on wc or wc+ memory, which would be odd for guest software
on a CPU coordinating with hypervisor software on a CPU and no real
I/O devices involved.

But it sure is confusing!  And it would be wrong if NetBSD/xen were
ever extended to non-x86 architectures like aarch64.

The usage is also inconsistent.  In xencons.c, the intent in this
logic is obviously that xen_rmb be a load-before-load barrier (and
perhaps better load-before-load/store, membar_acquire), and xen_wmb be
a store-before-store barrier (actually it needs to be
load/store-before-store, or membar_release):

                cons = xencons_interface->out_cons;
                prod = xencons_interface->out_prod;
                xen_rmb();
                while (prod != cons + sizeof(xencons_interface->out)) {
			...
                }
                xen_wmb();
                xencons_interface->out_prod = prod;
                xen_wmb();

But in xengnt.c, the intent in this logic is obviously that xen_rmb be
a store-before-store barrier:

                grant_table.gntt_v2[*entryp].full_page.frame = ma >> PAGE_SHIFT;
                grant_table.gntt_v2[*entryp].hdr.domid = dom;
                /*
                 * ensure that the above values reach global visibility 
                 * before permitting frame's access (done when we set flags)
                 */
                xen_rmb();
                grant_table.gntt_v2[*entryp].hdr.flags =
                    GTF_permit_access | (ro ? GTF_readonly : 0);

I suggest that:

1. xen_rmb be redefined as membar_acquire (load-before-load/store),
2. xen_wmb be redefined as membar_release (load/store-before-store),
and
3. all uses be audited to confirm they have the right semantics;

or, alternatively, that the macros be eliminated and the uses be
replaced by the appropriate membar_*.


sys/arch/xen/include/hypervisor.h rev. 1.48:
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/arch/xen/include/hypervisor.h?rev=1.48&content-type=text/x-cvsweb-markup&only_with_tag=MAIN
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/arch/xen/include/hypervisor.h.diff?r1=1.47&r2=1.48&only_with_tag=MAIN

sys/arch/xen/include/xenring.h rev. 1.2:
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/arch/xen/include/xenring.h?only_with_tag=MAIN
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/arch/xen/include/xenring.h.diff?r1=1.1&r2=1.2&only_with_tag=MAIN