On Wed, Apr 29, 2020 at 2:15 AM Michael S. Tsirkin <m...@redhat.com> wrote:
> On Tue, Apr 28, 2020 at 10:10:18PM +0530, Ani Sinha wrote:
> >
> >
> > On Tue, Apr 28, 2020 at 9:51 PM Michael S. Tsirkin <m...@redhat.com>
> wrote:
> >
> > On Tue, Apr 28, 2020 at 09:39:16PM +0530, Ani Sinha wrote:
> > >
> > > Ani
> > > On Apr 28, 2020, 21:35 +0530, Michael S. Tsirkin <m...@redhat.com>,
> wrote:
> > >
> > > On Tue, Apr 28, 2020 at 10:16:52AM +0000, Ani Sinha wrote:
> > >
> > > A new option "use_acpi_unplug" is introduced for PIIX
> which will
> > > selectively only disable hot unplugging of both hot
> plugged and
> > > cold plugged PCI devices on non-root PCI buses. This will
> prevent
> > > hot unplugging of devices from Windows based guests from
> system
> > > tray but will not prevent devices from being hot plugged
> into the
> > > guest.
> > >
> > > It has been tested on Windows guests.
> > >
> > > Signed-off-by: Ani Sinha <ani.si...@nutanix.com>
> > >
> > >
> > > It's still a non starter until we find something similar for
> PCIE and
> > > SHPC. Do guests check command status? Can some unplug commands
> fail?
> > >
> > >
> > > Ok I give up! I thought we debated this on the other thread.
> >
> > Sorry to hear that.
> > I'd rather you didn't, and worked on a solution that works for
> everyone.
> >
> >
> > That is extremely hard for one person to do, without inputs and ideas
> from the
> > community.
>
> What kind of input are you looking for?
Well there were several discussions in the other thread around how PCIE
behaves and how we can't change the slot features without a HW reset. Those
were useful inputs.
The approach you are taking as a maintainer is very discouraging. All I
have gotten from you is negativity and push back. There has been no other
engagement from you. If you expect one person to fix every use case, that
is an unrealistic expectation IMHO. Even if I could come up with a solution
for every case, testing every use case is a huge investment in time and
effort. No one outside the big distros will be motivated to do that. So
involvement from outside the distro community will be limited to minor
changes, bug fixes and easy code reworks.
My 2 cents.
>
> > Satisfying the entire world requires lot of time and energy
> > investment, not to mention a broad expertise in multiple technologies.
> >
> >
> >
> > Pushing back on merging code is unfortunately the only mechanism
> > maintainers have to make sure features are complete and
> > orthogonal to each other, so I'm not sure I can help otherwise.
> >
> > >
> > >
> > >
> > >
> > > ---
> > > hw/acpi/piix4.c | 3 +++
> > > hw/i386/acpi-build.c | 40
> > ++++++++++++++++++++++++++--------------
> > > 2 files changed, 29 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index 964d6f5..59fa707 100644
> > > --- a/hw/acpi/piix4.c
> > > +++ b/hw/acpi/piix4.c
> > > @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
> > >
> > > AcpiPciHpState acpi_pci_hotplug;
> > > bool use_acpi_pci_hotplug;
> > > + bool use_acpi_unplug;
> > >
> > > uint8_t disable_s3;
> > > uint8_t disable_s4;
> > > @@ -633,6 +634,8 @@ static Property piix4_pm_properties[]
> = {
> > > DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState,
> s4_val, 2),
> > > DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support",
> > PIIX4PMState,
> > > use_acpi_pci_hotplug, true),
> > > + DEFINE_PROP_BOOL("acpi-pci-hotunplug-enable-bridge",
> > PIIX4PMState,
> > > + use_acpi_unplug, true),
> > > DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> > > acpi_memory_hotplug.is_enabled, true),
> > > DEFINE_PROP_END_OF_LIST(),
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 23c77ee..71b3ac3 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -96,6 +96,7 @@ typedef struct AcpiPmInfo {
> > > bool s3_disabled;
> > > bool s4_disabled;
> > > bool pcihp_bridge_en;
> > > + bool pcihup_bridge_en;
> > > uint8_t s4_val;
> > > AcpiFadtData fadt;
> > > uint16_t cpu_hp_io_base;
> > > @@ -240,6 +241,9 @@ static void
> acpi_get_pm_info(MachineState
> > *machine,
> > > AcpiPmInfo *pm)
> > > pm->pcihp_bridge_en =
> > > object_property_get_bool(obj,
> > "acpi-pci-hotplug-with-bridge-support",
> > > NULL);
> > > + pm->pcihup_bridge_en =
> > > + object_property_get_bool(obj,
> > "acpi-pci-hotunplug-enable-bridge",
> > > + NULL);
> > > }
> > >
> > > static void acpi_get_misc_info(AcpiMiscInfo *info)
> > > @@ -451,7 +455,8 @@ static void
> build_append_pcihp_notify_entry
> > (Aml
> > > *method, int slot)
> > > }
> > >
> > > static void build_append_pci_bus_devices(Aml *parent_scope,
> > PCIBus
> > > *bus,
> > > - bool pcihp_bridge_en)
> > > + bool pcihp_bridge_en,
> > > + bool pcihup_bridge_en)
> > > {
> > > Aml *dev, *notify_method = NULL, *method;
> > > QObject *bsel;
> > > @@ -479,11 +484,14 @@ static void
> build_append_pci_bus_devices
> > (Aml
> > > *parent_scope, PCIBus *bus,
> > > dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
> > > aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
> > > aml_append(dev, aml_name_decl("_ADR", aml_int(slot <<
> 16)));
> > > - method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> > > - aml_append(method,
> > > - aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN"))
> > > - );
> > > - aml_append(dev, method);
> > > + if (pcihup_bridge_en || pci_bus_is_root(bus)) {
> > > + method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> > > + aml_append(method,
> > > + aml_call2("PCEJ", aml_name("BSEL"),
> > > + aml_name("_SUN"))
> > > + );
> > > + aml_append(dev, method);
> > > + }
> > > aml_append(parent_scope, dev);
> > >
> > > build_append_pcihp_notify_entry(notify_method, slot);
> > > @@ -537,12 +545,14 @@ static void
> build_append_pci_bus_devices
> > (Aml
> > > *parent_scope, PCIBus *bus,
> > > /* add _SUN/_EJ0 to make slot hotpluggable */
> > > aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
> > >
> > > - method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> > > - aml_append(method,
> > > - aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN"))
> > > - );
> > > - aml_append(dev, method);
> > > -
> > > + if (pcihup_bridge_en || pci_bus_is_root(bus)) {
> > > + method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> > > + aml_append(method,
> > > + aml_call2("PCEJ", aml_name("BSEL"),
> > > + aml_name("_SUN"))
> > > + );
> > > + aml_append(dev, method);
> > > + }
> > > if (bsel) {
> > > build_append_pcihp_notify_entry(notify_method, slot);
> > > }
> > > @@ -553,7 +563,8 @@ static void
> build_append_pci_bus_devices(Aml
> > > *parent_scope, PCIBus *bus,
> > > */
> > > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> > >
> > > - build_append_pci_bus_devices(dev, sec_bus,
> pcihp_bridge_en);
> > > + build_append_pci_bus_devices(dev, sec_bus,
> pcihp_bridge_en,
> > > + pcihup_bridge_en);
> > > }
> > > /* slot descriptor has been composed, add it into parent
> context
> > */
> > > aml_append(parent_scope, dev);
> > > @@ -2196,7 +2207,8 @@ build_dsdt(GArray *table_data,
> BIOSLinker
> > > *linker,
> > > if (bus) {
> > > Aml *scope = aml_scope("PCI0");
> > > /* Scan all PCI buses. Generate tables to support hotplug.
> */
> > > - build_append_pci_bus_devices(scope, bus,
> pm->pcihp_bridge_en);
> > > + build_append_pci_bus_devices(scope, bus,
> pm->pcihp_bridge_en,
> > > + pm->pcihup_bridge_en);
> > >
> > > if (TPM_IS_TIS_ISA(tpm)) {
> > > if (misc->tpm_version == TPM_VERSION_2_0) {
> > > --
> > > 1.9.4
> > >
> > >
> > >
> >
> >
>
>
