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

[Gerd Hoffmann <kraxel@redhat.com>]

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

Oops, cut+paste bug, will fix (/me wonders why gcc didn't throw an error
on that one).

> (b) Why are 8-byte accesses forbidden? ...

just a matter of setting .valid.max_access_size = 8, can do that of
course.

> > +
> >  /* 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?

Correct.

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

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

Hmm.  I think if we merge all the smm / smram / tseg patches in one go
this should work.  Without patches reading the ESMRAMC register returns
zero.  With the patches the three cache-disable bits are hardcoded to
'1'.  This should work for detecting tseg support.

You also have to test for smm support.  The current protocol for this is
that seabios checks whenever smm is already initialized (see
*_apmc_smm_setup() in seabios/src/fw/smm.c) and if so it skips smm
initialization.  Right now qemu sets the bit on reset when running on
kvm, so seabios doesn't try to use smm.  On tcg the bit is clear after
reset and seabios actually uses smm mode.

> > +    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?)

tseg is just normal ram (yes, located at the end of memory), but (once
tseg is enabled) only cpus in smm mode are allowed to access it.
Likewise busmaster dma access is rejected, so non-smm code can't use the
sata controller to access this indirectly.

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

It's a zero-length hole here, therefore it actually is the same.  But I
didn't bother to use "mch->below_4g_mem_size - 0" as start address to
make that more clear ;)

> (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);

Yes.

cheers,
  Gerd