On 06/07/13 16:17, Stefan Hajnoczi wrote:
> Sorry for the poor Reply-To, I'm not subscribed to edk2-devel and
> reviewed these patches on gmane's web-based archive.
Thank you! I'm sure it took up quite a bit of your time. I'll answer
below.
>
>> + //
>> + // virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device
>> + //
>> + MemoryFence ();
>> + RxCurUsed = *Dev->RxRing.Used.Idx;
>> + TxCurUsed = *Dev->TxRing.Used.Idx;
>> +
>> + if (InterruptStatus != NULL) {
>> + //
>> + // report the receive interrupt if there is data available for
>> reception,
>> + // report the transmit interrupt if we have transmitted at least one
>> buffer
>> + //
>> + *InterruptStatus = 0;
>> + if (Dev->RxLastUsed != RxCurUsed) {
>> + *InterruptStatus |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>> + }
>> + if (Dev->TxLastUsed != TxCurUsed) {
>> + ASSERT (Dev->TxCurPending > 0);
>> + *InterruptStatus |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> + }
>> + }
>> +
>> + if (TxBuf != NULL) {
>> + if (Dev->TxLastUsed == TxCurUsed) {
>> + *TxBuf = NULL;
>> + }
>> + else {
>> + UINT16 UsedElemIdx;
>> + UINT32 DescIdx;
>> +
>> + //
>> + // fetch the first descriptor among those that the hypervisor reports
>> + // completed
>> + //
>> + ASSERT (Dev->TxCurPending > 0);
>> + ASSERT (Dev->TxCurPending <= Dev->TxMaxPending);
>> +
>> + UsedElemIdx = Dev->TxLastUsed++ % Dev->TxRing.QueueSize;
>> + DescIdx = Dev->TxRing.Used.UsedElem[UsedElemIdx].Id;
>
> There is no read memory barrier between fetching TxCurUsed and
> fetching UsedElem[].Id. In theory I think there is no guarantee that
> Dev->TxRing.Used.UsedElem[UsedElemIdx].Id is fetched *after*
> Dev->TxRing.Used.Idx. On x86 it shouldn't be a problem but I expected
> a read memory barrier after comparing fetching Dev->TxRing.Used.Idx.
>
guest host
----- ----
- MemoryFence ()
- read next used elem speculatively,
out of order
- publish new used elem
- MemoryFence ()
- bump used index
- MemoryFence ()
- read updated used index,
- cur > last
- use stale DescIdx
In theory you're right. However :)
(1) What you said about x86 and speculative execution. edk2 targets x86,
x64, and Itanium; OVMF targets the first two.
(2) MemoryFence() in edk2 isn't actually a memory fence:
/**
Used to serialize load and store operations.
All loads and stores that proceed calls to this function are guaranteed
to be
globally visible when this function returns.
**/
VOID
EFIAPI
MemoryFence (
VOID
)
{
// This is a little bit of overkill and it is more about the compiler
that it is
// actually processor synchronization. This is like the _ReadWriteBarrier
// Microsoft specific intrinsic
__asm__ __volatile__ ("":::"memory");
}
(3) Under "2.4.2 Receiving Used Buffers From The Device" in
virtio-0.9.5,
(3a) there's a typo in the example code I believe (actually, two
instances). It goes like (manual transcript):
vring_disable_interrupts(vq);
for (;;) {
if (vq->last_seen_used != vring->used.idx) {
vring_enable_interrupts(vq);
mb();
if (vq->last_seen_used != vring->used.idx) {
break;
}
}
struct vring_used_elem *e = vring.used->ring[vq->last_seen_used %
vsz];
process_buffer(e);
vq->last_seen_used++;
}
The typo is, IMO,
vq->last_seen_used != vring->used.idx
as the "ring empty" condition. I think it should say the opposite:
vq->last_seen_used == vring->used.idx
(3b) The idea behind the loop is, as explained by the spec:
vring_disable_interrupts(vq);
for (;;) {
if (used_ring_empty(vq)) {
/* may have suppressed an interrupt here */
vring_enable_interrupts(vq);
mb();
if (used_ring_empty()) {
/* nothing to do for now, exit until next
* (possibly already pending?) interrupt,
* at which point we'll come here again
*/
break;
}
}
struct vring_used_elem *e = vring.used->ring[vq->last_seen_used %
vsz];
process_buffer(e);
vq->last_seen_used++;
}
In OVMF virtio we don't have interrupts, just polling clients unable
to go to sleep for indefinite time, so the above decays to:
for (;;) {
if (used_ring_empty(vq)) { /* (i) */
mb();
if (used_ring_empty()) { /* (ii) */
break;
}
}
struct vring_used_elem *e = vring.used->ring[vq->last_seen_used % vsz];
process_buffer(e);
vq->last_seen_used++;
}
If (i) evaluates to false (= used ring not empty at first check), then
the above code doesn't necessarily execute any barrier either, before
retrieving the used elem.
If the extra barrier proves necessary for attaining purity (*), I'd like
to address this in a followup series:
- I might have to modify VirtioLib too, used by VirtioBlkDxe and
VirtioScsiDxe,
- VirtioNetDxe has already spent more than a month pending on
edk2-devel, and I'm close to abandoning it. If possible I wouldn't
like to respin the series for theoretical purity (in particular given
that the "fix", an extra MemoryFence() call, is no memory barrier at
all, just documentation basically.)
Wrt. (*),
extra barrier proves necessary for attaining purity
translates to
Rusty accepts a patch, both fixing the typos and adding an extra
mb()
I'm willing to send a patch for the spec, but only after VirtioNetDxe
has been merged (ie. I'm willing to send a patch for the spec only as a
pre-requisite for any followup series addressing this, see above). This
is how it looks in my head (*unless* a more serious issue is found, of
course):
- Jordan merges the series,
- Laszlo sends a patch to Rusty for the spec,
- if Rusty rejects the patch, we're done,
- if Rusty accepts the patch, Laszlo sends a small series for VirtioLib
and VirtioNetDxe.
Thanks,
Laszlo
------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. A cloud service to automate IT design, transition and operations
2. Dashboards that offer high-level views of enterprise services
3. A single system of record for all IT processes
http://p.sf.net/sfu/servicenow-d2d-j
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
