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

[Nicholas Piggin]

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