* [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size @ 2020-07-20 9:58 Philippe Mathieu-Daudé 2020-07-20 10:04 ` no-reply ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2020-07-20 9:58 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Andrew Jeffery, Philippe Mathieu-Daudé, qemu-arm, Joel Stanley, Cédric Le Goater The SDRAM Memory Controller has a 32-bit address bus, thus supports up to 4 GiB of DRAM. There is a signed to unsigned conversion error with the AST2600 maximum memory size: (uint64_t)(2048 << 20) = (uint64_t)(-2147483648) = 0xffffffff40000000 = 16 EiB - 2 GiB Fix by using the IEC suffixes which are usually safer, and add a check to verify the memory is valid. This would have catched this bug: Unexpected error in aspeed_sdmc_realize() at hw/misc/aspeed_sdmc.c:261: qemu-system-arm: Invalid RAM size 16 EiB Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support") Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/misc/aspeed_sdmc.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c index 0737d8de81..76dd7e6a20 100644 --- a/hw/misc/aspeed_sdmc.c +++ b/hw/misc/aspeed_sdmc.c @@ -256,6 +256,12 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp) AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s); s->max_ram_size = asc->max_ram_size; + if (s->max_ram_size >= 4 * GiB) { + char *szstr = size_to_str(s->max_ram_size); + error_setg(errp, "Invalid RAM size %s", szstr); + g_free(szstr); + return; + } memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s, TYPE_ASPEED_SDMC, 0x1000); @@ -341,7 +347,7 @@ static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data) AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass); dc->desc = "ASPEED 2400 SDRAM Memory Controller"; - asc->max_ram_size = 512 << 20; + asc->max_ram_size = 512 * MiB; asc->compute_conf = aspeed_2400_sdmc_compute_conf; asc->write = aspeed_2400_sdmc_write; asc->valid_ram_sizes = aspeed_2400_ram_sizes; @@ -408,7 +414,7 @@ static void aspeed_2500_sdmc_class_init(ObjectClass *klass, void *data) AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass); dc->desc = "ASPEED 2500 SDRAM Memory Controller"; - asc->max_ram_size = 1024 << 20; + asc->max_ram_size = 1 * GiB; asc->compute_conf = aspeed_2500_sdmc_compute_conf; asc->write = aspeed_2500_sdmc_write; asc->valid_ram_sizes = aspeed_2500_ram_sizes; @@ -485,7 +491,7 @@ static void aspeed_2600_sdmc_class_init(ObjectClass *klass, void *data) AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass); dc->desc = "ASPEED 2600 SDRAM Memory Controller"; - asc->max_ram_size = 2048 << 20; + asc->max_ram_size = 2 * GiB; asc->compute_conf = aspeed_2600_sdmc_compute_conf; asc->write = aspeed_2600_sdmc_write; asc->valid_ram_sizes = aspeed_2600_ram_sizes; -- 2.21.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size 2020-07-20 9:58 [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size Philippe Mathieu-Daudé @ 2020-07-20 10:04 ` no-reply 2020-07-20 10:04 ` no-reply 2020-07-20 16:07 ` Cédric Le Goater 2 siblings, 0 replies; 8+ messages in thread From: no-reply @ 2020-07-20 10:04 UTC (permalink / raw) To: f4bug; +Cc: peter.maydell, andrew, qemu-devel, f4bug, qemu-arm, joel, clg Patchew URL: https://patchew.org/QEMU/20200720095829.22839-1-f4bug@amsat.org/ Hi, This series seems to have some coding style problems. See output below for more information: N/A. Internal error while reading log file The full log is available at http://patchew.org/logs/20200720095829.22839-1-f4bug@amsat.org/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size 2020-07-20 9:58 [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size Philippe Mathieu-Daudé 2020-07-20 10:04 ` no-reply @ 2020-07-20 10:04 ` no-reply 2020-07-20 16:07 ` Cédric Le Goater 2 siblings, 0 replies; 8+ messages in thread From: no-reply @ 2020-07-20 10:04 UTC (permalink / raw) To: f4bug; +Cc: peter.maydell, andrew, qemu-devel, f4bug, qemu-arm, joel, clg Patchew URL: https://patchew.org/QEMU/20200720095829.22839-1-f4bug@amsat.org/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200720095829.22839-1-f4bug@amsat.org/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size 2020-07-20 9:58 [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size Philippe Mathieu-Daudé 2020-07-20 10:04 ` no-reply 2020-07-20 10:04 ` no-reply @ 2020-07-20 16:07 ` Cédric Le Goater 2020-07-20 17:39 ` Philippe Mathieu-Daudé 2 siblings, 1 reply; 8+ messages in thread From: Cédric Le Goater @ 2020-07-20 16:07 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Joel Stanley On 7/20/20 11:58 AM, Philippe Mathieu-Daudé wrote: > The SDRAM Memory Controller has a 32-bit address bus, thus > supports up to 4 GiB of DRAM. There is a signed to unsigned > conversion error with the AST2600 maximum memory size: > > (uint64_t)(2048 << 20) = (uint64_t)(-2147483648) > = 0xffffffff40000000 > = 16 EiB - 2 GiB > > Fix by using the IEC suffixes which are usually safer, and add > a check to verify the memory is valid. This would have catched > this bug: > > Unexpected error in aspeed_sdmc_realize() at hw/misc/aspeed_sdmc.c:261: > qemu-system-arm: Invalid RAM size 16 EiB Indeed :/ > > Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support") > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/misc/aspeed_sdmc.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c > index 0737d8de81..76dd7e6a20 100644 > --- a/hw/misc/aspeed_sdmc.c > +++ b/hw/misc/aspeed_sdmc.c > @@ -256,6 +256,12 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp) > AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s); > > s->max_ram_size = asc->max_ram_size; > + if (s->max_ram_size >= 4 * GiB) { > + char *szstr = size_to_str(s->max_ram_size); > + error_setg(errp, "Invalid RAM size %s", szstr); > + g_free(szstr); > + return; > + } > I would put an assert() since the max RAM size is not user configurable. C. > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s, > TYPE_ASPEED_SDMC, 0x1000); > @@ -341,7 +347,7 @@ static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data) > AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass); > > dc->desc = "ASPEED 2400 SDRAM Memory Controller"; > - asc->max_ram_size = 512 << 20; > + asc->max_ram_size = 512 * MiB; > asc->compute_conf = aspeed_2400_sdmc_compute_conf; > asc->write = aspeed_2400_sdmc_write; > asc->valid_ram_sizes = aspeed_2400_ram_sizes; > @@ -408,7 +414,7 @@ static void aspeed_2500_sdmc_class_init(ObjectClass *klass, void *data) > AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass); > > dc->desc = "ASPEED 2500 SDRAM Memory Controller"; > - asc->max_ram_size = 1024 << 20; > + asc->max_ram_size = 1 * GiB; > asc->compute_conf = aspeed_2500_sdmc_compute_conf; > asc->write = aspeed_2500_sdmc_write; > asc->valid_ram_sizes = aspeed_2500_ram_sizes; > @@ -485,7 +491,7 @@ static void aspeed_2600_sdmc_class_init(ObjectClass *klass, void *data) > AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass); > > dc->desc = "ASPEED 2600 SDRAM Memory Controller"; > - asc->max_ram_size = 2048 << 20; > + asc->max_ram_size = 2 * GiB; > asc->compute_conf = aspeed_2600_sdmc_compute_conf; > asc->write = aspeed_2600_sdmc_write; > asc->valid_ram_sizes = aspeed_2600_ram_sizes; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size 2020-07-20 16:07 ` Cédric Le Goater @ 2020-07-20 17:39 ` Philippe Mathieu-Daudé 2020-07-21 8:13 ` Markus Armbruster 0 siblings, 1 reply; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2020-07-20 17:39 UTC (permalink / raw) To: Cédric Le Goater, qemu-devel Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Joel Stanley On 7/20/20 6:07 PM, Cédric Le Goater wrote: > On 7/20/20 11:58 AM, Philippe Mathieu-Daudé wrote: >> The SDRAM Memory Controller has a 32-bit address bus, thus >> supports up to 4 GiB of DRAM. There is a signed to unsigned >> conversion error with the AST2600 maximum memory size: >> >> (uint64_t)(2048 << 20) = (uint64_t)(-2147483648) >> = 0xffffffff40000000 >> = 16 EiB - 2 GiB >> >> Fix by using the IEC suffixes which are usually safer, and add >> a check to verify the memory is valid. This would have catched >> this bug: >> >> Unexpected error in aspeed_sdmc_realize() at hw/misc/aspeed_sdmc.c:261: >> qemu-system-arm: Invalid RAM size 16 EiB > > Indeed :/ > >> >> Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support") >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/misc/aspeed_sdmc.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c >> index 0737d8de81..76dd7e6a20 100644 >> --- a/hw/misc/aspeed_sdmc.c >> +++ b/hw/misc/aspeed_sdmc.c >> @@ -256,6 +256,12 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp) >> AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s); >> >> s->max_ram_size = asc->max_ram_size; >> + if (s->max_ram_size >= 4 * GiB) { >> + char *szstr = size_to_str(s->max_ram_size); >> + error_setg(errp, "Invalid RAM size %s", szstr); >> + g_free(szstr); >> + return; >> + } >> > > I would put an assert() since the max RAM size is not user configurable. As you wish, at this point I'm completely lost with error reporting. Per the manual (https://www.mail-archive.com/qemu-devel@nongnu.org/msg723217.html): "Many, many devices neglect to clean up properly on error, and get away with it only because all callers treat errors as fatal. If you decide to take cleanup shortcuts, say because the cleanup is untestable, consider adding a comment at least." So I'll go for address + comment: assert(s->max_ram_size < 4 * GiB); /* 32-bit address bus */ > > C. > >> memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s, >> TYPE_ASPEED_SDMC, 0x1000); >> @@ -341,7 +347,7 @@ static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data) >> AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass); >> >> dc->desc = "ASPEED 2400 SDRAM Memory Controller"; >> - asc->max_ram_size = 512 << 20; >> + asc->max_ram_size = 512 * MiB; >> asc->compute_conf = aspeed_2400_sdmc_compute_conf; >> asc->write = aspeed_2400_sdmc_write; >> asc->valid_ram_sizes = aspeed_2400_ram_sizes; >> @@ -408,7 +414,7 @@ static void aspeed_2500_sdmc_class_init(ObjectClass *klass, void *data) >> AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass); >> >> dc->desc = "ASPEED 2500 SDRAM Memory Controller"; >> - asc->max_ram_size = 1024 << 20; >> + asc->max_ram_size = 1 * GiB; >> asc->compute_conf = aspeed_2500_sdmc_compute_conf; >> asc->write = aspeed_2500_sdmc_write; >> asc->valid_ram_sizes = aspeed_2500_ram_sizes; >> @@ -485,7 +491,7 @@ static void aspeed_2600_sdmc_class_init(ObjectClass *klass, void *data) >> AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass); >> >> dc->desc = "ASPEED 2600 SDRAM Memory Controller"; >> - asc->max_ram_size = 2048 << 20; >> + asc->max_ram_size = 2 * GiB; >> asc->compute_conf = aspeed_2600_sdmc_compute_conf; >> asc->write = aspeed_2600_sdmc_write; >> asc->valid_ram_sizes = aspeed_2600_ram_sizes; >> > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size 2020-07-20 17:39 ` Philippe Mathieu-Daudé @ 2020-07-21 8:13 ` Markus Armbruster 2020-07-21 9:06 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 8+ messages in thread From: Markus Armbruster @ 2020-07-21 8:13 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Andrew Jeffery, qemu-devel, qemu-arm, Joel Stanley, Cédric Le Goater Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 7/20/20 6:07 PM, Cédric Le Goater wrote: >> On 7/20/20 11:58 AM, Philippe Mathieu-Daudé wrote: >>> The SDRAM Memory Controller has a 32-bit address bus, thus >>> supports up to 4 GiB of DRAM. There is a signed to unsigned >>> conversion error with the AST2600 maximum memory size: >>> >>> (uint64_t)(2048 << 20) = (uint64_t)(-2147483648) >>> = 0xffffffff40000000 >>> = 16 EiB - 2 GiB >>> >>> Fix by using the IEC suffixes which are usually safer, and add >>> a check to verify the memory is valid. This would have catched caught >>> this bug: >>> >>> Unexpected error in aspeed_sdmc_realize() at hw/misc/aspeed_sdmc.c:261: >>> qemu-system-arm: Invalid RAM size 16 EiB >> >> Indeed :/ >> >>> >>> Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support") >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> hw/misc/aspeed_sdmc.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c >>> index 0737d8de81..76dd7e6a20 100644 >>> --- a/hw/misc/aspeed_sdmc.c >>> +++ b/hw/misc/aspeed_sdmc.c >>> @@ -256,6 +256,12 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp) >>> AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s); >>> >>> s->max_ram_size = asc->max_ram_size; >>> + if (s->max_ram_size >= 4 * GiB) { >>> + char *szstr = size_to_str(s->max_ram_size); >>> + error_setg(errp, "Invalid RAM size %s", szstr); >>> + g_free(szstr); >>> + return; >>> + } >>> >> >> I would put an assert() since the max RAM size is not user configurable. > > As you wish, at this point I'm completely lost with error reporting. :-/ > Per the manual > (https://www.mail-archive.com/qemu-devel@nongnu.org/msg723217.html): > > "Many, many devices neglect to clean up properly on error, and get away > with it only because all callers treat errors as fatal. > > If you decide to take cleanup shortcuts, say because the cleanup is > untestable, consider adding a comment at least." > > So I'll go for address + comment: > > assert(s->max_ram_size < 4 * GiB); /* 32-bit address bus */ Makes sense. Note this is *not* a cleanup shortcut, at least not the kind I had in mind. What I had in mind is unclean failure, i.e. returning on error without proper cleanup: revert changes made so far, free resources. This is *wrong*. But the wrongness doesn't matter when all callers treat errors as fatal. Checking an impossible condition with assert() is better than treating it as an error and bungling its handling. If you treat it as an error, do it properly. Since I'm quite skeptical about the chances of pulling off "properly" for untestable things, I prefer assertions. There's another reason. User errors need to be handled gracefully. Programming errors should (in my opinion) trigger abort(), so they get fixed. When the spot that detects the error can't know which kind it is, you have to fail cleanly and let the caller decide how to handle the error. Example: object_property_find() errors out when the property doesn't exist. This may be a programming error, e.g. a well-known property isn't found, because a programmer mistyped the property name. Or it may be a user error, e.g. a user mistyped the property name argument of qom-get. When functions have multiple failure modes, and only some of them are programming errors, the caller typically can't tell them apart, and errs on the side of user error. Programming errors then get reported as (typically confusing!) user errors. The #1 reason for such awkward functions is lazy thinking + eager typing: by treating anything that can go wrong as an error for the caller to handle, I can replace thinking about what may go wrong and what must not go wrong by typing up a bunch of error paths. Great time saver as long as I stick to the time-honored strategy of not bothering to test my error paths. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size 2020-07-21 8:13 ` Markus Armbruster @ 2020-07-21 9:06 ` Philippe Mathieu-Daudé 2020-07-21 9:55 ` Markus Armbruster 0 siblings, 1 reply; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2020-07-21 9:06 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Andrew Jeffery, qemu-devel, qemu-arm, Cédric Le Goater, Joel Stanley On 7/21/20 10:13 AM, Markus Armbruster wrote: > Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > >> On 7/20/20 6:07 PM, Cédric Le Goater wrote: >>> On 7/20/20 11:58 AM, Philippe Mathieu-Daudé wrote: >>>> The SDRAM Memory Controller has a 32-bit address bus, thus >>>> supports up to 4 GiB of DRAM. There is a signed to unsigned >>>> conversion error with the AST2600 maximum memory size: >>>> >>>> (uint64_t)(2048 << 20) = (uint64_t)(-2147483648) >>>> = 0xffffffff40000000 >>>> = 16 EiB - 2 GiB >>>> >>>> Fix by using the IEC suffixes which are usually safer, and add >>>> a check to verify the memory is valid. This would have catched > > caught > >>>> this bug: >>>> >>>> Unexpected error in aspeed_sdmc_realize() at hw/misc/aspeed_sdmc.c:261: >>>> qemu-system-arm: Invalid RAM size 16 EiB >>> >>> Indeed :/ >>> >>>> >>>> Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support") >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>> --- >>>> hw/misc/aspeed_sdmc.c | 12 +++++++++--- >>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c >>>> index 0737d8de81..76dd7e6a20 100644 >>>> --- a/hw/misc/aspeed_sdmc.c >>>> +++ b/hw/misc/aspeed_sdmc.c >>>> @@ -256,6 +256,12 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp) >>>> AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s); >>>> >>>> s->max_ram_size = asc->max_ram_size; >>>> + if (s->max_ram_size >= 4 * GiB) { >>>> + char *szstr = size_to_str(s->max_ram_size); >>>> + error_setg(errp, "Invalid RAM size %s", szstr); >>>> + g_free(szstr); >>>> + return; >>>> + } >>>> >>> >>> I would put an assert() since the max RAM size is not user configurable. >> >> As you wish, at this point I'm completely lost with error reporting. > > :-/ > >> Per the manual >> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg723217.html): >> >> "Many, many devices neglect to clean up properly on error, and get away >> with it only because all callers treat errors as fatal. >> >> If you decide to take cleanup shortcuts, say because the cleanup is >> untestable, consider adding a comment at least." >> >> So I'll go for address + comment: >> >> assert(s->max_ram_size < 4 * GiB); /* 32-bit address bus */ > > Makes sense. > > Note this is *not* a cleanup shortcut, at least not the kind I had in > mind. > > What I had in mind is unclean failure, i.e. returning on error without > proper cleanup: revert changes made so far, free resources. This is > *wrong*. But the wrongness doesn't matter when all callers treat errors > as fatal. > > Checking an impossible condition with assert() is better than treating > it as an error and bungling its handling. If you treat it as an error, > do it properly. Since I'm quite skeptical about the chances of pulling > off "properly" for untestable things, I prefer assertions. > > There's another reason. User errors need to be handled gracefully. > Programming errors should (in my opinion) trigger abort(), so they get > fixed. > > When the spot that detects the error can't know which kind it is, you > have to fail cleanly and let the caller decide how to handle the error. > > Example: object_property_find() errors out when the property doesn't > exist. This may be a programming error, e.g. a well-known property > isn't found, because a programmer mistyped the property name. Or it may > be a user error, e.g. a user mistyped the property name argument of > qom-get. > > When functions have multiple failure modes, and only some of them are > programming errors, the caller typically can't tell them apart, and errs > on the side of user error. Programming errors then get reported as > (typically confusing!) user errors. A big part of your reply is worth adding in a "How to correctly use the Error* propagation API for dummies" in docs/devel document. > > The #1 reason for such awkward functions is lazy thinking + eager > typing: by treating anything that can go wrong as an error for the > caller to handle, I can replace thinking about what may go wrong and > what must not go wrong by typing up a bunch of error paths. Great time > saver as long as I stick to the time-honored strategy of not bothering > to test my error paths. Not all are easily testable :( Or do you have a recomendation? Like forcing an error in the code while developing, so the path is checked? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size 2020-07-21 9:06 ` Philippe Mathieu-Daudé @ 2020-07-21 9:55 ` Markus Armbruster 0 siblings, 0 replies; 8+ messages in thread From: Markus Armbruster @ 2020-07-21 9:55 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Andrew Jeffery, qemu-devel, qemu-arm, Joel Stanley, Cédric Le Goater Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 7/21/20 10:13 AM, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >> >>> On 7/20/20 6:07 PM, Cédric Le Goater wrote: >>>> On 7/20/20 11:58 AM, Philippe Mathieu-Daudé wrote: >>>>> The SDRAM Memory Controller has a 32-bit address bus, thus >>>>> supports up to 4 GiB of DRAM. There is a signed to unsigned >>>>> conversion error with the AST2600 maximum memory size: >>>>> >>>>> (uint64_t)(2048 << 20) = (uint64_t)(-2147483648) >>>>> = 0xffffffff40000000 >>>>> = 16 EiB - 2 GiB >>>>> >>>>> Fix by using the IEC suffixes which are usually safer, and add >>>>> a check to verify the memory is valid. This would have catched >> >> caught >> >>>>> this bug: >>>>> >>>>> Unexpected error in aspeed_sdmc_realize() at hw/misc/aspeed_sdmc.c:261: >>>>> qemu-system-arm: Invalid RAM size 16 EiB >>>> >>>> Indeed :/ >>>> >>>>> >>>>> Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support") >>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>> --- >>>>> hw/misc/aspeed_sdmc.c | 12 +++++++++--- >>>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c >>>>> index 0737d8de81..76dd7e6a20 100644 >>>>> --- a/hw/misc/aspeed_sdmc.c >>>>> +++ b/hw/misc/aspeed_sdmc.c >>>>> @@ -256,6 +256,12 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp) >>>>> AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s); >>>>> >>>>> s->max_ram_size = asc->max_ram_size; >>>>> + if (s->max_ram_size >= 4 * GiB) { >>>>> + char *szstr = size_to_str(s->max_ram_size); >>>>> + error_setg(errp, "Invalid RAM size %s", szstr); >>>>> + g_free(szstr); >>>>> + return; >>>>> + } >>>>> >>>> >>>> I would put an assert() since the max RAM size is not user configurable. >>> >>> As you wish, at this point I'm completely lost with error reporting. >> >> :-/ >> >>> Per the manual >>> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg723217.html): >>> >>> "Many, many devices neglect to clean up properly on error, and get away >>> with it only because all callers treat errors as fatal. >>> >>> If you decide to take cleanup shortcuts, say because the cleanup is >>> untestable, consider adding a comment at least." >>> >>> So I'll go for address + comment: >>> >>> assert(s->max_ram_size < 4 * GiB); /* 32-bit address bus */ >> >> Makes sense. >> >> Note this is *not* a cleanup shortcut, at least not the kind I had in >> mind. >> >> What I had in mind is unclean failure, i.e. returning on error without >> proper cleanup: revert changes made so far, free resources. This is >> *wrong*. But the wrongness doesn't matter when all callers treat errors >> as fatal. >> >> Checking an impossible condition with assert() is better than treating >> it as an error and bungling its handling. If you treat it as an error, >> do it properly. Since I'm quite skeptical about the chances of pulling >> off "properly" for untestable things, I prefer assertions. >> >> There's another reason. User errors need to be handled gracefully. >> Programming errors should (in my opinion) trigger abort(), so they get >> fixed. >> >> When the spot that detects the error can't know which kind it is, you >> have to fail cleanly and let the caller decide how to handle the error. >> >> Example: object_property_find() errors out when the property doesn't >> exist. This may be a programming error, e.g. a well-known property >> isn't found, because a programmer mistyped the property name. Or it may >> be a user error, e.g. a user mistyped the property name argument of >> qom-get. >> >> When functions have multiple failure modes, and only some of them are >> programming errors, the caller typically can't tell them apart, and errs >> on the side of user error. Programming errors then get reported as >> (typically confusing!) user errors. > > A big part of your reply is worth adding in a "How to correctly use the > Error* propagation API for dummies" in docs/devel document. Right now, that document lives in error.h. Has it outgrown that home? >> The #1 reason for such awkward functions is lazy thinking + eager >> typing: by treating anything that can go wrong as an error for the >> caller to handle, I can replace thinking about what may go wrong and >> what must not go wrong by typing up a bunch of error paths. Great time >> saver as long as I stick to the time-honored strategy of not bothering >> to test my error paths. > > Not all are easily testable :( Or do you have a recomendation? Like > forcing an error in the code while developing, so the path is checked? I prefer to at least understand how errors can happen. The understanding often leads to a way to test, or to the realization that it's actually a programming error. When the way to test requires some "unobtanium", i.e. hardware or software I can't access or set up with reasonable effort, I feel okay adding mockup code for testing, as a best effort. Proper test coverage is then up to the people with the necessary access and knowledge. There will always be the occasional edge case where you have to settle for less than proper testing. That's okay, life is short. All I really ask for is to at least *think* about the error paths and their testing. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-21 9:56 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-20 9:58 [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size Philippe Mathieu-Daudé 2020-07-20 10:04 ` no-reply 2020-07-20 10:04 ` no-reply 2020-07-20 16:07 ` Cédric Le Goater 2020-07-20 17:39 ` Philippe Mathieu-Daudé 2020-07-21 8:13 ` Markus Armbruster 2020-07-21 9:06 ` Philippe Mathieu-Daudé 2020-07-21 9:55 ` Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.