Removal of bus->msi assignment breaks MSI with stacked domains

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

This patch relies on architectures to implement either pcibios_msi_controller() or arch_setup_msi_irq(). It turns out that with stacked domains, none of this is actually necessary, as long as you can access to the msi_controller.

And everything was fine until this patch came around (and managed to test on a system where the PCI devices are not directly attached to the root bus). Of course, everything now breaks, as we cannot get to the MSI controller (which contains the domain we allocate the MSIs from).

In short, this patch breaks an important feature on which arm64 relies, and I believe this patch should be reverted ASAP.

Quoted 11 lines · Marc Zyngier

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

This patch relies on architectures to implement either pcibios_msi_controller() or arch_setup_msi_irq(). It turns out that with stacked domains, none of this is actually necessary, as long as you can access to the msi_controller.

Quoted 7 lines

And everything was fine until this patch came around (and managed to test on a system where the PCI devices are not directly attached to the root bus). Of course, everything now breaks, as we cannot get to the MSI controller (which contains the domain we allocate the MSIs from).

In short, this patch breaks an important feature on which arm64 relies, and I believe this patch should be reverted ASAP.

I'm happy to revert it from pci/msi, but I think Thomas has already pulled it into his branch, so he'd have to drop it, too.

Thomas, let me know if you want to do that. I suppose we could add a new patch to add it back, but that would leave bisection broken for the interval between c167caf8d174 and the patch that adds it back.

Quoted 20 lines · Bjorn Helgaas

Quoted 11 lines · Marc Zyngier

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

This patch relies on architectures to implement either pcibios_msi_controller() or arch_setup_msi_irq(). It turns out that with stacked domains, none of this is actually necessary, as long as you can access to the msi_controller.

Quoted 7 lines

And everything was fine until this patch came around (and managed to test on a system where the PCI devices are not directly attached to the root bus). Of course, everything now breaks, as we cannot get to the MSI controller (which contains the domain we allocate the MSIs from).

In short, this patch breaks an important feature on which arm64 relies, and I believe this patch should be reverted ASAP.

Quoted 6 lines

I'm happy to revert it from pci/msi, but I think Thomas has already pulled it into his branch, so he'd have to drop it, too.

Thomas, let me know if you want to do that. I suppose we could add a new patch to add it back, but that would leave bisection broken for the interval between c167caf8d174 and the patch that adds it back.

Fortunately my irq/irqdomain branch is not immutable yet. So we have no problem at that point. I can rebase on your branch until tomorrow night. Or just rebase on mainline and we sort out the merge conflicts later, i.e. delegate them to Linus so his job of pulling stuff gets not completely boring.

What I'm more worried about is whether this intended change is going to inflict a problem on Jiangs intention to deduce the MSI irq domain from the device, which we really need for making DMAR work w/o going through loops and hoops.

I have limited knowledge about the actual scope of iommu (DMAR) units versus device/bus/host-controllers, so I would appreciate a proper explanation for that from you or Jiang or both.

My guts feeling tells me that anything less granular than the bus level is wrong and according to my limited knowledge Intel even has DMARs which are assigned to a single device it's even more wrong. So the proper change would be not to push it from bus to something above the bus, but instead make it a per device property.

Quoted 43 lines · Thomas Gleixner

Quoted 20 lines · Bjorn Helgaas

Quoted 11 lines · Marc Zyngier

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

This patch relies on architectures to implement either pcibios_msi_controller() or arch_setup_msi_irq(). It turns out that with stacked domains, none of this is actually necessary, as long as you can access to the msi_controller.

Quoted 7 lines

And everything was fine until this patch came around (and managed to test on a system where the PCI devices are not directly attached to the root bus). Of course, everything now breaks, as we cannot get to the MSI controller (which contains the domain we allocate the MSIs from).

In short, this patch breaks an important feature on which arm64 relies, and I believe this patch should be reverted ASAP.

