* Re: [PATCH] cpuidle/pseries: Fixup CEDE0 latency only for POWER10 onwards
From: Vaidyanathan Srinivasan @ 2021-04-22 17:57 UTC (permalink / raw)
To: Gautham R. Shenoy
Cc: linux-pm, Daniel Lezcano, Rafael J. Wysocki, joedecke,
Aneesh Kumar K.V, linuxppc-dev
In-Reply-To: <1619104049-5118-1-git-send-email-ego@linux.vnet.ibm.com>
* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2021-04-22 20:37:29]:
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>
> Commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
> CEDE(0)") sets the exit latency of CEDE(0) based on the latency values
> of the Extended CEDE states advertised by the platform
>
> On some of the POWER9 LPARs, the older firmwares advertise a very low
> value of 2us for CEDE1 exit latency on a Dedicated LPAR. However the
> measured value is 5us on an average. Due to the low advertised exit
> latency, we are entering CEDE(0) more aggressively on such
> platforms. While this helps achieve SMT folding faster, we pay the
> penalty of having to send an IPI to wakeup the CPU when the target
> residency is very short. This is showing up as a performance
> regression on the newer kernels running on the LPARs with older
> firmware.
>
> Hence, set the exit latency of CEDE(0) based on the latency values
> advertized by platform only from POWER10 onwards. The values
> advertized on POWER10 platforms is more realistic and informed by the
> latency measurements.
>
> For platforms older than POWER10, retain the hardcoded value of exit
> latency, which is 10us. Though this is higher than the measured
> values, we would be erring on the side of caution.
>
> Reported-by: Enrico Joedecke <joedecke@de.ibm.com>
> Fixes: commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
> CEDE(0)")
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>
> ---
> drivers/cpuidle/cpuidle-pseries.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
> index a2b5c6f..7207467 100644
> --- a/drivers/cpuidle/cpuidle-pseries.c
> +++ b/drivers/cpuidle/cpuidle-pseries.c
> @@ -419,7 +419,8 @@ static int pseries_idle_probe(void)
> cpuidle_state_table = shared_states;
> max_idle_state = ARRAY_SIZE(shared_states);
> } else {
> - fixup_cede0_latency();
> + if (pvr_version_is(PVR_POWER10))
> + fixup_cede0_latency();
> cpuidle_state_table = dedicated_states;
> max_idle_state = NR_DEDICATED_STATES;
> }
Thanks for the fix. We should have such safeguards or fallbacks while
running on older platforms. This fix is needed because of the
unfortunate regression on some older platforms that we failed to
notice while designing and testing the original feature.
--Vaidy
^ permalink raw reply
* Re: [PATCH V2 net] ibmvnic: Continue with reset if set link down failed
From: Lijun Pan @ 2021-04-22 17:38 UTC (permalink / raw)
To: Michal Suchánek
Cc: netdev, Lijun Pan, Tom Falcon, Paul Mackerras, Dany Madden,
Jakub Kicinski, Sukadev Bhattiprolu, linuxppc-dev, David Miller
In-Reply-To: <20210422172135.GY6564@kitsune.suse.cz>
On Thu, Apr 22, 2021 at 12:21 PM Michal Suchánek <msuchanek@suse.de> wrote:
>
> Hello,
>
> On Thu, Apr 22, 2021 at 12:06:45AM -0500, Lijun Pan wrote:
> > On Wed, Apr 21, 2021 at 2:25 AM Sukadev Bhattiprolu
> > <sukadev@linux.ibm.com> wrote:
> > >
> > > Lijun Pan [ljp@linux.vnet.ibm.com] wrote:
> > > >
> > > >
> > > > > On Apr 20, 2021, at 4:35 PM, Dany Madden <drt@linux.ibm.com> wrote:
> > > > >
> > > > > When ibmvnic gets a FATAL error message from the vnicserver, it marks
> > > > > the Command Respond Queue (CRQ) inactive and resets the adapter. If this
> > > > > FATAL reset fails and a transmission timeout reset follows, the CRQ is
> > > > > still inactive, ibmvnic's attempt to set link down will also fail. If
> > > > > ibmvnic abandons the reset because of this failed set link down and this
> > > > > is the last reset in the workqueue, then this adapter will be left in an
> > > > > inoperable state.
> > > > >
> > > > > Instead, make the driver ignore this link down failure and continue to
> > > > > free and re-register CRQ so that the adapter has an opportunity to
> > > > > recover.
> > > >
> > > > This v2 does not adddress the concerns mentioned in v1.
> > > > And I think it is better to exit with error from do_reset, and schedule a thorough
> > > > do_hard_reset if the the adapter is already in unstable state.
> > >
> > > We had a FATAL error and when handling it, we failed to send a
> > > link-down message to the VIOS. So what we need to try next is to
> > > reset the connection with the VIOS. For this we must talk to the
> > > firmware using the H_FREE_CRQ and H_REG_CRQ hcalls. do_reset()
> > > does just that in ibmvnic_reset_crq().
> > >
> > > Now, sure we can attempt a "thorough hard reset" which also does
> > > the same hcalls to reestablish the connection. Is there any
> > > other magic in do_hard_reset()? But in addition, it also frees lot
> > > more Linux kernel buffers and reallocates them for instance.
> >
> > Working around everything in do_reset will make the code very difficult
> > to manage. Ultimately do_reset can do anything I am afraid, and do_hard_reset
> > can be removed completely or merged into do_reset.
Michal,
I should have given more details about the above statement. Thanks for
your detailed info in the below.
>
> This debate is not very constructive.
>
> In the context of driver that has separate do_reset and do_hard_reset
> this fix picks the correct one unless you can refute the arguments
> provided.
>
> Merging do_reset and do_hard_reset might be a good code cleanup which is
> out of the scope of this fix.
Right.
>
>
>
> Given that vast majority of fixes to the vnic driver are related to the
> reset handling it would improve stability and testability if every
> reset took the same code path.
I agree.
>
> In the context of merging do_hard_reset and do_reset the question is
> what is the intended distinction and performance gain by having
> 'lightweight' reset.
Right.
>
> I don't have a vnic protocol manual at hand and I suspect I would not
> get one even if I searched for one.
>
> From reading through the fixes in the past my understanding is that the
> full reset is required when the backend changes which then potentially
> requires different size/number of buffers.
Yes, full reset is better when the backend changes.
>
> What is the expected situation when reset is required without changing
> the backend?
>
> Is this so common that it warrants a separate 'lightweight' optimized
> function?
>
^ permalink raw reply
* Re: [PATCH V2 net] ibmvnic: Continue with reset if set link down failed
From: Michal Suchánek @ 2021-04-22 17:21 UTC (permalink / raw)
To: Lijun Pan
Cc: netdev, Lijun Pan, Tom Falcon, Paul Mackerras, Dany Madden,
Jakub Kicinski, Sukadev Bhattiprolu, linuxppc-dev, David Miller
In-Reply-To: <CAOhMmr4ckVFTZtSeHFHNgGPUA12xYO8WcUoakx7WdwQfSKBJhA@mail.gmail.com>
Hello,
On Thu, Apr 22, 2021 at 12:06:45AM -0500, Lijun Pan wrote:
> On Wed, Apr 21, 2021 at 2:25 AM Sukadev Bhattiprolu
> <sukadev@linux.ibm.com> wrote:
> >
> > Lijun Pan [ljp@linux.vnet.ibm.com] wrote:
> > >
> > >
> > > > On Apr 20, 2021, at 4:35 PM, Dany Madden <drt@linux.ibm.com> wrote:
> > > >
> > > > When ibmvnic gets a FATAL error message from the vnicserver, it marks
> > > > the Command Respond Queue (CRQ) inactive and resets the adapter. If this
> > > > FATAL reset fails and a transmission timeout reset follows, the CRQ is
> > > > still inactive, ibmvnic's attempt to set link down will also fail. If
> > > > ibmvnic abandons the reset because of this failed set link down and this
> > > > is the last reset in the workqueue, then this adapter will be left in an
> > > > inoperable state.
> > > >
> > > > Instead, make the driver ignore this link down failure and continue to
> > > > free and re-register CRQ so that the adapter has an opportunity to
> > > > recover.
> > >
> > > This v2 does not adddress the concerns mentioned in v1.
> > > And I think it is better to exit with error from do_reset, and schedule a thorough
> > > do_hard_reset if the the adapter is already in unstable state.
> >
> > We had a FATAL error and when handling it, we failed to send a
> > link-down message to the VIOS. So what we need to try next is to
> > reset the connection with the VIOS. For this we must talk to the
> > firmware using the H_FREE_CRQ and H_REG_CRQ hcalls. do_reset()
> > does just that in ibmvnic_reset_crq().
> >
> > Now, sure we can attempt a "thorough hard reset" which also does
> > the same hcalls to reestablish the connection. Is there any
> > other magic in do_hard_reset()? But in addition, it also frees lot
> > more Linux kernel buffers and reallocates them for instance.
>
> Working around everything in do_reset will make the code very difficult
> to manage. Ultimately do_reset can do anything I am afraid, and do_hard_reset
> can be removed completely or merged into do_reset.
This debate is not very constructive.
In the context of driver that has separate do_reset and do_hard_reset
this fix picks the correct one unless you can refute the arguments
provided.
Merging do_reset and do_hard_reset might be a good code cleanup which is
out of the scope of this fix.
Given that vast majority of fixes to the vnic driver are related to the
reset handling it would improve stability and testability if every
reset took the same code path.
In the context of merging do_hard_reset and do_reset the question is
what is the intended distinction and performance gain by having
'lightweight' reset.
I don't have a vnic protocol manual at hand and I suspect I would not
get one even if I searched for one.
From reading through the fixes in the past my understanding is that the
full reset is required when the backend changes which then potentially
requires different size/number of buffers.
What is the expected situation when reset is required without changing
the backend?
Is this so common that it warrants a separate 'lightweight' optimized
function?
Thanks
Michal
^ permalink raw reply
* Re: [PATCH V2 net] ibmvnic: Continue with reset if set link down failed
From: Lijun Pan @ 2021-04-22 17:01 UTC (permalink / raw)
To: Rick Lindsley
Cc: Thomas Falcon, netdev, Paul Mackerras, Dany Madden,
Jakub Kicinski, Sukadev Bhattiprolu, linuxppc-dev,
David S. Miller
In-Reply-To: <49b3b535-3b81-6ffd-44b7-6226507859fa@linux.vnet.ibm.com>
On Thu, Apr 22, 2021 at 2:07 AM Rick Lindsley
<ricklind@linux.vnet.ibm.com> wrote:
>
> On 4/21/21 10:30 PM, Lijun Pan wrote:
> >> Fixes: ed651a10875f ("ibmvnic: Updated reset handling")
> >> Signed-off-by: Dany Madden <drt@linux.ibm.com>
> >> Reviewed-by: Rick Lindsley <ricklind@linux.ibm.com>
> >> Reviewed-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> >
> > One thing I would like to point out as already pointed out by Nathan Lynch is
> > that those review-by tags given by the same groups of people from the same
> > company loses credibility over time if you never critique or ask
> > questions on the list.
> >
>
> Well, so far you aren't addressing either my critiques or questions.
>
> I have been asking questions but all I have from you are the above
> attempts to discredit the reputation of myself and other people, and
> non-technical statements like
>
> will make the code very difficult to manage
> I think there should be a trade off between optimization and stability.
> So I don't think you could even compare the two results
>
> On the other hand, from the original submission I see some very specific
> details:
>
> If ibmvnic abandons the reset because of this failed set link
> down and this is the last reset in the workqueue, then this
> adapter will be left in an inoperable state.
>
> and from a followup discussion:
>
> We had a FATAL error and when handling it, we failed to
> send a link-down message to the VIOS. So what we need
> to try next is to reset the connection with the VIOS. For
> this we must ...
>
> These are great technical points that could be argued or discussed.
> Problem is, I agree with them.
>
> I will ask again: can you please supply some technical reasons for
> your objections. Otherwise, your objections are meritless and at worst
> simply an ad hominem attack.
Well, from the beginning of v1, I started to provide technical inputs.
Then I was not
allowed to post anything in the community about this patch and VNIC
via ljp@linux.ibm.com except giving an ack-by/reviewed-by.
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Segher Boessenkool @ 2021-04-22 16:54 UTC (permalink / raw)
To: David Laight
Cc: devicetree@vger.kernel.org, kbuild-all@lists.01.org,
lkp@intel.com, robh@kernel.org, Lakshmi Ramasubramanian,
bauerman@linux.ibm.com, linuxppc-dev, dan.carpenter@oracle.com,
'Daniel Axtens'
In-Reply-To: <3e6b31d92d5042d982daeb989e49299e@AcuMS.aculab.com>
On Thu, Apr 22, 2021 at 08:05:27AM +0000, David Laight wrote:
> > (Does anyone - and by anyone I mean any large distro - compile with
> > local variables inited by the compiler?)
>
> There are compilers that initialise locals to zero for 'debug' builds
> and leave the 'random' for optimised 'release' builds.
> Lets not test what we are releasing!
Yeah, that's the worst of all possible worlds.
> I also think there is a new option to gcc (or clang?) to initialise
> on-stack structures and arrays to ensure garbage isn't passed.
> That seems to be a horrid performance hit!
> Especially in userspace where large stack allocations are almost free.
>
> Any auto-initialise ought to be with a semi-random value
> (especially not zero) so that it is never right and doesn't
> lead to lazy coding.
Many compilers did something like this, decades ago -- for debug builds.
Segher
^ permalink raw reply
* Re: [PATCH 2/2] powerpc: Print esr register when hitting Program Interrupt
From: Christophe Leroy @ 2021-04-22 16:49 UTC (permalink / raw)
To: Xiongwei Song, mpe, benh, paulus, npiggin, ravi.bangoria, mikey,
aneesh.kumar, 0x7f454c46
Cc: Xiongwei Song, linuxppc-dev, linux-kernel
In-Reply-To: <ac6cf0ae-7565-180e-03b2-5e72f89a823a@csgroup.eu>
Le 22/04/2021 à 17:29, Christophe Leroy a écrit :
>
>
> Le 22/04/2021 à 17:10, Xiongwei Song a écrit :
>> From: Xiongwei Song <sxwjean@gmail.com>
>>
>> The esr register has the details of Program Interrupt on BookE/4xx cpus,
>> printing its value is helpful.
>>
>> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
>> ---
>> arch/powerpc/kernel/process.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index 5c3830837f3a..664aecf8ee2e 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -1459,6 +1459,7 @@ static bool interrupt_detail_printable(int trap)
>> case INTERRUPT_MACHINE_CHECK:
>> case INTERRUPT_DATA_STORAGE:
>> case INTERRUPT_ALIGNMENT:
>> + case INTERRUPT_PROGRAM:
>
> With this, it will also print the DSISR on 8xx/6xx so it will print garbage.
>
> 8xx/6xx provide the information in SRR1. If you want to proceed, you have to do the same as in ISI:
> Copy the content of SRR1 into regs->dsisr in the assembly handler in head_book3s_32.S and in the
> instruction TLB error handler in head_8xx.S
In fact, we already have get_reason() called by do_program_check() to retrieve the reason of the
program check exception. Maybe it could be used to print usefull information instead of starting
doing almost the same is another way.
Or we do as I suggested above, and we remove that get_reason() stuff. But get_reason() is also used
by the alignment exception. So that doesn't look easy.
I don't know what the best approach is.
>
>> return true;
>> default:
>> return false;
>>
^ permalink raw reply
* Re: [PATCH 2/2] powerpc: Print esr register when hitting Program Interrupt
From: Christophe Leroy @ 2021-04-22 15:29 UTC (permalink / raw)
To: Xiongwei Song, mpe, benh, paulus, npiggin, ravi.bangoria, mikey,
aneesh.kumar, 0x7f454c46
Cc: Xiongwei Song, linuxppc-dev, linux-kernel
In-Reply-To: <20210422151022.17868-2-sxwjean@me.com>
Le 22/04/2021 à 17:10, Xiongwei Song a écrit :
> From: Xiongwei Song <sxwjean@gmail.com>
>
> The esr register has the details of Program Interrupt on BookE/4xx cpus,
> printing its value is helpful.
>
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
> arch/powerpc/kernel/process.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 5c3830837f3a..664aecf8ee2e 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1459,6 +1459,7 @@ static bool interrupt_detail_printable(int trap)
> case INTERRUPT_MACHINE_CHECK:
> case INTERRUPT_DATA_STORAGE:
> case INTERRUPT_ALIGNMENT:
> + case INTERRUPT_PROGRAM:
With this, it will also print the DSISR on 8xx/6xx so it will print garbage.
8xx/6xx provide the information in SRR1. If you want to proceed, you have to do the same as in ISI:
Copy the content of SRR1 into regs->dsisr in the assembly handler in head_book3s_32.S and in the
instruction TLB error handler in head_8xx.S
> return true;
> default:
> return false;
>
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: Make the code in __show_regs nice-looking
From: Christophe Leroy @ 2021-04-22 15:27 UTC (permalink / raw)
To: Xiongwei Song, mpe, benh, paulus, npiggin, ravi.bangoria, mikey,
aneesh.kumar, 0x7f454c46
Cc: Xiongwei Song, linuxppc-dev, linux-kernel
In-Reply-To: <20210422151022.17868-1-sxwjean@me.com>
Le 22/04/2021 à 17:10, Xiongwei Song a écrit :
> From: Xiongwei Song <sxwjean@gmail.com>
>
> Create a new function named interrupt_detail_printable to judge which
> interrupts can print esr/dsisr register.
What is the benefit of that function ? It may be interesting if the test was done at several places,
but as it is done at only one place, I don't thing it is an improvement.
Until know, you new immediately what was the traps that would print it. Now you have to go and look
into a sub-function.
And the name is not nice either. All other functions testing anything on the trap type are called
trap_is_something()
Here your function would be better called something like trap_sets_dsisr()
>
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
> arch/powerpc/kernel/process.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 89e34aa273e2..5c3830837f3a 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1453,9 +1453,23 @@ static void print_msr_bits(unsigned long val)
> #define REGS_PER_LINE 8
> #endif
>
> +static bool interrupt_detail_printable(int trap)
> +{
> + switch (trap) {
> + case INTERRUPT_MACHINE_CHECK:
> + case INTERRUPT_DATA_STORAGE:
> + case INTERRUPT_ALIGNMENT:
> + return true;
> + default:
> + return false;
> + }
> +
> + return false;
That's redundant with the default case inside the switch.
> +}
> +
> static void __show_regs(struct pt_regs *regs)
> {
> - int i, trap;
> + int i;
>
> printk("NIP: "REG" LR: "REG" CTR: "REG"\n",
> regs->nip, regs->link, regs->ctr);
> @@ -1464,12 +1478,9 @@ static void __show_regs(struct pt_regs *regs)
> printk("MSR: "REG" ", regs->msr);
> print_msr_bits(regs->msr);
> pr_cont(" CR: %08lx XER: %08lx\n", regs->ccr, regs->xer);
> - trap = TRAP(regs);
> if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
> pr_cont("CFAR: "REG" ", regs->orig_gpr3);
> - if (trap == INTERRUPT_MACHINE_CHECK ||
> - trap == INTERRUPT_DATA_STORAGE ||
> - trap == INTERRUPT_ALIGNMENT) {
> + if (interrupt_detail_printable(TRAP(regs))) {
> if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
> else
>
^ permalink raw reply
* [PATCH] cpuidle/pseries: Fixup CEDE0 latency only for POWER10 onwards
From: Gautham R. Shenoy @ 2021-04-22 15:07 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, Michael Ellerman,
Aneesh Kumar K.V, Vaidyanathan Srinivasan
Cc: Gautham R. Shenoy, linuxppc-dev, joedecke, linux-pm
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
CEDE(0)") sets the exit latency of CEDE(0) based on the latency values
of the Extended CEDE states advertised by the platform
On some of the POWER9 LPARs, the older firmwares advertise a very low
value of 2us for CEDE1 exit latency on a Dedicated LPAR. However the
measured value is 5us on an average. Due to the low advertised exit
latency, we are entering CEDE(0) more aggressively on such
platforms. While this helps achieve SMT folding faster, we pay the
penalty of having to send an IPI to wakeup the CPU when the target
residency is very short. This is showing up as a performance
regression on the newer kernels running on the LPARs with older
firmware.
Hence, set the exit latency of CEDE(0) based on the latency values
advertized by platform only from POWER10 onwards. The values
advertized on POWER10 platforms is more realistic and informed by the
latency measurements.
For platforms older than POWER10, retain the hardcoded value of exit
latency, which is 10us. Though this is higher than the measured
values, we would be erring on the side of caution.
Reported-by: Enrico Joedecke <joedecke@de.ibm.com>
Fixes: commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
CEDE(0)")
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
drivers/cpuidle/cpuidle-pseries.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index a2b5c6f..7207467 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -419,7 +419,8 @@ static int pseries_idle_probe(void)
cpuidle_state_table = shared_states;
max_idle_state = ARRAY_SIZE(shared_states);
} else {
- fixup_cede0_latency();
+ if (pvr_version_is(PVR_POWER10))
+ fixup_cede0_latency();
cpuidle_state_table = dedicated_states;
max_idle_state = NR_DEDICATED_STATES;
}
--
1.9.4
^ permalink raw reply related
* [PATCH 2/2] powerpc: Print esr register when hitting Program Interrupt
From: Xiongwei Song @ 2021-04-22 15:10 UTC (permalink / raw)
To: mpe, benh, paulus, christophe.leroy, npiggin, ravi.bangoria,
mikey, aneesh.kumar, 0x7f454c46
Cc: Xiongwei Song, linuxppc-dev, linux-kernel
In-Reply-To: <20210422151022.17868-1-sxwjean@me.com>
From: Xiongwei Song <sxwjean@gmail.com>
The esr register has the details of Program Interrupt on BookE/4xx cpus,
printing its value is helpful.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/kernel/process.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 5c3830837f3a..664aecf8ee2e 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1459,6 +1459,7 @@ static bool interrupt_detail_printable(int trap)
case INTERRUPT_MACHINE_CHECK:
case INTERRUPT_DATA_STORAGE:
case INTERRUPT_ALIGNMENT:
+ case INTERRUPT_PROGRAM:
return true;
default:
return false;
--
2.17.1
^ permalink raw reply related
* [PATCH 1/2] powerpc: Make the code in __show_regs nice-looking
From: Xiongwei Song @ 2021-04-22 15:10 UTC (permalink / raw)
To: mpe, benh, paulus, christophe.leroy, npiggin, ravi.bangoria,
mikey, aneesh.kumar, 0x7f454c46
Cc: Xiongwei Song, linuxppc-dev, linux-kernel
From: Xiongwei Song <sxwjean@gmail.com>
Create a new function named interrupt_detail_printable to judge which
interrupts can print esr/dsisr register.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/kernel/process.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 89e34aa273e2..5c3830837f3a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1453,9 +1453,23 @@ static void print_msr_bits(unsigned long val)
#define REGS_PER_LINE 8
#endif
+static bool interrupt_detail_printable(int trap)
+{
+ switch (trap) {
+ case INTERRUPT_MACHINE_CHECK:
+ case INTERRUPT_DATA_STORAGE:
+ case INTERRUPT_ALIGNMENT:
+ return true;
+ default:
+ return false;
+ }
+
+ return false;
+}
+
static void __show_regs(struct pt_regs *regs)
{
- int i, trap;
+ int i;
printk("NIP: "REG" LR: "REG" CTR: "REG"\n",
regs->nip, regs->link, regs->ctr);
@@ -1464,12 +1478,9 @@ static void __show_regs(struct pt_regs *regs)
printk("MSR: "REG" ", regs->msr);
print_msr_bits(regs->msr);
pr_cont(" CR: %08lx XER: %08lx\n", regs->ccr, regs->xer);
- trap = TRAP(regs);
if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
pr_cont("CFAR: "REG" ", regs->orig_gpr3);
- if (trap == INTERRUPT_MACHINE_CHECK ||
- trap == INTERRUPT_DATA_STORAGE ||
- trap == INTERRUPT_ALIGNMENT) {
+ if (interrupt_detail_printable(TRAP(regs))) {
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
else
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2] powerpc/kconfig: Restore alphabetic order of the selects under CONFIG_PPC
From: Michael Ellerman @ 2021-04-22 13:57 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <361ee3fc5009c709ae0ca592249bb0702c6ef073.1619024780.git.christophe.leroy@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Commit a7d2475af7ae ("powerpc: Sort the selects under CONFIG_PPC")
> sorted all selects under CONFIG_PPC.
>
> 4 years later, several items have been introduced at wrong place,
> a few other have been renamed without moving them to their correct
> place.
>
> Reorder them now.
>
> While we are at it, simplify the test for a couple of them:
> - PPC_64 && PPC_PSERIES is simplified in PPC_PSERIES
> - PPC_64 && PPC_BOOK3S is simplified in PPC_BOOK3S_64
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v2: Rebased on d20f726744a0 ("Automatic merge of 'next' into merge (2021-04-21 22:57)")
This will conflict badly with other things in linux-next if I merge it
now.
The best time to do this is just before rc1. I'll try to remember :)
cheers
^ permalink raw reply
* Re: [PATCH 1/2] vfio/pci: remove vfio_pci_nvlink2
From: Jason Gunthorpe @ 2021-04-22 13:52 UTC (permalink / raw)
To: Michael Ellerman
Cc: kvm, David Airlie, linux-kernel, dri-devel, Alex Williamson,
Paul Mackerras, Daniel Vetter, Greg Kroah-Hartman, linux-api,
linuxppc-dev, Christoph Hellwig
In-Reply-To: <87h7jybf9w.fsf@mpe.ellerman.id.au>
On Thu, Apr 22, 2021 at 11:49:31PM +1000, Michael Ellerman wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> > On Mon, 12 Apr 2021 19:41:41 +1000
> > Michael Ellerman <mpe@ellerman.id.au> wrote:
> >
> >> Alex Williamson <alex.williamson@redhat.com> writes:
> >> > On Fri, 26 Mar 2021 07:13:10 +0100
> >> > Christoph Hellwig <hch@lst.de> wrote:
> >> >
> >> >> This driver never had any open userspace (which for VFIO would include
> >> >> VM kernel drivers) that use it, and thus should never have been added
> >> >> by our normal userspace ABI rules.
> >> >>
> >> >> Signed-off-by: Christoph Hellwig <hch@lst.de>
> >> >> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> >> drivers/vfio/pci/Kconfig | 6 -
> >> >> drivers/vfio/pci/Makefile | 1 -
> >> >> drivers/vfio/pci/vfio_pci.c | 18 -
> >> >> drivers/vfio/pci/vfio_pci_nvlink2.c | 490 ----------------------------
> >> >> drivers/vfio/pci/vfio_pci_private.h | 14 -
> >> >> include/uapi/linux/vfio.h | 38 +--
> >> >> 6 files changed, 4 insertions(+), 563 deletions(-)
> >> >> delete mode 100644 drivers/vfio/pci/vfio_pci_nvlink2.c
> >> >
> >> > Hearing no objections, applied to vfio next branch for v5.13. Thanks,
> >>
> >> Looks like you only took patch 1?
> >>
> >> I can't take patch 2 on its own, that would break the build.
> >>
> >> Do you want to take both patches? There's currently no conflicts against
> >> my tree. It's possible one could appear before the v5.13 merge window,
> >> though it would probably just be something minor.
> >>
> >> Or I could apply both patches to my tree, which means patch 1 would
> >> appear as two commits in the git history, but that's not a big deal.
> >
> > I've already got a conflict in my next branch with patch 1, so it's
> > best to go through my tree. Seems like a shared branch would be
> > easiest to allow you to merge and manage potential conflicts against
> > patch 2, I've pushed a branch here:
> >
> > https://github.com/awilliam/linux-vfio.git v5.13/vfio/nvlink
>
> Thanks.
>
> My next is based on rc2, so I won't pull that in directly, because I
> don't want to pull all of rc6 in with it.
Linus is fine if you merge in rc's for development reasons. He doesn't
like it when people just merge rc's without a purpose.
Merge rc7 to your tree then pull the nvlink topic is acceptable.
Or just do nothing because Alex will send it through his tree - this
extra co-ordination is really only necessary if there are conflicts.
Jason
^ permalink raw reply
* Re: [PATCH 1/2] vfio/pci: remove vfio_pci_nvlink2
From: Michael Ellerman @ 2021-04-22 13:49 UTC (permalink / raw)
To: Alex Williamson
Cc: Jason Gunthorpe, kvm, David Airlie, linux-kernel, dri-devel,
Paul Mackerras, Daniel Vetter, Greg Kroah-Hartman, linux-api,
linuxppc-dev, Christoph Hellwig
In-Reply-To: <20210412082304.5e7c0a80@omen>
Alex Williamson <alex.williamson@redhat.com> writes:
> On Mon, 12 Apr 2021 19:41:41 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> Alex Williamson <alex.williamson@redhat.com> writes:
>> > On Fri, 26 Mar 2021 07:13:10 +0100
>> > Christoph Hellwig <hch@lst.de> wrote:
>> >
>> >> This driver never had any open userspace (which for VFIO would include
>> >> VM kernel drivers) that use it, and thus should never have been added
>> >> by our normal userspace ABI rules.
>> >>
>> >> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> >> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >> ---
>> >> drivers/vfio/pci/Kconfig | 6 -
>> >> drivers/vfio/pci/Makefile | 1 -
>> >> drivers/vfio/pci/vfio_pci.c | 18 -
>> >> drivers/vfio/pci/vfio_pci_nvlink2.c | 490 ----------------------------
>> >> drivers/vfio/pci/vfio_pci_private.h | 14 -
>> >> include/uapi/linux/vfio.h | 38 +--
>> >> 6 files changed, 4 insertions(+), 563 deletions(-)
>> >> delete mode 100644 drivers/vfio/pci/vfio_pci_nvlink2.c
>> >
>> > Hearing no objections, applied to vfio next branch for v5.13. Thanks,
>>
>> Looks like you only took patch 1?
>>
>> I can't take patch 2 on its own, that would break the build.
>>
>> Do you want to take both patches? There's currently no conflicts against
>> my tree. It's possible one could appear before the v5.13 merge window,
>> though it would probably just be something minor.
>>
>> Or I could apply both patches to my tree, which means patch 1 would
>> appear as two commits in the git history, but that's not a big deal.
>
> I've already got a conflict in my next branch with patch 1, so it's
> best to go through my tree. Seems like a shared branch would be
> easiest to allow you to merge and manage potential conflicts against
> patch 2, I've pushed a branch here:
>
> https://github.com/awilliam/linux-vfio.git v5.13/vfio/nvlink
Thanks.
My next is based on rc2, so I won't pull that in directly, because I
don't want to pull all of rc6 in with it.
I'll put it in a topic branch and merge it into my next after my first
pull has gone to Linus.
cheers
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/sstep: Add emulation support for ‘setb’ instruction
From: Naveen N. Rao @ 2021-04-22 10:01 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev, Michael Ellerman, Sathvika Vasireddy
In-Reply-To: <87lf9caycg.fsf@mpe.ellerman.id.au>
Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> writes:
>> Daniel Axtens wrote:
>>> Sathvika Vasireddy <sathvika@linux.vnet.ibm.com> writes:
>>>
>>>> This adds emulation support for the following instruction:
>>>> * Set Boolean (setb)
>>>>
>>>> Signed-off-by: Sathvika Vasireddy <sathvika@linux.vnet.ibm.com>
> ...
>>>
>>> If you do end up respinning the patch, I think it would be good to make
>>> the maths a bit clearer. I think it works because a left shift of 2 is
>>> the same as multiplying by 4, but it would be easier to follow if you
>>> used a temporary variable for btf.
>>
>> Indeed. I wonder if it is better to follow the ISA itself. Per the ISA,
>> the bit we are interested in is:
>> 4 x BFA + 32
>>
>> So, if we use that along with the PPC_BIT() macro, we get:
>> if (regs->ccr & PPC_BIT(ra + 32))
>
> Use of PPC_BIT risks annoying your maintainer :)
Uh oh... that isn't good :)
I looked up previous discussions and I think I now understand why you
don't prefer it.
But, I feel it helps make it easy to follow the code when referring to
the ISA. I'm wondering if it is just the name you dislike and if so,
does it make sense to rename PPC_BIT() to something else? We have
BIT_ULL(), so perhaps BIT_MSB_ULL() or MSB_BIT_ULL()?
- Naveen
^ permalink raw reply
* [PATCH kernel] powerpc/makefile: Do not redefine $(CPP) for preprocessor
From: Alexey Kardashevskiy @ 2021-04-22 9:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy
The $(CPP) (do only preprocessing) macro is already defined in Makefile.
However POWERPC redefines it and adds $(KBUILD_CFLAGS) which results
in flags duplication. Which is not a big deal by itself except for
the flags which depend on other flags and the compiler checks them
as it parses the command line.
Specifically, scripts/Makefile.build:304 generates ksyms for .S files.
If clang+llvm+sanitizer are enabled, this results in
-fno-lto -flto -fsanitize=cfi-mfcall .... -fno-lto -flto -fsanitize=cfi-mfcall
in the clang command line and triggers error:
clang-13: error: invalid argument '-fsanitize=cfi-mfcall' only allowed with '-flto'
This removes unnecessary CPP redifinition.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/Makefile | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index c9d2c7825cd6..3a2f2001c62b 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -214,7 +214,6 @@ KBUILD_CPPFLAGS += -I $(srctree)/arch/$(ARCH) $(asinstr)
KBUILD_AFLAGS += $(AFLAGS-y)
KBUILD_CFLAGS += $(call cc-option,-msoft-float)
KBUILD_CFLAGS += -pipe $(CFLAGS-y)
-CPP = $(CC) -E $(KBUILD_CFLAGS)
CHECKFLAGS += -m$(BITS) -D__powerpc__ -D__powerpc$(BITS)__
ifdef CONFIG_CPU_BIG_ENDIAN
--
2.25.1
^ permalink raw reply related
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Dan Carpenter @ 2021-04-22 9:34 UTC (permalink / raw)
To: David Laight
Cc: devicetree@vger.kernel.org, kbuild-all@lists.01.org,
lkp@intel.com, robh@kernel.org, Lakshmi Ramasubramanian,
linuxppc-dev, bauerman@linux.ibm.com, 'Daniel Axtens'
In-Reply-To: <3e6b31d92d5042d982daeb989e49299e@AcuMS.aculab.com>
On Thu, Apr 22, 2021 at 08:05:27AM +0000, David Laight wrote:
> From: Daniel Axtens
> > Sent: 22 April 2021 03:21
> >
> > > Hi Lakshmi,
> > >
> > >> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> > >>
> > >> Sorry - missed copying device-tree and powerpc mailing lists.
> > >>
> > >>> There are a few "goto out;" statements before the local variable "fdt"
> > >>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> > >>> elf64_load(). This will result in an uninitialized "fdt" being passed
> > >>> to kvfree() in this function if there is an error before the call to
> > >>> of_kexec_alloc_and_setup_fdt().
> > >>>
> > >>> Initialize the local variable "fdt" to NULL.
> > >>>
> > > I'm a huge fan of initialising local variables! But I'm struggling to
> > > find the code path that will lead to an uninit fdt being returned...
> >
> > OK, so perhaps this was putting it too strongly. I have been bitten
> > by uninitialised things enough in C that I may have taken a slightly
> > overly-agressive view of fixing them in the source rather than the
> > compiler. I do think compiler-level mitigations are better, and I take
> > the point that we don't want to defeat compiler checking.
> >
> > (Does anyone - and by anyone I mean any large distro - compile with
> > local variables inited by the compiler?)
>
> There are compilers that initialise locals to zero for 'debug' builds
> and leave the 'random' for optimised 'release' builds.
> Lets not test what we are releasing!
We're eventually going to move to a world where initializing to zero
it the default for the kernel. I think people will still want to
initialize to a poison value for debug builds.
Initializing to zero is better for debugging because it's more
predictable. An it avoid information leaks. And dereferencing random
uninitialized pointers is a privilege escalation but dereferencing a
NULL is just an Oops.
The speed impact is not very significant because (conceptually) it only
needs to be done where there is a compiler warning about uninitialized
variables. It's slightly more complicated in real life. In this case,
the compiler doesn't know what happens inside the kexec_build_elf_info()
function so it silences the warning. And GCC silences warnings if the
variable is initialized inside a loop even when it doesn't know that we
enter the loop.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH v4 00/14] Restricted DMA
From: Claire Chang @ 2021-04-22 8:20 UTC (permalink / raw)
To: Rob Herring, mpe, Joerg Roedel, Will Deacon, Frank Rowand,
Konrad Rzeszutek Wilk, boris.ostrovsky, jgross, Christoph Hellwig,
Marek Szyprowski
Cc: heikki.krogerus, peterz, grant.likely, paulus, mingo, sstabellini,
Saravana Kannan, xypron.glpk, Rafael J . Wysocki,
Bartosz Golaszewski, xen-devel, Thierry Reding, linux-devicetree,
linuxppc-dev, Nicolas Boichat, Dan Williams, Andy Shevchenko,
Greg KH, Randy Dunlap, lkml, list@263.net:IOMMU DRIVERS,
Jim Quinlan, Robin Murphy, bauerman
In-Reply-To: <20210209062131.2300005-1-tientzu@chromium.org>
v5 here: https://lore.kernel.org/patchwork/cover/1416899/ to rebase
onto Christoph's swiotlb cleanups.
^ permalink raw reply
* [PATCH v5 16/16] of: Add plumbing for restricted DMA pool
From: Claire Chang @ 2021-04-22 8:15 UTC (permalink / raw)
To: Joerg Roedel, Will Deacon, Frank Rowand, Konrad Rzeszutek Wilk,
boris.ostrovsky, jgross, Christoph Hellwig, Marek Szyprowski
Cc: heikki.krogerus, thomas.hellstrom, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, mingo, jxgao, sstabellini,
Saravana Kannan, xypron.glpk, Rafael J . Wysocki,
Bartosz Golaszewski, bskeggs, linux-pci, xen-devel,
Thierry Reding, intel-gfx, matthew.auld, linux-devicetree, daniel,
airlied, maarten.lankhorst, linuxppc-dev, jani.nikula,
Nicolas Boichat, rodrigo.vivi, bhelgaas, tientzu, Dan Williams,
Andy Shevchenko, nouveau, Greg KH, Randy Dunlap, lkml, tfiga,
list@263.net:IOMMU DRIVERS, Jim Quinlan, Robin Murphy, bauerman
In-Reply-To: <20210422081508.3942748-1-tientzu@chromium.org>
If a device is not behind an IOMMU, we look up the device node and set
up the restricted DMA when the restricted-dma-pool is presented.
Signed-off-by: Claire Chang <tientzu@chromium.org>
---
drivers/of/address.c | 25 +++++++++++++++++++++++++
drivers/of/device.c | 3 +++
drivers/of/of_private.h | 5 +++++
3 files changed, 33 insertions(+)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 54f221dde267..fff3adfe4986 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -8,6 +8,7 @@
#include <linux/logic_pio.h>
#include <linux/module.h>
#include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
#include <linux/pci.h>
#include <linux/pci_regs.h>
#include <linux/sizes.h>
@@ -1109,6 +1110,30 @@ bool of_dma_is_coherent(struct device_node *np)
}
EXPORT_SYMBOL_GPL(of_dma_is_coherent);
+int of_dma_set_restricted_buffer(struct device *dev)
+{
+ struct device_node *node;
+ int count, i;
+
+ if (!dev->of_node)
+ return 0;
+
+ count = of_property_count_elems_of_size(dev->of_node, "memory-region",
+ sizeof(phandle));
+ for (i = 0; i < count; i++) {
+ node = of_parse_phandle(dev->of_node, "memory-region", i);
+ /* There might be multiple memory regions, but only one
+ * restriced-dma-pool region is allowed.
+ */
+ if (of_device_is_compatible(node, "restricted-dma-pool") &&
+ of_device_is_available(node))
+ return of_reserved_mem_device_init_by_idx(
+ dev, dev->of_node, i);
+ }
+
+ return 0;
+}
+
/**
* of_mmio_is_nonposted - Check if device uses non-posted MMIO
* @np: device node
diff --git a/drivers/of/device.c b/drivers/of/device.c
index c5a9473a5fb1..d8d865223e51 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -165,6 +165,9 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
+ if (!iommu)
+ return of_dma_set_restricted_buffer(dev);
+
return 0;
}
EXPORT_SYMBOL_GPL(of_dma_configure_id);
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index d717efbd637d..e9237f5eff48 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -163,12 +163,17 @@ struct bus_dma_region;
#if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA)
int of_dma_get_range(struct device_node *np,
const struct bus_dma_region **map);
+int of_dma_set_restricted_buffer(struct device *dev);
#else
static inline int of_dma_get_range(struct device_node *np,
const struct bus_dma_region **map)
{
return -ENODEV;
}
+static inline int of_dma_get_restricted_buffer(struct device *dev)
+{
+ return -ENODEV;
+}
#endif
#endif /* _LINUX_OF_PRIVATE_H */
--
2.31.1.368.gbe11c130af-goog
^ permalink raw reply related
* [PATCH v5 15/16] dt-bindings: of: Add restricted DMA pool
From: Claire Chang @ 2021-04-22 8:15 UTC (permalink / raw)
To: Joerg Roedel, Will Deacon, Frank Rowand, Konrad Rzeszutek Wilk,
boris.ostrovsky, jgross, Christoph Hellwig, Marek Szyprowski
Cc: heikki.krogerus, thomas.hellstrom, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, mingo, jxgao, sstabellini,
Saravana Kannan, xypron.glpk, Rafael J . Wysocki,
Bartosz Golaszewski, bskeggs, linux-pci, xen-devel,
Thierry Reding, intel-gfx, matthew.auld, linux-devicetree, daniel,
airlied, maarten.lankhorst, linuxppc-dev, jani.nikula,
Nicolas Boichat, rodrigo.vivi, bhelgaas, tientzu, Dan Williams,
Andy Shevchenko, nouveau, Greg KH, Randy Dunlap, lkml, tfiga,
list@263.net:IOMMU DRIVERS, Jim Quinlan, Robin Murphy, bauerman
In-Reply-To: <20210422081508.3942748-1-tientzu@chromium.org>
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the reserved-memory node.
Signed-off-by: Claire Chang <tientzu@chromium.org>
---
.../reserved-memory/reserved-memory.txt | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
index e8d3096d922c..fc9a12c2f679 100644
--- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
@@ -51,6 +51,20 @@ compatible (optional) - standard definition
used as a shared pool of DMA buffers for a set of devices. It can
be used by an operating system to instantiate the necessary pool
management subsystem if necessary.
+ - restricted-dma-pool: This indicates a region of memory meant to be
+ used as a pool of restricted DMA buffers for a set of devices. The
+ memory region would be the only region accessible to those devices.
+ When using this, the no-map and reusable properties must not be set,
+ so the operating system can create a virtual mapping that will be used
+ for synchronization. The main purpose for restricted DMA is to
+ mitigate the lack of DMA access control on systems without an IOMMU,
+ which could result in the DMA accessing the system memory at
+ unexpected times and/or unexpected addresses, possibly leading to data
+ leakage or corruption. The feature on its own provides a basic level
+ of protection against the DMA overwriting buffer contents at
+ unexpected times. However, to protect against general data leakage and
+ system memory corruption, the system needs to provide way to lock down
+ the memory access, e.g., MPU.
- vendor specific string in the form <vendor>,[<device>-]<usage>
no-map (optional) - empty property
- Indicates the operating system must not create a virtual mapping
@@ -120,6 +134,11 @@ one for multimedia processing (named multimedia-memory@77000000, 64MiB).
compatible = "acme,multimedia-memory";
reg = <0x77000000 0x4000000>;
};
+
+ restricted_dma_mem_reserved: restricted_dma_mem_reserved {
+ compatible = "restricted-dma-pool";
+ reg = <0x50000000 0x400000>;
+ };
};
/* ... */
@@ -138,4 +157,9 @@ one for multimedia processing (named multimedia-memory@77000000, 64MiB).
memory-region = <&multimedia_reserved>;
/* ... */
};
+
+ pcie_device: pcie_device@0,0 {
+ memory-region = <&restricted_dma_mem_reserved>;
+ /* ... */
+ };
};
--
2.31.1.368.gbe11c130af-goog
^ permalink raw reply related
* [PATCH v5 14/16] dma-direct: Allocate memory from restricted DMA pool if available
From: Claire Chang @ 2021-04-22 8:15 UTC (permalink / raw)
To: Joerg Roedel, Will Deacon, Frank Rowand, Konrad Rzeszutek Wilk,
boris.ostrovsky, jgross, Christoph Hellwig, Marek Szyprowski
Cc: heikki.krogerus, thomas.hellstrom, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, mingo, jxgao, sstabellini,
Saravana Kannan, xypron.glpk, Rafael J . Wysocki,
Bartosz Golaszewski, bskeggs, linux-pci, xen-devel,
Thierry Reding, intel-gfx, matthew.auld, linux-devicetree, daniel,
airlied, maarten.lankhorst, linuxppc-dev, jani.nikula,
Nicolas Boichat, rodrigo.vivi, bhelgaas, tientzu, Dan Williams,
Andy Shevchenko, nouveau, Greg KH, Randy Dunlap, lkml, tfiga,
list@263.net:IOMMU DRIVERS, Jim Quinlan, Robin Murphy, bauerman
In-Reply-To: <20210422081508.3942748-1-tientzu@chromium.org>
The restricted DMA pool is preferred if available.
The restricted DMA pools provide a basic level of protection against the
DMA overwriting buffer contents at unexpected times. However, to protect
against general data leakage and system memory corruption, the system
needs to provide a way to lock down the memory access, e.g., MPU.
Signed-off-by: Claire Chang <tientzu@chromium.org>
---
kernel/dma/direct.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 7a27f0510fcc..29523d2a9845 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -78,6 +78,10 @@ static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
static void __dma_direct_free_pages(struct device *dev, struct page *page,
size_t size)
{
+#ifdef CONFIG_DMA_RESTRICTED_POOL
+ if (swiotlb_free(dev, page, size))
+ return;
+#endif
dma_free_contiguous(dev, page, size);
}
@@ -92,7 +96,17 @@ static struct page *__dma_direct_alloc_pages(struct device *dev, size_t size,
gfp |= dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask,
&phys_limit);
- page = dma_alloc_contiguous(dev, size, gfp);
+
+#ifdef CONFIG_DMA_RESTRICTED_POOL
+ page = swiotlb_alloc(dev, size);
+ if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
+ __dma_direct_free_pages(dev, page, size);
+ page = NULL;
+ }
+#endif
+
+ if (!page)
+ page = dma_alloc_contiguous(dev, size, gfp);
if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
dma_free_contiguous(dev, page, size);
page = NULL;
@@ -148,7 +162,7 @@ void *dma_direct_alloc(struct device *dev, size_t size,
gfp |= __GFP_NOWARN;
if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
- !force_dma_unencrypted(dev)) {
+ !force_dma_unencrypted(dev) && !is_dev_swiotlb_force(dev)) {
page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO);
if (!page)
return NULL;
@@ -161,8 +175,8 @@ void *dma_direct_alloc(struct device *dev, size_t size,
}
if (!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED) &&
- !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
- !dev_is_dma_coherent(dev))
+ !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) && !dev_is_dma_coherent(dev) &&
+ !is_dev_swiotlb_force(dev))
return arch_dma_alloc(dev, size, dma_handle, gfp, attrs);
/*
@@ -172,7 +186,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
if (IS_ENABLED(CONFIG_DMA_COHERENT_POOL) &&
!gfpflags_allow_blocking(gfp) &&
(force_dma_unencrypted(dev) ||
- (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) && !dev_is_dma_coherent(dev))))
+ (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
+ !dev_is_dma_coherent(dev))) &&
+ !is_dev_swiotlb_force(dev))
return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
/* we always manually zero the memory once we are done */
@@ -253,15 +269,15 @@ void dma_direct_free(struct device *dev, size_t size,
unsigned int page_order = get_order(size);
if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
- !force_dma_unencrypted(dev)) {
+ !force_dma_unencrypted(dev) && !is_dev_swiotlb_force(dev)) {
/* cpu_addr is a struct page cookie, not a kernel address */
dma_free_contiguous(dev, cpu_addr, size);
return;
}
if (!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED) &&
- !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
- !dev_is_dma_coherent(dev)) {
+ !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) && !dev_is_dma_coherent(dev) &&
+ !is_dev_swiotlb_force(dev)) {
arch_dma_free(dev, size, cpu_addr, dma_addr, attrs);
return;
}
@@ -289,7 +305,8 @@ struct page *dma_direct_alloc_pages(struct device *dev, size_t size,
void *ret;
if (IS_ENABLED(CONFIG_DMA_COHERENT_POOL) &&
- force_dma_unencrypted(dev) && !gfpflags_allow_blocking(gfp))
+ force_dma_unencrypted(dev) && !gfpflags_allow_blocking(gfp) &&
+ !is_dev_swiotlb_force(dev))
return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
page = __dma_direct_alloc_pages(dev, size, gfp);
--
2.31.1.368.gbe11c130af-goog
^ permalink raw reply related
* [PATCH v5 13/16] swiotlb: Add restricted DMA alloc/free support.
From: Claire Chang @ 2021-04-22 8:15 UTC (permalink / raw)
To: Joerg Roedel, Will Deacon, Frank Rowand, Konrad Rzeszutek Wilk,
boris.ostrovsky, jgross, Christoph Hellwig, Marek Szyprowski
Cc: heikki.krogerus, thomas.hellstrom, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, mingo, jxgao, sstabellini,
Saravana Kannan, xypron.glpk, Rafael J . Wysocki,
Bartosz Golaszewski, bskeggs, linux-pci, xen-devel,
Thierry Reding, intel-gfx, matthew.auld, linux-devicetree, daniel,
airlied, maarten.lankhorst, linuxppc-dev, jani.nikula,
Nicolas Boichat, rodrigo.vivi, bhelgaas, tientzu, Dan Williams,
Andy Shevchenko, nouveau, Greg KH, Randy Dunlap, lkml, tfiga,
list@263.net:IOMMU DRIVERS, Jim Quinlan, Robin Murphy, bauerman
In-Reply-To: <20210422081508.3942748-1-tientzu@chromium.org>
Add the functions, swiotlb_{alloc,free} to support the memory allocation
from restricted DMA pool.
Signed-off-by: Claire Chang <tientzu@chromium.org>
---
include/linux/swiotlb.h | 4 ++++
kernel/dma/swiotlb.c | 35 +++++++++++++++++++++++++++++++++--
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 0c5a18d9cf89..e8cf49bd90c5 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -134,6 +134,10 @@ unsigned int swiotlb_max_segment(void);
size_t swiotlb_max_mapping_size(struct device *dev);
bool is_swiotlb_active(struct device *dev);
void __init swiotlb_adjust_size(unsigned long size);
+#ifdef CONFIG_DMA_RESTRICTED_POOL
+struct page *swiotlb_alloc(struct device *dev, size_t size);
+bool swiotlb_free(struct device *dev, struct page *page, size_t size);
+#endif /* CONFIG_DMA_RESTRICTED_POOL */
#else
#define swiotlb_force SWIOTLB_NO_FORCE
static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index af0feb8eaead..274272c79080 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -454,8 +454,9 @@ static int find_slots(struct device *dev, phys_addr_t orig_addr,
index = wrap = wrap_index(mem, ALIGN(mem->index, stride));
do {
- if ((slot_addr(tbl_dma_addr, index) & iotlb_align_mask) !=
- (orig_addr & iotlb_align_mask)) {
+ if (orig_addr &&
+ (slot_addr(tbl_dma_addr, index) & iotlb_align_mask) !=
+ (orig_addr & iotlb_align_mask)) {
index = wrap_index(mem, index + 1);
continue;
}
@@ -695,6 +696,36 @@ late_initcall(swiotlb_create_default_debugfs);
#endif
#ifdef CONFIG_DMA_RESTRICTED_POOL
+struct page *swiotlb_alloc(struct device *dev, size_t size)
+{
+ struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
+ phys_addr_t tlb_addr;
+ int index;
+
+ if (!mem)
+ return NULL;
+
+ index = find_slots(dev, 0, size);
+ if (index == -1)
+ return NULL;
+
+ tlb_addr = slot_addr(mem->start, index);
+
+ return pfn_to_page(PFN_DOWN(tlb_addr));
+}
+
+bool swiotlb_free(struct device *dev, struct page *page, size_t size)
+{
+ phys_addr_t tlb_addr = page_to_phys(page);
+
+ if (!is_swiotlb_buffer(dev, tlb_addr))
+ return false;
+
+ release_slots(dev, tlb_addr);
+
+ return true;
+}
+
static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
struct device *dev)
{
--
2.31.1.368.gbe11c130af-goog
^ permalink raw reply related
* [PATCH v5 12/16] dma-direct: Add a new wrapper __dma_direct_free_pages()
From: Claire Chang @ 2021-04-22 8:15 UTC (permalink / raw)
To: Joerg Roedel, Will Deacon, Frank Rowand, Konrad Rzeszutek Wilk,
boris.ostrovsky, jgross, Christoph Hellwig, Marek Szyprowski
Cc: heikki.krogerus, thomas.hellstrom, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, mingo, jxgao, sstabellini,
Saravana Kannan, xypron.glpk, Rafael J . Wysocki,
Bartosz Golaszewski, bskeggs, linux-pci, xen-devel,
Thierry Reding, intel-gfx, matthew.auld, linux-devicetree, daniel,
airlied, maarten.lankhorst, linuxppc-dev, jani.nikula,
Nicolas Boichat, rodrigo.vivi, bhelgaas, tientzu, Dan Williams,
Andy Shevchenko, nouveau, Greg KH, Randy Dunlap, lkml, tfiga,
list@263.net:IOMMU DRIVERS, Jim Quinlan, Robin Murphy, bauerman
In-Reply-To: <20210422081508.3942748-1-tientzu@chromium.org>
Add a new wrapper __dma_direct_free_pages() that will be useful later
for swiotlb_free().
Signed-off-by: Claire Chang <tientzu@chromium.org>
---
kernel/dma/direct.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 7a88c34d0867..7a27f0510fcc 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -75,6 +75,12 @@ static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
min_not_zero(dev->coherent_dma_mask, dev->bus_dma_limit);
}
+static void __dma_direct_free_pages(struct device *dev, struct page *page,
+ size_t size)
+{
+ dma_free_contiguous(dev, page, size);
+}
+
static struct page *__dma_direct_alloc_pages(struct device *dev, size_t size,
gfp_t gfp)
{
@@ -237,7 +243,7 @@ void *dma_direct_alloc(struct device *dev, size_t size,
return NULL;
}
out_free_pages:
- dma_free_contiguous(dev, page, size);
+ __dma_direct_free_pages(dev, page, size);
return NULL;
}
@@ -273,7 +279,7 @@ void dma_direct_free(struct device *dev, size_t size,
else if (IS_ENABLED(CONFIG_ARCH_HAS_DMA_CLEAR_UNCACHED))
arch_dma_clear_uncached(cpu_addr, size);
- dma_free_contiguous(dev, dma_direct_to_page(dev, dma_addr), size);
+ __dma_direct_free_pages(dev, dma_direct_to_page(dev, dma_addr), size);
}
struct page *dma_direct_alloc_pages(struct device *dev, size_t size,
@@ -310,7 +316,7 @@ struct page *dma_direct_alloc_pages(struct device *dev, size_t size,
*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
return page;
out_free_pages:
- dma_free_contiguous(dev, page, size);
+ __dma_direct_free_pages(dev, page, size);
return NULL;
}
@@ -329,7 +335,7 @@ void dma_direct_free_pages(struct device *dev, size_t size,
if (force_dma_unencrypted(dev))
set_memory_encrypted((unsigned long)vaddr, 1 << page_order);
- dma_free_contiguous(dev, page, size);
+ __dma_direct_free_pages(dev, page, size);
}
#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
--
2.31.1.368.gbe11c130af-goog
^ permalink raw reply related
* [PATCH v5 11/16] swiotlb: Refactor swiotlb_tbl_unmap_single
From: Claire Chang @ 2021-04-22 8:15 UTC (permalink / raw)
To: Joerg Roedel, Will Deacon, Frank Rowand, Konrad Rzeszutek Wilk,
boris.ostrovsky, jgross, Christoph Hellwig, Marek Szyprowski
Cc: heikki.krogerus, thomas.hellstrom, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, mingo, jxgao, sstabellini,
Saravana Kannan, xypron.glpk, Rafael J . Wysocki,
Bartosz Golaszewski, bskeggs, linux-pci, xen-devel,
Thierry Reding, intel-gfx, matthew.auld, linux-devicetree, daniel,
airlied, maarten.lankhorst, linuxppc-dev, jani.nikula,
Nicolas Boichat, rodrigo.vivi, bhelgaas, tientzu, Dan Williams,
Andy Shevchenko, nouveau, Greg KH, Randy Dunlap, lkml, tfiga,
list@263.net:IOMMU DRIVERS, Jim Quinlan, Robin Murphy, bauerman
In-Reply-To: <20210422081508.3942748-1-tientzu@chromium.org>
Add a new function, release_slots, to make the code reusable for supporting
different bounce buffer pools, e.g. restricted DMA pool.
Signed-off-by: Claire Chang <tientzu@chromium.org>
---
kernel/dma/swiotlb.c | 35 ++++++++++++++++++++---------------
1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index b7d634d7a7eb..af0feb8eaead 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -547,27 +547,15 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
return tlb_addr;
}
-/*
- * tlb_addr is the physical address of the bounce buffer to unmap.
- */
-void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
- size_t mapping_size, enum dma_data_direction dir,
- unsigned long attrs)
+static void release_slots(struct device *dev, phys_addr_t tlb_addr)
{
- struct io_tlb_mem *mem = get_io_tlb_mem(hwdev);
+ struct io_tlb_mem *mem = get_io_tlb_mem(dev);
unsigned long flags;
- unsigned int offset = swiotlb_align_offset(hwdev, tlb_addr);
+ unsigned int offset = swiotlb_align_offset(dev, tlb_addr);
int index = (tlb_addr - offset - mem->start) >> IO_TLB_SHIFT;
int nslots = nr_slots(mem->slots[index].alloc_size + offset);
int count, i;
- /*
- * First, sync the memory before unmapping the entry
- */
- if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
- (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL))
- swiotlb_bounce(hwdev, tlb_addr, mapping_size, DMA_FROM_DEVICE);
-
/*
* Return the buffer to the free list by setting the corresponding
* entries to indicate the number of contiguous entries available.
@@ -602,6 +590,23 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
spin_unlock_irqrestore(&mem->lock, flags);
}
+/*
+ * tlb_addr is the physical address of the bounce buffer to unmap.
+ */
+void swiotlb_tbl_unmap_single(struct device *dev, phys_addr_t tlb_addr,
+ size_t mapping_size, enum dma_data_direction dir,
+ unsigned long attrs)
+{
+ /*
+ * First, sync the memory before unmapping the entry
+ */
+ if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
+ (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL))
+ swiotlb_bounce(dev, tlb_addr, mapping_size, DMA_FROM_DEVICE);
+
+ release_slots(dev, tlb_addr);
+}
+
void swiotlb_sync_single_for_device(struct device *dev, phys_addr_t tlb_addr,
size_t size, enum dma_data_direction dir)
{
--
2.31.1.368.gbe11c130af-goog
^ permalink raw reply related
* [PATCH v5 10/16] swiotlb: Move alloc_size to find_slots
From: Claire Chang @ 2021-04-22 8:15 UTC (permalink / raw)
To: Joerg Roedel, Will Deacon, Frank Rowand, Konrad Rzeszutek Wilk,
boris.ostrovsky, jgross, Christoph Hellwig, Marek Szyprowski
Cc: heikki.krogerus, thomas.hellstrom, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, mingo, jxgao, sstabellini,
Saravana Kannan, xypron.glpk, Rafael J . Wysocki,
Bartosz Golaszewski, bskeggs, linux-pci, xen-devel,
Thierry Reding, intel-gfx, matthew.auld, linux-devicetree, daniel,
airlied, maarten.lankhorst, linuxppc-dev, jani.nikula,
Nicolas Boichat, rodrigo.vivi, bhelgaas, tientzu, Dan Williams,
Andy Shevchenko, nouveau, Greg KH, Randy Dunlap, lkml, tfiga,
list@263.net:IOMMU DRIVERS, Jim Quinlan, Robin Murphy, bauerman
In-Reply-To: <20210422081508.3942748-1-tientzu@chromium.org>
Move the maintenance of alloc_size to find_slots for better code
reusability later.
Signed-off-by: Claire Chang <tientzu@chromium.org>
---
kernel/dma/swiotlb.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 96ff36d8ec53..b7d634d7a7eb 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -479,8 +479,11 @@ static int find_slots(struct device *dev, phys_addr_t orig_addr,
return -1;
found:
- for (i = index; i < index + nslots; i++)
+ for (i = index; i < index + nslots; i++) {
mem->slots[i].list = 0;
+ mem->slots[i].alloc_size =
+ alloc_size - ((i - index) << IO_TLB_SHIFT);
+ }
for (i = index - 1;
io_tlb_offset(i) != IO_TLB_SEGSIZE - 1 &&
mem->slots[i].list; i--)
@@ -535,11 +538,8 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
* This is needed when we sync the memory. Then we sync the buffer if
* needed.
*/
- for (i = 0; i < nr_slots(alloc_size + offset); i++) {
+ for (i = 0; i < nr_slots(alloc_size + offset); i++)
mem->slots[index + i].orig_addr = slot_addr(orig_addr, i);
- mem->slots[index + i].alloc_size =
- alloc_size - (i << IO_TLB_SHIFT);
- }
tlb_addr = slot_addr(mem->start, index) + offset;
if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
(dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL))
--
2.31.1.368.gbe11c130af-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox