Re: [PATCH RESEND 02/15] ppc: spapr: Add new/extend structs to support Nested PAPR API

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

On 9/7/23 06:36, Nicholas Piggin wrote:
> On Wed Sep 6, 2023 at 2:33 PM AEST, Harsh Prateek Bora wrote:
>> This patch introduces new data structures to be used with Nested PAPR
>> API. Also extends kvmppc_hv_guest_state with additional set of registers
>> supported with nested PAPR API.
>>
>> Signed-off-by: Michael Neuling <mikey@neuling.org>
>> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> ---
>>   include/hw/ppc/spapr_nested.h | 48 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>
>> diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
>> index 5cb668dd53..f8db31075b 100644
>> --- a/include/hw/ppc/spapr_nested.h
>> +++ b/include/hw/ppc/spapr_nested.h
>> @@ -189,6 +189,39 @@
>>   /* End of list of Guest State Buffer Element IDs */
>>   #define GSB_LAST                GSB_VCPU_SPR_ASDR
>>   
>> +typedef struct SpaprMachineStateNestedGuest {
>> +    unsigned long vcpus;
>> +    struct SpaprMachineStateNestedGuestVcpu *vcpu;
>> +    uint64_t parttbl[2];
>> +    uint32_t pvr_logical;
>> +    uint64_t tb_offset;
>> +} SpaprMachineStateNestedGuest;
>> +
>> +struct SpaprMachineStateNested {
>> +
>> +    uint8_t api;
>> +#define NESTED_API_KVM_HV  1
>> +#define NESTED_API_PAPR    2
>> +    uint64_t ptcr;
>> +    uint32_t lpid_max;
>> +    uint32_t pvr_base;
>> +    bool capabilities_set;
>> +    GHashTable *guests;
>> +};
>> +
>> +struct SpaprMachineStateNestedGuestVcpuRunBuf {
>> +    uint64_t addr;
>> +    uint64_t size;
>> +};
>> +
>> +typedef struct SpaprMachineStateNestedGuestVcpu {
>> +    bool enabled;
>> +    struct SpaprMachineStateNestedGuestVcpuRunBuf runbufin;
>> +    struct SpaprMachineStateNestedGuestVcpuRunBuf runbufout;
>> +    CPUPPCState env;
>> +    int64_t tb_offset;
>> +    int64_t dec_expiry_tb;
>> +} SpaprMachineStateNestedGuestVcpu;
>>   
>>   /*
>>    * Register state for entering a nested guest with H_ENTER_NESTED.
>> @@ -228,6 +261,21 @@ struct kvmppc_hv_guest_state {
>>       uint64_t dawr1;
>>       uint64_t dawrx1;
>>       /* Version 2 ends here */
>> +    uint64_t dec;
>> +    uint64_t fscr;
>> +    uint64_t fpscr;
>> +    uint64_t bescr;
>> +    uint64_t ebbhr;
>> +    uint64_t ebbrr;
>> +    uint64_t tar;
>> +    uint64_t dexcr;
>> +    uint64_t hdexcr;
>> +    uint64_t hashkeyr;
>> +    uint64_t hashpkeyr;
>> +    uint64_t ctrl;
>> +    uint64_t vscr;
>> +    uint64_t vrsave;
>> +    ppc_vsr_t vsr[64];
>>   };
> 
> Why? I can't see where it's used... This is API for the original HV
> hcalls which is possibly now broken because the code uses sizeof()
> when mapping it.

Yeh, I had realised after posting the patches to cleanup these 
leftovers. Please ignore these additions, shall be removed.

> 
> In general I'm not a fan of splitting patches by the type of code they
> add. Definitions for external APIs okay. But for things like internal
> structures I prefer added where they are introduced.
> 
Make sense, I shall revisit and move declarations wherever used first.

> It's actually harder to review a patch if related / dependent changes
> aren't in it, IMO. What should be split is unrelated or independent
> changes and logical steps. Same goes for hcalls too actually. Take a
> look at the series that introduced nested HV. 120f738a467 adds all the
> hcalls, all the structures, etc.
> 
> So I would also hink about squashing at least get/set capabilities
> hcalls together, and guest create/delete, and probably vcpu create/run.

Hmm, I think we can keep get/set capab, guest create/delete together as
you suggested. We may want to keep vcpu_run separate as it has
significant changes, to keep it easier for review?
Let me know if you think otherwise.

regards,
Harsh

> 
> Thanks,
> Nick