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

[Marcel Apfelbaum <marcel@redhat.com>]

On 11/03/2016 05:24 PM, Laine Stump wrote:
> On 11/03/2016 07:08 AM, Marcel Apfelbaum wrote:
>> 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...)
>
>
> We greatly dislike coding in behavior changes to libvirt based on machinetype/version or qemu version since a version number is a very broad and inaccurate sword. The best behavior changes are those
> that can be discovered by querying something specific to that behavior, which can't be done in this case, as there is no way to query *anything* specific to a machinetype without instantiating a
> virtual machine of that machinetype (if it's even queryable then, which is anyway irrelevant to libvirt, since our queries to qemu are done with -machine none).
>

+Eduardo

Hi Eduardo,
Can your work can help on this specific case?


> libvirt can of course deal with such a change in default behavior by qemu, but the way it will deal with it is by removing all assumptions about the default of shpc from the code, and replacing those
> assumptions with explicit setting of the option in the xml and on the qemu command line all the time.
>
> In the end, libvirt wouldn't gain anything from this change in qemu's default behavior - it would be more work than just adding a setting for shpc to libvirt and having *libvirt* change its default
> setting (which has the same ultimate results). Of course that's a relatively selfish (libvirt-centric) view, and I suppose direct users of the qemu commandline might get some benefit from changing the
> qemu default, but anyone concerned enough about the exact commandline contents to be running qemu directly would probably also not have a problem adding an option to the commandline if they really
> wanted a 1/32 increase in the number of available slots.
>

As I explained to Michael, this is not only about the extra slot, but
more about the usage model.
I do understand the libvirt point of view, on the other hand why
should we use a default QEMU value that the majority of users don't need?

Thanks,
Marcel

>
>>
>>
>> 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 <marcel@redhat.com>
>>>> ---
>>>>  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 \
>>>>
>>>
>>
>