| Message ID | 20211110211140.3057199-2-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for,6.2,v2,1/5] pcie: rename 'native-hotplug' to 'x-native-hotplug' | expand |
On Wed, 10 Nov 2021, Igor Mammedov wrote: > Mark property as experimental/internal adding 'x-' prefix. > > Property was introduced in 6.1 and it should have provided > ability to turn on native PCIE hotplug on port even when > ACPI PCI hotplug is in use is user explicitly sets property > on CLI. However that never worked since slot is wired to > ACPI hotplug controller. > Another non-intended usecase: disable native hotplug on slot > when APCI based hotplug is disabled, which works but slot has > 'hotplug' property for this taks. > > It should be relatively safe to rename it to experimental > as no users should exist for it and given that the property > is broken we don't really want to leave it around for much > longer lest users start using it. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Barring the comment below, Reviewed-by: Ani Sinha <ani@anisinha.ca> > --- > CC: qemu-stable@nongnu.org > --- > hw/i386/pc_q35.c | 2 +- > hw/pci/pcie_port.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 797e09500b..fc34b905ee 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -243,7 +243,7 @@ static void pc_q35_init(MachineState *machine) > NULL); > > if (acpi_pcihp) { > - object_register_sugar_prop(TYPE_PCIE_SLOT, "native-hotplug", > + object_register_sugar_prop(TYPE_PCIE_SLOT, "x-native-hotplug", > "false", true); Let us document the fact that this property is experimental. It was not at once obvious to me that an "x-" prefix meant to indicate experimental status. > } > > diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c > index da850e8dde..e95c1e5519 100644 > --- a/hw/pci/pcie_port.c > +++ b/hw/pci/pcie_port.c > @@ -148,7 +148,7 @@ static Property pcie_slot_props[] = { > DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0), > DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0), > DEFINE_PROP_BOOL("hotplug", PCIESlot, hotplug, true), > - DEFINE_PROP_BOOL("native-hotplug", PCIESlot, native_hotplug, true), > + DEFINE_PROP_BOOL("x-native-hotplug", PCIESlot, native_hotplug, true), > DEFINE_PROP_END_OF_LIST() > }; > > -- > 2.27.0 > >
On Thu, 11 Nov 2021 08:55:24 +0530 (IST) Ani Sinha <ani@anisinha.ca> wrote: > On Wed, 10 Nov 2021, Igor Mammedov wrote: > > > Mark property as experimental/internal adding 'x-' prefix. > > > > Property was introduced in 6.1 and it should have provided > > ability to turn on native PCIE hotplug on port even when > > ACPI PCI hotplug is in use is user explicitly sets property > > on CLI. However that never worked since slot is wired to > > ACPI hotplug controller. > > Another non-intended usecase: disable native hotplug on slot > > when APCI based hotplug is disabled, which works but slot has > > 'hotplug' property for this taks. > > > > It should be relatively safe to rename it to experimental > > as no users should exist for it and given that the property > > is broken we don't really want to leave it around for much > > longer lest users start using it. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Barring the comment below, > > Reviewed-by: Ani Sinha <ani@anisinha.ca> Thanks! > > > --- > > CC: qemu-stable@nongnu.org > > --- > > hw/i386/pc_q35.c | 2 +- > > hw/pci/pcie_port.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > > index 797e09500b..fc34b905ee 100644 > > --- a/hw/i386/pc_q35.c > > +++ b/hw/i386/pc_q35.c > > @@ -243,7 +243,7 @@ static void pc_q35_init(MachineState *machine) > > NULL); > > > > if (acpi_pcihp) { > > - object_register_sugar_prop(TYPE_PCIE_SLOT, "native-hotplug", > > + object_register_sugar_prop(TYPE_PCIE_SLOT, "x-native-hotplug", > > "false", true); > > Let us document the fact that this property is experimental. It was not at > once obvious to me that an "x-" prefix meant to indicate experimental > status. it's common knowledge, but quick grep shows we only documented x- prefix for qmp commands but not for properties even though properties were the first to use it. So we probably should document it somewhere. I thought we have acceptable property name format documented but I couldn't find it quickly (that would be a good place to document it). Care to post a patch? > > > > } > > > > diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c > > index da850e8dde..e95c1e5519 100644 > > --- a/hw/pci/pcie_port.c > > +++ b/hw/pci/pcie_port.c > > @@ -148,7 +148,7 @@ static Property pcie_slot_props[] = { > > DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0), > > DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0), > > DEFINE_PROP_BOOL("hotplug", PCIESlot, hotplug, true), > > - DEFINE_PROP_BOOL("native-hotplug", PCIESlot, native_hotplug, true), > > + DEFINE_PROP_BOOL("x-native-hotplug", PCIESlot, native_hotplug, true), > > DEFINE_PROP_END_OF_LIST() > > }; > > > > -- > > 2.27.0 > > > > >
On Fri, 12 Nov 2021, Igor Mammedov wrote: > On Thu, 11 Nov 2021 08:55:24 +0530 (IST) > Ani Sinha <ani@anisinha.ca> wrote: > > > On Wed, 10 Nov 2021, Igor Mammedov wrote: > > > > > Mark property as experimental/internal adding 'x-' prefix. > > > > > > Property was introduced in 6.1 and it should have provided > > > ability to turn on native PCIE hotplug on port even when > > > ACPI PCI hotplug is in use is user explicitly sets property > > > on CLI. However that never worked since slot is wired to > > > ACPI hotplug controller. > > > Another non-intended usecase: disable native hotplug on slot > > > when APCI based hotplug is disabled, which works but slot has > > > 'hotplug' property for this taks. > > > > > > It should be relatively safe to rename it to experimental > > > as no users should exist for it and given that the property > > > is broken we don't really want to leave it around for much > > > longer lest users start using it. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > Barring the comment below, > > > > Reviewed-by: Ani Sinha <ani@anisinha.ca> > > Thanks! > > > > > > --- > > > CC: qemu-stable@nongnu.org > > > --- > > > hw/i386/pc_q35.c | 2 +- > > > hw/pci/pcie_port.c | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > > > index 797e09500b..fc34b905ee 100644 > > > --- a/hw/i386/pc_q35.c > > > +++ b/hw/i386/pc_q35.c > > > @@ -243,7 +243,7 @@ static void pc_q35_init(MachineState *machine) > > > NULL); > > > > > > if (acpi_pcihp) { > > > - object_register_sugar_prop(TYPE_PCIE_SLOT, "native-hotplug", > > > + object_register_sugar_prop(TYPE_PCIE_SLOT, "x-native-hotplug", > > > "false", true); > > > > Let us document the fact that this property is experimental. It was not at > > once obvious to me that an "x-" prefix meant to indicate experimental > > status. > > it's common knowledge, but quick grep shows we only documented > x- prefix for qmp commands but not for properties even though > properties were the first to use it. So we probably should > document it somewhere. > I thought we have acceptable property name format documented sadly I could not find it either. > but I couldn't find it quickly (that would be a good place > to document it). > Care to post a patch? > > > > > > > > } > > > > > > diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c > > > index da850e8dde..e95c1e5519 100644 > > > --- a/hw/pci/pcie_port.c > > > +++ b/hw/pci/pcie_port.c > > > @@ -148,7 +148,7 @@ static Property pcie_slot_props[] = { > > > DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0), > > > DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0), > > > DEFINE_PROP_BOOL("hotplug", PCIESlot, hotplug, true), > > > - DEFINE_PROP_BOOL("native-hotplug", PCIESlot, native_hotplug, true), > > > + DEFINE_PROP_BOOL("x-native-hotplug", PCIESlot, native_hotplug, true), > > > DEFINE_PROP_END_OF_LIST() > > > }; > > > > > > -- > > > 2.27.0 > > > > > > > > > >
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 797e09500b..fc34b905ee 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -243,7 +243,7 @@ static void pc_q35_init(MachineState *machine) NULL); if (acpi_pcihp) { - object_register_sugar_prop(TYPE_PCIE_SLOT, "native-hotplug", + object_register_sugar_prop(TYPE_PCIE_SLOT, "x-native-hotplug", "false", true); } diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c index da850e8dde..e95c1e5519 100644 --- a/hw/pci/pcie_port.c +++ b/hw/pci/pcie_port.c @@ -148,7 +148,7 @@ static Property pcie_slot_props[] = { DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0), DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0), DEFINE_PROP_BOOL("hotplug", PCIESlot, hotplug, true), - DEFINE_PROP_BOOL("native-hotplug", PCIESlot, native_hotplug, true), + DEFINE_PROP_BOOL("x-native-hotplug", PCIESlot, native_hotplug, true), DEFINE_PROP_END_OF_LIST() };
Mark property as experimental/internal adding 'x-' prefix. Property was introduced in 6.1 and it should have provided ability to turn on native PCIE hotplug on port even when ACPI PCI hotplug is in use is user explicitly sets property on CLI. However that never worked since slot is wired to ACPI hotplug controller. Another non-intended usecase: disable native hotplug on slot when APCI based hotplug is disabled, which works but slot has 'hotplug' property for this taks. It should be relatively safe to rename it to experimental as no users should exist for it and given that the property is broken we don't really want to leave it around for much longer lest users start using it. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- CC: qemu-stable@nongnu.org --- hw/i386/pc_q35.c | 2 +- hw/pci/pcie_port.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)