From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45868) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2L6h-0001BN-1e for qemu-devel@nongnu.org; Thu, 03 Nov 2016 12:43:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2L6f-0005kF-RE for qemu-devel@nongnu.org; Thu, 03 Nov 2016 12:43:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37994) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c2L6f-0005id-JJ for qemu-devel@nongnu.org; Thu, 03 Nov 2016 12:43:13 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BCE195277F for ; Thu, 3 Nov 2016 16:43:12 +0000 (UTC) References: <1478099802-14188-1-git-send-email-marcel@redhat.com> <9485c319-9e97-fc71-68d8-b913f0ae792d@redhat.com> <90aefa54-f673-86cd-1749-a004eda65294@redhat.com> <3201bad1-8994-2605-c767-58afd2559e53@redhat.com> From: Marcel Apfelbaum Message-ID: <25b996b6-0d6a-0e6f-a7f7-13528e337ff3@redhat.com> Date: Thu, 3 Nov 2016 18:43:05 +0200 MIME-Version: 1.0 In-Reply-To: <3201bad1-8994-2605-c767-58afd2559e53@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] hw/pci: disable pci-bridge's shpc by default List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laine Stump , qemu-devel@nongnu.org, Eduardo Habkost Cc: kraxel@redhat.com, mst@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 >>>> --- >>>> 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 \ >>>> >>> >> >