diff mbox series

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

Message ID 20240312165851.2240242-36-npiggin@gmail.com
State New
Headers show
Series [PULL,01/38] target/ppc: Fix GDB SPR regnum indexing | expand

Commit Message

Nicholas Piggin March 12, 2024, 4:58 p.m. UTC
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>
---
 hw/ppc/spapr_nested.c         | 268 ++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h        |   3 +
 include/hw/ppc/spapr_nested.h |  23 +++
 3 files changed, 294 insertions(+)

Comments

Peter Maydell March 26, 2024, 4:02 p.m. UTC | #1
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?

> +        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
Harsh Prateek Bora March 27, 2024, 5:41 a.m. UTC | #2
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
Thomas Huth March 27, 2024, 8:05 a.m. UTC | #3
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 <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?

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
Peter Maydell March 28, 2024, 3:25 p.m. UTC | #4
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") ?

> >> +        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
Harsh Prateek Bora March 29, 2024, 3:53 a.m. UTC | #5
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
diff mbox series

Patch

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 &guest_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, &len, &num_elements);
+
+    }
+    return true;
+}
+
+static bool is_gsr_invalid(struct guest_state_request *gsr,
+                                   struct guest_state_element *element,
+                                   struct guest_state_element_type *type)
+{
+    if ((gsr->flags & GUEST_STATE_REQUEST_SET) &&
+        (*(uint64_t *)(element->value) & ~(type->mask))) {
+        log_element(element, gsr);
+        qemu_log_mask(LOG_GUEST_ERROR, "L1 can't set reserved bits "
+                      "(allowed mask: 0x%08"PRIx64")\n", type->mask);
+        return true;
+    }
+    return false;
+}
+
 static target_ulong h_guest_get_capabilities(PowerPCCPU *cpu,
                                              SpaprMachineState *spapr,
                                              target_ulong opcode,
@@ -1264,6 +1398,136 @@  static target_ulong h_guest_create_vcpu(PowerPCCPU *cpu,
     return H_SUCCESS;
 }
 
+static target_ulong getset_state(SpaprMachineStateNestedGuest *guest,
+                                 uint64_t vcpuid,
+                                 struct guest_state_request *gsr)
+{
+    void *ptr;
+    uint16_t id;
+    struct guest_state_element *element;
+    struct guest_state_element_type *type;
+    int64_t lenleft, num_elements;
+
+    lenleft = gsr->len;
+
+    if (!guest_state_request_check(gsr)) {
+        return H_P3;
+    }
+
+    num_elements = be32_to_cpu(gsr->gsb->num_elements);
+    element = gsr->gsb->elements;
+    /* Process the elements */
+    while (num_elements) {
+        type = NULL;
+        /* log_element(element, gsr); */
+
+        id = be16_to_cpu(element->id);
+        if (id == GSB_HV_VCPU_IGNORED_ID) {
+            goto next_element;
+        }
+
+        type = guest_state_element_type_find(id);
+        assert(type);
+
+        /* Get pointer to guest data to get/set */
+        if (type->location && type->copy) {
+            ptr = type->location(guest, vcpuid);
+            assert(ptr);
+            if (!~(type->mask) && is_gsr_invalid(gsr, element, type)) {
+                return H_INVALID_ELEMENT_VALUE;
+            }
+            type->copy(ptr + type->offset, element->value,
+                       gsr->flags & GUEST_STATE_REQUEST_SET ? true : false);
+        }
+
+next_element:
+        element = guest_state_element_next(element, &lenleft, &num_elements);
+    }
+
+    return H_SUCCESS;
+}
+
+static target_ulong map_and_getset_state(PowerPCCPU *cpu,
+                                         SpaprMachineStateNestedGuest *guest,
+                                         uint64_t vcpuid,
+                                         struct guest_state_request *gsr)
+{
+    target_ulong rc;
+    int64_t len;
+    bool is_write;
+
+    len = gsr->len;
+    /* only get_state would require write access to the provided buffer */
+    is_write = (gsr->flags & GUEST_STATE_REQUEST_SET) ? false : true;
+    gsr->gsb = address_space_map(CPU(cpu)->as, gsr->buf, (uint64_t *)&len,
+                                 is_write, MEMTXATTRS_UNSPECIFIED);
+    if (!gsr->gsb) {
+        rc = H_P3;
+        goto out1;
+    }
+
+    if (len != gsr->len) {
+        rc = H_P3;
+        goto out1;
+    }
+
+    rc = getset_state(guest, vcpuid, gsr);
+
+out1:
+    address_space_unmap(CPU(cpu)->as, gsr->gsb, len, is_write, len);
+    return rc;
+}
+
+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) {
+        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);
+}
+
+static target_ulong h_guest_set_state(PowerPCCPU *cpu,
+                                      SpaprMachineState *spapr,
+                                      target_ulong opcode,
+                                      target_ulong *args)
+{
+    return h_guest_getset_state(cpu, spapr, args, true);
+}
+
+static target_ulong h_guest_get_state(PowerPCCPU *cpu,
+                                      SpaprMachineState *spapr,
+                                      target_ulong opcode,
+                                      target_ulong *args)
+{
+    return h_guest_getset_state(cpu, spapr, args, false);
+}
+
 void spapr_register_nested_hv(void)
 {
     spapr_register_hypercall(KVMPPC_H_SET_PARTITION_TABLE, h_set_ptbl);
@@ -1289,6 +1553,8 @@  void spapr_register_nested_papr(void)
     spapr_register_hypercall(H_GUEST_CREATE, h_guest_create);
     spapr_register_hypercall(H_GUEST_DELETE, h_guest_delete);
     spapr_register_hypercall(H_GUEST_CREATE_VCPU, h_guest_create_vcpu);
+    spapr_register_hypercall(H_GUEST_SET_STATE, h_guest_set_state);
+    spapr_register_hypercall(H_GUEST_GET_STATE, h_guest_get_state);
 }
 
 void spapr_unregister_nested_papr(void)
