From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::241; helo=mail-io0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x241.google.com (mail-io0-x241.google.com [IPv6:2607:f8b0:4001:c06::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 32B59203BBB80 for ; Thu, 24 May 2018 07:54:04 -0700 (PDT) Received: by mail-io0-x241.google.com with SMTP id d73-v6so2636627iog.3 for ; Thu, 24 May 2018 07:54:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=DCTXF8uvfdK9f7+km4fuiLZ0Ehtr4C0FFskPK/jNAoE=; b=UQrtz3CT77eIzedC97Ge9F1S/GQzpwuVon9bwnbg1RjuErJUVfPmXBZ9O1aisy/yUB L9uLMxHj2+kJwk02ToV6AMEVAtUDWpHqigtMAokBT9FJqI6U5JY27l1TmeozZp4ZrvEs TWUL8AwwHh4whQNinGGc8lIRJqZJIwzZejLaE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=DCTXF8uvfdK9f7+km4fuiLZ0Ehtr4C0FFskPK/jNAoE=; b=r9WwY1Wlh55BE+DwsvsX9Ntj6CYobFuGwUAHyWhyo2o6fTG56jtCbOjdlFJ9kuqOFY K7pTX9EjBsRRQkmXhNjDTRPN8gLEk0PoZgd2fEtO1Nw69t5bNVvWMzH4ZFZGaEeprXAq ZNT4hnOUMft5G81P9wvFcJJrJpe8B1I9jb0lT8cK+hgcLDOHw+5R9pEZ+xW3dIOa/4nP iUmG1YBAlpJdSxRkJ9Jeism6EGQhg2Dd0Wt7zyUeuDnVIVnzRxPeLQ4eBDLzglYzRgQ8 6qrisXsjPixYBUlhEAqYDozbKeohGjPFT07RfVLRIsoYQ1OAeclWO+LJfYkGjp2q9ncJ 4S3A== X-Gm-Message-State: ALKqPwdNvDD/rl/u6U3zsw/EIttuWBXnwAG7nVl6F8HaM0hT8qh60H/p e5osMGLVldXzpeXsYbmzbEzWlpO9lMxtntxDr3265Q== X-Google-Smtp-Source: AB8JxZrSwAYetAvkfZlbHdapCT5mv+Grx8vhihGPzQ/zwYUyb1QYUk9NbCDejMvYqPO1VsvAzznRW/GmZ90NwJGcx0s= X-Received: by 2002:a6b:268b:: with SMTP id m133-v6mr7387602iom.107.1527173643421; Thu, 24 May 2018 07:54:03 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Thu, 24 May 2018 07:54:02 -0700 (PDT) In-Reply-To: <1cef03f3-f3be-9fd6-2644-a16ec95f1949@redhat.com> References: <20180523202121.8125-1-lersek@redhat.com> <20180523202121.8125-4-lersek@redhat.com> <1cef03f3-f3be-9fd6-2644-a16ec95f1949@redhat.com> From: Ard Biesheuvel Date: Thu, 24 May 2018 16:54:02 +0200 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen Subject: Re: [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 May 2018 14:54:04 -0000 Content-Type: text/plain; charset="UTF-8" On 24 May 2018 at 16:50, Laszlo Ersek wrote: > On 05/24/18 10:13, Ard Biesheuvel wrote: >> On 23 May 2018 at 22:21, Laszlo Ersek wrote: >>> Add a library class, and a UEFI_DRIVER lib instance, that are layered on >>> top of PciCapLib, and allow clients to plug an EFI_PCI_IO_PROTOCOL backend >>> into PciCapLib, for config space access. >>> >>> (Side note: >>> >> >> Again, please retain the below. > > Will do. > >>> +STATIC >>> +EFI_STATUS >>> +ProtoDevTransferConfig ( >>> + IN EFI_PCI_IO_PROTOCOL *PciIo, >>> + IN EFI_PCI_IO_PROTOCOL_CONFIG TransferFunction, >>> + IN UINT16 ConfigOffset, >>> + IN OUT UINT8 *Buffer, >>> + IN UINT16 Size >>> + ) >>> +{ >>> + while (Size > 0) { >>> + EFI_PCI_IO_PROTOCOL_WIDTH Width; >>> + UINT16 Count; >>> + EFI_STATUS Status; >>> + UINT16 Progress; >>> + >>> + // >>> + // Pick the largest access size that is allowed by the remaining transfer >>> + // Size and by the alignment of ConfigOffset. >>> + // >>> + // When the largest access size is available, transfer as many bytes as >>> + // possible in one iteration of the loop. Otherwise, transfer only one >>> + // unit, to improve the alignment. >>> + // >>> + if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) { >> >> Ugh. Just use '4' or sizeof(UINT32). >> >>> + Width = EfiPciIoWidthUint32; >>> + Count = Size >> Width; >>> + } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) { >>> + Width = EfiPciIoWidthUint16; >>> + Count = 1; >>> + } else { >>> + Width = EfiPciIoWidthUint8; >>> + Count = 1; >>> + } > > I used "BITx" and "(BITx - 1)" for consistency, and because they seemed > to convey the idea well (namely, shifting down the alignment). > > I'm fine replacing "BIT2" with "4", but then I believe I should also > replace "(BIT2 - 1)" with "3". Similarly, replace "BIT1" with "2", and > "(BIT1 -1)" with 1. > > Do you prefer the current code or the decimal constants? > IMHO, BITx is for bitmasks, not for numerical constants. So yes, if you think (as do I) that sizeof(UINTnn) is too wordy, just use the plain numbers please.