| Message ID | 20210518201146.794854-1-richard.henderson@linaro.org |
|---|---|
| Headers | show |
| Series | target/ppc: Clean up mmu translation | expand |
On Tue, May 18, 2021 at 03:11:22PM -0500, Richard Henderson wrote: > This attempts the cleanup I've been talking about with Bruno. > > On the way, there's a lot of MMUAccessType cleanup, to get the > code into the form I wanted the interface to share. There's a > lot more cleanup that could be done, particularly wrt the older > mmu models. I've applied 1..15, still looking at the rest. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 5/18/21 9:52 PM, David Gibson wrote: > On Tue, May 18, 2021 at 03:11:22PM -0500, Richard Henderson wrote: >> This attempts the cleanup I've been talking about with Bruno. >> >> On the way, there's a lot of MMUAccessType cleanup, to get the >> code into the form I wanted the interface to share. There's a >> lot more cleanup that could be done, particularly wrt the older >> mmu models. > > I've applied 1..15, still looking at the rest. Please dequeue. I want to create a new mmu-internal.h, which affects all the patches from #1. r~
On 5/19/21 3:37 PM, Richard Henderson wrote: > On 5/18/21 9:52 PM, David Gibson wrote: >> I've applied 1..15, still looking at the rest. > > Please dequeue. I want to create a new mmu-internal.h, which affects all the > patches from #1. Alternately, don't. I can move the function later, and it may be a while before I can get back to this. Two outstanding bugs: (1) mmu-radix64.c vs hypervisors. You'll not see these unless you run kvm inside of tcg. Basically, all usage of msr_{hv,pr,ir,dr} within ppc_*_xlate is incorrect. We should be pulling these from the 3 bits of mmu_idx, as outlined in the table in hreg_compute_hflags_value. When you start propagating that around, you see that the second-level translation for loading the pte (2 of the 3 calls to ppc_radix64_partition_scoped_xlate) should not be using the mmu_idx related to the user-mode guest access, but should be using the mmu_idx of the kernel/hypervisor that owns the page table. I can't see that mmu-radix64.c has the same problem. I'm not really sure how the second-level translation for hypervisors works there. Is it by the hypervisor altering the hash table as it is loaded? (2) The direct-segment accesses for 6xx and hash32 use ACCESS_* to conditionally reject an mmu access. This is flawed, because we only arrive into these translation routines on the softtlb slow path. If we have an ACCESS_INT and then an ACCESS_FLOAT, the first will load a tlb entry which the second will use to stay on the fast path. There are several possible solutions: (A) Give tlb_set_page size == 1 for direct-segment addresses. This will cause cputlb.c to force all references to the page back through the slow path and through tlb_fill. At which point env->access_type is correct for each access, and we can reject on type. This could be really slow. But since these direct-segment accesses are also uncached, I would expect the guest OS to only be using them for i/o access. Which is already slow, so perhaps the extra trip through tlb_fill isn't noticeable. (B) Use additional mmu_idx. Not ideal, since we wouldn't be sharing softtlb entries for ACCESS_INT and ACCESS_FLOAT and ACCESS_RES for the normal case. But the relevant mmu_models do not have hypervisor support, so we could still fit them in to 8 mmu_idx. We'd probably want to use special code for ACCESS_CACHE and ACCESS_EXT here. (C) Ignore this special case entirely, dropping everything related to env->access_type. The section of code that uses them is all for really old machine types with comments like /* Direct-store segment : absolutely *BUGGY* for now */ which is not encouraging. And short of writing our own test cases we're not likely to have any code to exercise this. r~
On Wed, May 19, 2021 at 05:47:05PM -0500, Richard Henderson wrote: > On 5/19/21 3:37 PM, Richard Henderson wrote: > > On 5/18/21 9:52 PM, David Gibson wrote: > > > I've applied 1..15, still looking at the rest. > > > > Please dequeue. I want to create a new mmu-internal.h, which affects > > all the patches from #1. > > Alternately, don't. I can move the function later, and it may be a while > before I can get back to this. Ok, I'll leave them in, since they're good cleanups even without the rest of the series. > > Two outstanding bugs: > > (1) mmu-radix64.c vs hypervisors. You'll not see these unless you run kvm > inside of tcg. > > Basically, all usage of msr_{hv,pr,ir,dr} within ppc_*_xlate is incorrect. > We should be pulling these from the 3 bits of mmu_idx, as outlined in the > table in hreg_compute_hflags_value. Ah, that's probably my fault. I reworked those substantially from the original code (closer to mmu_helper.c). I guess I didn't (and I suspect I still don't) really understand how the softmmu works. > When you start propagating that around, you see that the second-level > translation for loading the pte (2 of the 3 calls to > ppc_radix64_partition_scoped_xlate) should not be using the mmu_idx related > to the user-mode guest access, but should be using the mmu_idx of the > kernel/hypervisor that owns the page table. > > I can't see that mmu-radix64.c has the same problem. I'm not really sure > how the second-level translation for hypervisors works there. Is it by the > hypervisor altering the hash table as it is loaded? Uh.. you started by saying mmu-radix64.c then talked about the hash table, so I'm not sure which model you're talking about. For hash + hypervisor, then yes, there is no hardware 2-level translation, it all works by paravirtualizing updates to the hash table (this is the H_ENTER etc. code in hw/ppc/spapr_softmmu.c). > (2) The direct-segment accesses for 6xx and hash32 use ACCESS_* to > conditionally reject an mmu access. This is flawed, because we only arrive > into these translation routines on the softtlb slow path. If we have an > ACCESS_INT and then an ACCESS_FLOAT, the first will load a tlb entry which > the second will use to stay on the fast path. > > There are several possible solutions: > > (A) Give tlb_set_page size == 1 for direct-segment addresses. > This will cause cputlb.c to force all references to the page > back through the slow path and through tlb_fill. At which > point env->access_type is correct for each access, and we > can reject on type. > > This could be really slow. But since these direct-segment > accesses are also uncached, I would expect the guest OS to > only be using them for i/o access. Which is already slow, > so perhaps the extra trip through tlb_fill isn't noticeable. > > (B) Use additional mmu_idx. Not ideal, since we wouldn't be > sharing softtlb entries for ACCESS_INT and ACCESS_FLOAT > and ACCESS_RES for the normal case. But the relevant > mmu_models do not have hypervisor support, so we could > still fit them in to 8 mmu_idx. We'd probably want to > use special code for ACCESS_CACHE and ACCESS_EXT here. > > (C) Ignore this special case entirely, dropping everything > related to env->access_type. The section of code that > uses them is all for really old machine types with > comments like > > /* Direct-store segment : absolutely *BUGGY* for now */ > > which is not encouraging. And short of writing our own > test cases we're not likely to have any code to exercise > this. Indeed. Direct store segments are basically ancient history of the POWER architecture which Linux never used, and I don't think much else did either. So I'm inclined to go with (D) Just rip out all the direct store segment code and replace with some LOG_UNIMPs. Re-adding it working can be the job of the probably non-existent person who has an actual use case using them. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 5/23/21 10:26 PM, David Gibson wrote: > On Wed, May 19, 2021 at 05:47:05PM -0500, Richard Henderson wrote: >> On 5/19/21 3:37 PM, Richard Henderson wrote: >>> On 5/18/21 9:52 PM, David Gibson wrote: >>>> I've applied 1..15, still looking at the rest. >>> >>> Please dequeue. I want to create a new mmu-internal.h, which affects >>> all the patches from #1. >> >> Alternately, don't. I can move the function later, and it may be a while >> before I can get back to this. > > Ok, I'll leave them in, since they're good cleanups even without the > rest of the series. > >> >> Two outstanding bugs: >> >> (1) mmu-radix64.c vs hypervisors. You'll not see these unless you run kvm >> inside of tcg. >> >> Basically, all usage of msr_{hv,pr,ir,dr} within ppc_*_xlate is incorrect. >> We should be pulling these from the 3 bits of mmu_idx, as outlined in the >> table in hreg_compute_hflags_value. > > Ah, that's probably my fault. I reworked those substantially from the > original code (closer to mmu_helper.c). I guess I didn't (and I > suspect I still don't) really understand how the softmmu works. > >> When you start propagating that around, you see that the second-level >> translation for loading the pte (2 of the 3 calls to >> ppc_radix64_partition_scoped_xlate) should not be using the mmu_idx related >> to the user-mode guest access, but should be using the mmu_idx of the >> kernel/hypervisor that owns the page table. >> >> I can't see that mmu-radix64.c has the same problem. I'm not really sure >> how the second-level translation for hypervisors works there. Is it by the >> hypervisor altering the hash table as it is loaded? > > Uh.. you started by saying mmu-radix64.c then talked about the hash > table, so I'm not sure which model you're talking about. radix64 definitely has a problem. Then I wondered if hash64 had the same problem. > > For hash + hypervisor, then yes, there is no hardware 2-level > translation, it all works by paravirtualizing updates to the hash > table (this is the H_ENTER etc. code in hw/ppc/spapr_softmmu.c). Ah, gotcha. So, no, hash64 is unaffected. > Indeed. Direct store segments are basically ancient history of the > POWER architecture which Linux never used, and I don't think much else > did either. So I'm inclined to go with > > (D) Just rip out all the direct store segment code and replace with > some LOG_UNIMPs. Re-adding it working can be the job of the > probably non-existent person who has an actual use case using > them. Fair enough. r~
On 24/05/2021 00:26, David Gibson wrote: > On Wed, May 19, 2021 at 05:47:05PM -0500, Richard Henderson wrote: >> On 5/19/21 3:37 PM, Richard Henderson wrote: >>> On 5/18/21 9:52 PM, David Gibson wrote: >>>> I've applied 1..15, still looking at the rest. >>> Please dequeue. I want to create a new mmu-internal.h, which affects >>> all the patches from #1. >> Alternately, don't. I can move the function later, and it may be a while >> before I can get back to this. > Ok, I'll leave them in, since they're good cleanups even without the > rest of the series. Just as a heads up, for the disable-tcg to work, we need the rest of the patches, so we sort of need the rest as soon as possible. From a quick conversation with farosas (cc'ed here), I think these might be old bugs (or at least one of them were, and I'm missing something), which either me or lucas (also cc'ed) were planning on tackling next. If my understanding is correct, I don't think we lose anything by applying those and we finally support the disable flag. -- Bruno Piazera Larsen Instituto de Pesquisas ELDORADO <https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station> Departamento Computação Embarcada Analista de Software Trainee Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html> <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> </head> <body> <p><br> </p> <div class="moz-cite-prefix">On 24/05/2021 00:26, David Gibson wrote:<br> </div> <blockquote type="cite" cite="mid:YKsczpMuwDn006S4@yekko"> <pre class="moz-quote-pre" wrap="">On Wed, May 19, 2021 at 05:47:05PM -0500, Richard Henderson wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">On 5/19/21 3:37 PM, Richard Henderson wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">On 5/18/21 9:52 PM, David Gibson wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">I've applied 1..15, still looking at the rest. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Please dequeue. I want to create a new mmu-internal.h, which affects all the patches from #1. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Alternately, don't. I can move the function later, and it may be a while before I can get back to this. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Ok, I'll leave them in, since they're good cleanups even without the rest of the series.</pre> </blockquote> <p>Just as a heads up, for the disable-tcg to work, we need the rest of the patches, so we sort of need the rest as soon as possible.</p> <p>From a quick conversation with farosas (cc'ed here), I think these might be old bugs (or at least one of them were, and I'm missing something), which either me or lucas (also cc'ed) were planning on tackling next. <br> </p> <p>If my understanding is correct, I don't think we lose anything by applying those and we finally support the disable flag.<br> </p> <div class="moz-signature">-- <br> Bruno Piazera Larsen<br> <a href="https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station">Instituto de Pesquisas ELDORADO</a><br> Departamento Computação Embarcada<br> Analista de Software Trainee<br> <a href="https://www.eldorado.org.br/disclaimer.html">Aviso Legal - Disclaimer</a></div> </body> </html>