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