Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested

[Laszlo Ersek <lersek@redhat.com>]

On 04/20/15 11:19, Gerd Hoffmann wrote:
> add tseg window to smram region, so cpus can access it in smm mode.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/q35.c         | 13 +++++++++++++
>  include/hw/pci-host/q35.h |  2 +-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 412ff0a..7d21399 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -345,6 +345,13 @@ static void mch_update_smram(MCHPCIState *mch)
>                                          mch->below_4g_mem_size - tseg_size,
>                                          &mch->tseg_blackhole, 1);
>  
> +    memory_region_set_enabled(&mch->tseg_window, tseg_size);
> +    memory_region_set_size(&mch->tseg_window, tseg_size);
> +    memory_region_set_address(&mch->tseg_window,
> +                              mch->below_4g_mem_size - tseg_size);
> +    memory_region_set_alias_offset(&mch->tseg_window,
> +                                   mch->below_4g_mem_size - tseg_size);
> +
>      memory_region_transaction_commit();
>  }
>  
> @@ -500,6 +507,12 @@ static void mch_realize(PCIDevice *d, Error **errp)
>                                          mch->below_4g_mem_size,
>                                          &mch->tseg_blackhole, 1);
>  
> +    memory_region_init_alias(&mch->tseg_window, OBJECT(mch), "tseg-window",
> +                             mch->ram_memory, mch->below_4g_mem_size, 0);
> +    memory_region_set_enabled(&mch->tseg_window, false);
> +    memory_region_add_subregion(&mch->smram, mch->below_4g_mem_size,
> +                                &mch->tseg_window);
> +
>      init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
>               mch->pci_address_space, &mch->pam_regions[0],
>               PAM_BIOS_BASE, PAM_BIOS_SIZE);
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index ba64c70..23b7700 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -55,7 +55,7 @@ typedef struct MCHPCIState {
>      PAMMemoryRegion pam_regions[13];
>      MemoryRegion smram_region, open_high_smram;
>      MemoryRegion smram, low_smram, high_smram;
> -    MemoryRegion tseg_blackhole;
> +    MemoryRegion tseg_blackhole, tseg_window;
>      PcPciInfo pci_info;
>      ram_addr_t below_4g_mem_size;
>      ram_addr_t above_4g_mem_size;
> 

Why is this necessary? If you disable the black hole overlay, the access
will go to the RAM. (Or can't that be done per-CPU?)

I'm thinking, the last 1 / 2 / 8 megabytes should behave as RAM in all
of the following cases:
- no SMRAM programmed (tseg size = 0)
- SMRAM programmed (tseg size > 0), and it is open
- SMRAM programmed (tseg size > 0) and closed, but CPU in SMM

Does any of the above require anything else than simply disabling the
black hole overlay? (Sorry if I'm missing something obvious!) Assuming
that a lockdown prevents the reprogramming of tseg size, I think the
above could all be unified.

... Another question, related to SMM (but not related to SMRAM): Paolo,
am I right to think that we'll be keying off at least two independent
things of SMM-or-not: one is access to SMRAM (tseg), for LockBox and SMM
driver purposes, the other is pflash access (with the MemTxAttrs thing),
for the varstore?

(BTW in the meantime I found out about
EFI_SMM_FIRMWARE_VOLUME_BLOCK_PROTOCOL too, so at least in *theory* it
is clear what has to be done with / for the flash driver.)

Thanks!
Laszlo