Re: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommu implementation.
On Wed, 2013-06-26 at 06:24 +, Sethi Varun-B16395 wrote:
>
> > -Original Message-
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Tuesday, June 25, 2013 10:27 AM
> > To: Sethi Varun-B16395
> > Cc: j...@8bytes.org; io...@lists.linux-foundation.org; linuxppc-
> > d...@lists.ozlabs.org; linux-ker...@vger.kernel.org;
> > b...@kernel.crashing.org; ga...@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->bu
RE: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommu implementation.
> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Tuesday, June 25, 2013 10:27 AM
> To: Sethi Varun-B16395
> Cc: j...@8bytes.org; io...@lists.linux-foundation.org; linuxppc-
> d...@lists.ozlabs.org; linux-ker...@vger.kernel.org;
> b...@kernel.crashing.org; ga...@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.
> > + }
> > + } 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, usi
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?
> + }
> + } 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.
> + 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. This also isn't
testing ACS all the way up to the root complex or controller.
> + /*
> + * PCIe controller is not a paritionable entity
> + * free the controller device iommu_group.
> + */
> + if (pci_ctl->parent->iommu_group)
> + iommu_group_remove_device(pci_ctl->parent);
> + } else {
> + /*
> + * All devices connected to the controller will share the
> + * PCI controllers device group. If this is the first
> + * device to be probed for the pci controller, copy the
> + * device group information from the PCI controller device
> + * node and remove the PCI controller iommu group.
> + * For subsequent devices, the iommu g
Re: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommu implementation.
On Thu, Jun 20, 2013 at 09:31:28PM +0530, Varun Sethi wrote: > This patch provides the PAMU driver (fsl_pamu.c) and the corresponding IOMMU > API implementation (fsl_pamu_domain.c). The PAMU hardware driver (fsl_pamu.c) > has been derived from the work done by Ashish Kalra and Timur Tabi. AlexW, can you please have a look at the group-code again and ack the patch if it looks right to you? Thanks, Joerg ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
