|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 17/18] OvmfPkg/XenPvBlkDxe: Add BlockFront client.
> > > + OUT XenbusState *LastStatePtr OPTIONAL
> > > + )
> > > +{
> > > + XENBUS_PROTOCOL *XenbusIo = Dev->XenbusIo;
> > > + XenbusState State;
> > > + UINT64 Value;
> > > + XENSTORE_STATUS Status;
> > > +
> > > + while (TRUE) {
> > > + Status = XenbusReadInteger (XenbusIo, "state", TRUE, &Value);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + return Status;
> > > + }
> > > + if (Value > XenbusStateReconfigured) {
> > > + return XENSTORE_STATUS_EIO;
> > > + }
> > > + State = Value;
> > > + if (State >= WaitedState) {
> >
> > Not State != WaitedState ?
>
> I don't think so, the function is actually waiting for the State to be
> equal to WaitedState. At least, that how I wrote it. There might be a
> better algorithm.
Then State == WaitedState
seems the proper way?
>
> > > + break;
> > > + }
> > > + DEBUG ((EFI_D_INFO,
> > > + "XenPvBlk: waiting backend state %d, current: %d\n",
> > > + WaitedState, State));
> > > + XenbusIo->WaitForWatch (XenbusIo, Dev->StateWatchToken);
> > > + }
> > > +
> > > + if (LastStatePtr != NULL) {
> > > + *LastStatePtr = State;
> > > + }
> > > +
> > > + return XENSTORE_STATUS_SUCCESS;
> > > +}
> > > +
> > > +EFI_STATUS
> > > +XenPvBlockFrontInitialization (
> > > + IN XENBUS_PROTOCOL *XenbusIo,
> > > + IN CONST CHAR8 *NodeName,
> > > + OUT XEN_BLOCK_FRONT_DEVICE **DevPtr
> > > + )
> > > +{
> > > + XENSTORE_TRANSACTION xbt;
> > > + CHAR8 *DeviceType;
> > > + blkif_sring_t *SharedRing;
> > > + XENSTORE_STATUS Status;
> > > + XEN_BLOCK_FRONT_DEVICE *Dev;
> > > + XenbusState State;
> > > + UINT64 Value;
> > > +
> > > + ASSERT (NodeName != NULL);
> > > +
> > > + Dev = AllocateZeroPool (sizeof (XEN_BLOCK_FRONT_DEVICE));
> > > + Dev->Signature = XEN_BLOCK_FRONT_SIGNATURE;
> > > + Dev->NodeName = NodeName;
> > > + Dev->XenbusIo = XenbusIo;
> > > + Dev->DeviceId = XenbusIo->DeviceId;
> > > +
> > > + XenbusIo->XsRead (XenbusIo, XST_NIL, "device-type",
> > > (VOID**)&DeviceType);
> > > + if (AsciiStrCmp (DeviceType, "cdrom") == 0) {
> > > + Dev->MediaInfo.CdRom = TRUE;
> > > + } else {
> > > + Dev->MediaInfo.CdRom = FALSE;
> > > + }
> > > + FreePool (DeviceType);
> > > +
> > > + Status = XenbusReadInteger (XenbusIo, "backend-id", FALSE, &Value);
> > > + if (Status != XENSTORE_STATUS_SUCCESS || Value > UINT16_MAX) {
> > > + DEBUG ((EFI_D_ERROR, "XenPvBlk: Failed to get backend-id (%d)\n",
> > > + Status));
> > > + goto Error;
> > > + }
> > > + Dev->DomainId = Value;
> > > + XenbusIo->EventChannelAllocate (XenbusIo, Dev->DomainId,
> > > &Dev->EventChannel);
> > > +
> > > + SharedRing = (blkif_sring_t*) AllocatePages (1);
> > > + SHARED_RING_INIT (SharedRing);
> > > + FRONT_RING_INIT (&Dev->Ring, SharedRing, EFI_PAGE_SIZE);
> > > + XenbusIo->GrantAccess (XenbusIo,
> > > + Dev->DomainId,
> > > + (INTN) SharedRing >> EFI_PAGE_SHIFT,
> > > + FALSE,
> > > + &Dev->RingRef);
> > > +
> > > +Again:
> > > + Status = XenbusIo->XsTransactionStart (XenbusIo, &xbt);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_WARN, "XenPvBlk: Failed to start transaction, %d\n",
> > > Status));
> >
> > .. and do you want to error out?
>
> The original code (mini-os) is continuing in thes case, so I thought it
> was possible to do the initialisation without transaction. But maybe I'm
> wrong and this should be an error.
No explanation in the commit for that?
>
> > > + }
> > > +
> > > + Status = XenbusIo->XsPrintf (XenbusIo, xbt, NodeName, "ring-ref", "%d",
> > > + Dev->RingRef);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_ERROR, "XenPvBlk: Failed to write ring-ref.\n"));
> > > + goto AbortTransaction;
> > > + }
> > > + Status = XenbusIo->XsPrintf (XenbusIo, xbt, NodeName,
> > > + "event-channel", "%d", Dev->EventChannel);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_ERROR, "XenPvBlk: Failed to write event-channel.\n"));
> > > + goto AbortTransaction;
> > > + }
> > > + Status = XenbusIo->XsPrintf (XenbusIo, xbt, NodeName,
> > > + "protocol", "%a",
> > > XEN_IO_PROTO_ABI_NATIVE);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_ERROR, "XenPvBlk: Failed to write protocol.\n"));
> > > + goto AbortTransaction;
> > > + }
> > > +
> > > + Status = XenbusIo->SetState (XenbusIo, xbt, XenbusStateConnected);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_ERROR, "XenPvBlk: Failed switch state.\n"));
> > > + goto AbortTransaction;
> > > + }
> > > +
> > > + Status = XenbusIo->XsTransactionEnd (XenbusIo, xbt, FALSE);
> > > + if (Status == XENSTORE_STATUS_EAGAIN) {
> > > + goto Again;
> > > + }
> > > +
> > > + XenbusIo->RegisterWatchBackend (XenbusIo, "state",
> > > &Dev->StateWatchToken);
> > > +
> > > + // Waiting backend
> > > + Status = XenPvBlkWaitForBackendState (Dev, XenbusStateConnected,
> > > &State);
> > > + if (Status != XENSTORE_STATUS_SUCCESS || State !=
> > > XenbusStateConnected) {
> > > + DEBUG ((EFI_D_ERROR,
> > > + "XenPvBlk: backend for %a/%d not available, rc=%d
> > > state=%d\n",
> > > + XenbusIo->Type, XenbusIo->DeviceId, Status, State));
> > > + goto Error2;
> > > + }
> > > +
> > > + Status = XenbusReadInteger (XenbusIo, "info", TRUE, &Value);
> > > + if (Status != XENSTORE_STATUS_SUCCESS || Value > UINT32_MAX) {
> > > + goto Error2;
> > > + }
> > > + Dev->MediaInfo.VDiskInfo = Value;
> > > + if (Dev->MediaInfo.VDiskInfo & VDISK_READONLY) {
> > > + Dev->MediaInfo.ReadWrite = FALSE;
> > > + } else {
> > > + Dev->MediaInfo.ReadWrite = TRUE;
> > > + }
> > > +
> > > + Status = XenbusReadInteger (XenbusIo, "sectors", TRUE,
> > > &Dev->MediaInfo.Sectors);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + goto Error2;
> > > + }
> > > +
> > > + Status = XenbusReadInteger (XenbusIo, "sector-size", TRUE, &Value);
> > > + if (Status != XENSTORE_STATUS_SUCCESS || Value > UINT32_MAX) {
> > > + goto Error2;
> > > + }
> > > + Dev->MediaInfo.SectorSize = Value;
> >
> > Why not &Dev->MediaInfo.SectorSize in XenbusReadInteger?
> >
> > Is it because of types? If so, please add a comment stating that.
>
> Yes, because of the type, uint64 vs uint32.
>
> Maybe I could rename the XenbusReadInteger to XenbusReadUint64, and it
> would be more obvious that the function require a specific type.
That would be much better. Thank you
>
> > > +
> > > + // Default value
> > > + Value = 0;
> > > + Status = XenbusReadInteger (XenbusIo, "feature-barrier", TRUE, &Value);
> > > + if (Value == 1) {
> > > + Dev->MediaInfo.FeatureBarrier = TRUE;
> > > + } else {
> > > + Dev->MediaInfo.FeatureBarrier = FALSE;
> > > + }
> > > +
> > > + // Default value
> > > + Value = 0;
> > > + XenbusReadInteger (XenbusIo, "feature-flush-cache", TRUE, &Value);
> > > + if (Value == 1) {
> > > + Dev->MediaInfo.FeatureFlushCache = TRUE;
> > > + } else {
> > > + Dev->MediaInfo.FeatureFlushCache = FALSE;
> > > + }
> > > +
> > > + DEBUG ((EFI_D_INFO, "XenPvBlk: New disk with %d sectors of %d bytes\n",
> > > + Dev->MediaInfo.Sectors, Dev->MediaInfo.SectorSize));
> > > +
> > > + *DevPtr = Dev;
> > > + return EFI_SUCCESS;
> > > +
> > > +Error2:
> > > + XenbusIo->UnregisterWatch (XenbusIo, Dev->StateWatchToken);
> > > + XenbusIo->XsRemove (XenbusIo, XST_NIL, "ring-ref");
> > > + XenbusIo->XsRemove (XenbusIo, XST_NIL, "event-channel");
> >
> > XenbusIo->XsRemove (XenbusIo, XST_NIL, "protocol");
> >
> > ?
>
> I don't know, but I can do that.
>
> > > + goto Error;
> > > +AbortTransaction:
> > > + XenbusIo->XsTransactionEnd (XenbusIo, xbt, TRUE);
> > > +Error:
> > > + XenPvBlockFree (Dev);
> > > + return EFI_DEVICE_ERROR;
> > > +}
> > > +
> > > +VOID
> > > +XenPvBlockFrontShutdown (
> > > + IN XEN_BLOCK_FRONT_DEVICE *Dev
> > > + )
> > > +{
> > > + XENBUS_PROTOCOL *XenbusIo = Dev->XenbusIo;
> > > + XENSTORE_STATUS Status;
> > > + UINT64 Value;
> > > +
> > > + XenPvBlockSync (Dev);
> > > +
> > > + Status = XenbusIo->SetState (XenbusIo, XST_NIL, XenbusStateClosing);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_ERROR,
> > > + "XenPvBlk: error while changing state to Closing: %d\n",
> > > + Status));
> > > + goto Close;
> > > + }
> > > +
> > > + Status = XenPvBlkWaitForBackendState (Dev, XenbusStateClosing, NULL);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_ERROR,
> > > + "XenPvBlk: error while waiting for closing backend state:
> > > %d\n",
> > > + Status));
> > > + goto Close;
> > > + }
> > > +
> > > + Status = XenbusIo->SetState (XenbusIo, XST_NIL, XenbusStateClosed);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_ERROR,
> > > + "XenPvBlk: error while changing state to Closed: %d\n",
> > > + Status));
> > > + goto Close;
> > > + }
> > > +
> > > + Status = XenPvBlkWaitForBackendState (Dev, XenbusStateClosed, NULL);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_ERROR,
> > > + "XenPvBlk: error while waiting for closed backend state:
> > > %d\n",
> > > + Status));
> > > + goto Close;
> > > + }
> > > +
> > > + Status = XenbusIo->SetState (XenbusIo, XST_NIL,
> > > XenbusStateInitialising);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_ERROR,
> > > + "XenPvBlk: error while changing state to initialising: %d\n",
> > > + Status));
> > > + goto Close;
> > > + }
> > > +
> > > + while (TRUE) {
> > > + Status = XenbusReadInteger (XenbusIo, "state", TRUE, &Value);
> > > + if (Status != XENSTORE_STATUS_SUCCESS) {
> > > + DEBUG ((EFI_D_ERROR,
> > > + "XenPvBlk: error while waiting for new backend state:
> > > %d\n",
> > > + Status));
> > > + goto Close;
> > > + }
> > > + if (Value < XenbusStateInitWait || Value >= XenbusStateClosed) {
> > > + break;
> > > + }
> > > + DEBUG ((EFI_D_INFO,
> > > + "XenPvBlk: waiting backend state %d, current: %d\n",
> > > + XenbusStateInitWait, Value));
> > > + XenbusIo->WaitForWatch (XenbusIo, Dev->StateWatchToken);
> > > + }
> > > +
> > > +Close:
> > > + XenbusIo->UnregisterWatch (XenbusIo, Dev->StateWatchToken);
> > > + XenbusIo->XsRemove (XenbusIo, XST_NIL, "ring-ref");
> > > + XenbusIo->XsRemove (XenbusIo, XST_NIL, "event-channel");
> >
> > "protocol"
As in, also do the XenbusIo->XsRemove(...)
> > > +
> > > + XenPvBlockFree (Dev);
> > > +}
> > > +
> > > +STATIC
> > > +VOID
> > > +XenPvBlockWaitSlot (
> > > + IN XEN_BLOCK_FRONT_DEVICE *Dev
> > > + )
> > > +{
> > > + /* Wait for a slot */
> > > + if (RING_FULL (&Dev->Ring)) {
> > > + while (TRUE) {
> > > + XenPvBlockAsyncIoPoll (Dev);
> > > + if (!RING_FULL(&Dev->Ring)) {
> > > + break;
> > > + }
> > > + /* Really no slot, go to sleep. */
> > > + // XXX wait for an event on Dev->EventChannel
> > > + DEBUG ((EFI_D_INFO, "%a %d, waiting\n", __func__, __LINE__));
> > > + }
> > > + }
> > > +}
> > > +
> > > +/* Issue an aio */
> > > +VOID
> > > +XenPvBlockAsyncIo (
> > > + IN OUT XEN_BLOCK_FRONT_IO *IoData,
> > > + IN BOOLEAN IsWrite
> > > + )
> > > +{
> > > + XEN_BLOCK_FRONT_DEVICE *Dev = IoData->Dev;
> > > + XENBUS_PROTOCOL *XenbusIo = Dev->XenbusIo;
> > > + blkif_request_t *Request;
> > > + RING_IDX RingIndex;
> > > + BOOLEAN Notify;
> > > + INT32 NumSegments, Index;
> > > + UINTN Start, End;
> > > +
> > > + // Can't io at non-sector-aligned location
> > > + ASSERT(!(IoData->Offset & (Dev->MediaInfo.SectorSize - 1)));
> > > + // Can't io non-sector-sized amounts
> > > + ASSERT(!(IoData->Size & (Dev->MediaInfo.SectorSize - 1)));
> > > + // Can't io non-sector-aligned buffer
> > > + ASSERT(!((UINTN) IoData->Buffer & (Dev->MediaInfo.SectorSize - 1)));
> > > +
> > > + Start = (UINTN) IoData->Buffer & ~EFI_PAGE_MASK;
> > > + End = ((UINTN) IoData->Buffer + IoData->Size + EFI_PAGE_SIZE - 1) &
> > > ~EFI_PAGE_MASK;
> > > + IoData->NumRef = NumSegments = (End - Start) / EFI_PAGE_SIZE;
> > > +
> > > + // XXX is this comment still true ?
> >
> > I think it is outdated, as Linux would do max DMA up to the segment
> > size that is advertised. Ditto for the SCSI stack.
>
> OK, thanks.
>
> > > + /* qemu's IDE max multsect is 16 (8KB) and SCSI max DMA was set to
> > > 32KB,
> > > + * so max 44KB can't happen */
> > > + ASSERT (NumSegments <= BLKIF_MAX_SEGMENTS_PER_REQUEST);
> >
> > But this check is OK as we can't truly fit more on the ring.
>
> OK.
>
> > > +
> > > + XenPvBlockWaitSlot (Dev);
> >
> > What happens if there are multiple threads calling this
> > function? Don't we need a mutex to only allow
> > one thread to grab an RingIndex?
>
> So, OVMF does not have threads but the are "events", called by a timer.
> This code does not support it, yet, so I don't need any form of locking,
> yet.
Ok, why don't you mention that in the commit description.
>
> > > + RingIndex = Dev->Ring.req_prod_pvt;
> > > + Request = RING_GET_REQUEST (&Dev->Ring, RingIndex);
> > > +
> > > + Request->operation = IsWrite ? BLKIF_OP_WRITE : BLKIF_OP_READ;
> > > + Request->nr_segments = NumSegments;
> > > + Request->handle = Dev->DeviceId;
> > > + Request->id = (UINTN) IoData;
> > > + Request->sector_number = IoData->Offset / 512;
> > > +
> > > + for (Index = 0; Index < NumSegments; Index++) {
> > > + Request->seg[Index].first_sect = 0;
> >
> > Uh?
> > > + Request->seg[Index].last_sect = EFI_PAGE_SIZE / 512 - 1;
> >
> > Uh? I thought you wanted to use the real sector values?
> > > + }
> > > + Request->seg[0].first_sect = ((UINTN) IoData->Buffer & EFI_PAGE_MASK)
> > > / 512;
> > > + Request->seg[NumSegments - 1].last_sect =
> > > + (((UINTN) IoData->Buffer + IoData->Size - 1) & EFI_PAGE_MASK) /
> > > 512;
> >
> > What about in between 0 and NumSegments? You should also
> > fill out those values with valid entries.
>
> It's done by the previous for loop. Maybe this part is hard to
> understand without the interface explain.
> include io/blkif.h says:
> @first_sect: first sector in frame to transfer (inclusive).
> @last_sect: last sector in frame to transfer (inclusive).
>
> but does not explain what a frame is, beyond this comment on:
> grant_ref_t gref; /* reference to I/O buffer frame */
> (all this from the struct blkif_request_segment)
> Or maybe frame == page.
That is what it means. If you have a page, with 4 sectors
of data (so half-page) and they are discontingious you
have to enumerate the sector values. For example:
First sector = lba + 0 -> lba + 1; #2 sector = lba + 5-> lba + 6;
#3 sector = lba + 55 -> lba + 56; #4 sector = lba + 32768 -> lba + 32769
>
> Anyway, the way the segments are initialized feel weirds now, but I
> don't think it's incorrect, otherwise, I would not be able to boot any
> guest.
Well, you could because you might be reading only one sector per
I/O request. And then the first_sect and last_sect get filled
with proper values.
>
> > > + for (Index = 0; Index < NumSegments; Index++) {
> > > + UINTN Data = Start + Index * EFI_PAGE_SIZE;
> > > + if (!IsWrite) {
> > > + /* Trigger CoW if needed */
> >
> > Not sure I understand that reference?
>
> Maybe a reference to the original commit will help to understand:
> minios: Fix bug when blkfront reading into zero-mapped buffer by just poking
> the page.
> No need to use virtual_to_mfn() for the ring since that is a real page.
> 7b1257946080b63421a8dc59c7b3dfd67997c643 (xen.git)
>
> But I still don't undertand this.
I think that means the MiniOS would hit a page fault and allocate
a page. I don't know if EFI drivers have this concept?
>
> > > + *(CHAR8 *)(Data + (Request->seg[Index].first_sect << 9)) = 0;
> > > + MemoryFence ();
If the EFI 'things' that use the BlockIO drivers allocate an page
for read, then there is no need for this.
> > > + }
> > > + XenbusIo->GrantAccess (XenbusIo, Dev->DomainId,
> > > + Data >> EFI_PAGE_SHIFT, IsWrite,
> > > + &Request->seg[Index].gref);
> > > + IoData->GrantRef[Index] = Request->seg[Index].gref;
> > > + }
> > > +
> > > + Dev->Ring.req_prod_pvt = RingIndex + 1;
> > > +
> > > + MemoryFence ();
> > > + RING_PUSH_REQUESTS_AND_CHECK_NOTIFY (&Dev->Ring, Notify);
> > > +
> > > + if (Notify) {
> > > + XenbusIo->EventChannelNotify (XenbusIo, Dev->EventChannel);
> > > + }
> > > +}
> > > +
> > > +STATIC
> > > +VOID
> > > +XenPvBlockAsyncIoCallback (
> > > + IN OUT XEN_BLOCK_FRONT_IO *IoData,
> > > + IN EFI_STATUS Status
> > > + )
> > > +{
> > > + IoData->Data = (VOID *) 1;
> > > + IoData->Callback = NULL;
> > > +}
> > > +
> > > +VOID
> > > +XenPvBlockIo (
> > > + IN OUT XEN_BLOCK_FRONT_IO *IoData,
> > > + IN BOOLEAN IsWrite
> > > + )
> > > +{
> > > + ASSERT (IoData->Callback == NULL);
> > > + // XXX Signal/Event instead ?
> >
> > Maybe for later versions. I think this is OK for right now, thought
> > it will mean we can only do one I/O .. and have then to wait
> > until it is completed.
> >
> > That limititation should be mentioned in the commit description.
>
> That right, right now, it's only one IO at a time. I think my comment is
> more to tell me that Callback is not really one, and it could be replace
> by something more Uefi, an event.
>
> > > + IoData->Callback = XenPvBlockAsyncIoCallback;
> > > + XenPvBlockAsyncIo (IoData, IsWrite);
> > > + IoData->Data = NULL;
> > > +
> > > + while (TRUE) {
> > > + XenPvBlockAsyncIoPoll (IoData->Dev);
> > > + if (IoData->Data) {
> > > + break;
> > > + }
> > > + }
> > > +}
> > > +
> > > +STATIC
> > > +VOID
> > > +XenPvBlockPushOperation (
> > > + IN XEN_BLOCK_FRONT_DEVICE *Dev,
> > > + IN UINT8 Operation,
> > > + IN UINT64 Id
> > > + )
> > > +{
> > > + INT32 Index;
> > > + blkif_request_t *Request;
> > > + BOOLEAN Notify;
> > > +
> > > + XenPvBlockWaitSlot (Dev);
> >
> > How do we make sure that there aren't multiple threads
> > calling this?
>
> Not possible yet.
>
> > > + Index = Dev->Ring.req_prod_pvt;
> > > + Request = RING_GET_REQUEST(&Dev->Ring, Index);
> > > + Request->operation = Operation;
> > > + Request->nr_segments = 0;
> > > + Request->handle = Dev->DeviceId;
> > > + Request->id = Id;
> > > + /* Not needed anyway, but the backend will check it */
> > > + Request->sector_number = 0;
> > > + Dev->Ring.req_prod_pvt = Index + 1;
> > > + MemoryFence ();
> > > + RING_PUSH_REQUESTS_AND_CHECK_NOTIFY (&Dev->Ring, Notify);
> > > + if (Notify) {
> > > + XENBUS_PROTOCOL *XenbusIo = Dev->XenbusIo;
> > > + XenbusIo->EventChannelNotify (XenbusIo, Dev->EventChannel);
> > > + }
> > > +}
> > > +
> > > +VOID
> > > +XenPvBlockSync (
> > > + IN XEN_BLOCK_FRONT_DEVICE *Dev
> > > + )
> > > +{
> > > + if (Dev->MediaInfo.ReadWrite) {
> > > + if (Dev->MediaInfo.FeatureBarrier) {
> > > + XenPvBlockPushOperation (Dev, BLKIF_OP_WRITE_BARRIER, 0);
> > > + }
> > > +
> > > + if (Dev->MediaInfo.FeatureFlushCache) {
> > > + XenPvBlockPushOperation (Dev, BLKIF_OP_FLUSH_DISKCACHE, 0);
> > > + }
> > > + }
> > > +
> > > + /* Note: This won't finish if another thread enqueues requests. */
> > > + while (TRUE) {
> > > + XenPvBlockAsyncIoPoll (Dev);
> > > + if (RING_FREE_REQUESTS (&Dev->Ring) == RING_SIZE (&Dev->Ring)) {
> > > + break;
> > > + }
> > > + }
> > > +}
> > > +
> > > +VOID
> > > +XenPvBlockAsyncIoPoll (
> > > + IN XEN_BLOCK_FRONT_DEVICE *Dev
> > > + )
> > > +{
> > > + RING_IDX ProducerIndex, ConsumerIndex;
> > > + blkif_response_t *Response;
> > > + INT32 More;
> > > +
> > > + do {
> > > + ProducerIndex = Dev->Ring.sring->rsp_prod;
> > > + /* Ensure we see queued responses up to 'ProducerIndex'. */
> > > + MemoryFence ();
> > > + ConsumerIndex = Dev->Ring.rsp_cons;
> > > +
> > > + while (ConsumerIndex != ProducerIndex) {
> > > + XEN_BLOCK_FRONT_IO *IoData;
> > > + INT16 Status;
> > > +
> > > + Response = RING_GET_RESPONSE (&Dev->Ring, ConsumerIndex);
> > > +
> > > + IoData = (VOID *) (UINTN) Response->id;
> > > + Status = Response->status;
> > > +
> > > + switch (Response->operation) {
> > > + case BLKIF_OP_READ:
> > > + case BLKIF_OP_WRITE:
> > > + {
> > > + INT32 Index;
> > > +
> > > + if (Status != BLKIF_RSP_OKAY) {
> > > + DEBUG ((EFI_D_ERROR,
> > > + "XenPvBlk: "
> > > + "%a error %d on %a at offset %Ld, num bytes %Ld\n",
> > > + Response->operation == BLKIF_OP_READ ? "read" :
> > > "write",
> > > + Status, IoData->Dev->NodeName,
> > > + IoData->Offset,
> > > + IoData->Size));
> > > + }
> > > +
> > > + for (Index = 0; Index < IoData->NumRef; Index++) {
> > > + Dev->XenbusIo->GrantEndAccess (Dev->XenbusIo,
> > > IoData->GrantRef[Index]);
> > > + }
> > > +
> > > + break;
> > > + }
> > > +
> > > + case BLKIF_OP_WRITE_BARRIER:
> > > + if (Status != BLKIF_RSP_OKAY) {
> > > + DEBUG ((EFI_D_ERROR, "XenPvBlk: write barrier error %d\n",
> > > Status));
> > > + }
> > > + break;
> > > + case BLKIF_OP_FLUSH_DISKCACHE:
> > > + if (Status != BLKIF_RSP_OKAY) {
> > > + DEBUG ((EFI_D_ERROR, "XenPvBlk: flush error %d\n", Status));
> > > + }
> > > + break;
> > > +
> > > + default:
> > > + DEBUG ((EFI_D_ERROR,
> > > + "XenPvBlk: unrecognized block operation %d response
> > > (status %d)\n",
> > > + Response->operation, Status));
> > > + break;
> > > + }
> > > +
> > > + Dev->Ring.rsp_cons = ++ConsumerIndex;
> > > + /* Nota: callback frees IoData itself */
> > > + if (IoData && IoData->Callback) {
> > > + IoData->Callback (IoData, Status ? EFI_DEVICE_ERROR :
> > > EFI_SUCCESS);
> > > + }
> > > + if (Dev->Ring.rsp_cons != ConsumerIndex) {
> > > + /* We reentered, we must not continue here */
> >
> > Uh, how does that happen?
>
> That an original comment, I don't think it's possible yet in ovmf.
>
> > > + break;
> > > + }
> > > + }
> > > +
> > > + RING_FINAL_CHECK_FOR_RESPONSES (&Dev->Ring, More);
> > > + } while (More != 0);
> > > +}
>
> --
> Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |