* Re: Hijacking CPU_FTR_VSX for BGQ QPX
From: Jimi Xenidis @ 2012-12-05 15:44 UTC (permalink / raw)
To: Michael Neuling, Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <3948.1352522003@neuling.org>
Sorry for the pause, lots of other things getting done... questions =
below.
On Nov 9, 2012, at 10:33 PM, Michael Neuling <mikey@neuling.org> wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>=20
>> On Fri, 2012-11-09 at 11:43 -0600, Jimi Xenidis wrote:
>>> The CPU_FTR_* values are pretty tight (a few bits left) yes I need =
to save and restore the QPX registers.
>>> There are 32 QPX registers, each 32 bytes in size, it is otherwise =
managed by the FPSCR and MSR[FP]
>>>=20
>>> I was thinking that I could hijack the VSX, since there is no plan =
to add it to embedded yet.
>>> I could be explicit or create an alieas fo the same bit, but the =
basic effect (after increasing the save area size) would be something =
like the diff below.
>>> Thoughts?
>>=20
>> Don't. Use a different bit, we can always split the mask again if
>> needed, move more bits to mmu_features etc...
Ok
>>=20
>>> -#ifdef CONFIG_VSX
>>> +#if defined (CONFIG_VSX) && defined(CONFIG_BGQ)
>>> +# error "This code depends on CONFIG_VSX and CONFIG_BGQ being =
exclusive
>>> +#elif defined (CONFIG_VSX)
>>> +# define _REST_32VSRS(n,c,base) REST_32VSRS(n,c,base)
>>> +# define _SAVE_32VSRS(n,c,base) SAVE_32VSRS(n,c,base)
>>> +#elif defined(CONFIG_BGQ)
>>=20
>> Make a CONFIG_PPC_QPX or something like that specifically for the QPX
>> stuff that you can then "select" from CONFIG_PPC_BGQ (don't do just
>> CONFIG_BGQ).
ack
>>=20
>> And don't just "hijack" stuff like that, it should be a runtime =
option,
>> so add a new set etc... it should be possible to build a kernel that
>> boots on a BGQ or a hypothetical BookE chip with VSX.
ack
>=20
> Yeah both bluegene and VSX are designed for HPC, so it's not =
completely
> crazy that someone would put them together.
Not sure that is possible, since they both "include" FPU state, which is =
why hijacking the the FPU routines is so delicious.
-jx
>=20
> Also, we need to fix the CPU FTR issue. With PPR (Haren's stuff) and
> POWER8 we are going to blow CPU FTRs pretty soon anyway. This just =
adds
> to that.
>=20
> Mikey
>=20
>>=20
>>> +# define _REST_32VSRS(n,c,base) REST_32QRS(n,c,base)
>>> +# define _SAVE_32VSRS(n,c,base) SAVE_32QRS(n,c,base)
>>> +#endif
>>> +
>>> +#if defined (CONFIG_VSX) || defined(CONFIG_BGQ)
>>> #define REST_32FPVSRS(n,c,base) =
\
>>> BEGIN_FTR_SECTION =
\
>>> b 2f; =
\
>>> END_FTR_SECTION_IFSET(CPU_FTR_VSX); =
\
>>> REST_32FPRS(n,base); =
\
>>> b 3f; =
\
>>> -2: REST_32VSRS(n,c,base); =
\
>>> +2: _REST_32VSRS(n,c,base); =
\
>>> 3:
>>>=20
>>> #define SAVE_32FPVSRS(n,c,base) =
\
>>> @@ -41,7 +51,7 @@ BEGIN_FTR_SECTION =
\
>>> END_FTR_SECTION_IFSET(CPU_FTR_VSX); =
\
>>> SAVE_32FPRS(n,base); =
\
>>> b 3f; =
\
>>> -2: SAVE_32VSRS(n,c,base); =
\
>>> +2: _SAVE_32VSRS(n,c,base); =
\
>>> 3:
>>> #else
>>> #define REST_32FPVSRS(n,b,base) REST_32FPRS(n, base)
>>=20
>> Cheers,
>> Ben.
>>=20
>>=20
^ permalink raw reply
* Re: [linuxppc-release][PATCH] powerpc/pci-hotplug: fix init issue of rescanned pci device
From: Chen Yuanquan-B41889 @ 2012-12-05 9:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Bjorn Helgaas, linux-pci, linuxppc-dev, r61911
In-Reply-To: <1354695989.2351.13.camel@pasglop>
On 12/05/2012 04:26 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2012-12-05 at 16:20 +0800, Chen Yuanquan-B41889 wrote:
>> On 12/05/2012 03:17 PM, Benjamin Herrenschmidt wrote:
>>> On Wed, 2012-12-05 at 10:31 +0800, Yuanquan Chen wrote:
>>>> On powerpc arch, some fixup work of PCI/PCI-e device is just done during the
>>>> first scan at booting time. For the PCI/PCI-e device rescanned after linux OS
>>>> booting up, the fixup work won't be done, which leads to dma_set_mask error or
>>>> irq related issue in rescanned PCI/PCI-e device's driver. So, it does the same
>>>> fixup work for the rescanned device to avoid this issue.
>>> Hrm, the patch is a bit gross. First the code shouldn't be copy/pasted
>>> that way but factored out.
> Please, at least format your email properly so I can try to undertand
> without needing aspirin.
>
>> There's a judgement "if (!bus->is_added)" before calling of
>> pcibios_fixup_bus in pci_scan_child_bus, so for the rescanned device,
>> the fixup won't execute, which leads to fatal error in driver of rescanned
>> device on freescale powerpc, no this issues on x86 arch.
> First, none of that invalidates my statement that you shouldn't
> duplicate a whole block of code like this. Even if your approach is
> correct (which is debated separately), at the very least you should
> factor the code out into a common function between the two copies.
>
>> Remove the judgement, let it to do the pcibios_fixup_bus
>> directly, the error won't occur for the rescanned device. But it's
>> general code, not proper to change here, so copy the pcibios_fixup_bus
>> work to pcibios_enable_device.
>>
>>> I'm surprised also that is_added is false when pcibios_enable_device()
>>> gets called ... that looks strange to me. At what point is that enable
>>> happening in the hotplug sequence ?
>> All devices are rescanned and then call the pci_enable_devices and
>> pci_bus_add_devices.
> Where ? How ? What is the sequence happening ? In any case, I think if
> we need a proper fixup done per-device like that after scan we ought to
> create a new hook at the generic level rather than that sort of hack.
>
echo 1 > rescan to trigger dev_rescan_store:
dev_rescan_store->pci_rescan_bus->pci_scan_child_bus,
pci_assign_unassigned_bus_resources,
pci_enable_bridges, pci_bus_add_devices
pci_enable_bridges->pci_enable_device->__pci_enable_device_flags->do_pci_enable_device->
pcibios_enable_device
pci_bus_add_devices->pci_bus_add_device->"dev->is_added = 1"
Yeah, it's general fixup code for every rescanned PCI/PCI-e device on
powerpc at runtime. So if
we want to call it in a ppc_md member, we need to wrap it as a function
and assign it in every ppc_md,
it isn't proper for the general code.
Regards,
yuanquan
>> The patch code will be called by pci_enable_devices. The "dev->is_added"
>> is set in pci_bus_add_device
>> which is called by pci_bus_add_devices. So "dev->is_added" is false when
>> checking it in pcibios_enable_device
>> for the rescanned device.
> Who calls pci_enable_device() in the rescan case ? Why isn't it left to
> the driver ? I don't think we can rely on that behaviour not to change.
>
>>> How do you trigger the rescan anyway ?
>> Use the interface under /sys :
>> echo 1 > /sys/bus/pci/devices/xxx/remove
>>
>> then echo 1 to the pci device which is the bus of the removed device
>> echo 1 > /sys/bus/pci/devices/xxxx/rescan
>> the removed device will be scanned and it's driver module will be loaded
>> automatically.
> Yeah this code path are known to be fishy. I think the problem is at the
> generic abstraction level and that's where it needs to be fixed.
>
> Cheers,
> Ben.
>
>> Regards,
>> yuanquan
>>> I think the problem needs to be solve at a higher level, I'm adding
>>> linux-pci & Bjorn to the CC list.
>>>
>>> Cheers,
>>> Ben.
>>>
>>>> Signed-off-by: Yuanquan Chen <B41889@freescale.com>
>>>> ---
>>>> arch/powerpc/kernel/pci-common.c | 20 ++++++++++++++++++++
>>>> 1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>>>> index 7f94f76..f0fb070 100644
>>>> --- a/arch/powerpc/kernel/pci-common.c
>>>> +++ b/arch/powerpc/kernel/pci-common.c
>>>> @@ -1496,6 +1496,26 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>>>> if (ppc_md.pcibios_enable_device_hook(dev))
>>>> return -EINVAL;
>>>>
>>>> + if (!dev->is_added) {
>>>> + /*
>>>> + * Fixup NUMA node as it may not be setup yet by the generic
>>>> + * code and is needed by the DMA init
>>>> + */
>>>> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>>>> +
>>>> + /* Hook up default DMA ops */
>>>> + set_dma_ops(&dev->dev, pci_dma_ops);
>>>> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>>>> +
>>>> + /* Additional platform DMA/iommu setup */
>>>> + if (ppc_md.pci_dma_dev_setup)
>>>> + ppc_md.pci_dma_dev_setup(dev);
>>>> +
>>>> + /* Read default IRQs and fixup if necessary */
>>>> + pci_read_irq_line(dev);
>>>> + if (ppc_md.pci_irq_fixup)
>>>> + ppc_md.pci_irq_fixup(dev);
>>>> + }
>>>> return pci_enable_resources(dev, mask);
>>>> }
>>>>
>>>
>>>
>>>
>
>
>
>
^ permalink raw reply
* Re: [linuxppc-release][PATCH] powerpc/pci-hotplug: fix init issue of rescanned pci device
From: Benjamin Herrenschmidt @ 2012-12-05 8:26 UTC (permalink / raw)
To: Chen Yuanquan-B41889; +Cc: Bjorn Helgaas, linux-pci, linuxppc-dev, r61911
In-Reply-To: <50BF03C1.2070304@freescale.com>
On Wed, 2012-12-05 at 16:20 +0800, Chen Yuanquan-B41889 wrote:
> On 12/05/2012 03:17 PM, Benjamin Herrenschmidt wrote:
> > On Wed, 2012-12-05 at 10:31 +0800, Yuanquan Chen wrote:
> >> On powerpc arch, some fixup work of PCI/PCI-e device is just done during the
> >> first scan at booting time. For the PCI/PCI-e device rescanned after linux OS
> >> booting up, the fixup work won't be done, which leads to dma_set_mask error or
> >> irq related issue in rescanned PCI/PCI-e device's driver. So, it does the same
> >> fixup work for the rescanned device to avoid this issue.
> > Hrm, the patch is a bit gross. First the code shouldn't be copy/pasted
> > that way but factored out.
Please, at least format your email properly so I can try to undertand
without needing aspirin.
> There's a judgement "if (!bus->is_added)" before calling of
> pcibios_fixup_bus in pci_scan_child_bus, so for the rescanned device,
> the fixup won't execute, which leads to fatal error in driver of rescanned
> device on freescale powerpc, no this issues on x86 arch.
First, none of that invalidates my statement that you shouldn't
duplicate a whole block of code like this. Even if your approach is
correct (which is debated separately), at the very least you should
factor the code out into a common function between the two copies.
> Remove the judgement, let it to do the pcibios_fixup_bus
> directly, the error won't occur for the rescanned device. But it's
> general code, not proper to change here, so copy the pcibios_fixup_bus
> work to pcibios_enable_device.
>
> > I'm surprised also that is_added is false when pcibios_enable_device()
> > gets called ... that looks strange to me. At what point is that enable
> > happening in the hotplug sequence ?
> All devices are rescanned and then call the pci_enable_devices and
> pci_bus_add_devices.
Where ? How ? What is the sequence happening ? In any case, I think if
we need a proper fixup done per-device like that after scan we ought to
create a new hook at the generic level rather than that sort of hack.
> The patch code will be called by pci_enable_devices. The "dev->is_added"
> is set in pci_bus_add_device
> which is called by pci_bus_add_devices. So "dev->is_added" is false when
> checking it in pcibios_enable_device
> for the rescanned device.
Who calls pci_enable_device() in the rescan case ? Why isn't it left to
the driver ? I don't think we can rely on that behaviour not to change.
> > How do you trigger the rescan anyway ?
> Use the interface under /sys :
> echo 1 > /sys/bus/pci/devices/xxx/remove
>
> then echo 1 to the pci device which is the bus of the removed device
> echo 1 > /sys/bus/pci/devices/xxxx/rescan
> the removed device will be scanned and it's driver module will be loaded
> automatically.
Yeah this code path are known to be fishy. I think the problem is at the
generic abstraction level and that's where it needs to be fixed.
Cheers,
Ben.
> Regards,
> yuanquan
> > I think the problem needs to be solve at a higher level, I'm adding
> > linux-pci & Bjorn to the CC list.
> >
> > Cheers,
> > Ben.
> >
> >> Signed-off-by: Yuanquan Chen <B41889@freescale.com>
> >> ---
> >> arch/powerpc/kernel/pci-common.c | 20 ++++++++++++++++++++
> >> 1 file changed, 20 insertions(+)
> >>
> >> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> >> index 7f94f76..f0fb070 100644
> >> --- a/arch/powerpc/kernel/pci-common.c
> >> +++ b/arch/powerpc/kernel/pci-common.c
> >> @@ -1496,6 +1496,26 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> >> if (ppc_md.pcibios_enable_device_hook(dev))
> >> return -EINVAL;
> >>
> >> + if (!dev->is_added) {
> >> + /*
> >> + * Fixup NUMA node as it may not be setup yet by the generic
> >> + * code and is needed by the DMA init
> >> + */
> >> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
> >> +
> >> + /* Hook up default DMA ops */
> >> + set_dma_ops(&dev->dev, pci_dma_ops);
> >> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> >> +
> >> + /* Additional platform DMA/iommu setup */
> >> + if (ppc_md.pci_dma_dev_setup)
> >> + ppc_md.pci_dma_dev_setup(dev);
> >> +
> >> + /* Read default IRQs and fixup if necessary */
> >> + pci_read_irq_line(dev);
> >> + if (ppc_md.pci_irq_fixup)
> >> + ppc_md.pci_irq_fixup(dev);
> >> + }
> >> return pci_enable_resources(dev, mask);
> >> }
> >>
> >
> >
> >
> >
>
^ permalink raw reply
* Re: Understanding how kernel updates MMU hash table
From: Benjamin Herrenschmidt @ 2012-12-05 8:20 UTC (permalink / raw)
To: Pegasus11; +Cc: linuxppc-dev
In-Reply-To: <34760537.post@talk.nabble.com>
On Tue, 2012-12-04 at 21:56 -0800, Pegasus11 wrote:
> Hello.
>
> Ive been trying to understand how an hash PTE is updated. Im on a PPC970MP
> machine which using the IBM PowerPC 604e core.
Ah no, the 970 is a ... 970 core :-) It's a derivative of POWER4+ which
is quite different from the old 32-bit 604e.
> My Linux version is 2.6.10 (I
> am sorry I cannot migrate at the moment. Management issues and I can't help
> :-(( )
>
> Now onto the problem:
> hpte_update is invoked to sync the on-chip MMU cache which Linux uses as its
> TLB.
It's actually in-memory cache. There's also an on-chip TLB.
> So whenever a change is made to the PTE, it has to be propagated to the
> corresponding TLB entry. And this uses hpte_update for the same. Am I right
> here?
hpte_update takes care of tracking whether a Linux PTE was also cached
into the hash, in which case the hash is marked for invalidation. I
don't remember precisely how we did it in 2.6.10 but it's possible that
the actual invalidation of the hash and the corresponding TLB
invalidations are delayed.
> Now http://lxr.linux.no/linux-bk+*/+code=hpte_update hpte_update is
> declared as
>
> ' void hpte_update(pte_t *ptep, unsigned long pte, int wrprot) '.
> The arguments to this function is a POINTER to the PTE entry (needed to make
> a change persistent across function call right?), the PTE entry (as in the
> value) as well the wrprot flag.
>
> Now the code snippet thats bothering me is this:
> '
> 86 ptepage = virt_to_page(ptep);
> 87 mm = (struct mm_struct *) ptepage->mapping;
> 88 addr = ptepage->index +
> 89 (((unsigned long)ptep & ~PAGE_MASK) * PTRS_PER_PTE);
> '
>
> On line 86, we get the page structure for a given PTE but we pass the
> pointer to PTE not the PTE itself whereas virt_to_page is a macro defined
> as:
I don't remember why we did that in 2.6.10 however...
> #define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
>
> Why are passing the POINTER to pte here? I mean are we looking for the PAGE
> that is described by the PTE or are we looking for the PAGE which contains
> the pointer to PTE? Me things it is the later since the former is given by
> the VALUE of the PTE not its POINTER. Right?
The above gets the page that contains the PTEs indeed, in order to get
the associated mapping pointer which points to the struct mm_struct, and
the index, which together are used to re-constitute the virtual address,
probably in order to perform the actual invalidation. Nowadays, we just
pass the virtual address down from the call site.
> So if it indeed the later, what trickery are we here after? Perhaps
> following the snippet will make us understand? As I see from above, after
> that we get the 'address space object' associated with this page.
>
> What I don't understand is the following line:
> addr = ptepage->index + (((unsigned long)ptep & ~PAGE_MASK) *
> PTRS_PER_PTE);
>
> First we get the index of the page in the file i.e. the number of pages
> preceding the page which holds the address of PTEP. Then we get the lower 12
> bits of this page. Then we shift that these bits to the left by 12 again and
> to it we add the above index. What is this doing?
>
> There are other things in this function that I do not understand. I'd be
> glad if someone could give me a heads up on this.
It's gross, the point is to rebuild the virtual address. You should
*REALLY* update to a more recent kernel, that ancient code is broken in
many ways as far as I can tell.
Cheers,
Ben.
^ permalink raw reply
* Re: [linuxppc-release][PATCH] powerpc/pci-hotplug: fix init issue of rescanned pci device
From: Chen Yuanquan-B41889 @ 2012-12-05 8:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Bjorn Helgaas, linux-pci, linuxppc-dev, r61911
In-Reply-To: <1354691878.2351.2.camel@pasglop>
On 12/05/2012 03:17 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2012-12-05 at 10:31 +0800, Yuanquan Chen wrote:
>> On powerpc arch, some fixup work of PCI/PCI-e device is just done during the
>> first scan at booting time. For the PCI/PCI-e device rescanned after linux OS
>> booting up, the fixup work won't be done, which leads to dma_set_mask error or
>> irq related issue in rescanned PCI/PCI-e device's driver. So, it does the same
>> fixup work for the rescanned device to avoid this issue.
> Hrm, the patch is a bit gross. First the code shouldn't be copy/pasted
> that way but factored out.
There's a judgement "if (!bus->is_added)" before calling of
pcibios_fixup_bus
in pci_scan_child_bus, so for the rescanned device, the fixup won't execute,
which leads to fatal error in driver of rescanned device on freescale
powerpc, no
this issues on x86 arch. Remove the judgement, let it to do the
pcibios_fixup_bus
directly, the error won't occur for the rescanned device. But it's
general code, not
proper to change here, so copy the pcibios_fixup_bus work to
pcibios_enable_device.
> I'm surprised also that is_added is false when pcibios_enable_device()
> gets called ... that looks strange to me. At what point is that enable
> happening in the hotplug sequence ?
All devices are rescanned and then call the pci_enable_devices and
pci_bus_add_devices.
The patch code will be called by pci_enable_devices. The "dev->is_added"
is set in pci_bus_add_device
which is called by pci_bus_add_devices. So "dev->is_added" is false when
checking it in pcibios_enable_device
for the rescanned device.
> How do you trigger the rescan anyway ?
Use the interface under /sys :
echo 1 > /sys/bus/pci/devices/xxx/remove
then echo 1 to the pci device which is the bus of the removed device
echo 1 > /sys/bus/pci/devices/xxxx/rescan
the removed device will be scanned and it's driver module will be loaded
automatically.
Regards,
yuanquan
> I think the problem needs to be solve at a higher level, I'm adding
> linux-pci & Bjorn to the CC list.
>
> Cheers,
> Ben.
>
>> Signed-off-by: Yuanquan Chen <B41889@freescale.com>
>> ---
>> arch/powerpc/kernel/pci-common.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>> index 7f94f76..f0fb070 100644
>> --- a/arch/powerpc/kernel/pci-common.c
>> +++ b/arch/powerpc/kernel/pci-common.c
>> @@ -1496,6 +1496,26 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>> if (ppc_md.pcibios_enable_device_hook(dev))
>> return -EINVAL;
>>
>> + if (!dev->is_added) {
>> + /*
>> + * Fixup NUMA node as it may not be setup yet by the generic
>> + * code and is needed by the DMA init
>> + */
>> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>> +
>> + /* Hook up default DMA ops */
>> + set_dma_ops(&dev->dev, pci_dma_ops);
>> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>> +
>> + /* Additional platform DMA/iommu setup */
>> + if (ppc_md.pci_dma_dev_setup)
>> + ppc_md.pci_dma_dev_setup(dev);
>> +
>> + /* Read default IRQs and fixup if necessary */
>> + pci_read_irq_line(dev);
>> + if (ppc_md.pci_irq_fixup)
>> + ppc_md.pci_irq_fixup(dev);
>> + }
>> return pci_enable_resources(dev, mask);
>> }
>>
>
>
>
>
^ permalink raw reply
* Re: [PATCH 1/3] powerpc: Relocate prom_init.c on 64bit
From: Benjamin Herrenschmidt @ 2012-12-05 7:20 UTC (permalink / raw)
To: Anton Blanchard; +Cc: linuxppc-dev, paulus, amodra
In-Reply-To: <20121205110723.68839261@kryten>
On Wed, 2012-12-05 at 11:07 +1100, Anton Blanchard wrote:
> Hi Ben,
>
> > My only potential objection was that it might have been cleaner to
> > actually build prom_init.c as a separate binary alltogether and piggy
> > back it...
>
> Yeah, I went through a number of options before settling on this one.
Heh, yeah, that what happens when nothing is fully satisfactory...
.../...
> Build prom_init.c 32 bit, tack it onto the end of the vmlinux and
> add a trampoline at the start of the vmlinux.
>
> This means a vmlinux will be bootable and profiling tools will work
> against it. Even so it feels gross to me.
It does, might be less gross to have prom_init.c be .incbin'ed in
vmlinux like the vdso is ... better than tacking it at the end. Still
makes it annoying to debug crashes in prom_init itself.
I'll review your patch when I get a chance as the "least bad"
solution :-)
Cheers,
Ben.
^ permalink raw reply
* Re: [linuxppc-release][PATCH] powerpc/pci-hotplug: fix init issue of rescanned pci device
From: Benjamin Herrenschmidt @ 2012-12-05 7:17 UTC (permalink / raw)
To: Yuanquan Chen; +Cc: Bjorn Helgaas, linux-pci, linuxppc-dev, r61911
In-Reply-To: <1354674717-14426-1-git-send-email-B41889@freescale.com>
On Wed, 2012-12-05 at 10:31 +0800, Yuanquan Chen wrote:
> On powerpc arch, some fixup work of PCI/PCI-e device is just done during the
> first scan at booting time. For the PCI/PCI-e device rescanned after linux OS
> booting up, the fixup work won't be done, which leads to dma_set_mask error or
> irq related issue in rescanned PCI/PCI-e device's driver. So, it does the same
> fixup work for the rescanned device to avoid this issue.
Hrm, the patch is a bit gross. First the code shouldn't be copy/pasted
that way but factored out.
I'm surprised also that is_added is false when pcibios_enable_device()
gets called ... that looks strange to me. At what point is that enable
happening in the hotplug sequence ?
How do you trigger the rescan anyway ?
I think the problem needs to be solve at a higher level, I'm adding
linux-pci & Bjorn to the CC list.
Cheers,
Ben.
> Signed-off-by: Yuanquan Chen <B41889@freescale.com>
> ---
> arch/powerpc/kernel/pci-common.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 7f94f76..f0fb070 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1496,6 +1496,26 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> if (ppc_md.pcibios_enable_device_hook(dev))
> return -EINVAL;
>
> + if (!dev->is_added) {
> + /*
> + * Fixup NUMA node as it may not be setup yet by the generic
> + * code and is needed by the DMA init
> + */
> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
> +
> + /* Hook up default DMA ops */
> + set_dma_ops(&dev->dev, pci_dma_ops);
> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> +
> + /* Additional platform DMA/iommu setup */
> + if (ppc_md.pci_dma_dev_setup)
> + ppc_md.pci_dma_dev_setup(dev);
> +
> + /* Read default IRQs and fixup if necessary */
> + pci_read_irq_line(dev);
> + if (ppc_md.pci_irq_fixup)
> + ppc_md.pci_irq_fixup(dev);
> + }
> return pci_enable_resources(dev, mask);
> }
>
^ permalink raw reply
* Understanding how kernel updates MMU hash table
From: Pegasus11 @ 2012-12-05 5:56 UTC (permalink / raw)
To: linuxppc-dev
Hello.
Ive been trying to understand how an hash PTE is updated. Im on a PPC970MP
machine which using the IBM PowerPC 604e core. My Linux version is 2.6.10 (I
am sorry I cannot migrate at the moment. Management issues and I can't help
:-(( )
Now onto the problem:
hpte_update is invoked to sync the on-chip MMU cache which Linux uses as its
TLB. So whenever a change is made to the PTE, it has to be propagated to the
corresponding TLB entry. And this uses hpte_update for the same. Am I right
here?
Now http://lxr.linux.no/linux-bk+*/+code=hpte_update hpte_update is
declared as
' void hpte_update(pte_t *ptep, unsigned long pte, int wrprot) '.
The arguments to this function is a POINTER to the PTE entry (needed to make
a change persistent across function call right?), the PTE entry (as in the
value) as well the wrprot flag.
Now the code snippet thats bothering me is this:
'
86 ptepage = virt_to_page(ptep);
87 mm = (struct mm_struct *) ptepage->mapping;
88 addr = ptepage->index +
89 (((unsigned long)ptep & ~PAGE_MASK) * PTRS_PER_PTE);
'
On line 86, we get the page structure for a given PTE but we pass the
pointer to PTE not the PTE itself whereas virt_to_page is a macro defined
as:
#define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
Why are passing the POINTER to pte here? I mean are we looking for the PAGE
that is described by the PTE or are we looking for the PAGE which contains
the pointer to PTE? Me things it is the later since the former is given by
the VALUE of the PTE not its POINTER. Right?
So if it indeed the later, what trickery are we here after? Perhaps
following the snippet will make us understand? As I see from above, after
that we get the 'address space object' associated with this page.
What I don't understand is the following line:
addr = ptepage->index + (((unsigned long)ptep & ~PAGE_MASK) *
PTRS_PER_PTE);
First we get the index of the page in the file i.e. the number of pages
preceding the page which holds the address of PTEP. Then we get the lower 12
bits of this page. Then we shift that these bits to the left by 12 again and
to it we add the above index. What is this doing?
There are other things in this function that I do not understand. I'd be
glad if someone could give me a heads up on this.
--
View this message in context: http://old.nabble.com/Understanding-how-kernel-updates-MMU-hash-table-tp34760537p34760537.html
Sent from the linuxppc-dev mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [Patch v4 08/12] memory-hotplug: remove memmap of sparse-vmemmap
From: Jianguo Wu @ 2012-12-05 3:23 UTC (permalink / raw)
To: Tang Chen
Cc: linux-s390, linux-ia64, Wen Congyang, linux-acpi, linux-sh,
Len Brown, x86, linux-kernel, cmetcalf, linux-mm,
Yasuaki Ishimatsu, paulus, Minchan Kim, KOSAKI Motohiro,
David Rientjes, sparclinux, Christoph Lameter, linuxppc-dev,
Andrew Morton, Jiang Liu
In-Reply-To: <50BEAC66.8020500@cn.fujitsu.com>
Hi Tang,
On 2012/12/5 10:07, Tang Chen wrote:
> Hi Wu,
>
> On 12/04/2012 08:20 PM, Jianguo Wu wrote:
> (snip)
>>>
>>> Seems that we have different ways to handle pages allocated by bootmem
>>> or by regular allocator. Is the checking way in [PATCH 09/12] available
>>> here ?
>>>
>>> + /* bootmem page has reserved flag */
>>> + if (PageReserved(page)) {
>>> ......
>>> + }
>>>
>>> If so, I think we can just merge these two functions.
>>
>> Hmm, direct mapping table isn't allocated by bootmem allocator such as memblock, can't be free by put_page_bootmem().
>> But I will try to merge these two functions.
>>
>
> Oh, I didn't notice this, thanks. :)
>
> (snip)
>
>>>> +
>>>> + __split_large_page(kpte, address, pbase);
>>>
>>> Is this patch going to replace [PATCH 08/12] ?
>>>
>>
>> I wish to replace [PATCH 08/12], but need Congyang and Yasuaki to confirm first:)
>>
>>> If so, __split_large_page() was added and exported in [PATCH 09/12],
>>> then we should move it here, right ?
>>
>> yes.
>>
>> and what do you think about moving vmemmap_pud[pmd/pte]_remove() to arch/x86/mm/init_64.c,
>> to be consistent with vmemmap_populate() ?
>
> It is a good idea since pud/pmd/pte related code could be platform
> dependent. And I'm also trying to move vmemmap_free() to
> arch/x86/mm/init_64.c too. I want to have a common interface just
> like vmemmap_populate(). :)
>
Great.
>>
>> I will rework [PATCH 08/12] and [PATCH 09/12] soon.
>
> I am rebasing the whole patch set now. And I think I chould finish part
> of your work too. A new patch-set is coming soon, and your rework is
> also welcome. :)
>
Since you are rebasing now, I will wait for your new patche-set :).
Thanks.
Jianguo Wu
> Thanks. :)
>
>
>
> .
>
^ permalink raw reply
* [linuxppc-release][PATCH] powerpc/pci-hotplug: fix init issue of rescanned pci device
From: Yuanquan Chen @ 2012-12-05 2:31 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Yuanquan Chen, r61911
On powerpc arch, some fixup work of PCI/PCI-e device is just done during the
first scan at booting time. For the PCI/PCI-e device rescanned after linux OS
booting up, the fixup work won't be done, which leads to dma_set_mask error or
irq related issue in rescanned PCI/PCI-e device's driver. So, it does the same
fixup work for the rescanned device to avoid this issue.
Signed-off-by: Yuanquan Chen <B41889@freescale.com>
---
arch/powerpc/kernel/pci-common.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 7f94f76..f0fb070 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1496,6 +1496,26 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
if (ppc_md.pcibios_enable_device_hook(dev))
return -EINVAL;
+ if (!dev->is_added) {
+ /*
+ * Fixup NUMA node as it may not be setup yet by the generic
+ * code and is needed by the DMA init
+ */
+ set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
+
+ /* Hook up default DMA ops */
+ set_dma_ops(&dev->dev, pci_dma_ops);
+ set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
+
+ /* Additional platform DMA/iommu setup */
+ if (ppc_md.pci_dma_dev_setup)
+ ppc_md.pci_dma_dev_setup(dev);
+
+ /* Read default IRQs and fixup if necessary */
+ pci_read_irq_line(dev);
+ if (ppc_md.pci_irq_fixup)
+ ppc_md.pci_irq_fixup(dev);
+ }
return pci_enable_resources(dev, mask);
}
--
1.7.9.5
^ permalink raw reply related
* Re: [Patch v4 08/12] memory-hotplug: remove memmap of sparse-vmemmap
From: Tang Chen @ 2012-12-05 2:07 UTC (permalink / raw)
To: Jianguo Wu
Cc: linux-s390, linux-ia64, Wen Congyang, linux-acpi, linux-sh,
Len Brown, x86, linux-kernel, cmetcalf, linux-mm,
Yasuaki Ishimatsu, paulus, Minchan Kim, KOSAKI Motohiro,
David Rientjes, sparclinux, Christoph Lameter, linuxppc-dev,
Andrew Morton, Jiang Liu
In-Reply-To: <50BDEA82.4050809@huawei.com>
Hi Wu,
On 12/04/2012 08:20 PM, Jianguo Wu wrote:
(snip)
>>
>> Seems that we have different ways to handle pages allocated by bootmem
>> or by regular allocator. Is the checking way in [PATCH 09/12] available
>> here ?
>>
>> + /* bootmem page has reserved flag */
>> + if (PageReserved(page)) {
>> ......
>> + }
>>
>> If so, I think we can just merge these two functions.
>
> Hmm, direct mapping table isn't allocated by bootmem allocator such as memblock, can't be free by put_page_bootmem().
> But I will try to merge these two functions.
>
Oh, I didn't notice this, thanks. :)
(snip)
>>> +
>>> + __split_large_page(kpte, address, pbase);
>>
>> Is this patch going to replace [PATCH 08/12] ?
>>
>
> I wish to replace [PATCH 08/12], but need Congyang and Yasuaki to confirm first:)
>
>> If so, __split_large_page() was added and exported in [PATCH 09/12],
>> then we should move it here, right ?
>
> yes.
>
> and what do you think about moving vmemmap_pud[pmd/pte]_remove() to arch/x86/mm/init_64.c,
> to be consistent with vmemmap_populate() ?
It is a good idea since pud/pmd/pte related code could be platform
dependent. And I'm also trying to move vmemmap_free() to
arch/x86/mm/init_64.c too. I want to have a common interface just
like vmemmap_populate(). :)
>
> I will rework [PATCH 08/12] and [PATCH 09/12] soon.
I am rebasing the whole patch set now. And I think I chould finish part
of your work too. A new patch-set is coming soon, and your rework is
also welcome. :)
Thanks. :)
^ permalink raw reply
* Re: [PATCH 1/3] powerpc: Relocate prom_init.c on 64bit
From: Anton Blanchard @ 2012-12-05 0:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, paulus, amodra
In-Reply-To: <1353993743.2179.28.camel@pasglop>
Hi Ben,
> My only potential objection was that it might have been cleaner to
> actually build prom_init.c as a separate binary alltogether and piggy
> back it...
Yeah, I went through a number of options before settling on this one.
Move prom_init.c into arch/powerpc/boot
Pros:
- 32 bit environment fits well with 32 bit OF interface
- Clear separation enforced between prom_init.c and rest of kernel
- Can build boot wrapper -mrelocatable, similar to ppc32 kernel
Cons:
- tools that assume we can boot a vmlinux will break.
- distro bootloader tools
- provisioning and test environments
- profiling and debugging tools that currently use a vmlinux will break
- oprofile, perf
- Places more pressure on already fragile early memory map
- have to uncompress the zImage somewhere in the RMO
- prom_init.c needs to pull in quite a lot of kernel headers
- seemed difficult to force them to build 32 bit
- Increased simulator boot times due to uncompress stage
Build prom_init.c 32 bit and attach the vmlinux as a binary blob.
This means a vmlinux will be bootable, but profiling tools still wont
work.
Build prom_init.c 32 bit, tack it onto the end of the vmlinux and
add a trampoline at the start of the vmlinux.
This means a vmlinux will be bootable and profiling tools will work
against it. Even so it feels gross to me.
Anton
^ permalink raw reply
* Re: [PATCH 3/4 v5] iommu/fsl: Add iommu domain attributes required by fsl PAMU driver.
From: Scott Wood @ 2012-12-04 18:22 UTC (permalink / raw)
To: Sethi Varun-B16395
Cc: Wood Scott-B07421, Joerg Roedel, Tabi Timur-B04825,
linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <C5ECD7A89D1DC44195F34B25E172658D2B70EE@039-SN2MPN1-013.039d.mgd.msft.net>
On 12/04/2012 05:53:33 AM, Sethi Varun-B16395 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Monday, December 03, 2012 10:34 PM
> > To: Sethi Varun-B16395
> > Cc: Joerg Roedel; linux-kernel@vger.kernel.org; iommu@lists.linux-
> > foundation.org; Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; =20
> Tabi
> > Timur-B04825
> > Subject: Re: [PATCH 3/4 v5] iommu/fsl: Add iommu domain attributes
> > required by fsl PAMU driver.
> >
> > On 12/03/2012 10:57:29 AM, Sethi Varun-B16395 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: iommu-bounces@lists.linux-foundation.org [mailto:iommu-
> > > > bounces@lists.linux-foundation.org] On Behalf Of Joerg Roedel
> > > > Sent: Sunday, December 02, 2012 7:33 PM
> > > > To: Sethi Varun-B16395
> > > > Cc: linux-kernel@vger.kernel.org; =20
> iommu@lists.linux-foundation.org;
> > > Wood
> > > > Scott-B07421; linuxppc-dev@lists.ozlabs.org; Tabi Timur-B04825
> > > > Subject: Re: [PATCH 3/4 v5] iommu/fsl: Add iommu domain =20
> attributes
> > > > required by fsl PAMU driver.
> > > >
> > > > Hmm, we need to work out a good abstraction for this.
> > > >
> > > > On Tue, Nov 20, 2012 at 07:24:56PM +0530, Varun Sethi wrote:
> > > > > Added the following domain attributes required by FSL PAMU =20
> driver:
> > > > > 1. Subwindows field added to the iommu domain geometry =20
> attribute.
> > > >
> > > > Are the Subwindows mapped with full size or do you map only =20
> parts
> > > of the
> > > > subwindows?
> > > >
> > > [Sethi Varun-B16395] It's possible to map a part of the subwindow =20
> i.e.
> > > size of the mapping can be less than the sub window size.
> > >
> > > > > + * This attribute indicates number of DMA subwindows
> > supported
> > > by
> > > > > + * the geometry. If there is a single window that maps =20
> the
> > > entire
> > > > > + * geometry, attribute must be set to "1". A value of =20
> "0"
> > > implies
> > > > > + * that this mechanism is not used at all(normal paging =20
> is
> > > used).
> > > > > + * Value other than* "0" or "1" indicates the actual =20
> number
> > of
> > > > > + * subwindows.
> > > > > + */
> > > >
> > > > This semantic is ugly, how about a feature detection mechanism?
> > > >
> > > [Sethi Varun-B16395] A feature mechanism to query the type of =20
> IOMMU?
> >
> > A feature mechanism to determine whether this subwindow mechanism is
> > available, and what the limits are.
> >
> So, we use the IOMMU capability interface to find out if IOMMU =20
> supports sub windows or not, right? But still number of sub windows =20
> would be specified as a part of the geometry and the valid value for =20
> sub windows would 0,1 or actual number of sub windows.
How does a user of the interface find out what values are possible for =20
the "actual number of subwindows"? How does a user of the interface =20
find out whether there are any limitations on specifying a value of =20
zero (in the case of PAMU, that would be a maximum 1 MiB =20
naturally-aligned aperture to support arbitrary 4KiB mappings)?
-Scott=
^ permalink raw reply
* Re: [PATCH 1/2] vfio powerpc: enabled on powernv platform
From: Alex Williamson @ 2012-12-04 15:51 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <50BDB074.1000403@ozlabs.ru>
On Tue, 2012-12-04 at 19:12 +1100, Alexey Kardashevskiy wrote:
> On 04/12/12 04:35, Alex Williamson wrote:
> > On Mon, 2012-12-03 at 13:52 +1100, Alexey Kardashevskiy wrote:
> >> This patch initializes IOMMU groups based on the IOMMU
> >> configuration discovered during the PCI scan on POWERNV
> >> (POWER non virtualized) platform. The IOMMU groups are
> >> to be used later by VFIO driver (PCI pass through).
> >>
> >> It also implements an API for mapping/unmapping pages for
> >> guest PCI drivers and providing DMA window properties.
> >> This API is going to be used later by QEMU-VFIO to handle
> >> h_put_tce hypercalls from the KVM guest.
> >>
> >> Although this driver has been tested only on the POWERNV
> >> platform, it should work on any platform which supports
> >> TCE tables.
> >>
> >> To enable VFIO on POWER, enable SPAPR_TCE_IOMMU config
> >> option and configure VFIO as required.
> >>
> >> Cc: David Gibson <david@gibson.dropbear.id.au>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >> arch/powerpc/include/asm/iommu.h | 9 ++
> >> arch/powerpc/kernel/iommu.c | 186 ++++++++++++++++++++++++++++++++++
> >> arch/powerpc/platforms/powernv/pci.c | 135 ++++++++++++++++++++++++
> >> drivers/iommu/Kconfig | 8 ++
> >> 4 files changed, 338 insertions(+)
> >>
> >> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> >> index cbfe678..5c7087a 100644
> >> --- a/arch/powerpc/include/asm/iommu.h
> >> +++ b/arch/powerpc/include/asm/iommu.h
> >> @@ -76,6 +76,9 @@ struct iommu_table {
> >> struct iommu_pool large_pool;
> >> struct iommu_pool pools[IOMMU_NR_POOLS];
> >> unsigned long *it_map; /* A simple allocation bitmap for now */
> >> +#ifdef CONFIG_IOMMU_API
> >> + struct iommu_group *it_group;
> >> +#endif
> >> };
> >>
> >> struct scatterlist;
> >> @@ -147,5 +150,11 @@ static inline void iommu_restore(void)
> >> }
> >> #endif
> >>
> >> +extern long iommu_clear_tces(struct iommu_table *tbl, unsigned long entry,
> >> + unsigned long pages);
> >> +extern long iommu_put_tces(struct iommu_table *tbl, unsigned long entry,
> >> + uint64_t tce, enum dma_data_direction direction,
> >> + unsigned long pages);
> >> +
> >> #endif /* __KERNEL__ */
> >> #endif /* _ASM_IOMMU_H */
> >> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >> index ff5a6ce..2738aa4 100644
> >> --- a/arch/powerpc/kernel/iommu.c
> >> +++ b/arch/powerpc/kernel/iommu.c
> >> @@ -44,6 +44,7 @@
> >> #include <asm/kdump.h>
> >> #include <asm/fadump.h>
> >> #include <asm/vio.h>
> >> +#include <asm/tce.h>
> >>
> >> #define DBG(...)
> >>
> >> @@ -856,3 +857,188 @@ void iommu_free_coherent(struct iommu_table *tbl, size_t size,
> >> free_pages((unsigned long)vaddr, get_order(size));
> >> }
> >> }
> >> +
> >> +#ifdef CONFIG_IOMMU_API
> >> +/*
> >> + * SPAPR TCE API
> >> + */
> >> +
> >> +/*
> >> + * Returns the number of used IOMMU pages (4K) within
> >> + * the same system page (4K or 64K).
> >> + * bitmap_weight is not used as it does not support bigendian maps.
> >> + */
> >> +static int syspage_weight(unsigned long *map, unsigned long entry)
> >> +{
> >> + int ret = 0, nbits = PAGE_SIZE/IOMMU_PAGE_SIZE;
> >> +
> >> + /* Aligns TCE entry number to system page boundary */
> >> + entry &= PAGE_MASK >> IOMMU_PAGE_SHIFT;
> >> +
> >> + /* Count used 4K pages */
> >> + while (nbits--)
> >> + ret += (test_bit(entry++, map) == 0) ? 0 : 1;
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void tce_flush(struct iommu_table *tbl)
> >> +{
> >> + /* Flush/invalidate TLB caches if necessary */
> >> + if (ppc_md.tce_flush)
> >> + ppc_md.tce_flush(tbl);
> >> +
> >> + /* Make sure updates are seen by hardware */
> >> + mb();
> >> +}
> >> +
> >> +/*
> >> + * iommu_clear_tces clears tces and returned the number of system pages
> >> + * which it called put_page() on
> >> + */
> >> +static long clear_tces_nolock(struct iommu_table *tbl, unsigned long entry,
> >> + unsigned long pages)
> >> +{
> >> + int i, retpages = 0;
> >> + unsigned long oldtce, oldweight;
> >> + struct page *page;
> >> +
> >> + for (i = 0; i < pages; ++i) {
> >> + oldtce = ppc_md.tce_get(tbl, entry + i);
> >> + ppc_md.tce_free(tbl, entry + i, 1);
> >> +
> >> + oldweight = syspage_weight(tbl->it_map, entry);
> >> + __clear_bit(entry - tbl->it_offset, tbl->it_map);
> >> +
> >> + if (!(oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)))
> >> + continue;
> >
> > Could this happen earlier, above syspage_weight() and __clear_bit()?
>
>
> Want to clear it anyway if it is not cleared by some reason. Added WARN_ON.
The map shouldn't be set unless read/write is enabled, right? It seems
like we don't have a lot of trust in this bitmap.
> >> +
> >> + page = pfn_to_page(oldtce >> PAGE_SHIFT);
> >> +
> >> + WARN_ON(!page);
> >> + if (!page)
> >> + continue;
> >> +
> >> + if (oldtce & TCE_PCI_WRITE)
> >> + SetPageDirty(page);
> >> +
> >> + put_page(page);
> >> +
> >> + /* That was the last IOMMU page within the system page */
> >> + if ((oldweight == 1) && !syspage_weight(tbl->it_map, entry))
> >> + ++retpages;
> >
> > If you used __test_and_clear_bit() above I think you could avoid this
> > 2nd call to syspage_weight. A minor optimization though.
> >
> >> + }
> >> +
> >> + return retpages;
> >> +}
> >> +
> >> +/*
> >> + * iommu_clear_tces clears tces and returned the number
> >> + / of released system pages
> >> + */
> >
> > Something bad happened to your comments here.
> >
> >> +long iommu_clear_tces(struct iommu_table *tbl, unsigned long entry,
> >> + unsigned long pages)
> >> +{
> >> + int ret;
> >> + struct iommu_pool *pool = get_pool(tbl, entry);
> >> +
> >> + spin_lock(&(pool->lock));
> >> + ret = clear_tces_nolock(tbl, entry, pages);
> >> + tce_flush(tbl);
> >> + spin_unlock(&(pool->lock));
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(iommu_clear_tces);
> >> +
> >> +static int put_tce(struct iommu_table *tbl, unsigned long entry,
> >> + uint64_t tce, enum dma_data_direction direction)
> >> +{
> >> + int ret;
> >> + struct page *page = NULL;
> >> + unsigned long kva, offset, oldweight;
> >> +
> >> + /* Map new TCE */
> >> + offset = (tce & IOMMU_PAGE_MASK) - (tce & PAGE_MASK);
> >
> > Maybe the compiler will figure this out, but isn't this the same as tce
> > & (IOMMU_PAGE_MASK & PAGE_MASK)?
>
>
> it is rather (tce & (IOMMU_PAGE_MASK & ~PAGE_MASK)) but I cannot see how it
> is simpler and I doubt that it is faster enough to notice it anyhow :)
Yes, ~PAGE_MASK. IMHO, it's more intuitive.
> >> + ret = get_user_pages_fast(tce & PAGE_MASK, 1,
> >> + direction != DMA_TO_DEVICE, &page);
> >> + if (ret < 1) {
> >
> > Probably (ret != 1) here or else we never get to your >1 case below.
> >
> >> + printk(KERN_ERR "tce_vfio: get_user_pages_fast failed tce=%llx ioba=%lx ret=%d\n",
> >> + tce, entry << IOMMU_PAGE_SHIFT, ret);
> >
> > Use pr_err
> >
> >> + if (!ret || (ret > 1))
> >
> > Then (ret >= 0) here. Or return (ret >= 0) ? -EFAULT : ret
> >
> >> + ret = -EFAULT;
> >> + return ret;
> >> + }
> >
> > You're missing the code from x86 that handles mapping mmap'd ranges.
> > This is intended to allow peer-to-peer DMA between devices. Is that
> > intentional?
>
> I am not following you here. What code exactly are talking about? We do not
> track ranges at all and I do not see how it helps with p2p dma.
The code in type1 that checks PFNMAP and reserved pages that I only
sometimes remember how it works ;) The idea there is to allow p2p dma
by inserting iommu translations for non-page backed memory, ie. the
mmap'd BARs of other devices. It may be that the POWER topology is not
amenable to this since you have a whole PCI bus in your group and
intra-group p2p isn't iommu translated. I'm not sure how useful it is
even on x86, but KVM device assignment does it, so I added it to type1.
> >> +
> >> + kva = (unsigned long) page_address(page);
> >> + kva += offset;
> >> +
> >> + /* tce_build receives a virtual address */
> >> + entry += tbl->it_offset; /* Offset into real TCE table */
> >
> > Here's what makes me call the entry "relative" rather than zero-based.
>
> This is the bug actually, I overlooked it and I removed it now. Thanks for
> being so picky :)
Ah, ok. I'll look for it on the next round and make sure I understand
it.
> > The iova is relative to the start of dma32_window_start, ie. if the
> > window starts at bus address 512MB and I want to create a translation at
> > bus address 512MB, I pass in an iova of 0, right? The above adds the
> > window offset. So you've removed dma64 window, but we really need to
> > define iova better.
>
>
>
>
> >> + ret = ppc_md.tce_build(tbl, entry, 1, kva, direction, NULL);
> >> +
> >> + /* tce_build() only returns non-zero for transient errors */
> >> + if (unlikely(ret)) {
> >> + printk(KERN_ERR "tce_vfio: tce_put failed on tce=%llx ioba=%lx kva=%lx ret=%d\n",
> >> + tce, entry << IOMMU_PAGE_SHIFT, kva, ret);
> >
> > Use pr_err
> >
> >> + put_page(page);
> >> + return -EIO;
> >> + }
> >> +
> >> + /* Calculate if new system page has been locked */
> >> + oldweight = syspage_weight(tbl->it_map, entry);
> >> + __set_bit(entry - tbl->it_offset, tbl->it_map);
> >> +
> >> + return (oldweight == 0) ? 1 : 0;
> >> +}
> >> +
> >> +/*
> >> + * iommu_put_tces builds tces and returned the number of actually
> >> + * locked system pages
> >> + */
> >> +long iommu_put_tces(struct iommu_table *tbl, unsigned long entry,
> >> + uint64_t tce, enum dma_data_direction direction,
> >> + unsigned long pages)
> >> +{
> >> + int i, ret = 0, retpages = 0;
> >> + struct iommu_pool *pool = get_pool(tbl, entry);
> >> +
> >> + BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
> >> + BUG_ON(direction == DMA_NONE);
> >
> > This doesn't seem BUG worthy, -EINVAL? We can't assume tce_iommu_ioctl
> > will always be the only caller of this function.
>
>
> This is what other function does in this file.
Blech, ok.
> >> +
> >> + spin_lock(&(pool->lock));
> >> +
> >> + /* Check if any is in use */
> >> + for (i = 0; i < pages; ++i) {
> >> + unsigned long oldtce = ppc_md.tce_get(tbl, entry + i);
> >> + if ((oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)) ||
> >> + test_bit(entry + i, tbl->it_map)) {
> >> + WARN_ON(test_bit(entry + i, tbl->it_map));
> >
> > The WARN_ON seems to confirm that these are redundant tests, does that
> > imply we don't trust it_map? It would be a lot faster if we could rely
> > on it_map exclusively here.
>
>
> As for me, pretty minor optimization. I'm testing it now to see if I do not
> miss bits.
It would be a lot more re-assuring if we didn't need it ;) Thanks,
Alex
> >> + spin_unlock(&(pool->lock));
> >> + return -EBUSY;
> >> + }
> >> + }
> >> +
> >> + /* Put tces to the table */
> >> + for (i = 0; (i < pages) && (ret >= 0); ++i, tce += IOMMU_PAGE_SIZE) {
> >> + ret = put_tce(tbl, entry + i, tce, direction);
> >> + if (ret == 1)
> >> + ++retpages;
> >> + }
> >> +
> >> + /*
> >> + * If failed, release locked pages, otherwise return the number
> >> + * of locked system pages
> >> + */
> >> + if (ret < 0)
> >> + clear_tces_nolock(tbl, entry, i);
> >> + else
> >> + ret = retpages;
> >> +
> >> + tce_flush(tbl);
> >> + spin_unlock(&(pool->lock));
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(iommu_put_tces);
> >> +#endif /* CONFIG_IOMMU_API */
> >> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> >> index 05205cf..21250ef 100644
> >> --- a/arch/powerpc/platforms/powernv/pci.c
> >> +++ b/arch/powerpc/platforms/powernv/pci.c
> >> @@ -20,6 +20,7 @@
> >> #include <linux/irq.h>
> >> #include <linux/io.h>
> >> #include <linux/msi.h>
> >> +#include <linux/iommu.h>
> >>
> >> #include <asm/sections.h>
> >> #include <asm/io.h>
> >> @@ -613,3 +614,137 @@ void __init pnv_pci_init(void)
> >> ppc_md.teardown_msi_irqs = pnv_teardown_msi_irqs;
> >> #endif
> >> }
> >> +
> >> +#ifdef CONFIG_IOMMU_API
> >> +/*
> >> + * IOMMU groups support required by VFIO
> >> + */
> >> +static int add_device(struct device *dev)
> >> +{
> >> + struct iommu_table *tbl;
> >> + int ret = 0;
> >> +
> >> + if (WARN_ON(dev->iommu_group)) {
> >> + printk(KERN_WARNING "tce_vfio: device %s is already in iommu group %d, skipping\n",
> >> + dev_name(dev),
> >> + iommu_group_id(dev->iommu_group));
> >
> > Use pr_warn
> >
> >> + return -EBUSY;
> >> + }
> >> +
> >> + tbl = get_iommu_table_base(dev);
> >> + if (!tbl) {
> >> + pr_debug("tce_vfio: skipping device %s with no tbl\n",
> >> + dev_name(dev));
> >> + return 0;
> >> + }
> >> +
> >> + pr_debug("tce_vfio: adding %s to iommu group %d\n",
> >> + dev_name(dev), iommu_group_id(tbl->it_group));
> >> +
> >> + ret = iommu_group_add_device(tbl->it_group, dev);
> >> + if (ret < 0)
> >> + printk(KERN_ERR "tce_vfio: %s has not been added, ret=%d\n",
> >> + dev_name(dev), ret);
> >
> > Use pr_err
> >
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void del_device(struct device *dev)
> >> +{
> >> + iommu_group_remove_device(dev);
> >> +}
> >> +
> >> +static int iommu_bus_notifier(struct notifier_block *nb,
> >> + unsigned long action, void *data)
> >> +{
> >> + struct device *dev = data;
> >> +
> >> + switch (action) {
> >> + case BUS_NOTIFY_ADD_DEVICE:
> >> + return add_device(dev);
> >> + case BUS_NOTIFY_DEL_DEVICE:
> >> + del_device(dev);
> >> + return 0;
> >> + default:
> >> + return 0;
> >> + }
> >> +}
> >> +
> >> +static struct notifier_block tce_iommu_bus_nb = {
> >> + .notifier_call = iommu_bus_notifier,
> >> +};
> >> +
> >> +static void group_release(void *iommu_data)
> >> +{
> >> + struct iommu_table *tbl = iommu_data;
> >> + tbl->it_group = NULL;
> >> +}
> >> +
> >> +static int __init tce_iommu_init(void)
> >> +{
> >> + struct pci_dev *pdev = NULL;
> >> + struct iommu_table *tbl;
> >> + struct iommu_group *grp;
> >> +
> >> + /* Allocate and initialize IOMMU groups */
> >> + for_each_pci_dev(pdev) {
> >> + tbl = get_iommu_table_base(&pdev->dev);
> >> + if (!tbl)
> >> + continue;
> >> +
> >> + /* Skip already initialized */
> >> + if (tbl->it_group)
> >> + continue;
> >> +
> >> + grp = iommu_group_alloc();
> >> + if (IS_ERR(grp)) {
> >> + printk(KERN_INFO "tce_vfio: cannot create "
> >> + "new IOMMU group, ret=%ld\n",
> >> + PTR_ERR(grp));
> >
> > Use pr_info
> >
> >> + return PTR_ERR(grp);
> >> + }
> >> + tbl->it_group = grp;
> >> + iommu_group_set_iommudata(grp, tbl, group_release);
> >> + }
> >> +
> >> + bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
> >> +
> >> + /* Add PCI devices to VFIO groups */
> >> + for_each_pci_dev(pdev)
> >> + add_device(&pdev->dev);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void __exit tce_iommu_cleanup(void)
> >> +{
> >> + struct pci_dev *pdev = NULL;
> >> + struct iommu_table *tbl;
> >> + struct iommu_group *grp = NULL;
> >> +
> >> + bus_unregister_notifier(&pci_bus_type, &tce_iommu_bus_nb);
> >> +
> >> + /* Delete PCI devices from VFIO groups */
> >> + for_each_pci_dev(pdev)
> >> + del_device(&pdev->dev);
> >> +
> >> + /* Release VFIO groups */
> >> + for_each_pci_dev(pdev) {
> >> + tbl = get_iommu_table_base(&pdev->dev);
> >> + if (!tbl)
> >> + continue;
> >> + grp = tbl->it_group;
> >> +
> >> + /* Skip (already) uninitialized */
> >> + if (!grp)
> >> + continue;
> >> +
> >> + /* Do actual release, group_release() is expected to work */
> >> + iommu_group_put(grp);
> >> + BUG_ON(tbl->it_group);
> >> + }
> >> +}
> >> +
> >> +module_init(tce_iommu_init);
> >> +module_exit(tce_iommu_cleanup);
> >> +#endif /* CONFIG_IOMMU_API */
> >> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> >> index 9f69b56..29d11dc 100644
> >> --- a/drivers/iommu/Kconfig
> >> +++ b/drivers/iommu/Kconfig
> >> @@ -187,4 +187,12 @@ config EXYNOS_IOMMU_DEBUG
> >>
> >> Say N unless you need kernel log message for IOMMU debugging
> >>
> >> +config SPAPR_TCE_IOMMU
> >> + bool "sPAPR TCE IOMMU Support"
> >> + depends on PPC_POWERNV
> >> + select IOMMU_API
> >> + help
> >> + Enables bits of IOMMU API required by VFIO. The iommu_ops is
> >> + still not implemented.
> >> +
> >> endif # IOMMU_SUPPORT
> >
> > Thanks,
> >
> > Alex
> >
>
>
^ permalink raw reply
* Re: [PATCH] powerpc/pci-hotplug: fix the rescanned pci device's dma_set_mask issue
From: Kumar Gala @ 2012-12-04 14:32 UTC (permalink / raw)
To: Chen Yuanquan-B41889; +Cc: linuxppc-dev@lists.ozlabs.org list, Zang Roy-R61911
In-Reply-To: <42C14AFA-42D6-498D-9B38-A02CEAC2D0BA@kernel.crashing.org>
On Nov 28, 2012, at 11:34 AM, Kumar Gala wrote:
>=20
> On Nov 27, 2012, at 8:34 PM, Chen Yuanquan-B41889 wrote:
>=20
>> On 11/25/2012 08:41 PM, Kumar Gala wrote:
>>> On Nov 22, 2012, at 10:29 PM, Yuanquan Chen wrote:
>>>=20
>>>> On powerpc arch, dma_ops of rescanned pci device after system's =
booting up won't be
>>>> initialized by system, so it will fail to execute the dma_set_mask =
in the device's
>>>> driver. Initialize it to solve this issue.
>>>>=20
>>>> Signed-off-by: Yuanquan Chen <B41889@freescale.com>
>>>> ---
>>>> arch/powerpc/include/asm/dma-mapping.h | 7 +++++--
>>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>> This is not the right way to get the dma_ops setup. You need to =
find some other point for the hotplug scenario to get the dma_ops setup.
>>>=20
>>> - k
>>=20
>> Hi Kumar,
>>=20
>> I read the code about pci bus scan and rescan. Only the =
pcibios_fixup_bus in pci_scan_child_bus and
>> pcibios_enable_device in pci_rescan_bus are arch related code. The =
pcibios_fixup_bus won't be called
>> for the rescanned PCI devices due to the bus->is_added has been set =
for the first scanning at boot time.
>> So I think it's more reasonable to do the same work as =
pcibios_fixup_bus for rescanned PCI device in
>> pcibios_enable_device. The patch code is a copy of =
pcibios_setup_bus_devices called by pcibios_fixup_bus,
>> It can solve the dma_set_mask and irq related issues of rescanned PCI =
device on powerpc arch. What's
>> your opinion?
>>=20
>> Thanks,
>> yuanquan
>>=20
>> diff --git a/arch/powerpc/kernel/pci-common.c =
b/arch/powerpc/kernel/pci-common.c
>> index 7f94f76..30f7d61 100644
>> --- a/arch/powerpc/kernel/pci-common.c
>> +++ b/arch/powerpc/kernel/pci-common.c
>> @@ -1496,6 +1496,23 @@ int pcibios_enable_device(struct pci_dev *dev, =
int mask)
>> if (ppc_md.pcibios_enable_device_hook(dev))
>> return -EINVAL;
>>=20
>> + if (!dev->is_added) {
>> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>> +
>> + /* Hook up default DMA ops */
>> + set_dma_ops(&dev->dev, pci_dma_ops);
>> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>> +
>> + /* Additional platform DMA/iommu setup */
>> + if (ppc_md.pci_dma_dev_setup)
>> + ppc_md.pci_dma_dev_setup(dev);
>> +
>> + /* Read default IRQs and fixup if necessary */
>> + pci_read_irq_line(dev);
>> + if (ppc_md.pci_irq_fixup)
>> + ppc_md.pci_irq_fixup(dev);
>> + }
>> +
>> return pci_enable_resources(dev, mask);
>> }
>>=20
>=20
> This looks better than the first patch.. I'm surprised that the IBM =
guys don't run into a similar issue with their hotplug support.
>=20
> Maybe Ben or someone else will comment.
>=20
> - k
How about you post this as a clean patch, will be easier to get Ben to =
comment on that.
- k=
^ permalink raw reply
* Re: [PATCH 2/4] powerpc: enable ARCH_USE_BUILTIN_BSWAP
From: David Woodhouse @ 2012-12-04 14:32 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linux-arch, x86, linuxppc-dev
In-Reply-To: <20121204220232.35c7ece1f96e20cd884106b6@canb.auug.org.au>
[-- Attachment #1: Type: text/plain, Size: 498 bytes --]
On Tue, 2012-12-04 at 22:02 +1100, Stephen Rothwell wrote:
> > +config ARCH_USE_BUILTIN_BSWAP
> > + def_bool y
> > +
>
> This should be defined as bool in arch/Kconfig (probably in the previous
> patch) and then selected from appropriate architectures.
Thanks. Updated series at
git://git.infradead.org/users/dwmw2/byteswap.git
http://git.infradead.org/users/dwmw2/byteswap.git
I'll post them again in a day or so, incorporating any other feedback I
get.
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4 v6] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
From: Tabi Timur-B04825 @ 2012-12-04 12:29 UTC (permalink / raw)
To: Sethi Varun-B16395
Cc: joerg.roedel@amd.com, iommu@lists.linux-foundation.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Wood Scott-B07421
In-Reply-To: <C5ECD7A89D1DC44195F34B25E172658D2B715D@039-SN2MPN1-013.039d.mgd.msft.net>
Sethi Varun-B16395 wrote:
> This would in any case change with the new LIODN allocation scheme. I
> intend on introducing the new scheme as a separate patch.
At the very least, you should detect when an LIODN is too large and print=20
an error message.
--=20
Timur Tabi
Linux kernel developer at Freescale=
^ permalink raw reply
* Re: [Patch v4 08/12] memory-hotplug: remove memmap of sparse-vmemmap
From: Jianguo Wu @ 2012-12-04 12:20 UTC (permalink / raw)
To: Tang Chen
Cc: linux-s390, linux-ia64, Wen Congyang, linux-acpi, linux-sh,
Len Brown, x86, linux-kernel, cmetcalf, linux-mm,
Yasuaki Ishimatsu, paulus, Minchan Kim, KOSAKI Motohiro,
David Rientjes, sparclinux, Christoph Lameter, linuxppc-dev,
Andrew Morton, Jiang Liu
In-Reply-To: <50BDBEB7.3070807@cn.fujitsu.com>
Hi Tang,
Thanks for your review and comments, Please see below for my reply.
On 2012/12/4 17:13, Tang Chen wrote:
> Hi Wu,
>
> Sorry to make noise here. Please see below. :)
>
> On 12/03/2012 10:23 AM, Jianguo Wu wrote:
>> Signed-off-by: Jianguo Wu<wujianguo@huawei.com>
>> Signed-off-by: Jiang Liu<jiang.liu@huawei.com>
>> ---
>> include/linux/mm.h | 1 +
>> mm/sparse-vmemmap.c | 231 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> mm/sparse.c | 3 +-
>> 3 files changed, 234 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 5657670..1f26af5 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1642,6 +1642,7 @@ int vmemmap_populate(struct page *start_page, unsigned long pages, int node);
>> void vmemmap_populate_print_last(void);
>> void register_page_bootmem_memmap(unsigned long section_nr, struct page *map,
>> unsigned long size);
>> +void vmemmap_free(struct page *memmap, unsigned long nr_pages);
>>
>> enum mf_flags {
>> MF_COUNT_INCREASED = 1<< 0,
>> diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
>> index 1b7e22a..748732d 100644
>> --- a/mm/sparse-vmemmap.c
>> +++ b/mm/sparse-vmemmap.c
>> @@ -29,6 +29,10 @@
>> #include<asm/pgalloc.h>
>> #include<asm/pgtable.h>
>>
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +#include<asm/tlbflush.h>
>> +#endif
>> +
>> /*
>> * Allocate a block of memory to be used to back the virtual memory map
>> * or to back the page tables that are used to create the mapping.
>> @@ -224,3 +228,230 @@ void __init sparse_mem_maps_populate_node(struct page **map_map,
>> vmemmap_buf_end = NULL;
>> }
>> }
>> +
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +
>> +#define PAGE_INUSE 0xFD
>> +
>> +static void vmemmap_free_pages(struct page *page, int order)
>> +{
>> + struct zone *zone;
>> + unsigned long magic;
>> +
>> + magic = (unsigned long) page->lru.next;
>> + if (magic == SECTION_INFO || magic == MIX_SECTION_INFO) {
>> + put_page_bootmem(page);
>> +
>> + zone = page_zone(page);
>> + zone_span_writelock(zone);
>> + zone->present_pages++;
>> + zone_span_writeunlock(zone);
>> + totalram_pages++;
>
> Seems that we have different ways to handle pages allocated by bootmem
> or by regular allocator. Is the checking way in [PATCH 09/12] available
> here ?
>
> + /* bootmem page has reserved flag */
> + if (PageReserved(page)) {
> ......
> + }
>
> If so, I think we can just merge these two functions.
Hmm, direct mapping table isn't allocated by bootmem allocator such as memblock, can't be free by put_page_bootmem().
But I will try to merge these two functions.
>
>> + } else
>> + free_pages((unsigned long)page_address(page), order);
>> +}
>> +
>> +static void free_pte_table(pmd_t *pmd)
>> +{
>> + pte_t *pte, *pte_start;
>> + int i;
>> +
>> + pte_start = (pte_t *)pmd_page_vaddr(*pmd);
>> + for (i = 0; i< PTRS_PER_PTE; i++) {
>> + pte = pte_start + i;
>> + if (pte_val(*pte))
>> + return;
>> + }
>> +
>> + /* free a pte talbe */
>> + vmemmap_free_pages(pmd_page(*pmd), 0);
>> + spin_lock(&init_mm.page_table_lock);
>> + pmd_clear(pmd);
>> + spin_unlock(&init_mm.page_table_lock);
>> +}
>> +
>> +static void free_pmd_table(pud_t *pud)
>> +{
>> + pmd_t *pmd, *pmd_start;
>> + int i;
>> +
>> + pmd_start = (pmd_t *)pud_page_vaddr(*pud);
>> + for (i = 0; i< PTRS_PER_PMD; i++) {
>> + pmd = pmd_start + i;
>> + if (pmd_val(*pmd))
>> + return;
>> + }
>> +
>> + /* free a pmd talbe */
>> + vmemmap_free_pages(pud_page(*pud), 0);
>> + spin_lock(&init_mm.page_table_lock);
>> + pud_clear(pud);
>> + spin_unlock(&init_mm.page_table_lock);
>> +}
>> +
>> +static void free_pud_table(pgd_t *pgd)
>> +{
>> + pud_t *pud, *pud_start;
>> + int i;
>> +
>> + pud_start = (pud_t *)pgd_page_vaddr(*pgd);
>> + for (i = 0; i< PTRS_PER_PUD; i++) {
>> + pud = pud_start + i;
>> + if (pud_val(*pud))
>> + return;
>> + }
>> +
>> + /* free a pud table */
>> + vmemmap_free_pages(pgd_page(*pgd), 0);
>> + spin_lock(&init_mm.page_table_lock);
>> + pgd_clear(pgd);
>> + spin_unlock(&init_mm.page_table_lock);
>> +}
>
> All the free_xxx_table() are very similar to the functions in
> [PATCH 09/12]. Could we reuse them anyway ?
yes, we can reuse them.
>
>> +
>> +static int split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase)
>> +{
>> + struct page *page = pmd_page(*(pmd_t *)kpte);
>> + int i = 0;
>> + unsigned long magic;
>> + unsigned long section_nr;
>> +
>> + __split_large_page(kpte, address, pbase);
>
> Is this patch going to replace [PATCH 08/12] ?
>
I wish to replace [PATCH 08/12], but need Congyang and Yasuaki to confirm first:)
> If so, __split_large_page() was added and exported in [PATCH 09/12],
> then we should move it here, right ?
yes.
and what do you think about moving vmemmap_pud[pmd/pte]_remove() to arch/x86/mm/init_64.c,
to be consistent with vmemmap_populate() ?
I will rework [PATCH 08/12] and [PATCH 09/12] soon.
Thanks,
Jianguo Wu.
>
> If not, free_map_bootmem() and __kfree_section_memmap() were changed in
> [PATCH 08/12], and we need to handle this.
>
>> + __flush_tlb_all();
>> +
>> + magic = (unsigned long) page->lru.next;
>> + if (magic == SECTION_INFO) {
>> + section_nr = pfn_to_section_nr(page_to_pfn(page));
>> + while (i< PTRS_PER_PMD) {
>> + page++;
>> + i++;
>> + get_page_bootmem(section_nr, page, SECTION_INFO);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void vmemmap_pte_remove(pmd_t *pmd, unsigned long addr, unsigned long end)
>> +{
>> + pte_t *pte;
>> + unsigned long next;
>> + void *page_addr;
>> +
>> + pte = pte_offset_kernel(pmd, addr);
>> + for (; addr< end; pte++, addr += PAGE_SIZE) {
>> + next = (addr + PAGE_SIZE)& PAGE_MASK;
>> + if (next> end)
>> + next = end;
>> +
>> + if (pte_none(*pte))
>> + continue;
>> + if (IS_ALIGNED(addr, PAGE_SIZE)&&
>> + IS_ALIGNED(next, PAGE_SIZE)) {
>> + vmemmap_free_pages(pte_page(*pte), 0);
>> + spin_lock(&init_mm.page_table_lock);
>> + pte_clear(&init_mm, addr, pte);
>> + spin_unlock(&init_mm.page_table_lock);
>> + } else {
>> + /*
>> + * Removed page structs are filled with 0xFD.
>> + */
>> + memset((void *)addr, PAGE_INUSE, next - addr);
>> + page_addr = page_address(pte_page(*pte));
>> +
>> + if (!memchr_inv(page_addr, PAGE_INUSE, PAGE_SIZE)) {
>> + spin_lock(&init_mm.page_table_lock);
>> + pte_clear(&init_mm, addr, pte);
>> + spin_unlock(&init_mm.page_table_lock);
>> + }
>> + }
>> + }
>> +
>> + free_pte_table(pmd);
>> + __flush_tlb_all();
>> +}
>> +
>> +static void vmemmap_pmd_remove(pud_t *pud, unsigned long addr, unsigned long end)
>> +{
>> + unsigned long next;
>> + pmd_t *pmd;
>> +
>> + pmd = pmd_offset(pud, addr);
>> + for (; addr< end; addr = next, pmd++) {
>> + next = pmd_addr_end(addr, end);
>> + if (pmd_none(*pmd))
>> + continue;
>> +
>> + if (cpu_has_pse) {
>> + unsigned long pte_base;
>> +
>> + if (IS_ALIGNED(addr, PMD_SIZE)&&
>> + IS_ALIGNED(next, PMD_SIZE)) {
>> + vmemmap_free_pages(pmd_page(*pmd),
>> + get_order(PMD_SIZE));
>> + spin_lock(&init_mm.page_table_lock);
>> + pmd_clear(pmd);
>> + spin_unlock(&init_mm.page_table_lock);
>> + continue;
>> + }
>> +
>> + /*
>> + * We use 2M page, but we need to remove part of them,
>> + * so split 2M page to 4K page.
>> + */
>> + pte_base = get_zeroed_page(GFP_ATOMIC | __GFP_NOTRACK);
>> + if (!pte_base) {
>> + WARN_ON(1);
>> + continue;
>> + }
>> +
>> + split_large_page((pte_t *)pmd, addr, (pte_t *)pte_base);
>> + __flush_tlb_all();
>> +
>> + spin_lock(&init_mm.page_table_lock);
>> + pmd_populate_kernel(&init_mm, pmd, (pte_t *)pte_base);
>> + spin_unlock(&init_mm.page_table_lock);
>> + }
>> +
>> + vmemmap_pte_remove(pmd, addr, next);
>> + }
>> +
>> + free_pmd_table(pud);
>> + __flush_tlb_all();
>> +}
>> +
>> +static void vmemmap_pud_remove(pgd_t *pgd, unsigned long addr, unsigned long end)
>> +{
>> + unsigned long next;
>> + pud_t *pud;
>> +
>> + pud = pud_offset(pgd, addr);
>> + for (; addr< end; addr = next, pud++) {
>> + next = pud_addr_end(addr, end);
>> + if (pud_none(*pud))
>> + continue;
>> +
>> + vmemmap_pmd_remove(pud, addr, next);
>> + }
>> +
>> + free_pud_table(pgd);
>> + __flush_tlb_all();
>> +}
>> +
>> +void vmemmap_free(struct page *memmap, unsigned long nr_pages)
>> +{
>> + unsigned long addr = (unsigned long)memmap;
>> + unsigned long end = (unsigned long)(memmap + nr_pages);
>> + unsigned long next;
>> +
>> + for (; addr< end; addr = next) {
>> + pgd_t *pgd = pgd_offset_k(addr);
>> +
>> + next = pgd_addr_end(addr, end);
>> + if (!pgd_present(*pgd))
>> + continue;
>> +
>> + vmemmap_pud_remove(pgd, addr, next);
>> + sync_global_pgds(addr, next - 1);
>> + }
>> +}
>> +#endif
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index fac95f2..4060229 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -615,10 +615,11 @@ static inline struct page *kmalloc_section_memmap(unsigned long pnum, int nid,
>> }
>> static void __kfree_section_memmap(struct page *memmap, unsigned long nr_pages)
>> {
>> - return; /* XXX: Not implemented yet */
>> + vmemmap_free(memmap, nr_pages);
>> }
>> static void free_map_bootmem(struct page *page, unsigned long nr_pages)
>
> In the latest kernel, this line was:
> static void free_map_bootmem(struct page *memmap, unsigned long nr_pages)
>
>> {
>> + vmemmap_free(page, nr_pages);
>> }
>> #else
>> static struct page *__kmalloc_section_memmap(unsigned long nr_pages)
>
>
> .
>
^ permalink raw reply
* RE: [PATCH 4/4 v6] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
From: Sethi Varun-B16395 @ 2012-12-04 12:11 UTC (permalink / raw)
To: Tabi Timur-B04825
Cc: joerg.roedel@amd.com, iommu@lists.linux-foundation.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Wood Scott-B07421
In-Reply-To: <50BD056F.7070701@freescale.com>
> -----Original Message-----
> From: Tabi Timur-B04825
> Sent: Tuesday, December 04, 2012 1:33 AM
> To: Sethi Varun-B16395
> Cc: joerg.roedel@amd.com; iommu@lists.linux-foundation.org; linuxppc-
> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Wood Scott-B07421
> Subject: Re: [PATCH 4/4 v6] iommu/fsl: Freescale PAMU driver and IOMMU
> API implementation.
>=20
> Varun Sethi wrote:
>=20
> > + out_be32(&pamu_regs->ppbah, ((u64)ppaact_phys) >> 32);
> > + out_be32(&pamu_regs->ppbal, ppaact_phys);
> > + ppaact_phys =3D ppaact_phys + PAACT_SIZE;
> > + out_be32(&pamu_regs->pplah, ((u64)ppaact_phys) >> 32);
> > + out_be32(&pamu_regs->pplal, ppaact_phys);
>=20
> Instead of ((u64)ppaact_phys) >> 32, use upper_32_bits() and
> lower_32_bits().
>=20
> > +#define PAACE_NUMBER_ENTRIES 0xFF
>=20
> This is going to break with large LIODNs. Instead of hard-coding the
> size of the PPAACT, you need to scan the device tree for the largest
> LIODN, and make the array dynamically sized.
This would in any case change with the new LIODN allocation scheme. I inten=
d on introducing the new scheme as a separate patch.
-Varun
^ permalink raw reply
* RE: [PATCH 3/4 v5] iommu/fsl: Add iommu domain attributes required by fsl PAMU driver.
From: Sethi Varun-B16395 @ 2012-12-04 11:53 UTC (permalink / raw)
To: Wood Scott-B07421
Cc: Tabi Timur-B04825, Joerg Roedel, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org
In-Reply-To: <1354554223.2960.7@snotra>
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Monday, December 03, 2012 10:34 PM
> To: Sethi Varun-B16395
> Cc: Joerg Roedel; linux-kernel@vger.kernel.org; iommu@lists.linux-
> foundation.org; Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; Tabi
> Timur-B04825
> Subject: Re: [PATCH 3/4 v5] iommu/fsl: Add iommu domain attributes
> required by fsl PAMU driver.
>=20
> On 12/03/2012 10:57:29 AM, Sethi Varun-B16395 wrote:
> >
> >
> > > -----Original Message-----
> > > From: iommu-bounces@lists.linux-foundation.org [mailto:iommu-
> > > bounces@lists.linux-foundation.org] On Behalf Of Joerg Roedel
> > > Sent: Sunday, December 02, 2012 7:33 PM
> > > To: Sethi Varun-B16395
> > > Cc: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org;
> > Wood
> > > Scott-B07421; linuxppc-dev@lists.ozlabs.org; Tabi Timur-B04825
> > > Subject: Re: [PATCH 3/4 v5] iommu/fsl: Add iommu domain attributes
> > > required by fsl PAMU driver.
> > >
> > > Hmm, we need to work out a good abstraction for this.
> > >
> > > On Tue, Nov 20, 2012 at 07:24:56PM +0530, Varun Sethi wrote:
> > > > Added the following domain attributes required by FSL PAMU driver:
> > > > 1. Subwindows field added to the iommu domain geometry attribute.
> > >
> > > Are the Subwindows mapped with full size or do you map only parts
> > of the
> > > subwindows?
> > >
> > [Sethi Varun-B16395] It's possible to map a part of the subwindow i.e.
> > size of the mapping can be less than the sub window size.
> >
> > > > + * This attribute indicates number of DMA subwindows
> supported
> > by
> > > > + * the geometry. If there is a single window that maps the
> > entire
> > > > + * geometry, attribute must be set to "1". A value of "0"
> > implies
> > > > + * that this mechanism is not used at all(normal paging is
> > used).
> > > > + * Value other than* "0" or "1" indicates the actual number
> of
> > > > + * subwindows.
> > > > + */
> > >
> > > This semantic is ugly, how about a feature detection mechanism?
> > >
> > [Sethi Varun-B16395] A feature mechanism to query the type of IOMMU?
>=20
> A feature mechanism to determine whether this subwindow mechanism is
> available, and what the limits are.
>=20
So, we use the IOMMU capability interface to find out if IOMMU supports sub=
windows or not, right? But still number of sub windows would be specified =
as a part of the geometry and the valid value for sub windows would 0,1 or=
actual number of sub windows.
-Varun
=20
^ permalink raw reply
* Re: [PATCH 2/4] powerpc: enable ARCH_USE_BUILTIN_BSWAP
From: Stephen Rothwell @ 2012-12-04 11:02 UTC (permalink / raw)
To: David Woodhouse; +Cc: linux-arch, x86, linuxppc-dev
In-Reply-To: <1354616130-21587-3-git-send-email-dwmw2@infradead.org>
[-- Attachment #1: Type: text/plain, Size: 605 bytes --]
Hi David,
On Tue, 4 Dec 2012 10:15:28 +0000 David Woodhouse <dwmw2@infradead.org> wrote:
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index a902a5c..b4ea516 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -78,6 +78,9 @@ config ARCH_HAS_ILOG2_U64
> bool
> default y if 64BIT
>
> +config ARCH_USE_BUILTIN_BSWAP
> + def_bool y
> +
This should be defined as bool in arch/Kconfig (probably in the previous
patch) and then selected from appropriate architectures.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [Patch v4 03/12] memory-hotplug: remove redundant codes
From: Tang Chen @ 2012-12-04 10:31 UTC (permalink / raw)
To: Wen Congyang
Cc: linux-s390, linux-ia64, Len Brown, linux-acpi, linux-sh, x86,
linux-kernel, cmetcalf, Jianguo Wu, linux-mm, Yasuaki Ishimatsu,
paulus, Minchan Kim, KOSAKI Motohiro, David Rientjes, sparclinux,
Christoph Lameter, linuxppc-dev, Andrew Morton, Jiang Liu
In-Reply-To: <50BDC0DE.4010103@cn.fujitsu.com>
On 12/04/2012 05:22 PM, Tang Chen wrote:
> On 11/27/2012 06:00 PM, Wen Congyang wrote:
>> offlining memory blocks and checking whether memory blocks are offlined
>> are very similar. This patch introduces a new function to remove
>> redundant codes.
>>
>> CC: David Rientjes<rientjes@google.com>
>> CC: Jiang Liu<liuj97@gmail.com>
>> CC: Len Brown<len.brown@intel.com>
>> CC: Christoph Lameter<cl@linux.com>
>> Cc: Minchan Kim<minchan.kim@gmail.com>
>> CC: Andrew Morton<akpm@linux-foundation.org>
>> CC: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>> CC: Yasuaki Ishimatsu<isimatu.yasuaki@jp.fujitsu.com>
>> Signed-off-by: Wen Congyang<wency@cn.fujitsu.com>
>
> Can we merge this patch with [PATCH 03/12] ?
Sorry, I think we can merge this patch into [PATCH 02/12].
Thanks. :)
>
> Reviewed-by: Tang Chen <tangchen@cn.fujitsu.com>
>
^ permalink raw reply
* [PATCH 2/4] powerpc: enable ARCH_USE_BUILTIN_BSWAP
From: David Woodhouse @ 2012-12-04 10:15 UTC (permalink / raw)
To: dwmw2; +Cc: linux-arch, x86, linuxppc-dev
In-Reply-To: <1354616130-21587-1-git-send-email-dwmw2@infradead.org>
From: David Woodhouse <David.Woodhouse@intel.com>
By using the compiler intrinsics instead of hand-crafted opaque inline
assembler for byte-swapping, we let the compiler see what's actually
happening and it gets to use lwbrx/stwbrx instructions instead of a
normal load/store coupled with a sequence of rlwimi instructions to
move bits around.
Compiled-tested only. It gave a code size reduction of almost 4% for
ext2, and more like 2.5% for ext3/ext4.
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
arch/powerpc/Kconfig | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index a902a5c..b4ea516 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -78,6 +78,9 @@ config ARCH_HAS_ILOG2_U64
bool
default y if 64BIT
+config ARCH_USE_BUILTIN_BSWAP
+ def_bool y
+
config GENERIC_HWEIGHT
bool
default y
--
1.8.0
^ permalink raw reply related
* [PATCH 1/4] byteorder: allow arch to opt to use GCC intrinsics for byteswapping
From: David Woodhouse @ 2012-12-04 10:15 UTC (permalink / raw)
To: dwmw2; +Cc: linux-arch, x86, linuxppc-dev
In-Reply-To: <1354616130-21587-1-git-send-email-dwmw2@infradead.org>
From: David Woodhouse <David.Woodhouse@intel.com>
Since GCC 4.4, there have been __builtin_bswap32() and __builtin_bswap16()
intrinsics. A __builtin_bswap16() came a little later (4.6 for PowerPC,
48 for other platforms).
By using these instead of the inline assembler that most architectures
have in their __arch_swabXX() macros, we let the compiler see what's
actually happening. The resulting code should be at least as good, and
much *better* in the cases where it can be combined with a nearby load
or store, using a load-and-byteswap or store-and-byteswap instruction
(e.g. lwbrx/stwbrx on PowerPC, movbe on Atom).
When GCC is sufficiently recent *and* the architecture opts in to using
the intrinsics by setting CONFIG_ARCH_USE_BUILTIN_BSWAP, they will be
used in preference to the __arch_swabXX() macros. An architecture which
does not set ARCH_USE_BUILTIN_BSWAP will continue to use its own
hand-crafted macros.
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
include/linux/compiler-gcc4.h | 10 ++++++++++
include/linux/compiler-intel.h | 7 +++++++
include/uapi/linux/swab.h | 12 +++++++++---
3 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 412bc6c..dc16a85 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -63,3 +63,13 @@
#define __compiletime_warning(message) __attribute__((warning(message)))
#define __compiletime_error(message) __attribute__((error(message)))
#endif
+
+#ifdef CONFIG_ARCH_USE_BUILTIN_BSWAP
+#if __GNUC_MINOR__ >= 4
+#define __HAVE_BUILTIN_BSWAP32__
+#define __HAVE_BUILTIN_BSWAP64__
+#endif
+#if __GNUC_MINOR__ >= 8 || (defined(__powerpc__) && __GNUC_MINOR__ >= 6)
+#define __HAVE_BUILTIN_BSWAP16__
+#endif
+#endif
diff --git a/include/linux/compiler-intel.h b/include/linux/compiler-intel.h
index d8e636e..973ce10 100644
--- a/include/linux/compiler-intel.h
+++ b/include/linux/compiler-intel.h
@@ -29,3 +29,10 @@
#endif
#define uninitialized_var(x) x
+
+#ifndef __HAVE_BUILTIN_BSWAP16__
+/* icc has this, but it's called _bswap16 */
+#define __HAVE_BUILTIN_BSWAP16__
+#define __builtin_bswap16 _bswap16
+#endif
+
diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
index e811474..0e011eb 100644
--- a/include/uapi/linux/swab.h
+++ b/include/uapi/linux/swab.h
@@ -45,7 +45,9 @@
static inline __attribute_const__ __u16 __fswab16(__u16 val)
{
-#ifdef __arch_swab16
+#ifdef __HAVE_BUILTIN_BSWAP16__
+ return __builtin_bswap16(val);
+#elif defined (__arch_swab16)
return __arch_swab16(val);
#else
return ___constant_swab16(val);
@@ -54,7 +56,9 @@ static inline __attribute_const__ __u16 __fswab16(__u16 val)
static inline __attribute_const__ __u32 __fswab32(__u32 val)
{
-#ifdef __arch_swab32
+#ifdef __HAVE_BUILTIN_BSWAP32__
+ return __builtin_bswap32(val);
+#elif defined(__arch_swab32)
return __arch_swab32(val);
#else
return ___constant_swab32(val);
@@ -63,7 +67,9 @@ static inline __attribute_const__ __u32 __fswab32(__u32 val)
static inline __attribute_const__ __u64 __fswab64(__u64 val)
{
-#ifdef __arch_swab64
+#ifdef __HAVE_BUILTIN_BSWAP64__
+ return __builtin_bswap64(val);
+#elif defined (__arch_swab64)
return __arch_swab64(val);
#elif defined(__SWAB_64_THRU_32__)
__u32 h = val >> 32;
--
1.8.0
^ permalink raw reply related
* [PATCH 4/4] x86: add CONFIG_X86_MOVBE option
From: David Woodhouse @ 2012-12-04 10:15 UTC (permalink / raw)
To: dwmw2; +Cc: linux-arch, x86, linuxppc-dev
In-Reply-To: <1354616130-21587-1-git-send-email-dwmw2@infradead.org>
From: David Woodhouse <David.Woodhouse@intel.com>
Currently depends only on CONFIG_MATOM. This will change because big-core
CPUs are getting movbe too...
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
arch/x86/Kconfig.cpu | 4 ++++
arch/x86/Makefile | 1 +
2 files changed, 5 insertions(+)
diff --git a/arch/x86/Kconfig.cpu b/arch/x86/Kconfig.cpu
index f3b86d0..969f7a6 100644
--- a/arch/x86/Kconfig.cpu
+++ b/arch/x86/Kconfig.cpu
@@ -353,6 +353,10 @@ config X86_BSWAP
def_bool y
depends on X86_32 && !M386
+config X86_MOVBE
+ def_bool y
+ depends on MATOM
+
config X86_POPAD_OK
def_bool y
depends on X86_32 && !M386
diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 05afcca..0e71d76 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -64,6 +64,7 @@ else
$(call cc-option,-march=core2,$(call cc-option,-mtune=generic))
cflags-$(CONFIG_MATOM) += $(call cc-option,-march=atom) \
$(call cc-option,-mtune=atom,$(call cc-option,-mtune=generic))
+ cflags-$(CONFIG_X86_MOVBE) += $(call cc-option,-mmovbe)
cflags-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=generic)
KBUILD_CFLAGS += $(cflags-y)
--
1.8.0
^ 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