From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 97D0121E87982 for ; Wed, 13 Sep 2017 12:21:14 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 336BE883C0; Wed, 13 Sep 2017 19:24:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 336BE883C0 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-116.rdu2.redhat.com [10.10.120.116]) by smtp.corp.redhat.com (Postfix) with ESMTP id BF5C95D977; Wed, 13 Sep 2017 19:24:10 +0000 (UTC) From: Laszlo Ersek To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Ard Biesheuvel , Tom Lendacky References: <20170911121657.34992-1-brijesh.singh@amd.com> <20170911121657.34992-7-brijesh.singh@amd.com> <6c148d15-a98e-59c0-9b9e-227f3456233a@redhat.com> Message-ID: <2adc266a-949d-c546-9bed-d78fea19d969@redhat.com> Date: Wed, 13 Sep 2017 21:24:09 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <6c148d15-a98e-59c0-9b9e-227f3456233a@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 13 Sep 2017 19:24:12 +0000 (UTC) Subject: Re: [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Sep 2017 19:21:14 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/13/17 20:07, Laszlo Ersek wrote: > On 09/11/17 14:16, Brijesh Singh wrote: >> +*/ >> +EFI_STATUS >> +EFIAPI >> +VirtioNetMapTxBuf ( >> + IN VNET_DEV *Dev, >> + IN UINT16 DescIdx, >> + IN VOID *Buffer, >> + IN UINTN NumberOfBytes, >> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress >> + ) >> +{ >> + EFI_STATUS Status; >> + TX_BUF_MAP_INFO *TxBufMapInfo; >> + EFI_PHYSICAL_ADDRESS Address; >> + VOID *Mapping; >> + ORDERED_COLLECTION_ENTRY *Entry; >> + >> + TxBufMapInfo = AllocatePool (sizeof (*TxBufMapInfo)); >> + if (TxBufMapInfo == NULL) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterRead, >> + Buffer, >> + NumberOfBytes, >> + &Address, >> + &Mapping >> + ); >> + if (EFI_ERROR (Status)) { >> + goto FreeTxBufMapInfo; >> + } >> + >> + TxBufMapInfo->DescIdx = DescIdx; >> + TxBufMapInfo->Buffer = Buffer; >> + TxBufMapInfo->DeviceAddress = Address; >> + TxBufMapInfo->BufMap = Mapping; >> + >> + Status = OrderedCollectionInsert ( >> + Dev->TxBufMapInfoCollection, >> + &Entry, >> + TxBufMapInfo >> + ); >> + switch (Status) { >> + case RETURN_OUT_OF_RESOURCES: >> + Status = EFI_OUT_OF_RESOURCES; >> + goto UnmapTxBufBuffer; >> + case RETURN_ALREADY_STARTED: >> + Status = EFI_INVALID_PARAMETER; >> + goto UnmapTxBufBuffer; >> + default: >> + ASSERT (Status == RETURN_SUCCESS); >> + break; >> + } > > (14) Given that, in v3, the ordering key will be > "TX_BUF_MAP_INFO.DeviceAddress", the Status check after > OrderedCollectionInsert() should work like this (i.e., replace the > "switch" with the following): > > if (Status == EFI_OUT_OF_RESOURCES) { > goto UnmapTxBufBuffer; > } > ASSERT_EFI_ERROR (Status); > > In other words, ALREADY_STARTED should *never* be returned, because > the key comes from VirtioMapAllBytesInSharedBuffer(), and should be > unique. If there is a conflict, then the breakage is so serious that > we cannot do anything about it. I'd like to elaborate on my above comment. Let's consider what happens when client code calls SNP.Transmit() several times, in quick succession, using the *exact same* Buffer argument -- for queueing the same packet several times, for whatever reason --, *AND* we are using a virtio protocol implementation that identity-maps the packets. That means ALREADY_STARTED *will* be returned, because DeviceAddress will not be unique. The question is: is this a valid use of SNP.Transmit(), so that we must accommodate it? In order to answer this, let's look at the SNP.GetStatus() interface. SNP.GetStatus() reports transmit completion by returning the original TxBuf address. From the UEFI-2.7 spec, "EFI_SIMPLE_NETWORK.GetStatus()": If TxBuf is not NULL, a recycled transmit buffer address will be retrieved. If a recycled transmit buffer address is returned in TxBuf, then the buffer has been successfully transmitted, and the status for that buffer is cleared. It is clear that the transmit buffer address shall uniquely identify the transmit buffer; and that given a transmit buffer address, there is exactly *one status* for that transmit buffer / transmit buffer address. Therefore the use pattern I described above is invalid. However, to be on the safe side, even in RELEASE builds, I suggest that we keep your original error handling code, with the following modification (note that I'm replacing RETURN_ with EFI_, because we're already investigating an EFI_STATUS variable): -------- switch (Status) { case EFI_OUT_OF_RESOURCES: goto UnmapTxBufBuffer; case EFI_ALREADY_STARTED: // // This should never happen: it implies // // - an identity-mapping VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() // implementation -- which is fine, // // - and an SNP client that queues multiple instances of the exact same // buffer address with SNP.Transmit() -- which is undefined behavior, // based on the TxBuf language in UEFI-2.7, // EFI_SIMPLE_NETWORK.GetStatus(). // ASSERT (FALSE); Status = EFI_INVALID_PARAMETER; goto UnmapTxBufBuffer; default: ASSERT_EFI_ERROR (Status); break; } -------- Thanks! Laszlo