[RFC] PPC: MPIC: necessary readback after EOI?

[RFC] PPC: MPIC: necessary readback after EOI?

[Purcareata Bogdan]

Hello,

While doing some performance testing of a KVM guest on a PPC platform, I 
noticed that there's a read of the CPU_WHOAMI register after each MPIC 
EOI [1]. This has been present since the initial implementation of the 
MPIC driver [2]. In a KVM virtualized environment, this results in an 
additional kvm_exit.

Is the read back necessary? Is it used to provide some sort of 
synchronization mechanism, making sure that nothing else is executed 
until the EOI write is finished? I eliminated the mpic_cpu_read call and 
run the kernel on hardware and noticed no anomaly, however I am not sure 
of all the implications and race conditions it might lead to.

I was curious why the mpic_cpu_read(MPIC_INFO(CPU_WHOAMI)) was there in 
the first place and if it's still needed. If it's still required, I 
guess a better approach is to eliminate the call only if the kernel is 
running on the KVM guest side, where the MPIC is emulated and no longer 
requires a readback.

Thank you,
Bogdan P.

[1] http://lxr.free-electrons.com/source/arch/powerpc/sysdev/mpic.c#L659
[2] https://lkml.org/lkml/2004/10/22/483

Re: [RFC] PPC: MPIC: necessary readback after EOI?

[Andreas Mohr]

Hi,

> I was curious why the mpic_cpu_read(MPIC_INFO(CPU_WHOAMI)) was there in
> the first place and if it's still needed. If it's still required, I
> guess a better approach is to eliminate the call only if the kernel is
> running on the KVM guest side, where the MPIC is emulated and no longer
> requires a readback.

"Why not?"

A mechanism being "emulated"/"virtual" or not
may not necessarily be much of a distinction (if at all!).
The readback might be required
to properly fulfill all requirements
of a full state change protocol specification,
which might easily be the case for both RS(*) and virtual hardware.
And especially for virtual hardware
such a "readback" event
might be an extremely important "end of transaction" marker
which may often be needed for freeing of temporary resources etc.

I'm talking out of my *ss without any MPIC specifics here
(and especially not why the readback there actually is needed -
if that doesn't happen to be the case for PCI Posting reasons or some such),
but it's just intended as food for thought :)

*) Real Silicon (rather than RL - Real Life)

HTH,

Andreas Mohr

Re: [RFC] PPC: MPIC: necessary readback after EOI?

[Scott Wood]

On Mon, 2015-01-05 at 18:46 +0100, Andreas Mohr wrote:
> Hi,
> 
> > I was curious why the mpic_cpu_read(MPIC_INFO(CPU_WHOAMI)) was there in
> > the first place and if it's still needed. If it's still required, I
> > guess a better approach is to eliminate the call only if the kernel is
> > running on the KVM guest side, where the MPIC is emulated and no longer
> > requires a readback.
> 
> "Why not?"
> 
> A mechanism being "emulated"/"virtual" or not
> may not necessarily be much of a distinction (if at all!).
> The readback might be required
> to properly fulfill all requirements
> of a full state change protocol specification,
> which might easily be the case for both RS(*) and virtual hardware.
> And especially for virtual hardware
> such a "readback" event
> might be an extremely important "end of transaction" marker
> which may often be needed for freeing of temporary resources etc.

I'm not convinced that it's required in real silicon (though there are
many MPIC implementations which have their own quirks...), and I'm 100%
sure that it's not required in the QEMU/KVM implementation of MPIC.

It would have been nice if a code comment explained why it was doing the
readback...  I don't see any particular need to wait for EOI completion
here (unlike when masking).

-Scott

Re: [RFC] PPC: MPIC: necessary readback after EOI?

[Andreas Mohr]

[CC related ppl]

