| Message ID | 1378291667-8516-2-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 4 Sep 2013 13:48:29 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > For Q35, MMCFG address and size are guest configurable. > Update w32 property to make it behave accordingly. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/pci-host/q35.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index 4febd24..3f1d447 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch) > } > addr = pciexbar & addr_mask; > pcie_host_mmcfg_update(pehb, enable, addr, length); > + /* Leave enough space for the 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. > + */ > + if (enable) { > + mch->pci_info.w32.begin = addr + length; > + } else { > + mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; > + } > } I probably miss something but where is remapping in system address space? If there is none then, then updated w32 might mismatch actually/initially mapped alias. > /* PAM */
On Tue, 10 Sep 2013 16:46:36 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote: > > On Wed, 4 Sep 2013 13:48:29 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > For Q35, MMCFG address and size are guest configurable. > > > Update w32 property to make it behave accordingly. > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > > hw/pci-host/q35.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > > index 4febd24..3f1d447 100644 > > > --- a/hw/pci-host/q35.c > > > +++ b/hw/pci-host/q35.c > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch) > > > } > > > addr = pciexbar & addr_mask; > > > pcie_host_mmcfg_update(pehb, enable, addr, length); > > > + /* Leave enough space for the 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. > > > + */ > > > + if (enable) { > > > + mch->pci_info.w32.begin = addr + length; > > > + } else { > > > + mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; > > > + } > > > } > > I probably miss something but where is remapping in system address space? > > If there is none then, then updated w32 might mismatch actually/initially mapped alias. > > > > > /* PAM */ > > You mean mmcfg? > The re-mapping is in hw/pci/pcie_host.c no, I mean 32-bit PCI hole
On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote: > On Wed, 4 Sep 2013 13:48:29 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > For Q35, MMCFG address and size are guest configurable. > > Update w32 property to make it behave accordingly. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > hw/pci-host/q35.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > index 4febd24..3f1d447 100644 > > --- a/hw/pci-host/q35.c > > +++ b/hw/pci-host/q35.c > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch) > > } > > addr = pciexbar & addr_mask; > > pcie_host_mmcfg_update(pehb, enable, addr, length); > > + /* Leave enough space for the 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. > > + */ > > + if (enable) { > > + mch->pci_info.w32.begin = addr + length; > > + } else { > > + mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; > > + } > > } > I probably miss something but where is remapping in system address space? > If there is none then, then updated w32 might mismatch actually/initially mapped alias. > > > /* PAM */ You mean mmcfg? The re-mapping is in hw/pci/pcie_host.c
On Tue, Sep 10, 2013 at 03:46:13PM +0200, Igor Mammedov wrote: > On Tue, 10 Sep 2013 16:46:36 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote: > > > On Wed, 4 Sep 2013 13:48:29 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > For Q35, MMCFG address and size are guest configurable. > > > > Update w32 property to make it behave accordingly. > > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > > hw/pci-host/q35.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > > > index 4febd24..3f1d447 100644 > > > > --- a/hw/pci-host/q35.c > > > > +++ b/hw/pci-host/q35.c > > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch) > > > > } > > > > addr = pciexbar & addr_mask; > > > > pcie_host_mmcfg_update(pehb, enable, addr, length); > > > > + /* Leave enough space for the 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. > > > > + */ > > > > + if (enable) { > > > > + mch->pci_info.w32.begin = addr + length; > > > > + } else { > > > > + mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; > > > > + } > > > > } > > > I probably miss something but where is remapping in system address space? > > > If there is none then, then updated w32 might mismatch actually/initially mapped alias. > > > > > > > /* PAM */ > > > > You mean mmcfg? > > The re-mapping is in hw/pci/pcie_host.c > no, I mean 32-bit PCI hole It works differently - see mch_init. 32-bit PCI hole is not remapped - instead mmcfg overlaps it and shadows a chunk of it.
On Tue, 10 Sep 2013 16:53:58 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Sep 10, 2013 at 03:46:13PM +0200, Igor Mammedov wrote: > > On Tue, 10 Sep 2013 16:46:36 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote: > > > > On Wed, 4 Sep 2013 13:48:29 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > For Q35, MMCFG address and size are guest configurable. > > > > > Update w32 property to make it behave accordingly. > > > > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > --- > > > > > hw/pci-host/q35.c | 10 ++++++++++ > > > > > 1 file changed, 10 insertions(+) > > > > > > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > > > > index 4febd24..3f1d447 100644 > > > > > --- a/hw/pci-host/q35.c > > > > > +++ b/hw/pci-host/q35.c > > > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch) > > > > > } > > > > > addr = pciexbar & addr_mask; > > > > > pcie_host_mmcfg_update(pehb, enable, addr, length); > > > > > + /* Leave enough space for the 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. > > > > > + */ > > > > > + if (enable) { > > > > > + mch->pci_info.w32.begin = addr + length; > > > > > + } else { > > > > > + mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; > > > > > + } > > > > > } > > > > I probably miss something but where is remapping in system address space? > > > > If there is none then, then updated w32 might mismatch actually/initially mapped alias. > > > > > > > > > /* PAM */ > > > > > > You mean mmcfg? > > > The re-mapping is in hw/pci/pcie_host.c > > no, I mean 32-bit PCI hole > > It works differently - see mch_init. > 32-bit PCI hole is not remapped - instead mmcfg overlaps it > and shadows a chunk of it. > it probably won't cause harm, but actual pci_hole alias doesn't match its window reported via properties anymore (it's just inconsistent).
On Tue, Sep 10, 2013 at 04:09:21PM +0200, Igor Mammedov wrote: > On Tue, 10 Sep 2013 16:53:58 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Sep 10, 2013 at 03:46:13PM +0200, Igor Mammedov wrote: > > > On Tue, 10 Sep 2013 16:46:36 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote: > > > > > On Wed, 4 Sep 2013 13:48:29 +0300 > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > For Q35, MMCFG address and size are guest configurable. > > > > > > Update w32 property to make it behave accordingly. > > > > > > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > --- > > > > > > hw/pci-host/q35.c | 10 ++++++++++ > > > > > > 1 file changed, 10 insertions(+) > > > > > > > > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > > > > > index 4febd24..3f1d447 100644 > > > > > > --- a/hw/pci-host/q35.c > > > > > > +++ b/hw/pci-host/q35.c > > > > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch) > > > > > > } > > > > > > addr = pciexbar & addr_mask; > > > > > > pcie_host_mmcfg_update(pehb, enable, addr, length); > > > > > > + /* Leave enough space for the 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. > > > > > > + */ > > > > > > + if (enable) { > > > > > > + mch->pci_info.w32.begin = addr + length; > > > > > > + } else { > > > > > > + mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; > > > > > > + } > > > > > > } > > > > > I probably miss something but where is remapping in system address space? > > > > > If there is none then, then updated w32 might mismatch actually/initially mapped alias. > > > > > > > > > > > /* PAM */ > > > > > > > > You mean mmcfg? > > > > The re-mapping is in hw/pci/pcie_host.c > > > no, I mean 32-bit PCI hole > > > > It works differently - see mch_init. > > 32-bit PCI hole is not remapped - instead mmcfg overlaps it > > and shadows a chunk of it. > > > it probably won't cause harm, but actual pci_hole alias doesn't match > its window reported via properties anymore (it's just inconsistent). That's exactly what happens at the moment too: for q35 pci hole has a chunk that is not reported in w32. Do you imply that w32 should just report the PCI hole, and make users (e.g. acpi) take mmcfg out of that?
On Tue, 10 Sep 2013 17:16:24 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Sep 10, 2013 at 04:09:21PM +0200, Igor Mammedov wrote: > > On Tue, 10 Sep 2013 16:53:58 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Tue, Sep 10, 2013 at 03:46:13PM +0200, Igor Mammedov wrote: > > > > On Tue, 10 Sep 2013 16:46:36 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote: > > > > > > On Wed, 4 Sep 2013 13:48:29 +0300 > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > > > For Q35, MMCFG address and size are guest configurable. > > > > > > > Update w32 property to make it behave accordingly. > > > > > > > > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > > --- > > > > > > > hw/pci-host/q35.c | 10 ++++++++++ > > > > > > > 1 file changed, 10 insertions(+) > > > > > > > > > > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > > > > > > index 4febd24..3f1d447 100644 > > > > > > > --- a/hw/pci-host/q35.c > > > > > > > +++ b/hw/pci-host/q35.c > > > > > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch) > > > > > > > } > > > > > > > addr = pciexbar & addr_mask; > > > > > > > pcie_host_mmcfg_update(pehb, enable, addr, length); > > > > > > > + /* Leave enough space for the 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. > > > > > > > + */ > > > > > > > + if (enable) { > > > > > > > + mch->pci_info.w32.begin = addr + length; > > > > > > > + } else { > > > > > > > + mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; > > > > > > > + } > > > > > > > } > > > > > > I probably miss something but where is remapping in system address space? > > > > > > If there is none then, then updated w32 might mismatch actually/initially mapped alias. > > > > > > > > > > > > > /* PAM */ > > > > > > > > > > You mean mmcfg? > > > > > The re-mapping is in hw/pci/pcie_host.c > > > > no, I mean 32-bit PCI hole > > > > > > It works differently - see mch_init. > > > 32-bit PCI hole is not remapped - instead mmcfg overlaps it > > > and shadows a chunk of it. > > > > > it probably won't cause harm, but actual pci_hole alias doesn't match > > its window reported via properties anymore (it's just inconsistent). > > That's exactly what happens at the moment too: > for q35 pci hole has a chunk that is not reported in w32. > > Do you imply that w32 should just report the PCI hole, > and make users (e.g. acpi) take mmcfg out of that? initial pci_hole alias in system AS doesn't include mmcfg, it's mapped right after mmcfg and w32 range matches alias exactly. But after mmcfg update that might be not true. I suggest to move/remap alias to match new w32, to be consistent. > >
On Tue, Sep 10, 2013 at 05:05:43PM +0200, Igor Mammedov wrote: > On Tue, 10 Sep 2013 17:16:24 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Sep 10, 2013 at 04:09:21PM +0200, Igor Mammedov wrote: > > > On Tue, 10 Sep 2013 16:53:58 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Tue, Sep 10, 2013 at 03:46:13PM +0200, Igor Mammedov wrote: > > > > > On Tue, 10 Sep 2013 16:46:36 +0300 > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote: > > > > > > > On Wed, 4 Sep 2013 13:48:29 +0300 > > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > > > > > For Q35, MMCFG address and size are guest configurable. > > > > > > > > Update w32 property to make it behave accordingly. > > > > > > > > > > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > > > --- > > > > > > > > hw/pci-host/q35.c | 10 ++++++++++ > > > > > > > > 1 file changed, 10 insertions(+) > > > > > > > > > > > > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > > > > > > > index 4febd24..3f1d447 100644 > > > > > > > > --- a/hw/pci-host/q35.c > > > > > > > > +++ b/hw/pci-host/q35.c > > > > > > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch) > > > > > > > > } > > > > > > > > addr = pciexbar & addr_mask; > > > > > > > > pcie_host_mmcfg_update(pehb, enable, addr, length); > > > > > > > > + /* Leave enough space for the 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. > > > > > > > > + */ > > > > > > > > + if (enable) { > > > > > > > > + mch->pci_info.w32.begin = addr + length; > > > > > > > > + } else { > > > > > > > > + mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; > > > > > > > > + } > > > > > > > > } > > > > > > > I probably miss something but where is remapping in system address space? > > > > > > > If there is none then, then updated w32 might mismatch actually/initially mapped alias. > > > > > > > > > > > > > > > /* PAM */ > > > > > > > > > > > > You mean mmcfg? > > > > > > The re-mapping is in hw/pci/pcie_host.c > > > > > no, I mean 32-bit PCI hole > > > > > > > > It works differently - see mch_init. > > > > 32-bit PCI hole is not remapped - instead mmcfg overlaps it > > > > and shadows a chunk of it. > > > > > > > it probably won't cause harm, but actual pci_hole alias doesn't match > > > its window reported via properties anymore (it's just inconsistent). > > > > That's exactly what happens at the moment too: > > for q35 pci hole has a chunk that is not reported in w32. > > > > Do you imply that w32 should just report the PCI hole, > > and make users (e.g. acpi) take mmcfg out of that? > initial pci_hole alias in system AS doesn't include mmcfg, it's mapped right after mmcfg and > w32 range matches alias exactly. That would be a bug (unrelated to this patch series). But I don't see this code: looking at mch_init, memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size, &mch->pci_hole); so we have pci hole right after memory, and overlapping mmcfg, which makes sense to me. > But after mmcfg update that might be not true. I suggest to > move/remap alias to match new w32, to be consistent. > > > >
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index 4febd24..3f1d447 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch) } addr = pciexbar & addr_mask; pcie_host_mmcfg_update(pehb, enable, addr, length); + /* Leave enough space for the 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. + */ + if (enable) { + mch->pci_info.w32.begin = addr + length; + } else { + mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; + } } /* PAM */
For Q35, MMCFG address and size are guest configurable. Update w32 property to make it behave accordingly. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/pci-host/q35.c | 10 ++++++++++ 1 file changed, 10 insertions(+)