GCC9 is confused when building with CFLAG -O3:
hw/scsi/megasas.c: In function ‘megasas_scsi_realize’:
hw/scsi/megasas.c:2387:26: error: duplicated ‘if’ condition [-Werror=duplicated-cond]
2387 | } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
hw/scsi/megasas.c:2385:19: note: previously used here
2385 | if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
cc1: all warnings being treated as errors
When this device was introduced in commit e8f943c3bcc, the author
cared about modularity, using a definition for the firmware limit.
If we modify the limit, the code is valid. Add a check if the
definition got modified to a bigger limit.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Hannes Reinecke <hare@suse.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Fam Zheng <fam@euphon.net>
Cc: qemu-block@nongnu.org
---
hw/scsi/megasas.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index de9bd20887..ece1601b66 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2382,7 +2382,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
if (!s->hba_serial) {
s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
}
- if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
+ if (MEGASAS_MAX_SGE > 128
+ && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
} else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
--
2.21.0
On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote:
> GCC9 is confused when building with CFLAG -O3:
>
> hw/scsi/megasas.c: In function ‘megasas_scsi_realize’:
> hw/scsi/megasas.c:2387:26: error: duplicated ‘if’ condition [-Werror=duplicated-cond]
> 2387 | } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
> hw/scsi/megasas.c:2385:19: note: previously used here
> 2385 | if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
> cc1: all warnings being treated as errors
>
> When this device was introduced in commit e8f943c3bcc, the author
> cared about modularity, using a definition for the firmware limit.
> If we modify the limit, the code is valid. Add a check if the
> definition got modified to a bigger limit.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Fam Zheng <fam@euphon.net>
> Cc: qemu-block@nongnu.org
> ---
> hw/scsi/megasas.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index de9bd20887..ece1601b66 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2382,7 +2382,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
> if (!s->hba_serial) {
> s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
> }
> - if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
> + if (MEGASAS_MAX_SGE > 128
> + && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
> s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
> } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
> s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>
I'm not keen on this. It looks to me like the raw 128 case should be removed
-- surely that's the point of the symbolic constant. But I'll defer if a
maintainer disagrees.
r~
On 12/18/19 5:03 AM, Richard Henderson wrote:
> On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote:
>> GCC9 is confused when building with CFLAG -O3:
>>
>> hw/scsi/megasas.c: In function ‘megasas_scsi_realize’:
>> hw/scsi/megasas.c:2387:26: error: duplicated ‘if’ condition [-Werror=duplicated-cond]
>> 2387 | } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>> hw/scsi/megasas.c:2385:19: note: previously used here
>> 2385 | if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>> cc1: all warnings being treated as errors
>>
>> When this device was introduced in commit e8f943c3bcc, the author
>> cared about modularity, using a definition for the firmware limit.
>> If we modify the limit, the code is valid. Add a check if the
>> definition got modified to a bigger limit.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Cc: Hannes Reinecke <hare@suse.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Fam Zheng <fam@euphon.net>
>> Cc: qemu-block@nongnu.org
>> ---
>> hw/scsi/megasas.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index de9bd20887..ece1601b66 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -2382,7 +2382,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>> if (!s->hba_serial) {
>> s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
>> }
>> - if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>> + if (MEGASAS_MAX_SGE > 128
>> + && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>> s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>> } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>> s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>>
>
> I'm not keen on this. It looks to me like the raw 128 case should be removed
> -- surely that's the point of the symbolic constant. But I'll defer if a
> maintainer disagrees.
Is that approach acceptable?
-- >8 --
@@ -54,6 +54,9 @@
#define MEGASAS_FLAG_USE_QUEUE64 1
#define MEGASAS_MASK_USE_QUEUE64 (1 << MEGASAS_FLAG_USE_QUEUE64)
+QEMU_BUILD_BUG_MSG(MEGASAS_MAX_SGE > 128,
+ "Firmware limit too big for this device model");
+
static const char *mfi_frame_desc[] = {
"MFI init", "LD Read", "LD Write", "LD SCSI", "PD SCSI",
"MFI Doorbell", "MFI Abort", "MFI SMP", "MFI Stop"};
@@ -2382,9 +2385,7 @@ static void megasas_scsi_realize(PCIDevice *dev,
Error **errp)
if (!s->hba_serial) {
s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
}
- if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
- s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
- } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
+ if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
} else {
s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
---
On 12/18/19 7:52 AM, Philippe Mathieu-Daudé wrote:
>>> - if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>> + if (MEGASAS_MAX_SGE > 128
>>> + && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>> s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>>> } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>>> s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>>>
>>
>> I'm not keen on this. It looks to me like the raw 128 case should be removed
>> -- surely that's the point of the symbolic constant. But I'll defer if a
>> maintainer disagrees.
>
> Is that approach acceptable?
>
> -- >8 --
> @@ -54,6 +54,9 @@
> #define MEGASAS_FLAG_USE_QUEUE64 1
> #define MEGASAS_MASK_USE_QUEUE64 (1 << MEGASAS_FLAG_USE_QUEUE64)
>
> +QEMU_BUILD_BUG_MSG(MEGASAS_MAX_SGE > 128,
> + "Firmware limit too big for this device model");
> +
> static const char *mfi_frame_desc[] = {
> "MFI init", "LD Read", "LD Write", "LD SCSI", "PD SCSI",
> "MFI Doorbell", "MFI Abort", "MFI SMP", "MFI Stop"};
> @@ -2382,9 +2385,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error
> **errp)
> if (!s->hba_serial) {
> s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
> }
> - if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
> - s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
> - } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
> + if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
> s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
> } else {
> s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
Eh. I suppose. But what's the point of leaving the hard-coded 128? There's
certainly something I'm missing here...
r~
On 18/12/19 05:03, Richard Henderson wrote:
>> if (!s->hba_serial) {
>> s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
>> }
>> - if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>> + if (MEGASAS_MAX_SGE > 128
>> + && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>> s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>> } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>> s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>> } else {
>> s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
>> }
>
> I'm not keen on this. It looks to me like the raw 128 case should be removed
> -- surely that's the point of the symbolic constant. But I'll defer if a
> maintainer disagrees.
I don't really understand this chain of ifs. Hannes, does it make sense
to just remove the "if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE)" case,
or does Phil's variation (quoted in the patch fragment above) makes sense?
Or perhaps this rewrite:
max_sge = s->fw_sge + MFI_PASS_FRAME_SIZE;
if (max_sge < MEGASAS_MAX_SGE) {
if (max_sge < 64) {
error(...);
} else {
max_sge = max_sge < 128 ? 64 : 128;
}
}
s->fw_sge = max_sge - MFI_PASS_FRAME_SIZE;
Paolo
On 1/7/20 3:56 PM, Paolo Bonzini wrote:
> On 18/12/19 05:03, Richard Henderson wrote:
>>> if (!s->hba_serial) {
>>> s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
>>> }
>>> - if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>> + if (MEGASAS_MAX_SGE > 128
>>> + && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>> s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>>> } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>>> s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>>> } else {
>>> s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
>>> }
>>
>> I'm not keen on this. It looks to me like the raw 128 case should be removed
>> -- surely that's the point of the symbolic constant. But I'll defer if a
>> maintainer disagrees.
>
> I don't really understand this chain of ifs. Hannes, does it make sense
> to just remove the "if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE)" case,
> or does Phil's variation (quoted in the patch fragment above) makes sense?
>
> Or perhaps this rewrite:
>
> max_sge = s->fw_sge + MFI_PASS_FRAME_SIZE;
> if (max_sge < MEGASAS_MAX_SGE) {
> if (max_sge < 64) {
> error(...);
> } else {
> max_sge = max_sge < 128 ? 64 : 128;
> }
> }
> s->fw_sge = max_sge - MFI_PASS_FRAME_SIZE;
>
Yeah, do it.
The original code assumed that one could change MFI_PASS_FRAME_SIZE, but
it turned out not to be possible as it's being hardcoded in the drivers
themselves (even though the interface provides mechanisms to query it).
So we can remove the duplicate lines.
But then I prefer to stick with the define, and avoid having to check
the magic '128' value directly; rather use the define throughout the code.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
© 2016 - 2026 Red Hat, Inc.