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

[Marcel Apfelbaum <marcel@redhat.com>]

On 11/03/2016 06:18 AM, Michael S. Tsirkin wrote:
> On Wed, Nov 02, 2016 at 05:16:42PM +0200, 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

Hi Michael

>
> at the cost of breaking all hotplug for all non-acpi users.
>

Do we have a non-acpi user that is able to use the shpc component as-is today?
I remember we need to even tweak QEMU before it can be used, but I might be wrong.

And we don't touch the current machines < 2.8 .

>> and not only for hot-plug, without loosing any functionality.
>> Older machines will have shpc enabled for compatibility reasons.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
> Is an extra slot such a big deal? You can always add more bridges ...
>

It is not only about the slot itself, but more about the usage model.
The PCIe Upstream ports/DMI-PCI devices are also pci-bridges,
but for them slot 0 is allowed.
And what about the hotplug? Slot 0 is not usable at boot, but then is
usable again (for ACPI users) making people wondering:
  https://bugzilla.redhat.com/show_bug.cgi?id=1175113

My point is - can shpc be used as-is today? Even so, I suspect there are much (much)
less users using SHPC than ACPI based hotplug. If this is the case, why bother the
majority of the users? And for the shpc users, they can keep the prev machines
or change the command line, I think changes like this happens over the time.

Adding Markus for his opinion on command line changes.

Thanks,
Marcel

>> ---
>>  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 \
>> --
>> 2.5.5