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

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

[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.

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.
-- 
Jazz is not dead. It just smells funny...

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

[Bjorn Helgaas]

On Thu, Nov 20, 2014 at 04:31:45PM +0000, Marc Zyngier wrote:
> 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.

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.

Bjorn

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

[Thomas Gleixner]

On Thu, 20 Nov 2014, Bjorn Helgaas wrote:
> On Thu, Nov 20, 2014 at 04:31:45PM +0000, Marc Zyngier wrote:
> > 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.
> 
> 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.

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

Let me know ASAP.

Thanks,

	tglx

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

[Bjorn Helgaas]

On Thu, Nov 20, 2014 at 4:10 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 20 Nov 2014, Bjorn Helgaas wrote:
>> On Thu, Nov 20, 2014 at 04:31:45PM +0000, Marc Zyngier wrote:
>> > 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.
>>
>> 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.

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.

Bjorn

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

[Marc Zyngier]

Hi Bjorn,

On 20/11/14 23:30, Bjorn Helgaas wrote:
> On Thu, Nov 20, 2014 at 4:10 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Thu, 20 Nov 2014, Bjorn Helgaas wrote:
>>> On Thu, Nov 20, 2014 at 04:31:45PM +0000, Marc Zyngier wrote:
>>>> 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.
>>>
>>> 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.
> 
> 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.

The breakage only happens once we'll add the MSI controller drivers that
are actively using the stacked domain code (GICv2m and GICv3 ITS). That
should give us some flexibility to tweak things.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Yijing Wang]

>> 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.

> 
> 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.


> 
> 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
> 
> .
> 


-- 
Thanks!
Yijing

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

[Jiang Liu]

On 2014/11/21 9:54, Yijing Wang wrote:
>>> 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.
> 
>>
>> 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;	
> }
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
> 
> If I miss something, please let me know, thanks.
> 
> Thanks!
> Yijing.
> 
> 
>>
>> 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
>>
>> .
>>
> 
> 

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

[Yijing Wang]

On 2014/11/21 10:25, Jiang Liu wrote:
> On 2014/11/21 9:54, Yijing Wang wrote:
>>>> 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.
>>
>>>
>>> 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;	
>> }
> 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?

So the final solution depends the MSI refactoring work progress.
		   (glue layer)
I prefer pci_dev->msi_controller->(msi irq hierarchy domain)/(normal msi irq allocation code).
If we want to eliminate msi_controller, we must force all PCI MSI code to use hierarchy
irq domain. I doubt whether it is worth to do.

Thanks!
Yijing.

> Regards!
> Gerry
>>
>> If I miss something, please let me know, thanks.
>>
>> Thanks!
>> Yijing.
>>
>>
>>>
>>> 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
>>>
>>> .
>>>
>>
>>
> 
> .
> 


-- 
Thanks!
Yijing

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

[Marc Zyngier]

Yijing,

On 21/11/14 01:54, Yijing Wang wrote:
>>> 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.
> 
>>
>> 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.

That feels slightly convoluted for something that should be a very
simple operation. Does this mean you're trying to represent a situation
where:
- a single host bridge has multiple MSI controllers,
- this bridge serves multiple busses,
- devices on the same bus can talk to different MSI controllers?

That would be the only case where the current way we pass the
msi_controller around wouldn't work.

If that's what you're trying to do, I can see how this work, but I'd
suggest you put that infrastructure in place before tearing down the
existing one. This means being having support at the host-bridge level
and reasonable defaults for the non-complicated case where bus->msi is
exactly what you want.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Bjorn Helgaas]

On Fri, Nov 21, 2014 at 09:54:40AM +0800, Yijing Wang wrote:
> >> 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.
> 
> > 
> > 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.

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.

> 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;	
> }

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@google.com

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

[Yijing Wang]

在 2014/11/22 1:31, Bjorn Helgaas 写道:
> On Fri, Nov 21, 2014 at 09:54:40AM +0800, Yijing Wang wrote:
>>>> 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.
>>
>>> 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.
> 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.

Agree, we could use pci_msi_controller() to find msi_controller for 
pci_dev before a
better common way found.

>
>> 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;	
>> }
> 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.

OK. We need a better solution, not only for arm, also need to consider 
arm64 and
other platforms.

>
> 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@google.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Yijing Wang]

On 2014/11/21 0:31, Marc Zyngier wrote:
> 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 ?

> 
> 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.

Bjorn, could you help to revert this patch ?


> 
> Thanks,
> 
> 	M.
> 


-- 
Thanks!
Yijing

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

[Thomas Gleixner]

On Fri, 21 Nov 2014, Yijing Wang wrote:
> On 2014/11/21 0:31, Marc Zyngier wrote:
> > 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().

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

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

[Jiang Liu]

