| Message ID | 1462508442-9407-10-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/06/2016 07:20 AM, Cao jin wrote: > From bit to enum OnOffAuto. > > cc: Michael S. Tsirkin <mst@redhat.com> > cc: Markus Armbruster <armbru@redhat.com> > cc: Marcel Apfelbaum <marcel@redhat.com> > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > > Actually, I am not quite sure this device need this change, RFC. > Well, it already has the 'msi' property, so we may want to make it standard 'OnOffAuto'. One problem I can see is the change of semantics. Until now msi=on means 'auto'. From now on it means 'force msi=on', fail otherwise. If I got this right, old machines having msi=on will failed to start on platforms with msibroken=true, right? Maybe we should preserve the semantics for old machines? (this patch does not actually affect the semantics, but patch 11/11 should, otherwise why change it to OnOffAuto, right? ) Thanks, Marcel > hw/pci-bridge/pci_bridge_dev.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c > index 32f4daa..9e31f0e 100644 > --- a/hw/pci-bridge/pci_bridge_dev.c > +++ b/hw/pci-bridge/pci_bridge_dev.c > @@ -41,9 +41,10 @@ struct PCIBridgeDev { > > MemoryRegion bar; > uint8_t chassis_nr; > -#define PCI_BRIDGE_DEV_F_MSI_REQ 0 > -#define PCI_BRIDGE_DEV_F_SHPC_REQ 1 > +#define PCI_BRIDGE_DEV_F_SHPC_REQ 0 > uint32_t flags; > + > + OnOffAuto msi; > }; > typedef struct PCIBridgeDev PCIBridgeDev; > > @@ -65,7 +66,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) > } > } else { > /* MSI is not applicable without SHPC */ > - bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ); > + bridge_dev->msi = ON_OFF_AUTO_OFF; > } > > err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); > @@ -73,7 +74,8 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) > goto slotid_error; > } > > - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) && > + if ((bridge_dev->msi == ON_OFF_AUTO_AUTO || > + bridge_dev->msi == ON_OFF_AUTO_ON) && > msi_nonbroken) { > err = msi_init(dev, 0, 1, true, true); > if (err < 0) { > @@ -146,8 +148,8 @@ static Property pci_bridge_dev_properties[] = { > /* Note: 0 is not a legal chassis number. */ > DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr, > 0), > - DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags, > - PCI_BRIDGE_DEV_F_MSI_REQ, true), > + 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), > DEFINE_PROP_END_OF_LIST(), >
On Tue, May 17, 2016 at 03:39:14PM +0800, Cao jin wrote: > > > On 05/15/2016 09:25 PM, Marcel Apfelbaum wrote: > >On 05/06/2016 07:20 AM, Cao jin wrote: > >> From bit to enum OnOffAuto. > >> > >>cc: Michael S. Tsirkin <mst@redhat.com> > >>cc: Markus Armbruster <armbru@redhat.com> > >>cc: Marcel Apfelbaum <marcel@redhat.com> > >>Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > >>--- > >> > >>Actually, I am not quite sure this device need this change, RFC. > >> > > > >Well, it already has the 'msi' property, so we may want to make it > >standard 'OnOffAuto'. > >One problem I can see is the change of semantics. Until now msi=on means > >'auto'. From now on > >it means 'force msi=on', fail otherwise. If I got this right, old > >machines having msi=on > >will failed to start on platforms with msibroken=true, right? > > Exactly, and patch 11 indeed has semantics change. According to what we > discussed before: "if user want msi=on explicitly on command line, then > msi_init`s failure should result in failing to create device", this > semantics change seems can`t be avoid. > > >Maybe we should preserve the semantics for old machines? (this patch > >does not actually > >affect the semantics, but patch 11/11 should, otherwise why change it to > >OnOffAuto, right? ) > > IMHO, in this case, keep compat maybe a burden on us, and make command-line > confusing. See my understanding: > > before: > command line format is msi=on/off, and 'on' means auto > > now: > we change command line to msi=on/off/auto, and keep compat is meaning 'on' > still means auto? If so, why need 'auto' > > So, I am thinking, maybe we can help old machine user update their > configuration, I mean: when they set msi=on on platforms with > msibroken=true, they definitely fail, so we should give a clear hint in > error message to help them know what should do to start the machine, like > "please use msi=auto and try again" Personally I don't consider this a big issue. I don't think many people specify msi=on. > -- > Yours Sincerely, > > Cao jin >
On 05/15/2016 09:25 PM, Marcel Apfelbaum wrote: > On 05/06/2016 07:20 AM, Cao jin wrote: >> From bit to enum OnOffAuto. >> >> cc: Michael S. Tsirkin <mst@redhat.com> >> cc: Markus Armbruster <armbru@redhat.com> >> cc: Marcel Apfelbaum <marcel@redhat.com> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> >> Actually, I am not quite sure this device need this change, RFC. >> > > Well, it already has the 'msi' property, so we may want to make it > standard 'OnOffAuto'. > One problem I can see is the change of semantics. Until now msi=on means > 'auto'. From now on > it means 'force msi=on', fail otherwise. If I got this right, old > machines having msi=on > will failed to start on platforms with msibroken=true, right? Exactly, and patch 11 indeed has semantics change. According to what we discussed before: "if user want msi=on explicitly on command line, then msi_init`s failure should result in failing to create device", this semantics change seems can`t be avoid. > Maybe we should preserve the semantics for old machines? (this patch > does not actually > affect the semantics, but patch 11/11 should, otherwise why change it to > OnOffAuto, right? ) IMHO, in this case, keep compat maybe a burden on us, and make command-line confusing. See my understanding: before: command line format is msi=on/off, and 'on' means auto now: we change command line to msi=on/off/auto, and keep compat is meaning 'on' still means auto? If so, why need 'auto' So, I am thinking, maybe we can help old machine user update their configuration, I mean: when they set msi=on on platforms with msibroken=true, they definitely fail, so we should give a clear hint in error message to help them know what should do to start the machine, like "please use msi=auto and try again"
On 05/17/2016 03:38 PM, Michael S. Tsirkin wrote: > On Tue, May 17, 2016 at 03:39:14PM +0800, Cao jin wrote: >> >> > Personally I don't consider this a big issue. I don't think > many people specify msi=on. > I also think so:) If so, the semantics won`t change. Marcel, do you think it is ok if I add some info in error message to help user to start machine? And Markus, do you have comments about it?
On 05/23/2016 05:22 AM, Cao jin wrote: > > > On 05/17/2016 03:38 PM, Michael S. Tsirkin wrote: >> On Tue, May 17, 2016 at 03:39:14PM +0800, Cao jin wrote: >>> >>> > >> Personally I don't consider this a big issue. I don't think >> many people specify msi=on. >> > > I also think so:) If so, the semantics won`t change. > > Marcel, do you think it is ok if I add some info in error message to help user to start machine? Of course, giving the user a hint is always a good think IMO. Thanks, Marcel > > And Markus, do you have comments about it? >
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c index 32f4daa..9e31f0e 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -41,9 +41,10 @@ struct PCIBridgeDev { MemoryRegion bar; uint8_t chassis_nr; -#define PCI_BRIDGE_DEV_F_MSI_REQ 0 -#define PCI_BRIDGE_DEV_F_SHPC_REQ 1 +#define PCI_BRIDGE_DEV_F_SHPC_REQ 0 uint32_t flags; + + OnOffAuto msi; }; typedef struct PCIBridgeDev PCIBridgeDev; @@ -65,7 +66,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) } } else { /* MSI is not applicable without SHPC */ - bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ); + bridge_dev->msi = ON_OFF_AUTO_OFF; } err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); @@ -73,7 +74,8 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) goto slotid_error; } - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) && + if ((bridge_dev->msi == ON_OFF_AUTO_AUTO || + bridge_dev->msi == ON_OFF_AUTO_ON) && msi_nonbroken) { err = msi_init(dev, 0, 1, true, true); if (err < 0) { @@ -146,8 +148,8 @@ static Property pci_bridge_dev_properties[] = { /* Note: 0 is not a legal chassis number. */ DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr, 0), - DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags, - PCI_BRIDGE_DEV_F_MSI_REQ, true), + 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), DEFINE_PROP_END_OF_LIST(),
From bit to enum OnOffAuto. cc: Michael S. Tsirkin <mst@redhat.com> cc: Markus Armbruster <armbru@redhat.com> cc: Marcel Apfelbaum <marcel@redhat.com> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- Actually, I am not quite sure this device need this change, RFC. hw/pci-bridge/pci_bridge_dev.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)