[{"id":1770846,"web_url":"http://patchwork.ozlabs.org/comment/1770846/","msgid":"<20170918230358.GG27153@umbus>","list_archive_url":null,"date":"2017-09-18T23:03:58","subject":"Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: make index property\n\tmandatory","submitter":{"id":47,"url":"http://patchwork.ozlabs.org/api/people/47/","name":"David Gibson","email":"david@gibson.dropbear.id.au"},"content":"On Thu, Sep 14, 2017 at 04:14:44PM +0200, Greg Kurz wrote:\n> Creating several PHBs without index property confuses the DRC code\n> and causes issues:\n> - only the first index-less PHB is functional, the other ones will\n> silently ignore hotplugging of PCI devices\n> - QEMU will even terminate if these PHBs have cold-plugged devices\n\nSo, this is true, but it's kind of missing the point; we could fix\nthose things if we wanted. The real point here is that the non-index\noption is more trouble than it's worth: it's difficult to use, keeps\nrequiring (potentially incompatible) changes when some new parameter\nneeds adding, and is awkward to check for collisions.\n\n> qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device\n> is still awaiting release\n> \n> This happens because DR connectors for child PCI devices are created\n> with a DRC index that is derived from the PHB's index property. If the\n> PHBs are created without index, then the same value of -1 is used to\n> compute the DRC indexes for both PHBs, hence causing the collision.\n> \n> Also, the index property is used to compute the placement of the PHB's\n> memory regions. It is limited to 31 or 255, depending on the machine\n> type version. This fits well with the requirements of DRC indexes, which\n> need the PHB index to be a 16-bit value.\n> \n> This patch hence makes the index property mandatory. As a consequence,\n> the PHB's memory regions and BUID are now always configured according\n> to the index, and it is no longer possible to set them from the command\n> line. We have to introduce a PHB instance init function to initialize\n> the 64-bit window address to -1 because pseries-2.7 and older machines\n> don't set it.\n> \n> This DOES BREAK backwards compat, but we don't think the non-index\n> PHB feature was used in practice (at least libvirt doesn't) and the\n> simplification is worth it.\n> \n> Signed-off-by: Greg Kurz \n> ---\n> v1->v2: - error out if mem64_win_pciaddr is set but mem64_win_size\n> isn't\n> - set mem64_win_addr to -1 for old configuration with 32-bit\n> window below 2G in spapr_phb_realize()\n> - drop instance init function\n> \n> RFC->v1: - as suggested dy David, updated the changelog to explicitely\n> mention that we intentionally break backwards compat.\n> ---\n> hw/ppc/spapr_pci.c | 53 +++++++++++-----------------------------------------\n> 1 file changed, 11 insertions(+), 42 deletions(-)\n> \n> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c\n> index cf54160526fa..024638e18b53 100644\n> --- a/hw/ppc/spapr_pci.c\n> +++ b/hw/ppc/spapr_pci.c\n> @@ -1523,16 +1523,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);\n> Error *local_err = NULL;\n> \n> - if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != (uint32_t)-1)\n> - || (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2)\n> - || (sphb->mem_win_addr != (hwaddr)-1)\n> - || (sphb->mem64_win_addr != (hwaddr)-1)\n> - || (sphb->io_win_addr != (hwaddr)-1)) {\n> - error_setg(errp, \"Either \\\"index\\\" or other parameters must\"\n> - \" be specified for PAPR PHB, not both\");\n> - return;\n> - }\n> -\n> smc->phb_placement(spapr, sphb->index,\n> &sphb->buid, &sphb->io_win_addr,\n> &sphb->mem_win_addr, &sphb->mem64_win_addr,\n> @@ -1541,36 +1531,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> error_propagate(errp, local_err);\n> return;\n> }\n> - }\n> -\n> - if (sphb->buid == (uint64_t)-1) {\n> - error_setg(errp, \"BUID not specified for PHB\");\n> - return;\n> - }\n> -\n> - if ((sphb->dma_liobn[0] == (uint32_t)-1) ||\n> - ((sphb->dma_liobn[1] == (uint32_t)-1) && (windows_supported > 1))) {\n> - error_setg(errp, \"LIOBN(s) not specified for PHB\");\n> - return;\n> - }\n> -\n> - if (sphb->mem_win_addr == (hwaddr)-1) {\n> - error_setg(errp, \"Memory window address not specified for PHB\");\n> - return;\n> - }\n> -\n> - if (sphb->io_win_addr == (hwaddr)-1) {\n> - error_setg(errp, \"IO window address not specified for PHB\");\n> + } else {\n> + error_setg(errp, \"\\\"index\\\" for PAPR PHB is mandatory\");\n> return;\n> }\n> \n> if (sphb->mem64_win_size != 0) {\n> - if (sphb->mem64_win_addr == (hwaddr)-1) {\n> - error_setg(errp,\n> - \"64-bit memory window address not specified for PHB\");\n> - return;\n> - }\n> -\n> if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {\n> error_setg(errp, \"32-bit memory window of size 0x%\"HWADDR_PRIx\n> \" (max 2 GiB)\", sphb->mem_win_size);\n\nSome of these checks could probably just become assert()s now, where\nthey're things determined by the placement code and no longer have\nparameters, a problem indicates a bug in the machine rather than a\nuser error.\n\n> @@ -1581,6 +1547,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> /* 64-bit window defaults to identity mapping */\n> sphb->mem64_win_pciaddr = sphb->mem64_win_addr;\n> }\n> + } else if (sphb->mem64_win_pciaddr != (hwaddr) -1) {\n> + error_setg(errp, \"64-bit memory window requires \\\"mem64_win_size\\\"\");\n> + return;\n\nLikewise this new one.\n\n> } else if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {\n> /*\n> * For compatibility with old configuration, if no 64-bit MMIO\n> @@ -1594,6 +1563,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> sphb->mem64_win_pciaddr =\n> SPAPR_PCI_MEM_WIN_BUS_OFFSET + SPAPR_PCI_MEM32_WIN_SIZE;\n> sphb->mem_win_size = SPAPR_PCI_MEM32_WIN_SIZE;\n> + } else {\n> + /* Old configuration with the 32-bit MMIO window <= 2GiB don't need a\n> + * 64-bit MMIO window.\n> + */\n\nWouldn't it be easier to just change the test below to check for size\n!= 0 instead of checking for addr != -1 as it does now.\n\n> + sphb->mem64_win_addr = (hwaddr) -1;\n> + sphb->mem64_win_pciaddr = (hwaddr) -1;\n> }\n> \n> if (spapr_pci_find_phb(spapr, sphb->buid)) {\n> @@ -1789,18 +1764,12 @@ static void spapr_phb_reset(DeviceState *qdev)\n> \n> static Property spapr_phb_properties[] = {\n> DEFINE_PROP_UINT32(\"index\", sPAPRPHBState, index, -1),\n> - DEFINE_PROP_UINT64(\"buid\", sPAPRPHBState, buid, -1),\n> - DEFINE_PROP_UINT32(\"liobn\", sPAPRPHBState, dma_liobn[0], -1),\n> - DEFINE_PROP_UINT32(\"liobn64\", sPAPRPHBState, dma_liobn[1], -1),\n> - DEFINE_PROP_UINT64(\"mem_win_addr\", sPAPRPHBState, mem_win_addr, -1),\n> DEFINE_PROP_UINT64(\"mem_win_size\", sPAPRPHBState, mem_win_size,\n> SPAPR_PCI_MEM32_WIN_SIZE),\n> - DEFINE_PROP_UINT64(\"mem64_win_addr\", sPAPRPHBState, mem64_win_addr, -1),\n> DEFINE_PROP_UINT64(\"mem64_win_size\", sPAPRPHBState, mem64_win_size,\n> SPAPR_PCI_MEM64_WIN_SIZE),\n> DEFINE_PROP_UINT64(\"mem64_win_pciaddr\", sPAPRPHBState, mem64_win_pciaddr,\n> -1),\n> - DEFINE_PROP_UINT64(\"io_win_addr\", sPAPRPHBState, io_win_addr, -1),\n> DEFINE_PROP_UINT64(\"io_win_size\", sPAPRPHBState, io_win_size,\n> SPAPR_PCI_IO_WIN_SIZE),\n> DEFINE_PROP_BOOL(\"dynamic-reconfiguration\", sPAPRPHBState, dr_enabled,\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=gibson.dropbear.id.au\n\theader.i=@gibson.dropbear.id.au header.b=\"nNBJPT7I\"; \n\tdkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxKKM61Wbz9s82\n\tfor ;\n\tTue, 19 Sep 2017 20:45:43 +1000 (AEST)","from localhost ([::1]:41391 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duG29-0008L5-TS\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 06:45:41 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:51099)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duFtT-0001DF-Uo\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:36:47 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duFtR-0002nv-0Q\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:36:43 -0400","from ozlabs.org ([103.22.144.67]:35431)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duFtQ-0002jE-9E; Tue, 19 Sep 2017 06:36:40 -0400","by ozlabs.org (Postfix, from userid 1007)\n\tid 3xxK6p48KKz9t16; Tue, 19 Sep 2017 20:36:33 +1000 (AEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=gibson.dropbear.id.au; s=201602; t=1505817394;\n\tbh=UnFZ/vTgU/wBwNZ6S0MfgWdqk8Et2WSv5Kg/YRawU+Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nNBJPT7Ib/5SjP88n9T6+q3yHr1F47A9U0J6xGboA4gzrQskeGW//cd0c5MI0uWf2\n\tJL+3MgepIeRKZEIlCp2FxzYsH3rNsFnbx4eEz8HLQrliKV3JjMJep/ts0dA0yrsOrv\n\tShHQno1vvPzrL5+FDCKZDLcNdbKstiDztci6lwis=","Date":"Tue, 19 Sep 2017 09:03:58 +1000","From":"David Gibson ","To":"Greg Kurz ","Message-ID":"<20170918230358.GG27153@umbus>","References":"<150539846159.21523.16161730500010192093.stgit@bahia>\n\t<150539848430.21523.2732328570419779115.stgit@bahia>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"w2JjAQZceEVGylhD\"","Content-Disposition":"inline","In-Reply-To":"<150539848430.21523.2732328570419779115.stgit@bahia>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"103.22.144.67","Subject":"Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: make index property\n\tmandatory","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Alexey Kardashevskiy , Paolo Bonzini ,\n\tqemu-ppc@nongnu.org, qemu-devel@nongnu.org,\n\tMichael Roth ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771055,"web_url":"http://patchwork.ozlabs.org/comment/1771055/","msgid":"<20170919160357.082b991f@bahia.lab.toulouse-stg.fr.ibm.com>","list_archive_url":null,"date":"2017-09-19T14:03:57","subject":"Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: make index property\n\tmandatory","submitter":{"id":69178,"url":"http://patchwork.ozlabs.org/api/people/69178/","name":"Greg Kurz","email":"groug@kaod.org"},"content":"On Tue, 19 Sep 2017 09:03:58 +1000\nDavid Gibson wrote:\n\n> On Thu, Sep 14, 2017 at 04:14:44PM +0200, Greg Kurz wrote:\n> > Creating several PHBs without index property confuses the DRC code\n> > and causes issues:\n> > - only the first index-less PHB is functional, the other ones will\n> > silently ignore hotplugging of PCI devices\n> > - QEMU will even terminate if these PHBs have cold-plugged devices \n> \n> So, this is true, but it's kind of missing the point; we could fix\n> those things if we wanted. The real point here is that the non-index\n> option is more trouble than it's worth: it's difficult to use, keeps\n> requiring (potentially incompatible) changes when some new parameter\n> needs adding, and is awkward to check for collisions.\n> \n\nOk, I'll rewrite the changelog accordingly in the next spin.\n\n> > qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device\n> > is still awaiting release\n> > \n> > This happens because DR connectors for child PCI devices are created\n> > with a DRC index that is derived from the PHB's index property. If the\n> > PHBs are created without index, then the same value of -1 is used to\n> > compute the DRC indexes for both PHBs, hence causing the collision.\n> > \n> > Also, the index property is used to compute the placement of the PHB's\n> > memory regions. It is limited to 31 or 255, depending on the machine\n> > type version. This fits well with the requirements of DRC indexes, which\n> > need the PHB index to be a 16-bit value.\n> > \n> > This patch hence makes the index property mandatory. As a consequence,\n> > the PHB's memory regions and BUID are now always configured according\n> > to the index, and it is no longer possible to set them from the command\n> > line. We have to introduce a PHB instance init function to initialize\n> > the 64-bit window address to -1 because pseries-2.7 and older machines\n> > don't set it.\n> > \n> > This DOES BREAK backwards compat, but we don't think the non-index\n> > PHB feature was used in practice (at least libvirt doesn't) and the\n> > simplification is worth it.\n> > \n> > Signed-off-by: Greg Kurz \n> > ---\n> > v1->v2: - error out if mem64_win_pciaddr is set but mem64_win_size\n> > isn't\n> > - set mem64_win_addr to -1 for old configuration with 32-bit\n> > window below 2G in spapr_phb_realize()\n> > - drop instance init function\n> > \n> > RFC->v1: - as suggested dy David, updated the changelog to explicitely\n> > mention that we intentionally break backwards compat.\n> > ---\n> > hw/ppc/spapr_pci.c | 53 +++++++++++-----------------------------------------\n> > 1 file changed, 11 insertions(+), 42 deletions(-)\n> > \n> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c\n> > index cf54160526fa..024638e18b53 100644\n> > --- a/hw/ppc/spapr_pci.c\n> > +++ b/hw/ppc/spapr_pci.c\n> > @@ -1523,16 +1523,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);\n> > Error *local_err = NULL;\n> > \n> > - if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != (uint32_t)-1)\n> > - || (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2)\n> > - || (sphb->mem_win_addr != (hwaddr)-1)\n> > - || (sphb->mem64_win_addr != (hwaddr)-1)\n> > - || (sphb->io_win_addr != (hwaddr)-1)) {\n> > - error_setg(errp, \"Either \\\"index\\\" or other parameters must\"\n> > - \" be specified for PAPR PHB, not both\");\n> > - return;\n> > - }\n> > -\n> > smc->phb_placement(spapr, sphb->index,\n> > &sphb->buid, &sphb->io_win_addr,\n> > &sphb->mem_win_addr, &sphb->mem64_win_addr,\n> > @@ -1541,36 +1531,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> > error_propagate(errp, local_err);\n> > return;\n> > }\n> > - }\n> > -\n> > - if (sphb->buid == (uint64_t)-1) {\n> > - error_setg(errp, \"BUID not specified for PHB\");\n> > - return;\n> > - }\n> > -\n> > - if ((sphb->dma_liobn[0] == (uint32_t)-1) ||\n> > - ((sphb->dma_liobn[1] == (uint32_t)-1) && (windows_supported > 1))) {\n> > - error_setg(errp, \"LIOBN(s) not specified for PHB\");\n> > - return;\n> > - }\n> > -\n> > - if (sphb->mem_win_addr == (hwaddr)-1) {\n> > - error_setg(errp, \"Memory window address not specified for PHB\");\n> > - return;\n> > - }\n> > -\n> > - if (sphb->io_win_addr == (hwaddr)-1) {\n> > - error_setg(errp, \"IO window address not specified for PHB\");\n> > + } else {\n> > + error_setg(errp, \"\\\"index\\\" for PAPR PHB is mandatory\");\n> > return;\n> > }\n> > \n> > if (sphb->mem64_win_size != 0) {\n> > - if (sphb->mem64_win_addr == (hwaddr)-1) {\n> > - error_setg(errp,\n> > - \"64-bit memory window address not specified for PHB\");\n> > - return;\n> > - }\n> > -\n> > if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {\n> > error_setg(errp, \"32-bit memory window of size 0x%\"HWADDR_PRIx\n> > \" (max 2 GiB)\", sphb->mem_win_size); \n> \n> Some of these checks could probably just become assert()s now, where\n> they're things determined by the placement code and no longer have\n> parameters, a problem indicates a bug in the machine rather than a\n> user error.\n> \n\nAll the *_size properties are still user settable, as before... should we\nmake them internal and have the placement hook initialize them instead ?\n\n> > @@ -1581,6 +1547,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> > /* 64-bit window defaults to identity mapping */\n> > sphb->mem64_win_pciaddr = sphb->mem64_win_addr;\n> > }\n> > + } else if (sphb->mem64_win_pciaddr != (hwaddr) -1) {\n> > + error_setg(errp, \"64-bit memory window requires \\\"mem64_win_size\\\"\");\n> > + return; \n> \n> Likewise this new one.\n> \n\nmem64_win_pciaddr is still user settable as well...\n\n> > } else if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {\n> > /*\n> > * For compatibility with old configuration, if no 64-bit MMIO\n> > @@ -1594,6 +1563,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> > sphb->mem64_win_pciaddr =\n> > SPAPR_PCI_MEM_WIN_BUS_OFFSET + SPAPR_PCI_MEM32_WIN_SIZE;\n> > sphb->mem_win_size = SPAPR_PCI_MEM32_WIN_SIZE;\n> > + } else {\n> > + /* Old configuration with the 32-bit MMIO window <= 2GiB don't need a\n> > + * 64-bit MMIO window.\n> > + */ \n> \n> Wouldn't it be easier to just change the test below to check for size\n> != 0 instead of checking for addr != -1 as it does now.\n> \n\nYeah indeed.\n\n> > + sphb->mem64_win_addr = (hwaddr) -1;\n> > + sphb->mem64_win_pciaddr = (hwaddr) -1;\n> > }\n> > \n> > if (spapr_pci_find_phb(spapr, sphb->buid)) {\n> > @@ -1789,18 +1764,12 @@ static void spapr_phb_reset(DeviceState *qdev)\n> > \n> > static Property spapr_phb_properties[] = {\n> > DEFINE_PROP_UINT32(\"index\", sPAPRPHBState, index, -1),\n> > - DEFINE_PROP_UINT64(\"buid\", sPAPRPHBState, buid, -1),\n> > - DEFINE_PROP_UINT32(\"liobn\", sPAPRPHBState, dma_liobn[0], -1),\n> > - DEFINE_PROP_UINT32(\"liobn64\", sPAPRPHBState, dma_liobn[1], -1),\n> > - DEFINE_PROP_UINT64(\"mem_win_addr\", sPAPRPHBState, mem_win_addr, -1),\n> > DEFINE_PROP_UINT64(\"mem_win_size\", sPAPRPHBState, mem_win_size,\n> > SPAPR_PCI_MEM32_WIN_SIZE),\n> > - DEFINE_PROP_UINT64(\"mem64_win_addr\", sPAPRPHBState, mem64_win_addr, -1),\n> > DEFINE_PROP_UINT64(\"mem64_win_size\", sPAPRPHBState, mem64_win_size,\n> > SPAPR_PCI_MEM64_WIN_SIZE),\n> > DEFINE_PROP_UINT64(\"mem64_win_pciaddr\", sPAPRPHBState, mem64_win_pciaddr,\n> > -1),\n> > - DEFINE_PROP_UINT64(\"io_win_addr\", sPAPRPHBState, io_win_addr, -1),\n> > DEFINE_PROP_UINT64(\"io_win_size\", sPAPRPHBState, io_win_size,\n> > SPAPR_PCI_IO_WIN_SIZE),\n> > DEFINE_PROP_BOOL(\"dynamic-reconfiguration\", sPAPRPHBState, dr_enabled,\n> > \n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxPlq00KCz9s7h\n\tfor ;\n\tWed, 20 Sep 2017 00:05:25 +1000 (AEST)","from localhost ([::1]:43130 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duJ9P-0002Ku-3n\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 10:05:23 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:40119)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duJ90-0002KL-Fn\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:05:00 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duJ8v-0007cm-Tz\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:04:58 -0400","from 16.mo1.mail-out.ovh.net ([178.33.104.224]:41815)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1duJ8v-0007bj-Jq\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:04:53 -0400","from player691.ha.ovh.net (b9.ovh.net [213.186.33.59])\n\tby mo1.mail-out.ovh.net (Postfix) with ESMTP id 9225295A50\n\tfor ; Tue, 19 Sep 2017 16:04:51 +0200 (CEST)","from bahia.lab.toulouse-stg.fr.ibm.com\n\t(deibp9eh1--blueice1n3.emea.ibm.com [195.212.29.165])\n\t(Authenticated sender: groug@kaod.org)\n\tby player691.ha.ovh.net (Postfix) with ESMTPSA id F0B3E260133;\n\tTue, 19 Sep 2017 16:03:59 +0200 (CEST)"],"Date":"Tue, 19 Sep 2017 16:03:57 +0200","From":"Greg Kurz ","To":"David Gibson ","Message-ID":"<20170919160357.082b991f@bahia.lab.toulouse-stg.fr.ibm.com>","In-Reply-To":"<20170918230358.GG27153@umbus>","References":"<150539846159.21523.16161730500010192093.stgit@bahia>\n\t<150539848430.21523.2732328570419779115.stgit@bahia>\n\t<20170918230358.GG27153@umbus>","X-Mailer":"Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu)","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tboundary=\"Sig_/7EXtrxp/VpmTwXqaD8rMlwt\";\n\tprotocol=\"application/pgp-signature\"","X-Ovh-Tracer-Id":"2582251439370836454","X-VR-SPAMSTATE":"OK","X-VR-SPAMSCORE":"-100","X-VR-SPAMCAUSE":"gggruggvucftvghtrhhoucdtuddrfeelledrheejgdejfecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"178.33.104.224","Subject":"Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: make index property\n\tmandatory","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Alexey Kardashevskiy , Paolo Bonzini ,\n\tqemu-ppc@nongnu.org, qemu-devel@nongnu.org,\n\tMichael Roth ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771559,"web_url":"http://patchwork.ozlabs.org/comment/1771559/","msgid":"<20170920043125.GD5520@umbus.fritz.box>","list_archive_url":null,"date":"2017-09-20T04:31:25","subject":"Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: make index property\n\tmandatory","submitter":{"id":47,"url":"http://patchwork.ozlabs.org/api/people/47/","name":"David Gibson","email":"david@gibson.dropbear.id.au"},"content":"On Tue, Sep 19, 2017 at 04:03:57PM +0200, Greg Kurz wrote:\n> On Tue, 19 Sep 2017 09:03:58 +1000\n> David Gibson wrote:\n> \n> > On Thu, Sep 14, 2017 at 04:14:44PM +0200, Greg Kurz wrote:\n> > > Creating several PHBs without index property confuses the DRC code\n> > > and causes issues:\n> > > - only the first index-less PHB is functional, the other ones will\n> > > silently ignore hotplugging of PCI devices\n> > > - QEMU will even terminate if these PHBs have cold-plugged devices \n> > \n> > So, this is true, but it's kind of missing the point; we could fix\n> > those things if we wanted. The real point here is that the non-index\n> > option is more trouble than it's worth: it's difficult to use, keeps\n> > requiring (potentially incompatible) changes when some new parameter\n> > needs adding, and is awkward to check for collisions.\n> > \n> \n> Ok, I'll rewrite the changelog accordingly in the next spin.\n> \n> > > qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device\n> > > is still awaiting release\n> > > \n> > > This happens because DR connectors for child PCI devices are created\n> > > with a DRC index that is derived from the PHB's index property. If the\n> > > PHBs are created without index, then the same value of -1 is used to\n> > > compute the DRC indexes for both PHBs, hence causing the collision.\n> > > \n> > > Also, the index property is used to compute the placement of the PHB's\n> > > memory regions. It is limited to 31 or 255, depending on the machine\n> > > type version. This fits well with the requirements of DRC indexes, which\n> > > need the PHB index to be a 16-bit value.\n> > > \n> > > This patch hence makes the index property mandatory. As a consequence,\n> > > the PHB's memory regions and BUID are now always configured according\n> > > to the index, and it is no longer possible to set them from the command\n> > > line. We have to introduce a PHB instance init function to initialize\n> > > the 64-bit window address to -1 because pseries-2.7 and older machines\n> > > don't set it.\n> > > \n> > > This DOES BREAK backwards compat, but we don't think the non-index\n> > > PHB feature was used in practice (at least libvirt doesn't) and the\n> > > simplification is worth it.\n> > > \n> > > Signed-off-by: Greg Kurz \n> > > ---\n> > > v1->v2: - error out if mem64_win_pciaddr is set but mem64_win_size\n> > > isn't\n> > > - set mem64_win_addr to -1 for old configuration with 32-bit\n> > > window below 2G in spapr_phb_realize()\n> > > - drop instance init function\n> > > \n> > > RFC->v1: - as suggested dy David, updated the changelog to explicitely\n> > > mention that we intentionally break backwards compat.\n> > > ---\n> > > hw/ppc/spapr_pci.c | 53 +++++++++++-----------------------------------------\n> > > 1 file changed, 11 insertions(+), 42 deletions(-)\n> > > \n> > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c\n> > > index cf54160526fa..024638e18b53 100644\n> > > --- a/hw/ppc/spapr_pci.c\n> > > +++ b/hw/ppc/spapr_pci.c\n> > > @@ -1523,16 +1523,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> > > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);\n> > > Error *local_err = NULL;\n> > > \n> > > - if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != (uint32_t)-1)\n> > > - || (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2)\n> > > - || (sphb->mem_win_addr != (hwaddr)-1)\n> > > - || (sphb->mem64_win_addr != (hwaddr)-1)\n> > > - || (sphb->io_win_addr != (hwaddr)-1)) {\n> > > - error_setg(errp, \"Either \\\"index\\\" or other parameters must\"\n> > > - \" be specified for PAPR PHB, not both\");\n> > > - return;\n> > > - }\n> > > -\n> > > smc->phb_placement(spapr, sphb->index,\n> > > &sphb->buid, &sphb->io_win_addr,\n> > > &sphb->mem_win_addr, &sphb->mem64_win_addr,\n> > > @@ -1541,36 +1531,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> > > error_propagate(errp, local_err);\n> > > return;\n> > > }\n> > > - }\n> > > -\n> > > - if (sphb->buid == (uint64_t)-1) {\n> > > - error_setg(errp, \"BUID not specified for PHB\");\n> > > - return;\n> > > - }\n> > > -\n> > > - if ((sphb->dma_liobn[0] == (uint32_t)-1) ||\n> > > - ((sphb->dma_liobn[1] == (uint32_t)-1) && (windows_supported > 1))) {\n> > > - error_setg(errp, \"LIOBN(s) not specified for PHB\");\n> > > - return;\n> > > - }\n> > > -\n> > > - if (sphb->mem_win_addr == (hwaddr)-1) {\n> > > - error_setg(errp, \"Memory window address not specified for PHB\");\n> > > - return;\n> > > - }\n> > > -\n> > > - if (sphb->io_win_addr == (hwaddr)-1) {\n> > > - error_setg(errp, \"IO window address not specified for PHB\");\n> > > + } else {\n> > > + error_setg(errp, \"\\\"index\\\" for PAPR PHB is mandatory\");\n> > > return;\n> > > }\n> > > \n> > > if (sphb->mem64_win_size != 0) {\n> > > - if (sphb->mem64_win_addr == (hwaddr)-1) {\n> > > - error_setg(errp,\n> > > - \"64-bit memory window address not specified for PHB\");\n> > > - return;\n> > > - }\n> > > -\n> > > if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {\n> > > error_setg(errp, \"32-bit memory window of size 0x%\"HWADDR_PRIx\n> > > \" (max 2 GiB)\", sphb->mem_win_size); \n> > \n> > Some of these checks could probably just become assert()s now, where\n> > they're things determined by the placement code and no longer have\n> > parameters, a problem indicates a bug in the machine rather than a\n> > user error.\n> > \n> \n> All the *_size properties are still user settable, as before... should we\n> make them internal and have the placement hook initialize them instead ?\n\nAh, good point. I guess that still makes sense.\n\n> > > @@ -1581,6 +1547,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> > > /* 64-bit window defaults to identity mapping */\n> > > sphb->mem64_win_pciaddr = sphb->mem64_win_addr;\n> > > }\n> > > + } else if (sphb->mem64_win_pciaddr != (hwaddr) -1) {\n> > > + error_setg(errp, \"64-bit memory window requires \\\"mem64_win_size\\\"\");\n> > > + return; \n> > \n> > Likewise this new one.\n> > \n> \n> mem64_win_pciaddr is still user settable as well...\n\nThat doesn't sound like a good idea. If we're making index mandatory,\nwe might has well have it set *all* the windows.\n\n> \n> > > } else if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {\n> > > /*\n> > > * For compatibility with old configuration, if no 64-bit MMIO\n> > > @@ -1594,6 +1563,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)\n> > > sphb->mem64_win_pciaddr =\n> > > SPAPR_PCI_MEM_WIN_BUS_OFFSET + SPAPR_PCI_MEM32_WIN_SIZE;\n> > > sphb->mem_win_size = SPAPR_PCI_MEM32_WIN_SIZE;\n> > > + } else {\n> > > + /* Old configuration with the 32-bit MMIO window <= 2GiB don't need a\n> > > + * 64-bit MMIO window.\n> > > + */ \n> > \n> > Wouldn't it be easier to just change the test below to check for size\n> > != 0 instead of checking for addr != -1 as it does now.\n> > \n> \n> Yeah indeed.\n> \n> > > + sphb->mem64_win_addr = (hwaddr) -1;\n> > > + sphb->mem64_win_pciaddr = (hwaddr) -1;\n> > > }\n> > > \n> > > if (spapr_pci_find_phb(spapr, sphb->buid)) {\n> > > @@ -1789,18 +1764,12 @@ static void spapr_phb_reset(DeviceState *qdev)\n> > > \n> > > static Property spapr_phb_properties[] = {\n> > > DEFINE_PROP_UINT32(\"index\", sPAPRPHBState, index, -1),\n> > > - DEFINE_PROP_UINT64(\"buid\", sPAPRPHBState, buid, -1),\n> > > - DEFINE_PROP_UINT32(\"liobn\", sPAPRPHBState, dma_liobn[0], -1),\n> > > - DEFINE_PROP_UINT32(\"liobn64\", sPAPRPHBState, dma_liobn[1], -1),\n> > > - DEFINE_PROP_UINT64(\"mem_win_addr\", sPAPRPHBState, mem_win_addr, -1),\n> > > DEFINE_PROP_UINT64(\"mem_win_size\", sPAPRPHBState, mem_win_size,\n> > > SPAPR_PCI_MEM32_WIN_SIZE),\n> > > - DEFINE_PROP_UINT64(\"mem64_win_addr\", sPAPRPHBState, mem64_win_addr, -1),\n> > > DEFINE_PROP_UINT64(\"mem64_win_size\", sPAPRPHBState, mem64_win_size,\n> > > SPAPR_PCI_MEM64_WIN_SIZE),\n> > > DEFINE_PROP_UINT64(\"mem64_win_pciaddr\", sPAPRPHBState, mem64_win_pciaddr,\n> > > -1),\n> > > - DEFINE_PROP_UINT64(\"io_win_addr\", sPAPRPHBState, io_win_addr, -1),\n> > > DEFINE_PROP_UINT64(\"io_win_size\", sPAPRPHBState, io_win_size,\n> > > SPAPR_PCI_IO_WIN_SIZE),\n> > > DEFINE_PROP_BOOL(\"dynamic-reconfiguration\", sPAPRPHBState, dr_enabled,\n> > > \n> > \n> \n> \n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=gibson.dropbear.id.au\n\theader.i=@gibson.dropbear.id.au header.b=\"ckSBY5YM\"; \n\tdkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxnTt3Dbhz9sBW\n\tfor ;\n\tWed, 20 Sep 2017 14:54:42 +1000 (AEST)","from localhost ([::1]:46674 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duX20-0007xs-IE\n\tfor incoming@patchwork.ozlabs.org; Wed, 20 Sep 2017 00:54:40 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36194)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duX1E-0007wN-Jx\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 00:53:54 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duX1B-0001Jb-A5\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 00:53:52 -0400","from ozlabs.org ([103.22.144.67]:52689)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duX1A-00013r-Gt; Wed, 20 Sep 2017 00:53:49 -0400","by ozlabs.org (Postfix, from userid 1007)\n\tid 3xxnSm4bp6z9s82; Wed, 20 Sep 2017 14:53:44 +1000 (AEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=gibson.dropbear.id.au; s=201602; t=1505883224;\n\tbh=RKioRG1SUCYU41IveqsmjglHO3loSlLKZHyozNO5y8I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ckSBY5YMm8E0h12dhboIEbEJNccaasT8tGOc5tEQWB0ogNT8/hTXhH1SWPi9BBvI6\n\tMeIFduJIVDwaRVPHmBxC5i42biztEYF37MLxTPlX1vKOjiHyjjvkWV26rrgdDgpZWc\n\twshyc+yNcex7ZcjiPqT9uS2+EiuHm60wlE75OWRM=","Date":"Wed, 20 Sep 2017 14:31:25 +1000","From":"David Gibson ","To":"Greg Kurz ","Message-ID":"<20170920043125.GD5520@umbus.fritz.box>","References":"<150539846159.21523.16161730500010192093.stgit@bahia>\n\t<150539848430.21523.2732328570419779115.stgit@bahia>\n\t<20170918230358.GG27153@umbus>\n\t<20170919160357.082b991f@bahia.lab.toulouse-stg.fr.ibm.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"k4f25fnPtRuIRUb3\"","Content-Disposition":"inline","In-Reply-To":"<20170919160357.082b991f@bahia.lab.toulouse-stg.fr.ibm.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"103.22.144.67","Subject":"Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: make index property\n\tmandatory","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Alexey Kardashevskiy , Paolo Bonzini ,\n\tqemu-ppc@nongnu.org, qemu-devel@nongnu.org,\n\tMichael Roth ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]