On 2014/11/21 9:46, Thomas Gleixner wrote:
> On Fri, 21 Nov 2014, Yijing Wang wrote:
>> On 2014/11/21 0:31, Marc Zyngier wrote:
>>> 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().
> 
> 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.
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
> 
> Thanks,
> 
> 	tglx
> 

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

[Yijing Wang]

On 2014/11/21 10:03, Jiang Liu wrote:
> On 2014/11/21 9:46, Thomas Gleixner wrote:
>> On Fri, 21 Nov 2014, Yijing Wang wrote:
>>> On 2014/11/21 0:31, Marc Zyngier wrote:
>>>> 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().
>>
>> 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.
> 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.

Hi Gerry, I mean we could find msi_controller by calling
pci_host_bridge->pci_get_msi_irqdomain/msi_controller(struct pci_dev *pdev)
to avoid arch weak function like pcibios_get_msi_controller(struct pci_dev *pdev). :)


> Regards!
> Gerry
>>
>> Thanks,
>>
>> 	tglx
>>
> 
> .
> 


-- 
Thanks!
Yijing

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

[Yijing Wang]

On 2014/11/21 9:46, Thomas Gleixner wrote:
> On Fri, 21 Nov 2014, Yijing Wang wrote:
>> On 2014/11/21 0:31, Marc Zyngier wrote:
>>> 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().
> 
> 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.

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.

Thanks!
Yijing.

> 
> Thanks,
> 
> 	tglx
> 
> .
> 


-- 
Thanks!
Yijing

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

[Lucas Stach]

Am Freitag, den 21.11.2014, 10:05 +0800 schrieb Yijing Wang:
> On 2014/11/21 9:46, Thomas Gleixner wrote:
> > On Fri, 21 Nov 2014, Yijing Wang wrote:
> >> On 2014/11/21 0:31, Marc Zyngier wrote:
> >>> 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().
> > 
> > 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.
> 
> 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.
> 
I'm in favor of having the irqdomain attached to struct device even.
Please keep in mind that going forward PCI will not be the only bus
using MSI. Also having it attached to the device allows you to find the
the domain by just walking up the chain of parent devices until you find
one with an attached domain. A host bridge isn't different in that
regard from any other device. This should work for all platforms AFAICS.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

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

[Marc Zyngier]

Hi Thomas,

On 21/11/14 01:46, Thomas Gleixner wrote:
> On Fri, 21 Nov 2014, Yijing Wang wrote:
>> On 2014/11/21 0:31, Marc Zyngier wrote:
>>> 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().
> 
> 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.

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.
-- 
Jazz is not dead. It just smells funny...

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

[Thomas Gleixner]

Marc,

On Fri, 21 Nov 2014, Marc Zyngier wrote:
> On 21/11/14 01:46, Thomas Gleixner wrote:
> > 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.

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

> 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.

Thanks,

	tglx

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

[Marc Zyngier]

On 21/11/14 10:49, Thomas Gleixner wrote:
> Marc,
> 
> On Fri, 21 Nov 2014, Marc Zyngier wrote:
>> On 21/11/14 01:46, Thomas Gleixner wrote:
>>> 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.
> 
> And that's very useful if you want to support MSI on non PCI
> devices.
> 
>> 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.

At the moment, it serves multiple purpose:
- MSI configuration via setup_irq/teardown_irq: this is what most 32bit
ARM system are using (it has been introduced last year for that very
purpose)
- PCI controller vs MSI hw matching: both arm and arm64 are using this
to associate the PCI controller with the matching MSI hw, using the
device tree (msi-controller and msi-parent properties in DT, of_node
field in the msi_controller structure).
- associated irq_domain (I've added that bit).

I expect setup_irq and co to disappear at some point, once all the users
have been converted to stacked domains. The matching information is
harder to let go though. But that could be a structure that doesn't have
to be inflicted on everyone, if we can go from:

pci-device -> msi-controller -> irq-domain

to

pci-device -> irq-domain -> dt-topology-information

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Yijing Wang]

在 2014/11/21 18:29, Marc Zyngier 写道:
> Hi Thomas,
>
> On 21/11/14 01:46, Thomas Gleixner wrote:
>> On Fri, 21 Nov 2014, Yijing Wang wrote:
>>> On 2014/11/21 0:31, Marc Zyngier wrote:
>>>> 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().
>> 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.
> 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.

Hmmm, maybe I'm in the wrong direction,  I need to think more about it.

Thanks!
Yijing.


>
> 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.

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

[Marc Zyngier]

On 21/11/14 01:22, Yijing Wang wrote:
> On 2014/11/21 0:31, Marc Zyngier wrote:
>> 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 ?

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.
-- 
Jazz is not dead. It just smells funny...

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

[Yijing Wang]

在 2014/11/21 18:11, Marc Zyngier 写道:
> On 21/11/14 01:22, Yijing Wang wrote:
>> On 2014/11/21 0:31, Marc Zyngier wrote:
>>> 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 ?
> 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, I would have a look at it.

>
> Thanks,
>
> 	M.