From: Laine Stump <laine@redhat.com>
To: qemu-devel@nongnu.org
Cc: Marcel Apfelbaum <marcel@redhat.com>, kraxel@redhat.com, mst@redhat.com
Subject: Re: [Qemu-devel] [PATCH] hw/pci: disable pci-bridge's shpc by default
Date: Thu, 3 Nov 2016 11:24:58 -0400 [thread overview]
Message-ID: <3201bad1-8994-2605-c767-58afd2559e53@redhat.com> (raw)
In-Reply-To: <90aefa54-f673-86cd-1749-a004eda65294@redhat.com>
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).
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.
>
>
> 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 \
>>>
>>
>
next prev parent reply other threads:[~2016-11-03 15:25 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-02 15:16 [Qemu-devel] [PATCH] hw/pci: disable pci-bridge's shpc by default Marcel Apfelbaum
2016-11-02 16:01 ` Laine Stump
2016-11-03 11:08 ` Marcel Apfelbaum
2016-11-03 15:24 ` Laine Stump [this message]
2016-11-03 16:43 ` Marcel Apfelbaum
2016-11-03 19:09 ` Eduardo Habkost
2016-11-03 4:18 ` Michael S. Tsirkin
2016-11-03 11:05 ` Marcel Apfelbaum
2016-11-03 19:40 ` Michael S. Tsirkin
2016-11-05 16:46 ` Marcel Apfelbaum
2016-11-16 16:44 ` Andrew Jones
2016-11-16 17:05 ` Marcel Apfelbaum
2016-11-18 15:52 ` Andrew Jones
2016-11-18 15:55 ` Michael S. Tsirkin
2016-11-22 17:26 ` Marcel Apfelbaum
2016-11-22 20:25 ` Laurent Vivier
2016-11-23 11:08 ` Marcel Apfelbaum
2016-11-24 4:06 ` David Gibson
2016-11-24 9:39 ` Marcel Apfelbaum
2016-11-25 4:14 ` David Gibson
2016-11-22 17:47 ` Michael S. Tsirkin
2017-01-10 3:48 ` Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3201bad1-8994-2605-c767-58afd2559e53@redhat.com \
--to=laine@redhat.com \
--cc=kraxel@redhat.com \
--cc=marcel@redhat.com \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.