| Message ID | 20210512140813.112884-10-bruno.larsen@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: add support to disable-tcg | expand |
On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote: > From: "Lucas Mateus Castro (alqotel)"<lucas.araujo@eldorado.org.br> > > Added tlb_set_page and tlb_set_page_with_attrs to the > stubbed functions in exec-all.h as it is needed > in some functions when compiling without TCG > > Signed-off-by: Lucas Mateus Castro (alqotel)<lucas.araujo@eldorado.org.br> > --- > include/exec/exec-all.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) No, the caller is tcg-specific already. r~
On 12/05/2021 15:34, Richard Henderson wrote: > On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote: >> From: "Lucas Mateus Castro (alqotel)"<lucas.araujo@eldorado.org.br> >> >> Added tlb_set_page and tlb_set_page_with_attrs to the >> stubbed functions in exec-all.h as it is needed >> in some functions when compiling without TCG >> >> Signed-off-by: Lucas Mateus Castro >> (alqotel)<lucas.araujo@eldorado.org.br> >> --- >> include/exec/exec-all.h | 10 ++++++++++ >> 1 file changed, 10 insertions(+) > > No, the caller is tcg-specific already. > > > r~ tlb_set_page is called by many ppc_hash64_handle_mmu_fault, ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of which from what I've seen are only used inside #if defined(CONFIG_SOFTMMU). So what is the best way to deal with these tlb_set_page calls? Should these part of the _handle_mmu_fault functions never be reached or should these functions never be called? If it's the latter then should we change the #if defined to #if defined(CONFIG_SOFTMMU) && (CONFIG_TCG)? P.S: There was a miscommunication between me and Bruno, this should've been a RFC.
On 5/13/21 9:03 AM, Lucas Mateus Martins Araujo e Castro wrote: > tlb_set_page is called by many ppc_hash64_handle_mmu_fault, > ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of which from > what I've seen are only used inside #if defined(CONFIG_SOFTMMU). tlb_set_page should only be called from one place: ppc_cpu_tlb_fill. The other functions should fill in data, much like get_physical_address. > So what is the > best way to deal with these tlb_set_page calls? Should these part of the > _handle_mmu_fault functions never be reached or should these functions never be > called? There is some duplication between get_physical_address* and *handle_mmu_fault that should be fixed. What should be happening is that you have one function (per mmu type) that takes a virtual address and resolves a physical address. This bit of code should be written so that it is usable by both CPUClass.get_phys_page_attrs_debug and TCGCPUOps.tlb_fill. It appears as if ppc_radix64_xlate is the right interface for this. It appears that real mode handling is duplicated between hash64 and radix64, which could be unified. You should only call tlb_set_page from TCGCPUOps.tlb_fill, aka ppc_cpu_tlb_fill. TCGCPUOps.tlb_fill is obviously TCG only. The version you are looking at here is system emulation specific (sysemu, !defined(CONFIG_USER_ONLY)). There is a second version of this function, with the same signature, that is used for user emulation in the helpfully named user_only_helper.c. r~
On Thu, May 13, 2021 at 11:03:24AM -0300, Lucas Mateus Martins Araujo e Castro wrote: > > On 12/05/2021 15:34, Richard Henderson wrote: > > On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote: > > > From: "Lucas Mateus Castro (alqotel)"<lucas.araujo@eldorado.org.br> > > > > > > Added tlb_set_page and tlb_set_page_with_attrs to the > > > stubbed functions in exec-all.h as it is needed > > > in some functions when compiling without TCG > > > > > > Signed-off-by: Lucas Mateus Castro > > > (alqotel)<lucas.araujo@eldorado.org.br> > > > --- > > > include/exec/exec-all.h | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > No, the caller is tcg-specific already. > > > > > > r~ > > tlb_set_page is called by many ppc_hash64_handle_mmu_fault, > ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of which > from what I've seen are only used inside #if defined(CONFIG_SOFTMMU). So > what is the best way to deal with these tlb_set_page calls? Should these > part of the _handle_mmu_fault functions never be reached or should > these The handle_mmu_fault() functions per se shouldn't be included in a !SOFTMMU build. We might have to extract some of their internal logic for the gdb path, though. > functions never be called? > > If it's the latter then should we change the #if defined to #if > defined(CONFIG_SOFTMMU) && (CONFIG_TCG)? That definitely doesn't make sense. In practice CONFIG_SOFTMMU == CONFIG_TCG. > > > P.S: There was a miscommunication between me and Bruno, this should've been > a RFC. >
On Thu, May 13, 2021 at 06:44:01PM -0500, Richard Henderson wrote: 65;6401;1c> On 5/13/21 9:03 AM, Lucas Mateus Martins Araujo e Castro wrote: > > tlb_set_page is called by many ppc_hash64_handle_mmu_fault, > > ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of > > which from what I've seen are only used inside #if > > defined(CONFIG_SOFTMMU). > > tlb_set_page should only be called from one place: ppc_cpu_tlb_fill. The > other functions should fill in data, much like get_physical_address. > > > > So what is the best way to deal with these tlb_set_page calls? Should > > these part of the _handle_mmu_fault functions never be reached or should > > these functions never be called? > > There is some duplication between get_physical_address* and > *handle_mmu_fault that should be fixed. > > What should be happening is that you have one function (per mmu type) that > takes a virtual address and resolves a physical address. This bit of code > should be written so that it is usable by both > CPUClass.get_phys_page_attrs_debug and TCGCPUOps.tlb_fill. It appears as if > ppc_radix64_xlate is the right interface for this. > > It appears that real mode handling is duplicated between hash64 and radix64, > which could be unified. Any common handling between the hash and radix MMUs should go in mmu-book3s-v3.* That covers common things across the v3 (POWER9 and later) MMUs which includes both hash and radix mode. > You should only call tlb_set_page from TCGCPUOps.tlb_fill, aka > ppc_cpu_tlb_fill. TCGCPUOps.tlb_fill is obviously TCG only. > > The version you are looking at here is system emulation specific (sysemu, > !defined(CONFIG_USER_ONLY)). There is a second version of this function, > with the same signature, that is used for user emulation in the helpfully > named user_only_helper.c. > > > r~ >
On 17/05/2021 00:55, David Gibson wrote: > On Thu, May 13, 2021 at 11:03:24AM -0300, Lucas Mateus Martins Araujo e Castro wrote: >> On 12/05/2021 15:34, Richard Henderson wrote: >>> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote: >>>> From: "Lucas Mateus Castro (alqotel)"<lucas.araujo@eldorado.org.br> >>>> >>>> Added tlb_set_page and tlb_set_page_with_attrs to the >>>> stubbed functions in exec-all.h as it is needed >>>> in some functions when compiling without TCG >>>> >>>> Signed-off-by: Lucas Mateus Castro >>>> (alqotel)<lucas.araujo@eldorado.org.br> >>>> --- >>>> include/exec/exec-all.h | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>> No, the caller is tcg-specific already. >>> >>> >>> r~ >> tlb_set_page is called by many ppc_hash64_handle_mmu_fault, >> ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of which >> from what I've seen are only used inside #if defined(CONFIG_SOFTMMU). So >> what is the best way to deal with these tlb_set_page calls? Should these >> part of the _handle_mmu_fault functions never be reached or should >> these > The handle_mmu_fault() functions per se shouldn't be included in a > !SOFTMMU build. We might have to extract some of their internal logic > for the gdb path, though. > >> functions never be called? >> >> If it's the latter then should we change the #if defined to #if >> defined(CONFIG_SOFTMMU) && (CONFIG_TCG)? > That definitely doesn't make sense. In practice CONFIG_SOFTMMU == CONFIG_TCG. We figured it was the case, but from what I can tell, CONFIG_SOFTMMU is set when parsing the target list (in the configure script) and CONFIG_TCG is set later, when parsing which accelerators were requested. So even though SOFTMMU should imply TCG, the way it is coded right now doesn't. We could also try and change the configure script, but neither of us is really good with bash scripts, so this was the next best solution we came up with. > >> >> P.S: There was a miscommunication between me and Bruno, this should've been >> a RFC. >>
On 17/05/2021 00:58, David Gibson wrote: > On Thu, May 13, 2021 at 06:44:01PM -0500, Richard Henderson wrote: > 65;6401;1c> On 5/13/21 9:03 AM, Lucas Mateus Martins Araujo e Castro wrote: >>> tlb_set_page is called by many ppc_hash64_handle_mmu_fault, >>> ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of >>> which from what I've seen are only used inside #if >>> defined(CONFIG_SOFTMMU). >> tlb_set_page should only be called from one place: ppc_cpu_tlb_fill. The >> other functions should fill in data, much like get_physical_address. >> >> >>> So what is the best way to deal with these tlb_set_page calls? Should >>> these part of the _handle_mmu_fault functions never be reached or should >>> these functions never be called? >> There is some duplication between get_physical_address* and >> *handle_mmu_fault that should be fixed. >> >> What should be happening is that you have one function (per mmu type) that >> takes a virtual address and resolves a physical address. This bit of code >> should be written so that it is usable by both >> CPUClass.get_phys_page_attrs_debug and TCGCPUOps.tlb_fill. It appears as if >> ppc_radix64_xlate is the right interface for this. >> >> It appears that real mode handling is duplicated between hash64 and radix64, >> which could be unified. > Any common handling between the hash and radix MMUs should go in > mmu-book3s-v3.* That covers common things across the v3 (POWER9 and > later) MMUs which includes both hash and radix mode. I'm not completely sure how this should be handled, there's a get_physical_address in mmu_helper.c but it's a static function and divided by processor families instead of MMU types, so get_physical_address_* should be a new function? The new get_physical_address_* function would be a mmu-hash(32|64) that do something like ppc_radix64_xlate and add a function to mmu-book3s-v3 that call either the radix64 or the hash64 function and also handle real mode access. Also should the tlb_set_page calls in *_handle_mmu_fault be changed to ppc_cpu_tlb_fill or the function should themselves fill it? > >> You should only call tlb_set_page from TCGCPUOps.tlb_fill, aka >> ppc_cpu_tlb_fill. TCGCPUOps.tlb_fill is obviously TCG only. >> >> The version you are looking at here is system emulation specific (sysemu, >> !defined(CONFIG_USER_ONLY)). There is a second version of this function, >> with the same signature, that is used for user emulation in the helpfully >> named user_only_helper.c. >> >> >> r~ >>
On 5/17/21 11:59 AM, Lucas Mateus Martins Araujo e Castro wrote: > I'm not completely sure how this should be handled, there's a > get_physical_address in mmu_helper.c but it's a static function and divided by > processor families instead of MMU types, so get_physical_address_* should be a > new function? > > The new get_physical_address_* function would be a mmu-hash(32|64) that do > something like ppc_radix64_xlate and add a function to mmu-book3s-v3 that call > either the radix64 or the hash64 function and also handle real mode access. The entry points that we are concerned about are: ppc_cpu_get_phys_page_debug ppc_cpu_tlb_fill Currently there is a hook, pcc->handle_mmu_fault, which is used by ppc_cpu_tlb_fill, but is insufficiently general. We're going to remove that hook. We're going to add a new hook with the same interface as ppc_radix64_xlate that will be used by both ppc_cpu_get_phys_page_debug and ppc_cpu_tlb_fill. > Also should the tlb_set_page calls in *_handle_mmu_fault be changed to > ppc_cpu_tlb_fill or the function should themselves fill it? Only ppc_cpu_tlb_fill should call tlb_set_page. r~
On 17/05/2021 15:02, Richard Henderson wrote: > On 5/17/21 11:59 AM, Lucas Mateus Martins Araujo e Castro wrote: >> I'm not completely sure how this should be handled, there's a >> get_physical_address in mmu_helper.c but it's a static function and >> divided by processor families instead of MMU types, so >> get_physical_address_* should be a new function? >> >> The new get_physical_address_* function would be a mmu-hash(32|64) >> that do something like ppc_radix64_xlate and add a function to >> mmu-book3s-v3 that call either the radix64 or the hash64 function and >> also handle real mode access. > > The entry points that we are concerned about are: > ppc_cpu_get_phys_page_debug > ppc_cpu_tlb_fill > > Currently there is a hook, pcc->handle_mmu_fault, which is used by > ppc_cpu_tlb_fill, but is insufficiently general. We're going to > remove that hook. > > We're going to add a new hook with the same interface as > ppc_radix64_xlate that will be used by both > ppc_cpu_get_phys_page_debug and ppc_cpu_tlb_fill. > So, just to make sure we understand, what we want to do is: * take all the common code from *_handle_mmu_fault and put it in ppc_cpu_tlb_fill * take whatever is not common and hide it in an equivalent of ppc_radix64_xlate * make the 2 entry points only use these new functions, so we can compile ppc_cpu_get_phys_page_debug * move get_physical_address and all functions called by it somewhere that will compile when we disable tcg (again, to compile get_phys_page_debug) Is that it? Sorry if this is very obvious, we never dealt with hardware and mmu stuff before...
On Mon, May 17, 2021 at 08:07:24AM -0300, Bruno Piazera Larsen wrote: > > On 17/05/2021 00:55, David Gibson wrote: > > On Thu, May 13, 2021 at 11:03:24AM -0300, Lucas Mateus Martins Araujo e Castro wrote: > > > On 12/05/2021 15:34, Richard Henderson wrote: > > > > On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote: > > > > > From: "Lucas Mateus Castro (alqotel)"<lucas.araujo@eldorado.org.br> > > > > > > > > > > Added tlb_set_page and tlb_set_page_with_attrs to the > > > > > stubbed functions in exec-all.h as it is needed > > > > > in some functions when compiling without TCG > > > > > > > > > > Signed-off-by: Lucas Mateus Castro > > > > > (alqotel)<lucas.araujo@eldorado.org.br> > > > > > --- > > > > > include/exec/exec-all.h | 10 ++++++++++ > > > > > 1 file changed, 10 insertions(+) > > > > No, the caller is tcg-specific already. > > > > > > > > > > > > r~ > > > tlb_set_page is called by many ppc_hash64_handle_mmu_fault, > > > ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of which > > > from what I've seen are only used inside #if defined(CONFIG_SOFTMMU). So > > > what is the best way to deal with these tlb_set_page calls? Should these > > > part of the _handle_mmu_fault functions never be reached or should > > > these > > The handle_mmu_fault() functions per se shouldn't be included in a > > !SOFTMMU build. We might have to extract some of their internal logic > > for the gdb path, though. > > > > > functions never be called? > > > > > > If it's the latter then should we change the #if defined to #if > > > defined(CONFIG_SOFTMMU) && (CONFIG_TCG)? > > That definitely doesn't make sense. In practice CONFIG_SOFTMMU == CONFIG_TCG. Ugh.. sorry.. I was confused again. In practice CONFIG_SOFTMMU == !CONFIG_USER_ONLY. > We figured it was the case, but from what I can tell, CONFIG_SOFTMMU is set > when parsing the target list (in the configure script) and CONFIG_TCG is set > later, when parsing which accelerators were requested. So even though > SOFTMMU should imply TCG, the way it is coded right now doesn't. We could > also try and change the configure script, but neither of us is really good > with bash scripts, so this was the next best solution we came up with. > > > > > > > > P.S: There was a miscommunication between me and Bruno, this should've been > > > a RFC. > > >
On Mon, May 17, 2021 at 01:59:35PM -0300, Lucas Mateus Martins Araujo e Castro wrote: > > On 17/05/2021 00:58, David Gibson wrote: > > On Thu, May 13, 2021 at 06:44:01PM -0500, Richard Henderson wrote: > > 65;6401;1c> On 5/13/21 9:03 AM, Lucas Mateus Martins Araujo e Castro wrote: > > > > tlb_set_page is called by many ppc_hash64_handle_mmu_fault, > > > > ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of > > > > which from what I've seen are only used inside #if > > > > defined(CONFIG_SOFTMMU). > > > tlb_set_page should only be called from one place: ppc_cpu_tlb_fill. The > > > other functions should fill in data, much like get_physical_address. > > > > > > > > > > So what is the best way to deal with these tlb_set_page calls? Should > > > > these part of the _handle_mmu_fault functions never be reached or should > > > > these functions never be called? > > > There is some duplication between get_physical_address* and > > > *handle_mmu_fault that should be fixed. > > > > > > What should be happening is that you have one function (per mmu type) that > > > takes a virtual address and resolves a physical address. This bit of code > > > should be written so that it is usable by both > > > CPUClass.get_phys_page_attrs_debug and TCGCPUOps.tlb_fill. It appears as if > > > ppc_radix64_xlate is the right interface for this. > > > > > > It appears that real mode handling is duplicated between hash64 and radix64, > > > which could be unified. > > Any common handling between the hash and radix MMUs should go in > > mmu-book3s-v3.* That covers common things across the v3 (POWER9 and > > later) MMUs which includes both hash and radix mode. > > I'm not completely sure how this should be handled, there's a > get_physical_address in mmu_helper.c but it's a static function and divided > by processor families instead of MMU types, so get_physical_address_* should > be a new function? Sorry, I wasn't clear. mmu-v3 is only for things specifically common to the hash64 model (as implemented in mmu-hash64.c) and the radix model (as implemented in mmu-radix64.c). Which basically means things related to the POWER9 MMU which can switch between those modes. Things common to *more* MMU models (hash32, 4xx, 8xx, BookE, etc.) which includes most of what's in mmu-helper.c go elsewhere. > The new get_physical_address_* function would be a mmu-hash(32|64) that do > something like ppc_radix64_xlate and add a function to mmu-book3s-v3 that > call either the radix64 or the hash64 function and also handle real mode > access. > > Also should the tlb_set_page calls in *_handle_mmu_fault be changed to > ppc_cpu_tlb_fill or the function should themselves fill it? > > > > > > You should only call tlb_set_page from TCGCPUOps.tlb_fill, aka > > > ppc_cpu_tlb_fill. TCGCPUOps.tlb_fill is obviously TCG only. > > > > > > The version you are looking at here is system emulation specific (sysemu, > > > !defined(CONFIG_USER_ONLY)). There is a second version of this function, > > > with the same signature, that is used for user emulation in the helpfully > > > named user_only_helper.c. > > > > > > > > > r~ > > >
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 6b036cae8f..f287c88282 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -365,6 +365,16 @@ tlb_flush_page_bits_by_mmuidx_all_cpus_synced(CPUState *cpu, target_ulong addr, uint16_t idxmap, unsigned bits) { } +static inline void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, + hwaddr paddr, MemTxAttrs attrs, + int prot, int mmu_idx, target_ulong size) +{ +} +static inline void tlb_set_page(CPUState *cpu, target_ulong vaddr, + hwaddr paddr, int prot, + int mmu_idx, target_ulong size) +{ +} #endif /** * probe_access: