Re: [PATCH RESEND 03/15] ppc: spapr: Use SpaprMachineStateNested's ptcr instead of nested_ptcr

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

On 9/7/23 06:43, Nicholas Piggin wrote:
> On Wed Sep 6, 2023 at 2:33 PM AEST, Harsh Prateek Bora wrote:
>> Use nested guest state specific struct for storing related info.
> 
> So this is the patch I would introduce the SpaprMachineStateNested
> struct, with just the .ptrc member. Add other members to it as they
> are used in later patches.

Sure, will do.

> 
>>
>> Signed-off-by: Michael Neuling <mikey@neuling.org>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> ---
>>   hw/ppc/spapr.c         | 4 ++--
>>   hw/ppc/spapr_nested.c  | 4 ++--
>>   include/hw/ppc/spapr.h | 3 ++-
>>   3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 07e91e3800..e44686b04d 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1340,8 +1340,8 @@ static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
>>   
>>           assert(lpid != 0);
>>   
>> -        patb = spapr->nested_ptcr & PTCR_PATB;
>> -        pats = spapr->nested_ptcr & PTCR_PATS;
>> +        patb = spapr->nested.ptcr & PTCR_PATB;
>> +        pats = spapr->nested.ptcr & PTCR_PATS;
>>   
>>           /* Check if partition table is properly aligned */
>>           if (patb & MAKE_64BIT_MASK(0, pats + 12)) {
> 
> At this point I wonder if we should first move the nested part of
> spapr_get_pate into nested code. It's a bit of a wart to have it
> here when most of the other nested cases are abstracted from non
> nested code quite well.

Yeh, I also felt similar when modifying the existing nested code here.
Let me do the needful in next version.

> 
>> diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
>> index 121aa96ddc..a669470f1a 100644
>> --- a/hw/ppc/spapr_nested.c
>> +++ b/hw/ppc/spapr_nested.c
>> @@ -25,7 +25,7 @@ static target_ulong h_set_ptbl(PowerPCCPU *cpu,
>>           return H_PARAMETER;
>>       }
>>   
>> -    spapr->nested_ptcr = ptcr; /* Save new partition table */
>> +    spapr->nested.ptcr = ptcr; /* Save new partition table */
>>   
>>       return H_SUCCESS;
>>   }
>> @@ -157,7 +157,7 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>>       struct kvmppc_pt_regs *regs;
>>       hwaddr len;
>>   
>> -    if (spapr->nested_ptcr == 0) {
>> +    if (spapr->nested.ptcr == 0) {
>>           return H_NOT_AVAILABLE;
>>       }
>>   
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 3990fed1d9..c8b42af430 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -12,6 +12,7 @@
>>   #include "hw/ppc/spapr_xive.h"  /* For SpaprXive */
>>   #include "hw/ppc/xics.h"        /* For ICSState */
>>   #include "hw/ppc/spapr_tpm_proxy.h"
>> +#include "hw/ppc/spapr_nested.h" /* for SpaprMachineStateNested */
>>   
>>   struct SpaprVioBus;
>>   struct SpaprPhbState;
>> @@ -216,7 +217,7 @@ struct SpaprMachineState {
>>       uint32_t vsmt;       /* Virtual SMT mode (KVM's "core stride") */
>>   
>>       /* Nested HV support (TCG only) */
>> -    uint64_t nested_ptcr;
>> +    struct SpaprMachineStateNested nested;
> 
> I think convention says to use the typedef for these?

Sure, will update. Thanks.

regards,
Harsh

> 
> Thanks,
> Nick
> 
>>   
>>       Notifier epow_notifier;
>>       QTAILQ_HEAD(, SpaprEventLogEntry) pending_events;
>