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

2024-03-28 Thread Harsh Prateek Bora




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.

2024-03-28 Thread Peter Maydell
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.

2024-03-27 Thread Thomas Huth

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.

2024-03-26 Thread Harsh Prateek Bora




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.

2024-03-26 Thread Peter Maydell
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.

2024-03-12 Thread Nicholas Piggin
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