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

[Jordan Justen <jordan.l.justen@intel.com>]

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...

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

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

> > - 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
> 
>