| Message ID | 1464062689-32156-10-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Cao jin <caoj.fnst@cn.fujitsu.com> writes: >>From bit to enum OnOffAuto. > > cc: Hannes Reinecke <hare@suse.de> > cc: Paolo Bonzini <pbonzini@redhat.com> > 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> > --- > hw/scsi/megasas.c | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index 56fb645..e71a28b 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -48,11 +48,7 @@ > > #define MEGASAS_FLAG_USE_JBOD 0 > #define MEGASAS_MASK_USE_JBOD (1 << MEGASAS_FLAG_USE_JBOD) > -#define MEGASAS_FLAG_USE_MSI 1 > -#define MEGASAS_MASK_USE_MSI (1 << MEGASAS_FLAG_USE_MSI) > -#define MEGASAS_FLAG_USE_MSIX 2 > -#define MEGASAS_MASK_USE_MSIX (1 << MEGASAS_FLAG_USE_MSIX) > -#define MEGASAS_FLAG_USE_QUEUE64 3 > +#define MEGASAS_FLAG_USE_QUEUE64 1 > #define MEGASAS_MASK_USE_QUEUE64 (1 << MEGASAS_FLAG_USE_QUEUE64) > > static const char *mfi_frame_desc[] = { > @@ -96,6 +92,8 @@ typedef struct MegasasState { > int busy; > int diag; > int adp_reset; > + OnOffAuto msi; > + OnOffAuto msix; > > MegasasCmd *event_cmd; > int event_locale; > @@ -159,12 +157,12 @@ static bool megasas_use_queue64(MegasasState *s) > > static bool megasas_use_msi(MegasasState *s) > { > - return s->flags & MEGASAS_MASK_USE_MSI; > + return s->msi == ON_OFF_AUTO_AUTO || s->msi == ON_OFF_AUTO_ON; s->msi != ON_OFF_AUTO_OFF, please. > } As we'll see below, s->msi changes on realize(). Before the first realize(), it's whatever the user put there. Before this patch: megasas_mask_use_msi() returns false when the user explicitly requested msi=off, and true when the user did nothing or requested msi=on. After this patch: it returns false when the user explicitly requested msi=off, and true true when the user did nothing or requested msi=on or msi=auto. Okay. Afterwards, it reflects MSI capability. Before this patch: megasas_mask_use_msi() returns false when the device has no MSI capability, either because the user suppressed it with msi=off, or because we couldn't add it. After this patch: same. Okay. > > static bool megasas_use_msix(MegasasState *s) > { > - return s->flags & MEGASAS_MASK_USE_MSIX; > + return s->msix == ON_OFF_AUTO_AUTO || s->msix == ON_OFF_AUTO_ON; s->msi != ON_OFF_AUTO_OFF, please. > } Same correctness argument as for megasas_mask_use_msi(). > > static bool megasas_is_jbod(MegasasState *s) > @@ -2349,12 +2347,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) > > if (megasas_use_msi(s) && > msi_init(dev, 0x50, 1, true, false) < 0) { > - s->flags &= ~MEGASAS_MASK_USE_MSI; > + s->msi = ON_OFF_AUTO_OFF; > } > if (megasas_use_msix(s) && > msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, > &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { > - s->flags &= ~MEGASAS_MASK_USE_MSIX; > + s->msix = ON_OFF_AUTO_OFF; > } > if (pci_is_express(dev)) { > pcie_endpoint_cap_init(dev, 0xa0); Unlike the device models we've seen in earlier patches, this one overwrites its configuration to reflect actual device state. Hmm. I'll revisit this in my review of PATCH 11. > @@ -2422,10 +2420,8 @@ static Property megasas_properties_gen1[] = { > MEGASAS_DEFAULT_FRAMES), > DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial), > DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0), > - DEFINE_PROP_BIT("use_msi", MegasasState, flags, > - MEGASAS_FLAG_USE_MSI, false), > - DEFINE_PROP_BIT("use_msix", MegasasState, flags, > - MEGASAS_FLAG_USE_MSIX, false), > + DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO), > + DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO), > DEFINE_PROP_BIT("use_jbod", MegasasState, flags, > MEGASAS_FLAG_USE_JBOD, false), > DEFINE_PROP_END_OF_LIST(), > @@ -2438,10 +2434,8 @@ static Property megasas_properties_gen2[] = { > MEGASAS_GEN2_DEFAULT_FRAMES), > DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial), > DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0), > - DEFINE_PROP_BIT("use_msi", MegasasState, flags, > - MEGASAS_FLAG_USE_MSI, true), > - DEFINE_PROP_BIT("use_msix", MegasasState, flags, > - MEGASAS_FLAG_USE_MSIX, true), > + DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO), > + DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO), > DEFINE_PROP_BIT("use_jbod", MegasasState, flags, > MEGASAS_FLAG_USE_JBOD, false), > DEFINE_PROP_END_OF_LIST(),
On 06/01/2016 05:14 PM, Markus Armbruster wrote: >> >> static bool megasas_use_msix(MegasasState *s) >> { >> - return s->flags & MEGASAS_MASK_USE_MSIX; >> + return s->msix == ON_OFF_AUTO_AUTO || s->msix == ON_OFF_AUTO_ON; > > s->msi != ON_OFF_AUTO_OFF, please. > >> } > > Same correctness argument as for megasas_mask_use_msi(). > Hi Markus, I Cant find function names megasas_mask_use_msi, only macro that I deleted. Don`t quite follow the meaning, do I need to do anything about it? >> >> static bool megasas_is_jbod(MegasasState *s) >> @@ -2349,12 +2347,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) >> >> if (megasas_use_msi(s) && >> msi_init(dev, 0x50, 1, true, false) < 0) { >> - s->flags &= ~MEGASAS_MASK_USE_MSI; >> + s->msi = ON_OFF_AUTO_OFF; >> } >> if (megasas_use_msix(s) && >> msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, >> &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { >> - s->flags &= ~MEGASAS_MASK_USE_MSIX; >> + s->msix = ON_OFF_AUTO_OFF; >> } >> if (pci_is_express(dev)) { >> pcie_endpoint_cap_init(dev, 0xa0); > > Unlike the device models we've seen in earlier patches, this one > overwrites its configuration to reflect actual device state. Hmm. I'll > revisit this in my review of PATCH 11. > I guess it is still OK?
Cao jin <caoj.fnst@cn.fujitsu.com> writes: > On 06/01/2016 05:14 PM, Markus Armbruster wrote: > >>> >>> static bool megasas_use_msix(MegasasState *s) >>> { >>> - return s->flags & MEGASAS_MASK_USE_MSIX; >>> + return s->msix == ON_OFF_AUTO_AUTO || s->msix == ON_OFF_AUTO_ON; >> >> s->msi != ON_OFF_AUTO_OFF, please. >> >>> } >> >> Same correctness argument as for megasas_mask_use_msi(). >> > > Hi Markus, > I Cant find function names megasas_mask_use_msi, only macro that I > deleted. Don`t quite follow the meaning, do I need to do anything > about it? I meant megasas_use_msi(). Sorry for sowing confusion again :) > >>> >>> static bool megasas_is_jbod(MegasasState *s) >>> @@ -2349,12 +2347,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) >>> >>> if (megasas_use_msi(s) && >>> msi_init(dev, 0x50, 1, true, false) < 0) { >>> - s->flags &= ~MEGASAS_MASK_USE_MSI; >>> + s->msi = ON_OFF_AUTO_OFF; >>> } >>> if (megasas_use_msix(s) && >>> msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, >>> &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { >>> - s->flags &= ~MEGASAS_MASK_USE_MSIX; >>> + s->msix = ON_OFF_AUTO_OFF; >>> } >>> if (pci_is_express(dev)) { >>> pcie_endpoint_cap_init(dev, 0xa0); >> >> Unlike the device models we've seen in earlier patches, this one >> overwrites its configuration to reflect actual device state. Hmm. I'll >> revisit this in my review of PATCH 11. >> > > I guess it is still OK? This patch is okay. I was just observing that this device is different.
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 56fb645..e71a28b 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -48,11 +48,7 @@ #define MEGASAS_FLAG_USE_JBOD 0 #define MEGASAS_MASK_USE_JBOD (1 << MEGASAS_FLAG_USE_JBOD) -#define MEGASAS_FLAG_USE_MSI 1 -#define MEGASAS_MASK_USE_MSI (1 << MEGASAS_FLAG_USE_MSI) -#define MEGASAS_FLAG_USE_MSIX 2 -#define MEGASAS_MASK_USE_MSIX (1 << MEGASAS_FLAG_USE_MSIX) -#define MEGASAS_FLAG_USE_QUEUE64 3 +#define MEGASAS_FLAG_USE_QUEUE64 1 #define MEGASAS_MASK_USE_QUEUE64 (1 << MEGASAS_FLAG_USE_QUEUE64) static const char *mfi_frame_desc[] = { @@ -96,6 +92,8 @@ typedef struct MegasasState { int busy; int diag; int adp_reset; + OnOffAuto msi; + OnOffAuto msix; MegasasCmd *event_cmd; int event_locale; @@ -159,12 +157,12 @@ static bool megasas_use_queue64(MegasasState *s) static bool megasas_use_msi(MegasasState *s) { - return s->flags & MEGASAS_MASK_USE_MSI; + return s->msi == ON_OFF_AUTO_AUTO || s->msi == ON_OFF_AUTO_ON; } static bool megasas_use_msix(MegasasState *s) { - return s->flags & MEGASAS_MASK_USE_MSIX; + return s->msix == ON_OFF_AUTO_AUTO || s->msix == ON_OFF_AUTO_ON; } static bool megasas_is_jbod(MegasasState *s) @@ -2349,12 +2347,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) if (megasas_use_msi(s) && msi_init(dev, 0x50, 1, true, false) < 0) { - s->flags &= ~MEGASAS_MASK_USE_MSI; + s->msi = ON_OFF_AUTO_OFF; } if (megasas_use_msix(s) && msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { - s->flags &= ~MEGASAS_MASK_USE_MSIX; + s->msix = ON_OFF_AUTO_OFF; } if (pci_is_express(dev)) { pcie_endpoint_cap_init(dev, 0xa0); @@ -2422,10 +2420,8 @@ static Property megasas_properties_gen1[] = { MEGASAS_DEFAULT_FRAMES), DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial), DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0), - DEFINE_PROP_BIT("use_msi", MegasasState, flags, - MEGASAS_FLAG_USE_MSI, false), - DEFINE_PROP_BIT("use_msix", MegasasState, flags, - MEGASAS_FLAG_USE_MSIX, false), + DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO), + DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO), DEFINE_PROP_BIT("use_jbod", MegasasState, flags, MEGASAS_FLAG_USE_JBOD, false), DEFINE_PROP_END_OF_LIST(), @@ -2438,10 +2434,8 @@ static Property megasas_properties_gen2[] = { MEGASAS_GEN2_DEFAULT_FRAMES), DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial), DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0), - DEFINE_PROP_BIT("use_msi", MegasasState, flags, - MEGASAS_FLAG_USE_MSI, true), - DEFINE_PROP_BIT("use_msix", MegasasState, flags, - MEGASAS_FLAG_USE_MSIX, true), + DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO), + DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO), DEFINE_PROP_BIT("use_jbod", MegasasState, flags, MEGASAS_FLAG_USE_JBOD, false), DEFINE_PROP_END_OF_LIST(),
From bit to enum OnOffAuto. cc: Hannes Reinecke <hare@suse.de> cc: Paolo Bonzini <pbonzini@redhat.com> 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> --- hw/scsi/megasas.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)