On Tue, Jul 20, 2021 at 03:07:26PM +1000, Alexey Kardashevskiy wrote: > Coverity reported issues which are caused by mixing of signed return codes > from DTC and unsigned return codes of the client interface. > > This introduces PROM_ERROR and makes distinction between the error types. > > This fixes NEGATIVE_RETURNS, OVERRUN issues reported by Coverity. > > This adds a comment about the return parameters number in the VOF hcall. > The reason for such counting is to keep the numbers look the same in > vof_client_handle() and the Linux (an OF client). > > vmc->client_architecture_support() returns target_ulong and we want to > propagate this to the client (for example H_MULTI_THREADS_ACTIVE). > The VOF path to do_client_architecture_support() needs chopping off > the top 32bit but SLOF's H_CAS does not; and either way the return values > are either 0 or 32bit negative error code. For now this chops > the top 32bits. > > This makes "claim" fail if the allocated address is above 4GB as > the client interface is 32bit. This still allows claiming memory above > 4GB as potentially initrd can be put there and the client can read > the address from the FDT's "available" property. > > Fixes: CID 1458139, 1458138, 1458137, 1458133, 1458132 > Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
Well, I don't know for sure if it will fix every one, but it certainly
looks like an improvement. So, applied to ppc-for-6.1.
Couple of minor notes:
[snip]
> @@ -461,18 +458,18 @@ trace_exit:
> uint32_t vof_client_open_store(void *fdt, Vof *vof, const char *nodename,
> const char *prop, const char *path)
> {
> - int node = fdt_path_offset(fdt, nodename);
> - int inst, offset;
> + int offset, node = fdt_path_offset(fdt, nodename);
> + uint32_t inst;
>
> offset = fdt_path_offset(fdt, path);
> if (offset < 0) {
> trace_vof_error_unknown_path(path);
> - return offset;
> + return PROM_ERROR;
Note that this case was a real bug, not just Coverity being pedantic.
> }
>
> inst = vof_do_open(fdt, vof, offset, path);
>
> - return fdt_setprop_cell(fdt, node, prop, inst);
> + return fdt_setprop_cell(fdt, node, prop, inst) >= 0 ? 0 : PROM_ERROR;
> }
>
> static uint32_t vof_open(void *fdt, Vof *vof, uint32_t pathaddr)
> @@ -481,13 +478,13 @@ static uint32_t vof_open(void *fdt, Vof *vof, uint32_t
> pathaddr)
> int offset;
>
> if (readstr(pathaddr, path, sizeof(path))) {
> - return -1;
> + return PROM_ERROR;
> }
>
> offset = path_offset(fdt, path);
> if (offset < 0) {
> trace_vof_error_unknown_path(path);
> - return offset;
> + return PROM_ERROR;
As is this one.
[snip]
> @@ -826,7 +824,7 @@ trace_exit:
> static uint32_t vof_call_interpret(uint32_t cmdaddr, uint32_t param1,
> uint32_t param2, uint32_t *ret2)
> {
> - uint32_t ret = -1;
> + uint32_t ret = PROM_ERROR;
> char cmd[VOF_MAX_FORTHCODE] = "";
>
> /* No interpret implemented so just call a trace */
> @@ -895,13 +893,20 @@ static uint32_t vof_client_handle(MachineState *ms,
> void *fdt, Vof *vof,
> } else if (cmpserv("write", 3, 1)) {
> ret = vof_write(vof, args[0], args[1], args[2]);
> } else if (cmpserv("claim", 3, 1)) {
> - ret = vof_claim(vof, args[0], args[1], args[2]);
> - if (ret != -1) {
> + uint64_t ret64 = vof_claim(vof, args[0], args[1], args[2]);
> +
> + if (ret64 < 0x100000000UL) {
This assumes that 2^32 fits in an unsigned long; I'm not certainly if
that's true for all supported qemu host arches. ULL would be safer
here.
> vof_dt_memory_available(fdt, vof->claimed, vof->claimed_base);
> + ret = (uint32_t)ret64;
> + } else {
> + if (ret64 != -1) {
> + vof_release(vof, ret, args[1]);
> + }
> + ret = PROM_ERROR;
> }
> } else if (cmpserv("release", 2, 0)) {
> ret = vof_release(vof, args[0], args[1]);
> - if (ret != -1) {
> + if (ret != PROM_ERROR) {
> vof_dt_memory_available(fdt, vof->claimed, vof->claimed_base);
> }
> } else if (cmpserv("call-method", 0, 0)) {
> @@ -965,11 +970,15 @@ int vof_client_call(MachineState *ms, Vof *vof, void
> *fdt,
> }
>
> nret = be32_to_cpu(args_be.nret);
> + if (nret > ARRAY_SIZE(args_be.args) - nargs) {
> + return -EINVAL;
> + }
> ret = vof_client_handle(ms, fdt, vof, service, args, nargs, rets, nret);
> if (!nret) {
> return 0;
> }
>
> + /* @nrets includes the value which this function returns */
> args_be.args[nargs] = cpu_to_be32(ret);
> for (i = 1; i < nret; ++i) {
> args_be.args[nargs + i] = cpu_to_be32(rets[i - 1]);
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 6e90a0107247..da6e74b80dc1 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -88,8 +88,8 @@ vof_getproplen(uint32_t ph, const char *prop, uint32_t ret)
> "ph=0x%x \"%s\" => l
> vof_setprop(uint32_t ph, const char *prop, const char *val, uint32_t vallen,
> uint32_t ret) "ph=0x%x \"%s\" [%s] len=%d => ret=%d"
> vof_open(const char *path, uint32_t ph, uint32_t ih) "%s ph=0x%x => ih=0x%x"
> vof_interpret(const char *cmd, uint32_t param1, uint32_t param2, uint32_t
> ret, uint32_t ret2) "[%s] 0x%x 0x%x => 0x%x 0x%x"
> -vof_package_to_path(uint32_t ph, const char *tmp, uint32_t ret) "ph=0x%x =>
> %s len=%d"
> -vof_instance_to_path(uint32_t ih, uint32_t ph, const char *tmp, uint32_t
> ret) "ih=0x%x ph=0x%x => %s len=%d"
> +vof_package_to_path(uint32_t ph, const char *tmp, int ret) "ph=0x%x => %s
> len=%d"
> +vof_instance_to_path(uint32_t ih, uint32_t ph, const char *tmp, int ret)
> "ih=0x%x ph=0x%x => %s len=%d"
> vof_instance_to_package(uint32_t ih, uint32_t ph) "ih=0x%x => ph=0x%x"
> vof_write(uint32_t ih, unsigned cb, const char *msg) "ih=0x%x [%u] \"%s\""
> vof_avail(uint64_t start, uint64_t end, uint64_t size)
> "0x%"PRIx64"..0x%"PRIx64" size=0x%"PRIx64
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
