Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Michael S. Tsirkin]

On Wed, Aug 07, 2013 at 09:29:39AM +0200, Gerd Hoffmann wrote:
> We'll need some way for seabios to fill in the pci window information
> into the qemu-provided tables.  Easiest way to do that would be to
> extend the COMMAND_ADD_POINTER bios linker script command.

This idea certainly has an advantage: the two patch-sets
(to control PCI hole from QEMU, and to pass ACPI tables
 from QEMU) would become independent.

One difficulty would be coming up with a sane interface
that's not tied to specific AML code: unlike table
pointers which have a specific fixed-width format, we are talking
about generic AML code here.  Patching that works (we do it today with
the ACPI_EXTRACT code) but requires that you code AML in a specific way,
for example, number encoding is variable-length so we pad values ahead
of the time making sure the AML encoding is large enough to hold the
maximum possible value.

Also, in the past both Anthony and Kevin indicated preference to
the pci-info solution that we have in QEMU today.

-- 
MST

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Michael S. Tsirkin]

On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
> >>   (3) mmconf xbar start (MCFG, q35 only, at 0xb0000000 now).
> >>   (4) pmbase (FADT, at 0xb000 now).
> >>
> >> Especially 3+4 tend to be compile-time constants in the firmware as they
> >> are needed very early in the setup process.
> > 
> > So we don't need them in pci-config, just stick constant in ACPI.
> 
> I don't want them be constant.  I want allow the firmware pick them.
> Our mmconfig xbar is 256M and can handle 256 busses.  I'd like to have
> the option to reduce that to 64M and place it somewhere else.
> 
> Also coreboot and seabios use different values for pmbase.  coreboot on
> q35 maps the pmbase below 0x1000.  Which surely makes sense.  When we
> don't place chipset stuff at 0xb000 we can assign the 0xb000->0xbfff
> window to a pci bridge instead.

Yes, this might be useful. But I don't think it's required to
use linker to patch ACPI tables for this - we can write ASL code to read
the register back from device configuration, instead.

Also, keep qemu-devel Cc'd on these discussions please.

-- 
MST

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Gerd Hoffmann]

On 08/08/13 10:37, Michael S. Tsirkin wrote:
> On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
>>>>   (3) mmconf xbar start (MCFG, q35 only, at 0xb0000000 now).
>>>>   (4) pmbase (FADT, at 0xb000 now).
>>>>
>>>> Especially 3+4 tend to be compile-time constants in the firmware as they
>>>> are needed very early in the setup process.
>>>
>>> So we don't need them in pci-config, just stick constant in ACPI.
>>
>> I don't want them be constant.  I want allow the firmware pick them.
>> Our mmconfig xbar is 256M and can handle 256 busses.  I'd like to have
>> the option to reduce that to 64M and place it somewhere else.
>>
>> Also coreboot and seabios use different values for pmbase.  coreboot on
>> q35 maps the pmbase below 0x1000.  Which surely makes sense.  When we
>> don't place chipset stuff at 0xb000 we can assign the 0xb000->0xbfff
>> window to a pci bridge instead.
> 
> Yes, this might be useful. But I don't think it's required to
> use linker to patch ACPI tables for this - we can write ASL code to read
> the register back from device configuration, instead.

No, we can't, because the address is in the FADT.

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Michael S. Tsirkin]

On Thu, Aug 08, 2013 at 10:57:44AM +0200, Gerd Hoffmann wrote:
> On 08/08/13 10:37, Michael S. Tsirkin wrote:
> > On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
> >>>>   (3) mmconf xbar start (MCFG, q35 only, at 0xb0000000 now).
> >>>>   (4) pmbase (FADT, at 0xb000 now).
> >>>>
> >>>> Especially 3+4 tend to be compile-time constants in the firmware as they
> >>>> are needed very early in the setup process.
> >>>
> >>> So we don't need them in pci-config, just stick constant in ACPI.
> >>
> >> I don't want them be constant.  I want allow the firmware pick them.
> >> Our mmconfig xbar is 256M and can handle 256 busses.  I'd like to have
> >> the option to reduce that to 64M and place it somewhere else.
> >>
> >> Also coreboot and seabios use different values for pmbase.  coreboot on
> >> q35 maps the pmbase below 0x1000.  Which surely makes sense.  When we
> >> don't place chipset stuff at 0xb000 we can assign the 0xb000->0xbfff
> >> window to a pci bridge instead.
> > 
> > Yes, this might be useful. But I don't think it's required to
> > use linker to patch ACPI tables for this - we can write ASL code to read
> > the register back from device configuration, instead.
> 
> No, we can't, because the address is in the FADT.
> 
> cheers,
>   Gerd

I see. Yes, PM base is there, this in fact makes it possible
to patch it by linker in a sane way.
But to make addresses usable to devices they also need to be declared in
the _CRS for the PCI root, correct? Which is code in DSDT.

-- 
MST

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Gerd Hoffmann]

On 08/08/13 11:52, Michael S. Tsirkin wrote:
> On Thu, Aug 08, 2013 at 10:57:44AM +0200, Gerd Hoffmann wrote:
>> On 08/08/13 10:37, Michael S. Tsirkin wrote:
>>> On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
>>>> Also coreboot and seabios use different values for pmbase.  coreboot on
>>>> q35 maps the pmbase below 0x1000.  Which surely makes sense.  When we
>>>> don't place chipset stuff at 0xb000 we can assign the 0xb000->0xbfff
>>>> window to a pci bridge instead.
>>>
>>> Yes, this might be useful. But I don't think it's required to
>>> use linker to patch ACPI tables for this - we can write ASL code to read
>>> the register back from device configuration, instead.
>>
>> No, we can't, because the address is in the FADT.
>>
>> cheers,
>>   Gerd
> 
> I see. Yes, PM base is there, this in fact makes it possible
> to patch it by linker in a sane way.

Exactly.  Likewise the mmconf xbar config in the MCFG table.

> But to make addresses usable to devices they also need to be declared in
> the _CRS for the PCI root, correct? Which is code in DSDT.

Yes, the address ranges used for pci devices (aka 32bit + 64bit pci
window) need to be there.  Well, placing in SSDT, then referencing from
DSDT works too, and this is what seabios does today to dynamically
adjust stuff.  Fixing up the SSDT using the linker is probably easier as
we generate it anyway.

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Michael S. Tsirkin]

On Thu, Aug 08, 2013 at 12:21:32PM +0200, Gerd Hoffmann wrote:
> On 08/08/13 11:52, Michael S. Tsirkin wrote:
> > On Thu, Aug 08, 2013 at 10:57:44AM +0200, Gerd Hoffmann wrote:
> >> On 08/08/13 10:37, Michael S. Tsirkin wrote:
> >>> On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
> >>>> Also coreboot and seabios use different values for pmbase.  coreboot on
> >>>> q35 maps the pmbase below 0x1000.  Which surely makes sense.  When we
> >>>> don't place chipset stuff at 0xb000 we can assign the 0xb000->0xbfff
> >>>> window to a pci bridge instead.

