Skip to content

Doorbell Buffer fences are inappropriately lax #1025

@iximeow

Description

@iximeow

std::sync::atomic::fence talks about the general pattern we have in queue.rs where we fence(Release); write() and read(); fence(Acquire). but we're not actually doing either of

* followed by an atomic write ‘X’ with any ordering on some atomic object ‘m’,
...
* an atomic read ‘Y’ with any ordering on ‘m’,

since the mem.write and mem.read are through a *u32 and normal non-atomic pointer write/read. and for the same reasons I'd described in #1005, it would be inappropriate to convert these to a &AtomicU32 and do reads/writes anyway. more "UB by the spec, probably fine in practice": from https://doc.rust-lang.org/std/sync/atomic/#memory-model-for-atomic-accesses, a guest could do a conflicting write or access the atomic with any other size concurrent to our operation in Propolis.

and worse, the guest OS might not care about C++ atomic semantics anyway; even if everything were on the up and up about atomic accesses, we don't know the guest has fulfilled its end of a fence(Release)-like operation, so we don't really know that we've established a happens-before relationship with guest memory.

I'm not yet sure exactly what kind of fence (mfence? or nothing?) we do need or where we should do that fencing, just that what we're doing now is definitely not what we'd want.. just making these mfence seems simple, but makes me wonder what the expected fencing is between a shadow doorbell read and, say, reading a submission queue entry from the indicated queue?

Metadata

Metadata

Assignees

Labels

bugSomething that isn't working.

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions