| Message ID | 1449994112-7054-7-git-send-email-shmulik.ladkani@ravellosystems.com |
|---|---|
| State | New |
| Headers | show |
On 13/12/2015 09:08, Shmulik Ladkani wrote: > Following the previous patch which changed pvscsi to be a pci express > device, this patch introduces a boolean property 'x-disable-pcie'. > > Its default value is false, exposing pvscsi as a pcie device. > > Setting 'x-disable-pcie' to 'on' preserves the old 'pci device' (non > express) behavior. This allows migration to older versions. > > Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> > --- > hw/scsi/vmw_pvscsi.c | 2 ++ > include/hw/compat.h | 4 ++++ > 2 files changed, 6 insertions(+) > > diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c > index fb53b24..6b4b989 100644 > --- a/hw/scsi/vmw_pvscsi.c > +++ b/hw/scsi/vmw_pvscsi.c > @@ -1241,6 +1241,8 @@ static Property pvscsi_properties[] = { > DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1), > DEFINE_PROP_BIT("x-old-pci-configuration", PVSCSIState, compat_flags, > PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT, false), > + DEFINE_PROP_BIT("x-disable-pcie", PVSCSIState, compat_flags, > + PVSCSI_COMPAT_DISABLE_PCIE_BIT, false), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 66e4aff..bcb36ef 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -11,6 +11,10 @@ > .property = "x-old-pci-configuration",\ > .value = "on",\ > },{\ > + .driver = "pvscsi",\ > + .property = "x-disable-pcie",\ > + .value = "on",\ > + },{\ > .driver = "e1000",\ > .property = "extra_mac_registers",\ > .value = "off",\ > This series is okay, but the use of "x-" in the compat properties struck me as odd. I see that the "x-" prefix was first introduced for compat properties in commit 1811e64 ("hw/virtio: Add PCIe capability to virtio devices", 2015-11-10). The rationale for "x-" was either 1) to provide fine tuning for debugging-like options; 2) for places where the API is known to be somewhat broken and will be fixed later. Marcel, Michael, what was the justification for using "x-" in commit 1811e64? That said, I wouldn't mind using the opportunity of "x-" to remove the double negation and rename the property to just "pcie". :) Paolo
On Mon, Dec 14, 2015 at 06:07:31PM +0100, Paolo Bonzini wrote: > > > On 13/12/2015 09:08, Shmulik Ladkani wrote: > > Following the previous patch which changed pvscsi to be a pci express > > device, this patch introduces a boolean property 'x-disable-pcie'. > > > > Its default value is false, exposing pvscsi as a pcie device. > > > > Setting 'x-disable-pcie' to 'on' preserves the old 'pci device' (non > > express) behavior. This allows migration to older versions. > > > > Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> > > --- > > hw/scsi/vmw_pvscsi.c | 2 ++ > > include/hw/compat.h | 4 ++++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c > > index fb53b24..6b4b989 100644 > > --- a/hw/scsi/vmw_pvscsi.c > > +++ b/hw/scsi/vmw_pvscsi.c > > @@ -1241,6 +1241,8 @@ static Property pvscsi_properties[] = { > > DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1), > > DEFINE_PROP_BIT("x-old-pci-configuration", PVSCSIState, compat_flags, > > PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT, false), > > + DEFINE_PROP_BIT("x-disable-pcie", PVSCSIState, compat_flags, > > + PVSCSI_COMPAT_DISABLE_PCIE_BIT, false), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > diff --git a/include/hw/compat.h b/include/hw/compat.h > > index 66e4aff..bcb36ef 100644 > > --- a/include/hw/compat.h > > +++ b/include/hw/compat.h > > @@ -11,6 +11,10 @@ > > .property = "x-old-pci-configuration",\ > > .value = "on",\ > > },{\ > > + .driver = "pvscsi",\ > > + .property = "x-disable-pcie",\ > > + .value = "on",\ > > + },{\ > > .driver = "e1000",\ > > .property = "extra_mac_registers",\ > > .value = "off",\ > > > > This series is okay, but the use of "x-" in the compat properties struck > me as odd. > > I see that the "x-" prefix was first introduced for compat properties in > commit 1811e64 ("hw/virtio: Add PCIe capability to virtio devices", > 2015-11-10). The rationale for "x-" was either 1) to provide fine > tuning for debugging-like options; 2) for places where the API is known > to be somewhat broken and will be fixed later. > > Marcel, Michael, what was the justification for using "x-" in commit > 1811e64? That said, I wouldn't mind using the opportunity of "x-" to > remove the double negation and rename the property to just "pcie". :) > > Paolo Well qapi/qmp docs say things like: Any name (command, event, type, field, or enum value) beginning with "x-" is marked experimental, and may be withdrawn or changed incompatibly in a future release. It's thus reasonable to use this for internal properties, that we don't want users to play with. BTW, we probably should teach -help to hide these options by default.
> Well qapi/qmp docs say things like: > > Any name (command, event, type, field, or enum value) beginning with > "x-" is marked experimental, and may be withdrawn or changed > incompatibly in a future release. > > It's thus reasonable to use this for internal properties, > that we don't want users to play with. What distinguishes an internal from an external property? Everything except links to backends would be "internal". We've used a much more restrictive definition so far than that---basically known-broken or debugging-only---and I think it's more appropriate. > BTW, we probably should teach -help to hide these options by default. Please, no obscuring of functionality. Debugging-only functionality definitely belongs in -help. Paolo
On Mon, Dec 14, 2015 at 12:28:50PM -0500, Paolo Bonzini wrote: > > Well qapi/qmp docs say things like: > > > > Any name (command, event, type, field, or enum value) beginning with > > "x-" is marked experimental, and may be withdrawn or changed > > incompatibly in a future release. > > > > It's thus reasonable to use this for internal properties, > > that we don't want users to play with. > > What distinguishes an internal from an external property? Everything > except links to backends would be "internal". How do you mean? We have a ton of properties e.g. to control which offloads are allowed for virtio net. Internal are properties which are not there for users, performing some technical function instead. > We've used a much more restrictive definition so far than that---basically > known-broken or debugging-only---and I think it's more appropriate. > > > BTW, we probably should teach -help to hide these options by default. > > Please, no obscuring of functionality. Debugging-only functionality > definitely belongs in -help. > > Paolo Sure. This option is not for debugging though. It's set internally by machine types to avoid breaking migration. I don't see any reason for users to set it.
On 14/12/2015 18:35, Michael S. Tsirkin wrote: > > What distinguishes an internal from an external property? Everything > > except links to backends would be "internal". > > How do you mean? We have a ton of properties e.g. > to control which offloads are allowed for virtio net. Why would users set them? > It's set internally by machine types to avoid breaking > migration. I don't see any reason for users to set it. But they do set it :) albeit only through machine types. I don't think it's different from offloads, just much more specialized. Or do you mean that it could go away if we decide to remove very old machine types? I think we would remove compat properties connected to those machine types as well, even without "x-". Paolo
On Mon, Dec 14, 2015 at 07:04:33PM +0100, Paolo Bonzini wrote: > > > On 14/12/2015 18:35, Michael S. Tsirkin wrote: > > > What distinguishes an internal from an external property? Everything > > > except links to backends would be "internal". > > > > How do you mean? We have a ton of properties e.g. > > to control which offloads are allowed for virtio net. > > Why would users set them? > > > It's set internally by machine types to avoid breaking > > migration. I don't see any reason for users to set it. > > But they do set it :) albeit only through machine types. I don't think > it's different from offloads, just much more specialized. > > Or do you mean that it could go away if we decide to remove very old > machine types? I think we would remove compat properties connected to > those machine types as well, even without "x-". > > Paolo Then we'll break users who set them directly for some reason. So x- means "not part of stable ABI". No?
Hi, On Mon, 14 Dec 2015 20:26:35 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > It's set internally by machine types to avoid breaking > > > migration. I don't see any reason for users to set it. > > > > But they do set it :) albeit only through machine types. I don't think > > it's different from offloads, just much more specialized. > > > > Or do you mean that it could go away if we decide to remove very old > > machine types? I think we would remove compat properties connected to > > those machine types as well, even without "x-". > > > > Then we'll break users who set them directly for some reason. > So x- means "not part of stable ABI". > No? BTW, different drivers use different naming approaches. E.g. d209c744 'hw/audio/intel-hda: Fix MSI capability address' suggests a "old_msi_addr" property (yes, underscores) for intel-hda. Michael, you were the reviewer ;-) Perhaps we can standartize that "c-" prefix denotes compat properties? Regards, Shmulik
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c index fb53b24..6b4b989 100644 --- a/hw/scsi/vmw_pvscsi.c +++ b/hw/scsi/vmw_pvscsi.c @@ -1241,6 +1241,8 @@ static Property pvscsi_properties[] = { DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1), DEFINE_PROP_BIT("x-old-pci-configuration", PVSCSIState, compat_flags, PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT, false), + DEFINE_PROP_BIT("x-disable-pcie", PVSCSIState, compat_flags, + PVSCSI_COMPAT_DISABLE_PCIE_BIT, false), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/compat.h b/include/hw/compat.h index 66e4aff..bcb36ef 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -11,6 +11,10 @@ .property = "x-old-pci-configuration",\ .value = "on",\ },{\ + .driver = "pvscsi",\ + .property = "x-disable-pcie",\ + .value = "on",\ + },{\ .driver = "e1000",\ .property = "extra_mac_registers",\ .value = "off",\
Following the previous patch which changed pvscsi to be a pci express device, this patch introduces a boolean property 'x-disable-pcie'. Its default value is false, exposing pvscsi as a pcie device. Setting 'x-disable-pcie' to 'on' preserves the old 'pci device' (non express) behavior. This allows migration to older versions. Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> --- hw/scsi/vmw_pvscsi.c | 2 ++ include/hw/compat.h | 4 ++++ 2 files changed, 6 insertions(+)