On Wed, Sep 29, 2021 at 7:58 PM Igor Mammedov <imammedo@xxxxxxxxxx> wrote:
>
> On Tue, 28 Sep 2021 11:47:26 +0100
> Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote:
>
> > On Tue, Sep 28, 2021 at 03:28:04PM +0530, Ani Sinha wrote:
> > >
> > >
> > > On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
> > >
> > > > On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
> > > > >
> > > > >
> > > > > On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
> > > > >
> > > > > > On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
> > > > > > > This change introduces libvirt xml support for the following two pm options:
> [...]
> > > > > The <target hotplug="on/off"> switch in libvirt for pcie-root-ports
> > > > > currently does not care whether native or acpi hotplug is used. It simply
> > > > > turns on the hotplug for that particular port. Whether ACPI or native is
> > > > > used is controlled by this global flag that Julia has introduced in 6.1.
>
>
> > > > Right so we have
> > > >
>
> *1*)
> following applies to piix4/q35:
> * ACPI hotplug when enabled, affects _only_ cold-plugged 'bridges'
> since it requires 'slots' being described in DSDT table which
> in current impl. is static table built at reset time.
>
> (i.e. built-in or 'bridges' specified on command line,
> where 'bridges' could be PCI-PCI or PCIe-PCI or root/downstream-ports')
> for anything else ('bridges' added with device_add) native hotplug
> is in use (whether it's SHPC or PCI-E native).
>
> ACPI hotplug wiring is done by calling qbus_set_hotplug_handler()
> * for root bus piix4_pm_realize()/ich9_pm_init()
> * for anything else acpi_pcihp_device_plug_cb()
>
>
> > > > * PIIX4
> > > >
> > > > - acpi-root-pci-hotplug=bool
> > > >
> > > > Whether hotplug is enabled for the root bridge or not
> > > >
> > > > <target hotplug="on/off"> for pci-root controller
> > > >
> > > >
> > > > - acpi-pci-hotplug-with-bridge-support=bool
> > > >
> > > > Toggles support for ACPI based hotplug across all bridges.
> > > > If disabled will there will be no hotplug at all for PIIX4 ?
> > > > Or does 'shpc' come into play in that scenario ?
>
> 'SHPC' hotplug kicks in if it's enabled. (defaults to 'on' except 2.9 machine type)
>
> on q35/APCI side of things we always advertise -all_ hotplug methods available
>
> build_q35_osc_method()
> /*
> * Always allow native PME, AER (no dependencies)
> * Allow SHPC (PCI bridges can have SHPC controller)
> */
> aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
>
> bits 0, 1 are Native PCI-E hotplug and SHPC respectively
>
> for PIIX4 we don't have _OSC so it's up to guest OS to make up
> supported methods.
>
> In order of preference:
> * Windows supports ACPI hotplug then Native PCI-E (SHPC never worked there)
Hmm. from https://www.mail-archive.com/qemu-devel@xxxxxxxxxx/msg746673.html :
Me:
> Stupid question. If both native and acpi is enabled which one does OS chose?
> Does this choice vary between OSes and different flavours of the same OS like
> Windows?
Julia: It will always choose native.
re: SHPC this is the reason I thought SHPC was disabled. In my
experiments, Windows did not seem to care about SHPC.
> * Linux supports ACPI hotplug, SHPC, Native PCI-E
> (SHPC worked poorly due to need to reserve IO for bridges
> io reservation hinting (impl. later by Marcel))
>
> > > > PIIX combinations
> > > >
> > > > (1) acpi-root-pci-hotplug=yes
> > > > acpi-pci-hotplug-with-bridge-support=yes
> > > >
> > > > - All bridges have hotplug
> > > >
> > > > (2) acpi-root-pci-hotplug=yes
> > > > acpi-pci-hotplug-with-bridge-support=no
> > > >
> > > > - No bridges have hotplug
> > > >
> > > > (3) acpi-root-pci-hotplug=no
> > > > acpi-pci-hotplug-with-bridge-support=yes
> > > >
> > > > - All bridges except root have hotplug
>
> requested by Promox guys, to battle against Windows 'feature' that
> lets any user to unplug sole NIC using an icon on taskbar.
I implemented that on qemu side :-)
>
> (Laine mentioned we have similar per port control for PCI-E
> ('hotplug' property) that was requested by other users
> probably for the same reason)
>
> so acpi-root-pci-hotplug is similar to pcie-root-port.hotplug
> with a difference that the former applies to whole root bus
> on PIIX4, where the later could be controlled per root port.
>
> > > > (4) acpi-root-pci-hotplug=no
> > > > acpi-pci-hotplug-with-bridge-support=no
> > > >
> > > > - No bridges have hotplug. Essentially identical to (2)
> > >
> > > no (4) is not identical to (2). In (4) no hotplug is enabled. In (2) pci
> > > root bus still has hotplug enabled.
> >
> > So you're saying that acpi-root-pci-hotplug=yes overrides the
> > global request acpi-pci-hotplug-with-bridge-support=no and
> > turns ACPI hotplug back on for the pcie-root
>
> historically ACPI hotplug on root bus was always supported
> without any option, i.e. acpi-root-pci-hotplug=yes by default.
> acpi-pci-hotplug-with-bridge-support does what its name
> claims - i.e. adds hotplug for bridges (at least on PIIX4).
>
> > > > * Q35
>
> clarification [*1*] still applies
>
> > > >
> > > >
> > > > - acpi-pci-hotplug-with-bridge-support=bool
> > > >
> > > > Toggles support for ACPI based hotplug. If disabled native
> > > > PCIe hotplug is activated instead
> > > >
> > > >
> > > > * pcie-root-port
> > > >
> > > > - hotplug=bool
> > > >
> > > > Toggles whether hotplug is enabled on the port. Affects
> > > > either native and ACPI based hotplug, depending on what
> > > > acpi-pci-hotplug-with-bridge-support=bool is set to ?
> > > >
> > > > <target hotplug="on/off"> for pcie-root-port controller
> > > >
> > > > - native_hotplug=bool
> > > >
> > > > Can be used to also enable native hotplug, even when ACPI
> > > > based hotplug is globally enabled. IOW only really relevant
> > > > when acpi-pci-hotplug-with-bridge-support=true
> > > >
> > > >
> > > > Q35 combinations
> > > >
> > > > (1) acpi-pci-hotplug-with-bridge-support=yes
> name is a bit confusing depending on the point of view,
> but if one thinks about root/downstream-ports as bridges
> it fits just fine.
>
> > > > pcie-root-port.hotplug=yes
> > > > pcie-root-port.native_hotplug=yes
>
> it probably needs to say here what defaults are in place currently.
> maybe say it once above (1) for all Q35 section
>
>
> > > > ports have both ACPI + native hotplug, guest prefers native
>
> are you sure about 'prefers'?
>
> > > >
> > > >
> > > > (2) acpi-pci-hotplug-with-bridge-support=yes
>
> > > > pcie-root-port.hotplug=no
> > > > pcie-root-port.native_hotplug=yes
> seems as invalid config, we should error out
>
> > > > ports have no hotplug, despite native_hotplug=yes IIUC
> > > >
> > > >
> > > > (3) acpi-pci-hotplug-with-bridge-support=yes
> > > > pcie-root-port.hotplug=yes
> > > > pcie-root-port.native_hotplug=no
> > > >
> > > > ports have ACPI hotplug only
> > > >
> > > >
> > > > (4) acpi-pci-hotplug-with-bridge-support=yes
> > > > pcie-root-port.hotplug=no
> > > > pcie-root-port.native_hotplug=no
> > > >
> > > > ports have no hotplug
> > > >
> > > >
> > > > (5) acpi-pci-hotplug-with-bridge-support=no
> > > > pcie-root-port.hotplug=yes
> > > > pcie-root-port.native_hotplug=yes
> > > >
> > > > ports have native hotplug
> > > >
> > > >
> > > > (6) acpi-pci-hotplug-with-bridge-support=no
>
> > > > pcie-root-port.hotplug=no
> > > > pcie-root-port.native_hotplug=yes
> ditto
>
> > > >
> > > > ports have no hotplug, despite native_hotplug=yes IIUC
> > > >
> > > >
> > > > (7) acpi-pci-hotplug-with-bridge-support=no
> > > > pcie-root-port.hotplug=yes
> > > > pcie-root-port.native_hotplug=no
> > > >
> > > > ports have no hotplug, despite hotplug=yes IIUC
> looks wrong too, but harder to check without picking into
> global acpi-pci-hotplug-with-bridge-support from 'random'
> 'port' device. not sure what to do here
>
> > > >
> > > > (8) acpi-pci-hotplug-with-bridge-support=no
> > > > pcie-root-port.hotplug=no
> > > > pcie-root-port.native_hotplug=no
> > > >
> > > > ports have no hotplug
>
> Ani,
>
> it would be nice to have a similar doc in QEMU, lets say docs/pic_hoptplug.rst
> and maybe move there relevant sections from pcie.txt/pcie_pci_bridge.txt
> to have everything PCI hotplug related in one place.
Yeah as I was discussing this here and on IRC with Laine, I realized
this is all very convoluted. We should definitely document this. Will
work on it after I am done with this patchset.
