Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 03:22:28PM +0100, Laszlo Ersek wrote:
> On 11/26/13 15:04, Michael S. Tsirkin wrote:
> > On Tue, Nov 26, 2013 at 12:00:51PM +0100, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>> I think it's down to other qemu bugs (such as _CRS not covering
> >>> all of PCI memory), we shall just fix them.
> >>
> >> i.e. the attached patch should fixup things.
> >>
> >> cheers,
> >>   Gerd
> >>
> > 
> > Yes, I think it's a start. Q35 is a bit harder because of the MMIO
> > region.  Do we want to tweak end too? There's all kind of
> > stuff there so need to be careful ...
> 
> I sent you a very similar patch last evening, and you ignored it. In
> that same email I asked about the mmconfig stuff as well, and you
> ignored that too. Attached.

I thought Igor answered this reasonably well actually.
I answered myself just in case ...


> >> >From a81b8d66e24fd298ce7654d424a378337e6cf132 Mon Sep 17 00:00:00 2001
> >> From: Gerd Hoffmann <address@hidden>
> >> Date: Tue, 26 Nov 2013 11:46:11 +0100
> >> Subject: [PATCH] piix: fix 32bit pci hole
> >>
> >> Make the 32bit pci hole start at end of ram, so all possible address
> >> space is covered.  Of course the firmware can use less than that.
> >> Leaving space unused is no problem, mapping pci bars outside the
> >> hole causes problems though.
> >>
> >> Signed-off-by: Gerd Hoffmann <address@hidden>
> >> ---
> >>  hw/pci-host/piix.c | 10 +---------
> >>  1 file changed, 1 insertion(+), 9 deletions(-)
> >>
> >> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >> index edc974e..1414a2b 100644
> >> --- a/hw/pci-host/piix.c
> >> +++ b/hw/pci-host/piix.c
> >> @@ -345,15 +345,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >>      f->ram_memory = ram_memory;
> >>  
> >>      i440fx = I440FX_PCI_HOST_BRIDGE(dev);
> >> -    /* Set PCI window size the way seabios has always done it. */
> >> -    /* Power of 2 so bios can cover it with a single MTRR */
> >> -    if (ram_size <= 0x80000000) {
> >> -        i440fx->pci_info.w32.begin = 0x80000000;
> >> -    } else if (ram_size <= 0xc0000000) {
> >> -        i440fx->pci_info.w32.begin = 0xc0000000;
> >> -    } else {
> >> -        i440fx->pci_info.w32.begin = 0xe0000000;
> >> -    }
> >> +    i440fx->pci_info.w32.begin = ram_size;
> 
> This doesn't clamp the w32.begin value into [0x80000000, 0xe0000000],
> which seems wrong.
> 
> Guys, I'm confused and annoyed out of my brains by the divergence here.
> In my perception Michael, Igor, and Gerd are all proposing different
> things, and my ideas are either rejected or ignored (even though they
> occasionally overlap with ideas from others). After struggling with this
> for more than a week, and having being awake for 27 hrs, I'm officially
> stopping to care right now.

Awake for 27 hours is not good.
Take a break, get some rest :)
I think what you propose below makes sense overall,
and it also seems that everyone agrees on this.

However there are some minor implementation issues and people started
arguing about them.
Also, everyone here is human, it's easy to miss a mail in a long thread.


> Ping me when qemu has something to offer
> that's neither ridden by SeaBIOS legacy nor requires an AML interpreter
> in OVMF's PEI phase.
> 
> Thanks,
> Laszlo

Something along the lines of what you propose below will do it right?


