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

[Marcel Apfelbaum <marcel@redhat.com>]

On 11/03/2016 09:40 PM, Michael S. Tsirkin wrote:
> On Thu, Nov 03, 2016 at 01:05:44PM +0200, Marcel Apfelbaum wrote:
>> 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?
>
> power and some arm systems I guess?
>

Adding Andrew , maybe he can give us an answer.

Anybody else can help answering this?

>> 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.
>
> The reason is that these devices are not themselves
> hotpluggable. Isn't there a flag that allows adding
> a non hotpluggable device? Allowing these would be one solution.
>
>> 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
>
> Let's just disallow that then for consistency?
>

I suppose we can do that... not sure if it worth it.

Thanks,
Marcel

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