Re: [PATCH 5/5] OvmfPkg/AcpiPlatformDxe: replay QEMU_LOADER_WRITE_POINTER commands at S3

[Laszlo Ersek <lersek@redhat.com>]

On 02/21/17 01:17, Jordan Justen wrote:
> On 2017-02-17 14:48:52, Laszlo Ersek wrote:
>> On 02/17/17 23:41, Laszlo Ersek wrote:
>>
>> [snip]
>>
>>> I briefly considered a QemuFwCfgLib API that lets the caller inquire
>>> about DMA enablement, but it just doesn't feel right, and (considering
>>> the above) it's not even useful. Later I would like to abstract away
>>> this S3 boot script munging, from both this driver and SmmControl2Dxe,
>>> but the interface is a hard nut:
>>>
>>> - SEC lib instance (exists only for x86): must return EFI_UNSUPPORTED
>>> - PEI/DXE instance (exists only for x86): must be split into
>>>   - PEI only: returns EFI_UNSUPPORTED
>>>   - DXE only:
>>>     - returns EFI_UNSUPPORTED if S3 is disabled
>>>     - works if the protocol is available
>>>     - what does it do if the protocol is not available *yet*?
>>>       - can't use a DEPEX (whole client module won't load with S3
>>>         disabled!)
>>>       - register protocol notify in constructor? then what?
>>>         - collect operations and write them to boot script at EndOfDxe?
>>>           - that always turns out to be a bad idea, due to unspecified
>>>             callback invocation order
>>
>> One thing I can imagine here is: simply push the dependency handling to
>> the client module. If the protocol is not found, just return
>> EFI_NOT_STARTED, and let the client module deal with that. The client
>> can set its own DEPEX, or register a protocol notify as necessary. So I
>> guess this could be solved after all, hopefully.
> 
> Yeah, I figured the S3 depex is the tricky part. Also, adding an S3
> specific interface doesn't fit well in QemuFwCfgLib, which I think
> should focus on just the raw fw-cfg interface. Based on that, I'm not
> particularly enthused about QemuFwCfgS3Enabled.
> 
> Maybe we could add a new S3+fw-cfg library interface? Maybe
> QemuS3FwCfgLib? I'm not sure it makes sense, or solves all the
> requirements anyhow...

I think it would make sense, yes; I actually thought of that too, but
didn't dare suggest it, remembering how opposed you had been to a
separate IoFifoLib recently :)

> 
> ...so, let's figure that out later. :)

I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=394>  for this.

> 
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Series committed in range 85520606ad45..df73df138d9d.

I plan to post an "easy" followup series, but that won't change
functionality, and there's no similar time pressure for it.

Thank you very much for the prompt review!
Laszlo

> 
>>> - DXE only instance (exists for arm/aarch64 only): very different from
>>>   the x86 impl already (uses chunked MMIO at the bottom, not REP IO),
>>>   so unification with the above is hard
>>>
>>> While writing this patch, I was racking my brain for hours about this. I
>>> think I must have restarted this patch three or four times just because
>>> of that.
>>>
>>> Ultimately, it's the second driver where we use a meaningful boot script
>>> fragment, and usually the third occurrence of a pattern is where the
>>> generics start to emerge, and (hopefully) become a candidate for
>>> factoring out. I dislike the repetitive and hard-to-read nature of the
>>> opcode additions (hence the profuse comments above), but from the two
>>> instances we can look at (SmmControl2Dxe and this patch), I couldn't
>>> really abstract away a natural interface.
>>>
>>> I think it is a relatively big task anyway, deserving its own BZ (not to
>>> be confused with the also separate BZ that I proposed for residual /
>>> cleanup patches on top of this series, should you recommend such.) If
>>> you felt inclined to research / prototype that, that would be huge...
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>>
>>>> -Jordan
>>
>>