Re: [PULL 35/38] spapr: nested: Introduce H_GUEST_[GET|SET]_STATE hcalls.
On 3/28/24 20:55, Peter Maydell wrote: On Wed, 27 Mar 2024 at 05:41, Harsh Prateek Bora wrote: On 3/26/24 21:32, Peter Maydell wrote: On Tue, 12 Mar 2024 at 17:11, Nicholas Piggin wrote: From: Harsh Prateek Bora 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 Signed-off-by: Michael Neuling Signed-off-by: Harsh Prateek Bora Signed-off-by: Nicholas Piggin 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, ); +} thanks -- PMM
Re: [PULL 35/38] spapr: nested: Introduce H_GUEST_[GET|SET]_STATE hcalls.
On Wed, 27 Mar 2024 at 05:41, Harsh Prateek Bora wrote: > > > > On 3/26/24 21:32, Peter Maydell wrote: > > On Tue, 12 Mar 2024 at 17:11, Nicholas Piggin wrote: > >> > >> From: Harsh Prateek Bora > >> > >> 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 > >> Signed-off-by: Michael Neuling > >> Signed-off-by: Harsh Prateek Bora > >> Signed-off-by: Nicholas Piggin > > > > 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, ); > >> +} > > thanks -- PMM
Re: [PULL 35/38] spapr: nested: Introduce H_GUEST_[GET|SET]_STATE hcalls.
On 27/03/2024 06.41, Harsh Prateek Bora wrote: On 3/26/24 21:32, Peter Maydell wrote: On Tue, 12 Mar 2024 at 17:11, Nicholas Piggin wrote: From: Harsh Prateek Bora 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 Signed-off-by: Michael Neuling Signed-off-by: Harsh Prateek Bora Signed-off-by: Nicholas Piggin 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? spapr_numa.c is only included with CONFIG_PSERIES in hw/ppc/meson.build, so that should already take care that this code is only compiled with a 64-bit target ... Can we somehow teach Coverity to take that into consideration? Thomas
Re: [PULL 35/38] spapr: nested: Introduce H_GUEST_[GET|SET]_STATE hcalls.
On 3/26/24 21:32, Peter Maydell wrote: On Tue, 12 Mar 2024 at 17:11, Nicholas Piggin wrote: From: Harsh Prateek Bora 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 Signed-off-by: Michael Neuling Signed-off-by: Harsh Prateek Bora Signed-off-by: Nicholas Piggin 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, ); +} thanks -- PMM
Re: [PULL 35/38] spapr: nested: Introduce H_GUEST_[GET|SET]_STATE hcalls.
On Tue, 12 Mar 2024 at 17:11, Nicholas Piggin wrote: > > From: Harsh Prateek Bora > > 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 > Signed-off-by: Michael Neuling > Signed-off-by: Harsh Prateek Bora > Signed-off-by: Nicholas Piggin 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? > +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, ); > +} thanks -- PMM
[PULL 35/38] spapr: nested: Introduce H_GUEST_[GET|SET]_STATE hcalls.
From: Harsh Prateek Bora 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 Signed-off-by: Michael Neuling Signed-off-by: Harsh Prateek Bora Signed-off-by: Nicholas Piggin --- hw/ppc/spapr_nested.c | 268 ++ include/hw/ppc/spapr.h| 3 + include/hw/ppc/spapr_nested.h | 23 +++ 3 files changed, 294 insertions(+) diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c index cd7fa90350..cea282926f 100644 --- a/hw/ppc/spapr_nested.c +++ b/hw/ppc/spapr_nested.c @@ -1029,6 +1029,140 @@ void spapr_nested_gsb_init(void) } } +static struct guest_state_element *guest_state_element_next( +struct guest_state_element *element, +int64_t *len, +int64_t *num_elements) +{ +uint16_t size; + +/* size is of element->value[] only. Not whole guest_state_element */ +size = be16_to_cpu(element->size); + +if (len) { +*len -= size + offsetof(struct guest_state_element, value); +} + +if (num_elements) { +*num_elements -= 1; +} + +return (struct guest_state_element *)(element->value + size); +} + +static +struct guest_state_element_type *guest_state_element_type_find(uint16_t id) +{ +int i; + +for (i = 0; i < ARRAY_SIZE(guest_state_element_types); i++) +if (id == guest_state_element_types[i].id) { +return _state_element_types[i]; +} + +return NULL; +} + +static void log_element(struct guest_state_element *element, +struct guest_state_request *gsr) +{ +qemu_log_mask(LOG_GUEST_ERROR, "h_guest_%s_state id:0x%04x size:0x%04x", + gsr->flags & GUEST_STATE_REQUEST_SET ? "set" : "get", + be16_to_cpu(element->id), be16_to_cpu(element->size)); +qemu_log_mask(LOG_GUEST_ERROR, "buf:0x%016"PRIx64" ...\n", + be64_to_cpu(*(uint64_t *)element->value)); +} + +static bool guest_state_request_check(struct guest_state_request *gsr) +{ +int64_t num_elements, len = gsr->len; +struct guest_state_buffer *gsb = gsr->gsb; +struct guest_state_element *element; +struct guest_state_element_type *type; +uint16_t id, size; + +/* gsb->num_elements = 0 == 32 bits long */ +assert(len >= 4); + +num_elements = be32_to_cpu(gsb->num_elements); +element = gsb->elements; +len -= sizeof(gsb->num_elements); + +/* Walk the buffer to validate the length */ +while (num_elements) { + +id = be16_to_cpu(element->id); +size = be16_to_cpu(element->size); + +if (false) { +log_element(element, gsr); +} +/* buffer size too small */ +if (len < 0) { +return false; +} + +type = guest_state_element_type_find(id); +if (!type) { +qemu_log_mask(LOG_GUEST_ERROR, "Element ID %04x unknown\n", id); +log_element(element, gsr); +return false; +} + +if (id == GSB_HV_VCPU_IGNORED_ID) { +goto next_element; +} + +if (size != type->size) { +qemu_log_mask(LOG_GUEST_ERROR, "Size mismatch. Element ID:%04x." + "Size Exp:%i Got:%i\n", id, type->size, size); +log_element(element, gsr); +return false; +} + +if ((type->flags & GUEST_STATE_ELEMENT_TYPE_FLAG_READ_ONLY) && +(gsr->flags & GUEST_STATE_REQUEST_SET)) { +qemu_log_mask(LOG_GUEST_ERROR, "Trying to set a read-only Element " + "ID:%04x.\n", id); +return false; +} + +if (type->flags & GUEST_STATE_ELEMENT_TYPE_FLAG_GUEST_WIDE) { +/* guest wide element type */ +if (!(gsr->flags & GUEST_STATE_REQUEST_GUEST_WIDE)) { +qemu_log_mask(LOG_GUEST_ERROR, "trying to set a guest wide " + "Element ID:%04x.\n", id); +return false; +} +} else { +/* thread wide element type */ +if (gsr->flags & GUEST_STATE_REQUEST_GUEST_WIDE) { +qemu_log_mask(LOG_GUEST_ERROR, "trying to set a thread wide " + "Element ID:%04x.\n", id); +return false; +} +} +next_element: +element = guest_state_element_next(element, , _elements); + +} +return true; +} + +static bool is_gsr_invalid(struct guest_state_request *gsr, + struct guest_state_element *element, + struct