| Message ID | 20230906043333.448244-3-harshpb@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Nested PAPR API (KVM on PowerVM) | expand |
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. 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. 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. Thanks, Nick
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
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]; }; /* Latest version of hv_guest_state structure */