@@ -1298,6 +1564,8 @@  void spapr_unregister_nested_papr(void)
     spapr_unregister_hypercall(H_GUEST_CREATE);
     spapr_unregister_hypercall(H_GUEST_DELETE);
     spapr_unregister_hypercall(H_GUEST_CREATE_VCPU);
+    spapr_unregister_hypercall(H_GUEST_SET_STATE);
+    spapr_unregister_hypercall(H_GUEST_GET_STATE);
 }
 
 #else
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 070135793a..6223873641 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -366,6 +366,7 @@  struct SpaprMachineState {
 #define H_OVERLAP         -68
 #define H_STATE           -75
 #define H_IN_USE          -77
+#define H_INVALID_ELEMENT_VALUE            -81
 #define H_UNSUPPORTED_FLAG -256
 #define H_MULTI_THREADS_ACTIVE -9005
 
@@ -589,6 +590,8 @@  struct SpaprMachineState {
 #define H_GUEST_SET_CAPABILITIES 0x464
 #define H_GUEST_CREATE           0x470
 #define H_GUEST_CREATE_VCPU      0x474
+#define H_GUEST_GET_STATE        0x478
+#define H_GUEST_SET_STATE        0x47C
 #define H_GUEST_DELETE           0x488
 
 #define MAX_HCALL_OPCODE         H_GUEST_DELETE
diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
index 3da02df7bb..bf9b258c29 100644
--- a/include/hw/ppc/spapr_nested.h
+++ b/include/hw/ppc/spapr_nested.h
@@ -224,6 +224,10 @@  typedef struct SpaprMachineStateNestedGuest {
 #define HVMASK_MSR                    0xEBFFFFFFFFBFEFFF
 #define HVMASK_HDEXCR                 0x00000000FFFFFFFF
 #define HVMASK_TB_OFFSET              0x000000FFFFFFFFFF
+#define GSB_MAX_BUF_SIZE              (1024 * 1024)
+#define H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE 0x8000000000000000
+#define GUEST_STATE_REQUEST_GUEST_WIDE       0x1
+#define GUEST_STATE_REQUEST_SET              0x2
 
 /*
  * As per ISA v3.1B, following bits are reserved:
@@ -322,6 +326,25 @@  typedef struct SpaprMachineStateNestedGuest {
 #define GSE_ENV_DWM(i, f, m) \
     GUEST_STATE_ELEMENT_MSK(i, 8, f, copy_state_8to8, m)
 
+struct guest_state_element {
+    uint16_t id;
+    uint16_t size;
+    uint8_t value[];
+} QEMU_PACKED;
+
+struct guest_state_buffer {
+    uint32_t num_elements;
+    struct guest_state_element elements[];
+} QEMU_PACKED;
+
+/* Actual buffer plus some metadata about the request */
+struct guest_state_request {
+    struct guest_state_buffer *gsb;
+    int64_t buf;
+    int64_t len;
+    uint16_t flags;
+};
+
 /*
  * Register state for entering a nested guest with H_ENTER_NESTED.
  * New member must be added at the end.