Re: [Qemu-devel] Modeling x86 early initialization accurately

[Avi Kivity <avi@redhat.com>]

Carl-Daniel Hailfinger wrote:
>> This is pretty reasonable.
>>     
>
> So this would be my first patch, together with a patch to change the
> allocation to read/write once a special MSR is written.
>
> Is it possible to change the type of allocation from readonly to
> read/write if the backing store has been allocated with qemu_ram_alloc()?
> Can I simply call cpu_register_physical_memory() again for the same
> target region and the newer register will take precedence?
> Is the "special MSR" solution acceptable? If yes, which number should I
> pick? Or is that my choice?
>
>   

Isn't this usually a chipset function?  In this case a chipset register 
is more appropriate.  Best would be to implement the actual chipset that 
qemu qemulates.

  

>> Yes, I saw this patch but since it's just debugging code, it's not
>> interesting for inclusion.
>>     
>
> Quite a few x86 processors reset themselves if they encounter an unknown
> MSR write. Should we do the same? If not, would spewing a loud debug
> message be appropriate?
>
>   

The standard behaviour is to #GP.

  

>> I'm concerned that modeling this could have a non negligible overhead
>> and could be very difficult in something like KVM.  Can you describe
>> exactly what coreboot is expecting that we are not implementing?  How
>> is it relying on cache locking?
>>     
>
> Since there is no RAM before RAM initialization, we have no way to keep
> a stack. That rules out implementing RAM init in C (which is fond of
> using a stack for local variables, parameters and call return addresses)
> unless you either can fake some RAM or have a C compiler which needs no
> stack. Faking some RAM is way easier.
> Basically, we use MTRRs to declare everything uncached except one small
> (4-64k sized with page granularity) area in the CPU address space which
> has cache type writeback. That area is called the CAR (Cache-as-RAM)
> area. Reads in that area will allocate a cache line and subsequent reads
> will hit the cache directly. Writes in that area will allocate a cache
> line if none already exists for the given address. Writes to the area
> will never be passed to RAM. Reads and writes outside the CAR area will
> go directly through to RAM/ROM. Writes outside the CAR area will be
> discarded. Since everything besides the CAR area is declared as uncached
> and any access outside the cache area won't cause cacheline evictions,
> the cache is effectively locked.
>
> From a firmware perspective, the following implementation is good enough:
> 1. CAR enable: Copy the contents of the address area designated for CAR
> from the underlying (readonly RAM/ROM) backing store to a new "CAR"
> read/write backing store mapped to the same CPU physical address area.
> 2. CAR usage: All reads/writes to the CAR area hit the "CAR" read/write
> backing store. All other reads outside the CAR area hit the normal
> backing store. All writes outside the CAR area are discarded if they
> would have ended up in RAM. Writes to MMIO regions are still honored.
> 3. RAM enabling: The backing store for RAM outside the CAR area now
> accepts writes.
> 3. CAR disabling: The "CAR" backing store is either discarded (INVD
> instruction) or written to RAM (WBINVD instruction).
>
> The runtime performance hit of this implementation should be negligible
> because there is no need to check for CAR on each memory access. Only
> the relevant MSR writes need to be handled to change allocation type.
> Once CAR is disabled, the memory allocation and mapping should match
> exactly what the current code does. That means any performance hit would
> only matter during the time CAR is active. That's probably a few hundred
> instructions after poweron until RAM is enabled.
>   

If we can detect this, we can handle it with kvm by allocating a memory 
slot to back the cache.  But I don't see how we can detect it reliably 
(mtrrs are handled completely within the kernel, and I wouldn't want 
this hack in the kernel).

-- 
error compiling committee.c: too many arguments to function