Re: [Qemu-devel] [PATCH] hw/pci: disable pci-bridge's shpc by default

[Marcel Apfelbaum]

On 11/02/2016 06:01 PM, Laine Stump wrote:
> On 11/02/2016 11:16 AM, Marcel Apfelbaum wrote:
> > The shpc component is optional while  ACPI hotplug is used
> > for hot-plugging PCI devices into a PCI-PCI bridge.
> > Disabling the shpc by default will make slot 0 usable at boot time
> > and not only for hot-plug, without loosing any functionality.
> > Older machines will have shpc enabled for compatibility reasons.
>
> From libvirt's POV, changing qemu's default for shpc in qemu only serves to 
> confuse; since we have no way to discover what is the default setting 
> (especially problematic since it is different for
> different machinetype versions), we now either need to keep a table of what the 
> setting is for various machinetypes, or we need to just always set it whether 
> it's on or off.
>
> So for us, the simplest thing is for the default to remain the same (since for 
> so long we haven't set this option as we didn't even know what it did, or 
> indeed even that it existed), and for libvirt
> to learn about the option, make its default in the config files "on", but begin setting 
> it to "off" in all newly defined machines.

Hi Laine,

Please see my reply to Michael regarding the "why", anyway, can't libvirt deal 
with it?
Start use the shpc parameter no matter what QEMU does by default while keeping it 
'on' for machines < 2.8.
(this is only a suggestion, of course...)


Thanks,
Marcel

> (The change to the default for virtio devices' disable-legacy depending on 
> where the device was connected could also have been problematic, except that 
> the change in default value of that has no
> direct consequences on the rest of the configuration - libvirt will still use 
> the controllers in exactly the same way, and the same commandlines for the same 
> machinetype/versions will still result in
> identical behavior (so it's not possible for libvirt to know whether or not IO 
> is enabled for the virtio device, nor to know what is the device's . In the 
> case of pci-bridge's shpc though, the whole
> point of disabling shpc is to make another slot available for libvirt to use 
> for a device, so presumably the config *beyond the pci-bridge device* needs to 
> change as a result of this change in default
> setting, so libvirt needs to know the setting and behave differently.)
>
> > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > ---
> >  hw/pci-bridge/pci_bridge_dev.c | 2 +-
> >  include/hw/compat.h            | 4 ++++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> > index 5dbd933..647ad80 100644
> > --- a/hw/pci-bridge/pci_bridge_dev.c
> > +++ b/hw/pci-bridge/pci_bridge_dev.c
> > @@ -163,7 +163,7 @@ static Property pci_bridge_dev_properties[] = {
> >      DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi,
> >                              ON_OFF_AUTO_AUTO),
> >      DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
> > -                    PCI_BRIDGE_DEV_F_SHPC_REQ, true),
> > +                    PCI_BRIDGE_DEV_F_SHPC_REQ, false),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 0f06e11..388b7ec 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -18,6 +18,10 @@
> >          .driver   = "intel-iommu",\
> >          .property = "x-buggy-eim",\
> >          .value    = "true",\
> > +    },{\
> > +        .driver   = "pci-bridge",\
> > +        .property = "shpc",\
> > +        .value    = "on",\
> >      },
> >
> >  #define HW_COMPAT_2_6 \