[PATCH v4 15/15] spapr: nested: Set the PCR when logical PVR is set

Harsh Prateek Bora posted 15 patches 8 months, 3 weeks ago
Maintainers: Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>
There is a newer version of this series
[PATCH v4 15/15] spapr: nested: Set the PCR when logical PVR is set
Posted by Harsh Prateek Bora 8 months, 3 weeks ago
From: Amit Machhiwal <amachhiw@linux.vnet.ibm.com>

In APIv1, KVM L0 sets the PCR, while in the nested papr APIv2, this
doesn't work as the PCR can't be set via the guest state buffer; the
logical PVR is set via the GSB though.

This change sets the PCR whenever the logical PVR is set via the GSB.
Also, unlike the other registers, the value 1 in a defined bit in the
PCR makes the affected resources unavailable and the value 0 makes
them available. Hence, the PCR is set accordingly.

Signed-off-by: Amit Machhiwal <amachhiw@linux.vnet.ibm.com>
Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
---
 include/hw/ppc/spapr_nested.h |  9 +++++++++
 hw/ppc/spapr_nested.c         | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
index da918d2dd0..f67c721f53 100644
--- a/include/hw/ppc/spapr_nested.h
+++ b/include/hw/ppc/spapr_nested.h
@@ -229,6 +229,15 @@ typedef struct SpaprMachineStateNestedGuest {
 #define GUEST_STATE_REQUEST_GUEST_WIDE       0x1
 #define GUEST_STATE_REQUEST_SET              0x2
 
+/* As per ISA v3.1B, following bits are reserved:
+ *      0:2
+ *      4:57  (ISA mentions bit 58 as well but it should be used for P10)
+ *      61:63 (hence, haven't included PCR bits for v2.06 and v2.05
+ *             in LOW BITS)
+ */
+#define PCR_LOW_BITS   (PCR_COMPAT_3_10 | PCR_COMPAT_3_00)
+#define HVMASK_PCR     ~PCR_LOW_BITS
+
 #define GUEST_STATE_ELEMENT(i, sz, s, f, ptr, c) { \
     .id = (i),                                     \
     .size = (sz),                                  \
diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
index 6e6a90616e..af8a482337 100644
--- a/hw/ppc/spapr_nested.c
+++ b/hw/ppc/spapr_nested.c
@@ -740,9 +740,11 @@ static void out_buf_min_size(void *a, void *b, bool set)
 
 static void copy_logical_pvr(void *a, void *b, bool set)
 {
+    SpaprMachineStateNestedGuest *guest;
     uint32_t *buf; /* 1 word */
     uint32_t *pvr_logical_ptr;
     uint32_t pvr_logical;
+    target_ulong pcr = 0;
 
     pvr_logical_ptr = a;
     buf = b;
@@ -755,6 +757,28 @@ static void copy_logical_pvr(void *a, void *b, bool set)
     pvr_logical = be32_to_cpu(buf[0]);
 
     *pvr_logical_ptr = pvr_logical;
+
+    if (*pvr_logical_ptr) {
+        switch (*pvr_logical_ptr) {
+            case CPU_POWERPC_LOGICAL_3_10:
+                pcr = PCR_COMPAT_3_10 | PCR_COMPAT_3_00;
+                break;
+            case CPU_POWERPC_LOGICAL_3_00:
+                pcr = PCR_COMPAT_3_00;
+                break;
+            default:
+                qemu_log_mask(LOG_GUEST_ERROR,
+                    "Could not set PCR for LPVR=0x%08x\n", *pvr_logical_ptr);
+                return;
+        }
+    }
+
+    guest = container_of(pvr_logical_ptr,
+                         struct SpaprMachineStateNestedGuest,
+                         pvr_logical);
+    for (int i = 0; i < guest->vcpus; i++) {
+        guest->vcpu[i].state.pcr = ~pcr | HVMASK_PCR;
+    }
 }
 
 static void copy_tb_offset(void *a, void *b, bool set)
-- 
2.39.3
Re: [PATCH v4 15/15] spapr: nested: Set the PCR when logical PVR is set
Posted by Nicholas Piggin 8 months, 2 weeks ago
On Tue Feb 20, 2024 at 6:36 PM AEST, Harsh Prateek Bora wrote:
> From: Amit Machhiwal <amachhiw@linux.vnet.ibm.com>
>
> In APIv1, KVM L0 sets the PCR, while in the nested papr APIv2, this
> doesn't work as the PCR can't be set via the guest state buffer; the
> logical PVR is set via the GSB though.
>
> This change sets the PCR whenever the logical PVR is set via the GSB.
> Also, unlike the other registers, the value 1 in a defined bit in the
> PCR makes the affected resources unavailable and the value 0 makes
> them available. Hence, the PCR is set accordingly.

Should this be squashed in as a fix?

Thanks,
Nick

>
> Signed-off-by: Amit Machhiwal <amachhiw@linux.vnet.ibm.com>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> ---
>  include/hw/ppc/spapr_nested.h |  9 +++++++++
>  hw/ppc/spapr_nested.c         | 24 ++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> index da918d2dd0..f67c721f53 100644
> --- a/include/hw/ppc/spapr_nested.h
> +++ b/include/hw/ppc/spapr_nested.h
> @@ -229,6 +229,15 @@ typedef struct SpaprMachineStateNestedGuest {
>  #define GUEST_STATE_REQUEST_GUEST_WIDE       0x1
>  #define GUEST_STATE_REQUEST_SET              0x2
>  
> +/* As per ISA v3.1B, following bits are reserved:
> + *      0:2
> + *      4:57  (ISA mentions bit 58 as well but it should be used for P10)
> + *      61:63 (hence, haven't included PCR bits for v2.06 and v2.05
> + *             in LOW BITS)
> + */
> +#define PCR_LOW_BITS   (PCR_COMPAT_3_10 | PCR_COMPAT_3_00)
> +#define HVMASK_PCR     ~PCR_LOW_BITS
> +
>  #define GUEST_STATE_ELEMENT(i, sz, s, f, ptr, c) { \
>      .id = (i),                                     \
>      .size = (sz),                                  \
> diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> index 6e6a90616e..af8a482337 100644
> --- a/hw/ppc/spapr_nested.c
> +++ b/hw/ppc/spapr_nested.c
> @@ -740,9 +740,11 @@ static void out_buf_min_size(void *a, void *b, bool set)
>  
>  static void copy_logical_pvr(void *a, void *b, bool set)
>  {
> +    SpaprMachineStateNestedGuest *guest;
>      uint32_t *buf; /* 1 word */
>      uint32_t *pvr_logical_ptr;
>      uint32_t pvr_logical;
> +    target_ulong pcr = 0;
>  
>      pvr_logical_ptr = a;
>      buf = b;
> @@ -755,6 +757,28 @@ static void copy_logical_pvr(void *a, void *b, bool set)
>      pvr_logical = be32_to_cpu(buf[0]);
>  
>      *pvr_logical_ptr = pvr_logical;
> +
> +    if (*pvr_logical_ptr) {
> +        switch (*pvr_logical_ptr) {
> +            case CPU_POWERPC_LOGICAL_3_10:
> +                pcr = PCR_COMPAT_3_10 | PCR_COMPAT_3_00;
> +                break;
> +            case CPU_POWERPC_LOGICAL_3_00:
> +                pcr = PCR_COMPAT_3_00;
> +                break;
> +            default:
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                    "Could not set PCR for LPVR=0x%08x\n", *pvr_logical_ptr);
> +                return;
> +        }
> +    }
> +
> +    guest = container_of(pvr_logical_ptr,
> +                         struct SpaprMachineStateNestedGuest,
> +                         pvr_logical);
> +    for (int i = 0; i < guest->vcpus; i++) {
> +        guest->vcpu[i].state.pcr = ~pcr | HVMASK_PCR;
> +    }
>  }
>  
>  static void copy_tb_offset(void *a, void *b, bool set)
Re: [PATCH v4 15/15] spapr: nested: Set the PCR when logical PVR is set
Posted by Harsh Prateek Bora 8 months, 1 week ago

On 2/27/24 15:53, Nicholas Piggin wrote:
> On Tue Feb 20, 2024 at 6:36 PM AEST, Harsh Prateek Bora wrote:
>> From: Amit Machhiwal <amachhiw@linux.vnet.ibm.com>
>>
>> In APIv1, KVM L0 sets the PCR, while in the nested papr APIv2, this
>> doesn't work as the PCR can't be set via the guest state buffer; the
>> logical PVR is set via the GSB though.
>>
>> This change sets the PCR whenever the logical PVR is set via the GSB.
>> Also, unlike the other registers, the value 1 in a defined bit in the
>> PCR makes the affected resources unavailable and the value 0 makes
>> them available. Hence, the PCR is set accordingly.
> 
> Should this be squashed in as a fix?

Yeh, it can be squashed with 10/15 GSB initialization patch, will update 
as suggested in v5.

regards,
Harsh
> 
> Thanks,
> Nick
> 
>>
>> Signed-off-by: Amit Machhiwal <amachhiw@linux.vnet.ibm.com>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> ---
>>   include/hw/ppc/spapr_nested.h |  9 +++++++++
>>   hw/ppc/spapr_nested.c         | 24 ++++++++++++++++++++++++
>>   2 files changed, 33 insertions(+)
>>
>> diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
>> index da918d2dd0..f67c721f53 100644
>> --- a/include/hw/ppc/spapr_nested.h
>> +++ b/include/hw/ppc/spapr_nested.h
>> @@ -229,6 +229,15 @@ typedef struct SpaprMachineStateNestedGuest {
>>   #define GUEST_STATE_REQUEST_GUEST_WIDE       0x1
>>   #define GUEST_STATE_REQUEST_SET              0x2
>>   
>> +/* As per ISA v3.1B, following bits are reserved:
>> + *      0:2
>> + *      4:57  (ISA mentions bit 58 as well but it should be used for P10)
>> + *      61:63 (hence, haven't included PCR bits for v2.06 and v2.05
>> + *             in LOW BITS)
>> + */
>> +#define PCR_LOW_BITS   (PCR_COMPAT_3_10 | PCR_COMPAT_3_00)
>> +#define HVMASK_PCR     ~PCR_LOW_BITS
>> +
>>   #define GUEST_STATE_ELEMENT(i, sz, s, f, ptr, c) { \
>>       .id = (i),                                     \
>>       .size = (sz),                                  \
>> diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
>> index 6e6a90616e..af8a482337 100644
>> --- a/hw/ppc/spapr_nested.c
>> +++ b/hw/ppc/spapr_nested.c
>> @@ -740,9 +740,11 @@ static void out_buf_min_size(void *a, void *b, bool set)
>>   
>>   static void copy_logical_pvr(void *a, void *b, bool set)
>>   {
>> +    SpaprMachineStateNestedGuest *guest;
>>       uint32_t *buf; /* 1 word */
>>       uint32_t *pvr_logical_ptr;
>>       uint32_t pvr_logical;
>> +    target_ulong pcr = 0;
>>   
>>       pvr_logical_ptr = a;
>>       buf = b;
>> @@ -755,6 +757,28 @@ static void copy_logical_pvr(void *a, void *b, bool set)
>>       pvr_logical = be32_to_cpu(buf[0]);
>>   
>>       *pvr_logical_ptr = pvr_logical;
>> +
>> +    if (*pvr_logical_ptr) {
>> +        switch (*pvr_logical_ptr) {
>> +            case CPU_POWERPC_LOGICAL_3_10:
>> +                pcr = PCR_COMPAT_3_10 | PCR_COMPAT_3_00;
>> +                break;
>> +            case CPU_POWERPC_LOGICAL_3_00:
>> +                pcr = PCR_COMPAT_3_00;
>> +                break;
>> +            default:
>> +                qemu_log_mask(LOG_GUEST_ERROR,
>> +                    "Could not set PCR for LPVR=0x%08x\n", *pvr_logical_ptr);
>> +                return;
>> +        }
>> +    }
>> +
>> +    guest = container_of(pvr_logical_ptr,
>> +                         struct SpaprMachineStateNestedGuest,
>> +                         pvr_logical);
>> +    for (int i = 0; i < guest->vcpus; i++) {
>> +        guest->vcpu[i].state.pcr = ~pcr | HVMASK_PCR;
>> +    }
>>   }
>>   
>>   static void copy_tb_offset(void *a, void *b, bool set)
>