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

[Michael S. Tsirkin]

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?

> 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.




-- 
MST