> Date: Mon, 25 Nov 2013 15:46:58 +0100
> From: Laszlo Ersek <address@hidden>
> To: "Michael S. Tsirkin" <address@hidden>
> CC: Igor Mammedov <address@hidden>
> Subject: Re: changing the pci32 window in qemu's DSDT/SSDT
> Message-ID: <address@hidden>
> In-Reply-To: <address@hidden>
> 
> CC'ing Igor
> 
> On 11/25/13 14:24, Michael S. Tsirkin wrote:
> > On Sun, Nov 24, 2013 at 06:43:38PM +0100, Laszlo Ersek wrote:
> >> Hello Michael,
> >>
> >> I didn't completely get your point on IRC regarding this subject. You
> >> were saying that OVMF shouldn't be changed, but the boundaries
> >> exported by qemu should (and that qemu should stop caring about
> >> SeaBIOS tradition in this regard).
> >>
> >> You mentioned MMCONFIG and I don't understand that reference. Can you
> >> please elaborate how you think we should proceed?
> >>
> >> Thanks!
> >> Laszlo
> >
> > I refer to this:
> >     /* Leave enough space for the biggest MCFG BAR */
> >     /* TODO: this matches current bios behaviour, but
> >      * it's not a power of two, which means an MTRR
> >      * can't cover it exactly.
> >      */
> >     s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> >         MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> >     s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> >
> > We must make sure CRS does not include the mmcfg region.
> 
> Sorry but I still don't understand :)
> 
> First, OVMF is currently for i440fx only, and the quoted part is from
> "hw/pci-host/q35.c". I of course agree that we should fix this in a
> uniform manner in qemu -- just saying that for OVMF the following would
> suffice:
> 
>   diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>   index edc974e..9753fea 100644
>   --- a/hw/pci-host/piix.c
>   +++ b/hw/pci-host/piix.c
>   @@ -345,12 +345,10 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>        f->ram_memory = ram_memory;
> 
>        i440fx = I440FX_PCI_HOST_BRIDGE(dev);
>   -    /* Set PCI window size the way seabios has always done it. */
>   -    /* Power of 2 so bios can cover it with a single MTRR */
>        if (ram_size <= 0x80000000) {
>            i440fx->pci_info.w32.begin = 0x80000000;
>   -    } else if (ram_size <= 0xc0000000) {
>   -        i440fx->pci_info.w32.begin = 0xc0000000;
>   +    } else if (ram_size <= 0xe0000000) {
>   +        i440fx->pci_info.w32.begin = ram_size;
>        } else {
>            i440fx->pci_info.w32.begin = 0xe0000000;
>        }
> 
> Second, the code that you quoted from "hw/pci-host/q35.c" seems to do
> exactly what you're saying we must do.
> 
>   static Property mch_props[] = {
>       DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, Q35PCIHost, 
> parent_obj.base_addr,
>                           MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> 
> This sets the default mmconfig base address at
> MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT. The code you qouted seems to ensure
> that the range starting at MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT, for a
> length of MCH_HOST_BRIDGE_PCIEXBAR_MAX (ie from 0xb0000000 to
> 0xc0000000) is not included in CRS. In other words, it sets w32.begin to
> right above this BAR (to 0xc0000000), hence the mmcfg region is already
> excluded from CRS.
> 
> So, what do you mean? Do you want to make w32.begin depend on the
> PCIE_HOST_MCFG_BASE property dynamically (ie. to notice when the user
> changes that option on the command line)?
> 
>   diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>   index c043998..48e6758 100644
>   --- a/hw/pci-host/q35.c
>   +++ b/hw/pci-host/q35.c
>   @@ -181,11 +181,7 @@ static void q35_host_initfn(Object *obj)
>                            NULL, NULL, NULL, NULL);
> 
>        /* Leave enough space for the biggest MCFG BAR */
>   -    /* TODO: this matches current bios behaviour, but
>   -     * it's not a power of two, which means an MTRR
>   -     * can't cover it exactly.
>   -     */
>   -    s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
>   +    s->mch.pci_info.w32.begin = PCIE_HOST_BRIDGE(obj)->base_addr +
>            MCH_HOST_BRIDGE_PCIEXBAR_MAX;
>        s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
>    }
> 
> Sorry for being dense...
> 
> Thanks
> Laszlo