Re-reading this - if this has value, can't we generalize it
and make all firmware behave the same, getting values from QEMU?

> >>>
> >>> Yes, this might be useful. But I don't think it's required to
> >>> use linker to patch ACPI tables for this - we can write ASL code to read
> >>> the register back from device configuration, instead.
> >>
> >> No, we can't, because the address is in the FADT.
> >>
> >> cheers,
> >>   Gerd
> > 
> > I see. Yes, PM base is there, this in fact makes it possible
> > to patch it by linker in a sane way.
> 
> Exactly.  Likewise the mmconf xbar config in the MCFG table.
> > But to make addresses usable to devices they also need to be declared in
> > the _CRS for the PCI root, correct? Which is code in DSDT.
> 
> Yes, the address ranges used for pci devices (aka 32bit + 64bit pci
> window) need to be there.  Well, placing in SSDT, then referencing from
> DSDT works too, and this is what seabios does today to dynamically
> adjust stuff.  Fixing up the SSDT using the linker is probably easier as
> we generate it anyway.
> 
> cheers,
>   Gerd

Yes but as I said, this makes things messy, since AML encoding for
numbers isn't fixed width.

-- 
MST

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Gerd Hoffmann]

On 08/08/13 16:13, Michael S. Tsirkin wrote:
> On Thu, Aug 08, 2013 at 12:21:32PM +0200, Gerd Hoffmann wrote:
>> On 08/08/13 11:52, Michael S. Tsirkin wrote:
>>> On Thu, Aug 08, 2013 at 10:57:44AM +0200, Gerd Hoffmann wrote:
>>>> On 08/08/13 10:37, Michael S. Tsirkin wrote:
>>>>> On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
>>>>>> Also coreboot and seabios use different values for pmbase.  coreboot on
>>>>>> q35 maps the pmbase below 0x1000.  Which surely makes sense.  When we
>>>>>> don't place chipset stuff at 0xb000 we can assign the 0xb000->0xbfff
>>>>>> window to a pci bridge instead.
> 
> Re-reading this - if this has value, can't we generalize it
> and make all firmware behave the same, getting values from QEMU?

