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 EA80820945BE6 for ; Wed, 13 Sep 2017 11:04:19 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 77690356D0; Wed, 13 Sep 2017 18:07:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 77690356D0 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.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 C75F15D6AE; Wed, 13 Sep 2017 18:07:15 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <20170911121657.34992-1-brijesh.singh@amd.com> <20170911121657.34992-7-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <6c148d15-a98e-59c0-9b9e-227f3456233a@redhat.com> Date: Wed, 13 Sep 2017 20:07:14 +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: <20170911121657.34992-7-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 13 Sep 2017 18:07:17 +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 18:04:20 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/11/17 14:16, Brijesh Singh wrote: > When device is behind IOMMU, driver is require to pass the device address > of TxBuf in the Tx VRING. The patch adds helper functions and data > structure to map and unmap the TxBuf system physical address to a device > address. > > Since the TxBuf is returned back to caller from VirtioNetGetStatus() hence > we use OrderedCollection interface to save the TxBuf system physical to > device address mapping. After the TxBuf is succesfully transmitted > VirtioNetUnmapTxBuf() does the reverse lookup in OrderedCollection data > structure to get the system physical address of TxBuf for a given device > address. > > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Tom Lendacky > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/VirtioNetDxe/VirtioNet.inf | 1 + > OvmfPkg/VirtioNetDxe/VirtioNet.h | 32 ++++ > OvmfPkg/VirtioNetDxe/SnpInitialize.c | 15 +- > OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 188 ++++++++++++++++++++ > 4 files changed, 235 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.inf b/OvmfPkg/VirtioNetDxe/VirtioNet.inf > index a855ad4ac154..9ff6d87e6190 100644 > --- a/OvmfPkg/VirtioNetDxe/VirtioNet.inf > +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.inf > @@ -49,6 +49,7 @@ > DebugLib > DevicePathLib > MemoryAllocationLib > + OrderedCollectionLib > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiLib > diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h > index 3f48bcc6b67c..906bec8e88f3 100644 > --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h > +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > > #define VNET_SIG SIGNATURE_32 ('V', 'N', 'E', 'T') > > @@ -100,6 +101,7 @@ typedef struct { > VOID *TxSharedReqMap; // VirtioNetInitTx > UINT16 TxLastUsed; // VirtioNetInitTx > EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx > + ORDERED_COLLECTION *TxBufMapInfoCollection; // VirtioNetInitTx (1) Hmm, I like the name, but I don't like the unaligned comment. I also don't like the idea of moving all the other comments. Can you just call this field "TxBufCollection"? Then the comment can be aligned. > } VNET_DEV; > > > @@ -281,6 +283,36 @@ VirtioNetUninitRing ( > ); > > // > +// utility functions to map caller-supplied Tx buffer system physical address > +// to a device address and vice versa > +// I like this comment! > +EFI_STATUS > +EFIAPI > +VirtioNetMapTxBuf ( > + IN VNET_DEV *Dev, > + IN UINT16 DescIdx, > + IN VOID *Buffer, > + IN UINTN NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress > + ); > + > +INTN > +EFIAPI > +VirtioNetTxMapInfoCompare ( > + IN CONST VOID *UserStruct1, > + IN CONST VOID *UserStruct2 > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioNetUnmapTxBuf ( > + IN VNET_DEV *Dev, > + IN UINT16 DescIdx, > + OUT VOID **Buffer, > + IN EFI_PHYSICAL_ADDRESS DeviceAddress > + ); > + I will comment on these functions in detail below, I just want to say: - the location of the declarations in this header file is fine, but (2) Please try to keep the same order between the new functions in "VirtioNet.h", and in "SnpSharedHelpers.c". > +// > // event callbacks > // > VOID > diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > index 6cedb406a172..a8ffb9a8a7b1 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c > +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > @@ -176,6 +176,15 @@ VirtioNetInitTx ( > return EFI_OUT_OF_RESOURCES; > } > > + Dev->TxBufMapInfoCollection = OrderedCollectionInit ( > + VirtioNetTxMapInfoCompare, > + VirtioNetTxMapInfoCompare > + ); (3) VirtioNetTxMapInfoCompare is not right for both of the arguments, but I'll come to that below. > + if (Dev->TxBufMapInfoCollection == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + goto FreeTxFreeStack; > + } > + (4) The code is good, but please update the documentation of "@retval EFI_OUT_OF_RESOURCES". > // > // Allocate TxSharedReq header and map with BusMasterCommonBuffer so that it > // can be accessed equally by both processor and device. > @@ -186,7 +195,7 @@ VirtioNetInitTx ( > &TxSharedReqBuffer > ); > if (EFI_ERROR (Status)) { > - goto FreeTxFreeStack; > + goto UninitMapInfoCollection; > } > > ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq); > @@ -267,6 +276,10 @@ FreeTxSharedReqBuffer: > EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)), > TxSharedReqBuffer > ); > + > +UninitMapInfoCollection: > + OrderedCollectionUninit (Dev->TxBufMapInfoCollection); > + > FreeTxFreeStack: > FreePool (Dev->TxFreeStack); > > diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > index 2ec3dc385a9f..dafb538b4b5a 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > @@ -18,6 +18,17 @@ > > #include "VirtioNet.h" > > +// > +// The user structure for the ordered collection that will track the mapping > +// info of the packets queued in TxRing > +// > +typedef struct { > + UINT16 DescIdx; > + VOID *Buffer; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + VOID *BufMap; > +} TX_BUF_MAP_INFO; > + Great, I like the new name for the structure, and the leading comment too. Let me comment on the fields: (5) Please drop the DescIdx field. The reverse mapping should occur strictly from DeviceAddress to Buffer, as we documented in the previous patch. DescIdx is indeed *how* we get at DeviceAddress in the next patch (namely the hypervisor sends us the DescIdx for the head descriptor, we calculate the DescIdx for the tail descriptor, and then grab DeviceAddress from the tail descriptor.) However, for the reverse mapping itself, DescIdx is irrelevant. The key field for the reverse lookup is DeviceAddress. (6) Please add a postfix style comment to the DeviceAddress field: EFI_PHYSICAL_ADDRESS DeviceAddress; // lookup key for reverse mapping > /** > Release RX and TX resources on the boundary of the > EfiSimpleNetworkInitialized state. > @@ -54,6 +65,20 @@ VirtioNetShutdownTx ( > IN OUT VNET_DEV *Dev > ) > { > + ORDERED_COLLECTION_ENTRY *Entry, *Entry2; > + TX_BUF_MAP_INFO *TxBufMapInfo; > + > + for (Entry = OrderedCollectionMin (Dev->TxBufMapInfoCollection); > + Entry != NULL; > + Entry = Entry2) { > + Entry2 = OrderedCollectionNext (Entry); > + TxBufMapInfo = (TX_BUF_MAP_INFO *)Entry2; This assignment is wrong, for two reasons: first, you should be using Entry, not Entry2, second, this is not how the OrderedCollectionLib API works. The OrderedCollectionLib class operates with four objects; from primitive to abstract/complex: - ordering key - user structure (containing an ordering key) - iterator (pointing to a collection node which in turn points to a user structure) - collection (consisting of all nodes, and also knowing the comparator functions for ordering keys and user structures) If you look over "MdePkg/Include/Library/OrderedCollectionLib.h", the functions explain what they take and what they output. Let me go over some of the functions quickly: - comparator between two user structures: this is needed whenever you insert (link) a brand new user structure into the collection. The structure being inserted has to be compared against existing structures, using the ordering key field in both. - comparator between a bare ordering key and a full structure: this is needed when searching for an existent structure in the collection. For the lookup, you don't want to create a fake user structure, just provide the bare key itself. So the comparator has to compare the bare key against the complete user structures in the collection (using the ordering keys embedded into the latter). - OrderedCollectionUserStruct(): this is a conversion function from "iterator" ("entry") to "user structure". Several functions output iterators: (a) lookup, (b) minimum, (c) maximum, (d) next, (e) prev. Furthermore, (f) "insert" outputs an iterator for the just inserted structure, or to the conflicting structure, if there is one. - find: takes a standalone key and looks up the user structure with that key in the collection. Returns an iterator. If you care about the rest of the fields in the found structure, you have to conver the iterator to user structure, with OrderedCollectionUserStruct(). - min, max, next, prev: should be obvious if you look at the prototypes - insert: takes a full user structure and links it into the collection, if there is no conflict between keys. Optionally outputs an iterator for the just-linked user structure (for example you can use that to advance to the just-following element in the collection, for whatever reason). If there is a conflict (user structure already present with that key), the iterator for the conflicting user struct is output (optionally). If you care about the rest of the fields in the conflicting element, you have to convert the iterator with OrderedCollectionUserStruct(). - delete: takes an iterator (which you may have generated in any way described above), unlinks the corresponding user structure from the tree, and (optionally) hands the user structure to the caller at once. The last part is a convenience parameter, because frequently you have to unlink a user structure (which invalidates its iterator) and then destroy the user structure too. Normally you'd have to call OrderedCollectionUserStruct() on the iterator *first* (to get the user structure), and call delete *second*, on the same iterator. With the convenience parameter, it's OK to call just delete. (7) Therefore please simply drop this assignment, and below I'll show how to lay out the loop body. > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, TxBufMapInfo->BufMap); > + FreePool (TxBufMapInfo); > + OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, NULL); (8) OK, so here's how to do the loop body (after assigning "Entry2", which is good): VOID *UserStruct; OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, &UserStruct); TxBufMapInfo = UserStruct; Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, TxBufMapInfo->BufMap); FreePool (TxBufMapInfo); > + } > + OrderedCollectionUninit (Dev->TxBufMapInfoCollection); > + > Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap); > Dev->VirtIo->FreeSharedPages ( > Dev->VirtIo, (9) In this function (VirtioNetShutdownTx()), please keep the same order of teardown as on the error path in VirtioNetInitTx(). Namely, the ordered collection should be emptied and uninited just before freeing "Dev->TxFreeStack". > @@ -83,3 +108,166 @@ VirtioNetUninitRing ( > Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap); > VirtioRingUninit (Dev->VirtIo, Ring); > } > + > + > +/** > + Map Caller-supplied TxBuf buffer to the device-mapped address > + > + @param[in] Dev The VNET_DEV driver instance which wants to > + map the Tx packet. > + @param[in] DescIdx VRING descriptor index which will point to > + the device address (10) Please drop the "DescIdx" parameter. > + @param[in] Buffer The system physical address of TxBuf > + @param[in] NumberOfBytes Number of bytes to map > + @param[out] DeviceAddress The resulting device address for the bus > + master access. > + > + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to > + a lack of resources. > + @retval EFI_INVALID_PARAMETER The VRING descriptor index is already mapped. (11) Please drop the EFI_INVALID_PARAMETER retval (more on it below). (12) Please add "@return Status codes from VirtioMapAllBytesInSharedBuffer()". (13) Please add "@retval EFI_SUCCESS ..." for completeness. > +*/ > +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. > + ASSERT (OrderedCollectionUserStruct (Entry) == TxBufMapInfo); (15) I suggest to drop the "Entry" local variable, together with the ASSERT() that depends on it. Also, pass NULL in &Entry's stead to OrderedCollectionInsert(). (... if you really would like to keep the ASSERT(), it's your call, ultimately.) > + > + *DeviceAddress = Address; > + > + return EFI_SUCCESS; > + > +UnmapTxBufBuffer: (16) I think this label would be nicer if we renamed it to "UnmapTxBuf". > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping); > + > +FreeTxBufMapInfo: > + FreePool (TxBufMapInfo); > + return Status; > +} > + > +/** > + Unmap (aka reverse mapping) device mapped TxBuf buffer to the system > + physical address > + > + @param[in] Dev The VNET_DEV driver instance which wants to > + map the Tx packet. (17) s/map/reverse- and unmap/ > + @param[in] DescIdx VRING descriptor index which point to > + the device address (18) Please drop DescIdx > + @param[out] Buffer The system physical address of TxBuf > + @param[out] DeviceAddress The device address for the TxBuf The [out] label on DeviceAddress is wrong, the variable is marked IN below. This error in the patch is not surprising, given that the function doesn't actually use "DeviceAddress" for anything right now. This will be fixed in v3, where DescIdx disappears from this representation, and DeviceAddress will become the reverse lookup key. (19) So please change [out] to [in] on DeviceAddress. > + > + @retval EFI_INVALID_PARAMETER The VRING descriptor index is not mapped (20) Please update the comment: "DeviceAddress is not mapped" (21) Please add @retval EFI_SUCCESS The TxBuf at DeviceAddress has been unmapped, and Buffer has been set to TxBuf's system physical address. > +*/ > +EFI_STATUS > +EFIAPI > +VirtioNetUnmapTxBuf ( > + IN VNET_DEV *Dev, > + IN UINT16 DescIdx, > + OUT VOID **Buffer, > + IN EFI_PHYSICAL_ADDRESS DeviceAddress > + ) > +{ > + TX_BUF_MAP_INFO StandaloneKey; > + ORDERED_COLLECTION_ENTRY *Entry; > + TX_BUF_MAP_INFO *UserStruct; > + VOID *Ptr; > + EFI_STATUS Status; > + > + StandaloneKey.DescIdx = DescIdx; > + Entry = OrderedCollectionFind (Dev->TxBufMapInfoCollection, &StandaloneKey); Here you actually construct a full user structure, *not* a stand-alone key. The stand-alone key (in version 2) is simply the "DescIdx" input parameter. It "stands alone" because it is not embedded in a user structure (i.e., a TX_BUF_MAP_INFO object). Therefore, in version 3, (22) please drop the "StandaloneKey" local variable, and call OrderedCollectionFind() like this: Entry = OrderedCollectionFind (Dev->TxBufMapInfoCollection, &DeviceAddress); Namely, in version 3, DeviceAddress is the ordering key, and passing it like above makes it "stand alone". > + if (Entry == NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, &Ptr); (23) This function call is correct, but the variable names can be improved: - please rename "UserStruct" to "TxBufMapInfo", - please rename "Ptr" to "UserStruct". > + > + UserStruct = Ptr; > + ASSERT (UserStruct->DescIdx == DescIdx); (24) drop the ASSERT() (... well, if you don't trust OrderedCollectionLib, you can keep the ASSERT() -- same story as point (15), it's your call; but then update the key field to DeviceAddress) > + > + *Buffer = UserStruct->Buffer; (25) Too much whitespace. > + Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, UserStruct->BufMap); > + FreePool (UserStruct); > + > + return Status; > +} Here we unmap a bus master read operation. An unmap operation tears down the association unconditionally, the only thing that can fail is committing the device's data to system memory -- but that doesn't make sense after a bus master read op. Even if we got an error status from the unmap, the function still succeeds. (26) Therefore please ignore the return value of UnmapSharedBuffer() -- do not assign it to Status --, and return the EFI_SUCCESS constant. (27) In fact you can drop the "Status" variable. > + > +/** > + Comparator function for two user structures. > + > + @param[in] UserStruct1 Pointer to the first user structure. > + > + @param[in] UserStruct2 Pointer to the second user structure. > + > + @retval <0 If UserStruct1 compares less than UserStruct2. > + > + @retval 0 If UserStruct1 compares equal to UserStruct2. > + > + @retval >0 If UserStruct1 compares greater than UserStruct2. > +*/ (28) Please update the documentation to: Comparator function for two TX_BUF_MAP_INFO objects. ... ... Pointer to the (first|second) TX_BUF_MAP_INFO object The parameter names themselves (UserStruct1, UserStruct2) are fine. > +INTN > +EFIAPI > +VirtioNetTxMapInfoCompare ( (29) The function name should be "VirtioNetTxBufMapInfoCompare". > + IN CONST VOID *UserStruct1, > + IN CONST VOID *UserStruct2 > + ) > +{ > + CONST TX_BUF_MAP_INFO *MapInfo1; > + CONST TX_BUF_MAP_INFO *MapInfo2; > + > + MapInfo1 = UserStruct1; > + MapInfo2 = UserStruct2; > + > + return MapInfo1->DescIdx < MapInfo2->DescIdx ? -1 : > + MapInfo1->DescIdx > MapInfo2->DescIdx ? 1 : > + 0; > +} > (30) Please replace the DescIdx field references with "DeviceAddress" field references. (31) Please add the following function (to VirtioNet.h as well), and pass it to OrderedCollectionInit() as second parameter: --------------- /** Compare a standalone DeviceAddress against a TX_BUF_MAP_INFO object containing an embedded DeviceAddress. @param[in] StandaloneKey Pointer to DeviceAddress, which has type EFI_PHYSICAL_ADDRESS. @param[in] UserStruct Pointer to the TX_BUF_MAP_INFO object with the embedded DeviceAddress. @retval <0 If StandaloneKey compares less than UserStruct's key. @retval 0 If StandaloneKey compares equal to UserStruct's key. @retval >0 If StandaloneKey compares greater than UserStruct's key. **/ INTN EFIAPI VirtioNetTxBufDeviceAddressCompare ( IN CONST VOID *StandaloneKey, IN CONST VOID *UserStruct ) { CONST EFI_PHYSICAL_ADDRESS *DeviceAddress; CONST TX_BUF_MAP_INFO *MapInfo; DeviceAddress = StandaloneKey; MapInfo = UserStruct; return *DeviceAddress < MapInfo->DeviceAddress ? -1 : *DeviceAddress > MapInfo->DeviceAddress ? 1 : 0; } --------------- Thank you! Laszlo