Quoted 6 lines

I'm happy to revert it from pci/msi, but I think Thomas has already pulled it into his branch, so he'd have to drop it, too.

Thomas, let me know if you want to do that. I suppose we could add a new patch to add it back, but that would leave bisection broken for the interval between c167caf8d174 and the patch that adds it back.

Fortunately my irq/irqdomain branch is not immutable yet. So we have no problem at that point. I can rebase on your branch until tomorrow night. Or just rebase on mainline and we sort out the merge conflicts later, i.e. delegate them to Linus so his job of pulling stuff gets not completely boring.

What I'm more worried about is whether this intended change is going to inflict a problem on Jiangs intention to deduce the MSI irq domain from the device, which we really need for making DMAR work w/o going through loops and hoops.

I have limited knowledge about the actual scope of iommu (DMAR) units versus device/bus/host-controllers, so I would appreciate a proper explanation for that from you or Jiang or both.

Quoted 5 lines

My guts feeling tells me that anything less granular than the bus level is wrong and according to my limited knowledge Intel even has DMARs which are assigned to a single device it's even more wrong. So the proper change would be not to push it from bus to something above the bus, but instead make it a per device property.

I'm not an expert, so hopefully Jiang and Marc will clarify this.

My understanding is that Intel VT-d can do MSI remapping on a per-function basis, so I agree that pci_msi_controller() should probably take a pci_dev instead of a pci_bus. I think Yijing's series only has one caller, which has the pci_dev, so that should be an easy change.

Marc, does c167caf8d174 break arm64 by itself? Or does the breakage happen later on, after adding more stacked domain stuff? If the former, we should definitely revert c167caf8d174 to preserve bisectability, independent of how we fix the pci_msi_controller() interface.

Quoted 3 lines

Thomas, let me know if you want to do that. I suppose we could add a new patch to add it back, but that would leave bisection broken for the interval between c167caf8d174 and the patch that adds it back.

Fortunately my irq/irqdomain branch is not immutable yet. So we have no problem at that point. I can rebase on your branch until tomorrow night. Or just rebase on mainline and we sort out the merge conflicts later, i.e. delegate them to Linus so his job of pulling stuff gets not completely boring.

What I'm more worried about is whether this intended change is going to inflict a problem on Jiangs intention to deduce the MSI irq domain from the device, which we really need for making DMAR work w/o going through loops and hoops.

I have limited knowledge about the actual scope of iommu (DMAR) units versus device/bus/host-controllers, so I would appreciate a proper explanation for that from you or Jiang or both.

My guts feeling tells me that anything less granular than the bus level is wrong and according to my limited knowledge Intel even has DMARs which are assigned to a single device it's even more wrong. So the proper change would be not to push it from bus to something above the bus, but instead make it a per device property.

But my knowledge there is limited, so I rely on the PCI/architecture experts to sort that out.

Let me know ASAP.

Thanks,

tglx

.

Quoted 9 lines

Quoted 3 lines

Thomas, let me know if you want to do that. I suppose we could add a new patch to add it back, but that would leave bisection broken for the interval between c167caf8d174 and the patch that adds it back.

Fortunately my irq/irqdomain branch is not immutable yet. So we have no problem at that point. I can rebase on your branch until tomorrow night. Or just rebase on mainline and we sort out the merge conflicts later, i.e. delegate them to Linus so his job of pulling stuff gets not completely boring.

Hi Thomas, sorry for my introducing the broken.

Quoted 9 lines

What I'm more worried about is whether this intended change is going to inflict a problem on Jiangs intention to deduce the MSI irq domain from the device, which we really need for making DMAR work w/o going through loops and hoops.

I have limited knowledge about the actual scope of iommu (DMAR) units versus device/bus/host-controllers, so I would appreciate a proper explanation for that from you or Jiang or both.

