* Re: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommu implementation.
From: Alex Williamson @ 2013-06-26 19:14 UTC (permalink / raw)
To: Sethi Varun-B16395
Cc: Wood Scott-B07421, Timur Tabi, joro@8bytes.org,
linux-kernel@vger.kernel.org, Yoder Stuart-B08248,
iommu@lists.linux-foundation.org, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <C5ECD7A89D1DC44195F34B25E172658D5DD70C@039-SN2MPN1-011.039d.mgd.msft.net>
On Wed, 2013-06-26 at 06:24 +0000, Sethi Varun-B16395 wrote:
>
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Tuesday, June 25, 2013 10:27 AM
> > To: Sethi Varun-B16395
> > Cc: joro@8bytes.org; iommu@lists.linux-foundation.org; linuxppc-
> > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> > benh@kernel.crashing.org; galak@kernel.crashing.org; Yoder Stuart-B08248;
> > Wood Scott-B07421; Timur Tabi
> > Subject: Re: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommu
> > implementation.
> >
> > On Thu, 2013-06-20 at 21:31 +0530, Varun Sethi wrote:
> >
> > > +#define REQ_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR |
> > PCI_ACS_UF)
> > > +
> > > +static struct iommu_group *get_device_iommu_group(struct device *dev)
> > > +{
> > > + struct iommu_group *group;
> > > +
> > > + group = iommu_group_get(dev);
> > > + if (!group)
> > > + group = iommu_group_alloc();
> > > +
> > > + return group;
> > > +}
> > > +
> > [snip]
> > > +
> >
> > This really gets parent or peer, right?
> >
> > > +static struct iommu_group *get_peer_pci_device_group(struct pci_dev
> > > +*pdev) {
> > > + struct iommu_group *group = NULL;
> > > +
> > > + /* check if this is the first device on the bus*/
> > > + if (pdev->bus_list.next == pdev->bus_list.prev) {
> >
> > It's a list_head, use list functions. The list implementation should be
> > treated as opaque.
> >
> > if (list_is_singular(&pdev->bus_list))
> >
> > > + struct pci_bus *bus = pdev->bus->parent;
> > > + /* Traverese the parent bus list to get
> > > + * pdev & dev for the sibling device.
> > > + */
> > > + while (bus) {
> > > + if (!list_empty(&bus->devices)) {
> > > + pdev = container_of(bus->devices.next,
> > > + struct pci_dev, bus_list);
> >
> > pdev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
> >
> > > + group = iommu_group_get(&pdev->dev);
> > > + break;
> > > + } else
> > > + bus = bus->parent;
> >
> > Is this ever reached? Don't you always have bus->self?
> >
> [Sethi Varun-B16395] Not sure I understand. Trying to get the group
> information from the parent bus, if there are no sibling devices on
> the current bus.
I assume there's always a bridge on a bus, but maybe that bridge
(parent->self) is not in the list of parent->devices? Is that the case?
If not, then there's always a device on the bus, the bridge that created
it.
> > > + }
> > > + } else {
> > > + /*
> > > + * Get the pdev & dev for the sibling device
> > > + */
> > > + pdev = container_of(pdev->bus_list.prev,
> > > + struct pci_dev, bus_list);
> >
> > How do you know if you're at the head or tail of the list?
> >
> > struct pci_dev *tmp;
> > list_for_each_entry(tmp, &pdev->bus_list, bus_list) {
> > if (tmp == pdev)
> > continue;
> >
> > group = iommu_group_get(&tmp->dev);
> > break;
> > }
> >
> > > + group = iommu_group_get(&pdev->dev);
> > > + }
> > > +
> > > + return group;
> > > +}
> > > +
> > > +static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
> > > +{
> > > + struct iommu_group *group = NULL;
> > > + struct pci_dev *bridge, *dma_pdev = NULL;
> > > + struct pci_controller *pci_ctl;
> > > + bool pci_endpt_partioning;
> > > +
> > > + pci_ctl = pci_bus_to_host(pdev->bus);
> > > + pci_endpt_partioning = check_pci_ctl_endpt_part(pci_ctl);
> > > + /* We can partition PCIe devices so assign device group to the
> > device */
> > > + if (pci_endpt_partioning) {
> > > + bridge = pci_find_upstream_pcie_bridge(pdev);
> > > + if (bridge) {
> > > + if (pci_is_pcie(bridge))
> > > + dma_pdev = pci_get_domain_bus_and_slot(
> > > + pci_domain_nr(pdev->bus),
> > > + bridge->subordinate->number, 0);
> > > + if (!dma_pdev)
> > > + dma_pdev = pci_dev_get(bridge);
> > > + } else
> > > + dma_pdev = pci_dev_get(pdev);
> > > +
> > > + /* Account for quirked devices */
> > > + swap_pci_ref(&dma_pdev, pci_get_dma_source(dma_pdev));
> > > +
> > > + /*
> > > + * If it's a multifunction device that does not support our
> > > + * required ACS flags, add to the same group as function 0.
> > > + */
> >
> > See c14d2690 in Joerg's next tree, using function 0 was a poor
> > assumption.
> [Sethi Varun-B16395] ok.
>
> >
> > > + if (dma_pdev->multifunction &&
> > > + !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS))
> > > + swap_pci_ref(&dma_pdev,
> > > + pci_get_slot(dma_pdev->bus,
> > > + PCI_DEVFN(PCI_SLOT(dma_pdev-
> > >devfn),
> > > + 0)));
> > > +
> > > + group = get_device_iommu_group(&pdev->dev);
> > > + pci_dev_put(pdev);
> >
> > What was the point of all the above if we use pdev here instead of
> > dma_pdev? Wrong device and broken reference counting.
> [Sethi Varun-B16395] Will fix this
>
> This also isn't
> > testing ACS all the way up to the root complex or controller.
> [Sethi Varun-B16395] In our case the IOMMU can differentiate
> transactions based on the LIODN. The PCIe controller can generate a
> unique LIODN based on the bus,device,function number. I believe this
> would even be true for devices connected to a PCIe bridge (and not on
> the root bus). So, do we still need to check for ACS up to the root
> node?
ACS is the PCI capability that tells us whether a device allows
peer-to-peer or requires all DMA to be forwarded upstream. At any step
along the way from an endpoint to the IOMMU the transaction can be
re-routed. We therefore need to test the ACS property not only of the
endpoint, but every device between the endpoint and the IOMMU to form
the IOMMU groups. An IOMMU cannot isolate a device if a DMA access is
re-routed by a link in the topology that prevents the IOMMU from even
being part of the transaction. Thanks,
Alex
^ permalink raw reply
* Re: powerpc/85xx: Move ePAPR paravirt initialization earlier
From: Scott Wood @ 2013-06-26 18:56 UTC (permalink / raw)
To: Tudor Laurentiu; +Cc: linuxppc-dev
In-Reply-To: <51CAE047.8030605@freescale.com>
On 06/26/2013 07:36:23 AM, Tudor Laurentiu wrote:
> On 06/26/2013 02:14 AM, Scott Wood wrote:
>> This would require converting
>> the code to use the early device tree functions.
>>=20
>=20
> I see. Had a look at that api and it seems pretty limited.
> I couldn't find a simple way of reading a property other than =20
> scanning the entire tree with of_scan_flat_dt().
What's wrong with scanning the entire tree? The data format doesn't =20
allow any more efficient type of lookup (it was designed to be simple =20
and small, not fast).
-Scott=
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Srivatsa S. Bhat @ 2013-06-26 18:28 UTC (permalink / raw)
To: Tejun Heo
Cc: peterz, fweisbec, linux-kernel, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, Steven Rostedt, namhyung, tglx, laijs, zhong,
netdev, oleg, sbw, David Laight, akpm, linuxppc-dev
In-Reply-To: <20130626172948.GD4405@mtj.dyndns.org>
On 06/26/2013 10:59 PM, Tejun Heo wrote:
> Hello,
>
> On Wed, Jun 26, 2013 at 11:33:43AM -0400, Steven Rostedt wrote:
>> I thought the whole deal with this patchset was to remove stop_machine
>> from CPU hotplug. Why halt all CPUs just to remove one? stomp_machine()
>> is extremely intrusive for the entire system, where as one CPU making
>> sure all CPUs schedule isn't very intrusive at all.
>>
>> I didn't think the idea of this patch set was to make CPU hotplug
>> faster, just less intrusive to the system.
>
> Yeap, removal of stop_machine is a great improvement in itself.
Absolutely. To make hotplug less intrusive on the system.
> ISTR
> mentions of hot-unplug latency but I could be mistaken. Srivatsa, can
> you please chime in on that?
>
Yes, we were discussing hot-unplug latency for use-cases such as
suspend/resume. We didn't want to make those operations slower in the
process of removing stop_machine() from hotplug.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Srivatsa S. Bhat @ 2013-06-26 18:22 UTC (permalink / raw)
To: Tejun Heo
Cc: peterz, fweisbec, oleg, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, Steven Rostedt, namhyung, tglx, laijs, zhong,
netdev, linux-kernel, sbw, David Laight, akpm, linuxppc-dev
In-Reply-To: <20130626152148.GA4405@mtj.dyndns.org>
On 06/26/2013 08:51 PM, Tejun Heo wrote:
> On Wed, Jun 26, 2013 at 10:51:11AM -0400, Steven Rostedt wrote:
>>> It would also increase the latency of CPU-hotunplug operations.
>>
>> Is that a big deal?
>
> I thought that was the whole deal with this patchset - making cpu
> hotunplugs lighter and faster mostly for powersaving. That said, just
> removing stop_machine call would be a pretty good deal and I don't
> know how meaningful reducing CPU hotunplug latency is. Srivatsa?
>
Keeping the hotunplug latency is important for suspend/resume, where
we take all non-boot CPUs in a loop. That's an interesting use-case
where intrusiveness doesn't matter much, but latency does. So yes,
making CPU hotplug faster is also one of the goals of this patchset.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Srivatsa S. Bhat @ 2013-06-26 18:18 UTC (permalink / raw)
To: David Laight
Cc: peterz, fweisbec, oleg, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, rostedt, namhyung, tglx, laijs, zhong, netdev,
linux-kernel, sbw, tj, akpm, linuxppc-dev
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B72AA@saturn3.aculab.com>
On 06/26/2013 07:59 PM, David Laight wrote:
>> Once stop_machine() is gone from the CPU offline path, we won't be able
>> to depend on disabling preemption to prevent CPUs from going offline
>> from under us.
>
> Could you use an rcu-like sequence so that disabling pre-emption
> would be enough?
>
> Something like rebuilding the cpu list, then forcing yourself
> to run on all the cpu.
>
Certainly, and we had debated schemes similar to that (including
schemes invoking synchronize_sched() itself) in earlier discussions.
(But IIRC even those schemes required converting call-sites from
preempt_disable() to get/put_online_cpus_atomic(), to properly
synchronize).
> That would be far less intrusive.
>
But that would increase the latency of hotplug operations like Paul
pointed out, and that in turn is not good for use-cases such as
suspend/resume, where we take all non-boot CPUs offline in a loop.
(That would mean executing the above logic num_online_cpus() times!).
So we started hunting for ideas that can make the hotplug writer
side not only less intrusive, but also remain fast. That's how
the later designs evolved.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Tejun Heo @ 2013-06-26 17:29 UTC (permalink / raw)
To: Steven Rostedt
Cc: peterz, fweisbec, oleg, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, namhyung, tglx, laijs, zhong, netdev,
linux-kernel, sbw, David Laight, Srivatsa S. Bhat, akpm,
linuxppc-dev
In-Reply-To: <1372260823.18733.263.camel@gandalf.local.home>
Hello,
On Wed, Jun 26, 2013 at 11:33:43AM -0400, Steven Rostedt wrote:
> I thought the whole deal with this patchset was to remove stop_machine
> from CPU hotplug. Why halt all CPUs just to remove one? stomp_machine()
> is extremely intrusive for the entire system, where as one CPU making
> sure all CPUs schedule isn't very intrusive at all.
>
> I didn't think the idea of this patch set was to make CPU hotplug
> faster, just less intrusive to the system.
Yeap, removal of stop_machine is a great improvement in itself. ISTR
mentions of hot-unplug latency but I could be mistaken. Srivatsa, can
you please chime in on that?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Steven Rostedt @ 2013-06-26 15:33 UTC (permalink / raw)
To: Tejun Heo
Cc: peterz, fweisbec, oleg, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, namhyung, tglx, laijs, zhong, netdev,
linux-kernel, sbw, David Laight, Srivatsa S. Bhat, akpm,
linuxppc-dev
In-Reply-To: <20130626152148.GA4405@mtj.dyndns.org>
On Wed, 2013-06-26 at 08:21 -0700, Tejun Heo wrote:
> On Wed, Jun 26, 2013 at 10:51:11AM -0400, Steven Rostedt wrote:
> > > It would also increase the latency of CPU-hotunplug operations.
> >
> > Is that a big deal?
>
> I thought that was the whole deal with this patchset - making cpu
> hotunplugs lighter and faster mostly for powersaving. That said, just
> removing stop_machine call would be a pretty good deal and I don't
> know how meaningful reducing CPU hotunplug latency is. Srivatsa?
I thought the whole deal with this patchset was to remove stop_machine
from CPU hotplug. Why halt all CPUs just to remove one? stomp_machine()
is extremely intrusive for the entire system, where as one CPU making
sure all CPUs schedule isn't very intrusive at all.
I didn't think the idea of this patch set was to make CPU hotplug
faster, just less intrusive to the system.
-- Steve
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Tejun Heo @ 2013-06-26 15:21 UTC (permalink / raw)
To: Steven Rostedt
Cc: peterz, fweisbec, oleg, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, namhyung, tglx, laijs, zhong, netdev,
linux-kernel, sbw, David Laight, Srivatsa S. Bhat, akpm,
linuxppc-dev
In-Reply-To: <1372258271.18733.256.camel@gandalf.local.home>
On Wed, Jun 26, 2013 at 10:51:11AM -0400, Steven Rostedt wrote:
> > It would also increase the latency of CPU-hotunplug operations.
>
> Is that a big deal?
I thought that was the whole deal with this patchset - making cpu
hotunplugs lighter and faster mostly for powersaving. That said, just
removing stop_machine call would be a pretty good deal and I don't
know how meaningful reducing CPU hotunplug latency is. Srivatsa?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Steven Rostedt @ 2013-06-26 14:51 UTC (permalink / raw)
To: paulmck
Cc: peterz, fweisbec, oleg, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, nikunj, linux-pm, rusty,
namhyung, tglx, laijs, zhong, netdev, linux-kernel, sbw,
David Laight, Srivatsa S. Bhat, tj, akpm, linuxppc-dev
In-Reply-To: <20130626143424.GN3828@linux.vnet.ibm.com>
On Wed, 2013-06-26 at 07:34 -0700, Paul E. McKenney wrote:
> On Wed, Jun 26, 2013 at 03:29:40PM +0100, David Laight wrote:
> > > Once stop_machine() is gone from the CPU offline path, we won't be able
> > > to depend on disabling preemption to prevent CPUs from going offline
> > > from under us.
> >
> > Could you use an rcu-like sequence so that disabling pre-emption
> > would be enough?
> >
> > Something like rebuilding the cpu list, then forcing yourself
> > to run on all the cpu.
> >
> > That would be far less intrusive.
>
> It would also increase the latency of CPU-hotunplug operations.
>
Is that a big deal?
-- Steve
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Paul E. McKenney @ 2013-06-26 14:45 UTC (permalink / raw)
To: David Laight
Cc: peterz, fweisbec, oleg, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, nikunj, linux-pm, rusty,
rostedt, namhyung, tglx, laijs, zhong, netdev, linux-kernel, sbw,
Srivatsa S. Bhat, tj, akpm, linuxppc-dev
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B72AA@saturn3.aculab.com>
On Wed, Jun 26, 2013 at 03:29:40PM +0100, David Laight wrote:
> > Once stop_machine() is gone from the CPU offline path, we won't be able
> > to depend on disabling preemption to prevent CPUs from going offline
> > from under us.
>
> Could you use an rcu-like sequence so that disabling pre-emption
> would be enough?
>
> Something like rebuilding the cpu list, then forcing yourself
> to run on all the cpu.
>
> That would be far less intrusive.
Maybe... I must defer to Srivatsa on this question.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Paul E. McKenney @ 2013-06-26 14:33 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: peterz, fweisbec, linux-kernel, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, nikunj, linux-pm, rusty,
rostedt, namhyung, tglx, laijs, zhong, netdev, oleg, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <51CAF624.6060004@linux.vnet.ibm.com>
On Wed, Jun 26, 2013 at 07:39:40PM +0530, Srivatsa S. Bhat wrote:
> On 06/26/2013 03:30 AM, Paul E. McKenney wrote:
> > On Wed, Jun 26, 2013 at 01:57:55AM +0530, Srivatsa S. Bhat wrote:
> >> Once stop_machine() is gone from the CPU offline path, we won't be able
> >> to depend on disabling preemption to prevent CPUs from going offline
> >> from under us.
> >>
> >> In RCU code, rcu_implicit_dynticks_qs() checks if a CPU is offline,
> >> while being protected by a spinlock. Use the get/put_online_cpus_atomic()
> >> APIs to prevent CPUs from going offline, while invoking from atomic context.
> >
> > I am not completely sure that this is needed. Here is a (quite possibly
> > flawed) argument for its not being needed:
> >
> > o rcu_gp_init() holds off CPU-hotplug operations during
> > grace-period initialization. Therefore, RCU will avoid
> > looking for quiescent states from CPUs that were offline
> > (and thus in an extended quiescent state) at the beginning
> > of the grace period.
> >
> > o If force_qs_rnp() is looking for a quiescent state from
> > a given CPU, and if it senses that CPU as being offline,
> > then even without synchronization we know that the CPU
> > was offline some time during the current grace period.
> >
> > After all, it was online at the beginning of the grace
> > period (otherwise, we would not be looking at it at all),
> > and our later sampling of its state must have therefore
> > happened after the start of the grace period. Given that
> > the grace period has not yet ended, it also has to happened
> > before the end of the grace period.
> >
> > o Therefore, we should be able to sample the offline state
> > without synchronization.
> >
>
> Thanks a lot for explaining the synchronization design in detail, Paul!
> I agree that get/put_online_cpus_atomic() is not necessary here.
>
> Regarding the debug checks under CONFIG_DEBUG_HOTPLUG_CPU, to avoid
> false-positives, I'm thinking of introducing a few _nocheck() variants,
> on a case-by-case basis, like cpu_is_offline_nocheck() (useful here in RCU)
> and for_each_online_cpu_nocheck() (useful in percpu-counter code, as
> pointed out by Tejun Heo). These fine synchronization details are kinda
> hard to encapsulate in that debug logic, so we can use the _nocheck()
> variants here to avoid getting splats when running with DEBUG_HOTPLUG_CPU
> enabled.
Good point, and seems like a reasonable approach to me.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Paul E. McKenney @ 2013-06-26 14:34 UTC (permalink / raw)
To: David Laight
Cc: peterz, fweisbec, oleg, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, nikunj, linux-pm, rusty,
rostedt, namhyung, tglx, laijs, zhong, netdev, linux-kernel, sbw,
Srivatsa S. Bhat, tj, akpm, linuxppc-dev
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B72AA@saturn3.aculab.com>
On Wed, Jun 26, 2013 at 03:29:40PM +0100, David Laight wrote:
> > Once stop_machine() is gone from the CPU offline path, we won't be able
> > to depend on disabling preemption to prevent CPUs from going offline
> > from under us.
>
> Could you use an rcu-like sequence so that disabling pre-emption
> would be enough?
>
> Something like rebuilding the cpu list, then forcing yourself
> to run on all the cpu.
>
> That would be far less intrusive.
It would also increase the latency of CPU-hotunplug operations.
Thanx, Paul
^ permalink raw reply
* RE: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: David Laight @ 2013-06-26 14:29 UTC (permalink / raw)
To: Srivatsa S. Bhat, paulmck
Cc: peterz, fweisbec, oleg, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, nikunj, linux-pm, rusty,
rostedt, namhyung, tglx, laijs, zhong, netdev, linux-kernel, sbw,
tj, akpm, linuxppc-dev
In-Reply-To: <51CAF624.6060004@linux.vnet.ibm.com>
> Once stop_machine() is gone from the CPU offline path, we won't be =
able
> to depend on disabling preemption to prevent CPUs from going offline
> from under us.
Could you use an rcu-like sequence so that disabling pre-emption
would be enough?
Something like rebuilding the cpu list, then forcing yourself
to run on all the cpu.
That would be far less intrusive.
David
^ permalink raw reply
* Re: linux-next: manual merge of the akpm tree with the powerpc tree
From: Oleg Nesterov @ 2013-06-26 14:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, Michael Neuling, linux-kernel, linux-next,
Andrew Morton, linuxppc-dev
In-Reply-To: <1372234231.18612.6.camel@pasglop>
On 06/26, Benjamin Herrenschmidt wrote:
>
> On Wed, 2013-06-26 at 16:56 +1000, Stephen Rothwell wrote:
> > Today's linux-next merge of the akpm tree got a conflict in
> > arch/powerpc/kernel/ptrace.c between commit b0b0aa9c7faf
> > ("powerpc/hw_brk: Fix setting of length for exact mode breakpoints") from
> > the powerpc tree and commit "ptrace/powerpc: revert "hw_breakpoints: Fix
> > racy access to ptrace breakpoints"" from the akpm tree.
> >
> > I fixed it up (see below) and can carry the fix as necessary (no action
> > is required).
>
> Where does the latter come from ?
ptrace-powerpc-revert-hw_breakpoints-fix-racy-access-to-ptrace-breakpoints.patch
attached below.
You were cc'ed every time ;)
> Why didn't it go through the powerpc tree ?
Because this series needs to update any user of ptrace_get/put_breakpoints
in arch/ (simply remove these calls), then change the core kernel code, then
fix arch/86.
------------------------------------------------------
From: Oleg Nesterov <oleg@redhat.com>
Subject: ptrace/powerpc: revert "hw_breakpoints: Fix racy access to ptrace breakpoints"
This reverts commit 07fa7a0a8a586 ("hw_breakpoints: Fix racy access to
ptrace breakpoints") and removes ptrace_get/put_breakpoints() added by
other commits.
The patch was fine but we can no longer race with SIGKILL after 9899d11f
("ptrace: ensure arch_ptrace/ptrace_request can never race with SIGKILL"),
the __TASK_TRACED tracee can't be woken up and ->ptrace_bps[] can't go
away.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Michael Neuling <mikey@neuling.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jan Kratochvil <jan.kratochvil@redhat.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
arch/powerpc/kernel/ptrace.c | 20 --------------------
1 file changed, 20 deletions(-)
diff -puN arch/powerpc/kernel/ptrace.c~ptrace-powerpc-revert-hw_breakpoints-fix-racy-access-to-ptrace-breakpoints arch/powerpc/kernel/ptrace.c
--- a/arch/powerpc/kernel/ptrace.c~ptrace-powerpc-revert-hw_breakpoints-fix-racy-access-to-ptrace-breakpoints
+++ a/arch/powerpc/kernel/ptrace.c
@@ -974,16 +974,12 @@ int ptrace_set_debugreg(struct task_stru
hw_brk.type = (data & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
hw_brk.len = 8;
#ifdef CONFIG_HAVE_HW_BREAKPOINT
- if (ptrace_get_breakpoints(task) < 0)
- return -ESRCH;
-
bp = thread->ptrace_bps[0];
if ((!data) || !(hw_brk.type & HW_BRK_TYPE_RDWR)) {
if (bp) {
unregister_hw_breakpoint(bp);
thread->ptrace_bps[0] = NULL;
}
- ptrace_put_breakpoints(task);
return 0;
}
if (bp) {
@@ -996,11 +992,9 @@ int ptrace_set_debugreg(struct task_stru
ret = modify_user_hw_breakpoint(bp, &attr);
if (ret) {
- ptrace_put_breakpoints(task);
return ret;
}
thread->ptrace_bps[0] = bp;
- ptrace_put_breakpoints(task);
thread->hw_brk = hw_brk;
return 0;
}
@@ -1015,12 +1009,9 @@ int ptrace_set_debugreg(struct task_stru
ptrace_triggered, NULL, task);
if (IS_ERR(bp)) {
thread->ptrace_bps[0] = NULL;
- ptrace_put_breakpoints(task);
return PTR_ERR(bp);
}
- ptrace_put_breakpoints(task);
-
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
task->thread.hw_brk = hw_brk;
#else /* CONFIG_PPC_ADV_DEBUG_REGS */
@@ -1439,9 +1430,6 @@ static long ppc_set_hwdebug(struct task_
if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
brk.type |= HW_BRK_TYPE_WRITE;
#ifdef CONFIG_HAVE_HW_BREAKPOINT
- if (ptrace_get_breakpoints(child) < 0)
- return -ESRCH;
-
/*
* Check if the request is for 'range' breakpoints. We can
* support it if range < 8 bytes.
@@ -1449,12 +1437,10 @@ static long ppc_set_hwdebug(struct task_
if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE) {
len = bp_info->addr2 - bp_info->addr;
} else if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT) {
- ptrace_put_breakpoints(child);
return -EINVAL;
}
bp = thread->ptrace_bps[0];
if (bp) {
- ptrace_put_breakpoints(child);
return -ENOSPC;
}
@@ -1468,11 +1454,9 @@ static long ppc_set_hwdebug(struct task_
ptrace_triggered, NULL, child);
if (IS_ERR(bp)) {
thread->ptrace_bps[0] = NULL;
- ptrace_put_breakpoints(child);
return PTR_ERR(bp);
}
- ptrace_put_breakpoints(child);
return 1;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
@@ -1516,16 +1500,12 @@ static long ppc_del_hwdebug(struct task_
return -EINVAL;
#ifdef CONFIG_HAVE_HW_BREAKPOINT
- if (ptrace_get_breakpoints(child) < 0)
- return -ESRCH;
-
bp = thread->ptrace_bps[0];
if (bp) {
unregister_hw_breakpoint(bp);
thread->ptrace_bps[0] = NULL;
} else
ret = -ENOENT;
- ptrace_put_breakpoints(child);
return ret;
#else /* CONFIG_HAVE_HW_BREAKPOINT */
if (child->thread.hw_brk.address == 0)
_
Patches currently in -mm which might be from oleg@redhat.com are
posix_cpu_timer-consolidate-expiry-time-type.patch
posix_cpu_timers-consolidate-timer-list-cleanups.patch
posix_cpu_timers-consolidate-expired-timers-check.patch
posix-timers-correctly-get-dying-task-time-sample-in-posix_cpu_timer_schedule.patch
posix_timers-fix-racy-timer-delta-caching-on-task-exit.patch
lockdep-introduce-lock_acquire_exclusive-shared-helper-macros.patch
lglock-update-lockdep-annotations-to-report-recursive-local-locks.patch
autofs4-allow-autofs-to-work-outside-the-initial-pid-namespace.patch
autofs4-translate-pids-to-the-right-namespace-for-the-daemon.patch
ptrace-x86-revert-hw_breakpoints-fix-racy-access-to-ptrace-breakpoints.patch
ptrace-powerpc-revert-hw_breakpoints-fix-racy-access-to-ptrace-breakpoints.patch
ptrace-arm-revert-hw_breakpoints-fix-racy-access-to-ptrace-breakpoints.patch
ptrace-sh-revert-hw_breakpoints-fix-racy-access-to-ptrace-breakpoints.patch
ptrace-revert-prepare-to-fix-racy-accesses-on-task-breakpoints.patch
ptrace-x86-simplify-the-disable-logic-in-ptrace_write_dr7.patch
ptrace-x86-dont-delay-disable-till-second-pass-in-ptrace_write_dr7.patch
ptrace-x86-introduce-ptrace_register_breakpoint.patch
ptrace-x86-ptrace_write_dr7-should-create-bp-if-disabled.patch
ptrace-x86-cleanup-ptrace_set_debugreg.patch
ptrace-ptrace_detach-should-do-flush_ptrace_hw_breakpointchild.patch
ptrace-x86-flush_ptrace_hw_breakpoint-shoule-clear-the-virtual-debug-registers.patch
x86-kill-tif_debug.patch
^ permalink raw reply
* Re: Regression in RCU subsystem in latest mainline kernel
From: Paul E. McKenney @ 2013-06-26 14:16 UTC (permalink / raw)
To: Michael Ellerman
Cc: Rojhalat Ibrahim, linuxppc-dev, linux-kernel, Steven Rostedt
In-Reply-To: <20130626081057.GB10796@concordia>
On Wed, Jun 26, 2013 at 06:10:58PM +1000, Michael Ellerman wrote:
> On Tue, Jun 25, 2013 at 09:03:32AM -0700, Paul E. McKenney wrote:
> > On Tue, Jun 25, 2013 at 05:44:23PM +1000, Michael Ellerman wrote:
> > > On Tue, Jun 25, 2013 at 05:19:14PM +1000, Michael Ellerman wrote:
> > > >
> > > > Here's another trace from 3.10-rc7 plus a few local patches.
> > >
> > > And here's another with CONFIG_RCU_CPU_STALL_INFO=y in case that's useful:
> > >
> > > PASS running test_pmc5_6_overuse()
> > > INFO: rcu_sched self-detected stall on CPU
> > > 8: (1 GPs behind) idle=8eb/140000000000002/0 softirq=215/220
> >
> > So this CPU has been out of action since before the beginning of the
> > current grace period ("1 GPs behind"). It is not idle, having taken
> > a pair of nested interrupts from process context (matching the stack
> > below). This CPU has take five softirqs since the last grace period
> > that it noticed, which makes it likely that the loop is within the
> > softirq handler.
> >
> > > (t=2100 jiffies g=18446744073709551583 c=18446744073709551582 q=13)
> >
> > Assuming HZ=100, this stall has been going on for 21 seconds. There
> > is a grace period in progress according to RCU's global state (which
> > this CPU is not yet aware of). There are a total of 13 RCU callbacks
> > queued across the entire system.
> >
> > If the system is at all responsive, I suggest using ftrace (either from
> > the boot command line or at runtime) to trace __do_softirq() and
> > hrtimer_interrupt().
>
> Thanks for decoding it Paul.
>
> I've narrowed down the test case and I think this is probably just a
> case of too many perf interrupts. If I reduce the sampling period by
> half the test runs fine.
>
> There is logic in perf to detect an interrupt storm, but for some reason
> it's not saving us. I'll dig in there, but I don't think it's an RCU
> problem.
Whew! ;-)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Srivatsa S. Bhat @ 2013-06-26 14:09 UTC (permalink / raw)
To: paulmck
Cc: peterz, fweisbec, linux-kernel, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, nikunj, linux-pm, rusty,
rostedt, namhyung, tglx, laijs, zhong, netdev, oleg, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <20130625220026.GG3828@linux.vnet.ibm.com>
On 06/26/2013 03:30 AM, Paul E. McKenney wrote:
> On Wed, Jun 26, 2013 at 01:57:55AM +0530, Srivatsa S. Bhat wrote:
>> Once stop_machine() is gone from the CPU offline path, we won't be able
>> to depend on disabling preemption to prevent CPUs from going offline
>> from under us.
>>
>> In RCU code, rcu_implicit_dynticks_qs() checks if a CPU is offline,
>> while being protected by a spinlock. Use the get/put_online_cpus_atomic()
>> APIs to prevent CPUs from going offline, while invoking from atomic context.
>
> I am not completely sure that this is needed. Here is a (quite possibly
> flawed) argument for its not being needed:
>
> o rcu_gp_init() holds off CPU-hotplug operations during
> grace-period initialization. Therefore, RCU will avoid
> looking for quiescent states from CPUs that were offline
> (and thus in an extended quiescent state) at the beginning
> of the grace period.
>
> o If force_qs_rnp() is looking for a quiescent state from
> a given CPU, and if it senses that CPU as being offline,
> then even without synchronization we know that the CPU
> was offline some time during the current grace period.
>
> After all, it was online at the beginning of the grace
> period (otherwise, we would not be looking at it at all),
> and our later sampling of its state must have therefore
> happened after the start of the grace period. Given that
> the grace period has not yet ended, it also has to happened
> before the end of the grace period.
>
> o Therefore, we should be able to sample the offline state
> without synchronization.
>
Thanks a lot for explaining the synchronization design in detail, Paul!
I agree that get/put_online_cpus_atomic() is not necessary here.
Regarding the debug checks under CONFIG_DEBUG_HOTPLUG_CPU, to avoid
false-positives, I'm thinking of introducing a few _nocheck() variants,
on a case-by-case basis, like cpu_is_offline_nocheck() (useful here in RCU)
and for_each_online_cpu_nocheck() (useful in percpu-counter code, as
pointed out by Tejun Heo). These fine synchronization details are kinda
hard to encapsulate in that debug logic, so we can use the _nocheck()
variants here to avoid getting splats when running with DEBUG_HOTPLUG_CPU
enabled.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v2 38/45] MIPS: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Ralf Baechle @ 2013-06-26 13:39 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: linux-mips, David Daney, peterz, fweisbec, linux-kernel, walken,
mingo, linux-arch, vincent.guittot, xiaoguangrong, Steven J. Hill,
wangyun, paulmck, nikunj, linux-pm, rusty, rostedt, Yong Zhang,
namhyung, tglx, Florian Fainelli, John Crispin, laijs, zhong,
Sanjay Lal, netdev, oleg, sbw, tj, akpm, linuxppc-dev
In-Reply-To: <20130625203257.16593.15358.stgit@srivatsabhat.in.ibm.com>
On Wed, Jun 26, 2013 at 02:02:57AM +0530, Srivatsa S. Bhat wrote:
> Once stop_machine() is gone from the CPU offline path, we won't be able
> to depend on disabling preemption to prevent CPUs from going offline
> from under us.
>
> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going
> offline, while invoking from atomic context.
I think the same change also needs to be applied to r4k_on_each_cpu() in
arch/mips/mm/c-r4k.c which currently looks like:
static inline void r4k_on_each_cpu(void (*func) (void *info), void *info)
{
preempt_disable();
#if !defined(CONFIG_MIPS_MT_SMP) && !defined(CONFIG_MIPS_MT_SMTC)
smp_call_function(func, info, 1);
#endif
func(info);
preempt_enable();
}
This is a slightly specialized version of on_each_cpu() which only calls
out to other CPUs in actual multi-core environments and also - unlike
on_each_cpu() doesn't disable interrupts for the sake of better
interrupt latencies.
Which reminds me ...
Andrew, I was wondering why did 78eef01b0fae087c5fadbd85dd4fe2918c3a015f
[[PATCH] on_each_cpu(): disable local interrupts] disable interrupts?
The log is:
----- snip -----
When on_each_cpu() runs the callback on other CPUs, it runs with local
interrupts disabled. So we should run the function with local interrupts
disabled on this CPU, too.
And do the same for UP, so the callback is run in the same environment on bo
UP and SMP. (strictly it should do preempt_disable() too, but I think
local_irq_disable is sufficiently equivalent).
[...]
----- snip -----
I'm not entirely convinced the symmetry between UP and SMP environments is
really worth it. Would anybody mind removing the local_irq_disable() ...
local_irq_enable() from on_each_cpu()?
Ralf
^ permalink raw reply
* Re: linux-next: manual merge of the akpm tree with the powerpc tree
From: Andrew Morton @ 2013-06-26 13:13 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, Michael Neuling, linux-kernel, Oleg Nesterov,
linux-next, linuxppc-dev
In-Reply-To: <1372234231.18612.6.camel@pasglop>
On Wed, 26 Jun 2013 18:10:31 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Wed, 2013-06-26 at 16:56 +1000, Stephen Rothwell wrote:
> > Today's linux-next merge of the akpm tree got a conflict in
> > arch/powerpc/kernel/ptrace.c between commit b0b0aa9c7faf
> > ("powerpc/hw_brk: Fix setting of length for exact mode breakpoints") from
> > the powerpc tree and commit "ptrace/powerpc: revert "hw_breakpoints: Fix
> > racy access to ptrace breakpoints"" from the akpm tree.
> >
> > I fixed it up (see below) and can carry the fix as necessary (no action
> > is required).
>
> Where does the latter come from ? Why didn't it go through the powerpc tree ?
um, the most rudimentary search will take you to Oleg's patch series.
You were cc'ed on the whole thing two or three times.
^ permalink raw reply
* Re: powerpc/85xx: Move ePAPR paravirt initialization earlier
From: Tudor Laurentiu @ 2013-06-26 12:36 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <20130625231404.GA29587@home.buserror.net>
On 06/26/2013 02:14 AM, Scott Wood wrote:
> On Tue, Mar 05, 2013 at 05:52:36PM +0200, Laurentiu TUDOR wrote:
>> From: Tudor Laurentiu<laurentiu.tudor@freescale.com>
>>
>> The ePAPR para-virtualization needs to happen very early
>> otherwise the bytechannel based console will silently
>> drop some of the early boot messages.
>>
>> Before this patch, this is how the kernel log started:
>> -----------------------------------------------------
>> > Brought up 2 CPUs
>> > devtmpfs: initialized
>> > NET: Registered protocol family 16
>> [...]
>> -----------------------------------------------------
>>
>> After the patch the early messages show up:
>> -----------------------------------------------------
>> > Using P5020 DS machine description
>> > MMU: Supported page sizes
>> > 4 KB as direct
>> > 4096 KB as direct
>> [...]
>> -----------------------------------------------------
>>
>> At console init, the kernel tried to flush the log buffer.
>> Since the paravirt was not yet initialized the console write
>> function failed silently, thus losing the buffered messages.
> [snip]
>> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
>> index 6da881b..ce092ac 100644
>> --- a/arch/powerpc/kernel/setup_64.c
>> +++ b/arch/powerpc/kernel/setup_64.c
>> @@ -66,6 +66,7 @@
>> #include<asm/code-patching.h>
>> #include<asm/kvm_ppc.h>
>> #include<asm/hugetlb.h>
>> +#include<asm/epapr_hcalls.h>
>>
>> #include "setup.h"
>>
>> @@ -599,6 +600,8 @@ void __init setup_arch(char **cmdline_p)
>> /* Initialize the MMU context management stuff */
>> mmu_context_init();
>>
>> + epapr_paravirt_init();
>> +
>> kvm_linear_init();
>>
>> /* Interrupt code needs to be 64K-aligned */
>
> Is this early enough? There's udbg activity before this. Maybe it
> should even go before udbg_early_init...
Right. I'll try moving it in machine_init().
> This would require converting
> the code to use the early device tree functions.
>
I see. Had a look at that api and it seems pretty limited.
I couldn't find a simple way of reading a property other than scanning
the entire tree with of_scan_flat_dt().
---
Best Regards, Laurentiu
^ permalink raw reply
* Re: [PATCH v2] powerpc/vfio: Add missing locks for take/release TCE table ownership
From: Alexey Kardashevskiy @ 2013-06-26 12:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alexey Kardashevskiy, linux-kernel, Paul Mackerras,
Anton Blanchard, linuxppc-dev, David Gibson
In-Reply-To: <1372227662-28302-1-git-send-email-aik@ozlabs.ru>
Ben, please ignore this.
Need some more code there.
On 06/26/2013 04:21 PM, Alexey Kardashevskiy wrote:
> VFIO IOMMU driver for sPAPR TCE locks the whole DMA window by setting
> ones to iommu_table.it_map. However this was not protected by the locks
> which other clients of iommu_table use.
>
> The patch fixes this.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>
> v1->v2:
> * Fixed a potential warning from lockdep.
>
> ---
> arch/powerpc/kernel/iommu.c | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index b20ff17..5c1fc89 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -1076,25 +1076,42 @@ EXPORT_SYMBOL_GPL(iommu_put_tce_user_mode);
> int iommu_take_ownership(struct iommu_table *tbl)
> {
> unsigned long sz = (tbl->it_size + 7) >> 3;
> + unsigned long i, flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(&tbl->large_pool.lock, flags);
> + for (i = 0; i < tbl->nr_pools; i++)
> + spin_lock(&tbl->pools[i].lock);
>
> if (tbl->it_offset == 0)
> clear_bit(0, tbl->it_map);
>
> if (!bitmap_empty(tbl->it_map, tbl->it_size)) {
> pr_err("iommu_tce: it_map is not empty");
> - return -EBUSY;
> + ret = -EBUSY;
> + goto unlock_exit;
> }
>
> memset(tbl->it_map, 0xff, sz);
> iommu_clear_tces_and_put_pages(tbl, tbl->it_offset, tbl->it_size);
>
> - return 0;
> +unlock_exit:
> + for (i = 0; i < tbl->nr_pools; i++)
> + spin_unlock(&tbl->pools[i].lock);
> + spin_unlock_irqrestore(&tbl->large_pool.lock, flags);
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(iommu_take_ownership);
>
> void iommu_release_ownership(struct iommu_table *tbl)
> {
> unsigned long sz = (tbl->it_size + 7) >> 3;
> + unsigned long i, flags;
> +
> + spin_lock_irqsave(&tbl->large_pool.lock, flags);
> + for (i = 0; i < tbl->nr_pools; i++)
> + spin_lock(&tbl->pools[i].lock);
>
> iommu_clear_tces_and_put_pages(tbl, tbl->it_offset, tbl->it_size);
> memset(tbl->it_map, 0, sz);
> @@ -1102,6 +1119,10 @@ void iommu_release_ownership(struct iommu_table *tbl)
> /* Restore bit#0 set by iommu_init_table() */
> if (tbl->it_offset == 0)
> set_bit(0, tbl->it_map);
> +
> + for (i = 0; i < tbl->nr_pools; i++)
> + spin_unlock(&tbl->pools[i].lock);
> + spin_unlock_irqrestore(&tbl->large_pool.lock, flags);
> }
> EXPORT_SYMBOL_GPL(iommu_release_ownership);
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 4/6 v5] KVM: PPC: exit to user space on "ehpriv" instruction
From: Alexander Graf @ 2013-06-26 10:33 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: Wood Scott-B07421, mikey@neuling.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, kvm-ppc@vger.kernel.org,
tiejun.chen, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D07086E00@039-SN2MPN1-013.039d.mgd.msft.net>
On 26.06.2013, at 11:27, Bhushan Bharat-R65777 wrote:
>=20
>=20
>> -----Original Message-----
>> From: tiejun.chen [mailto:tiejun.chen@windriver.com]
>> Sent: Wednesday, June 26, 2013 2:47 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; Wood =
Scott-
>> B07421; benh@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org; =
linux-
>> kernel@vger.kernel.org; mikey@neuling.org
>> Subject: Re: [PATCH 4/6 v5] KVM: PPC: exit to user space on "ehpriv" =
instruction
>>=20
>> On 06/26/2013 04:44 PM, Bhushan Bharat-R65777 wrote:
>>>=20
>>>=20
>>>> -----Original Message-----
>>>> From: tiejun.chen [mailto:tiejun.chen@windriver.com]
>>>> Sent: Wednesday, June 26, 2013 12:25 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; =
Wood
>>>> Scott- B07421; benh@kernel.crashing.org;
>>>> linuxppc-dev@lists.ozlabs.org; linux- kernel@vger.kernel.org;
>>>> mikey@neuling.org; Bhushan Bharat-R65777
>>>> Subject: Re: [PATCH 4/6 v5] KVM: PPC: exit to user space on =
"ehpriv"
>>>> instruction
>>>>=20
>>>> On 06/26/2013 01:42 PM, Bharat Bhushan wrote:
>>>>> "ehpriv" instruction is used for setting software breakpoints by
>>>>> user space. This patch adds support to exit to user space with
>>>>> "run->debug" have relevant information.
>>>>>=20
>>>>> As this is the first point we are using run->debug, also defined =
the
>>>>> run->debug structure.
>>>>>=20
>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>>>> ---
>>>>> arch/powerpc/include/asm/disassemble.h | 4 ++++
>>>>> arch/powerpc/include/uapi/asm/kvm.h | 21 =
+++++++++++++++++----
>>>>> arch/powerpc/kvm/e500_emulate.c | 27 =
+++++++++++++++++++++++++++
>>>>> 3 files changed, 48 insertions(+), 4 deletions(-)
>>>>>=20
>>>>> diff --git a/arch/powerpc/include/asm/disassemble.h
>>>> b/arch/powerpc/include/asm/disassemble.h
>>>>> index 9b198d1..856f8de 100644
>>>>> --- a/arch/powerpc/include/asm/disassemble.h
>>>>> +++ b/arch/powerpc/include/asm/disassemble.h
>>>>> @@ -77,4 +77,8 @@ static inline unsigned int get_d(u32 inst)
>>>>> return inst & 0xffff;
>>>>> }
>>>>>=20
>>>>> +static inline unsigned int get_oc(u32 inst) {
>>>>> + return (inst >> 11) & 0x7fff;
>>>>> +}
>>>>> #endif /* __ASM_PPC_DISASSEMBLE_H__ */ diff --git
>>>>> a/arch/powerpc/include/uapi/asm/kvm.h
>>>> b/arch/powerpc/include/uapi/asm/kvm.h
>>>>> index 0fb1a6e..ded0607 100644
>>>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>>>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>>>>> @@ -269,7 +269,24 @@ struct kvm_fpu {
>>>>> __u64 fpr[32];
>>>>> };
>>>>>=20
>>>>> +/*
>>>>> + * Defines for h/w breakpoint, watchpoint (read, write or both) =
and
>>>>> + * software breakpoint.
>>>>> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and =
"status"
>>>>> + * for KVM_DEBUG_EXIT.
>>>>> + */
>>>>> +#define KVMPPC_DEBUG_NONE 0x0
>>>>> +#define KVMPPC_DEBUG_BREAKPOINT (1UL << 1)
>>>>> +#define KVMPPC_DEBUG_WATCH_WRITE (1UL << 2)
>>>>> +#define KVMPPC_DEBUG_WATCH_READ (1UL << 3)
>>>>> struct kvm_debug_exit_arch {
>>>>> + __u64 address;
>>>>> + /*
>>>>> + * exiting to userspace because of h/w breakpoint, watchpoint
>>>>> + * (read, write or both) and software breakpoint.
>>>>> + */
>>>>> + __u32 status;
>>>>> + __u32 reserved;
>>>>> };
>>>>>=20
>>>>> /* for KVM_SET_GUEST_DEBUG */
>>>>> @@ -281,10 +298,6 @@ struct kvm_guest_debug_arch {
>>>>> * Type denotes h/w breakpoint, read watchpoint, =
write
>>>>> * watchpoint or watchpoint (both read and =
write).
>>>>> */
>>>>> -#define KVMPPC_DEBUG_NONE 0x0
>>>>> -#define KVMPPC_DEBUG_BREAKPOINT (1UL << 1)
>>>>> -#define KVMPPC_DEBUG_WATCH_WRITE (1UL << 2)
>>>>> -#define KVMPPC_DEBUG_WATCH_READ (1UL << 3)
>>>>> __u32 type;
>>>>> __u32 reserved;
>>>>> } bp[16];
>>>>> diff --git a/arch/powerpc/kvm/e500_emulate.c
>>>>> b/arch/powerpc/kvm/e500_emulate.c index b10a012..dab9d07 100644
>>>>> --- a/arch/powerpc/kvm/e500_emulate.c
>>>>> +++ b/arch/powerpc/kvm/e500_emulate.c
>>>>> @@ -26,6 +26,8 @@
>>>>> #define XOP_TLBRE 946
>>>>> #define XOP_TLBWE 978
>>>>> #define XOP_TLBILX 18
>>>>> +#define XOP_EHPRIV 270
>>>>> +#define EHPRIV_OC_DEBUG 0
>>>>=20
>>>> As I think the case, "OC =3D 0", is a bit specific since IIRC, if =
the
>>>> OC operand is omitted, its equal 0 by default. So I think we should
>>>> start this OC value from 1 or other magic number.
>>>=20
>>> ehpriv instruction is defined to be used as:
>>> ehpriv OC // where OC can be 0,1, ... n and in extended for it =
can be
>>> used as
>>> ehpriv // With no OC, and here it assumes OC =3D 0 So OC =3D 0 =
is not
>>> specific but "ehpriv" is same as "ehpriv 0".
>>=20
>> Yes, this is just what I mean.
>>=20
>>>=20
>>> I do not think of any special reason to reserve "ehpriv" and "ehpriv =
0".
>>=20
>> So I still prefer we can reserve the 'ehpriv' without OC operand as =
one simple
>> approach to test or develop something for KVM quickly because its =
really
>> convenient to trap into the hypervisor only with one 'ehpriv' =
instruction
>> easily.
>>=20
>> But I have no further objection if you guys are fine to this ;-)
>=20
> I can see the using "ehpriv" can be a default choice. But all ehvpriv =
trap is handled at one place (in a single function) so the accidently =
overlap with debug should not be an issue.
>=20
> I too do not have any strong opinion to keep either way, so want to =
leave as is ;-).
Seconded. On x86 we also just use int3 for soft breakpoints IIRC.
Alex
^ permalink raw reply
* Re: [PATCH 3/6] powerpc/powernv: Replace variables with flags
From: Gavin Shan @ 2013-06-26 10:08 UTC (permalink / raw)
To: David Laight; +Cc: linuxppc-dev, Gavin Shan
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B72A6@saturn3.aculab.com>
On Wed, Jun 26, 2013 at 10:12:16AM +0100, David Laight wrote:
>> We have 2 fields in "struct pnv_phb" to trace the states. The patch
>> replace the fields with one and introduces flags for that. The patch
>> doesn't impact the logic.
>
>What is the benefit of this change?
>
There might have more flags coming in. Putting all flags together
could be maintained more easily. It doesn't save much memory as
you pointed.
>> +
>> +#define PNV_EEH_STATE_ENABLED (1 << 0) /* EEH enabled */
>> +#define PNV_EEH_STATE_REMOVED (1 << 1) /* PHB removed */
>> +
>> #endif /* CONFIG_EEH */
>>
>> struct pnv_phb {
>> @@ -92,8 +96,7 @@ struct pnv_phb {
>>
>> #ifdef CONFIG_EEH
>> struct pnv_eeh_ops *eeh_ops;
>> - int eeh_enabled;
>> - int removed;
>> + int eeh_state;
>> #endif
>>
>> #ifdef CONFIG_DEBUG_FS
>
>All I can see is that it possibly reduces a structure by 4 bytes
>while adding extra code.
>(On 64 bit systems there might be a 4 byte pad added.)
>
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH 8/8] powerpc/perf: Add power8 EBB support
From: Anshuman Khandual @ 2013-06-26 9:58 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, sukadev, Paul Mackerras
In-Reply-To: <1372073336-8189-8-git-send-email-michael@ellerman.id.au>
> @@ -117,6 +117,7 @@
> (EVENT_UNIT_MASK << EVENT_UNIT_SHIFT) | \
> (EVENT_COMBINE_MASK << EVENT_COMBINE_SHIFT) | \
> (EVENT_MARKED_MASK << EVENT_MARKED_SHIFT) | \
> + (1ull << EVENT_CONFIG_EBB_SHIFT) | \
We should define this macro like EVENT_MARKED_MASK
#define EVENT_EBB_MASK 0x1
Numeric value of "1ull" stands out odd in the scheme.
> EVENT_PSEL_MASK)
>
> + * EBB -* | |
> + * | | Count of events for each PMC.
> + * L1 I/D qualifier -* | p1, p2, p3, p4, p5, p6.
> * nc - number of counters -*
> *
> * The PMC fields P1..P6, and NC, are adder fields. As we accumulate constraints
> @@ -159,6 +160,9 @@
> #define CNST_THRESH_VAL(v) (((v) & EVENT_THRESH_MASK) << 32)
> #define CNST_THRESH_MASK CNST_THRESH_VAL(EVENT_THRESH_MASK)
>
> +#define CNST_EBB_VAL(v) (((v) & 1) << 24)
EVENT_EBB_MASK can be used here as well.
^ permalink raw reply
* RE: [PATCH 4/6 v5] KVM: PPC: exit to user space on "ehpriv" instruction
From: Bhushan Bharat-R65777 @ 2013-06-26 9:27 UTC (permalink / raw)
To: tiejun.chen
Cc: Wood Scott-B07421, mikey@neuling.org, kvm@vger.kernel.org,
agraf@suse.de, kvm-ppc@vger.kernel.org,
linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <51CAB199.8080306@windriver.com>
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogdGllanVuLmNoZW4gW21h
aWx0bzp0aWVqdW4uY2hlbkB3aW5kcml2ZXIuY29tXQ0KPiBTZW50OiBXZWRuZXNkYXksIEp1bmUg
MjYsIDIwMTMgMjo0NyBQTQ0KPiBUbzogQmh1c2hhbiBCaGFyYXQtUjY1Nzc3DQo+IENjOiBrdm0t
cHBjQHZnZXIua2VybmVsLm9yZzsga3ZtQHZnZXIua2VybmVsLm9yZzsgYWdyYWZAc3VzZS5kZTsg
V29vZCBTY290dC0NCj4gQjA3NDIxOyBiZW5oQGtlcm5lbC5jcmFzaGluZy5vcmc7IGxpbnV4cHBj
LWRldkBsaXN0cy5vemxhYnMub3JnOyBsaW51eC0NCj4ga2VybmVsQHZnZXIua2VybmVsLm9yZzsg
bWlrZXlAbmV1bGluZy5vcmcNCj4gU3ViamVjdDogUmU6IFtQQVRDSCA0LzYgdjVdIEtWTTogUFBD
OiBleGl0IHRvIHVzZXIgc3BhY2Ugb24gImVocHJpdiIgaW5zdHJ1Y3Rpb24NCj4gDQo+IE9uIDA2
LzI2LzIwMTMgMDQ6NDQgUE0sIEJodXNoYW4gQmhhcmF0LVI2NTc3NyB3cm90ZToNCj4gPg0KPiA+
DQo+ID4+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ID4+IEZyb206IHRpZWp1bi5jaGVu
IFttYWlsdG86dGllanVuLmNoZW5Ad2luZHJpdmVyLmNvbV0NCj4gPj4gU2VudDogV2VkbmVzZGF5
LCBKdW5lIDI2LCAyMDEzIDEyOjI1IFBNDQo+ID4+IFRvOiBCaHVzaGFuIEJoYXJhdC1SNjU3NzcN
Cj4gPj4gQ2M6IGt2bS1wcGNAdmdlci5rZXJuZWwub3JnOyBrdm1Admdlci5rZXJuZWwub3JnOyBh
Z3JhZkBzdXNlLmRlOyBXb29kDQo+ID4+IFNjb3R0LSBCMDc0MjE7IGJlbmhAa2VybmVsLmNyYXNo
aW5nLm9yZzsNCj4gPj4gbGludXhwcGMtZGV2QGxpc3RzLm96bGFicy5vcmc7IGxpbnV4LSBrZXJu
ZWxAdmdlci5rZXJuZWwub3JnOw0KPiA+PiBtaWtleUBuZXVsaW5nLm9yZzsgQmh1c2hhbiBCaGFy
YXQtUjY1Nzc3DQo+ID4+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggNC82IHY1XSBLVk06IFBQQzogZXhp
dCB0byB1c2VyIHNwYWNlIG9uICJlaHByaXYiDQo+ID4+IGluc3RydWN0aW9uDQo+ID4+DQo+ID4+
IE9uIDA2LzI2LzIwMTMgMDE6NDIgUE0sIEJoYXJhdCBCaHVzaGFuIHdyb3RlOg0KPiA+Pj4gImVo
cHJpdiIgaW5zdHJ1Y3Rpb24gaXMgdXNlZCBmb3Igc2V0dGluZyBzb2Z0d2FyZSBicmVha3BvaW50
cyBieQ0KPiA+Pj4gdXNlciBzcGFjZS4gVGhpcyBwYXRjaCBhZGRzIHN1cHBvcnQgdG8gZXhpdCB0
byB1c2VyIHNwYWNlIHdpdGgNCj4gPj4+ICJydW4tPmRlYnVnIiBoYXZlIHJlbGV2YW50IGluZm9y
bWF0aW9uLg0KPiA+Pj4NCj4gPj4+IEFzIHRoaXMgaXMgdGhlIGZpcnN0IHBvaW50IHdlIGFyZSB1
c2luZyBydW4tPmRlYnVnLCBhbHNvIGRlZmluZWQgdGhlDQo+ID4+PiBydW4tPmRlYnVnIHN0cnVj
dHVyZS4NCj4gPj4+DQo+ID4+PiBTaWduZWQtb2ZmLWJ5OiBCaGFyYXQgQmh1c2hhbiA8YmhhcmF0
LmJodXNoYW5AZnJlZXNjYWxlLmNvbT4NCj4gPj4+IC0tLQ0KPiA+Pj4gICAgYXJjaC9wb3dlcnBj
L2luY2x1ZGUvYXNtL2Rpc2Fzc2VtYmxlLmggfCAgICA0ICsrKysNCj4gPj4+ICAgIGFyY2gvcG93
ZXJwYy9pbmNsdWRlL3VhcGkvYXNtL2t2bS5oICAgIHwgICAyMSArKysrKysrKysrKysrKysrKy0t
LS0NCj4gPj4+ICAgIGFyY2gvcG93ZXJwYy9rdm0vZTUwMF9lbXVsYXRlLmMgICAgICAgIHwgICAy
NyArKysrKysrKysrKysrKysrKysrKysrKysrKysNCj4gPj4+ICAgIDMgZmlsZXMgY2hhbmdlZCwg
NDggaW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMoLSkNCj4gPj4+DQo+ID4+PiBkaWZmIC0tZ2l0
IGEvYXJjaC9wb3dlcnBjL2luY2x1ZGUvYXNtL2Rpc2Fzc2VtYmxlLmgNCj4gPj4gYi9hcmNoL3Bv
d2VycGMvaW5jbHVkZS9hc20vZGlzYXNzZW1ibGUuaA0KPiA+Pj4gaW5kZXggOWIxOThkMS4uODU2
ZjhkZSAxMDA2NDQNCj4gPj4+IC0tLSBhL2FyY2gvcG93ZXJwYy9pbmNsdWRlL2FzbS9kaXNhc3Nl
bWJsZS5oDQo+ID4+PiArKysgYi9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20vZGlzYXNzZW1ibGUu
aA0KPiA+Pj4gQEAgLTc3LDQgKzc3LDggQEAgc3RhdGljIGlubGluZSB1bnNpZ25lZCBpbnQgZ2V0
X2QodTMyIGluc3QpDQo+ID4+PiAgICAJcmV0dXJuIGluc3QgJiAweGZmZmY7DQo+ID4+PiAgICB9
DQo+ID4+Pg0KPiA+Pj4gK3N0YXRpYyBpbmxpbmUgdW5zaWduZWQgaW50IGdldF9vYyh1MzIgaW5z
dCkgew0KPiA+Pj4gKwlyZXR1cm4gKGluc3QgPj4gMTEpICYgMHg3ZmZmOw0KPiA+Pj4gK30NCj4g
Pj4+ICAgICNlbmRpZiAvKiBfX0FTTV9QUENfRElTQVNTRU1CTEVfSF9fICovIGRpZmYgLS1naXQN
Cj4gPj4+IGEvYXJjaC9wb3dlcnBjL2luY2x1ZGUvdWFwaS9hc20va3ZtLmgNCj4gPj4gYi9hcmNo
L3Bvd2VycGMvaW5jbHVkZS91YXBpL2FzbS9rdm0uaA0KPiA+Pj4gaW5kZXggMGZiMWE2ZS4uZGVk
MDYwNyAxMDA2NDQNCj4gPj4+IC0tLSBhL2FyY2gvcG93ZXJwYy9pbmNsdWRlL3VhcGkvYXNtL2t2
bS5oDQo+ID4+PiArKysgYi9hcmNoL3Bvd2VycGMvaW5jbHVkZS91YXBpL2FzbS9rdm0uaA0KPiA+
Pj4gQEAgLTI2OSw3ICsyNjksMjQgQEAgc3RydWN0IGt2bV9mcHUgew0KPiA+Pj4gICAgCV9fdTY0
IGZwclszMl07DQo+ID4+PiAgICB9Ow0KPiA+Pj4NCj4gPj4+ICsvKg0KPiA+Pj4gKyAqIERlZmlu
ZXMgZm9yIGgvdyBicmVha3BvaW50LCB3YXRjaHBvaW50IChyZWFkLCB3cml0ZSBvciBib3RoKSBh
bmQNCj4gPj4+ICsgKiBzb2Z0d2FyZSBicmVha3BvaW50Lg0KPiA+Pj4gKyAqIFRoZXNlIGFyZSB1
c2VkIGFzICJ0eXBlIiBpbiBLVk1fU0VUX0dVRVNUX0RFQlVHIGlvY3RsIGFuZCAic3RhdHVzIg0K
PiA+Pj4gKyAqIGZvciBLVk1fREVCVUdfRVhJVC4NCj4gPj4+ICsgKi8NCj4gPj4+ICsjZGVmaW5l
IEtWTVBQQ19ERUJVR19OT05FCQkweDANCj4gPj4+ICsjZGVmaW5lIEtWTVBQQ19ERUJVR19CUkVB
S1BPSU5UCQkoMVVMIDw8IDEpDQo+ID4+PiArI2RlZmluZSBLVk1QUENfREVCVUdfV0FUQ0hfV1JJ
VEUJKDFVTCA8PCAyKQ0KPiA+Pj4gKyNkZWZpbmUgS1ZNUFBDX0RFQlVHX1dBVENIX1JFQUQJCSgx
VUwgPDwgMykNCj4gPj4+ICAgIHN0cnVjdCBrdm1fZGVidWdfZXhpdF9hcmNoIHsNCj4gPj4+ICsJ
X191NjQgYWRkcmVzczsNCj4gPj4+ICsJLyoNCj4gPj4+ICsJICogZXhpdGluZyB0byB1c2Vyc3Bh
Y2UgYmVjYXVzZSBvZiBoL3cgYnJlYWtwb2ludCwgd2F0Y2hwb2ludA0KPiA+Pj4gKwkgKiAocmVh
ZCwgd3JpdGUgb3IgYm90aCkgYW5kIHNvZnR3YXJlIGJyZWFrcG9pbnQuDQo+ID4+PiArCSAqLw0K
PiA+Pj4gKwlfX3UzMiBzdGF0dXM7DQo+ID4+PiArCV9fdTMyIHJlc2VydmVkOw0KPiA+Pj4gICAg
fTsNCj4gPj4+DQo+ID4+PiAgICAvKiBmb3IgS1ZNX1NFVF9HVUVTVF9ERUJVRyAqLw0KPiA+Pj4g
QEAgLTI4MSwxMCArMjk4LDYgQEAgc3RydWN0IGt2bV9ndWVzdF9kZWJ1Z19hcmNoIHsNCj4gPj4+
ICAgIAkJICogVHlwZSBkZW5vdGVzIGgvdyBicmVha3BvaW50LCByZWFkIHdhdGNocG9pbnQsIHdy
aXRlDQo+ID4+PiAgICAJCSAqIHdhdGNocG9pbnQgb3Igd2F0Y2hwb2ludCAoYm90aCByZWFkIGFu
ZCB3cml0ZSkuDQo+ID4+PiAgICAJCSAqLw0KPiA+Pj4gLSNkZWZpbmUgS1ZNUFBDX0RFQlVHX05P
TkUJCTB4MA0KPiA+Pj4gLSNkZWZpbmUgS1ZNUFBDX0RFQlVHX0JSRUFLUE9JTlQJCSgxVUwgPDwg
MSkNCj4gPj4+IC0jZGVmaW5lIEtWTVBQQ19ERUJVR19XQVRDSF9XUklURQkoMVVMIDw8IDIpDQo+
ID4+PiAtI2RlZmluZSBLVk1QUENfREVCVUdfV0FUQ0hfUkVBRAkJKDFVTCA8PCAzKQ0KPiA+Pj4g
ICAgCQlfX3UzMiB0eXBlOw0KPiA+Pj4gICAgCQlfX3UzMiByZXNlcnZlZDsNCj4gPj4+ICAgIAl9
IGJwWzE2XTsNCj4gPj4+IGRpZmYgLS1naXQgYS9hcmNoL3Bvd2VycGMva3ZtL2U1MDBfZW11bGF0
ZS5jDQo+ID4+PiBiL2FyY2gvcG93ZXJwYy9rdm0vZTUwMF9lbXVsYXRlLmMgaW5kZXggYjEwYTAx
Mi4uZGFiOWQwNyAxMDA2NDQNCj4gPj4+IC0tLSBhL2FyY2gvcG93ZXJwYy9rdm0vZTUwMF9lbXVs
YXRlLmMNCj4gPj4+ICsrKyBiL2FyY2gvcG93ZXJwYy9rdm0vZTUwMF9lbXVsYXRlLmMNCj4gPj4+
IEBAIC0yNiw2ICsyNiw4IEBADQo+ID4+PiAgICAjZGVmaW5lIFhPUF9UTEJSRSAgIDk0Ng0KPiA+
Pj4gICAgI2RlZmluZSBYT1BfVExCV0UgICA5NzgNCj4gPj4+ICAgICNkZWZpbmUgWE9QX1RMQklM
WCAgMTgNCj4gPj4+ICsjZGVmaW5lIFhPUF9FSFBSSVYgIDI3MA0KPiA+Pj4gKyNkZWZpbmUgRUhQ
UklWX09DX0RFQlVHIDANCj4gPj4NCj4gPj4gQXMgSSB0aGluayB0aGUgY2FzZSwgIk9DID0gMCIs
IGlzIGEgYml0IHNwZWNpZmljIHNpbmNlIElJUkMsIGlmIHRoZQ0KPiA+PiBPQyBvcGVyYW5kIGlz
IG9taXR0ZWQsIGl0cyBlcXVhbCAwIGJ5IGRlZmF1bHQuIFNvIEkgdGhpbmsgd2Ugc2hvdWxkDQo+
ID4+IHN0YXJ0IHRoaXMgT0MgdmFsdWUgZnJvbSAxIG9yIG90aGVyIG1hZ2ljIG51bWJlci4NCj4g
Pg0KPiA+IGVocHJpdiBpbnN0cnVjdGlvbiBpcyBkZWZpbmVkIHRvIGJlIHVzZWQgYXM6DQo+ID4g
CWVocHJpdiBPQyAvLyB3aGVyZSBPQyBjYW4gYmUgMCwxLCAuLi4gbiBhbmQgaW4gZXh0ZW5kZWQg
Zm9yIGl0IGNhbiBiZQ0KPiA+IHVzZWQgYXMNCj4gPiAJZWhwcml2IC8vIFdpdGggbm8gT0MsIGFu
ZCBoZXJlIGl0IGFzc3VtZXMgT0MgPSAwIFNvIE9DID0gMCBpcyBub3QNCj4gPiBzcGVjaWZpYyBi
dXQgImVocHJpdiIgaXMgc2FtZSBhcyAiZWhwcml2IDAiLg0KPiANCj4gWWVzLCB0aGlzIGlzIGp1
c3Qgd2hhdCBJIG1lYW4uDQo+IA0KPiA+DQo+ID4gSSBkbyBub3QgdGhpbmsgb2YgYW55IHNwZWNp
YWwgcmVhc29uIHRvIHJlc2VydmUgImVocHJpdiIgYW5kICJlaHByaXYgMCIuDQo+IA0KPiBTbyBJ
IHN0aWxsIHByZWZlciB3ZSBjYW4gcmVzZXJ2ZSB0aGUgJ2VocHJpdicgd2l0aG91dCBPQyBvcGVy
YW5kIGFzIG9uZSBzaW1wbGUNCj4gYXBwcm9hY2ggdG8gdGVzdCBvciBkZXZlbG9wIHNvbWV0aGlu
ZyBmb3IgS1ZNIHF1aWNrbHkgYmVjYXVzZSBpdHMgcmVhbGx5DQo+IGNvbnZlbmllbnQgdG8gdHJh
cCBpbnRvIHRoZSBoeXBlcnZpc29yIG9ubHkgd2l0aCBvbmUgJ2VocHJpdicgaW5zdHJ1Y3Rpb24N
Cj4gZWFzaWx5Lg0KPiANCj4gQnV0IEkgaGF2ZSBubyBmdXJ0aGVyIG9iamVjdGlvbiBpZiB5b3Ug
Z3V5cyBhcmUgZmluZSB0byB0aGlzIDstKQ0KDQpJIGNhbiBzZWUgdGhlIHVzaW5nICJlaHByaXYi
IGNhbiBiZSBhIGRlZmF1bHQgY2hvaWNlLiBCdXQgYWxsIGVodnByaXYgdHJhcCBpcyBoYW5kbGVk
IGF0IG9uZSBwbGFjZSAoaW4gYSBzaW5nbGUgZnVuY3Rpb24pIHNvIHRoZSBhY2NpZGVudGx5IG92
ZXJsYXAgd2l0aCBkZWJ1ZyBzaG91bGQgbm90IGJlIGFuIGlzc3VlLg0KDQpJIHRvbyBkbyBub3Qg
aGF2ZSBhbnkgc3Ryb25nIG9waW5pb24gdG8ga2VlcCBlaXRoZXIgd2F5LCBzbyB3YW50IHRvIGxl
YXZlIGFzIGlzIDstKS4NCg0KLUJoYXJhdA0KDQo+IA0KPiBUaWVqdW4NCj4gDQo+ID4NCj4gPiBU
aGFua3MNCj4gPiAtQmhhcmF0DQo+ID4NCj4gPj4NCj4gPj4gQW5kIGlmIHBvc3NpYmxlLCB3ZSdk
IGJldHRlciBhZGQgc29tZSBjb21tZW50cyB0byBkZXNjcmliZSB0aGlzIHRvDQo+ID4+IG1ha2Ug
dGhlIE9DIGRlZmluaXRpb24gcmVhZGFibGUuDQo+ID4+DQo+ID4+IFRpZWp1bg0KPiA+Pg0KPiA+
Pj4NCj4gPj4+ICAgICNpZmRlZiBDT05GSUdfS1ZNX0U1MDBNQw0KPiA+Pj4gICAgc3RhdGljIGlu
dCBkYmVsbDJwcmlvKHVsb25nIHBhcmFtKSBAQCAtODIsNiArODQsMjYgQEAgc3RhdGljIGludA0K
PiA+Pj4ga3ZtcHBjX2U1MDBfZW11bF9tc2dzbmQoc3RydWN0IGt2bV92Y3B1ICp2Y3B1LA0KPiA+
PiBpbnQgcmIpDQo+ID4+PiAgICB9DQo+ID4+PiAgICAjZW5kaWYNCj4gPj4+DQo+ID4+PiArc3Rh
dGljIGludCBrdm1wcGNfZTUwMF9lbXVsX2VocHJpdihzdHJ1Y3Qga3ZtX3J1biAqcnVuLCBzdHJ1
Y3QNCj4gPj4+ICtrdm1fdmNwdQ0KPiA+PiAqdmNwdSwNCj4gPj4+ICsJCQkJICAgdW5zaWduZWQg
aW50IGluc3QsIGludCAqYWR2YW5jZSkgew0KPiA+Pj4gKwlpbnQgZW11bGF0ZWQgPSBFTVVMQVRF
X0RPTkU7DQo+ID4+PiArDQo+ID4+PiArCXN3aXRjaCAoZ2V0X29jKGluc3QpKSB7DQo+ID4+PiAr
CWNhc2UgRUhQUklWX09DX0RFQlVHOg0KPiA+Pj4gKwkJcnVuLT5leGl0X3JlYXNvbiA9IEtWTV9F
WElUX0RFQlVHOw0KPiA+Pj4gKwkJcnVuLT5kZWJ1Zy5hcmNoLmFkZHJlc3MgPSB2Y3B1LT5hcmNo
LnBjOw0KPiA+Pj4gKwkJcnVuLT5kZWJ1Zy5hcmNoLnN0YXR1cyA9IDA7DQo+ID4+PiArCQlrdm1w
cGNfYWNjb3VudF9leGl0KHZjcHUsIERFQlVHX0VYSVRTKTsNCj4gPj4+ICsJCWVtdWxhdGVkID0g
RU1VTEFURV9FWElUX1VTRVI7DQo+ID4+PiArCQkqYWR2YW5jZSA9IDA7DQo+ID4+PiArCQlicmVh
azsNCj4gPj4+ICsJZGVmYXVsdDoNCj4gPj4+ICsJCWVtdWxhdGVkID0gRU1VTEFURV9GQUlMOw0K
PiA+Pj4gKwl9DQo+ID4+PiArCXJldHVybiBlbXVsYXRlZDsNCj4gPj4+ICt9DQo+ID4+PiArDQo+
ID4+PiAgICBpbnQga3ZtcHBjX2NvcmVfZW11bGF0ZV9vcChzdHJ1Y3Qga3ZtX3J1biAqcnVuLCBz
dHJ1Y3Qga3ZtX3ZjcHUgKnZjcHUsDQo+ID4+PiAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICB1bnNpZ25lZCBpbnQgaW5zdCwgaW50ICphZHZhbmNlKQ0KPiA+Pj4gICAgew0KPiA+Pj4gQEAg
LTEzMCw2ICsxNTIsMTEgQEAgaW50IGt2bXBwY19jb3JlX2VtdWxhdGVfb3Aoc3RydWN0IGt2bV9y
dW4gKnJ1biwNCj4gPj4+IHN0cnVjdA0KPiA+PiBrdm1fdmNwdSAqdmNwdSwNCj4gPj4+ICAgIAkJ
CWVtdWxhdGVkID0ga3ZtcHBjX2U1MDBfZW11bF90bGJpdmF4KHZjcHUsIGVhKTsNCj4gPj4+ICAg
IAkJCWJyZWFrOw0KPiA+Pj4NCj4gPj4+ICsJCWNhc2UgWE9QX0VIUFJJVjoNCj4gPj4+ICsJCQll
bXVsYXRlZCA9IGt2bXBwY19lNTAwX2VtdWxfZWhwcml2KHJ1biwgdmNwdSwgaW5zdCwNCj4gPj4+
ICsJCQkJCQkJICAgYWR2YW5jZSk7DQo+ID4+PiArCQkJYnJlYWs7DQo+ID4+PiArDQo+ID4+PiAg
ICAJCWRlZmF1bHQ6DQo+ID4+PiAgICAJCQllbXVsYXRlZCA9IEVNVUxBVEVfRkFJTDsNCj4gPj4+
ICAgIAkJfQ0KPiA+Pj4NCj4gPj4NCj4gPg0KPiANCg0K
^ permalink raw reply
* Re: [PATCH 4/6 v5] KVM: PPC: exit to user space on "ehpriv" instruction
From: tiejun.chen @ 2013-06-26 9:17 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: Wood Scott-B07421, mikey@neuling.org, kvm@vger.kernel.org,
agraf@suse.de, kvm-ppc@vger.kernel.org,
linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D07086C2F@039-SN2MPN1-013.039d.mgd.msft.net>
On 06/26/2013 04:44 PM, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: tiejun.chen [mailto:tiejun.chen@windriver.com]
>> Sent: Wednesday, June 26, 2013 12:25 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; Wood Scott-
>> B07421; benh@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org; linux-
>> kernel@vger.kernel.org; mikey@neuling.org; Bhushan Bharat-R65777
>> Subject: Re: [PATCH 4/6 v5] KVM: PPC: exit to user space on "ehpriv" instruction
>>
>> On 06/26/2013 01:42 PM, Bharat Bhushan wrote:
>>> "ehpriv" instruction is used for setting software breakpoints
>>> by user space. This patch adds support to exit to user space
>>> with "run->debug" have relevant information.
>>>
>>> As this is the first point we are using run->debug, also defined
>>> the run->debug structure.
>>>
>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>> ---
>>> arch/powerpc/include/asm/disassemble.h | 4 ++++
>>> arch/powerpc/include/uapi/asm/kvm.h | 21 +++++++++++++++++----
>>> arch/powerpc/kvm/e500_emulate.c | 27 +++++++++++++++++++++++++++
>>> 3 files changed, 48 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/disassemble.h
>> b/arch/powerpc/include/asm/disassemble.h
>>> index 9b198d1..856f8de 100644
>>> --- a/arch/powerpc/include/asm/disassemble.h
>>> +++ b/arch/powerpc/include/asm/disassemble.h
>>> @@ -77,4 +77,8 @@ static inline unsigned int get_d(u32 inst)
>>> return inst & 0xffff;
>>> }
>>>
>>> +static inline unsigned int get_oc(u32 inst)
>>> +{
>>> + return (inst >> 11) & 0x7fff;
>>> +}
>>> #endif /* __ASM_PPC_DISASSEMBLE_H__ */
>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h
>> b/arch/powerpc/include/uapi/asm/kvm.h
>>> index 0fb1a6e..ded0607 100644
>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>>> @@ -269,7 +269,24 @@ struct kvm_fpu {
>>> __u64 fpr[32];
>>> };
>>>
>>> +/*
>>> + * Defines for h/w breakpoint, watchpoint (read, write or both) and
>>> + * software breakpoint.
>>> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
>>> + * for KVM_DEBUG_EXIT.
>>> + */
>>> +#define KVMPPC_DEBUG_NONE 0x0
>>> +#define KVMPPC_DEBUG_BREAKPOINT (1UL << 1)
>>> +#define KVMPPC_DEBUG_WATCH_WRITE (1UL << 2)
>>> +#define KVMPPC_DEBUG_WATCH_READ (1UL << 3)
>>> struct kvm_debug_exit_arch {
>>> + __u64 address;
>>> + /*
>>> + * exiting to userspace because of h/w breakpoint, watchpoint
>>> + * (read, write or both) and software breakpoint.
>>> + */
>>> + __u32 status;
>>> + __u32 reserved;
>>> };
>>>
>>> /* for KVM_SET_GUEST_DEBUG */
>>> @@ -281,10 +298,6 @@ struct kvm_guest_debug_arch {
>>> * Type denotes h/w breakpoint, read watchpoint, write
>>> * watchpoint or watchpoint (both read and write).
>>> */
>>> -#define KVMPPC_DEBUG_NONE 0x0
>>> -#define KVMPPC_DEBUG_BREAKPOINT (1UL << 1)
>>> -#define KVMPPC_DEBUG_WATCH_WRITE (1UL << 2)
>>> -#define KVMPPC_DEBUG_WATCH_READ (1UL << 3)
>>> __u32 type;
>>> __u32 reserved;
>>> } bp[16];
>>> diff --git a/arch/powerpc/kvm/e500_emulate.c b/arch/powerpc/kvm/e500_emulate.c
>>> index b10a012..dab9d07 100644
>>> --- a/arch/powerpc/kvm/e500_emulate.c
>>> +++ b/arch/powerpc/kvm/e500_emulate.c
>>> @@ -26,6 +26,8 @@
>>> #define XOP_TLBRE 946
>>> #define XOP_TLBWE 978
>>> #define XOP_TLBILX 18
>>> +#define XOP_EHPRIV 270
>>> +#define EHPRIV_OC_DEBUG 0
>>
>> As I think the case, "OC = 0", is a bit specific since IIRC, if the OC
>> operand is omitted, its equal 0 by default. So I think we should start this OC
>> value from 1 or other magic number.
>
> ehpriv instruction is defined to be used as:
> ehpriv OC // where OC can be 0,1, ... n
> and in extended for it can be used as
> ehpriv // With no OC, and here it assumes OC = 0
> So OC = 0 is not specific but "ehpriv" is same as "ehpriv 0".
Yes, this is just what I mean.
>
> I do not think of any special reason to reserve "ehpriv" and "ehpriv 0".
So I still prefer we can reserve the 'ehpriv' without OC operand as one simple
approach to test or develop something for KVM quickly because its really
convenient to trap into the hypervisor only with one 'ehpriv' instruction easily.
But I have no further objection if you guys are fine to this ;-)
Tiejun
>
> Thanks
> -Bharat
>
>>
>> And if possible, we'd better add some comments to describe this to make the OC
>> definition readable.
>>
>> Tiejun
>>
>>>
>>> #ifdef CONFIG_KVM_E500MC
>>> static int dbell2prio(ulong param)
>>> @@ -82,6 +84,26 @@ static int kvmppc_e500_emul_msgsnd(struct kvm_vcpu *vcpu,
>> int rb)
>>> }
>>> #endif
>>>
>>> +static int kvmppc_e500_emul_ehpriv(struct kvm_run *run, struct kvm_vcpu
>> *vcpu,
>>> + unsigned int inst, int *advance)
>>> +{
>>> + int emulated = EMULATE_DONE;
>>> +
>>> + switch (get_oc(inst)) {
>>> + case EHPRIV_OC_DEBUG:
>>> + run->exit_reason = KVM_EXIT_DEBUG;
>>> + run->debug.arch.address = vcpu->arch.pc;
>>> + run->debug.arch.status = 0;
>>> + kvmppc_account_exit(vcpu, DEBUG_EXITS);
>>> + emulated = EMULATE_EXIT_USER;
>>> + *advance = 0;
>>> + break;
>>> + default:
>>> + emulated = EMULATE_FAIL;
>>> + }
>>> + return emulated;
>>> +}
>>> +
>>> int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>> unsigned int inst, int *advance)
>>> {
>>> @@ -130,6 +152,11 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct
>> kvm_vcpu *vcpu,
>>> emulated = kvmppc_e500_emul_tlbivax(vcpu, ea);
>>> break;
>>>
>>> + case XOP_EHPRIV:
>>> + emulated = kvmppc_e500_emul_ehpriv(run, vcpu, inst,
>>> + advance);
>>> + break;
>>> +
>>> default:
>>> emulated = EMULATE_FAIL;
>>> }
>>>
>>
>
^ permalink raw reply
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