On Thu, Jul 09, 2020 at 07:12:47PM +0200, Greg Kurz wrote: > Some recent error handling cleanups unveiled issues with our support of > PCI bridges: > > 1) QEMU aborts when using non-standard PCI bridge types, > unveiled by commit 7ef1553dac "spapr_pci: Drop some dead error handling" > > $ qemu-system-ppc64 -M pseries -device pcie-pci-bridge > Unexpected error in object_property_find() at qom/object.c:1240: > qemu-system-ppc64: -device pcie-pci-bridge: Property '.chassis_nr' not found > Aborted (core dumped)
Oops, I thought we had a check that we actually had a "pci-bridge"
device before continuing with the hotplug, but I guess not.
> This happens because we assume all PCI bridge types to have a "chassis_nr"
> property. This property only exists with the standard PCI bridge type
> "pci-bridge" actually. We could possibly revert 7ef1553dac but it seems
> much simpler to check the presence of "chassis_nr" earlier.
Hrm, right, 7ef1553dac was not really correct since add_drcs() really
can fail.
> 2) QEMU abort if same "chassis_nr" value is used several times,
> unveiled by commit d2623129a7de "qom: Drop parameter @errp of
> object_property_add() & friends"
>
> $ qemu-system-ppc64 -M pseries -device pci-bridge,chassis_nr=1 \
> -device pci-bridge,chassis_nr=1
> Unexpected error in object_property_try_add() at qom/object.c:1167:
> qemu-system-ppc64: -device pci-bridge,chassis_nr=1: attempt to add duplicate
> property '40000100' to object (type 'container')
> Aborted (core dumped)
>
> This happens because we assume that "chassis_nr" values are unique, but
> nobody enforces that and we end up generating duplicate DRC ids. The PCI
> code doesn't really care for duplicate "chassis_nr" properties since it
> is only used to initialize the "Chassis Number Register" of the bridge,
> with no functional impact on QEMU. So, even if passing the same value
> several times might look weird, it never broke anything before, so
> I guess we don't necessarily want to enforce strict checking in the PCI
> code now.
Yeah, I guess. I'm pretty sure that the chassis number of bridges is
supposed to be system-unique (well, unique within the PCI domain at
least, I guess) as part of the hardware spec. So specifying multiple
chassis ids the same is a user error, but we need a better failure
mode.
> Workaround both issues in the PAPR code: check that the bridge has a
> unique and non null "chassis_nr" when plugging it into its parent bus.
>
> Fixes: 05929a6c5dfe ("spapr: Don't use bus number for building DRC ids")
Arguably, it's really fixing 7ef1553dac.
> Reported-by: Thomas Huth <th...@redhat.com>
> Signed-off-by: Greg Kurz <gr...@kaod.org>
I had a few misgivings about the details of this, but I think I've
convinced myself they're fine. There's a couple of things I'd like to
polish, but I'll do that as a follow up.
> ---
> hw/ppc/spapr_pci.c | 57
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 329002ac040e..09d52ef7954d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1480,6 +1480,57 @@ static void spapr_pci_bridge_plug(SpaprPhbState *phb,
> add_drcs(phb, bus);
> }
>
> +/* Returns non-zero if the value of "chassis_nr" is already in use */
> +static int check_chassis_nr(Object *obj, void *opaque)
> +{
> + int new_chassis_nr =
> + object_property_get_uint(opaque, "chassis_nr", &error_abort);
> + int chassis_nr =
> + object_property_get_uint(obj, "chassis_nr", NULL);
> +
> + if (!object_dynamic_cast(obj, TYPE_PCI_BRIDGE)) {
> + return 0;
> + }
> +
> + /* Skip unsupported bridge types */
> + if (!chassis_nr) {
> + return 0;
> + }
> +
> + /* Skip self */
> + if (obj == opaque) {
> + return 0;
> + }
> +
> + return chassis_nr == new_chassis_nr;
> +}
> +
> +static bool bridge_has_valid_chassis_nr(Object *bridge, Error **errp)
> +{
> + int chassis_nr =
> + object_property_get_uint(bridge, "chassis_nr", NULL);
> +
> + /*
> + * slotid_cap_init() already ensures that "chassis_nr" isn't null for
> + * standard PCI bridges, so this really tells if "chassis_nr" is present
> + * or not.
> + */
> + if (!chassis_nr) {
> + error_setg(errp, "PCI Bridge lacks a \"chassis_nr\" property");
> + error_append_hint(errp, "Try -device pci-bridge instead.\n");
> + return false;
> + }
> +
> + /* We want unique values for "chassis_nr" */
> + if (object_child_foreach_recursive(object_get_root(), check_chassis_nr,
> + bridge)) {
> + error_setg(errp, "Bridge chassis %d already in use", chassis_nr);
> + return false;
> + }
> +
> + return true;
> +}
> +
> static void spapr_pci_plug(HotplugHandler *plug_handler,
> DeviceState *plugged_dev, Error **errp)
> {
> @@ -1491,6 +1542,12 @@ static void spapr_pci_plug(HotplugHandler
> *plug_handler,
> PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
> uint32_t slotnr = PCI_SLOT(pdev->devfn);
>
> + if (pc->is_bridge) {
> + if (!bridge_has_valid_chassis_nr(OBJECT(plugged_dev), errp)) {
> + return;
> + }
> + }
> +
> /* if DR is disabled we don't need to do anything in the case of
> * hotplug or coldplug callbacks
> */
>
>
--
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
signature.asc
Description: PGP signature