In my personal opinion, if it's not necessary, we should not put stuff into pci_dev or pci_bus. If we plan to save msi_controller in pci_bus or pci_dev.
I have a proposal, I would be appreciated if you could give some comments. First we refactor pci_host_bridge to make a generic pci_host_bridge, then we could save pci domain in it to eliminate arch specific functions. I aslo wanted to save msi_controller as pci domain, but now Jiang refactor hierarchy irq domain, and pci devices under the same pci host bridge may need to associate to different msi_controllers.

So I want to associate a msi_controller finding ops with generic pci_host_bridge, then every pci device could find its msi_controller/irq_domain by a common function

E.g

struct msi_controller *pci_msi_controller(struct pci_dev *pdev)

{
	struct msi_controller *ctrl;
	struct pci_host_bridge *host = find_pci_host_bridge(pdev->bus);
	if (host && host->pci_get_msi_controller)
		ctrl = pci_host_bridge->pci_get_msi_controller(struct pci_dev *pdev);

	return ctrl;	
}

If I miss something, please let me know, thanks.

Thanks!
Yijing.

Quoted 18 lines

My guts feeling tells me that anything less granular than the bus level is wrong and according to my limited knowledge Intel even has DMARs which are assigned to a single device it's even more wrong. So the proper change would be not to push it from bus to something above the bus, but instead make it a per device property.

But my knowledge there is limited, so I rely on the PCI/architecture experts to sort that out.

Let me know ASAP.

Thanks,

tglx

.

Quoted 40 lines · Yijing Wang

Quoted 9 lines

Quoted 3 lines

Thomas, let me know if you want to do that. I suppose we could add a new patch to add it back, but that would leave bisection broken for the interval between c167caf8d174 and the patch that adds it back.

Fortunately my irq/irqdomain branch is not immutable yet. So we have no problem at that point. I can rebase on your branch until tomorrow night. Or just rebase on mainline and we sort out the merge conflicts later, i.e. delegate them to Linus so his job of pulling stuff gets not completely boring.

Hi Thomas, sorry for my introducing the broken.

Quoted 9 lines

What I'm more worried about is whether this intended change is going to inflict a problem on Jiangs intention to deduce the MSI irq domain from the device, which we really need for making DMAR work w/o going through loops and hoops.

I have limited knowledge about the actual scope of iommu (DMAR) units versus device/bus/host-controllers, so I would appreciate a proper explanation for that from you or Jiang or both.

In my personal opinion, if it's not necessary, we should not put stuff into pci_dev or pci_bus. If we plan to save msi_controller in pci_bus or pci_dev.
I have a proposal, I would be appreciated if you could give some comments. First we refactor pci_host_bridge to make a generic pci_host_bridge, then we could save pci domain in it to eliminate arch specific functions. I aslo wanted to save msi_controller as pci domain, but now Jiang refactor hierarchy irq domain, and pci devices under the same pci host bridge may need to associate to different msi_controllers.

So I want to associate a msi_controller finding ops with generic pci_host_bridge, then every pci device could find its msi_controller/irq_domain by a common function

E.g

struct msi_controller *pci_msi_controller(struct pci_dev *pdev)

Quoted 8 lines

{
	struct msi_controller *ctrl;
	struct pci_host_bridge *host = find_pci_host_bridge(pdev->bus);
	if (host && host->pci_get_msi_controller)
		ctrl = pci_host_bridge->pci_get_msi_controller(struct pci_dev *pdev);

	return ctrl;	
}

Hi Yijing,
This may be a little overhead for x86 because we could get
irqdomain from pci_dev itself through:

	pci_dev->dev.archdata.iommu->ir_msi_domain.
	This doesn't work currently because pci_dev->dev.archdata.iommu

