Re: [PATCH 4/4] ppc/spapr: Move spapr nested HV to a new file

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

On 6/15/23 17:21, Nicholas Piggin wrote:
> On Thu Jun 15, 2023 at 4:30 PM AEST, Harsh Prateek Bora wrote:
>>
>>
>> On 6/8/23 14:43, Nicholas Piggin wrote:
>>> Create spapr_nested.c for most of the nested HV implementation.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>    hw/ppc/meson.build     |   1 +
>>>    hw/ppc/spapr_hcall.c   | 415 +---------------------------------
>>>    hw/ppc/spapr_nested.c  | 496 +++++++++++++++++++++++++++++++++++++++++
>>>    include/hw/ppc/spapr.h |  61 +----
>>>    4 files changed, 499 insertions(+), 474 deletions(-)
>>>    create mode 100644 hw/ppc/spapr_nested.c
> 
> [snip]
> 
>>> diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
>>> new file mode 100644
>>> index 0000000000..c06dd8903c
>>> --- /dev/null
>>> +++ b/hw/ppc/spapr_nested.c
>>> @@ -0,0 +1,496 @@
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/cutils.h"
>>> +#include "exec/exec-all.h"
>>> +#include "helper_regs.h"
>>> +#include "hw/ppc/ppc.h"
>>> +#include "hw/ppc/spapr.h"
>>> +#include "hw/ppc/spapr_cpu_core.h"
>>> +
>>> +/*
>>> + * Register state for entering a nested guest with H_ENTER_NESTED.
>>> + * New member must be added at the end.
>>> + */
>>> +struct kvmppc_hv_guest_state {
>>> +    uint64_t version;      /* version of this structure layout, must be first */
>>> +    uint32_t lpid;
>>> +    uint32_t vcpu_token;
>>> +    /* These registers are hypervisor privileged (at least for writing) */
>>> +    uint64_t lpcr;
>>> +    uint64_t pcr;
>>> +    uint64_t amor;
>>> +    uint64_t dpdes;
>>> +    uint64_t hfscr;
>>> +    int64_t tb_offset;
>>> +    uint64_t dawr0;
>>> +    uint64_t dawrx0;
>>> +    uint64_t ciabr;
>>> +    uint64_t hdec_expiry;
>>> +    uint64_t purr;
>>> +    uint64_t spurr;
>>> +    uint64_t ic;
>>> +    uint64_t vtb;
>>> +    uint64_t hdar;
>>> +    uint64_t hdsisr;
>>> +    uint64_t heir;
>>> +    uint64_t asdr;
>>> +    /* These are OS privileged but need to be set late in guest entry */
>>> +    uint64_t srr0;
>>> +    uint64_t srr1;
>>> +    uint64_t sprg[4];
>>> +    uint64_t pidr;
>>> +    uint64_t cfar;
>>> +    uint64_t ppr;
>>> +    /* Version 1 ends here */
>>> +    uint64_t dawr1;
>>> +    uint64_t dawrx1;
>>> +    /* Version 2 ends here */
>>> +};
>>> +
>>> +/* Latest version of hv_guest_state structure */
>>> +#define HV_GUEST_STATE_VERSION  2
>>> +
>>> +/* Linux 64-bit powerpc pt_regs struct, used by nested HV */
>>> +struct kvmppc_pt_regs {
>>> +    uint64_t gpr[32];
>>> +    uint64_t nip;
>>> +    uint64_t msr;
>>> +    uint64_t orig_gpr3;    /* Used for restarting system calls */
>>> +    uint64_t ctr;
>>> +    uint64_t link;
>>> +    uint64_t xer;
>>> +    uint64_t ccr;
>>> +    uint64_t softe;        /* Soft enabled/disabled */
>>> +    uint64_t trap;         /* Reason for being here */
>>> +    uint64_t dar;          /* Fault registers */
>>> +    uint64_t dsisr;        /* on 4xx/Book-E used for ESR */
>>> +    uint64_t result;       /* Result of a system call */
>>> +};
>>
>> Now that we have a separated spapr_nested.c for nested related code,
>> Can above definitions and other nested related defines (like struct
>> nested_ppc_state below) be moved to a new spapr_nested.h as well?
>> Otherwise, looks good to me.
> 
> They're private to this file, so do we need to? I'm on the fence about
> it, maybe because they're hcall ABI. I don't object to adding a new
> spapr_nested.h for nested related things in general though.
> 

Yeh, I would recommend moving nested specific declarations to 
spapr_nested.h as would be more organized.

Thanks
Harsh

> Thanks,
> Nick