On Wed, 27 Mar 2024 at 05:41, Harsh Prateek Bora <hars...@linux.ibm.com> wrote: > > > > On 3/26/24 21:32, Peter Maydell wrote: > > On Tue, 12 Mar 2024 at 17:11, Nicholas Piggin <npig...@gmail.com> wrote: > >> > >> From: Harsh Prateek Bora <hars...@linux.ibm.com> > >> > >> Introduce the nested PAPR hcalls: > >> - H_GUEST_GET_STATE which is used to get state of a nested guest or > >> a guest VCPU. The value field for each element in the request is > >> destination to be updated to reflect current state on success. > >> - H_GUEST_SET_STATE which is used to modify the state of a guest or > >> a guest VCPU. On success, guest (or its VCPU) state shall be > >> updated as per the value field for the requested element(s). > >> > >> Reviewed-by: Nicholas Piggin <npig...@gmail.com> > >> Signed-off-by: Michael Neuling <mi...@neuling.org> > >> Signed-off-by: Harsh Prateek Bora <hars...@linux.ibm.com> > >> Signed-off-by: Nicholas Piggin <npig...@gmail.com> > > > > Hi; Coverity points out a problem with this code (CID 1540008, 1540009): > > > > > > > >> +static target_ulong h_guest_getset_state(PowerPCCPU *cpu, > >> + SpaprMachineState *spapr, > >> + target_ulong *args, > >> + bool set) > >> +{ > >> + target_ulong flags = args[0]; > >> + target_ulong lpid = args[1]; > >> + target_ulong vcpuid = args[2]; > >> + target_ulong buf = args[3]; > >> + target_ulong buflen = args[4]; > >> + struct guest_state_request gsr; > >> + SpaprMachineStateNestedGuest *guest; > >> + > >> + guest = spapr_get_nested_guest(spapr, lpid); > >> + if (!guest) { > >> + return H_P2; > >> + } > >> + gsr.buf = buf; > >> + assert(buflen <= GSB_MAX_BUF_SIZE); > >> + gsr.len = buflen; > >> + gsr.flags = 0; > >> + if (flags & H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE) { > > > > flags is a target_ulong, which means it might only be 32 bits. > > But H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE has a bit set in the > > upper 32 bits only. So Coverity complains about this condition > > being always-zero and the body of the if being dead code. > > > > What was the intention here? > > Hi Peter, > Ideally this is intended to be running on a ppc64 where target_ulong > should be uint64_t. I guess same holds true for existing nested-hv code > as well.
Sorry, I'm afraid I misread the Coverity report here; sorry for the confusion. The 32-vs-64 bits question is a red herring. What Coverity is actually pointing out is in this next bit: > >> + gsr.flags |= GUEST_STATE_REQUEST_GUEST_WIDE; > >> + } > >> + if (flags & !H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE) { The C operator ! is the logical-NOT operator; since H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE is a non-zero value that means that !H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE is 0; so we're testing (flags & 0), which is always false, and this is the if() body which is dead-code as a result. Should this be the bitwise-NOT ~ (ie "if any flag other than this one is set"), or should this be an else clause to the previous if() (ie "if this flag is not set") ? > >> + return H_PARAMETER; /* flag not supported yet */ > >> + } > >> + > >> + if (set) { > >> + gsr.flags |= GUEST_STATE_REQUEST_SET; > >> + } > >> + return map_and_getset_state(cpu, guest, vcpuid, &gsr); > >> +} > > thanks -- PMM