Messages in this thread | | | Date | Sun, 12 Apr 2009 16:06:50 +0200 | From | Ingo Molnar <> | Subject | Re: [RFC -tip] x86: do_IRQ - send APIC EOI for x86-32 on irq without handler v3 |
| |
* Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> [Ingo Molnar - Fri, Apr 10, 2009 at 04:00:23PM +0200] > | > | * Cyrill Gorcunov <gorcunov@gmail.com> wrote: > | > | > [Ingo Molnar - Fri, Apr 10, 2009 at 02:27:50PM +0200] > | > | > | > | * Cyrill Gorcunov <gorcunov@openvz.org> wrote: > | > | > | > | > Ingo, I've checked the sources and as far as I see > | > | > we could NOP'ify apic->write indeed but I have > | > | > an internal feeling that this will bring us more problem > | > | > in future (for example it could be the following scenario: > | > | > some screwed APIC would require cleaning of LVT's or > | > | > IRR after resume regardless if it was initialized > | > | > or not at all). Mostly I mean that the idea of making > | > | > apic->write NOP'ified is quite elegant indeed but > | > | > cut off the subset of apic operations (we need > | > | > apic->read anyway) somehow bothering me from inside :) > | > | > | > | it's as if assigned a special type of 'dummy apic' struct apic. It > | > | wont cause problems down the line: we use the new APIC driver > | > | infrastructure to abstract out quirks. > | > > | > Well, it's not that new actually :-) > | > | Yeah, i mean the new unified/modernized code in 2.6.30-to-be. > | > | > | > | > | one small detail: > | > | > | > | > +/* Ack APIC irq if it's enabled only */ > | > | > +static inline void ack_APIC_irq_safe(void) > | > | > +{ > | > | > +#ifdef CONFIG_X86_LOCAL_APIC > | > | > + if (cpu_has_apic) > | > | > + ack_APIC_irq(); > | > | > +#endif > | > | > | > | we dont need the cpu_has_apic check there, do we? In the > | > | !cpu_has_apic the ->write method should be a dummy. > | > > | > Yes. In case you're talking about it'll not be needed > | > (we will find earlier whether cpu_has_apic or not). > | > | yeah. > | > | Ingo > | > > Ingo, > > I think you meant something like the patch below. It's > not finished yet -- I need to find out right place for > calling freshly introduced apic_disable_write_op. > Will continue tomorrow. > > But even having it not completed yet I would like to > get some feedbackabout code structure in general.
Yeah, the goal now looks good.
Note, i'd suggest to not expose it like this:
> extern u64 native_apic_icr_read(void); > +extern void native_apic_write_dummy(u32 reg, u32 v); > > #define EIM_8BIT_APIC_ID 0 > #define EIM_32BIT_APIC_ID 1 > @@ -372,6 +373,15 @@ static inline void apic_write(u32 reg, u > apic->write(reg, val); > } > > +/* > + * right after this call apic->write doesn't do anything > + * note that there is no restore operation it works one way > + */ > +static inline void apic_disable_write_op(void) > +{ > + apic->write = native_apic_write_dummy; > +} > +
But have a central and opaque:
extern void apic_disable(void);
function, defined in apic.c - which does all the internal details (like installing a dummy ->write) entry.
Ingo
| |