[RFC,v4,3/9] softmmu: Add helpers for a new slowpath

The new helpers rely on the legacy ones to perform the actual read/write.

The LoadLink helper (helper_ldlink_name) prepares the way for the
following SC operation. It sets the linked address and the size of the
access.
These helper also update the TLB entry of the page involved in the
LL/SC for those vCPUs that have the bit set (dirty), so that the
following accesses made by all the vCPUs will follow the slow path.

The StoreConditional helper (helper_stcond_name) returns 1 if the
store has to fail due to a concurrent access to the same page by
another vCPU. A 'concurrent access' can be a store made by *any* vCPU
(although, some implementations allow stores made by the CPU that issued
the LoadLink).

Suggested-by: Jani Kokkonen <jani.kokkonen@huawei.com>
Suggested-by: Claudio Fontana <claudio.fontana@huawei.com>
Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 cputlb.c                |   3 ++
 softmmu_llsc_template.h | 124 ++++++++++++++++++++++++++++++++++++++++++++++++
 softmmu_template.h      |  12 +++++
 3 files changed, 139 insertions(+)
 create mode 100644 softmmu_llsc_template.h

On Fri, Aug 7, 2015 at 7:03 PM, Alvise Rigo
<a.rigo@virtualopensystems.com> wrote:
> The new helpers rely on the legacy ones to perform the actual read/write.
>
> The LoadLink helper (helper_ldlink_name) prepares the way for the
> following SC operation. It sets the linked address and the size of the
> access.
> These helper also update the TLB entry of the page involved in the
> LL/SC for those vCPUs that have the bit set (dirty), so that the
> following accesses made by all the vCPUs will follow the slow path.
>
> The StoreConditional helper (helper_stcond_name) returns 1 if the
> store has to fail due to a concurrent access to the same page by
> another vCPU. A 'concurrent access' can be a store made by *any* vCPU
> (although, some implementations allow stores made by the CPU that issued
> the LoadLink).
>
> Suggested-by: Jani Kokkonen <jani.kokkonen@huawei.com>
> Suggested-by: Claudio Fontana <claudio.fontana@huawei.com>
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> ---
>  cputlb.c                |   3 ++
>  softmmu_llsc_template.h | 124 ++++++++++++++++++++++++++++++++++++++++++++++++
>  softmmu_template.h      |  12 +++++
>  3 files changed, 139 insertions(+)
>  create mode 100644 softmmu_llsc_template.h
>
> diff --git a/cputlb.c b/cputlb.c
> index 251eec8..f45afc6 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -415,6 +415,8 @@ static inline void lookup_and_reset_cpus_ll_addr(hwaddr addr, hwaddr size)
>
>  #define MMUSUFFIX _mmu
>
> +/* Generates LoadLink/StoreConditional helpers in softmmu_template.h */
> +#define GEN_EXCLUSIVE_HELPERS
>  #define SHIFT 0
>  #include "softmmu_template.h"
>
> @@ -427,6 +429,7 @@ static inline void lookup_and_reset_cpus_ll_addr(hwaddr addr, hwaddr size)
>  #define SHIFT 3
>  #include "softmmu_template.h"
>  #undef MMUSUFFIX
> +#undef GEN_EXCLUSIVE_HELPERS
>
>  #define MMUSUFFIX _cmmu
>  #undef GETPC_ADJ
> diff --git a/softmmu_llsc_template.h b/softmmu_llsc_template.h
> new file mode 100644
> index 0000000..d2e92b4
> --- /dev/null
> +++ b/softmmu_llsc_template.h
> @@ -0,0 +1,124 @@
> +/*
> + *  Software MMU support (esclusive load/store operations)
> + *
> + * Generate helpers used by TCG for qemu_ldlink/stcond ops.
> + *
> + * Included from softmmu_template.h only.
> + *
> + * Copyright (c) 2015 Virtual Open Systems
> + *
> + * Authors:
> + *  Alvise Rigo <a.rigo@virtualopensystems.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/* This template does not generate together the le and be version, but only one
> + * of the two depending on whether BIGENDIAN_EXCLUSIVE_HELPERS has been set.
> + * The same nomenclature as softmmu_template.h is used for the exclusive
> + * helpers.  */
> +
> +#ifdef BIGENDIAN_EXCLUSIVE_HELPERS
> +
> +#define helper_ldlink_name  glue(glue(helper_be_ldlink, USUFFIX), MMUSUFFIX)
> +#define helper_stcond_name  glue(glue(helper_be_stcond, SUFFIX), MMUSUFFIX)
> +#define helper_ld_legacy glue(glue(helper_be_ld, USUFFIX), MMUSUFFIX)
> +#define helper_st_legacy glue(glue(helper_be_st, SUFFIX), MMUSUFFIX)
> +
> +#else /* LE helpers + 8bit helpers (generated only once for both LE end BE) */
> +
> +#if DATA_SIZE > 1
> +#define helper_ldlink_name  glue(glue(helper_le_ldlink, USUFFIX), MMUSUFFIX)
> +#define helper_stcond_name  glue(glue(helper_le_stcond, SUFFIX), MMUSUFFIX)
> +#define helper_ld_legacy glue(glue(helper_le_ld, USUFFIX), MMUSUFFIX)
> +#define helper_st_legacy glue(glue(helper_le_st, SUFFIX), MMUSUFFIX)