is set on the first dma mapping request, but we have a plan to set it when creating PCI devices so we don't need to search the iommu list at runtime.
Even the whole msi_controller concept may be killed for x86.
Actually I'm trying to convert all MSI arch code to use hierarchy irqdomain, then we don't need arch_setup_msi_irqs() and msi_controller.setup_irq() and related anymore. But the issue is that it affects too many architectures and may cause slightly code size increase.
If we could convert all PCI MSI code to use hierarchy irqdomain,
then the suggested interface is:

	struct irq_domain *pci_get_msi_irqdomain(struct pci_dev *pdev);

Thoughts?

Regards!
Gerry

Quoted 27 lines

If I miss something, please let me know, thanks.

Thanks!
Yijing.

Quoted 18 lines

My guts feeling tells me that anything less granular than the bus level is wrong and according to my limited knowledge Intel even has DMARs which are assigned to a single device it's even more wrong. So the proper change would be not to push it from bus to something above the bus, but instead make it a per device property.

But my knowledge there is limited, so I rely on the PCI/architecture experts to sort that out.

Let me know ASAP.

Thanks,

tglx

.

.

Quoted 9 lines

Quoted 3 lines

Thomas, let me know if you want to do that. I suppose we could add a new patch to add it back, but that would leave bisection broken for the interval between c167caf8d174 and the patch that adds it back.

Fortunately my irq/irqdomain branch is not immutable yet. So we have no problem at that point. I can rebase on your branch until tomorrow night. Or just rebase on mainline and we sort out the merge conflicts later, i.e. delegate them to Linus so his job of pulling stuff gets not completely boring.

Hi Thomas, sorry for my introducing the broken.

Quoted 9 lines

What I'm more worried about is whether this intended change is going to inflict a problem on Jiangs intention to deduce the MSI irq domain from the device, which we really need for making DMAR work w/o going through loops and hoops.

I have limited knowledge about the actual scope of iommu (DMAR) units versus device/bus/host-controllers, so I would appreciate a proper explanation for that from you or Jiang or both.

In my personal opinion, if it's not necessary, we should not put stuff into pci_dev or pci_bus. If we plan to save msi_controller in pci_bus or pci_dev.
I have a proposal, I would be appreciated if you could give some comments. First we refactor pci_host_bridge to make a generic pci_host_bridge, then we could save pci domain in it to eliminate arch specific functions. I aslo wanted to save msi_controller as pci domain, but now Jiang refactor hierarchy irq domain, and pci devices under the same pci host bridge may need to associate to different msi_controllers.

So I want to associate a msi_controller finding ops with generic pci_host_bridge, then every pci device could find its msi_controller/irq_domain by a common function

E.g

struct msi_controller *pci_msi_controller(struct pci_dev *pdev)

{
	struct msi_controller *ctrl;
	struct pci_host_bridge *host = find_pci_host_bridge(pdev->bus);
	if (host && host->pci_get_msi_controller)
		ctrl = pci_host_bridge->pci_get_msi_controller(struct pci_dev *pdev);

	return ctrl;	
}

If I miss something, please let me know, thanks.

Quoted 9 lines

Quoted 3 lines

Thomas, let me know if you want to do that. I suppose we could add a new patch to add it back, but that would leave bisection broken for the interval between c167caf8d174 and the patch that adds it back.

Fortunately my irq/irqdomain branch is not immutable yet. So we have no problem at that point. I can rebase on your branch until tomorrow night. Or just rebase on mainline and we sort out the merge conflicts later, i.e. delegate them to Linus so his job of pulling stuff gets not completely boring.

Hi Thomas, sorry for my introducing the broken.

Quoted 9 lines

What I'm more worried about is whether this intended change is going to inflict a problem on Jiangs intention to deduce the MSI irq domain from the device, which we really need for making DMAR work w/o going through loops and hoops.

I have limited knowledge about the actual scope of iommu (DMAR) units versus device/bus/host-controllers, so I would appreciate a proper explanation for that from you or Jiang or both.

