hw/ppc/spapr_pci.c | 52 ++++++++++------------------------------------------ 1 file changed, 10 insertions(+), 42 deletions(-)
Creating several PHBs without index property confuses the DRC code
and causes issues:
- only the first index-less PHB is functional, the other ones will
silently ignore hotplugging of PCI devices
- QEMU will even terminate if these PHBs have cold-plugged devices
qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device
is still awaiting release
This happens because DR connectors for child PCI devices are created
with a DRC index that is derived from the PHB's index property. If the
PHBs are created without index, then the same value of -1 is used to
compute the DRC indexes for both PHBs, hence causing the collision.
Also, the index property is used to compute the placement of the PHB's
memory regions. It is limited to 31 or 255, depending on the machine
type version. This fits well with the requirements of DRC indexes, which
need the PHB index to be a 16-bit value.
This patch hence makes the index property mandatory. As a consequence,
the PHB's memory regions and BUID are now always configured according
to the index, and it is no longer possible to set them from the command
line. We have to introduce a PHB instance init function to initialize
the 64-bit window address to -1 because pseries-2.7 and older machines
don't set it.
This DOES BREAK backwards compat, but we don't think the non-index
PHB feature was used in practice (at least libvirt doesn't) and the
simplification is worth it.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
RFC->v1: - as suggested dy David, updated the changelog to explicitely
mention that we intentionally break backwards compat.
---
hw/ppc/spapr_pci.c | 52 ++++++++++------------------------------------------
1 file changed, 10 insertions(+), 42 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cf54160526fa..9a338b7f197b 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1523,16 +1523,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
Error *local_err = NULL;
- if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != (uint32_t)-1)
- || (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2)
- || (sphb->mem_win_addr != (hwaddr)-1)
- || (sphb->mem64_win_addr != (hwaddr)-1)
- || (sphb->io_win_addr != (hwaddr)-1)) {
- error_setg(errp, "Either \"index\" or other parameters must"
- " be specified for PAPR PHB, not both");
- return;
- }
-
smc->phb_placement(spapr, sphb->index,
&sphb->buid, &sphb->io_win_addr,
&sphb->mem_win_addr, &sphb->mem64_win_addr,
@@ -1541,36 +1531,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
error_propagate(errp, local_err);
return;
}
- }
-
- if (sphb->buid == (uint64_t)-1) {
- error_setg(errp, "BUID not specified for PHB");
- return;
- }
-
- if ((sphb->dma_liobn[0] == (uint32_t)-1) ||
- ((sphb->dma_liobn[1] == (uint32_t)-1) && (windows_supported > 1))) {
- error_setg(errp, "LIOBN(s) not specified for PHB");
- return;
- }
-
- if (sphb->mem_win_addr == (hwaddr)-1) {
- error_setg(errp, "Memory window address not specified for PHB");
- return;
- }
-
- if (sphb->io_win_addr == (hwaddr)-1) {
- error_setg(errp, "IO window address not specified for PHB");
+ } else {
+ error_setg(errp, "\"index\" for PAPR PHB is mandatory");
return;
}
if (sphb->mem64_win_size != 0) {
- if (sphb->mem64_win_addr == (hwaddr)-1) {
- error_setg(errp,
- "64-bit memory window address not specified for PHB");
- return;
- }
-
if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
error_setg(errp, "32-bit memory window of size 0x%"HWADDR_PRIx
" (max 2 GiB)", sphb->mem_win_size);
@@ -1789,18 +1755,12 @@ static void spapr_phb_reset(DeviceState *qdev)
static Property spapr_phb_properties[] = {
DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1),
- DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
- DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn[0], -1),
- DEFINE_PROP_UINT32("liobn64", sPAPRPHBState, dma_liobn[1], -1),
- DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
SPAPR_PCI_MEM32_WIN_SIZE),
- DEFINE_PROP_UINT64("mem64_win_addr", sPAPRPHBState, mem64_win_addr, -1),
DEFINE_PROP_UINT64("mem64_win_size", sPAPRPHBState, mem64_win_size,
SPAPR_PCI_MEM64_WIN_SIZE),
DEFINE_PROP_UINT64("mem64_win_pciaddr", sPAPRPHBState, mem64_win_pciaddr,
-1),
- DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
DEFINE_PROP_UINT64("io_win_size", sPAPRPHBState, io_win_size,
SPAPR_PCI_IO_WIN_SIZE),
DEFINE_PROP_BOOL("dynamic-reconfiguration", sPAPRPHBState, dr_enabled,
@@ -1937,6 +1897,13 @@ static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
return sphb->dtbusname;
}
+static void spapr_phb_instance_init(Object *obj)
+{
+ sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(obj);
+
+ sphb->mem64_win_addr = (hwaddr)-1;
+}
+
static void spapr_phb_class_init(ObjectClass *klass, void *data)
{
PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
@@ -1960,6 +1927,7 @@ static const TypeInfo spapr_phb_info = {
.parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(sPAPRPHBState),
.class_init = spapr_phb_class_init,
+ .instance_init = spapr_phb_instance_init,
.interfaces = (InterfaceInfo[]) {
{ TYPE_HOTPLUG_HANDLER },
{ }
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCH] spapr_pci: make index property mandatory
Message-id: 150537259490.3298.1180094221641142666.stgit@bahia
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/150537259490.3298.1180094221641142666.stgit@bahia -> patchew/150537259490.3298.1180094221641142666.stgit@bahia
t [tag update] patchew/20170913142036.2469-1-lvivier@redhat.com -> patchew/20170913142036.2469-1-lvivier@redhat.com
Switched to a new branch 'test'
705ee2bccc spapr_pci: make index property mandatory
=== OUTPUT BEGIN ===
Checking PATCH 1/1: spapr_pci: make index property mandatory...
ERROR: spaces required around that '-' (ctx:VxV)
#126: FILE: hw/ppc/spapr_pci.c:1920:
+ sphb->mem64_win_addr = (hwaddr)-1;
^
total: 1 errors, 0 warnings, 92 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
On Thu, Sep 14, 2017 at 09:03:15AM +0200, Greg Kurz wrote:
> Creating several PHBs without index property confuses the DRC code
> and causes issues:
> - only the first index-less PHB is functional, the other ones will
> silently ignore hotplugging of PCI devices
> - QEMU will even terminate if these PHBs have cold-plugged devices
>
> qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device
> is still awaiting release
>
> This happens because DR connectors for child PCI devices are created
> with a DRC index that is derived from the PHB's index property. If the
> PHBs are created without index, then the same value of -1 is used to
> compute the DRC indexes for both PHBs, hence causing the collision.
>
> Also, the index property is used to compute the placement of the PHB's
> memory regions. It is limited to 31 or 255, depending on the machine
> type version. This fits well with the requirements of DRC indexes, which
> need the PHB index to be a 16-bit value.
>
> This patch hence makes the index property mandatory. As a consequence,
> the PHB's memory regions and BUID are now always configured according
> to the index, and it is no longer possible to set them from the command
> line. We have to introduce a PHB instance init function to initialize
> the 64-bit window address to -1 because pseries-2.7 and older machines
> don't set it.
>
> This DOES BREAK backwards compat, but we don't think the non-index
> PHB feature was used in practice (at least libvirt doesn't) and the
> simplification is worth it.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> RFC->v1: - as suggested dy David, updated the changelog to explicitely
> mention that we intentionally break backwards compat.
> ---
> hw/ppc/spapr_pci.c | 52 ++++++++++------------------------------------------
> 1 file changed, 10 insertions(+), 42 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cf54160526fa..9a338b7f197b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1523,16 +1523,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> Error *local_err = NULL;
>
> - if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != (uint32_t)-1)
> - || (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2)
> - || (sphb->mem_win_addr != (hwaddr)-1)
> - || (sphb->mem64_win_addr != (hwaddr)-1)
> - || (sphb->io_win_addr != (hwaddr)-1)) {
> - error_setg(errp, "Either \"index\" or other parameters must"
> - " be specified for PAPR PHB, not both");
> - return;
> - }
> -
> smc->phb_placement(spapr, sphb->index,
> &sphb->buid, &sphb->io_win_addr,
> &sphb->mem_win_addr, &sphb->mem64_win_addr,
> @@ -1541,36 +1531,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> error_propagate(errp, local_err);
> return;
> }
> - }
> -
> - if (sphb->buid == (uint64_t)-1) {
> - error_setg(errp, "BUID not specified for PHB");
> - return;
> - }
> -
> - if ((sphb->dma_liobn[0] == (uint32_t)-1) ||
> - ((sphb->dma_liobn[1] == (uint32_t)-1) && (windows_supported > 1))) {
> - error_setg(errp, "LIOBN(s) not specified for PHB");
> - return;
> - }
> -
> - if (sphb->mem_win_addr == (hwaddr)-1) {
> - error_setg(errp, "Memory window address not specified for PHB");
> - return;
> - }
> -
> - if (sphb->io_win_addr == (hwaddr)-1) {
> - error_setg(errp, "IO window address not specified for PHB");
> + } else {
> + error_setg(errp, "\"index\" for PAPR PHB is mandatory");
> return;
> }
>
> if (sphb->mem64_win_size != 0) {
> - if (sphb->mem64_win_addr == (hwaddr)-1) {
> - error_setg(errp,
> - "64-bit memory window address not specified for PHB");
> - return;
> - }
> -
> if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
> error_setg(errp, "32-bit memory window of size 0x%"HWADDR_PRIx
> " (max 2 GiB)", sphb->mem_win_size);
> @@ -1789,18 +1755,12 @@ static void spapr_phb_reset(DeviceState *qdev)
>
> static Property spapr_phb_properties[] = {
> DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1),
> - DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
> - DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn[0], -1),
> - DEFINE_PROP_UINT32("liobn64", sPAPRPHBState, dma_liobn[1], -1),
> - DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
> DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
> SPAPR_PCI_MEM32_WIN_SIZE),
> - DEFINE_PROP_UINT64("mem64_win_addr", sPAPRPHBState, mem64_win_addr, -1),
> DEFINE_PROP_UINT64("mem64_win_size", sPAPRPHBState, mem64_win_size,
> SPAPR_PCI_MEM64_WIN_SIZE),
> DEFINE_PROP_UINT64("mem64_win_pciaddr", sPAPRPHBState, mem64_win_pciaddr,
> -1),
> - DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
> DEFINE_PROP_UINT64("io_win_size", sPAPRPHBState, io_win_size,
> SPAPR_PCI_IO_WIN_SIZE),
> DEFINE_PROP_BOOL("dynamic-reconfiguration", sPAPRPHBState, dr_enabled,
> @@ -1937,6 +1897,13 @@ static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
> return sphb->dtbusname;
> }
>
> +static void spapr_phb_instance_init(Object *obj)
> +{
> + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(obj);
> +
> + sphb->mem64_win_addr = (hwaddr)-1;
> +}
> +
Why do we need to initialize this field especially?
> static void spapr_phb_class_init(ObjectClass *klass, void *data)
> {
> PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> @@ -1960,6 +1927,7 @@ static const TypeInfo spapr_phb_info = {
> .parent = TYPE_PCI_HOST_BRIDGE,
> .instance_size = sizeof(sPAPRPHBState),
> .class_init = spapr_phb_class_init,
> + .instance_init = spapr_phb_instance_init,
> .interfaces = (InterfaceInfo[]) {
> { TYPE_HOTPLUG_HANDLER },
> { }
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Thu, 14 Sep 2017 22:31:26 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Thu, Sep 14, 2017 at 09:03:15AM +0200, Greg Kurz wrote:
> > Creating several PHBs without index property confuses the DRC code
> > and causes issues:
> > - only the first index-less PHB is functional, the other ones will
> > silently ignore hotplugging of PCI devices
> > - QEMU will even terminate if these PHBs have cold-plugged devices
> >
> > qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device
> > is still awaiting release
> >
> > This happens because DR connectors for child PCI devices are created
> > with a DRC index that is derived from the PHB's index property. If the
> > PHBs are created without index, then the same value of -1 is used to
> > compute the DRC indexes for both PHBs, hence causing the collision.
> >
> > Also, the index property is used to compute the placement of the PHB's
> > memory regions. It is limited to 31 or 255, depending on the machine
> > type version. This fits well with the requirements of DRC indexes, which
> > need the PHB index to be a 16-bit value.
> >
> > This patch hence makes the index property mandatory. As a consequence,
> > the PHB's memory regions and BUID are now always configured according
> > to the index, and it is no longer possible to set them from the command
> > line. We have to introduce a PHB instance init function to initialize
> > the 64-bit window address to -1 because pseries-2.7 and older machines
> > don't set it.
> >
> > This DOES BREAK backwards compat, but we don't think the non-index
> > PHB feature was used in practice (at least libvirt doesn't) and the
> > simplification is worth it.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > RFC->v1: - as suggested dy David, updated the changelog to explicitely
> > mention that we intentionally break backwards compat.
> > ---
[...snip...]
> > +static void spapr_phb_instance_init(Object *obj)
> > +{
> > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(obj);
> > +
> > + sphb->mem64_win_addr = (hwaddr)-1;
> > +}
> > +
>
> Why do we need to initialize this field especially?
>
It is *somehow* explained in the commit log:
We have to introduce a PHB instance init function to initialize the
64-bit window address to -1 because pseries-2.7 and older machines
don't set it. [in the phb_placement hook]
/*
* We don't set the 64-bit MMIO window, relying on the PHB's
* fallback behaviour of automatically splitting a large "32-bit"
* window into contiguous 32-bit and 64-bit windows
*/
and spapr_phb_realize() doesn't set it either unless
sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE...
But thinking again, I guess I should add an else block in spapr_phb_realize()
instead.
I'll send a v2 (and I'll send the checkpatch fix along it if you don't
mind).
> > static void spapr_phb_class_init(ObjectClass *klass, void *data)
> > {
> > PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> > @@ -1960,6 +1927,7 @@ static const TypeInfo spapr_phb_info = {
> > .parent = TYPE_PCI_HOST_BRIDGE,
> > .instance_size = sizeof(sPAPRPHBState),
> > .class_init = spapr_phb_class_init,
> > + .instance_init = spapr_phb_instance_init,
> > .interfaces = (InterfaceInfo[]) {
> > { TYPE_HOTPLUG_HANDLER },
> > { }
> >
>
© 2016 - 2024 Red Hat, Inc.