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 E111C21D49C9E for ; Wed, 9 Aug 2017 09:48:21 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A5D3FFC3DA; Wed, 9 Aug 2017 16:50:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A5D3FFC3DA Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-102.phx2.redhat.com [10.3.116.102]) by smtp.corp.redhat.com (Postfix) with ESMTP id D4D136A57A; Wed, 9 Aug 2017 16:50:37 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1502107139-412-1-git-send-email-brijesh.singh@amd.com> <1502107139-412-3-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <659fe2f7-0d17-cde7-c3d3-36e541ff7805@redhat.com> Date: Wed, 9 Aug 2017 18:50:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1502107139-412-3-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 09 Aug 2017 16:50:39 +0000 (UTC) Subject: Re: [PATCH v1 02/14] OvmfPkg/Virtio10Dxe: Implement new member 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, 09 Aug 2017 16:48:22 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit (1) For the subject, I suggest again OvmfPkg/Virtio10Dxe: Implement IOMMU-like member functions On 08/07/17 13:58, Brijesh Singh wrote: > The patch implements the newly added member functions by respectively > delegating the job to: > > - EFI_PCI_IO_PROTOCOL.AllocateBuffer() -- with BootServicesData > - EFI_PCI_IO_PROTOCOL.FreeBuffer() > - EFI_PCI_IO_PROTOCOL.Map() > - EFI_PCI_IO_PROTOCOL.Unmap() (2) looks good, but please spell out the new virtio protocol member names as well. (This will become more important in the commit message of the next patch, where the reader has to guess what member functions exactly get the "no-op" implementation.) And then we should be consistent in the commit messages. > > Suggested-by: Laszlo Ersek > 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/Virtio10Dxe/Virtio10.c | 114 +++++++++++++++++++- > 1 file changed, 113 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c > index a8a6a58c3f1d..5bc8f1c7ee27 100644 > --- a/OvmfPkg/Virtio10Dxe/Virtio10.c > +++ b/OvmfPkg/Virtio10Dxe/Virtio10.c > @@ -2,6 +2,7 @@ > A non-transitional driver for VirtIo 1.0 PCI devices. > > Copyright (C) 2016, Red Hat, Inc. > + Copyright (C) 2017, AMD Inc, All rights reserved.
> > This program and the accompanying materials are licensed and made available > under the terms and conditions of the BSD License which accompanies this > @@ -772,6 +773,113 @@ Virtio10ReadDevice ( > return Status; > } > > +STATIC > +EFI_STATUS > +EFIAPI > +Virtio10AllocateSharedPages ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN Pages, > + IN OUT VOID **HostAddress > + ) > +{ > + VIRTIO_1_0_DEV *Dev; > + EFI_STATUS Status; > + > + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This); > + > + Status = Dev->PciIo->AllocateBuffer ( > + Dev->PciIo, > + AllocateAnyPages, > + EfiBootServicesData, > + Pages, > + HostAddress, > + 0 (3) We should pass Attributes=EFI_PCI_ATTRIBUTE_MEMORY_CACHED, even if only for cosmetic purposes (at the moment). (4) Independently, this makes me think about EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE. * PciIoAllocateBuffer() [MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c] sets the EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute internally, before calling out to the PCI Root Bridge IO Protocol, *if* "PciIoDevice->Attributes" has EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE set. In turn: - If there is an IOMMU protocol in the system, then RootBridgeIoAllocateBuffer() [MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c] passes EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE through to the IOMMU protocol's AllocateBuffer() member *if* "RootBridge->DmaAbove4G" is set. Otherwise the DUAL_ADDRESS_CYCLE hint is cleared. (EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE has the same numeric value, 0x8000, as EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE.) - If there is no IOMMU protocol in the system, then RootBridgeIoAllocateBuffer() itself allocates memory, again obeying EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE and "RootBridge->DmaAbove4G". So regardless of whether there is an IOMMU protocol in the system or not, on the PCI Root Bridge IO level, 64-bit AllocateBuffer() is be permitted according to device-level DUAL_ADDRESS_CYCLE *AND* root-bridge-level DmaAbove4G. * Let's consider what's going to happen if we convert the virtio drivers to the new virtio proto member functions, and (in addition) Virtio10Dxe is converted to explicit PciIo.AllocateBuffer(): - In OvmfPkg, allocations that used to be plain AllocatePool() or AllocatePages(), will be restricted to 32-bit, due to two reasons (either of which is sufficient in this regard): - In "OvmfPkg/Library/PciHostBridgeLib", we clear DmaAbove4G, for hysterical raisins. - In Virtio10Dxe, we don't set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE. - In ArmVirtPkg, allocations that used to be plain AllocatePool() or AllocatePages(), will also be restricted to 32-bit. While "ArmVirtPkg/Library/FdtPciHostBridgeLib" *sets* DmaAbove4G, lack of EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in Virtio10Dxe alone is suffices to trigger the restriction. * Therefore, please extend this patch with the following: in the Virtio10BindingStart() function [OvmfPkg/Virtio10Dxe/Virtio10.c], please replace SetAttributes = 0; with SetAttributes = EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE; It will not help OvmfPkg, but it will keep the 4GB restriction out of ArmVirtPkg. (I'm unsure if we can simply flip "DmaAbove4G" to TRUE in OvmfPkg.) * (In the past, Ard wrote a number of similar patches for physical device drivers: 167c3fb45674 MdeModulePkg/EhciDxe: enable 64-bit PCI DMA 4e28ea2c29e0 MdeModulePkg/NvmExpressDxe: enable 64-bit PCI DMA df0a0e4b6fae MdeModulePkg/SdMmcPciHcDxe: enable 64-bit PCI DMA 5c1b371a8839 MdeModulePkg/XhciDxe: enable 64-bit PCI DMA e58a71d9c50b MdeModulePkg/PciHostBridgeDxe: restrict 64-bit DMA to devices that support it 4c0b2d25c61c ArmVirtPkg/FdtPciHostBridgeLib: enable 64-bit PCI DMA and that's what we should do here, in separate patches, for the virtio-pci transport drivers.) (5) Looking more at the PciIo attribute setting in both virtio-PCI transport drivers, it seems that I failed to set EFI_PCI_IO_ATTRIBUTE_BUS_MASTER in both. Virtio devices read and write guest RAM (they don't just decode their IO and/or MMIO BARs), which translates to "bus master". So, please clean up my mess :) , and prepend two separate patches to the series: - "OvmfPkg/VirtioPciDeviceDxe: supply missing BUS_MASTER attribute" Please change EFI_PCI_IO_ATTRIBUTE_IO to EFI_PCI_IO_ATTRIBUTE_IO | EFI_PCI_IO_ATTRIBUTE_BUS_MASTER in VirtioPciDeviceBindingStart() [OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c]. - "OvmfPkg/Virtio10Dxe: supply missing BUS_MASTER attribute" Please change SetAttributes = 0; to SetAttributes = EFI_PCI_IO_ATTRIBUTE_BUS_MASTER; in Virtio10BindingStart() [OvmfPkg/Virtio10Dxe/Virtio10.c]. (And then do point (4) on top of that, separately.) Okay, summary of points (3), (4) and (5): - please prepend *two* patches, setting EFI_PCI_IO_ATTRIBUTE_BUS_MASTER in each virtio-pci transport driver -- point (5) - please set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in the modern-only driver, in this patch -- point (4) - please pass Attributes=EFI_PCI_ATTRIBUTE_MEMORY_CACHED to Dev->PciIo->AllocateBuffer(), in this patch -- point (3). Phew, this is complex. :) > + ); > + return Status; > +} > + > +STATIC > +VOID > +EFIAPI > +Virtio10FreeSharedPages ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN Pages, > + IN VOID *HostAddress > + ) > +{ > + VIRTIO_1_0_DEV *Dev; > + > + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This); > + > + Dev->PciIo->FreeBuffer ( > + Dev->PciIo, > + Pages, > + HostAddress > + ); > +} Looks OK. > +STATIC > +EFI_STATUS > +EFIAPI > +Virtio10MapSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN VIRTIO_MAP_OPERATION Operation, > + IN VOID *HostAddress, > + IN OUT UINTN *NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ) > +{ > + EFI_STATUS Status; > + VIRTIO_1_0_DEV *Dev; > + EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation; > + > + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This); > + > + // > + // Map VIRTIO_MAP_OPERATION to EFI_PCI_IO_PROTOCOL_OPERATION > + // > + if (Operation == EfiVirtIoOperationBusMasterRead) { > + PciIoOperation = EfiPciIoOperationBusMasterRead; > + } else if (Operation == EfiVirtIoOperationBusMasterWrite) { > + PciIoOperation = EfiPciIoOperationBusMasterWrite; > + } else if (Operation == EfiVirtIoOperationBusMasterCommonBuffer) { > + PciIoOperation = EfiPciIoOperationBusMasterCommonBuffer; > + } else { > + return EFI_UNSUPPORTED; > + } (6) This mapping lends itself to simplification by virtue of a switch statement. (Please remember that the "case" labels are indented *zero* positions relative to the "switch" keyword.) (7) Rather than EFI_UNSUPPORTED, EFI_INVALID_PARAMETER should be returned ("One or more parameters are invalid.") (8) You don't seem to be using "VirtioOperationMaximum" for range checking (that's fine, with the switch we can check each value individually, and then add a "default" case) -- please consider removing VirtioOperationMaximum from the protocol interface then. Regarding 64-bit transparency: this code is good, as long as we cover point (4) above: - EFI_PCI_IO_PROTOCOL_OPERATION has no separate 32-bit and 64-bit constants, - EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION does make such a distinction, - PciIoMap() in "MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c" shifts the former op constants up to the 64-bit latter op constants, if EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE is enabled for the device (hence my reference to (4)). The rest also looks good. Thanks, Laszlo > + > + Status = Dev->PciIo->Map ( > + Dev->PciIo, > + PciIoOperation, > + HostAddress, > + NumberOfBytes, > + DeviceAddress, > + Mapping > + ); > + return Status; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +Virtio10UnmapSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN VOID *Mapping > + ) > +{ > + EFI_STATUS Status; > + VIRTIO_1_0_DEV *Dev; > + > + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This); > + > + Status = Dev->PciIo->Unmap ( > + Dev->PciIo, > + Mapping > + ); > + > + return Status; > +} > > STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = { > VIRTIO_SPEC_REVISION (1, 0, 0), > @@ -788,7 +896,11 @@ STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = { > Virtio10GetDeviceStatus, > Virtio10SetDeviceStatus, > Virtio10WriteDevice, > - Virtio10ReadDevice > + Virtio10ReadDevice, > + Virtio10AllocateSharedPages, > + Virtio10FreeSharedPages, > + Virtio10MapSharedBuffer, > + Virtio10UnmapSharedBuffer > }; > > >