On Mon, Jan 05, 2015 at 12:10:54PM -0600, Scott Wood wrote:
> On Mon, 2015-01-05 at 18:46 +0100, Andreas Mohr wrote:
> > Hi,
> > 
> > > I was curious why the mpic_cpu_read(MPIC_INFO(CPU_WHOAMI)) was there in
> > > the first place and if it's still needed. If it's still required, I
> > > guess a better approach is to eliminate the call only if the kernel is
> > > running on the KVM guest side, where the MPIC is emulated and no longer
> > > requires a readback.
> > 
> > "Why not?"
> > 
> > A mechanism being "emulated"/"virtual" or not
> > may not necessarily be much of a distinction (if at all!).
> > The readback might be required
> > to properly fulfill all requirements
> > of a full state change protocol specification,
> > which might easily be the case for both RS(*) and virtual hardware.
> > And especially for virtual hardware
> > such a "readback" event
> > might be an extremely important "end of transaction" marker
> > which may often be needed for freeing of temporary resources etc.
> 
> I'm not convinced that it's required in real silicon (though there are
> many MPIC implementations which have their own quirks...), and I'm 100%
> sure that it's not required in the QEMU/KVM implementation of MPIC.
> 
> It would have been nice if a code comment explained why it was doing the
> readback...  I don't see any particular need to wait for EOI completion
> here (unlike when masking).

Hmm, yeah.

git clone git://git.kernel.org/pub/scm/linux/kernel/git/history/history.git
git checkout v2.6.11
git blame ./ppc64/kernel/mpic.c
git show 378193eb
    [PATCH] ppc64: Rewrite the openpic driver

+/* Send an EOI */
+static inline void mpic_eoi(struct mpic *mpic)
+{
+       mpic_cpu_write(MPIC_CPU_EOI, 0);
+       (void)mpic_cpu_read(MPIC_CPU_WHOAMI);
+}


-static void openpic_eoi(void)
-{
-       DECL_THIS_CPU;
-
-       CHECK_THIS_CPU;
-       openpic_write(&OpenPIC->THIS_CPU.EOI, 0);
-       /* Handle PCI write posting */
-       (void)openpic_read(&OpenPIC->THIS_CPU.EOI);
-}


So, this does seem to be about PCI posted writes after all.
Which begs the question whether all PIC hardware is connected via PCI bus,
which... is not the case for emulated hardware at least, I'd think.

And it's somewhat unfortunate
that the comment in fact was removed in that commit
(perhaps reinstate this comment in all of the various mpic.c life forms?).

Andreas Mohr

Re: [RFC] PPC: MPIC: necessary readback after EOI?

[Scott Wood]

On Mon, 2015-01-05 at 19:43 +0100, Andreas Mohr wrote:
> [CC related ppl]
> 
> On Mon, Jan 05, 2015 at 12:10:54PM -0600, Scott Wood wrote:
> > On Mon, 2015-01-05 at 18:46 +0100, Andreas Mohr wrote:
> > > Hi,
> > > 
> > > > I was curious why the mpic_cpu_read(MPIC_INFO(CPU_WHOAMI)) was there in
> > > > the first place and if it's still needed. If it's still required, I
> > > > guess a better approach is to eliminate the call only if the kernel is
> > > > running on the KVM guest side, where the MPIC is emulated and no longer
> > > > requires a readback.
> > > 
> > > "Why not?"
> > > 
> > > A mechanism being "emulated"/"virtual" or not
> > > may not necessarily be much of a distinction (if at all!).
> > > The readback might be required
> > > to properly fulfill all requirements
> > > of a full state change protocol specification,
> > > which might easily be the case for both RS(*) and virtual hardware.
> > > And especially for virtual hardware
> > > such a "readback" event
> > > might be an extremely important "end of transaction" marker
> > > which may often be needed for freeing of temporary resources etc.
> > 
> > I'm not convinced that it's required in real silicon (though there are
> > many MPIC implementations which have their own quirks...), and I'm 100%
> > sure that it's not required in the QEMU/KVM implementation of MPIC.
> > 
> > It would have been nice if a code comment explained why it was doing the
> > readback...  I don't see any particular need to wait for EOI completion
> > here (unlike when masking).
> 
> Hmm, yeah.
> 
> git clone git://git.kernel.org/pub/scm/linux/kernel/git/history/history.git
> git checkout v2.6.11
> git blame ./ppc64/kernel/mpic.c
> git show 378193eb
>     [PATCH] ppc64: Rewrite the openpic driver
> 
> +/* Send an EOI */
> +static inline void mpic_eoi(struct mpic *mpic)
> +{
> +       mpic_cpu_write(MPIC_CPU_EOI, 0);
> +       (void)mpic_cpu_read(MPIC_CPU_WHOAMI);
> +}
> 
> 
> -static void openpic_eoi(void)
> -{
> -       DECL_THIS_CPU;
> -
> -       CHECK_THIS_CPU;
> -       openpic_write(&OpenPIC->THIS_CPU.EOI, 0);
> -       /* Handle PCI write posting */
> -       (void)openpic_read(&OpenPIC->THIS_CPU.EOI);
> -}
> 
> 
> So, this does seem to be about PCI posted writes after all.
> Which begs the question whether all PIC hardware is connected via PCI bus,
> which... is not the case for emulated hardware at least, I'd think.
> 
> And it's somewhat unfortunate
> that the comment in fact was removed in that commit
> (perhaps reinstate this comment in all of the various mpic.c life forms?).

