Re: [PATCH V2] Add a new PIIX option to control PCI hot unplugging of devices on non-root buses

[Igor Mammedov]

On Thu, 21 May 2020 06:07:25 -0400
"Michael S. Tsirkin" <address@hidden> wrote:

> On Thu, May 21, 2020 at 09:32:17AM +0200, Igor Mammedow wrote:
> > On Wed, 20 May 2020 12:13:35 -0400
> > "Michael S. Tsirkin" <address@hidden> wrote:
> >   
> > > On Wed, May 20, 2020 at 02:20:12PM +0200, Igor Mammedow wrote:  
> > > > On Wed, 20 May 2020 07:23:21 -0400
> > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > >     
> > > > > On Wed, May 20, 2020 at 01:05:47PM +0200, Igor Mammedow wrote:    
> > > > > > On Wed, 20 May 2020 06:28:37 -0400
> > > > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > > > >       
> > > > > > > On Wed, May 20, 2020 at 11:56:26AM +0200, Igor Mammedow
> > > > > > > wrote:      
> > > > > > > > On Wed, 20 May 2020 05:47:53 -0400
> > > > > > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > > > > > >         
> > > > > > > > > On Wed, May 20, 2020 at 11:43:54AM +0200, Igor Mammedow
> > > > > > > > > wrote:        
> > > > > > > > > > On Fri, 15 May 2020 12:13:53 +0000
> > > > > > > > > > Ani Sinha <address@hidden> wrote:
> > > > > > > > > >           
> > > > > > > > > > > > On May 14, 2020, at 1:13 AM, Igor Mammedov
> > > > > > > > > > > > <address@hidden> wrote:           
> > > > > > > > > > > >> 
> > > > > > > > > > > >>             
> > > > > > > > > > > >>> Will following hack work for you?
> > > > > > > > > > > >>> possible permutations
> > > > > > > > > > > >>> 1) ACPI hotplug everywhere
> > > > > > > > > > > >>> -global PIIX4_PM.acpi-pci-hotplug=on -global
> > > > > > > > > > > >>> PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on
> > > > > > > > > > > >>> -device pci-bridge,chassis_nr=1,shpc=doesnt_matter
> > > > > > > > > > > >>> -device e1000,bus=pci.1,addr=01,id=netdev1 
> > > > > > > > > > > >>> 
> > > > > > > > > > > >>> 2) No hotplug at all
> > > > > > > > > > > >>> -global PIIX4_PM.acpi-pci-hotplug=off -global
> > > > > > > > > > > >>> PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on
> > > > > > > > > > > >>> -device pci-bridge,chassis_nr=1,shpc=off -device
> > > > > > > > > > > >>> e1000,bus=pci.1,addr=01,id=netdev1
> > > > > > > > > > > >>> 
> > > > > > > > > > > >>> -global PIIX4_PM.acpi-pci-hotplug=off -global
> > > > > > > > > > > >>> PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off
> > > > > > > > > > > >>> -device pci-bridge,chassis_nr=1,shpc=doesnt_matter
> > > > > > > > > > > >>> -device e1000,bus=pci.1,addr=01,id=netdev1
> > > > > > > > > > > >>>       
> > > > > > > > > > > >> 
> > > > > > > > > > > >> Given that my patch is not acceptable, I’d prefer
> > > > > > > > > > > >> the following in the order of preference:
> > > > > > > > > > > >> 
> > > > > > > > > > > >> (a) Have an option to disable hot ejection of
> > > > > > > > > > > >> PCI-PCI bridge so that Windows does not even show
> > > > > > > > > > > >> this HW in the “safely remove HW” option. If we
> > > > > > > > > > > >> can do this then from OS perspective the GUI
> > > > > > > > > > > >> options will be same as what is available with
> > > > > > > > > > > >> PCIE/q35 - none of the devices will be hot
> > > > > > > > > > > >> ejectable if the hot plug option is turned off
> > > > > > > > > > > >> from the PCIE slots where devices are plugged
> > > > > > > > > > > >> into. I looked at the code. It seems to manipulate
> > > > > > > > > > > >> ACPI tables of the empty slots of the root bus
> > > > > > > > > > > >> where no devices are attached (see comment "/* add
> > > > > > > > > > > >> hotplug slots for non present devices */ “). For
> > > > > > > > > > > >> cold plugged bridges, it recurses down to scan the
> > > > > > > > > > > >> slots of the bridge. Is it possible to disable hot
> > > > > > > > > > > >> plug for the slot to which the bridge is attached?
> > > > > > > > > > > >>            
> > > > > > > > > > > > 
> > > > > > > > > > > > I don't think it's possible to have per slot
> > > > > > > > > > > > hotplug on conventional PCI hardware. it's per
> > > > > > > > > > > > bridge property.   
> > > > > > > > > > > 
> > > > > > > > > > > We add the AMLs per empty slot though. When the pic
> > > > > > > > > > > bridge is attached, we do nothing, just recurse into
> > > > > > > > > > > the bridge slots. That is what I was asking, if it was
> > > > > > > > > > > possible to just disable the AMLs or use some tricks
> > > > > > > > > > > to say that this particular slot is not hotpluggable.
> > > > > > > > > > > I am not sure why Windows is trying to eject the PCI
> > > > > > > > > > > bridge and failing. Maybe something related to this
> > > > > > > > > > > comment?
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > /* When hotplug for bridges is enabled, bridges are
> > > > > > > > > > >                               
> > > > > > > > > > >          * described in ACPI separately (see
> > > > > > > > > > > build_pci_bus_end). 
> > > > > > > > > > >          * In this case they aren't themselves
> > > > > > > > > > > hot-pluggable. 
> > > > > > > > > > >          * Hotplugged bridges *are* hot-pluggable.
> > > > > > > > > > > */          
> > > > > > > > > > 
> > > > > > > > > > thinking some more on this topic, it seems that with
> > > > > > > > > > ACPI hotplug we already have implicit non-hotpluggble
> > > > > > > > > > slot (slot with bridge) while the rest are staying
> > > > > > > > > > hotpluggable.
> > > > > > > > > > 
> > > > > > > > > > So my question is: if it's acceptable to add
> > > > > > > > > > 'PCIDevice::hotpluggable" property to all PCI devices so
> > > > > > > > > > that user / libvirt could set it to false in case they
> > > > > > > > > > do not want coldplugged device be considered as
> > > > > > > > > > hotpluggable? (this way other devices could be treated
> > > > > > > > > > the same way as bridges)
> > > > > > > > > > 
> > > > > > > > > > [...]          
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I think Julia already posted a patch adding this to
> > > > > > > > > downstream pcie bridges. Adding this to pci slots sounds
> > > > > > > > > like a reasonable thing.        
> > > > > > > > Question was more about external interface, were we do not
> > > > > > > > have ports as separate devices with conventional PCI. The
> > > > > > > > only knob we have is a a PCI device, where we have a
> > > > > > > > property to turn on/off hotplug. ex: -device
> > > > > > > > e1000,hotpluggable=off and if libvirt would be able to use
> > > > > > > > it        
> > > > > > > 
> > > > > > > It would make sense but is it practical to add the capability
> > > > > > > is added in a generic way to all bridges and hosts?
> > > > > > > If not how do users probe for presence of the capability?      
> > > > > > it probably won't work with native SHPC hotplug (which looks to
> > > > > > be incomplete in QEMU anyway), but it should work with ACPI and
> > > > > > per port PCIE hotplugs.
> > > > > > In case of SHPC, we probably should be able to cleanly error out
> > > > > > with 'unsupported' reason if  "hotpluggable" conflicts with
> > > > > > bridge policy.      
> > > > > 
> > > > > "Try it and see if it works" is somewhat problematic from
> > > > > management POV since there's a never ending stream of new things
> > > > > they would have to try. If this approach is taken, we'd have to
> > > > > try to loop in some people from libvirt and see what's their
> > > > > take.    
> > > > to clarify, we are talking here about bridges to conventional
> > > > PCI with native SHPC hotplug semantics wrt mgmt and
> > > > potential pcidevice.hotpluggable property.
> > > > (the later should work fine in ACPI and PCIE hoptlug cases).
> > > > 
> > > > currently by default pci bridges have property shpc=off, so mgmt
> > > > should know that deals with PCI bridge and has to enable SHPC
> > > > on bridge explicitly,    
> > > 
> > > Wait a second does that actually affect hotplug with ACPI too?  
> > What do you mean exactly?  
> 
> I just tried to figure out why do you say
> "mgmt should know that deals with PCI bridge and has to enable SHPC
>  on bridge explicitly,"
> since on x86 bridges support hotplug through ACPI bypassing SHPC.

Current state in QEMU is that for ACPI hotplug, it doesn't have to do anything
"acpi-pci-hotplug-with-bridge-support=on" by default and it's global setting for
coldplugged bridges, also it (sort of) overrides SHPC (even if was enabled with
'shpc=on' on a concrete coldplugged bridge).
(I wouldn't be surprised if mixing modes actually isn't functional)

Then there is hotplugged bridges (added at -S time or at runtime), for those
mgmt has to set 'shpc=on' explicitly to be able to hotplug devices into it.
(so mgmt has to know that it's dealing wants SHPC and I suppose it can be taught
that SHPC doesn't support mixed hotplug policy on slots/devices attached to it).
Whether mgmt actually does something with shpc or not, I have no slightest idea.
(CCing Laine, perhaps he can clarify if we should care about SHPC here)


> > > > in which case it could probably be taught that
> > > > using conflicting hotpluggable for device attached to bridge and
> > > > shpc values is wrong thing.
> > > > If that's not it, then I'm not sure what kind of discovery you are
> > > > talking about.    
> > > 
> > > 
> > > 
> > >   
>