Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/i386/acpi-build.c | 7 ++++---
hw/i386/pc.c | 9 ++++-----
hw/i386/pc_q35.c | 4 ++--
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6b7bade183..f9efb6be41 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2766,7 +2766,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
ACPI_BUILD_ALIGN_SIZE);
if (tables_blob->len > legacy_table_size) {
/* Should happen only with PCI bridges and -M pc-i440fx-2.0. */
- warn_report("migration may not work.");
+ warn_report("ACPI tables are larger than legacy_table_size");
+ warn_report("migration may not work");
}
g_array_set_size(tables_blob, legacy_table_size);
} else {
@@ -2774,9 +2775,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
if (tables_blob->len > ACPI_BUILD_TABLE_SIZE / 2) {
/* As of QEMU 2.1, this fires with 160 VCPUs and 255 memory slots. */
warn_report("ACPI tables are larger than 64k.");
- warn_report("migration may not work.");
+ warn_report("migration may not work");
warn_report("please remove CPUs, NUMA nodes, "
- "memory slots or PCI bridges.");
+ "memory slots or PCI bridges");
}
acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
}
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 465e91cc5b..084ca796c2 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -383,8 +383,8 @@ ISADevice *pc_find_fdc0(void)
if (state.multiple) {
warn_report("multiple floppy disk controllers with "
"iobase=0x3f0 have been found");
- error_printf("the one being picked for CMOS setup might not reflect "
- "your intent\n");
+ warn_report("the one being picked for CMOS setup might not reflect "
+ "your intent");
}
return state.floppy;
@@ -2087,9 +2087,8 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
}
if (value < (1ULL << 20)) {
- warn_report("small max_ram_below_4g(%"PRIu64
- ") less than 1M. BIOS may not work..",
- value);
+ warn_report("max_ram_below_4g (%" PRIu64 ") is less than 1M; "
+ "BIOS may not work.", value);
}
pcms->max_ram_below_4g = value;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 1653a47f0a..682c576cf1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -101,8 +101,8 @@ static void pc_q35_init(MachineState *machine)
lowmem = pcms->max_ram_below_4g;
if (machine->ram_size - lowmem > lowmem &&
lowmem & ((1ULL << 30) - 1)) {
- warn_report("Large machine and max_ram_below_4g(%"PRIu64
- ") not a multiple of 1G; possible bad performance.",
+ warn_report("Large machine and max_ram_below_4g (%"PRIu64") not a "
+ "multiple of 1G; possible bad performance.",
pcms->max_ram_below_4g);
}
}
--
2.11.0
Alistair Francis <alistair.francis@xilinx.com> writes:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
You forgot to cc: Eduardo. Fixed.
> ---
>
> hw/i386/acpi-build.c | 7 ++++---
> hw/i386/pc.c | 9 ++++-----
> hw/i386/pc_q35.c | 4 ++--
> 3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6b7bade183..f9efb6be41 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2766,7 +2766,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> ACPI_BUILD_ALIGN_SIZE);
> if (tables_blob->len > legacy_table_size) {
> /* Should happen only with PCI bridges and -M pc-i440fx-2.0. */
> - warn_report("migration may not work.");
> + warn_report("ACPI tables are larger than legacy_table_size");
> + warn_report("migration may not work");
The user has no idea what legacy_table_size means, what its value might
be, or what he can do to reduce it.
Recommend
warn_report("ACPI tables too large, migration may not work");
If the user can do something to reduce the table size, printing suitable
hints would be nice. Printing both tables_blob->len and
legacy_table_size might also help then.
> }
> g_array_set_size(tables_blob, legacy_table_size);
> } else {
> @@ -2774,9 +2775,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> if (tables_blob->len > ACPI_BUILD_TABLE_SIZE / 2) {
> /* As of QEMU 2.1, this fires with 160 VCPUs and 255 memory slots. */
> warn_report("ACPI tables are larger than 64k.");
The warning text hardcodes the value of ACPI_BUILD_TABLE_SIZE / 2. Not
nice. Clean up while there?
> - warn_report("migration may not work.");
> + warn_report("migration may not work");
> warn_report("please remove CPUs, NUMA nodes, "
> - "memory slots or PCI bridges.");
> + "memory slots or PCI bridges");
Aha, here's what the user can do.
What about:
warn_report("ACPI tables are large, migration may not work");
error_printf("Try removing CPUs, NUMA nodes, memory slots"
" or PCI bridges.");
If we want to show actual size and limit, then this might do instead:
warn_report("ACPI table size %u exceeds %d bytes,"
" migration may not work",
tables_blob->len, ACPI_BUILD_TABLE_SIZE / 2);
error_printf("Try removing CPUs, NUMA nodes, memory slots"
" or PCI bridges.");
> }
> acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> }
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 465e91cc5b..084ca796c2 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -383,8 +383,8 @@ ISADevice *pc_find_fdc0(void)
> if (state.multiple) {
> warn_report("multiple floppy disk controllers with "
> "iobase=0x3f0 have been found");
> - error_printf("the one being picked for CMOS setup might not reflect "
> - "your intent\n");
> + warn_report("the one being picked for CMOS setup might not reflect "
> + "your intent");
Please keep error_printf() here.
> }
>
> return state.floppy;
> @@ -2087,9 +2087,8 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
> }
>
> if (value < (1ULL << 20)) {
> - warn_report("small max_ram_below_4g(%"PRIu64
> - ") less than 1M. BIOS may not work..",
> - value);
> + warn_report("max_ram_below_4g (%" PRIu64 ") is less than 1M; "
> + "BIOS may not work.", value);
The user has no idea what max_ram_below_4g might be. Suggest:
warn_report("Only %" PRIu64 " bytes of RAM below the 4GiB boundary,"
"BIOS may not work with less than 1MiB");
> }
>
> pcms->max_ram_below_4g = value;
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 1653a47f0a..682c576cf1 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -101,8 +101,8 @@ static void pc_q35_init(MachineState *machine)
> lowmem = pcms->max_ram_below_4g;
> if (machine->ram_size - lowmem > lowmem &&
> lowmem & ((1ULL << 30) - 1)) {
> - warn_report("Large machine and max_ram_below_4g(%"PRIu64
> - ") not a multiple of 1G; possible bad performance.",
> + warn_report("Large machine and max_ram_below_4g (%"PRIu64") not a "
> + "multiple of 1G; possible bad performance.",
Space between string literal and PRIu64, please.
The user has no idea what max_ram_below_4g might be, or what makes the
machine "large".
> pcms->max_ram_below_4g);
> }
> }
On Wed, Jul 12, 2017 at 11:39:59AM +0200, Markus Armbruster wrote:
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
> > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
>
> You forgot to cc: Eduardo. Fixed.
>
> > ---
> >
> > hw/i386/acpi-build.c | 7 ++++---
> > hw/i386/pc.c | 9 ++++-----
> > hw/i386/pc_q35.c | 4 ++--
> > 3 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 6b7bade183..f9efb6be41 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2766,7 +2766,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> > ACPI_BUILD_ALIGN_SIZE);
> > if (tables_blob->len > legacy_table_size) {
> > /* Should happen only with PCI bridges and -M pc-i440fx-2.0. */
> > - warn_report("migration may not work.");
> > + warn_report("ACPI tables are larger than legacy_table_size");
> > + warn_report("migration may not work");
>
> The user has no idea what legacy_table_size means, what its value might
> be, or what he can do to reduce it.
>
> Recommend
>
> warn_report("ACPI tables too large, migration may not work");
>
> If the user can do something to reduce the table size, printing suitable
> hints would be nice. Printing both tables_blob->len and
> legacy_table_size might also help then.
>
> > }
> > g_array_set_size(tables_blob, legacy_table_size);
> > } else {
> > @@ -2774,9 +2775,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> > if (tables_blob->len > ACPI_BUILD_TABLE_SIZE / 2) {
> > /* As of QEMU 2.1, this fires with 160 VCPUs and 255 memory slots. */
> > warn_report("ACPI tables are larger than 64k.");
>
> The warning text hardcodes the value of ACPI_BUILD_TABLE_SIZE / 2. Not
> nice. Clean up while there?
>
> > - warn_report("migration may not work.");
> > + warn_report("migration may not work");
> > warn_report("please remove CPUs, NUMA nodes, "
> > - "memory slots or PCI bridges.");
> > + "memory slots or PCI bridges");
>
> Aha, here's what the user can do.
>
> What about:
>
> warn_report("ACPI tables are large, migration may not work");
> error_printf("Try removing CPUs, NUMA nodes, memory slots"
> " or PCI bridges.");
>
> If we want to show actual size and limit, then this might do instead:
>
> warn_report("ACPI table size %u exceeds %d bytes,"
> " migration may not work",
> tables_blob->len, ACPI_BUILD_TABLE_SIZE / 2);
> error_printf("Try removing CPUs, NUMA nodes, memory slots"
> " or PCI bridges.");
Yep, this suggestion is good for both cases: the check
(ACPI_BUILD_TABLE_SIZE / 2), and the check for legacy_table_size.
>
> > }
> > acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> > }
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 465e91cc5b..084ca796c2 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -383,8 +383,8 @@ ISADevice *pc_find_fdc0(void)
> > if (state.multiple) {
> > warn_report("multiple floppy disk controllers with "
> > "iobase=0x3f0 have been found");
> > - error_printf("the one being picked for CMOS setup might not reflect "
> > - "your intent\n");
> > + warn_report("the one being picked for CMOS setup might not reflect "
> > + "your intent");
>
> Please keep error_printf() here.
>
I think I suggested warn_report() here for consistency, because I
have seen other cases where multiple warn_report() calls were
used. We probably want to change those other cases like you
suggested above.
> > }
> >
> > return state.floppy;
> > @@ -2087,9 +2087,8 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
> > }
> >
> > if (value < (1ULL << 20)) {
> > - warn_report("small max_ram_below_4g(%"PRIu64
> > - ") less than 1M. BIOS may not work..",
> > - value);
> > + warn_report("max_ram_below_4g (%" PRIu64 ") is less than 1M; "
> > + "BIOS may not work.", value);
>
> The user has no idea what max_ram_below_4g might be. Suggest:
>
> warn_report("Only %" PRIu64 " bytes of RAM below the 4GiB boundary,"
> "BIOS may not work with less than 1MiB");
Actually, the user probably knows what it is, because this setter
will be invoked only if "-M max-ram-below-4g=..." is used in the
command-line. We should fix the spelling to "max-ram-below-4g",
though.
>
> > }
> >
> > pcms->max_ram_below_4g = value;
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 1653a47f0a..682c576cf1 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -101,8 +101,8 @@ static void pc_q35_init(MachineState *machine)
> > lowmem = pcms->max_ram_below_4g;
> > if (machine->ram_size - lowmem > lowmem &&
> > lowmem & ((1ULL << 30) - 1)) {
> > - warn_report("Large machine and max_ram_below_4g(%"PRIu64
> > - ") not a multiple of 1G; possible bad performance.",
> > + warn_report("Large machine and max_ram_below_4g (%"PRIu64") not a "
> > + "multiple of 1G; possible bad performance.",
>
> Space between string literal and PRIu64, please.
>
> The user has no idea what max_ram_below_4g might be, [...]
Same as above: the warning should appear only if the user set
"max-ram-below-4g" explicitly, so the user probably knows what it
is.
> [...] or what makes the
> machine "large".
True.
>
> > pcms->max_ram_below_4g);
> > }
> > }
--
Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Wed, Jul 12, 2017 at 11:39:59AM +0200, Markus Armbruster wrote:
>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>
>> > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
>>
>> You forgot to cc: Eduardo. Fixed.
>>
>> > ---
>> >
>> > hw/i386/acpi-build.c | 7 ++++---
>> > hw/i386/pc.c | 9 ++++-----
>> > hw/i386/pc_q35.c | 4 ++--
>> > 3 files changed, 10 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> > index 6b7bade183..f9efb6be41 100644
>> > --- a/hw/i386/acpi-build.c
>> > +++ b/hw/i386/acpi-build.c
>> > @@ -2766,7 +2766,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>> > ACPI_BUILD_ALIGN_SIZE);
>> > if (tables_blob->len > legacy_table_size) {
>> > /* Should happen only with PCI bridges and -M pc-i440fx-2.0. */
>> > - warn_report("migration may not work.");
>> > + warn_report("ACPI tables are larger than legacy_table_size");
>> > + warn_report("migration may not work");
>>
>> The user has no idea what legacy_table_size means, what its value might
>> be, or what he can do to reduce it.
>>
>> Recommend
>>
>> warn_report("ACPI tables too large, migration may not work");
>>
>> If the user can do something to reduce the table size, printing suitable
>> hints would be nice. Printing both tables_blob->len and
>> legacy_table_size might also help then.
>>
>> > }
>> > g_array_set_size(tables_blob, legacy_table_size);
>> > } else {
>> > @@ -2774,9 +2775,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>> > if (tables_blob->len > ACPI_BUILD_TABLE_SIZE / 2) {
>> > /* As of QEMU 2.1, this fires with 160 VCPUs and 255 memory slots. */
>> > warn_report("ACPI tables are larger than 64k.");
>>
>> The warning text hardcodes the value of ACPI_BUILD_TABLE_SIZE / 2. Not
>> nice. Clean up while there?
>>
>> > - warn_report("migration may not work.");
>> > + warn_report("migration may not work");
>> > warn_report("please remove CPUs, NUMA nodes, "
>> > - "memory slots or PCI bridges.");
>> > + "memory slots or PCI bridges");
>>
>> Aha, here's what the user can do.
>>
>> What about:
>>
>> warn_report("ACPI tables are large, migration may not work");
>> error_printf("Try removing CPUs, NUMA nodes, memory slots"
>> " or PCI bridges.");
>>
>> If we want to show actual size and limit, then this might do instead:
>>
>> warn_report("ACPI table size %u exceeds %d bytes,"
>> " migration may not work",
>> tables_blob->len, ACPI_BUILD_TABLE_SIZE / 2);
>> error_printf("Try removing CPUs, NUMA nodes, memory slots"
>> " or PCI bridges.");
>
> Yep, this suggestion is good for both cases: the check
> (ACPI_BUILD_TABLE_SIZE / 2), and the check for legacy_table_size.
>
>>
>> > }
>> > acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
>> > }
>> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> > index 465e91cc5b..084ca796c2 100644
>> > --- a/hw/i386/pc.c
>> > +++ b/hw/i386/pc.c
>> > @@ -383,8 +383,8 @@ ISADevice *pc_find_fdc0(void)
>> > if (state.multiple) {
>> > warn_report("multiple floppy disk controllers with "
>> > "iobase=0x3f0 have been found");
>> > - error_printf("the one being picked for CMOS setup might not reflect "
>> > - "your intent\n");
>> > + warn_report("the one being picked for CMOS setup might not reflect "
>> > + "your intent");
>>
>> Please keep error_printf() here.
>>
>
> I think I suggested warn_report() here for consistency, because I
> have seen other cases where multiple warn_report() calls were
> used. We probably want to change those other cases like you
> suggested above.
>
>> > }
>> >
>> > return state.floppy;
>> > @@ -2087,9 +2087,8 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
>> > }
>> >
>> > if (value < (1ULL << 20)) {
>> > - warn_report("small max_ram_below_4g(%"PRIu64
>> > - ") less than 1M. BIOS may not work..",
>> > - value);
>> > + warn_report("max_ram_below_4g (%" PRIu64 ") is less than 1M; "
>> > + "BIOS may not work.", value);
>>
>> The user has no idea what max_ram_below_4g might be. Suggest:
>>
>> warn_report("Only %" PRIu64 " bytes of RAM below the 4GiB boundary,"
>> "BIOS may not work with less than 1MiB");
>
> Actually, the user probably knows what it is, because this setter
> will be invoked only if "-M max-ram-below-4g=..." is used in the
> command-line. We should fix the spelling to "max-ram-below-4g",
> though.
>
>>
>> > }
>> >
>> > pcms->max_ram_below_4g = value;
>> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> > index 1653a47f0a..682c576cf1 100644
>> > --- a/hw/i386/pc_q35.c
>> > +++ b/hw/i386/pc_q35.c
>> > @@ -101,8 +101,8 @@ static void pc_q35_init(MachineState *machine)
>> > lowmem = pcms->max_ram_below_4g;
>> > if (machine->ram_size - lowmem > lowmem &&
>> > lowmem & ((1ULL << 30) - 1)) {
>> > - warn_report("Large machine and max_ram_below_4g(%"PRIu64
>> > - ") not a multiple of 1G; possible bad performance.",
>> > + warn_report("Large machine and max_ram_below_4g (%"PRIu64") not a "
>> > + "multiple of 1G; possible bad performance.",
>>
>> Space between string literal and PRIu64, please.
>>
>> The user has no idea what max_ram_below_4g might be, [...]
>
> Same as above: the warning should appear only if the user set
> "max-ram-below-4g" explicitly, so the user probably knows what it
> is.
>
>> [...] or what makes the
>> machine "large".
>
> True.
>
>>
>> > pcms->max_ram_below_4g);
>> > }
>> > }
Alistair, I suggest I apply just the other six patches for now, and you
improve this patch without undue time pressure. What do you think?
On Thu, Jul 13, 2017 at 8:24 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
>> On Wed, Jul 12, 2017 at 11:39:59AM +0200, Markus Armbruster wrote:
>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>
>>> > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
>>>
>>> You forgot to cc: Eduardo. Fixed.
>>>
>>> > ---
>>> >
>>> > hw/i386/acpi-build.c | 7 ++++---
>>> > hw/i386/pc.c | 9 ++++-----
>>> > hw/i386/pc_q35.c | 4 ++--
>>> > 3 files changed, 10 insertions(+), 10 deletions(-)
>>> >
>>> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> > index 6b7bade183..f9efb6be41 100644
>>> > --- a/hw/i386/acpi-build.c
>>> > +++ b/hw/i386/acpi-build.c
>>> > @@ -2766,7 +2766,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>>> > ACPI_BUILD_ALIGN_SIZE);
>>> > if (tables_blob->len > legacy_table_size) {
>>> > /* Should happen only with PCI bridges and -M pc-i440fx-2.0. */
>>> > - warn_report("migration may not work.");
>>> > + warn_report("ACPI tables are larger than legacy_table_size");
>>> > + warn_report("migration may not work");
>>>
>>> The user has no idea what legacy_table_size means, what its value might
>>> be, or what he can do to reduce it.
>>>
>>> Recommend
>>>
>>> warn_report("ACPI tables too large, migration may not work");
>>>
>>> If the user can do something to reduce the table size, printing suitable
>>> hints would be nice. Printing both tables_blob->len and
>>> legacy_table_size might also help then.
>>>
>>> > }
>>> > g_array_set_size(tables_blob, legacy_table_size);
>>> > } else {
>>> > @@ -2774,9 +2775,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>>> > if (tables_blob->len > ACPI_BUILD_TABLE_SIZE / 2) {
>>> > /* As of QEMU 2.1, this fires with 160 VCPUs and 255 memory slots. */
>>> > warn_report("ACPI tables are larger than 64k.");
>>>
>>> The warning text hardcodes the value of ACPI_BUILD_TABLE_SIZE / 2. Not
>>> nice. Clean up while there?
>>>
>>> > - warn_report("migration may not work.");
>>> > + warn_report("migration may not work");
>>> > warn_report("please remove CPUs, NUMA nodes, "
>>> > - "memory slots or PCI bridges.");
>>> > + "memory slots or PCI bridges");
>>>
>>> Aha, here's what the user can do.
>>>
>>> What about:
>>>
>>> warn_report("ACPI tables are large, migration may not work");
>>> error_printf("Try removing CPUs, NUMA nodes, memory slots"
>>> " or PCI bridges.");
>>>
>>> If we want to show actual size and limit, then this might do instead:
>>>
>>> warn_report("ACPI table size %u exceeds %d bytes,"
>>> " migration may not work",
>>> tables_blob->len, ACPI_BUILD_TABLE_SIZE / 2);
>>> error_printf("Try removing CPUs, NUMA nodes, memory slots"
>>> " or PCI bridges.");
>>
>> Yep, this suggestion is good for both cases: the check
>> (ACPI_BUILD_TABLE_SIZE / 2), and the check for legacy_table_size.
>>
>>>
>>> > }
>>> > acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
>>> > }
>>> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> > index 465e91cc5b..084ca796c2 100644
>>> > --- a/hw/i386/pc.c
>>> > +++ b/hw/i386/pc.c
>>> > @@ -383,8 +383,8 @@ ISADevice *pc_find_fdc0(void)
>>> > if (state.multiple) {
>>> > warn_report("multiple floppy disk controllers with "
>>> > "iobase=0x3f0 have been found");
>>> > - error_printf("the one being picked for CMOS setup might not reflect "
>>> > - "your intent\n");
>>> > + warn_report("the one being picked for CMOS setup might not reflect "
>>> > + "your intent");
>>>
>>> Please keep error_printf() here.
>>>
>>
>> I think I suggested warn_report() here for consistency, because I
>> have seen other cases where multiple warn_report() calls were
>> used. We probably want to change those other cases like you
>> suggested above.
>>
>>> > }
>>> >
>>> > return state.floppy;
>>> > @@ -2087,9 +2087,8 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
>>> > }
>>> >
>>> > if (value < (1ULL << 20)) {
>>> > - warn_report("small max_ram_below_4g(%"PRIu64
>>> > - ") less than 1M. BIOS may not work..",
>>> > - value);
>>> > + warn_report("max_ram_below_4g (%" PRIu64 ") is less than 1M; "
>>> > + "BIOS may not work.", value);
>>>
>>> The user has no idea what max_ram_below_4g might be. Suggest:
>>>
>>> warn_report("Only %" PRIu64 " bytes of RAM below the 4GiB boundary,"
>>> "BIOS may not work with less than 1MiB");
>>
>> Actually, the user probably knows what it is, because this setter
>> will be invoked only if "-M max-ram-below-4g=..." is used in the
>> command-line. We should fix the spelling to "max-ram-below-4g",
>> though.
>>
>>>
>>> > }
>>> >
>>> > pcms->max_ram_below_4g = value;
>>> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>> > index 1653a47f0a..682c576cf1 100644
>>> > --- a/hw/i386/pc_q35.c
>>> > +++ b/hw/i386/pc_q35.c
>>> > @@ -101,8 +101,8 @@ static void pc_q35_init(MachineState *machine)
>>> > lowmem = pcms->max_ram_below_4g;
>>> > if (machine->ram_size - lowmem > lowmem &&
>>> > lowmem & ((1ULL << 30) - 1)) {
>>> > - warn_report("Large machine and max_ram_below_4g(%"PRIu64
>>> > - ") not a multiple of 1G; possible bad performance.",
>>> > + warn_report("Large machine and max_ram_below_4g (%"PRIu64") not a "
>>> > + "multiple of 1G; possible bad performance.",
>>>
>>> Space between string literal and PRIu64, please.
>>>
>>> The user has no idea what max_ram_below_4g might be, [...]
>>
>> Same as above: the warning should appear only if the user set
>> "max-ram-below-4g" explicitly, so the user probably knows what it
>> is.
>>
>>> [...] or what makes the
>>> machine "large".
>>
>> True.
>>
>>>
>>> > pcms->max_ram_below_4g);
>>> > }
>>> > }
>
> Alistair, I suggest I apply just the other six patches for now, and you
> improve this patch without undue time pressure. What do you think?
That sounds great! I can move this patch into a future cleanup series
(which has a growing amount of work) and work on it when I return. The
sooner this series is in the better, then people can start to use
warn_error() so we don't have more calls to convert.
Thanks,
Alistair
© 2016 - 2026 Red Hat, Inc.