Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Michael S. Tsirkin]

On Fri, Jul 23, 2021 at 09:48:23AM +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 22, 2021 at 01:49:34PM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote:
> > > Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35)
> > > switched PCI hotplug from native to ACPI one by default.
> > > 
> > > That however breaks ihotplug on following CLI that used to work:
> > 
> > s/ihotplug/hotplug/ ?
> > 
> > >    -nodefaults -machine q35 \
> > >    -device 
> > > pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1
> > >  \
> > >    -device 
> > > pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2
> > > 
> > > where PCI device is hotplugged to pcie-root-port-1 with error on guest 
> > > side:
> > > 
> > >   ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], 
> > > AE_NOT_FOUND (20201113/psargs-330)
> > >   ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error 
> > > (AE_NOT_FOUND) (20201113/psparse-531)
> > >   ACPI Error: Aborting method \_GPE._E01 due to previous error 
> > > (AE_NOT_FOUND) (20201113/psparse-531)
> > >   ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] 
> > > (20201113/evgpe-515)
> > > 
> > > cause is that QEMU's ACPI hotplug never supported functions other then 0
> > > and due to bug it was generating notification entries for not described
> > > functions.
> > > 
> > > Technically there is no reason not to describe cold-plugged bridges
> > > (root ports) on functions other then 0, as they similaraly to bridge
> > > on function 0 are unpluggable.
> > > 
> > > Fix consists of describing cold-plugged bridges[root ports] on functions
> > > other than 0.
> > 
> > 
> > I would add: since we need to describe multifunction devices
> > 
> > 
> > > 
> > > Fixes: 17858a169508609ca9063c544833e5a1adeb7b52
> > 
> > use short hash and include subject within ("subject here") please
> 
> Using short hashes isn't a good idea in commits IMHO. A git short
> hash is only guaranteed unique at the time it is generated. In future
> the repo might gain commits that result in a clashing short hash.
> Using the full hash is good.
> 
> 
> Regards,
> Daniel

It's a good point but it became a standard practice at this point.
At least with the subject it's unlikely to be ambiguous too often.


> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|