| Message ID | 1486704360-27361-6-git-send-email-sjitindarsingh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 10, 2017 at 04:25:55PM +1100, Suraj Jitindar Singh wrote: > ISA v3.00 adds the idea of a partition table which is used to store the > address translation details for all partitions on the system. The partition > table consists of double word entries indexed by partition id where the second > double word contains the location of the process table in guest memory. The > process table is registered by the guest via a h-call. > > We need somewhere to store the address of the process table so we add an entry > to the sPAPRMachineState struct called patb_entry to represent the second > doubleword of a single partition table entry corresponding to the current > guest. We need to store this value so we know if the guest is using radix or > hash translation and the location of the corresponding process table in guest > memory. Since we only have a single guest per qemu instance, we only need one > entry. > > Since the partition table is technically a hypervisor resource we require that > access to it is abstracted by the virtual hypervisor through the calls > [set/get]_patbe(). Currently the value of the entry is never set (and thus > defaults to 0 indicating hash), but it will be required to both implement > POWER9 kvm support and tcg radix support. > > We also add this field to be migrated as part of the sPAPRMachineState as we > will need it on the receiving side as the guest will never tell us this > information again and we need it to perform translation. > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > --- > hw/ppc/spapr.c | 19 +++++++++++++++++++ > include/hw/ppc/spapr.h | 1 + > target/ppc/cpu.h | 2 ++ > 3 files changed, 22 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index e465d7a..057adae 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1018,6 +1018,20 @@ static void emulate_spapr_hypercall(PPCVirtualHypervisor *vhyp, > } > } > > +static void spapr_set_patbe(PPCVirtualHypervisor *vhyp, uint64_t val) > +{ > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > + > + spapr->patb_entry = val; > +} > + > +static uint64_t spapr_get_patbe(PPCVirtualHypervisor *vhyp) > +{ > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); So, it'll amount to the same thing of course, but using SPAPR_MACHINE(vhyp) here is a little more logically correct. > + > + return spapr->patb_entry; > +} > + > #define HPTE(_table, _i) (void *)(((uint64_t *)(_table)) + ((_i) * 2)) > #define HPTE_VALID(_hpte) (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID) > #define HPTE_DIRTY(_hpte) (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY) > @@ -1141,6 +1155,8 @@ static void ppc_spapr_reset(void) > /* Check for unknown sysbus devices */ > foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL); > > + spapr->patb_entry = 0; I'm assuming that the patb_entry has some control bits making this distinguishable from a process table at GPA 0? > /* Allocate and/or reset the hash page table */ > spapr_reallocate_hpt(spapr, > spapr_hpt_shift_for_ramsize(machine->maxram_size), > @@ -1340,6 +1356,7 @@ static const VMStateDescription vmstate_spapr = { > VMSTATE_UINT64_TEST(rtc_offset, sPAPRMachineState, version_before_3), > > VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2), > + VMSTATE_UINT64(patb_entry, sPAPRMachineState), Ah.. yeah, you can't just add things to the VMStateDescription, because that'll break parsing of existing migration streams. You could bump the version, but that breaks backwards migration. So, the usual approach here is to add a new optional subsection - see vmstate_spapr_ov5_cas for an example. In this case you could have the .needed function return true only if pathb_entry != 0 - so it won't be transmitted for either POWER7/8 or for POWER9 in legacy mode, which seems to make sense. > VMSTATE_END_OF_LIST() > }, > .subsections = (const VMStateDescription*[]) { > @@ -2733,6 +2750,8 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > nc->nmi_monitor_handler = spapr_nmi; > smc->phb_placement = spapr_phb_placement; > vhc->hypercall = emulate_spapr_hypercall; > + vhc->set_patbe = spapr_set_patbe; > + vhc->get_patbe = spapr_get_patbe; > } > > static const TypeInfo spapr_machine_info = { > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index a2d8964..c6a929a 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -63,6 +63,7 @@ struct sPAPRMachineState { > > void *htab; > uint32_t htab_shift; > + uint64_t patb_entry; /* Process tbl registed in H_REGISTER_PROCESS_TABLE */ > hwaddr rma_size; > int vrma_adjust; > ssize_t rtas_size; > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 425e79d..a148729 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1218,6 +1218,8 @@ struct PPCVirtualHypervisor { > struct PPCVirtualHypervisorClass { > InterfaceClass parent; > void (*hypercall)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu); > + void (*set_patbe)(PPCVirtualHypervisor *vhyp, uint64_t val); So.. I don't actually see any situation that would require set_patbe. We need get_patbe for the CPU code to get the process table addr from the machine. But the hypercall to set it is already in the machine, so we don't need to go the other way. > + uint64_t (*get_patbe)(PPCVirtualHypervisor *vhyp); > }; > > #define TYPE_PPC_VIRTUAL_HYPERVISOR "ppc-virtual-hypervisor"
On Mon, 2017-02-13 at 13:17 +1100, David Gibson wrote: > On Fri, Feb 10, 2017 at 04:25:55PM +1100, Suraj Jitindar Singh wrote: > > > > ISA v3.00 adds the idea of a partition table which is used to store > > the > > address translation details for all partitions on the system. The > > partition > > table consists of double word entries indexed by partition id where > > the second > > double word contains the location of the process table in guest > > memory. The > > process table is registered by the guest via a h-call. > > > > We need somewhere to store the address of the process table so we > > add an entry > > to the sPAPRMachineState struct called patb_entry to represent the > > second > > doubleword of a single partition table entry corresponding to the > > current > > guest. We need to store this value so we know if the guest is using > > radix or > > hash translation and the location of the corresponding process > > table in guest > > memory. Since we only have a single guest per qemu instance, we > > only need one > > entry. > > > > Since the partition table is technically a hypervisor resource we > > require that > > access to it is abstracted by the virtual hypervisor through the > > calls > > [set/get]_patbe(). Currently the value of the entry is never set > > (and thus > > defaults to 0 indicating hash), but it will be required to both > > implement > > POWER9 kvm support and tcg radix support. > > > > We also add this field to be migrated as part of the > > sPAPRMachineState as we > > will need it on the receiving side as the guest will never tell us > > this > > information again and we need it to perform translation. > > > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > > --- > > hw/ppc/spapr.c | 19 +++++++++++++++++++ > > include/hw/ppc/spapr.h | 1 + > > target/ppc/cpu.h | 2 ++ > > 3 files changed, 22 insertions(+) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index e465d7a..057adae 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1018,6 +1018,20 @@ static void > > emulate_spapr_hypercall(PPCVirtualHypervisor *vhyp, > > } > > } > > > > +static void spapr_set_patbe(PPCVirtualHypervisor *vhyp, uint64_t > > val) > > +{ > > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > + > > + spapr->patb_entry = val; > > +} > > + > > +static uint64_t spapr_get_patbe(PPCVirtualHypervisor *vhyp) > > +{ > > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > So, it'll amount to the same thing of course, but using > SPAPR_MACHINE(vhyp) here is a little more logically correct. Ok, will fix that up > > > > > + > > + return spapr->patb_entry; > > +} > > + > > #define HPTE(_table, _i) (void *)(((uint64_t *)(_table)) + ((_i) > > * 2)) > > #define HPTE_VALID(_hpte) (tswap64(*((uint64_t *)(_hpte))) & > > HPTE64_V_VALID) > > #define HPTE_DIRTY(_hpte) (tswap64(*((uint64_t *)(_hpte))) & > > HPTE64_V_HPTE_DIRTY) > > @@ -1141,6 +1155,8 @@ static void ppc_spapr_reset(void) > > /* Check for unknown sysbus devices */ > > foreach_dynamic_sysbus_device(find_unknown_sysbus_device, > > NULL); > > > > + spapr->patb_entry = 0; > I'm assuming that the patb_entry has some control bits making this > distinguishable from a process table at GPA 0? Yeah, so process table size is also contained here which must be >= 24, so this is technically an invalid entry, which is what we want for a default value. > > > > > /* Allocate and/or reset the hash page table */ > > spapr_reallocate_hpt(spapr, > > spapr_hpt_shift_for_ramsize(machine- > > >maxram_size), > > @@ -1340,6 +1356,7 @@ static const VMStateDescription vmstate_spapr > > = { > > VMSTATE_UINT64_TEST(rtc_offset, sPAPRMachineState, > > version_before_3), > > > > VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2), > > + VMSTATE_UINT64(patb_entry, sPAPRMachineState), > Ah.. yeah, you can't just add things to the VMStateDescription, > because that'll break parsing of existing migration streams. You > could bump the version, but that breaks backwards migration. > > So, the usual approach here is to add a new optional subsection - see > vmstate_spapr_ov5_cas for an example. In this case you could have > the > .needed function return true only if pathb_entry != 0 - so it won't > be > transmitted for either POWER7/8 or for POWER9 in legacy mode, which > seems to make sense. Sounds like a good way of doing it which will behave how we want. I'll do it like you suggest. > > > > > VMSTATE_END_OF_LIST() > > }, > > .subsections = (const VMStateDescription*[]) { > > @@ -2733,6 +2750,8 @@ static void > > spapr_machine_class_init(ObjectClass *oc, void *data) > > nc->nmi_monitor_handler = spapr_nmi; > > smc->phb_placement = spapr_phb_placement; > > vhc->hypercall = emulate_spapr_hypercall; > > + vhc->set_patbe = spapr_set_patbe; > > + vhc->get_patbe = spapr_get_patbe; > > } > > > > static const TypeInfo spapr_machine_info = { > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index a2d8964..c6a929a 100644 > > --- a/include/hw/ppc/spapr.h > > +++ b/include/hw/ppc/spapr.h > > @@ -63,6 +63,7 @@ struct sPAPRMachineState { > > > > void *htab; > > uint32_t htab_shift; > > + uint64_t patb_entry; /* Process tbl registed in > > H_REGISTER_PROCESS_TABLE */ > > hwaddr rma_size; > > int vrma_adjust; > > ssize_t rtas_size; > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > > index 425e79d..a148729 100644 > > --- a/target/ppc/cpu.h > > +++ b/target/ppc/cpu.h > > @@ -1218,6 +1218,8 @@ struct PPCVirtualHypervisor { > > struct PPCVirtualHypervisorClass { > > InterfaceClass parent; > > void (*hypercall)(PPCVirtualHypervisor *vhyp, PowerPCCPU > > *cpu); > > + void (*set_patbe)(PPCVirtualHypervisor *vhyp, uint64_t val); > So.. I don't actually see any situation that would require set_patbe. > We need get_patbe for the CPU code to get the process table addr from > the machine. But the hypercall to set it is already in the machine, > so we don't need to go the other way. Good point, I wasn't thinking. I'll remove this. > > > > > + uint64_t (*get_patbe)(PPCVirtualHypervisor *vhyp); > > }; > > > > #define TYPE_PPC_VIRTUAL_HYPERVISOR "ppc-virtual-hypervisor"
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index e465d7a..057adae 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1018,6 +1018,20 @@ static void emulate_spapr_hypercall(PPCVirtualHypervisor *vhyp, } } +static void spapr_set_patbe(PPCVirtualHypervisor *vhyp, uint64_t val) +{ + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); + + spapr->patb_entry = val; +} + +static uint64_t spapr_get_patbe(PPCVirtualHypervisor *vhyp) +{ + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); + + return spapr->patb_entry; +} + #define HPTE(_table, _i) (void *)(((uint64_t *)(_table)) + ((_i) * 2)) #define HPTE_VALID(_hpte) (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID) #define HPTE_DIRTY(_hpte) (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY) @@ -1141,6 +1155,8 @@ static void ppc_spapr_reset(void) /* Check for unknown sysbus devices */ foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL); + spapr->patb_entry = 0; + /* Allocate and/or reset the hash page table */ spapr_reallocate_hpt(spapr, spapr_hpt_shift_for_ramsize(machine->maxram_size), @@ -1340,6 +1356,7 @@ static const VMStateDescription vmstate_spapr = { VMSTATE_UINT64_TEST(rtc_offset, sPAPRMachineState, version_before_3), VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2), + VMSTATE_UINT64(patb_entry, sPAPRMachineState), VMSTATE_END_OF_LIST() }, .subsections = (const VMStateDescription*[]) { @@ -2733,6 +2750,8 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) nc->nmi_monitor_handler = spapr_nmi; smc->phb_placement = spapr_phb_placement; vhc->hypercall = emulate_spapr_hypercall; + vhc->set_patbe = spapr_set_patbe; + vhc->get_patbe = spapr_get_patbe; } static const TypeInfo spapr_machine_info = { diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index a2d8964..c6a929a 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -63,6 +63,7 @@ struct sPAPRMachineState { void *htab; uint32_t htab_shift; + uint64_t patb_entry; /* Process tbl registed in H_REGISTER_PROCESS_TABLE */ hwaddr rma_size; int vrma_adjust; ssize_t rtas_size; diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 425e79d..a148729 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -1218,6 +1218,8 @@ struct PPCVirtualHypervisor { struct PPCVirtualHypervisorClass { InterfaceClass parent; void (*hypercall)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu); + void (*set_patbe)(PPCVirtualHypervisor *vhyp, uint64_t val); + uint64_t (*get_patbe)(PPCVirtualHypervisor *vhyp); }; #define TYPE_PPC_VIRTUAL_HYPERVISOR "ppc-virtual-hypervisor"
ISA v3.00 adds the idea of a partition table which is used to store the address translation details for all partitions on the system. The partition table consists of double word entries indexed by partition id where the second double word contains the location of the process table in guest memory. The process table is registered by the guest via a h-call. We need somewhere to store the address of the process table so we add an entry to the sPAPRMachineState struct called patb_entry to represent the second doubleword of a single partition table entry corresponding to the current guest. We need to store this value so we know if the guest is using radix or hash translation and the location of the corresponding process table in guest memory. Since we only have a single guest per qemu instance, we only need one entry. Since the partition table is technically a hypervisor resource we require that access to it is abstracted by the virtual hypervisor through the calls [set/get]_patbe(). Currently the value of the entry is never set (and thus defaults to 0 indicating hash), but it will be required to both implement POWER9 kvm support and tcg radix support. We also add this field to be migrated as part of the sPAPRMachineState as we will need it on the receiving side as the guest will never tell us this information again and we need it to perform translation. Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> --- hw/ppc/spapr.c | 19 +++++++++++++++++++ include/hw/ppc/spapr.h | 1 + target/ppc/cpu.h | 2 ++ 3 files changed, 22 insertions(+)