Re: [PATCH] q35: lpc: allow to lock down 128K RAM at default SMBASE address


Laszlo Ersek
 

On 09/10/19 17:58, Igor Mammedov wrote:
On Mon, 9 Sep 2019 21:15:44 +0200
Laszlo Ersek <lersek@redhat.com> wrote:
[...]

It looks like fwcfg smi feature negotiation isn't reusable in this case.
I'd prefer not to add another device just for another SMI feature
negotiation/activation.
I thought it could be a register on the new CPU hotplug controller that
we're going to need anyway (if I understand correctly, at least).

But:

How about stealing reserved register from pci-host similar to your
extended TSEG commit (2f295167 q35/mch: implement extended TSEG sizes)?
(Looking into spec can (ab)use 0x58 or 0x59 register)
Yes, that should work.

In fact, I had considered 0x58 / 0x59 when looking for unused registers
for extended TSEG configuration:

http://mid.mail-archive.com/d8802612-0b11-776f-b209-53bbdaf67515@redhat.com

So I'm OK with this, thank you.

More below:

... I've done some testing too. Applying the QEMU patch on top of
89ea03a7dc83, my plan was:

- do not change OVMF, just see if it continues booting with the QEMU
patch

- then negotiate bit#1 too, in step (1a) -- this is when I'd expect (3a)
to break.

Unfortunately, the result is worse than that; even without negotiating
bit#1 (i.e. in the baseline test), the firmware crashes (reboots) in
step (3a). I've checked "info mtree", and all occurences of
"smbase-blackhole" and "smbase-blackhole" are marked [disabled]. I'm not
sure what's wrong with the baseline test (i.e. without negotiating
bit#1). If I drop the patch (build QEMU at 89ea03a7dc83), then things
work fine.
that was a bug in my code, which always made lock down effective on
feature_ok selection, which breaks relocation for reasons you've
explained above.

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 17a8cd1b51..32ddf54fc2 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -383,7 +383,7 @@ static const MemoryRegionOps smbase_blackhole_ops = {

static void ich9_lpc_smbase_locked_update(ICH9LPCState *lpc)
{
- bool en = lpc->smi_negotiated_features & ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT;
+ bool en = lpc->smi_negotiated_features & (UINT64_C(1) << ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT);

memory_region_transaction_begin();
memory_region_set_enabled(&lpc->smbase_blackhole, en);
I see.

ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT is 1, with the intended value for
bitmask checkin) being 1LLU<<1 == 2LLU.

Due to the bug, the function would check value 1 in the bitmask -- which
in fact corresponds to bit#0. Bit#0 happens to be
ICH9_LPC_SMI_F_BROADCAST_BIT.

And because OVMF would negotiate that feature (= broadcast SMI) even in
the baseline test, it ended up enabling the "locked smbase" feature too.

So ultimately I think we can consider this a valid test (= with
meaningful result); the result is that we can't reuse these fw_cfg files
for "locked smbase", as discussed above.

Thanks!
Laszlo

Join rfc@edk2.groups.io to automatically receive all group messages.