pmbase is a compile-time constant (aka #define) in both seabios and
coreboot, and making this runtime-configurable is non-trivial.  See
src/smm.c in seabios for one reason why.

Moving it to another fixed address below 0x1000 doesn't work, breaks
with old qemu+new seabios and other way around.

I think easiest is to allow firmware pick it and fixup the tables
provided by qemu accordingly.

Picking pmbase works in firmware today btw as both coreboot+seabios
generate a fadt with the correct values.  Only nit is that this depends
on qemu 1.4+ as piix4 pmbase register was read-only in older qemu
versions.  So everybody uses 0xb000 by default for bug compatibility
with older qemu versions.

>> Yes, the address ranges used for pci devices (aka 32bit + 64bit pci
>> window) need to be there.  Well, placing in SSDT, then referencing from
>> DSDT works too, and this is what seabios does today to dynamically
>> adjust stuff.  Fixing up the SSDT using the linker is probably easier as
>> we generate it anyway.
>>
>> cheers,
>>   Gerd
> 
> Yes but as I said, this makes things messy, since AML encoding for
> numbers isn't fixed width.

In seabios we have fixed 32bit / 64bit width today, from acpi.c:

    // store pci io windows
    *(u32*)&ssdt_ptr[acpi_pci32_start[0]] = cpu_to_le32(pcimem_start);
    *(u32*)&ssdt_ptr[acpi_pci32_end[0]] = cpu_to_le32(pcimem_end - 1);
    if (pcimem64_start) {
        ssdt_ptr[acpi_pci64_valid[0]] = 1;
        *(u64*)&ssdt_ptr[acpi_pci64_start[0]] = cpu_to_le64(pcimem64_start);
        *(u64*)&ssdt_ptr[acpi_pci64_end[0]] = cpu_to_le64(pcimem64_end - 1);
        *(u64*)&ssdt_ptr[acpi_pci64_length[0]] = cpu_to_le64(
            pcimem64_end - pcimem64_start);
    } else {
        ssdt_ptr[acpi_pci64_valid[0]] = 0;
    }

Storing fixup instructions for these fields in the linker script
shouldn't be hard I think.

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Kevin O'Connor]

On Thu, Aug 08, 2013 at 04:56:55PM +0200, Gerd Hoffmann wrote:
> On 08/08/13 16:13, Michael S. Tsirkin wrote:
> > On Thu, Aug 08, 2013 at 12:21:32PM +0200, Gerd Hoffmann wrote:
> >> On 08/08/13 11:52, Michael S. Tsirkin wrote:
> >>> On Thu, Aug 08, 2013 at 10:57:44AM +0200, Gerd Hoffmann wrote:
> >>>> On 08/08/13 10:37, Michael S. Tsirkin wrote:
> >>>>> On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
> >>>>>> Also coreboot and seabios use different values for pmbase.  coreboot on
> >>>>>> q35 maps the pmbase below 0x1000.  Which surely makes sense.  When we
> >>>>>> don't place chipset stuff at 0xb000 we can assign the 0xb000->0xbfff
> >>>>>> window to a pci bridge instead.
> > 
> > Re-reading this - if this has value, can't we generalize it
> > and make all firmware behave the same, getting values from QEMU?
> 
> pmbase is a compile-time constant (aka #define) in both seabios and
> coreboot, and making this runtime-configurable is non-trivial.  See
> src/smm.c in seabios for one reason why.

I don't think SeaBIOS should modify tables provided by QEMU.  That
leads to confusion on the source of the data and mixed
responsibilities which results in greater complexity in both QEMU and
SeaBIOS.

SeaBIOS doesn't have any info that QEMU doesn't have.  So, I think
it's safe for QEMU to be the sole authority for the table content.

Converting src/smm.c to use a runtime value isn't hard - just change
the assembler from: "mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04,
%dx\n" to: "mov 4(my_acpi_base), %dx\n" and make sure to define the
global variable my_acpi_base as VARFSEG.

> >> Yes, the address ranges used for pci devices (aka 32bit + 64bit pci
> >> window) need to be there.  Well, placing in SSDT, then referencing from
> >> DSDT works too, and this is what seabios does today to dynamically
> >> adjust stuff.  Fixing up the SSDT using the linker is probably easier as
> >> we generate it anyway.
> >>
> > Yes but as I said, this makes things messy, since AML encoding for
> > numbers isn't fixed width.
> 
> In seabios we have fixed 32bit / 64bit width today, from acpi.c:
> 
>     // store pci io windows
>     *(u32*)&ssdt_ptr[acpi_pci32_start[0]] = cpu_to_le32(pcimem_start);
>     *(u32*)&ssdt_ptr[acpi_pci32_end[0]] = cpu_to_le32(pcimem_end - 1);
>     if (pcimem64_start) {
>         ssdt_ptr[acpi_pci64_valid[0]] = 1;
>         *(u64*)&ssdt_ptr[acpi_pci64_start[0]] = cpu_to_le64(pcimem64_start);
>         *(u64*)&ssdt_ptr[acpi_pci64_end[0]] = cpu_to_le64(pcimem64_end - 1);
>         *(u64*)&ssdt_ptr[acpi_pci64_length[0]] = cpu_to_le64(
>             pcimem64_end - pcimem64_start);
>     } else {
>         ssdt_ptr[acpi_pci64_valid[0]] = 0;
>     }
> 
> Storing fixup instructions for these fields in the linker script
> shouldn't be hard I think.

I don't think SeaBIOS should continue to do the above once the tables
are moved to QEMU.  QEMU has all the info SeaBIOS has, so it can
generate the tables correctly on its own.

In all practical situations, the PCI window should be at least an
order of magnatude greater than the sum of the PCI bars in the system.
If the bars are actually bigger than the window, then things are going
to fail - the best the firmware can do is try to fail gracefully.  I
don't think it's worth the complexity to design mixed ownership and
advanced interfaces just so we can fail slightly better.

If this is a real worry, QEMU can sum all the PCI bars and warn the
user if they don't fit.

-Kevin

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Gerd Hoffmann]

   Hi,

>> pmbase is a compile-time constant (aka #define) in both seabios and
>> coreboot, and making this runtime-configurable is non-trivial.  See
>> src/smm.c in seabios for one reason why.

> Converting src/smm.c to use a runtime value isn't hard - just change
> the assembler from: "mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04,
> %dx\n" to: "mov 4(my_acpi_base), %dx\n" and make sure to define the
> global variable my_acpi_base as VARFSEG.

Ah, good, I give that a try.  Need to check how that works out for
coreboot though.

That leaves the mmconf xbar location.  We can continue to have everybody
agree this should be mapped @ 0xb0000000 and be done with it.  Making
this configurable via fw_cfg is no problem for seabios.  coreboot can't
deal with it though, it sets up the xbar _very_ early because it does
the complete pci setup via mmconf.

>> In seabios we have fixed 32bit / 64bit width today, from acpi.c:
>>
>>     // store pci io windows
>>     *(u32*)&ssdt_ptr[acpi_pci32_start[0]] = cpu_to_le32(pcimem_start);
>>     *(u32*)&ssdt_ptr[acpi_pci32_end[0]] = cpu_to_le32(pcimem_end - 1);
>>     if (pcimem64_start) {
>>         ssdt_ptr[acpi_pci64_valid[0]] = 1;
>>         *(u64*)&ssdt_ptr[acpi_pci64_start[0]] = cpu_to_le64(pcimem64_start);
>>         *(u64*)&ssdt_ptr[acpi_pci64_end[0]] = cpu_to_le64(pcimem64_end - 1);
>>         *(u64*)&ssdt_ptr[acpi_pci64_length[0]] = cpu_to_le64(
>>             pcimem64_end - pcimem64_start);
>>     } else {
>>         ssdt_ptr[acpi_pci64_valid[0]] = 0;
>>     }
>>
>> Storing fixup instructions for these fields in the linker script
>> shouldn't be hard I think.
> 
> I don't think SeaBIOS should continue to do the above once the tables
> are moved to QEMU.  QEMU has all the info SeaBIOS has, so it can
> generate the tables correctly on its own.

The loader script provided by qemu has fixup instructions, which is
needed to fixup pointers to other acpi tables.  The idea is to use that
mechanism to also allow th firmware to fixup addresses like pmbase in
the qemu-generated tables.

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Kevin O'Connor]

On Fri, Aug 09, 2013 at 08:25:00AM +0200, Gerd Hoffmann wrote:
> > I don't think SeaBIOS should continue to do the above once the tables
> > are moved to QEMU.  QEMU has all the info SeaBIOS has, so it can
> > generate the tables correctly on its own.
> 
> The loader script provided by qemu has fixup instructions, which is
> needed to fixup pointers to other acpi tables.  The idea is to use that
> mechanism to also allow th firmware to fixup addresses like pmbase in
> the qemu-generated tables.

Yes, but why should QEMU tell SeaBIOS to modify the table for pmbase
when it can just modify the table itself?

-Kevin

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Gerd Hoffmann]

On 08/10/13 05:10, Kevin O'Connor wrote:
> On Fri, Aug 09, 2013 at 08:25:00AM +0200, Gerd Hoffmann wrote:
>>> I don't think SeaBIOS should continue to do the above once the tables
>>> are moved to QEMU.  QEMU has all the info SeaBIOS has, so it can
>>> generate the tables correctly on its own.
>>
>> The loader script provided by qemu has fixup instructions, which is
>> needed to fixup pointers to other acpi tables.  The idea is to use that
>> mechanism to also allow th firmware to fixup addresses like pmbase in
>> the qemu-generated tables.
> 
> Yes, but why should QEMU tell SeaBIOS to modify the table for pmbase
> when it can just modify the table itself?

We'll need some way to make sure the pmbase (also mmconf xbar) set by
the firmware matches the pmbase address filled into the acpi tables by
qemu ...

So the options we have are:

  (1) Hardcode the address everywhere.  This is pretty close to the
      current state, 0xb000 is hard-coded pretty much everywhere,
      basically because older qemu versions had the pmbase register
      readonly with 0xb000.  I'd like to move the pmbase somewhere else
      long-term, to free the 0xb000-0xbfff window, so I'd like to avoid
      that.

  (2) Have qemu pick pmbase/xbar addr.  Doesn't work due to
      initialization order issues (especially xbar for coreboot).

  (3) Have firmware pick pmbase/xbar, have fixup instructions for the
      addresses in in the loader script, simliar to the fixup
      instructions for table-to-table pointers.

  (4) [ new idea by mst ]  Have firmware pick pmbase/xbar, then have
      qemu look at the hardware registers programmed by the firmware,
      use pmbase/xbar addresses found there there when generating the
      tables.

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Kevin O'Connor]

On Mon, Aug 12, 2013 at 08:05:08AM +0200, Gerd Hoffmann wrote:
> We'll need some way to make sure the pmbase (also mmconf xbar) set by
> the firmware matches the pmbase address filled into the acpi tables by
> qemu ...
> 
> So the options we have are:
> 
>   (1) Hardcode the address everywhere.  This is pretty close to the
>       current state, 0xb000 is hard-coded pretty much everywhere,
>       basically because older qemu versions had the pmbase register
>       readonly with 0xb000.  I'd like to move the pmbase somewhere else
>       long-term, to free the 0xb000-0xbfff window, so I'd like to avoid
>       that.
> 
>   (2) Have qemu pick pmbase/xbar addr.  Doesn't work due to
>       initialization order issues (especially xbar for coreboot).
> 
>   (3) Have firmware pick pmbase/xbar, have fixup instructions for the
>       addresses in in the loader script, simliar to the fixup
>       instructions for table-to-table pointers.
> 
>   (4) [ new idea by mst ]  Have firmware pick pmbase/xbar, then have
>       qemu look at the hardware registers programmed by the firmware,
>       use pmbase/xbar addresses found there there when generating the
>       tables.

I don't much like option 3 or 4.

Although hardcoding (option 1) is ugly, I think that ugliness does not
justify the complexity of run-time patching (3/4).

As for option 2 - I don't see why coreboot couldn't read the values
out of fw_cfg early on for the handful of cases like this.

-Kevin

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Gerd Hoffmann]