In my personal opinion, if it's not necessary, we should not put stuff into pci_dev or pci_bus. If we plan to save msi_controller in pci_bus or pci_dev.
I have a proposal, I would be appreciated if you could give some comments. First we refactor pci_host_bridge to make a generic pci_host_bridge, then we could save pci domain in it to eliminate arch specific functions. I aslo wanted to save msi_controller as pci domain, but now Jiang refactor hierarchy irq domain, and pci devices under the same pci host bridge may need to associate to different msi_controllers.

So I want to associate a msi_controller finding ops with generic pci_host_bridge, then every pci device could find its msi_controller/irq_domain by a common function

E.g

struct msi_controller *pci_msi_controller(struct pci_dev *pdev)

{
	struct msi_controller *ctrl;
	struct pci_host_bridge *host = find_pci_host_bridge(pdev->bus);
	if (host && host->pci_get_msi_controller)
		ctrl = pci_host_bridge->pci_get_msi_controller(struct pci_dev *pdev);

	return ctrl;	
}

Quoted 18 lines

Quoted 8 lines

Quoted 3 lines

Thomas, let me know if you want to do that. I suppose we could add a new patch to add it back, but that would leave bisection broken for the interval between c167caf8d174 and the patch that adds it back.

Fortunately my irq/irqdomain branch is not immutable yet. So we have no problem at that point. I can rebase on your branch until tomorrow night. Or just rebase on mainline and we sort out the merge conflicts later, i.e. delegate them to Linus so his job of pulling stuff gets not completely boring.

Hi Thomas, sorry for my introducing the broken.

Quoted 8 lines

What I'm more worried about is whether this intended change is going to inflict a problem on Jiangs intention to deduce the MSI irq domain from the device, which we really need for making DMAR work w/o going through loops and hoops.

I have limited knowledge about the actual scope of iommu (DMAR) units versus device/bus/host-controllers, so I would appreciate a proper explanation for that from you or Jiang or both.

Quoted 10 lines

In my personal opinion, if it's not necessary, we should not put stuff into pci_dev or pci_bus. If we plan to save msi_controller in pci_bus or pci_dev.
I have a proposal, I would be appreciated if you could give some comments. First we refactor pci_host_bridge to make a generic pci_host_bridge, then we could save pci domain in it to eliminate arch specific functions. I aslo wanted to save msi_controller as pci domain, but now Jiang refactor hierarchy irq domain, and pci devices under the same pci host bridge may need to associate to different msi_controllers.

I think this is getting ahead of ourselves. Let's make small steps.

We currently have the msi_controller pointer in struct pci_bus. That was there even before your series. Your series added pci_msi_controller(), and I reworked it so it looks like this:

     static struct msi_controller *pci_msi_controller(struct pci_dev *dev)
     {
	   struct msi_controller *msi_ctrl = dev->bus->msi;

	   if (msi_ctrl)
		   return msi_ctrl;

	   return pcibios_msi_controller(dev);
     }

So now your series basically just removes the ARM add_bus() and remove_bus() methods and gets the MSI controller info from the ARM pci_sys_data struct instead of from pci_bus. Of course, that assumes that on ARM, all devices under a host bridge have the same MSI controller. That seems like an unwarranted assumption, but if you want to do it for ARM, that's fine with me.

Quoted 14 lines

So I want to associate a msi_controller finding ops with generic pci_host_bridge, then every pci device could find its msi_controller/irq_domain by a common function

E.g

struct msi_controller *pci_msi_controller(struct pci_dev *pdev)

{
	struct msi_controller *ctrl;
	struct pci_host_bridge *host = find_pci_host_bridge(pdev->bus);
	if (host && host->pci_get_msi_controller)
		ctrl = pci_host_bridge->pci_get_msi_controller(struct pci_dev *pdev);

Quoted 2 lines

	return ctrl;	
}

You can do this for ARM if you want (and your series already accomplishes the same effect, though implemented differently). But I don't think this is appropriate for the PCI core.