But even for PCI, why do we need to wait for this write to complete?
Back in the arch/ppc days ppc_md.get_irq() was called in a loop, so it
would make sense that we'd need the next IRQ to be ready by the time
it's called again, but now that there's no get_irq() loop we shouldn't
need to wait.

-Scott

Re: [RFC] PPC: MPIC: necessary readback after EOI?

[Benjamin Herrenschmidt]

On Mon, 2015-01-05 at 12:10 -0600, Scott Wood wrote:
> It would have been nice if a code comment explained why it was doing the
> readback...  I don't see any particular need to wait for EOI completion
> here (unlike when masking).

The EOI is what causes the MPIC to drop it's EE output to the CPU, if the
EOI is processed too slowly & asynchronously (posted write + 33Mhz MPIC)
we observe cases of spurrious interrupts. We had some macs basically getting
a spurrious irq for every MPIC interrupts...

Cheers,
Ben.

Re: [RFC] PPC: MPIC: necessary readback after EOI?

[Scott Wood]

On Wed, 2015-01-07 at 15:44 +0100, Benjamin Herrenschmidt wrote:
> On Mon, 2015-01-05 at 12:10 -0600, Scott Wood wrote:
> > It would have been nice if a code comment explained why it was doing the
> > readback...  I don't see any particular need to wait for EOI completion
> > here (unlike when masking).
> 
> The EOI is what causes the MPIC to drop it's EE output to the CPU, if the
> EOI is processed too slowly & asynchronously (posted write + 33Mhz MPIC)
> we observe cases of spurrious interrupts. We had some macs basically getting
> a spurrious irq for every MPIC interrupts...

Shouldn't reading INTACK be what causes the MPIC to drop its EE output?

-Scott

Re: [RFC] PPC: MPIC: necessary readback after EOI?

[Benjamin Herrenschmidt]

On Wed, 2015-01-07 at 11:04 -0600, Scott Wood wrote:
> On Wed, 2015-01-07 at 15:44 +0100, Benjamin Herrenschmidt wrote:
> > On Mon, 2015-01-05 at 12:10 -0600, Scott Wood wrote:
> > > It would have been nice if a code comment explained why it was doing the
> > > readback...  I don't see any particular need to wait for EOI completion
> > > here (unlike when masking).
> > 
> > The EOI is what causes the MPIC to drop it's EE output to the CPU, if the
> > EOI is processed too slowly & asynchronously (posted write + 33Mhz MPIC)
> > we observe cases of spurrious interrupts. We had some macs basically getting
> > a spurrious irq for every MPIC interrupts...
> 
> Shouldn't reading INTACK be what causes the MPIC to drop its EE output?