On Mo, 2013-08-12 at 18:42 -0400, Kevin O'Connor wrote:
> On Mon, Aug 12, 2013 at 08:05:08AM +0200, Gerd Hoffmann wrote:
> > We'll need some way to make sure the pmbase (also mmconf xbar) set by
> > the firmware matches the pmbase address filled into the acpi tables by
> > qemu ...
> > 
> > So the options we have are:
> > 
> >   (1) Hardcode the address everywhere.  This is pretty close to the
> >       current state, 0xb000 is hard-coded pretty much everywhere,
> >       basically because older qemu versions had the pmbase register
> >       readonly with 0xb000.  I'd like to move the pmbase somewhere else
> >       long-term, to free the 0xb000-0xbfff window, so I'd like to avoid
> >       that.
> > 
> >   (2) Have qemu pick pmbase/xbar addr.  Doesn't work due to
> >       initialization order issues (especially xbar for coreboot).
> > 
> >   (3) Have firmware pick pmbase/xbar, have fixup instructions for the
> >       addresses in in the loader script, simliar to the fixup
> >       instructions for table-to-table pointers.
> > 
> >   (4) [ new idea by mst ]  Have firmware pick pmbase/xbar, then have
> >       qemu look at the hardware registers programmed by the firmware,
> >       use pmbase/xbar addresses found there there when generating the
> >       tables.
> 
> I don't much like option 3 or 4.
> 
> Although hardcoding (option 1) is ugly, I think that ugliness does not
> justify the complexity of run-time patching (3/4).

Maybe this wasn't clear, but in (4) the table is generated by *qemu*
with the values programmed by the firmware.

> As for option 2 - I don't see why coreboot couldn't read the values
> out of fw_cfg early on for the handful of cases like this.

Because both mmconf xbar and pmbase are special:

The mmconf xbar is setup as one of the first things coreboot does, even
before romstage, then coreboot does the complete pci initialization
using mmconf.

pmbase handling depends on southbridge/mainboard code, but it tends to
be setup early (in romstage) too.  On some boards ram detection needs to
fiddle with pmbase registers.

Setting pmbase at runtime looks doable though, even though we might have
to go for a temporary location for the pmbase in romstage, then move it
to the final place requested by qemu later.  But it needs some
qemu-specific tweaks in shared southbridge code though as picking pmbase
at runtime isn't something which happens on real hardware.  I'd like to
avoid that if possible.

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Kevin O'Connor]

On Tue, Aug 13, 2013 at 08:49:21AM +0200, Gerd Hoffmann wrote:
> On Mo, 2013-08-12 at 18:42 -0400, Kevin O'Connor wrote:
> > On Mon, Aug 12, 2013 at 08:05:08AM +0200, Gerd Hoffmann wrote:
> > > We'll need some way to make sure the pmbase (also mmconf xbar) set by
> > > the firmware matches the pmbase address filled into the acpi tables by
> > > qemu ...
> > > 
> > > So the options we have are:
> > > 
> > >   (1) Hardcode the address everywhere.  This is pretty close to the
> > >       current state, 0xb000 is hard-coded pretty much everywhere,
> > >       basically because older qemu versions had the pmbase register
> > >       readonly with 0xb000.  I'd like to move the pmbase somewhere else
> > >       long-term, to free the 0xb000-0xbfff window, so I'd like to avoid
> > >       that.
> > > 
> > >   (2) Have qemu pick pmbase/xbar addr.  Doesn't work due to
> > >       initialization order issues (especially xbar for coreboot).
> > > 
> > >   (3) Have firmware pick pmbase/xbar, have fixup instructions for the
> > >       addresses in in the loader script, simliar to the fixup
> > >       instructions for table-to-table pointers.
> > > 
> > >   (4) [ new idea by mst ]  Have firmware pick pmbase/xbar, then have
> > >       qemu look at the hardware registers programmed by the firmware,
> > >       use pmbase/xbar addresses found there there when generating the
> > >       tables.
> > 
> > I don't much like option 3 or 4.
> > 
> > Although hardcoding (option 1) is ugly, I think that ugliness does not
> > justify the complexity of run-time patching (3/4).
> 
> Maybe this wasn't clear, but in (4) the table is generated by *qemu*
> with the values programmed by the firmware.

Yes.  I still don't much like it.  I'd think it would be much simpler
for qemu to generate the tables once at startup and not have to patch
them at runtime.  It also introduces an obscure dependency on the
ordering of the firmware.

> > As for option 2 - I don't see why coreboot couldn't read the values
> > out of fw_cfg early on for the handful of cases like this.
> 
> Because both mmconf xbar and pmbase are special:
> 
> The mmconf xbar is setup as one of the first things coreboot does, even
> before romstage, then coreboot does the complete pci initialization
> using mmconf.

It's not hard to read a value from fw_cfg early on (even in
assembler):

outw(FWCFG_MY_EARLY_PORT, PORT_QEMU_CFG_CTL);
u8 myval = inb(PORT_QEMU_CFG_DATA);

Also, if this needs to be determined before the ram controller is
initialized, then I think it's fine to hard code the value (real
machines will almost assuredly hardcode as well).

-Kevin

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Gerd Hoffmann]

> > Maybe this wasn't clear, but in (4) the table is generated by *qemu*
> > with the values programmed by the firmware.
> 
> Yes.  I still don't much like it.  I'd think it would be much simpler
> for qemu to generate the tables once at startup and not have to patch
> them at runtime.  It also introduces an obscure dependency on the
> ordering of the firmware.

The ordering isn't a big problem I think as it fits the normal order how
the firmware boots up: hardware initialization goes first, generating
the tables for the guest comes later on.

> > The mmconf xbar is setup as one of the first things coreboot does, even
> > before romstage, then coreboot does the complete pci initialization
> > using mmconf.
> 
> It's not hard to read a value from fw_cfg early on (even in
> assembler):
> 
> outw(FWCFG_MY_EARLY_PORT, PORT_QEMU_CFG_CTL);
> u8 myval = inb(PORT_QEMU_CFG_DATA);

Well, only if the stuff isn't in a fw_cfg file, which happens to be the
case for all new stuff we are adding.  Also I'm not sure I can easily
make the xbar location a runtime variable in coreboot.

> Also, if this needs to be determined before the ram controller is
> initialized, then I think it's fine to hard code the value (real
> machines will almost assuredly hardcode as well).

