Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
On Thu, Jan 09, 2020 at 08:13:38AM +0100, Cédric Le Goater wrote:
> void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
> {
> -int irq = book3s_dbell2irq(rb);
> +int irq = book3s_dbell2irq(rb, 1);
> >>>
> >>> true/false are preferred to 0/1 for bool types.
> >>
> >> yes or a define ?
> >
> > Sorry, I don't understand the question. The second argument to
> > book3s_dbell2irq() is a 'bool' type, so the parameter should be 'true'
> > rather than '1'.
>
> Yes. I was thinking of adding defines for the 'true' and 'false' value
> to clarify what the parameter is doing. But it does not seem really
> useful now.
You don't need your own defines, they already exist.
--
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
signature.asc
Description: PGP signature
Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
{
-int irq = book3s_dbell2irq(rb);
+int irq = book3s_dbell2irq(rb, 1);
>>>
>>> true/false are preferred to 0/1 for bool types.
>>
>> yes or a define ?
>
> Sorry, I don't understand the question. The second argument to
> book3s_dbell2irq() is a 'bool' type, so the parameter should be 'true'
> rather than '1'.
Yes. I was thinking of adding defines for the 'true' and 'false' value
to clarify what the parameter is doing. But it does not seem really
useful now.
Thanks,
C.
Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
On Wed, Jan 08, 2020 at 04:32:19PM +0100, Cédric Le Goater wrote:
> On 12/17/19 5:00 AM, David Gibson wrote:
> > On Thu, Nov 28, 2019 at 02:46:58PM +0100, Cédric Le Goater wrote:
> >> From: Suraj Jitindar Singh
> >>
> >> Privileged message send facilities exist on POWER8 processors and
> >> later and include a register and instructions which can be used to
> >> generate, observe/modify the state of and clear privileged doorbell
> >> exceptions as described below.
> >
> > So, it's probably worth noting somewhere here that the "privileged
> > doorbell" instructions, being supervisor privileged than the existing
> > doorbell instructions which are hypervisor privileged.
>
> Sure. I will add a sentence on the topic.
>
> >> The Directed Privileged Doorbell Exception State (DPDES) register
> >> reflects the state of pending privileged doorbell exceptions and can
> >> also be used to modify that state. The register can be used to read
> >> and modify the state of privileged doorbell exceptions for all threads
> >> of a subprocessor and thus is a shared facility for that subprocessor.
> >> The register can be read/written by the hypervisor and read by the
> >> supervisor if enabled in the HFSCR, otherwise a hypervisor facility
> >> unavailable exception is generated.
> >>
> >> The privileged message send and clear instructions (msgsndp & msgclrp)
> >> are used to generate and clear the presence of a directed privileged
> >> doorbell exception, respectively. The msgsndp instruction can be used
> >> to target any thread of the current subprocessor, msgclrp acts on the
> >> thread issuing the instruction. These instructions are privileged, but
> >> will generate a hypervisor facility unavailable exception if not
> >> enabled in the HFSCR and executed in privileged non-hypervisor
> >> state. The HV facility unavailable exception will be addressed in
> >> other patch.
> >>
> >> Add and implement this register and instructions by reading or
> >> modifying the pending interrupt state of the cpu.
> >>
> >> Note that TCG only supports one thread per core and so we only need to
> >> worry about the cpu making the access.
> >>
> >> Signed-off-by: Suraj Jitindar Singh
> >> [clg: - introduced a book3s_msgsnd_common() helper to reduce code
> >>duplication
> >> - modified book3s_dbell2irq()
> >> - moved out the support for hypervisor facility unavailable
> >>exception
> >> - modified commit log to add a statement on the HV FU exception
> >> - checkpatch fixes
> >> - compile fixes for the ppc-softmmu and linux-user targets ]
> >> Signed-off-by: Cédric Le Goater
> >>
> >> Signed-off-by: Cédric Le Goater
> >
> > Double signoff?
>
> I don't know why git-am added an extra one :/ I hope this is gone.
>
> >> ---
> >> target/ppc/helper.h | 4 +++
> >> target/ppc/excp_helper.c| 43 -
> >> target/ppc/misc_helper.c| 22 +
> >> target/ppc/translate.c | 26
> >> target/ppc/translate_init.inc.c | 20 ---
> >> 5 files changed, 105 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> >> index cd0dfe383a2a..76518a1df6f0 100644
> >> --- a/target/ppc/helper.h
> >> +++ b/target/ppc/helper.h
> >> @@ -657,6 +657,10 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG,
> >> tl, env)
> >> DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
> >> DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
> >> DEF_HELPER_2(store_ptcr, void, env, tl)
> >> +DEF_HELPER_FLAGS_1(load_dpdes, TCG_CALL_NO_RWG, tl, env)
> >> +DEF_HELPER_FLAGS_2(store_dpdes, TCG_CALL_NO_RWG, void, env, tl)
> >> +DEF_HELPER_1(book3s_msgsndp, void, tl)
> >> +DEF_HELPER_2(book3s_msgclrp, void, env, tl)
> >> #endif
> >> DEF_HELPER_2(store_sdr1, void, env, tl)
> >> DEF_HELPER_2(store_pidr, void, env, tl)
> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> >> index 50b004d00d1e..5a247945e97f 100644
> >> --- a/target/ppc/excp_helper.c
> >> +++ b/target/ppc/excp_helper.c
> >> @@ -898,7 +898,11 @@ static void ppc_hw_interrupt(CPUPPCState *env)
> >> }
> >> if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
> >> env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
> >> -powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
> >> +if (env->insns_flags & PPC_SEGMENT_64B) {
> >> +powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_SDOOR);
> >> +} else {
> >> +powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
> >> +}
> >> return;
> >> }
> >> if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDOORBELL)) {
> >> @@ -1219,7 +1223,7 @@ void helper_msgsnd(target_ulong rb)
> >> }
> >>
> >> /* Server Processor Control */
> >> -static int book3s_dbell2irq(target_ulong rb)
> >>
Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
On 12/17/19 5:00 AM, David Gibson wrote:
> On Thu, Nov 28, 2019 at 02:46:58PM +0100, Cédric Le Goater wrote:
>> From: Suraj Jitindar Singh
>>
>> Privileged message send facilities exist on POWER8 processors and
>> later and include a register and instructions which can be used to
>> generate, observe/modify the state of and clear privileged doorbell
>> exceptions as described below.
>
> So, it's probably worth noting somewhere here that the "privileged
> doorbell" instructions, being supervisor privileged than the existing
> doorbell instructions which are hypervisor privileged.
Sure. I will add a sentence on the topic.
>> The Directed Privileged Doorbell Exception State (DPDES) register
>> reflects the state of pending privileged doorbell exceptions and can
>> also be used to modify that state. The register can be used to read
>> and modify the state of privileged doorbell exceptions for all threads
>> of a subprocessor and thus is a shared facility for that subprocessor.
>> The register can be read/written by the hypervisor and read by the
>> supervisor if enabled in the HFSCR, otherwise a hypervisor facility
>> unavailable exception is generated.
>>
>> The privileged message send and clear instructions (msgsndp & msgclrp)
>> are used to generate and clear the presence of a directed privileged
>> doorbell exception, respectively. The msgsndp instruction can be used
>> to target any thread of the current subprocessor, msgclrp acts on the
>> thread issuing the instruction. These instructions are privileged, but
>> will generate a hypervisor facility unavailable exception if not
>> enabled in the HFSCR and executed in privileged non-hypervisor
>> state. The HV facility unavailable exception will be addressed in
>> other patch.
>>
>> Add and implement this register and instructions by reading or
>> modifying the pending interrupt state of the cpu.
>>
>> Note that TCG only supports one thread per core and so we only need to
>> worry about the cpu making the access.
>>
>> Signed-off-by: Suraj Jitindar Singh
>> [clg: - introduced a book3s_msgsnd_common() helper to reduce code
>> duplication
>> - modified book3s_dbell2irq()
>> - moved out the support for hypervisor facility unavailable
>> exception
>> - modified commit log to add a statement on the HV FU exception
>> - checkpatch fixes
>> - compile fixes for the ppc-softmmu and linux-user targets ]
>> Signed-off-by: Cédric Le Goater
>>
>> Signed-off-by: Cédric Le Goater
>
> Double signoff?
I don't know why git-am added an extra one :/ I hope this is gone.
>> ---
>> target/ppc/helper.h | 4 +++
>> target/ppc/excp_helper.c| 43 -
>> target/ppc/misc_helper.c| 22 +
>> target/ppc/translate.c | 26
>> target/ppc/translate_init.inc.c | 20 ---
>> 5 files changed, 105 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index cd0dfe383a2a..76518a1df6f0 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -657,6 +657,10 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl,
>> env)
>> DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
>> DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
>> DEF_HELPER_2(store_ptcr, void, env, tl)
>> +DEF_HELPER_FLAGS_1(load_dpdes, TCG_CALL_NO_RWG, tl, env)
>> +DEF_HELPER_FLAGS_2(store_dpdes, TCG_CALL_NO_RWG, void, env, tl)
>> +DEF_HELPER_1(book3s_msgsndp, void, tl)
>> +DEF_HELPER_2(book3s_msgclrp, void, env, tl)
>> #endif
>> DEF_HELPER_2(store_sdr1, void, env, tl)
>> DEF_HELPER_2(store_pidr, void, env, tl)
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 50b004d00d1e..5a247945e97f 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -898,7 +898,11 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>> }
>> if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
>> env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
>> -powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
>> +if (env->insns_flags & PPC_SEGMENT_64B) {
>> +powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_SDOOR);
>> +} else {
>> +powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
>> +}
>> return;
>> }
>> if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDOORBELL)) {
>> @@ -1219,7 +1223,7 @@ void helper_msgsnd(target_ulong rb)
>> }
>>
>> /* Server Processor Control */
>> -static int book3s_dbell2irq(target_ulong rb)
>> +static int book3s_dbell2irq(target_ulong rb, bool hv_dbell)
>> {
>> int msg = rb & DBELL_TYPE_MASK;
>>
>> @@ -1228,12 +1232,16 @@ static int book3s_dbell2irq(target_ulong rb)
>> * message type is 5. All other types are reserved and the
>> * instruction
Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
On Thu, Nov 28, 2019 at 02:46:58PM +0100, Cédric Le Goater wrote:
> From: Suraj Jitindar Singh
>
> Privileged message send facilities exist on POWER8 processors and
> later and include a register and instructions which can be used to
> generate, observe/modify the state of and clear privileged doorbell
> exceptions as described below.
So, it's probably worth noting somewhere here that the "privileged
doorbell" instructions, being supervisor privileged than the existing
doorbell instructions which are hypervisor privileged.
> The Directed Privileged Doorbell Exception State (DPDES) register
> reflects the state of pending privileged doorbell exceptions and can
> also be used to modify that state. The register can be used to read
> and modify the state of privileged doorbell exceptions for all threads
> of a subprocessor and thus is a shared facility for that subprocessor.
> The register can be read/written by the hypervisor and read by the
> supervisor if enabled in the HFSCR, otherwise a hypervisor facility
> unavailable exception is generated.
>
> The privileged message send and clear instructions (msgsndp & msgclrp)
> are used to generate and clear the presence of a directed privileged
> doorbell exception, respectively. The msgsndp instruction can be used
> to target any thread of the current subprocessor, msgclrp acts on the
> thread issuing the instruction. These instructions are privileged, but
> will generate a hypervisor facility unavailable exception if not
> enabled in the HFSCR and executed in privileged non-hypervisor
> state. The HV facility unavailable exception will be addressed in
> other patch.
>
> Add and implement this register and instructions by reading or
> modifying the pending interrupt state of the cpu.
>
> Note that TCG only supports one thread per core and so we only need to
> worry about the cpu making the access.
>
> Signed-off-by: Suraj Jitindar Singh
> [clg: - introduced a book3s_msgsnd_common() helper to reduce code
> duplication
> - modified book3s_dbell2irq()
> - moved out the support for hypervisor facility unavailable
> exception
> - modified commit log to add a statement on the HV FU exception
> - checkpatch fixes
> - compile fixes for the ppc-softmmu and linux-user targets ]
> Signed-off-by: Cédric Le Goater
>
> Signed-off-by: Cédric Le Goater
Double signoff?
> ---
> target/ppc/helper.h | 4 +++
> target/ppc/excp_helper.c| 43 -
> target/ppc/misc_helper.c| 22 +
> target/ppc/translate.c | 26
> target/ppc/translate_init.inc.c | 20 ---
> 5 files changed, 105 insertions(+), 10 deletions(-)
>
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index cd0dfe383a2a..76518a1df6f0 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -657,6 +657,10 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl,
> env)
> DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
> DEF_HELPER_2(store_ptcr, void, env, tl)
> +DEF_HELPER_FLAGS_1(load_dpdes, TCG_CALL_NO_RWG, tl, env)
> +DEF_HELPER_FLAGS_2(store_dpdes, TCG_CALL_NO_RWG, void, env, tl)
> +DEF_HELPER_1(book3s_msgsndp, void, tl)
> +DEF_HELPER_2(book3s_msgclrp, void, env, tl)
> #endif
> DEF_HELPER_2(store_sdr1, void, env, tl)
> DEF_HELPER_2(store_pidr, void, env, tl)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 50b004d00d1e..5a247945e97f 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -898,7 +898,11 @@ static void ppc_hw_interrupt(CPUPPCState *env)
> }
> if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
> env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
> -powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
> +if (env->insns_flags & PPC_SEGMENT_64B) {
> +powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_SDOOR);
> +} else {
> +powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
> +}
> return;
> }
> if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDOORBELL)) {
> @@ -1219,7 +1223,7 @@ void helper_msgsnd(target_ulong rb)
> }
>
> /* Server Processor Control */
> -static int book3s_dbell2irq(target_ulong rb)
> +static int book3s_dbell2irq(target_ulong rb, bool hv_dbell)
> {
> int msg = rb & DBELL_TYPE_MASK;
>
> @@ -1228,12 +1232,16 @@ static int book3s_dbell2irq(target_ulong rb)
> * message type is 5. All other types are reserved and the
> * instruction is a no-op
> */
> -return msg == DBELL_TYPE_DBELL_SERVER ? PPC_INTERRUPT_HDOORBELL : -1;
> +if (msg == DBELL_TYPE_DBELL_SERVER) {
> +return hv_dbell ? PPC_INTERRUPT_HDOORBELL : PPC_INTERRUPT_DOORBELL;
> +}
> +
> +return -1;