For anybody who is on this thread but not the original, I reworked the series slightly, see [1].

Bjorn

[1] http://lkml.kernel.org/r/20141121172018.GA6578 at google.com --
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

This patch relies on architectures to implement either pcibios_msi_controller() or arch_setup_msi_irq(). It turns out that with stacked domains, none of this is actually necessary, as long as you can access to the msi_controller.

And everything was fine until this patch came around (and managed to test on a system where the PCI devices are not directly attached to the root bus). Of course, everything now breaks, as we cannot get to the MSI controller (which contains the domain we allocate the MSIs from).

In short, this patch breaks an important feature on which arm64 relies, and I believe this patch should be reverted ASAP.

Thanks,

	M.

Quoted 5 lines · Marc Zyngier

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

Sorry, this is my first part to refactor MSI related code, now how to get pci msi_controller depends arch functions(pcibios_msi_controller() or arch_setup_msi_irq()), we are working on generic pci_host_bridge, after that, we could eventually eliminate MSI arch functions and find pci dev 's msi controller by pci_host_bridge->get_msi_controller().

Quoted 7 lines · Yijing Wang

Quoted 5 lines · Marc Zyngier

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

Quoted 6 lines

Sorry, this is my first part to refactor MSI related code, now how to get pci msi_controller depends arch functions(pcibios_msi_controller() or arch_setup_msi_irq()), we are working on generic pci_host_bridge, after that, we could eventually eliminate MSI arch functions and find pci dev 's msi controller by pci_host_bridge->get_msi_controller().

The main question is why you think that pci_host_bridge is the proper place to store that information.

On x86 we have DMAR units associated to a single device. Each DMAR unit is a seperate MSI irq domain.

Can you guarantee that the pci_host_bridge is the right point to provide the association of the device to the irq domain?

So the real question is:

What is the association level requirement to properly identify the irqdomain for a specific device on any given architecture with and without IOMMU, interrupt redirection etc.

To be honest: I don't know.

My gut feeling tells me that it's at the device level, but I really leave that decision to the experts in that field.

Thanks,

tglx

Quoted 26 lines · Thomas Gleixner

Quoted 7 lines · Yijing Wang

Quoted 5 lines · Marc Zyngier

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

Quoted 6 lines

Sorry, this is my first part to refactor MSI related code, now how to get pci msi_controller depends arch functions(pcibios_msi_controller() or arch_setup_msi_irq()), we are working on generic pci_host_bridge, after that, we could eventually eliminate MSI arch functions and find pci dev 's msi controller by pci_host_bridge->get_msi_controller().

The main question is why you think that pci_host_bridge is the proper place to store that information.

On x86 we have DMAR units associated to a single device. Each DMAR unit is a seperate MSI irq domain.

Can you guarantee that the pci_host_bridge is the right point to provide the association of the device to the irq domain?

So the real question is:

Quoted 8 lines

What is the association level requirement to properly identify the irqdomain for a specific device on any given architecture with and without IOMMU, interrupt redirection etc.

To be honest: I don't know.

My gut feeling tells me that it's at the device level, but I really leave that decision to the experts in that field.

Hi Thomas and Yijing,
Since we are allocating interrupts for a PCI device, it's
natural to get irqdomain from the PCI device itself. If we try to get irqdomain from a PCI bus or host bridge like pci_get_msi_irqdomain(bus or hostbridge), it may fail for x86 because x86 may build per-device irqdomain theoretically. So the preferred interface prototype is:
pci_get_msi_irqdomain(pci_dev) or pcibios_msi_controller(pci_dev)
It's flexible enough. For architectures on which irqdomain is associated with PCI bus or host bridge, you could get the bus or host bridge from pci_dev. And it won't cause extra computation because you always need to get bus or host bridge from the pci_dev.

Regards!
Gerry

Quoted 5 lines

Thanks,

tglx

.

Quoted 7 lines · Yijing Wang