Hrm, looks like I had too much wine or something, you are correct yes,
it's the intack, so my explanation is bogus.

So we are down to possibly delaying the raising back of the CPU priority
which is not a big deal indeed, we could probably get rid of the read
back.

Ben.

Re: [RFC] PPC: MPIC: necessary readback after EOI?

[Benjamin Herrenschmidt]

On Mon, 2015-01-05 at 18:46 +0100, Andreas Mohr wrote:
> Hi,
> 
> > I was curious why the mpic_cpu_read(MPIC_INFO(CPU_WHOAMI)) was there in
> > the first place and if it's still needed. If it's still required, I
> > guess a better approach is to eliminate the call only if the kernel is
> > running on the KVM guest side, where the MPIC is emulated and no longer
> > requires a readback.
> 
> "Why not?"
> 
> A mechanism being "emulated"/"virtual" or not
> may not necessarily be much of a distinction (if at all!).
> The readback might be required
> to properly fulfill all requirements
> of a full state change protocol specification,
> which might easily be the case for both RS(*) and virtual hardware.
> And especially for virtual hardware
> such a "readback" event
> might be an extremely important "end of transaction" marker
> which may often be needed for freeing of temporary resources etc.

In that case it was purely something we added after trial and error to
correct a problem, it's not specified as necessary. Basically it's about
making the store synchronous to the MPIC logic. It's definitely not
necessary on an emulated implementation.

> I'm talking out of my *ss without any MPIC specifics here
> (and especially not why the readback there actually is needed -
> if that doesn't happen to be the case for PCI Posting reasons or some such),
> but it's just intended as food for thought :)
> 
> *) Real Silicon (rather than RL - Real Life)
> 
> HTH,
> 
> Andreas Mohr
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] PPC: MPIC: necessary readback after EOI?

[Benjamin Herrenschmidt]

On Mon, 2015-01-05 at 16:14 +0200, Purcareata Bogdan wrote:
> Hello,
> 
> While doing some performance testing of a KVM guest on a PPC platform, I 
> noticed that there's a read of the CPU_WHOAMI register after each MPIC 
> EOI [1]. This has been present since the initial implementation of the 
> MPIC driver [2]. In a KVM virtualized environment, this results in an 
> additional kvm_exit.
> 
> Is the read back necessary? Is it used to provide some sort of 
> synchronization mechanism, making sure that nothing else is executed 
> until the EOI write is finished? I eliminated the mpic_cpu_read call and 
> run the kernel on hardware and noticed no anomaly, however I am not sure 
> of all the implications and race conditions it might lead to.

It was done to ensure that the store to the EOI has reached the MPIC and
been fully processed before re-enabling interrupts on the CPU. On some
implementations, the MPIC runs quite slowly (significantly slower than
the core) and the stores to it are asynchronous, so we had situation
where we would restore interrupts while the MPIC hasn't yet de-asserted
the output line.

One way to work around the performance loss for you would be to add some
DT property to indicate to the guest that the read isn't necessary.

> I was curious why the mpic_cpu_read(MPIC_INFO(CPU_WHOAMI)) was there in 
> the first place and if it's still needed. If it's still required, I 
> guess a better approach is to eliminate the call only if the kernel is 
> running on the KVM guest side, where the MPIC is emulated and no longer 
> requires a readback.
> 
> Thank you,
> Bogdan P.
> 
> [1] http://lxr.free-electrons.com/source/arch/powerpc/sysdev/mpic.c#L659
> [2] https://lkml.org/lkml/2004/10/22/483
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] PPC: MPIC: necessary readback after EOI?

[Segher Boessenkool]

On Wed, Jan 07, 2015 at 03:40:10PM +0100, Benjamin Herrenschmidt wrote:
> One way to work around the performance loss for you would be to add some
> DT property to indicate to the guest that the read isn't necessary.

Or KVM could use a virtual interrupt controller better suited to its
needs.


Segher