From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.4144.1586414423665224298 for ; Wed, 08 Apr 2020 23:40:23 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=QlOd2mj7; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1586414422; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=1uvap7qg6r6TPm1gvUhAPPWDy8WcQdv5PA3pfQiqSic=; b=QlOd2mj7AfJ8xk38hGp+qH6dtBug1NT1e7IpAjHZTghpPad7LNwSW0trDcgzHzZIQ+BLVR Rp6KavM6j+JTBmCC9wjz+OZY7ALzLjOJmUT0+SEXqs5j+QHxxXNPuo+FskbAB6BYym+Yb5 kNo2rdM3c5IR6f45+r4LzQRdj60Ohds= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-109-WXz61slVMZej2qxR8lM9kw-1; Thu, 09 Apr 2020 02:40:15 -0400 X-MC-Unique: WXz61slVMZej2qxR8lM9kw-1 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 mimecast-mx01.redhat.com (Postfix) with ESMTPS id B7AD68017FC; Thu, 9 Apr 2020 06:40:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-213.ams2.redhat.com [10.36.112.213]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9BFDC5DA7C; Thu, 9 Apr 2020 06:40:13 +0000 (UTC) Subject: Re: [PATCH v2 (add signed-off-by)] OvmfPkg: Add bhyve support into AcpiTimerLib To: Rebecca Cran , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel References: <20200325201652.54298-1-rebecca@bsdio.com> <040c7e56-cb2f-b0ae-736c-0e1a804946f1@bsdio.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 9 Apr 2020 08:40:12 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <040c7e56-cb2f-b0ae-736c-0e1a804946f1@bsdio.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 04/09/20 07:26, Rebecca Cran wrote: > On 3/27/2020 1:01 PM, Laszlo Ersek wrote: >> >> I'm quite happy about this patch, but perhaps for an unexpected reason: >> namely, because it showcases how non-intuitive and unpredictable it can >> be to customize existent code for a new platform. > > Thanks! I was wondering if I should try and add new code into OvmfPkg, > or keep it separate. Also, good point about the commit message: I get > frustrated when people don't write proper/full messages, so I'm happy > you called me out on it. > > > The existing bhyve port removes everything related to QemuFwCfgLib, such > as calls to QemuFwCfgFindFile in PciHostBridgeLib.c. I'm not sure how I > should proceed given that there's so much commonality between the ovmf > and bhyve versions of the file: currently calls to QemuFwCfgFindFile > don't resolve since references are absent from the .dsc file, so I'm > wondering if I should re-add it, add a "#ifndef BHYVE" or similar to > avoid attempting to compile that code, or duplicate the file with that > code removed? I'd recommend the following approaches for your consideration: (1) Yubo Miao is in the process of factoring out logic that is shared between ArmVirtPkg's and OvmfPkg's PciHostBridgeLib instances: http://mid.mail-archive.com/57fd0043-d63a-55b0-9c55-4ca079331885@redhat.com https://edk2.groups.io/g/devel/message/57062 I proposed that the new lib class be called PciHostBridgeUtilityLib. You could follow that development, with the intent of building BhyvePkg's PciHostBridgeLib instance in terms of the new PciHostBridgeUtilityLib. That would decrease code duplication in BhyvePkg. (2) If OvmfPkg's PciHostBridgeLib already does the right thing for BhyvePkg, assuming the QemuFwCfgFindFile ("etc/extra-pci-roots", &FwCfgItem, &FwCfgSize) call returns an error code, then you could introduce a Null instance of the QemuFwCfgLib class, under OvmfPkg/Library/QemuFwCfgLib. The new INF file could be called "QemuFwCfgLibNull.inf". There would be a new source file called "QemuFwCfgLibNull.c", with the following function definitions: - QemuFwCfgIsAvailable: return constant FALSE - QemuFwCfgSelectItem, QemuFwCfgReadBytes, QemuFwCfgWriteBytes, QemuFwCfgSkipBytes: do nothing - QemuFwCfgRead8, QemuFwCfgRead16, QemuFwCfgRead32, QemuFwCfgRead64: return 0 - QemuFwCfgFindFile: return RETURN_UNSUPPORTED Then you could resolve QemuFwCfgLib to this Null instance in the bhyve DSC file. (This approach might help you reuse further OvmfPkg modules in BhyvePkg. A new QemuFwCfgLibNull instance could simplify the existent "OvmfPkg/OvmfXen.dsc" platform too.) Thanks Laszlo