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

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

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