A question about state machine function state_next()

A question about state machine function state_next()

[Baoquan He]

Hi Joerg,

I am reading amd iommu code because I have some knowledge about intel
iommu since review Zhenhua's fixing kdump error patches. Now there's
a question I didn't find answer.

In amd iommu state_next() is the state machine running function. However
I only found 4 function to call iommu_go_to_state() to change the state,
they are:
amd_iommu_detect()
amd_iommu_prepare()
amd_iommu_enable()
amd_iommu_init()

And they are called according to above sequence. It means only below 4
cases are checked and the code blocks are executed. Then where to call
amd_iommu_enable_interrupts() and amd_iommu_init_dma(). Could you help
to tell what I missed?

static int __init state_next(void)
{
	int ret = 0;

	switch (init_state) {
	case IOMMU_START_STATE:     //checked and execute
	case IOMMU_IVRS_DETECTED:   //checked and execute
	case IOMMU_ACPI_FINISHED:   //checked and execute
	case IOMMU_ENABLED:         //checked and execute
	...
}

Thanks
Baoquan

Re: A question about state machine function state_next()

[Joerg Roedel]

Hi Baoquan,

On Mon, Jun 01, 2015 at 11:09:40AM +0800, Baoquan He wrote:
> I am reading amd iommu code because I have some knowledge about intel
> iommu since review Zhenhua's fixing kdump error patches. Now there's
> a question I didn't find answer.
> 
> In amd iommu state_next() is the state machine running function. However
> I only found 4 function to call iommu_go_to_state() to change the state,
> they are:
> amd_iommu_detect()
> amd_iommu_prepare()
> amd_iommu_enable()
> amd_iommu_init()
> 
> And they are called according to above sequence. It means only below 4
> cases are checked and the code blocks are executed. Then where to call
> amd_iommu_enable_interrupts() and amd_iommu_init_dma(). Could you help
> to tell what I missed?
> 
> static int __init state_next(void)
> {
> 	int ret = 0;
> 
> 	switch (init_state) {
> 	case IOMMU_START_STATE:     //checked and execute
> 	case IOMMU_IVRS_DETECTED:   //checked and execute
> 	case IOMMU_ACPI_FINISHED:   //checked and execute
> 	case IOMMU_ENABLED:         //checked and execute
> 	...
> }

Yes, it is right that only these four states are used as exit-states for
the functions above. The other states are not really needed, but the
work done is these steps is. Originally my idea was to use the
intermedite states to see where initialization failed, but that is
easier to achieve elsewhere.

So this has been on my list of things to clean up, just didn't got
around to do it yet.


	Joerg

Re: A question about state machine function state_next()

[Baoquan He]

On 06/01/15 at 08:42am, Joerg Roedel wrote:
> Hi Baoquan,
> 
> On Mon, Jun 01, 2015 at 11:09:40AM +0800, Baoquan He wrote:
> > I am reading amd iommu code because I have some knowledge about intel
> > iommu since review Zhenhua's fixing kdump error patches. Now there's
> > a question I didn't find answer.
> > 
> > In amd iommu state_next() is the state machine running function. However
> > I only found 4 function to call iommu_go_to_state() to change the state,
> > they are:
> > amd_iommu_detect()
> > amd_iommu_prepare()
> > amd_iommu_enable()
> > amd_iommu_init()
> > 
> > And they are called according to above sequence. It means only below 4
> > cases are checked and the code blocks are executed. Then where to call
> > amd_iommu_enable_interrupts() and amd_iommu_init_dma(). Could you help
> > to tell what I missed?
> > 
> > static int __init state_next(void)
> > {
> > 	int ret = 0;
> > 
> > 	switch (init_state) {
> > 	case IOMMU_START_STATE:     //checked and execute
> > 	case IOMMU_IVRS_DETECTED:   //checked and execute
> > 	case IOMMU_ACPI_FINISHED:   //checked and execute
> > 	case IOMMU_ENABLED:         //checked and execute
> > 	...
> > }
> 
> Yes, it is right that only these four states are used as exit-states for
> the functions above. The other states are not really needed, but the
> work done is these steps is. Originally my idea was to use the
> intermedite states to see where initialization failed, but that is
> easier to achieve elsewhere.
> 
> So this has been on my list of things to clean up, just didn't got
> around to do it yet.

Hi Joerg,

Thanks a lot for explanation.

Then I am wondering  how amd_iommu_dma_ops is assigned. Maybe I need
check all functions more clearly. Actually I am investigating how to
port Zhenhua's kdump fix for intel iommu to amd iommu since the similar
bug happened on systems with amd iommu. If you don't mind I can make
these clean up during I understand these iommu codes. And if Zhenhua
plan to post patch for amd fix, I can help review and test. Otherwise I
can make some research and try to post a draft patch.

Thanks
Baoquan


> 
> 
> 	Joerg
> 

Re: A question about state machine function state_next()

[Joerg Roedel]

Hi Baoquan,

On Mon, Jun 01, 2015 at 05:09:02PM +0800, Baoquan He wrote:
> Then I am wondering  how amd_iommu_dma_ops is assigned. Maybe I need
> check all functions more clearly.

The AMD IOMMU driver only uses per-device dma_ops. They are assigned to
each device in device_dma_ops_init() at boot and in the
device_change_notifier() on hotplug events.


> Actually I am investigating how to port Zhenhua's kdump fix for intel
> iommu to amd iommu since the similar bug happened on systems with amd
> iommu. If you don't mind I can make these clean up during I understand
> these iommu codes. And if Zhenhua plan to post patch for amd fix, I
> can help review and test. Otherwise I can make some research and try
> to post a draft patch.

Thanks for looking into this. Unfortunatly this somehow conflicts with
my recent default-domain patch-set, which moves functionality into the
IOMMU core and converts the AMD driver to make use of it. I have no real
idea yet how to make both work, but it shouldn't be too hard. Maybe you can
have a look into that patch-set too?


	Joerg

Re: A question about state machine function state_next()

[Baoquan He]

On 06/01/15 at 11:21am, Joerg Roedel wrote:
> Hi Baoquan,
> 
> On Mon, Jun 01, 2015 at 05:09:02PM +0800, Baoquan He wrote:
> > Then I am wondering  how amd_iommu_dma_ops is assigned. Maybe I need
> > check all functions more clearly.
> 
> The AMD IOMMU driver only uses per-device dma_ops. They are assigned to
> each device in device_dma_ops_init() at boot and in the
> device_change_notifier() on hotplug events.
> 
> 
> > Actually I am investigating how to port Zhenhua's kdump fix for intel
> > iommu to amd iommu since the similar bug happened on systems with amd
> > iommu. If you don't mind I can make these clean up during I understand
> > these iommu codes. And if Zhenhua plan to post patch for amd fix, I
> > can help review and test. Otherwise I can make some research and try
> > to post a draft patch.
> 
> Thanks for looking into this. Unfortunatly this somehow conflicts with
> my recent default-domain patch-set, which moves functionality into the
> IOMMU core and converts the AMD driver to make use of it. I have no real
> idea yet how to make both work, but it shouldn't be too hard. Maybe you can
> have a look into that patch-set too?

Yeah, sure. If you have got a plan I can help review when you post them. 

Thanks for the update.

Thanks
Baoquan

Re: A question about state machine function state_next()

[Joerg Roedel]

On Mon, Jun 01, 2015 at 06:19:26PM +0800, Baoquan He wrote:
> On 06/01/15 at 11:21am, Joerg Roedel wrote:
> > Thanks for looking into this. Unfortunatly this somehow conflicts with
> > my recent default-domain patch-set, which moves functionality into the
> > IOMMU core and converts the AMD driver to make use of it. I have no real
> > idea yet how to make both work, but it shouldn't be too hard. Maybe you can
> > have a look into that patch-set too?
> 
> Yeah, sure. If you have got a plan I can help review when you post them. 
> 
> Thanks for the update.

I actually posted them last week :) Find them here:

	https://lkml.org/lkml/2015/5/28/488

Cheers,

	Joerg

Re: A question about state machine function state_next()

[Baoquan He]

On 06/01/15 at 12:33pm, Joerg Roedel wrote:
> On Mon, Jun 01, 2015 at 06:19:26PM +0800, Baoquan He wrote:
> > On 06/01/15 at 11:21am, Joerg Roedel wrote:
> > > Thanks for looking into this. Unfortunatly this somehow conflicts with
> > > my recent default-domain patch-set, which moves functionality into the
> > > IOMMU core and converts the AMD driver to make use of it. I have no real
> > > idea yet how to make both work, but it shouldn't be too hard. Maybe you can
> > > have a look into that patch-set too?
> > 
> > Yeah, sure. If you have got a plan I can help review when you post them. 
> > 
> > Thanks for the update.
> 
> I actually posted them last week :) Find them here:
> 
> 	https://lkml.org/lkml/2015/5/28/488

Great, I will review them and try to learn more about iommu.

Thanks
Baoquan

Re: A question about state machine function state_next()

[Baoquan He]

On 06/01/15 at 11:21am, Joerg Roedel wrote:
> Hi Baoquan,
> 
> On Mon, Jun 01, 2015 at 05:09:02PM +0800, Baoquan He wrote:
> > Then I am wondering  how amd_iommu_dma_ops is assigned. Maybe I need
> > check all functions more clearly.
> 
> The AMD IOMMU driver only uses per-device dma_ops. They are assigned to
> each device in device_dma_ops_init() at boot and in the
> device_change_notifier() on hotplug events.

Checked the code again, it may be a code bug if this is done in
device_dma_ops_init(). Since this is called in
amd_iommu_init_dma_ops()<-amd_iommu_init_dma(). And amd_iommu_init_dma()
is only called in case IOMMU_INTERRUPTS_EN code block. According to the
code flow if I understand correctly, it can't be here at boot stage.
state_next() will change the state according to the calling times. And
in amd iommu only four times to call iommu_go_to_state, it can't go to
IOMMU_PCI_INIT and the after cases.

I am not sure if my understanding is correct, or I missed something.

Thanks
Baoquan

> 
> 
> > Actually I am investigating how to port Zhenhua's kdump fix for intel
> > iommu to amd iommu since the similar bug happened on systems with amd
> > iommu. If you don't mind I can make these clean up during I understand
> > these iommu codes. And if Zhenhua plan to post patch for amd fix, I
> > can help review and test. Otherwise I can make some research and try
> > to post a draft patch.
> 
> Thanks for looking into this. Unfortunatly this somehow conflicts with
> my recent default-domain patch-set, which moves functionality into the
> IOMMU core and converts the AMD driver to make use of it. I have no real
> idea yet how to make both work, but it shouldn't be too hard. Maybe you can
> have a look into that patch-set too?
> 
> 
> 	Joerg
> 

Re: A question about state machine function state_next()

[Joerg Roedel]

On Mon, Jun 01, 2015 at 07:18:02PM +0800, Baoquan He wrote:
> Checked the code again, it may be a code bug if this is done in
> device_dma_ops_init(). Since this is called in
> amd_iommu_init_dma_ops()<-amd_iommu_init_dma(). And amd_iommu_init_dma()
> is only called in case IOMMU_INTERRUPTS_EN code block. According to the
> code flow if I understand correctly, it can't be here at boot stage.
> state_next() will change the state according to the calling times. And
> in amd iommu only four times to call iommu_go_to_state, it can't go to
> IOMMU_PCI_INIT and the after cases.
> 
> I am not sure if my understanding is correct, or I missed something.

You probably missed the loop in iommu_go_to_state() which will call
state_next() until it reaches either the requested state or an error
state.


	Joerg

Re: A question about state machine function state_next()

[Baoquan He]

On 06/01/15 at 02:01pm, Joerg Roedel wrote:
> On Mon, Jun 01, 2015 at 07:18:02PM +0800, Baoquan He wrote:
> > Checked the code again, it may be a code bug if this is done in
> > device_dma_ops_init(). Since this is called in
> > amd_iommu_init_dma_ops()<-amd_iommu_init_dma(). And amd_iommu_init_dma()
> > is only called in case IOMMU_INTERRUPTS_EN code block. According to the
> > code flow if I understand correctly, it can't be here at boot stage.
> > state_next() will change the state according to the calling times. And
> > in amd iommu only four times to call iommu_go_to_state, it can't go to
> > IOMMU_PCI_INIT and the after cases.
> > 
> > I am not sure if my understanding is correct, or I missed something.
> 
> You probably missed the loop in iommu_go_to_state() which will call
> state_next() until it reaches either the requested state or an error
> state.

Ah, right. IOMMU_INITIALIZED can cover all left cases until
IOMMU_ENABLED. Many thanks.

Thanks
Baoquan