qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Harsh Prateek Bora
Subject: Re: [PATCH RESEND 02/15] ppc: spapr: Add new/extend structs to support Nested PAPR API
Date: Mon, 11 Sep 2023 12:17:35 +0530
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.14.0



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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]