Re: [SeaBIOS] [PATCH v2 1/9] seabios: make mttr UC area setup dynamic

[Gerd Hoffmann]

On 10/09/12 05:35, Jason Baron wrote:
> From: Jason Baron <[e-mail]>
>
> Set up the UC area of mtrr dynamically based on mtrr_base. This allows
> the bios to work for other chipsets that might want to set the mtrr.

Nice catch, didn't notice the mtrr thing when making the pci window dynamic.
> +u64 mtrr_base = BUILD_PCIMEM_START;
> +
>  u64 pcimem_start   = BUILD_PCIMEM_START;

Any reason you add a new variable instead of just using pcimem_start
directly?

I guess when pcimem_start is updated mtrr_base should be updated too.
And we probably should make sure pcimem_start has a value mtrr can
handle.  Which pretty much implies pcimem_start can be one of
0x80000000, 0xc0000000 or 0xe000000 depending on the amout of memory the
guest has if we want stick to a single mtrr register.

cheers,
  Gerd

[PATCH v2 1/9] seabios: make mttr UC area setup dynamic

[Jason Baron]

On Tue, Oct 09, 2012 at 08:48:38AM +0200, Gerd Hoffmann wrote:
> On 10/09/12 05:35, Jason Baron wrote:
> > From: Jason Baron <[e-mail]>
> >
> > Set up the UC area of mtrr dynamically based on mtrr_base. This allows
> > the bios to work for other chipsets that might want to set the mtrr.
>
> Nice catch, didn't notice the mtrr thing when making the pci window dynamic.
> > +u64 mtrr_base = BUILD_PCIMEM_START;
> > +
> >  u64 pcimem_start   = BUILD_PCIMEM_START;
>
> Any reason you add a new variable instead of just using pcimem_start
> directly?

For the piix case. pcimem_start can be set to RamSize at the beginning
of pci_bios_map_devices(). And the mtrr uc region has always been at
0xe000000, so I needed two variables.

That raises a question to me as to whether the uc region is covering the
pci mem space if RamSize != 0xe000000?
> I guess when pcimem_start is updated mtrr_base should be updated too.
> And we probably should make sure pcimem_start has a value mtrr can
> handle.  Which pretty much implies pcimem_start can be one of
> 0x80000000, 0xc0000000 or 0xe000000 depending on the amout of memory the
> guest has if we want stick to a single mtrr register.

That's why I set mtrr_base on q35 to 0xc0000000 (in order to have a
single mtrr uc region). That said, it does not cover the mmconfig space
currently, which runs from 0xb0000000-0xc0000000. So, I'm not sure if
mmconfig should be covered by the UC region? If so, I can shift the
beginning of the mmconfig to 0xc0000000 (making the pci i/o space start
at 0xd0000000), or we can introduce a second mtrr region.

Thanks,

-Jason

[PATCH v2 1/9] seabios: make mttr UC area setup dynamic

[Gerd Hoffmann]

On 10/09/12 16:55, Jason Baron wrote:
> On Tue, Oct 09, 2012 at 08:48:38AM +0200, Gerd Hoffmann wrote:
> > On 10/09/12 05:35, Jason Baron wrote:
> > > From: Jason Baron <[e-mail]>
> > >
> > > Set up the UC area of mtrr dynamically based on mtrr_base. This allows
> > > the bios to work for other chipsets that might want to set the mtrr.
> >
> > Nice catch, didn't notice the mtrr thing when making the pci window dynamic.
> > > +u64 mtrr_base = BUILD_PCIMEM_START;
> > > +
> > >  u64 pcimem_start   = BUILD_PCIMEM_START;
> >
> > Any reason you add a new variable instead of just using pcimem_start
> > directly?
>
> For the piix case. pcimem_start can be set to RamSize at the beginning
> of pci_bios_map_devices(). And the mtrr uc region has always been at
> 0xe000000, so I needed two variables.
>
> That raises a question to me as to whether the uc region is covering the
> pci mem space if RamSize != 0xe000000?

It doesn't which is a bug (this is what I've tried to say with the "nice
catch" above).
> > I guess when pcimem_start is updated mtrr_base should be updated too.
> > And we probably should make sure pcimem_start has a value mtrr can
> > handle.  Which pretty much implies pcimem_start can be one of
> > 0x80000000, 0xc0000000 or 0xe000000 depending on the amout of memory the
> > guest has if we want stick to a single mtrr register.

[ note: the above refers to piix ]
> That's why I set mtrr_base on q35 to 0xc0000000 (in order to have a
> single mtrr uc region).

Yep.  You could likewise pick 0x80000000 or 0xc0000000 depending on
installed guest memory.  Would mean to make mmconfig location dynamic
too (at 0x70000000 / 0xb0000000), not sure how easy that is wrt
initialization ordering.  But it isn't that important long-term, with
the increating memory sizes for physical & virtual machines it will be
more and more rare that we can use 0x8000000+ for the pci window.
> That said, it does not cover the mmconfig space
> currently, which runs from 0xb0000000-0xc0000000. So, I'm not sure if
> mmconfig should be covered by the UC region?

Dunno.  On my real laptop it seems to be not covered.  But at the end of
the day this is virtual only so it is more or less a cosmetic issue.

cheers,
  Gerd