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 <mi...@neuling.org>
> Signed-off-by: Shivaprasad G Bhat <sb...@linux.ibm.com>
> Signed-off-by: Harsh Prateek Bora <hars...@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
