| Message ID | e8488ee4-396c-02b0-5e28-39513c5f2b37@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-06-30 at 18:01 +0200, Cédric Le Goater wrote: > +static uint32_t ppc_hash64_pte_size_decode(PowerPCCPU *cpu, uint64_t > pte0, > + uint64_t pte1, uint32_t > slb_pshift) > { > - switch (slb_pshift) { > - case 12: > - return 12; > - case 16: > - if ((pte1 & 0xf000) == 0x1000) { > - return 16; > - } > - return 0; > - case 24: > - if ((pte1 & 0xff000) == 0) { > - return 24; > - } > - return 0; > - } > - return 0; > + unsigned spshift; > + > + return ppc_hash64_hpte_page_shift_noslb(cpu, pte0, pte1, > &spshift); > } Why not call ppc_hash64_hpte_page_shift_noslb() directly from the call site ? That or rename it to ppc_hash64_pte_size_decode :-) Otherwise yes, your patch looks correct as in what doesppc_hash64_hpte_page_shift_noslb() is definitely more correct than what ppc_hash64_pte_size_decode() is doing. Cheers, Ben.
On Fri, Jul 01, 2016 at 08:13:47AM +1000, Benjamin Herrenschmidt wrote: > On Thu, 2016-06-30 at 18:01 +0200, Cédric Le Goater wrote: > > +static uint32_t ppc_hash64_pte_size_decode(PowerPCCPU *cpu, uint64_t > > pte0, > > + uint64_t pte1, uint32_t > > slb_pshift) > > { > > - switch (slb_pshift) { > > - case 12: > > - return 12; > > - case 16: > > - if ((pte1 & 0xf000) == 0x1000) { > > - return 16; > > - } > > - return 0; > > - case 24: > > - if ((pte1 & 0xff000) == 0) { > > - return 24; > > - } > > - return 0; > > - } > > - return 0; > > + unsigned spshift; > > + > > + return ppc_hash64_hpte_page_shift_noslb(cpu, pte0, pte1, > > &spshift); > > } > > Why not call ppc_hash64_hpte_page_shift_noslb() directly from the call > site ? That or rename it to ppc_hash64_pte_size_decode :-) Right.. that is the usage that ppc_hash64_hpte_page_shift_noslb() was intended for. > Otherwise yes, your patch looks correct as in what > doesppc_hash64_hpte_page_shift_noslb() is definitely more correct than > what ppc_hash64_pte_size_decode() is doing. >
On 07/01/2016 12:13 AM, Benjamin Herrenschmidt wrote: > On Thu, 2016-06-30 at 18:01 +0200, Cédric Le Goater wrote: >> +static uint32_t ppc_hash64_pte_size_decode(PowerPCCPU *cpu, uint64_t >> pte0, >> + uint64_t pte1, uint32_t >> slb_pshift) >> { >> - switch (slb_pshift) { >> - case 12: >> - return 12; >> - case 16: >> - if ((pte1 & 0xf000) == 0x1000) { >> - return 16; >> - } >> - return 0; >> - case 24: >> - if ((pte1 & 0xff000) == 0) { >> - return 24; >> - } >> - return 0; >> - } >> - return 0; >> + unsigned spshift; >> + >> + return ppc_hash64_hpte_page_shift_noslb(cpu, pte0, pte1, >> &spshift); >> } > > Why not call ppc_hash64_hpte_page_shift_noslb() directly from the call > site ? That or rename it to ppc_hash64_pte_size_decode :-) yes, clearly :) but that segment page shift is bothering me. David, Do you think I can remove that parameter as it is never used or do you have some plans for it ? > Otherwise yes, your patch looks correct as in what > doesppc_hash64_hpte_page_shift_noslb() is definitely more correct than > what ppc_hash64_pte_size_decode() is doing. Thanks, C.
Index: qemu-dgibson-for-2.7.git/target-ppc/mmu-hash64.c =================================================================== --- qemu-dgibson-for-2.7.git.orig/target-ppc/mmu-hash64.c +++ qemu-dgibson-for-2.7.git/target-ppc/mmu-hash64.c @@ -453,23 +453,12 @@ void ppc_hash64_stop_access(PowerPCCPU * /* Returns the effective page shift or 0. MPSS isn't supported yet so * this will always be the slb_pshift or 0 */ -static uint32_t ppc_hash64_pte_size_decode(uint64_t pte1, uint32_t slb_pshift) +static uint32_t ppc_hash64_pte_size_decode(PowerPCCPU *cpu, uint64_t pte0, + uint64_t pte1, uint32_t slb_pshift) { - switch (slb_pshift) { - case 12: - return 12; - case 16: - if ((pte1 & 0xf000) == 0x1000) { - return 16; - } - return 0; - case 24: - if ((pte1 & 0xff000) == 0) { - return 24; - } - return 0; - } - return 0; + unsigned spshift; + + return ppc_hash64_hpte_page_shift_noslb(cpu, pte0, pte1, &spshift); } static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash, @@ -494,7 +483,8 @@ static hwaddr ppc_hash64_pteg_search(Pow if ((pte0 & HPTE64_V_VALID) && (secondary == !!(pte0 & HPTE64_V_SECONDARY)) && HPTE64_V_COMPARE(pte0, ptem)) { - uint32_t pshift = ppc_hash64_pte_size_decode(pte1, slb_pshift); + uint32_t pshift = ppc_hash64_pte_size_decode(cpu, pte0, pte1, + slb_pshift); if (pshift == 0) { continue; }