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

[Laszlo Ersek <lersek@redhat.com>]

In general, commit messages for the series would be appreciated by the
uninitiated :)

Then,

On 04/20/15 11:19, Gerd Hoffmann wrote:
> route access to tseg into nowhere when enabled,
> for both cpus and busmaster dma.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/q35.c         | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci-host/q35.h |  1 +
>  2 files changed, 58 insertions(+)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index f0d840c..412ff0a 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -198,6 +198,28 @@ static const TypeInfo q35_host_info = {
>   * MCH D0:F0
>   */
>  
> +static uint64_t tseg_blackhole_read(void *ptr, hwaddr reg, unsigned size)
> +{
> +    return 0xffffffff;
> +}
> +
> +static void tseg_blackhole_write(void *opaque, hwaddr addr, uint64_t val,
> +                                 unsigned width)
> +{
> +    /* nothing */
> +}
> +
> +static const MemoryRegionOps tseg_blackhole_ops = {
> +    .read = tseg_blackhole_read,
> +    .write = tseg_blackhole_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .impl.min_access_size = 4,
> +    .impl.max_access_size = 4,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};

(a) you've specified .endianness twice (with inconsistent initializers
at that, although the value returned by the accessor happens to be
unaffected).

(b) Why are 8-byte accesses forbidden? ... Hm, actually, the above
doesn't forbid it, it just causes qemu to split up the access. Should be
fine.

> +
>  /* PCIe MMCFG */
>  static void mch_update_pciexbar(MCHPCIState *mch)
>  {
> @@ -267,6 +289,7 @@ static void mch_update_smram(MCHPCIState *mch)
>  {
>      PCIDevice *pd = PCI_DEVICE(mch);
>      bool h_smrame = (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME);
> +    uint32_t tseg_size;
>  
>      /* implement SMRAM.D_LCK */
>      if (pd->config[MCH_HOST_BRIDGE_SMRAM] & MCH_HOST_BRIDGE_SMRAM_D_LCK) {
> @@ -296,6 +319,32 @@ static void mch_update_smram(MCHPCIState *mch)
>          memory_region_set_enabled(&mch->high_smram, false);
>      }
>  
> +    if (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_T_EN) {
> +        switch (pd->config[MCH_HOST_BRIDGE_ESMRAMC] &
> +                MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK) {
> +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_1MB:
> +            tseg_size = 1024 * 1024;
> +            break;
> +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_2MB:
> +            tseg_size = 1024 * 1024 * 2;
> +            break;
> +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_8MB:
> +            tseg_size = 1024 * 1024 * 8;
> +            break;
> +        default:
> +            tseg_size = 0;
> +            break;
> +        }
> +    } else {
> +        tseg_size = 0;
> +    }

- so, is the guest supposed to select the tseg size by writing this
register?

- I assume this register is not reprogrammable once SMRAM is locked --
is that correct?

- can the guest somehow use this facility to detect, dynamically, the
presence of this feature? (For now I'm thinking about a static build
flag for OVMF that would enable SMM support, but I'm 99% sure Jordan
will object and ask for a dynamic feature detection.)

> +    memory_region_del_subregion(mch->system_memory, &mch->tseg_blackhole);

Hmm. I think this API is for something else. It is not for hole punching.

... Ah, okay, you *are* removing the previously configured blackhole
region (ie. you're not hole punching), I see. But, does this work if
tseg_blackhole is still all zeroes? (Ie. this is the first invocation.)

> +    memory_region_set_enabled(&mch->tseg_blackhole, tseg_size);
> +    memory_region_set_size(&mch->tseg_blackhole, tseg_size);
> +    memory_region_add_subregion_overlap(mch->system_memory,
> +                                        mch->below_4g_mem_size - tseg_size,
> +                                        &mch->tseg_blackhole, 1);

So, does tseg overlay the last few MB of the RAM below 4GB? (Ie. right
before the PCI hole starts?)

> +
>      memory_region_transaction_commit();
>  }
>  
> @@ -443,6 +492,14 @@ static void mch_realize(PCIDevice *d, Error **errp)
>      object_property_add_alias(qdev_get_machine(), "smram",
>                                OBJECT(&mch->smram), ".", NULL);
>  
> +    memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
> +                          &tseg_blackhole_ops, NULL,
> +                          "tseg-blackhole", 0);

Okay, this answers one of the above, tseg_blackhole is never uninitialized.

> +    memory_region_set_enabled(&mch->tseg_blackhole, false);
> +    memory_region_add_subregion_overlap(mch->system_memory,
> +                                        mch->below_4g_mem_size,
> +                                        &mch->tseg_blackhole, 1);

The address (2nd argument) is inconsistent with the one in
mch_update_smram() -- this function call places the tseg black hole at
the beginning of the 32-bit PCI hole, not just below it.

(OTOH it doesn't really matter, because the region is disabled. Still,
it would be easier to understand.)

... I'm thinking loud about what this means for the OVMF memory space
map... The central function we use in PlatformPei is
GetSystemMemorySizeBelow4gb(), which reads the CMOS (0x34/0x35). That
gives us "below_4g_mem_size" in OVMF.

Currently that value is used for three purposes:
(a) place the permanent PEI RAM so that it ends at the top of
"below_4g_mem_size"; PublishPeiMemory()
(b) create one of the memory HOBs, QemuInitializeRam(),
(c) create the MMIO HOB that describes the 32-bit PCI hole,
    MemMapInitialization()

If the last 1 / 2 / 8 MB of the low RAM can't be used as general purpose
RAM, then (a) is affected (it should simply be sunk by the tseg size of
choice); (b) is affected similarly (just decrease the top of the memory
HOB); (c) is not affected (the start of the low PCI hole remains
unchanged). Okay...

> +
>      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 61bfe7e..ba64c70 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -55,6 +55,7 @@ typedef struct MCHPCIState {
>      PAMMemoryRegion pam_regions[13];
>      MemoryRegion smram_region, open_high_smram;
>      MemoryRegion smram, low_smram, high_smram;
> +    MemoryRegion tseg_blackhole;
>      PcPciInfo pci_info;
>      ram_addr_t below_4g_mem_size;
>      ram_addr_t above_4g_mem_size;
> 

Thanks!
Laszlo