Re: [PATCH v2 (add signed-off-by)] OvmfPkg: Add bhyve support into AcpiTimerLib

["Laszlo Ersek" <lersek@redhat.com>]

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