Re: [PULL 35/38] spapr: nested: Introduce H_GUEST_[GET|SET]_STATE hcalls.

[Harsh Prateek Bora]

On 3/28/24 20:55, Peter Maydell wrote:
> On Wed, 27 Mar 2024 at 05:41, Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
> > On 3/26/24 21:32, Peter Maydell wrote:
> > > On Tue, 12 Mar 2024 at 17:11, Nicholas Piggin <npiggin@gmail.com> wrote:
> > > > From: Harsh Prateek Bora <harshpb@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 <npiggin@gmail.com>
> > > > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > > > Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > > > Signed-off-by: Nicholas Piggin <npiggin@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") ?

Oh, this should have been bitwise-NOT, I shall send a follow-up patch 
for the fix.

regards,
Harsh
> > > > +        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