On Wed, 10 Nov 2021 09:08:34 +0000
Daniel P. Berrangé <berra...@redhat.com> wrote:
> On Wed, Nov 10, 2021 at 01:04:54AM -0500, Michael S. Tsirkin wrote:
> > On Wed, Nov 10, 2021 at 06:30:10AM +0100, Julia Suvorova wrote:
> > > Rename the option to better represent its function - toggle Hot-Plug
> > > Capable bit in the PCIe Slot Capability.
> > >
> > > Signed-off-by: Julia Suvorova <jus...@redhat.com>
> > > ---
> > > include/hw/pci/pcie_port.h | 2 +-
> > > hw/i386/pc_q35.c | 2 +-
> > > hw/pci-bridge/gen_pcie_root_port.c | 6 +++++-
> > > hw/pci/pcie.c | 2 +-
> > > hw/pci/pcie_port.c | 2 +-
> > > 5 files changed, 9 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> > > index e25b289ce8..0da6d66c95 100644
> > > --- a/include/hw/pci/pcie_port.h
> > > +++ b/include/hw/pci/pcie_port.h
> > > @@ -60,7 +60,7 @@ struct PCIESlot {
> > > /* Indicates whether any type of hot-plug is allowed on the slot */
> > > bool hotplug;
> > >
> > > - bool native_hotplug;
> > > + bool native_hpc_bit;
> > >
> > > QLIST_ENTRY(PCIESlot) next;
> > > };
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >
> >
> > This is ok.
> >
> >
> > > index 797e09500b..ded61f8ef7 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, "native-hpc-bit",
> > > "false", true);
> > > }
> > >
> >
> >
> > This part is problematic since we made the feature user-settable in 6.1.
> > We can have two features if we really want to ...
> > I don't think we have a way to mark properties deprecated, do we?
describe them in deprecated.rst
>
> IMHO just leave the feature with its current name. It won't be the
> first thing without a "perfect" name and the name doesn't have a
> negative impact on mgmt apps. Changing the name will cause more
> pain than it solves.
looking at the code, it was introduced in 6.1 for internal needs mostly
3f3cbbb23 hw/pci/pcie: Do not set HPC flag if acpihp is used
to hide slot from guest's native-hotplug module.
Even if user tried explicitly to set native-hoplug=on on a slot
with ACPI hotplug enabled, native hotplug will not work properly
as slot's hotplug is still handled by acpi_pcihp_device_plug_cb().
Given above and that it shouldn't been used by users,
I'd rather rename it to x-native-hotplug now than go through deprecation
process and expose it for 2 more releases.
> >
> > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c
> > > b/hw/pci-bridge/gen_pcie_root_port.c
> > > index 20099a8ae3..ed079d72b3 100644
> > > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > > @@ -87,7 +87,11 @@ static void gen_rp_realize(DeviceState *dev, Error
> > > **errp)
> > > return;
> > > }
> > >
> > > - if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hotplug) {
> > > + /*
> > > + * Make sure that IO is assigned in case the slot is hot-pluggable
> > > + * but it is not visible through the PCIe Slot Capabilities
> > > + */
> > > + if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hpc_bit) {
> > > grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE;
> > > }
> > > int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 914a9bf3d1..ebe002831e 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -535,7 +535,7 @@ void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
> > > * hot-plug is disabled on the slot.
> > > */
> > > if (s->hotplug &&
> > > - (s->native_hotplug || DEVICE(dev)->hotplugged)) {
> > > + (s->native_hpc_bit || DEVICE(dev)->hotplugged)) {
> > > pci_long_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCAP,
> > > PCI_EXP_SLTCAP_HPS |
> > > PCI_EXP_SLTCAP_HPC);
> > > diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
> > > index da850e8dde..eae06d10e2 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("native-hpc-bit", PCIESlot, native_hpc_bit, true),
> > > DEFINE_PROP_END_OF_LIST()
> > > };
> > >
> > > --
> > > 2.31.1
> >
> >
>
> Regards,
> Daniel
