Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when
 ICC_CTLR_EL1.PMHE is clear
To:     Marc Zyngier <maz@kernel.org>
CC:     Will Deacon <will@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        James Morse <james.morse@arm.com>,
        Julien Thierry <julien.thierry.kdev@gmail.com>,
        <huawei.libin@huawei.com>, <guohanjun@huawei.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>
References: <20191002090613.14236-1-maz@kernel.org>
 <20191002090613.14236-2-maz@kernel.org>
 <ad164b94-06af-ffe7-b8ff-317b4078b1a5@huawei.com>
 <4ed2ed389a81cc0ec6f3150ce38517a5@www.loen.fr>
From:   "liwei (GF)" <liwei391@huawei.com>
Message-ID: <28f18d5f-b04c-e082-6a03-580740244590@huawei.com>
Date:   Sat, 26 Oct 2019 09:42:32 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <4ed2ed389a81cc0ec6f3150ce38517a5@www.loen.fr>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Marc,

On 2019/10/23 20:13, Marc Zyngier wrote:
> Hi Wei,
> 
> On 2019-10-23 09:38, liwei (GF) wrote:
>> Hi Marc,
>>
>> On 2019/10/2 17:06, Marc Zyngier wrote:
>>> The GICv3 architecture specification is incredibly misleading when it
>>> comes to PMR and the requirement for a DSB. It turns out that this DSB
>>> is only required if the CPU interface sends an Upstream Control
>>> message to the redistributor in order to update the RD's view of PMR.
>>>
>>> This message is only sent when ICC_CTLR_EL1.PMHE is set, which isn't
>>> the case in Linux. It can still be set from EL3, so some special care
>>> is required. But the upshot is that in the (hopefuly large) majority
>>> of the cases, we can drop the DSB altogether.
>>>
>>> This relies on a new static key being set if the boot CPU has PMHE
>>> set. The drawback is that this static key has to be exported to
>>> modules.
>>>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Will Deacon <will@kernel.org>
>>> Cc: James Morse <james.morse@arm.com>
>>> Cc: Julien Thierry <julien.thierry.kdev@gmail.com>
>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/include/asm/barrier.h   | 12 ++++++++++++
>>>  arch/arm64/include/asm/daifflags.h |  3 ++-
>>>  arch/arm64/include/asm/irqflags.h  | 19 ++++++++++---------
>>>  arch/arm64/include/asm/kvm_host.h  |  3 +--
>>>  arch/arm64/kernel/entry.S          |  6 ++++--
>>>  arch/arm64/kvm/hyp/switch.c        |  4 ++--
>>>  drivers/irqchip/irq-gic-v3.c       | 20 ++++++++++++++++++++
>>>  include/linux/irqchip/arm-gic-v3.h |  2 ++
>>>  8 files changed, 53 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
>>> index e0e2b1946f42..7d9cc5ec4971 100644
>>> --- a/arch/arm64/include/asm/barrier.h
>>> +++ b/arch/arm64/include/asm/barrier.h
>>> @@ -29,6 +29,18 @@
>>>                           SB_BARRIER_INSN"nop\n",    \
>>>                           ARM64_HAS_SB))
>>>
>>> +#ifdef CONFIG_ARM64_PSEUDO_NMI
>>> +#define pmr_sync()                        \
>>> +    do {                            \
>>> +        extern struct static_key_false gic_pmr_sync;    \
>>> +                                \
>>> +        if (static_branch_unlikely(&gic_pmr_sync))    \
>>> +            dsb(sy);                \
>>> +    } while(0)
>>> +#else
>>> +#define pmr_sync()    do {} while (0)
>>> +#endif
>>> +
>>
>> Thank you for solving this problem, it helps a lot indeed.
>>
>> The pmr_sync() will call dsb(sy) when ARM64_PSEUDO_NMI=y and
>> gic_pmr_sync=force,
>> but if pseudo nmi is not enabled through boot option, it just take one more
>> redundant calling than before at the following two place.
>>
>> I think change dsb(sy) to
>> +                       asm volatile(ALTERNATIVE("nop", "dsb sy",      \
>> +                               ARM64_HAS_IRQ_PRIO_MASKING)     \
>> +                               : : : "memory");                \
>> may be more appropriate.
> 
> I'm not sure I understand what you mean. The static key defaults to false,
> so if pseudo_nmi is not enabled, this dsb(sy) is simply never executed.
> 
> Am I missing something obvious?
> 
> Thanks,
> 
>         M.
> 
You are right, my mistake. Sorry for confusing you.

Thanks,
Wei