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

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

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.

Hi Nick,
Do you think keeping both nested APIs (i.e. entire spapr_nested.c)
within #ifdef TARGET_PPC64 would be a better choice here?

regards,
Harsh

> 
>> +        gsr.flags |= GUEST_STATE_REQUEST_GUEST_WIDE;
>> +    }
>> +    if (flags & !H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE) {
>> +        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