Wouldn't be better here to not use the helpers from softmmu_template.h?
Doing so, in softmmu_template.h we don't need to differentiate the
normal stores from the SCs
This at a cost of some additional boilerplate in softmmu_llsc_template.h.

Thanks,
alvise

> +#else /* DATA_SIZE <= 1 */
> +#define helper_ldlink_name  glue(glue(helper_ret_ldlink, USUFFIX), MMUSUFFIX)
> +#define helper_stcond_name  glue(glue(helper_ret_stcond, SUFFIX), MMUSUFFIX)
> +#define helper_ld_legacy glue(glue(helper_ret_ld, USUFFIX), MMUSUFFIX)
> +#define helper_st_legacy glue(glue(helper_ret_st, SUFFIX), MMUSUFFIX)
> +#endif
> +
> +#endif
> +
> +WORD_TYPE helper_ldlink_name(CPUArchState *env, target_ulong addr,
> +                                TCGMemOpIdx oi, uintptr_t retaddr)
> +{
> +    WORD_TYPE ret;
> +    int index;
> +    CPUState *cpu;
> +    hwaddr hw_addr;
> +    unsigned mmu_idx = get_mmuidx(oi);
> +
> +    /* Use the proper load helper from cpu_ldst.h */
> +    ret = helper_ld_legacy(env, addr, mmu_idx, retaddr);
> +
> +    index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> +
> +    /* hw_addr = hwaddr of the page (i.e. section->mr->ram_addr + xlat)
> +     * plus the offset (i.e. addr & ~TARGET_PAGE_MASK) */
> +    hw_addr = (env->iotlb[mmu_idx][index].addr & TARGET_PAGE_MASK) + addr;
> +
> +    cpu_physical_memory_clear_excl_dirty(hw_addr, ENV_GET_CPU(env)->cpu_index);
> +    /* If all the vCPUs have the EXCL bit set for this page there is no need
> +     * to request any flush. */
> +    if (cpu_physical_memory_excl_is_dirty(hw_addr, smp_cpus)) {
> +        CPU_FOREACH(cpu) {
> +            if (current_cpu != cpu) {
> +                if (cpu_physical_memory_excl_is_dirty(hw_addr,
> +                                                    cpu->cpu_index)) {
> +                    cpu_physical_memory_clear_excl_dirty(hw_addr,
> +                                                         cpu->cpu_index);
> +                    tlb_flush(cpu, 1);
> +                }
> +            }
> +        }
> +    }
> +
> +    env->excl_protected_range.begin = hw_addr;
> +    env->excl_protected_range.end = hw_addr + DATA_SIZE;
> +
> +    /* For this vCPU, just update the TLB entry, no need to flush. */
> +    env->tlb_table[mmu_idx][index].addr_write |= TLB_EXCL;
> +
> +    return ret;
> +}
> +
> +WORD_TYPE helper_stcond_name(CPUArchState *env, target_ulong addr,
> +                             DATA_TYPE val, TCGMemOpIdx oi,
> +                             uintptr_t retaddr)
> +{
> +    WORD_TYPE ret;
> +    unsigned mmu_idx = get_mmuidx(oi);
> +
> +    /* We set it preventively to true to distinguish the following legacy
> +     * access as one made by the store conditional wrapper. If the store
> +     * conditional does not succeed, the value will be set to 0.*/
> +    env->excl_succeeded = 1;
> +    helper_st_legacy(env, addr, val, mmu_idx, retaddr);
> +
> +    if (env->excl_succeeded) {
> +        env->excl_succeeded = 0;
> +        ret = 0;
> +    } else {
> +        ret = 1;
> +    }
> +
> +    return ret;
> +}
> +
> +#undef helper_ldlink_name
> +#undef helper_stcond_name
> +#undef helper_ld_legacy
> +#undef helper_st_legacy
> diff --git a/softmmu_template.h b/softmmu_template.h
> index e4431e8..ad65d20 100644
> --- a/softmmu_template.h
> +++ b/softmmu_template.h
> @@ -640,6 +640,18 @@ void probe_write(CPUArchState *env, target_ulong addr, int mmu_idx,
>  #endif
>  #endif /* !defined(SOFTMMU_CODE_ACCESS) */
>
> +#ifdef GEN_EXCLUSIVE_HELPERS
> +
> +#if DATA_SIZE > 1 /* The 8-bit helpers are generate along with LE helpers */
> +#define BIGENDIAN_EXCLUSIVE_HELPERS
> +#include "softmmu_llsc_template.h"
> +#undef BIGENDIAN_EXCLUSIVE_HELPERS
> +#endif
> +
> +#include "softmmu_llsc_template.h"
> +
> +#endif /* !defined(GEN_EXCLUSIVE_HELPERS) */
> +
>  #undef READ_ACCESS_TYPE
>  #undef SHIFT
>  #undef DATA_TYPE
> --
> 2.5.0
>

On 11/08/2015 15:32, alvise rigo wrote:
>> > +#if DATA_SIZE > 1
>> > +#define helper_ldlink_name  glue(glue(helper_le_ldlink, USUFFIX), MMUSUFFIX)
>> > +#define helper_stcond_name  glue(glue(helper_le_stcond, SUFFIX), MMUSUFFIX)
>> > +#define helper_ld_legacy glue(glue(helper_le_ld, USUFFIX), MMUSUFFIX)
>> > +#define helper_st_legacy glue(glue(helper_le_st, SUFFIX), MMUSUFFIX)
> Wouldn't be better here to not use the helpers from softmmu_template.h?
> Doing so, in softmmu_template.h we don't need to differentiate the
> normal stores from the SCs
> This at a cost of some additional boilerplate in softmmu_llsc_template.h.

A lot more boilerplate I think?

This patch looks good, I would just s/_legacy// because it's not really
legacy.

Paolo

On Tue, Aug 11, 2015 at 3:52 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 11/08/2015 15:32, alvise rigo wrote:
>>> > +#if DATA_SIZE > 1
>>> > +#define helper_ldlink_name  glue(glue(helper_le_ldlink, USUFFIX), MMUSUFFIX)
>>> > +#define helper_stcond_name  glue(glue(helper_le_stcond, SUFFIX), MMUSUFFIX)
>>> > +#define helper_ld_legacy glue(glue(helper_le_ld, USUFFIX), MMUSUFFIX)
>>> > +#define helper_st_legacy glue(glue(helper_le_st, SUFFIX), MMUSUFFIX)
>> Wouldn't be better here to not use the helpers from softmmu_template.h?
>> Doing so, in softmmu_template.h we don't need to differentiate the
>> normal stores from the SCs
>> This at a cost of some additional boilerplate in softmmu_llsc_template.h.
>
> A lot more boilerplate I think?
>
> This patch looks good, I would just s/_legacy// because it's not really
> legacy.

Agreed.

alvise

>
> Paolo

On 07/08/2015 19:03, Alvise Rigo wrote:
> +
> +    /* For this vCPU, just update the TLB entry, no need to flush. */
> +    env->tlb_table[mmu_idx][index].addr_write |= TLB_EXCL;

Couldn't this vCPU also have two aliasing entries in the TLB?

Paolo

Yes, it could. However, it's really unlikely that a vCPU, after
issuing a LL to the virtual address x, it stores to the same phys
address using the virtual address y.

I'm not really sure If we really need to handle these cases.

alvise

On Wed, Aug 12, 2015 at 2:43 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 07/08/2015 19:03, Alvise Rigo wrote:
>> +
>> +    /* For this vCPU, just update the TLB entry, no need to flush. */
>> +    env->tlb_table[mmu_idx][index].addr_write |= TLB_EXCL;
>
> Couldn't this vCPU also have two aliasing entries in the TLB?
>
> Paolo

On 12/08/2015 15:09, alvise rigo wrote:
> Yes, it could. However, it's really unlikely that a vCPU, after
> issuing a LL to the virtual address x, it stores to the same phys
> address using the virtual address y.
> 
> I'm not really sure If we really need to handle these cases.

Ok, if we had to it's just a matter of adding a TLB flush here.

Paolo