Re: [Qemu-devel] [PATCH v2 6/6] vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property

[Michael S. Tsirkin]

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 <address@hidden>
> > ---
> >  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.

-- 
MST