Yea, real machines hardcode that.  As mentioned for coreboot this is a
compile-time constant (aka #define), which makes sense of course.  With
firmware generating the acpi tables this works fine, it just uses the
#defined value when writing the tables.

With qemu generating the acpi tables we can hardcode it to the same
value in both firmware and qemu.  Which pretty much implies we can never
ever change it in the future without breaking stuff.  Thats why I'd like
avoid that in the first place.

Making qemu lookup the values in the hardware registers, after the
firmware programmed them, has the big advantage that we don't need any
new paravirtual interfaces to have firmware and qemu agree on the
values ...

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

  Hi,

> Converting src/smm.c to use a runtime value isn't hard - just change
> the assembler from: "mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04,
> %dx\n" to: "mov 4(my_acpi_base), %dx\n" and make sure to define the
> global variable my_acpi_base as VARFSEG.

The apm fix brought a ctl register variable we can use directly, so I
tried the attached patch, then got this:

  Linking out/rom.o
out/code32flat.o: In function `smm_relocation_end':
(.text.asm./home/kraxel/projects/seabios/src/smm.c.72+0x37): relocation
truncated to fit: R_386_16 against symbol `acpi_pm1a_cnt' defined in
.data.varfseg./home/kraxel/projects/seabios/src/acpi.c.21 section in
out/code32flat.o
out/code32flat.o: In function `smm_relocation_end':
(.text.asm./home/kraxel/projects/seabios/src/smm.c.72+0x46): relocation
truncated to fit: R_386_16 against symbol `acpi_pm1a_cnt' defined in
.data.varfseg./home/kraxel/projects/seabios/src/acpi.c.21 section in
out/code32flat.o
make: *** [out/rom.o] Error 1

cheers,
  Gerd


[-- Attachment #2: 0001-wip-make-pmbase-runtime.patch --]
[-- Type: text/plain, Size: 1689 bytes --]

From 2d3cf0af70727664c0ab5f17dae99b9f3043b631 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 9 Aug 2013 11:43:51 +0200
Subject: [PATCH] [wip] make pmbase runtime

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 src/acpi.c |    2 +-
 src/acpi.h |    2 +-
 src/smm.c  |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/acpi.c b/src/acpi.c
index 8db1ed4..db33595 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -18,7 +18,7 @@
 
 #include "acpi-dsdt.hex"
 
-u32 acpi_pm1a_cnt VARFSEG;
+u16 acpi_pm1a_cnt VARFSEG;
 
 static void
 build_header(struct acpi_table_header *h, u32 sig, int len, u8 rev)
diff --git a/src/acpi.h b/src/acpi.h
index f0d24d4..5c478a1 100644
--- a/src/acpi.h
+++ b/src/acpi.h
@@ -36,7 +36,7 @@ struct rsdp_descriptor {        /* Root System Descriptor Pointer */
 };
 
 extern struct rsdp_descriptor *RsdpAddr;
-extern u32 acpi_pm1a_cnt;
+extern u16 acpi_pm1a_cnt;
 
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */
diff --git a/src/smm.c b/src/smm.c
index a424a29..a788a82 100644
--- a/src/smm.c
+++ b/src/smm.c
@@ -48,7 +48,7 @@ ASM32FLAT(
     "  jne 1f\n"
 
     /* ACPI disable */
-    "  mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
+    "  mov (acpi_pm1a_cnt), %dx\n" /* PMCNTRL */
     "  inw %dx, %ax\n"
     "  andw $~1, %ax\n"
     "  outw %ax, %dx\n"
@@ -60,7 +60,7 @@ ASM32FLAT(
     "  jne 2f\n"
 
     /* ACPI enable */
-    "  mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
+    "  mov (acpi_pm1a_cnt), %dx\n" /* PMCNTRL */
     "  inw %dx, %ax\n"
     "  orw $1, %ax\n"
     "  outw %ax, %dx\n"
-- 
1.7.9.7

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Kevin O'Connor]

On Fri, Aug 09, 2013 at 11:45:59AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > Converting src/smm.c to use a runtime value isn't hard - just change
> > the assembler from: "mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04,
> > %dx\n" to: "mov 4(my_acpi_base), %dx\n" and make sure to define the
> > global variable my_acpi_base as VARFSEG.
> 
> The apm fix brought a ctl register variable we can use directly, so I
> tried the attached patch, then got this:
> 
>   Linking out/rom.o
> out/code32flat.o: In function `smm_relocation_end':
> (.text.asm./home/kraxel/projects/seabios/src/smm.c.72+0x37): relocation
> truncated to fit: R_386_16 against symbol `acpi_pm1a_cnt' defined in
> .data.varfseg./home/kraxel/projects/seabios/src/acpi.c.21 section in
> out/code32flat.o
> out/code32flat.o: In function `smm_relocation_end':
> (.text.asm./home/kraxel/projects/seabios/src/smm.c.72+0x46): relocation
> truncated to fit: R_386_16 against symbol `acpi_pm1a_cnt' defined in
> .data.varfseg./home/kraxel/projects/seabios/src/acpi.c.21 section in
> out/code32flat.o
> make: *** [out/rom.o] Error 1

Use "addr32 movw (acpi_pm1a_cnt), %dx" instead of "mov
(acpi_pm1a_cnt), %dx".

That said, having the smi handler read from the f-segment probably
isn't the best thing to do (though it should work).  Better would be
to make a variable between smm_code_start/end and copy the value in as
part of the smm init, though admittedly that is a bit more
complicated.

-Kevin

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Kevin O'Connor]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

On Fri, Aug 09, 2013 at 11:30:14PM -0400, Kevin O'Connor wrote:
> On Fri, Aug 09, 2013 at 11:45:59AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > Converting src/smm.c to use a runtime value isn't hard - just change
> > > the assembler from: "mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04,
> > > %dx\n" to: "mov 4(my_acpi_base), %dx\n" and make sure to define the
> > > global variable my_acpi_base as VARFSEG.
> > 
> > The apm fix brought a ctl register variable we can use directly, so I
> > tried the attached patch, then got this:
> > 
> >   Linking out/rom.o
> > out/code32flat.o: In function `smm_relocation_end':
> > (.text.asm./home/kraxel/projects/seabios/src/smm.c.72+0x37): relocation
> > truncated to fit: R_386_16 against symbol `acpi_pm1a_cnt' defined in
> > .data.varfseg./home/kraxel/projects/seabios/src/acpi.c.21 section in
> > out/code32flat.o
> > out/code32flat.o: In function `smm_relocation_end':
> > (.text.asm./home/kraxel/projects/seabios/src/smm.c.72+0x46): relocation
> > truncated to fit: R_386_16 against symbol `acpi_pm1a_cnt' defined in
> > .data.varfseg./home/kraxel/projects/seabios/src/acpi.c.21 section in
> > out/code32flat.o
> > make: *** [out/rom.o] Error 1
> 
> Use "addr32 movw (acpi_pm1a_cnt), %dx" instead of "mov
> (acpi_pm1a_cnt), %dx".

I ran a quick test and the two attached patches seem to work okay.

-Kevin

[-- Attachment #2: 0001-Minor-cleanups-to-smm-assembler.patch --]
[-- Type: text/plain, Size: 2907 bytes --]

>From 8799c75ad8f1c4b7573f5117dbf695f4b98a9131 Mon Sep 17 00:00:00 2001
From: Kevin O'Connor <kevin@koconnor.net>
Date: Sat, 10 Aug 2013 11:39:14 -0400
Subject: [PATCH 1/2] Minor cleanups to smm assembler.
To: seabios@seabios.org

Use size prefixes on assembler instructions.

Split the relocation smm handler into a separate section from the main
runtime smm handler.

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
---
 src/smm.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/src/smm.c b/src/smm.c
index a424a29..d8473fd 100644
--- a/src/smm.c
+++ b/src/smm.c
@@ -16,31 +16,35 @@
 ASM32FLAT(
     ".global smm_relocation_start\n"
     ".global smm_relocation_end\n"
-    ".global smm_code_start\n"
-    ".global smm_code_end\n"
-    "  .code16\n"
+    "  .code16gcc\n"
 
     /* code to relocate SMBASE to 0xa0000 */
     "smm_relocation_start:\n"
-    "  mov $" __stringify(BUILD_SMM_INIT_ADDR) " + 0x7efc, %ebx\n"
-    "  addr32 mov (%ebx), %al\n"  /* revision ID to see if x86_64 or x86 */
-    "  cmp $0x64, %al\n"
+    "  movl $" __stringify(BUILD_SMM_INIT_ADDR) " + 0x7efc, %ebx\n"
+    "  addr32 movb (%ebx), %al\n"  /* revision ID to see if x86_64 or x86 */
+    "  cmpb $0x64, %al\n"
     "  je 1f\n"
-    "  mov $" __stringify(BUILD_SMM_INIT_ADDR) " + 0x7ef8, %ebx\n"
+    "  movl $" __stringify(BUILD_SMM_INIT_ADDR) " + 0x7ef8, %ebx\n"
     "  jmp 2f\n"
     "1:\n"
-    "  mov $" __stringify(BUILD_SMM_INIT_ADDR) " + 0x7f00, %ebx\n"
+    "  movl $" __stringify(BUILD_SMM_INIT_ADDR) " + 0x7f00, %ebx\n"
     "2:\n"
     "  movl $" __stringify(BUILD_SMM_ADDR) " - 0x8000, %eax\n"
     "  addr32 movl %eax, (%ebx)\n"
     /* indicate to the BIOS that the SMM code was executed */
-    "  mov $0x00, %al\n"
+    "  movb $0x00, %al\n"
     "  movw $" __stringify(PORT_SMI_STATUS) ", %dx\n"
     "  outb %al, %dx\n"
     "  rsm\n"
     "smm_relocation_end:\n"
+    "  .code32\n"
+    );
 
+ASM32FLAT(
     /* minimal SMM code to enable or disable ACPI */
+    ".global smm_code_start\n"
+    ".global smm_code_end\n"
+    "  .code16gcc\n"
     "smm_code_start:\n"
     "  movw $" __stringify(PORT_SMI_CMD) ", %dx\n"
     "  inb %dx, %al\n"
@@ -48,7 +52,7 @@ ASM32FLAT(
     "  jne 1f\n"
 
     /* ACPI disable */
-    "  mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
+    "  movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
     "  inw %dx, %ax\n"
     "  andw $~1, %ax\n"
     "  outw %ax, %dx\n"
@@ -56,11 +60,11 @@ ASM32FLAT(
     "  jmp 2f\n"
 
     "1:\n"
-    "  cmp $0xf1, %al\n"
+    "  cmpb $0xf1, %al\n"
     "  jne 2f\n"
 
     /* ACPI enable */
-    "  mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
+    "  movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
     "  inw %dx, %ax\n"
     "  orw $1, %ax\n"
     "  outw %ax, %dx\n"
-- 
1.7.11.7


[-- Attachment #3: 0002-make-pmbase-runtime.patch --]
[-- Type: text/plain, Size: 1725 bytes --]

>From 8a1e7b8f698a3e459d61c489feae602ed9bc83fc Mon Sep 17 00:00:00 2001
From: Kevin O'Connor <kevin@koconnor.net>
Date: Sat, 10 Aug 2013 11:48:00 -0400
Subject: [PATCH 2/2] make pmbase runtime
To: seabios@seabios.org

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 src/acpi.c | 2 +-
 src/acpi.h | 2 +-
 src/smm.c  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/acpi.c b/src/acpi.c
index 8db1ed4..db33595 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -18,7 +18,7 @@
 
 #include "acpi-dsdt.hex"
 
-u32 acpi_pm1a_cnt VARFSEG;
+u16 acpi_pm1a_cnt VARFSEG;
 
 static void
 build_header(struct acpi_table_header *h, u32 sig, int len, u8 rev)
diff --git a/src/acpi.h b/src/acpi.h
index f0d24d4..5c478a1 100644
--- a/src/acpi.h
+++ b/src/acpi.h
@@ -36,7 +36,7 @@ struct rsdp_descriptor {        /* Root System Descriptor Pointer */
 };
 
 extern struct rsdp_descriptor *RsdpAddr;
-extern u32 acpi_pm1a_cnt;
+extern u16 acpi_pm1a_cnt;
 
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */
diff --git a/src/smm.c b/src/smm.c
index d8473fd..b06107e 100644
--- a/src/smm.c
+++ b/src/smm.c
@@ -52,7 +52,7 @@ ASM32FLAT(
     "  jne 1f\n"
 
     /* ACPI disable */
-    "  movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
+    "  addr32 movw (acpi_pm1a_cnt), %dx\n" /* PMCNTRL */
     "  inw %dx, %ax\n"
     "  andw $~1, %ax\n"
     "  outw %ax, %dx\n"
@@ -64,7 +64,7 @@ ASM32FLAT(
     "  jne 2f\n"
 
     /* ACPI enable */
-    "  movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
+    "  addr32 movw (acpi_pm1a_cnt), %dx\n" /* PMCNTRL */
     "  inw %dx, %ax\n"
     "  orw $1, %ax\n"
     "  outw %ax, %dx\n"
-- 
1.7.11.7

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Michael S. Tsirkin]

On Fri, Aug 09, 2013 at 12:13:06AM -0400, Kevin O'Connor wrote:
> On Thu, Aug 08, 2013 at 04:56:55PM +0200, Gerd Hoffmann wrote:
> > On 08/08/13 16:13, Michael S. Tsirkin wrote:
> > > On Thu, Aug 08, 2013 at 12:21:32PM +0200, Gerd Hoffmann wrote:
> > >> On 08/08/13 11:52, Michael S. Tsirkin wrote:
> > >>> On Thu, Aug 08, 2013 at 10:57:44AM +0200, Gerd Hoffmann wrote:
> > >>>> On 08/08/13 10:37, Michael S. Tsirkin wrote:
> > >>>>> On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
> > >>>>>> Also coreboot and seabios use different values for pmbase.  coreboot on
> > >>>>>> q35 maps the pmbase below 0x1000.  Which surely makes sense.  When we
> > >>>>>> don't place chipset stuff at 0xb000 we can assign the 0xb000->0xbfff
> > >>>>>> window to a pci bridge instead.
> > > 
> > > Re-reading this - if this has value, can't we generalize it
> > > and make all firmware behave the same, getting values from QEMU?
> > 
> > pmbase is a compile-time constant (aka #define) in both seabios and
> > coreboot, and making this runtime-configurable is non-trivial.  See
> > src/smm.c in seabios for one reason why.
> 
> I don't think SeaBIOS should modify tables provided by QEMU.  That
> leads to confusion on the source of the data and mixed
> responsibilities which results in greater complexity in both QEMU and
> SeaBIOS.
> 
> SeaBIOS doesn't have any info that QEMU doesn't have.  So, I think
> it's safe for QEMU to be the sole authority for the table content.
> 
> Converting src/smm.c to use a runtime value isn't hard - just change
> the assembler from: "mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04,
> %dx\n" to: "mov 4(my_acpi_base), %dx\n" and make sure to define the
> global variable my_acpi_base as VARFSEG.
> 
> > >> Yes, the address ranges used for pci devices (aka 32bit + 64bit pci
> > >> window) need to be there.  Well, placing in SSDT, then referencing from
> > >> DSDT works too, and this is what seabios does today to dynamically
> > >> adjust stuff.  Fixing up the SSDT using the linker is probably easier as
> > >> we generate it anyway.
> > >>
> > > Yes but as I said, this makes things messy, since AML encoding for
> > > numbers isn't fixed width.
> > 
> > In seabios we have fixed 32bit / 64bit width today, from acpi.c:
> > 
> >     // store pci io windows
> >     *(u32*)&ssdt_ptr[acpi_pci32_start[0]] = cpu_to_le32(pcimem_start);
> >     *(u32*)&ssdt_ptr[acpi_pci32_end[0]] = cpu_to_le32(pcimem_end - 1);
> >     if (pcimem64_start) {
> >         ssdt_ptr[acpi_pci64_valid[0]] = 1;
> >         *(u64*)&ssdt_ptr[acpi_pci64_start[0]] = cpu_to_le64(pcimem64_start);
> >         *(u64*)&ssdt_ptr[acpi_pci64_end[0]] = cpu_to_le64(pcimem64_end - 1);
> >         *(u64*)&ssdt_ptr[acpi_pci64_length[0]] = cpu_to_le64(
> >             pcimem64_end - pcimem64_start);
> >     } else {
> >         ssdt_ptr[acpi_pci64_valid[0]] = 0;
> >     }
> > 
> > Storing fixup instructions for these fields in the linker script
> > shouldn't be hard I think.
> 
> I don't think SeaBIOS should continue to do the above once the tables
> are moved to QEMU.  QEMU has all the info SeaBIOS has, so it can
> generate the tables correctly on its own.
> 
> In all practical situations, the PCI window should be at least an
> order of magnatude greater than the sum of the PCI bars in the system.
> If the bars are actually bigger than the window, then things are going
> to fail - the best the firmware can do is try to fail gracefully.  I
> don't think it's worth the complexity to design mixed ownership and
> advanced interfaces just so we can fail slightly better.
> 
> If this is a real worry, QEMU can sum all the PCI bars and warn the
> user if they don't fit.
> 
> -Kevin

If we make it a rule that PCI is`setup before ACPI tables
are read, then QEMU can do the patching itself when
it detects BIOS reading the tables.

Gerd, Laszlo,others,  does this rule work for alternative firmwares?

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Kevin O'Connor]

On Fri, Aug 09, 2013 at 06:49:18PM +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 09, 2013 at 12:13:06AM -0400, Kevin O'Connor wrote:
> > I don't think SeaBIOS should continue to do the above once the tables
> > are moved to QEMU.  QEMU has all the info SeaBIOS has, so it can
> > generate the tables correctly on its own.
> > 
> > In all practical situations, the PCI window should be at least an
> > order of magnatude greater than the sum of the PCI bars in the system.
> > If the bars are actually bigger than the window, then things are going
> > to fail - the best the firmware can do is try to fail gracefully.  I
> > don't think it's worth the complexity to design mixed ownership and
> > advanced interfaces just so we can fail slightly better.
> > 
> > If this is a real worry, QEMU can sum all the PCI bars and warn the
> > user if they don't fit.
> 
> If we make it a rule that PCI is`setup before ACPI tables
> are read, then QEMU can do the patching itself when
> it detects BIOS reading the tables.

QEMU can do the patching regardless of the order of initialization.
The PCI bar sizes are static - they have no relationship to the timing
of the BIOS PCI initialization.

-Kevin

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Gerd Hoffmann]

  Hi,

> If we make it a rule that PCI is`setup before ACPI tables
> are read, then QEMU can do the patching itself when
> it detects BIOS reading the tables.

Approach makes sense to me.  The ordering constrain shouldn't be a big
burden, hardware detection+bringup (including pci setup) is the first
thing done by the firmware, loading/generating acpi tables is one of the
last things.  And it avoids the need to communicate the addresses (or
patch locations) between qemu+firmware.

What do you want to use this for?  pmbase and xbar are simple, they are
just a single register read.  pci io windows needs a root bus scan, but
should be doable too.

> Gerd, Laszlo,others,  does this rule work for alternative firmwares?

It surely works for coreboot, and I would be very surprised if this
causes trouble for ovmf.

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Michael S. Tsirkin]

On Mon, Aug 12, 2013 at 08:37:00AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > If we make it a rule that PCI is`setup before ACPI tables
> > are read, then QEMU can do the patching itself when
> > it detects BIOS reading the tables.
> 
> Approach makes sense to me.  The ordering constrain shouldn't be a big
> burden, hardware detection+bringup (including pci setup) is the first
> thing done by the firmware, loading/generating acpi tables is one of the
> last things.  And it avoids the need to communicate the addresses (or
> patch locations) between qemu+firmware.
> 
> What do you want to use this for?  pmbase and xbar are simple, they are
> just a single register read.  pci io windows needs a root bus scan, but
> should be doable too.

Right. We'll need to migrate the offsets for patching since
they are tied to specific AML and this can change.

> > Gerd, Laszlo,others,  does this rule work for alternative firmwares?
> 
> It surely works for coreboot, and I would be very surprised if this
> causes trouble for ovmf.
> 
> cheers,
>   Gerd

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Laszlo Ersek]

On 08/09/13 17:49, Michael S. Tsirkin wrote:
> On Fri, Aug 09, 2013 at 12:13:06AM -0400, Kevin O'Connor wrote:
>> On Thu, Aug 08, 2013 at 04:56:55PM +0200, Gerd Hoffmann wrote:
>>> On 08/08/13 16:13, Michael S. Tsirkin wrote:
>>>> On Thu, Aug 08, 2013 at 12:21:32PM +0200, Gerd Hoffmann wrote:
>>>>> On 08/08/13 11:52, Michael S. Tsirkin wrote:
>>>>>> On Thu, Aug 08, 2013 at 10:57:44AM +0200, Gerd Hoffmann wrote:
>>>>>>> On 08/08/13 10:37, Michael S. Tsirkin wrote:
>>>>>>>> On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
>>>>>>>>> Also coreboot and seabios use different values for pmbase.  coreboot on
>>>>>>>>> q35 maps the pmbase below 0x1000.  Which surely makes sense.  When we
>>>>>>>>> don't place chipset stuff at 0xb000 we can assign the 0xb000->0xbfff
>>>>>>>>> window to a pci bridge instead.
>>>>
>>>> Re-reading this - if this has value, can't we generalize it
>>>> and make all firmware behave the same, getting values from QEMU?
>>>
>>> pmbase is a compile-time constant (aka #define) in both seabios and
>>> coreboot, and making this runtime-configurable is non-trivial.  See
>>> src/smm.c in seabios for one reason why.
>>
>> I don't think SeaBIOS should modify tables provided by QEMU.  That
>> leads to confusion on the source of the data and mixed
>> responsibilities which results in greater complexity in both QEMU and
>> SeaBIOS.
>>
>> SeaBIOS doesn't have any info that QEMU doesn't have.  So, I think
>> it's safe for QEMU to be the sole authority for the table content.
>>
>> Converting src/smm.c to use a runtime value isn't hard - just change
>> the assembler from: "mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04,
>> %dx\n" to: "mov 4(my_acpi_base), %dx\n" and make sure to define the
>> global variable my_acpi_base as VARFSEG.
>>
>>>>> Yes, the address ranges used for pci devices (aka 32bit + 64bit pci
>>>>> window) need to be there.  Well, placing in SSDT, then referencing from
>>>>> DSDT works too, and this is what seabios does today to dynamically
>>>>> adjust stuff.  Fixing up the SSDT using the linker is probably easier as
>>>>> we generate it anyway.
>>>>>
>>>> Yes but as I said, this makes things messy, since AML encoding for
>>>> numbers isn't fixed width.
>>>
>>> In seabios we have fixed 32bit / 64bit width today, from acpi.c:
>>>
>>>     // store pci io windows
>>>     *(u32*)&ssdt_ptr[acpi_pci32_start[0]] = cpu_to_le32(pcimem_start);
>>>     *(u32*)&ssdt_ptr[acpi_pci32_end[0]] = cpu_to_le32(pcimem_end - 1);
>>>     if (pcimem64_start) {
>>>         ssdt_ptr[acpi_pci64_valid[0]] = 1;
>>>         *(u64*)&ssdt_ptr[acpi_pci64_start[0]] = cpu_to_le64(pcimem64_start);
>>>         *(u64*)&ssdt_ptr[acpi_pci64_end[0]] = cpu_to_le64(pcimem64_end - 1);
>>>         *(u64*)&ssdt_ptr[acpi_pci64_length[0]] = cpu_to_le64(
>>>             pcimem64_end - pcimem64_start);
>>>     } else {
>>>         ssdt_ptr[acpi_pci64_valid[0]] = 0;
>>>     }
>>>
>>> Storing fixup instructions for these fields in the linker script
>>> shouldn't be hard I think.
>>
>> I don't think SeaBIOS should continue to do the above once the tables
>> are moved to QEMU.  QEMU has all the info SeaBIOS has, so it can
>> generate the tables correctly on its own.
>>
>> In all practical situations, the PCI window should be at least an
>> order of magnatude greater than the sum of the PCI bars in the system.
>> If the bars are actually bigger than the window, then things are going
>> to fail - the best the firmware can do is try to fail gracefully.  I
>> don't think it's worth the complexity to design mixed ownership and
>> advanced interfaces just so we can fail slightly better.
>>
>> If this is a real worry, QEMU can sum all the PCI bars and warn the
>> user if they don't fit.
>>
>> -Kevin
> 
> If we make it a rule that PCI is`setup before ACPI tables
> are read, then QEMU can do the patching itself when
> it detects BIOS reading the tables.
> 
> Gerd, Laszlo,others,  does this rule work for alternative firmwares?

OVMF currently determines the boundaries of the PCI window (32-bit) that
it is going to advertise in the _CRS method by scanning the "map of the
memory resources in the global coherency domain of the processor".

It finds the top of the system memory (under 4GB) and the smallest
interval that covers all of the memory mapped IO ranges. The subset of
the latter interval that is above the top of the system memory is then
advertised.

See PopulateFwData() in "OvmfPkg/AcpiPlatformDxe/Qemu.c", and
GetMemorySpaceMap() in "VOLUME 2: Platform Initialization Specification
/ Driver Execution Environment Core Interface".

... With this I'm trying to say that for now OVMF keys the 32-bit window
it advertises off regions that are "currently being decoded by a
component as memory-mapped I/O that can be used to access I/O devices in
the platform".

This seems to imply that PCI setup precedes ACPI table installation.

Laszlo

Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Michael S. Tsirkin]

On Thu, Aug 08, 2013 at 10:51:59AM +0200, Gerd Hoffmann wrote:
> On 08/08/13 10:22, Michael S. Tsirkin wrote:
> > On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>>> Huh?  The 32bit window is sized according to the installed memory.
> >>>> That
> >>>> logic is in seabios and you'll try to move it to qemu, using pci-info.
> >>>> It wasn't in qemu before ...
> >>>
> >>> The logic is in hw/i386/pc_piix.c and always was.
> >>
> >> What exactly you are refering to?
> > 
> > pc_init1 which picks addresses and passes them on to
> > i440fx_init.
> 
> Yep.  qemu figured where it wants map memory.  The unused 32bit address
> space goes into the pci hole.  cmos memory size is set accordingly.
> seabios gets the memory size from cmos, then it knows where the pci hole
> starts.  seabios rounds it up (i.e. may leave some of it unused) to be
> able to cover the complete hole with a single mtrr entry, but that isn't
> a issue and can be changed if needed.  The mtrr thing is more or less
> cosmetical anyway in a virtual machine.
> 
> >> Memory configuration is in the cmos, firmware can figure where it can
> >> place pci devices from that.  There is no need for a new interface.
> > 
> > The assumption being that whatever is not memory is PCI?
> > I'm not sure that's right.
> 
> Maybe not in general, but I'm pretty sure for the x86 chipsets we are
> emulating it is.

I think this is the basic question.

Speaking about PIIX:
http://download.intel.com/design/chipsets/datashts/29054901.pdf
it only supported 1G RAM and 32 bit PCI.

What happened with RAM below 1G is this:
 top of RAM to 0xfec00000 is PCI - this is emulated correctly
 fec10000 to ffe00000 is PCI - this is not emulated correctly

What happens with RAM >1G is all PV, it doesn't exist
on real hardware.

Re-adding qemu-devel. Can you please keep it Cc'd?

-- 
MST