Quoted 5 lines · Marc Zyngier

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

Quoted 6 lines

Sorry, this is my first part to refactor MSI related code, now how to get pci msi_controller depends arch functions(pcibios_msi_controller() or arch_setup_msi_irq()), we are working on generic pci_host_bridge, after that, we could eventually eliminate MSI arch functions and find pci dev 's msi controller by pci_host_bridge->get_msi_controller().

The main question is why you think that pci_host_bridge is the proper place to store that information.

On x86 we have DMAR units associated to a single device. Each DMAR unit is a seperate MSI irq domain.

Can you guarantee that the pci_host_bridge is the right point to provide the association of the device to the irq domain?

So the real question is:

What is the association level requirement to properly identify the irqdomain for a specific device on any given architecture with and without IOMMU, interrupt redirection etc.

To be honest: I don't know.

My gut feeling tells me that it's at the device level, but I really leave that decision to the experts in that field.

Thanks,

tglx

.

Quoted 32 lines · Yijing Wang

Quoted 7 lines · Marc Zyngier

Quoted 5 lines

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

Quoted 6 lines

Sorry, this is my first part to refactor MSI related code, now how to get pci msi_controller depends arch functions(pcibios_msi_controller() or arch_setup_msi_irq()), we are working on generic pci_host_bridge, after that, we could eventually eliminate MSI arch functions and find pci dev 's msi controller by pci_host_bridge->get_msi_controller().

The main question is why you think that pci_host_bridge is the proper place to store that information.

On x86 we have DMAR units associated to a single device. Each DMAR unit is a seperate MSI irq domain.

Can you guarantee that the pci_host_bridge is the right point to provide the association of the device to the irq domain?

So the real question is:

What is the association level requirement to properly identify the irqdomain for a specific device on any given architecture with and without IOMMU, interrupt redirection etc.

To be honest: I don't know.

Quoted 2 lines

My gut feeling tells me that it's at the device level, but I really leave that decision to the experts in that field.

I choose the pci_host_bridge to place the .get_msi_ctrl() ops, because I think how to associate pci_dev and msi_controller is platform specific, and we could initialize pci_host_bridge in platform pci host drivers to avoid call platform specific functions when we scan or setup a pci device.

Quoted 7 lines · Yijing Wang

Quoted 5 lines · Marc Zyngier

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

Quoted 6 lines

Sorry, this is my first part to refactor MSI related code, now how to get pci msi_controller depends arch functions(pcibios_msi_controller() or arch_setup_msi_irq()), we are working on generic pci_host_bridge, after that, we could eventually eliminate MSI arch functions and find pci dev 's msi controller by pci_host_bridge->get_msi_controller().

The main question is why you think that pci_host_bridge is the proper place to store that information.

On x86 we have DMAR units associated to a single device. Each DMAR unit is a seperate MSI irq domain.

Can you guarantee that the pci_host_bridge is the right point to provide the association of the device to the irq domain?

So the real question is:

What is the association level requirement to properly identify the irqdomain for a specific device on any given architecture with and without IOMMU, interrupt redirection etc.

To be honest: I don't know.

My gut feeling tells me that it's at the device level, but I really leave that decision to the experts in that field.

Quoted 10 lines · Thomas Gleixner

So the real question is:

What is the association level requirement to properly identify the irqdomain for a specific device on any given architecture with and without IOMMU, interrupt redirection etc.

To be honest: I don't know.

My gut feeling tells me that it's at the device level, but I really leave that decision to the experts in that field.

Given the above requirement (single device associated to DMAR), I can see two possibilities:
- we represent DMAR as a single PCI bus: feels a bit artificial
- we move the MSI domain to the device, as you suggested.

The second one seems a lot more attractive to me.

Also, it is not clear to me what is the advantage of getting rid of the MSI controller. By doing so, we loose an important part of the topology information (the irq domain is another level of abstraction).

Marc,

