| Message ID | 20190814072355.15333-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/mmu: Storage key reference and change bit handling | expand |
David Hildenbrand <david@redhat.com> writes: > Whenever we modify a storage key, we shuld flush the TLBs of all CPUs, > so the MMU fault handling code can properly consider the changed storage > key (to e.g., properly set the reference and change bit on the next > accesses). > > These functions are barely used in modern Linux guests, so the performance > implications are neglectable for now. > > This is a preparation for better reference and change bit handling for > TCG, which will require more MMU changes. > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/mem_helper.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > index 29d9eaa5b7..ed54265e03 100644 > --- a/target/s390x/mem_helper.c > +++ b/target/s390x/mem_helper.c > @@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2) > > key = (uint8_t) r1; > skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); > + /* TODO: Flush only entries with this target address */ > + tlb_flush_all_cpus_synced(env_cpu(env)); Doesn't: tlb_flush_page_all_cpus_synced(env_cpu(env), addr & TARGET_PAGE_MASK); do what you want here? > } > > /* reset reference bit extended */ > @@ -1843,6 +1845,8 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2) > if (skeyclass->set_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) { > return 0; > } > + /* TODO: Flush only entries with this target address */ > + tlb_flush_all_cpus_synced(env_cpu(env)); > > /* > * cc -- Alex Bennée
On 14.08.19 12:06, Alex Bennée wrote: > > David Hildenbrand <david@redhat.com> writes: > >> Whenever we modify a storage key, we shuld flush the TLBs of all CPUs, >> so the MMU fault handling code can properly consider the changed storage >> key (to e.g., properly set the reference and change bit on the next >> accesses). >> >> These functions are barely used in modern Linux guests, so the performance >> implications are neglectable for now. >> >> This is a preparation for better reference and change bit handling for >> TCG, which will require more MMU changes. >> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> target/s390x/mem_helper.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c >> index 29d9eaa5b7..ed54265e03 100644 >> --- a/target/s390x/mem_helper.c >> +++ b/target/s390x/mem_helper.c >> @@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2) >> >> key = (uint8_t) r1; >> skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); >> + /* TODO: Flush only entries with this target address */ >> + tlb_flush_all_cpus_synced(env_cpu(env)); > > Doesn't: > > tlb_flush_page_all_cpus_synced(env_cpu(env), addr & TARGET_PAGE_MASK); > > do what you want here? I would have to flush all TLB entries that target this physical page, not the entry of the single virtual page. So that does, unfortunately, not work.
David Hildenbrand <david@redhat.com> writes: > On 14.08.19 12:06, Alex Bennée wrote: >> >> David Hildenbrand <david@redhat.com> writes: >> >>> Whenever we modify a storage key, we shuld flush the TLBs of all CPUs, >>> so the MMU fault handling code can properly consider the changed storage >>> key (to e.g., properly set the reference and change bit on the next >>> accesses). >>> >>> These functions are barely used in modern Linux guests, so the performance >>> implications are neglectable for now. >>> >>> This is a preparation for better reference and change bit handling for >>> TCG, which will require more MMU changes. >>> >>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> target/s390x/mem_helper.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c >>> index 29d9eaa5b7..ed54265e03 100644 >>> --- a/target/s390x/mem_helper.c >>> +++ b/target/s390x/mem_helper.c >>> @@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2) >>> >>> key = (uint8_t) r1; >>> skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); >>> + /* TODO: Flush only entries with this target address */ >>> + tlb_flush_all_cpus_synced(env_cpu(env)); >> >> Doesn't: >> >> tlb_flush_page_all_cpus_synced(env_cpu(env), addr & TARGET_PAGE_MASK); >> >> do what you want here? > > I would have to flush all TLB entries that target this physical page, > not the entry of the single virtual page. So that does, unfortunately, > not work. Ahh I see. Well maybe that should be the comment instead: /* * As we can only flush by virtual address and not all the entries * that point to a physical address we have to flush the whole TLB * here. */ tlb_flush_all_cpus_synced(env_cpu(env)); ? -- Alex Bennée
On 14.08.19 12:44, Alex Bennée wrote: > > David Hildenbrand <david@redhat.com> writes: > >> On 14.08.19 12:06, Alex Bennée wrote: >>> >>> David Hildenbrand <david@redhat.com> writes: >>> >>>> Whenever we modify a storage key, we shuld flush the TLBs of all CPUs, >>>> so the MMU fault handling code can properly consider the changed storage >>>> key (to e.g., properly set the reference and change bit on the next >>>> accesses). >>>> >>>> These functions are barely used in modern Linux guests, so the performance >>>> implications are neglectable for now. >>>> >>>> This is a preparation for better reference and change bit handling for >>>> TCG, which will require more MMU changes. >>>> >>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> target/s390x/mem_helper.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c >>>> index 29d9eaa5b7..ed54265e03 100644 >>>> --- a/target/s390x/mem_helper.c >>>> +++ b/target/s390x/mem_helper.c >>>> @@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2) >>>> >>>> key = (uint8_t) r1; >>>> skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); >>>> + /* TODO: Flush only entries with this target address */ >>>> + tlb_flush_all_cpus_synced(env_cpu(env)); >>> >>> Doesn't: >>> >>> tlb_flush_page_all_cpus_synced(env_cpu(env), addr & TARGET_PAGE_MASK); >>> >>> do what you want here? >> >> I would have to flush all TLB entries that target this physical page, >> not the entry of the single virtual page. So that does, unfortunately, >> not work. > > Ahh I see. Well maybe that should be the comment instead: > > /* > * As we can only flush by virtual address and not all the entries > * that point to a physical address we have to flush the whole TLB > * here. > */ > tlb_flush_all_cpus_synced(env_cpu(env)); > > ? I think "entries that target this address" makes it clear that we are talking about the TLB entry target address, not the virtual source address. But I can adjust it. Thanks.
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 29d9eaa5b7..ed54265e03 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2) key = (uint8_t) r1; skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); + /* TODO: Flush only entries with this target address */ + tlb_flush_all_cpus_synced(env_cpu(env)); } /* reset reference bit extended */ @@ -1843,6 +1845,8 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2) if (skeyclass->set_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) { return 0; } + /* TODO: Flush only entries with this target address */ + tlb_flush_all_cpus_synced(env_cpu(env)); /* * cc