Quoted 12 lines · Marc Zyngier

Quoted 10 lines · Thomas Gleixner

So the real question is:

What is the association level requirement to properly identify the irqdomain for a specific device on any given architecture with and without IOMMU, interrupt redirection etc.

To be honest: I don't know.

My gut feeling tells me that it's at the device level, but I really leave that decision to the experts in that field.

Quoted 6 lines

Given the above requirement (single device associated to DMAR), I can see two possibilities:
- we represent DMAR as a single PCI bus: feels a bit artificial
- we move the MSI domain to the device, as you suggested.

The second one seems a lot more attractive to me.

And that's very useful if you want to support MSI on non PCI devices.

Quoted 3 lines

Also, it is not clear to me what is the advantage of getting rid of the MSI controller. By doing so, we loose an important part of the topology information (the irq domain is another level of abstraction).

That was probably my misunderstanding of the msi controller. I had the impression it's just there to expose the MSI properties of a device, i.e. a magic wrapper which can be replaced by the MSI irqdomain work.

If that handles other information as well, then it's probably a misnomer to begin with.

Hi Thomas,

Quoted 24 lines · Thomas Gleixner

Quoted 6 lines · Yijing Wang

Quoted 5 lines · Marc Zyngier

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

Quoted 6 lines

Sorry, this is my first part to refactor MSI related code, now how to get pci msi_controller depends arch functions(pcibios_msi_controller() or arch_setup_msi_irq()), we are working on generic pci_host_bridge, after that, we could eventually eliminate MSI arch functions and find pci dev 's msi controller by pci_host_bridge->get_msi_controller().

The main question is why you think that pci_host_bridge is the proper place to store that information.

On x86 we have DMAR units associated to a single device. Each DMAR unit is a seperate MSI irq domain.

Can you guarantee that the pci_host_bridge is the right point to provide the association of the device to the irq domain?

So the real question is:

Quoted 8 lines

What is the association level requirement to properly identify the irqdomain for a specific device on any given architecture with and without IOMMU, interrupt redirection etc.

To be honest: I don't know.

My gut feeling tells me that it's at the device level, but I really leave that decision to the experts in that field.

Given the above requirement (single device associated to DMAR), I can see two possibilities:
- we represent DMAR as a single PCI bus: feels a bit artificial
- we move the MSI domain to the device, as you suggested.

The second one seems a lot more attractive to me. What I don't completely see is how the host bridge has all required the knowledge.

Also, it is not clear to me what is the advantage of getting rid of the MSI controller. By doing so, we loose an important part of the topology information (the irq domain is another level of abstraction).

Thanks,

	M.

Quoted 5 lines · Marc Zyngier

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

Sorry, this is my first part to refactor MSI related code, now
how to get pci msi_controller depends arch functions(pcibios_msi_controller() or arch_setup_msi_irq()), we are working on generic pci_host_bridge, after that, we could eventually eliminate MSI arch functions and find pci dev 's msi controller by pci_host_bridge->get_msi_controller().

Marc, could you tell me what pci host driver in your test platform ?

Quoted 12 lines · Marc Zyngier

Quoted 5 lines

Bjorn, Yijing,

I've just realized that patch c167caf8d174 (PCI/MSI: Remove useless bus->msi assignment) completely breaks MSI on arm64 when using the new MSI stacked domain:

Sorry, this is my first part to refactor MSI related code, now
how to get pci msi_controller depends arch functions(pcibios_msi_controller() or arch_setup_msi_irq()), we are working on generic pci_host_bridge, after that, we could eventually eliminate MSI arch functions and find pci dev 's msi controller by pci_host_bridge->get_msi_controller().

Quoted 1 line

Marc, could you tell me what pci host driver in your test platform ?

I'm using pci-host-generic (with a couple of patches to help it fit the new "generic pci" infrastructure).

This lives at:
git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
irq/arm64-pci

Thanks,

	M.