| Message ID | 1343222672-25312-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 2012-07-25 15:24, Peter Maydell wrote: > Rename the function kvm_irqchip_set_irq() to kvm_inject_async_irq(), > since it can be used for asynchronous interrupt injection whether > there is a full irqchip model in the kernel or not. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/kvm/i8259.c | 2 +- > hw/kvm/ioapic.c | 2 +- > kvm-all.c | 6 +++--- > kvm.h | 2 +- > 4 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/hw/kvm/i8259.c b/hw/kvm/i8259.c > index 94d1b9a..ab970db 100644 > --- a/hw/kvm/i8259.c > +++ b/hw/kvm/i8259.c > @@ -94,7 +94,7 @@ static void kvm_pic_set_irq(void *opaque, int irq, int level) > { > int delivered; > > - delivered = kvm_irqchip_set_irq(kvm_state, irq, level); > + delivered = kvm_inject_async_irq(kvm_state, irq, level); > apic_report_irq_delivered(delivered); > } > > diff --git a/hw/kvm/ioapic.c b/hw/kvm/ioapic.c > index 3ae3175..d7add35 100644 > --- a/hw/kvm/ioapic.c > +++ b/hw/kvm/ioapic.c > @@ -82,7 +82,7 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level) > KVMIOAPICState *s = opaque; > int delivered; > > - delivered = kvm_irqchip_set_irq(kvm_state, s->kvm_gsi_base + irq, level); > + delivered = kvm_inject_async_irq(kvm_state, s->kvm_gsi_base + irq, level); > apic_report_irq_delivered(delivered); > } > > diff --git a/kvm-all.c b/kvm-all.c > index 3354c6f..9f14274 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -853,7 +853,7 @@ static void kvm_handle_interrupt(CPUArchState *env, int mask) > } > } > > -int kvm_irqchip_set_irq(KVMState *s, int irq, int level) > +int kvm_inject_async_irq(KVMState *s, int irq, int level) > { > struct kvm_irq_level event; > int ret; > @@ -864,7 +864,7 @@ int kvm_irqchip_set_irq(KVMState *s, int irq, int level) > event.irq = irq; > ret = kvm_vm_ioctl(s, s->irqchip_inject_ioctl, &event); > if (ret < 0) { > - perror("kvm_set_irqchip_line"); > + perror("kvm_inject_async_irq"); > abort(); > } > > @@ -1089,7 +1089,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg) > > assert(route->kroute.type == KVM_IRQ_ROUTING_MSI); > > - return kvm_irqchip_set_irq(s, route->kroute.gsi, 1); > + return kvm_inject_async_irq(s, route->kroute.gsi, 1); > } > > int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg) > diff --git a/kvm.h b/kvm.h > index 00abe36..cfdc95e 100644 > --- a/kvm.h > +++ b/kvm.h > @@ -144,7 +144,7 @@ int kvm_arch_on_sigbus(int code, void *addr); > > void kvm_arch_init_irq_routing(KVMState *s); > > -int kvm_irqchip_set_irq(KVMState *s, int irq, int level); > +int kvm_inject_async_irq(KVMState *s, int irq, int level); > int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg); > > void kvm_irqchip_add_irq_route(KVMState *s, int gsi, int irqchip, int pin); > Acked-by: Jan Kiszka <jan.kiszka@siemens.com>
On 07/25/2012 04:24 PM, Peter Maydell wrote: > Rename the function kvm_irqchip_set_irq() to kvm_inject_async_irq(), > since it can be used for asynchronous interrupt injection whether > there is a full irqchip model in the kernel or not. > > @@ -144,7 +144,7 @@ int kvm_arch_on_sigbus(int code, void *addr); > > void kvm_arch_init_irq_routing(KVMState *s); > > -int kvm_irqchip_set_irq(KVMState *s, int irq, int level); > +int kvm_inject_async_irq(KVMState *s, int irq, int level); > int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg); > "interrupt injection" refers to the act of setting up an interrupt to be delivered to the guest at the next entry, so it is synchronous by nature. It was sloppy of me to use the term, but let's not introduce it to the code as well. The correct terminology would be: interrupt injection: synchronous, in-vcpu, after all masks have been evaluated. Straight into the vein. interrupt queueing: asynchronous, extra-vcpu, before any masks Since interrupt queueing (well that name isn't perfect for level triggered interrupts, since it may not queue anything...) is the norm, I think it's better to keep the set_irq() naming and mark the synchronous interrupt injection function as special.
On 2012-07-25 17:47, Avi Kivity wrote: > On 07/25/2012 04:24 PM, Peter Maydell wrote: >> Rename the function kvm_irqchip_set_irq() to kvm_inject_async_irq(), >> since it can be used for asynchronous interrupt injection whether >> there is a full irqchip model in the kernel or not. >> > >> @@ -144,7 +144,7 @@ int kvm_arch_on_sigbus(int code, void *addr); >> >> void kvm_arch_init_irq_routing(KVMState *s); >> >> -int kvm_irqchip_set_irq(KVMState *s, int irq, int level); >> +int kvm_inject_async_irq(KVMState *s, int irq, int level); >> int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg); >> > > "interrupt injection" refers to the act of setting up an interrupt to be > delivered to the guest at the next entry, so it is synchronous by > nature. It was sloppy of me to use the term, but let's not introduce it > to the code as well. > > The correct terminology would be: > interrupt injection: synchronous, in-vcpu, after all masks have been > evaluated. Straight into the vein. > interrupt queueing: asynchronous, extra-vcpu, before any masks > > Since interrupt queueing (well that name isn't perfect for level > triggered interrupts, since it may not queue anything...) is the norm, I > think it's better to keep the set_irq() naming and mark the synchronous > interrupt injection function as special. We don't have a synchronous function anymore, it's part of the pre-run code of x86 IIRC. Jan
On 07/25/2012 06:53 PM, Jan Kiszka wrote: > On 2012-07-25 17:47, Avi Kivity wrote: >> On 07/25/2012 04:24 PM, Peter Maydell wrote: >>> Rename the function kvm_irqchip_set_irq() to kvm_inject_async_irq(), >>> since it can be used for asynchronous interrupt injection whether >>> there is a full irqchip model in the kernel or not. >>> >> >>> @@ -144,7 +144,7 @@ int kvm_arch_on_sigbus(int code, void *addr); >>> >>> void kvm_arch_init_irq_routing(KVMState *s); >>> >>> -int kvm_irqchip_set_irq(KVMState *s, int irq, int level); >>> +int kvm_inject_async_irq(KVMState *s, int irq, int level); >>> int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg); >>> >> >> "interrupt injection" refers to the act of setting up an interrupt to be >> delivered to the guest at the next entry, so it is synchronous by >> nature. It was sloppy of me to use the term, but let's not introduce it >> to the code as well. >> >> The correct terminology would be: >> interrupt injection: synchronous, in-vcpu, after all masks have been >> evaluated. Straight into the vein. >> interrupt queueing: asynchronous, extra-vcpu, before any masks >> >> Since interrupt queueing (well that name isn't perfect for level >> triggered interrupts, since it may not queue anything...) is the norm, I >> think it's better to keep the set_irq() naming and mark the synchronous >> interrupt injection function as special. > > We don't have a synchronous function anymore, it's part of the pre-run > code of x86 IIRC. Right. There's a DPRINTF() there that talks about injection, too. So I think this patch can be dropped.
On 25 July 2012 16:47, Avi Kivity <avi@redhat.com> wrote: > On 07/25/2012 04:24 PM, Peter Maydell wrote: >> -int kvm_irqchip_set_irq(KVMState *s, int irq, int level); >> +int kvm_inject_async_irq(KVMState *s, int irq, int level); > "interrupt injection" refers to the act of setting up an interrupt to be > delivered to the guest at the next entry, so it is synchronous by > nature. It was sloppy of me to use the term, but let's not introduce it > to the code as well. Ah. I'd assumed it just meant "inject this into the kernel". (ie the point at which the interrupt is no longer qemu's problem :-)) > The correct terminology would be: > interrupt injection: synchronous, in-vcpu, after all masks have been > evaluated. Straight into the vein. > interrupt queueing: asynchronous, extra-vcpu, before any masks > > Since interrupt queueing (well that name isn't perfect for level > triggered interrupts, since it may not queue anything...) is the norm, I > think it's better to keep the set_irq() naming and mark the synchronous > interrupt injection function as special. So just call this 'kvm_set_irq()' ? -- PMM
On 25 July 2012 16:55, Avi Kivity <avi@redhat.com> wrote: > On 07/25/2012 06:53 PM, Jan Kiszka wrote: >> We don't have a synchronous function anymore, it's part of the pre-run >> code of x86 IIRC. > > Right. There's a DPRINTF() there that talks about injection, too. So I > think this patch can be dropped. The main purpose of the patch is to remove 'irqchip' from the function name, because the function isn't restricted to use with in-kernel irqchips. -- PMM
On 2012-07-25 17:56, Peter Maydell wrote: > On 25 July 2012 16:55, Avi Kivity <avi@redhat.com> wrote: >> On 07/25/2012 06:53 PM, Jan Kiszka wrote: >>> We don't have a synchronous function anymore, it's part of the pre-run >>> code of x86 IIRC. >> >> Right. There's a DPRINTF() there that talks about injection, too. So I >> think this patch can be dropped. > > The main purpose of the patch is to remove 'irqchip' from the > function name, because the function isn't restricted to use > with in-kernel irqchips. Hmm, what was question again? Ah: Do we have an arch that implements it without providing a (logical) irqchip? At least at this time (including ARM)? Jan
On 07/25/2012 06:56 PM, Peter Maydell wrote: > On 25 July 2012 16:55, Avi Kivity <avi@redhat.com> wrote: >> On 07/25/2012 06:53 PM, Jan Kiszka wrote: >>> We don't have a synchronous function anymore, it's part of the pre-run >>> code of x86 IIRC. >> >> Right. There's a DPRINTF() there that talks about injection, too. So I >> think this patch can be dropped. > > The main purpose of the patch is to remove 'irqchip' from the > function name, because the function isn't restricted to use > with in-kernel irqchips. kvm_set_irq(), then.
On 07/25/2012 06:55 PM, Peter Maydell wrote: >> The correct terminology would be: >> interrupt injection: synchronous, in-vcpu, after all masks have been >> evaluated. Straight into the vein. >> interrupt queueing: asynchronous, extra-vcpu, before any masks >> >> Since interrupt queueing (well that name isn't perfect for level >> triggered interrupts, since it may not queue anything...) is the norm, I >> think it's better to keep the set_irq() naming and mark the synchronous >> interrupt injection function as special. > > So just call this 'kvm_set_irq()' ? Yes.
On 25 July 2012 16:58, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2012-07-25 17:56, Peter Maydell wrote: >> On 25 July 2012 16:55, Avi Kivity <avi@redhat.com> wrote: >>> On 07/25/2012 06:53 PM, Jan Kiszka wrote: >>>> We don't have a synchronous function anymore, it's part of the pre-run >>>> code of x86 IIRC. >>> >>> Right. There's a DPRINTF() there that talks about injection, too. So I >>> think this patch can be dropped. >> >> The main purpose of the patch is to remove 'irqchip' from the >> function name, because the function isn't restricted to use >> with in-kernel irqchips. > > Hmm, what was question again? Ah: Do we have an arch that implements it > without providing a (logical) irqchip? At least at this time (including > ARM)? Well, it depends what you mean by 'irqchip' (part of the point of this series being that there isn't a coherent architecture independent definition of that and so we shouldn't use the term in architecture-independent code). On ARM we will use KVM_IRQ_LINE whether we have an in-kernel VGIC or not, because we always use async interrupt injection. (That is, the same arguments for "why should this function be guarded by kvm_async_interrupt_injection() rather than kvm_irqchip_in_kernel()?" apply to "why should this function not have 'irqchip' in the function name.) -- PMM
On 2012-07-25 18:09, Peter Maydell wrote: > On 25 July 2012 16:58, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> On 2012-07-25 17:56, Peter Maydell wrote: >>> On 25 July 2012 16:55, Avi Kivity <avi@redhat.com> wrote: >>>> On 07/25/2012 06:53 PM, Jan Kiszka wrote: >>>>> We don't have a synchronous function anymore, it's part of the pre-run >>>>> code of x86 IIRC. >>>> >>>> Right. There's a DPRINTF() there that talks about injection, too. So I >>>> think this patch can be dropped. >>> >>> The main purpose of the patch is to remove 'irqchip' from the >>> function name, because the function isn't restricted to use >>> with in-kernel irqchips. >> >> Hmm, what was question again? Ah: Do we have an arch that implements it >> without providing a (logical) irqchip? At least at this time (including >> ARM)? > > Well, it depends what you mean by 'irqchip' (part of the point of > this series being that there isn't a coherent architecture > independent definition of that and so we shouldn't use the term > in architecture-independent code). > On ARM we will use KVM_IRQ_LINE whether we have an in-kernel VGIC > or not, because we always use async interrupt injection. > > (That is, the same arguments for "why should this function be > guarded by kvm_async_interrupt_injection() rather than > kvm_irqchip_in_kernel()?" apply to "why should this function not > have 'irqchip' in the function name.) Wasn't Avi's point that you do have an irqchip in your KVM support? Jan
On 25 July 2012 17:11, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2012-07-25 18:09, Peter Maydell wrote: >> On 25 July 2012 16:58, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>> Hmm, what was question again? Ah: Do we have an arch that implements it >>> without providing a (logical) irqchip? At least at this time (including >>> ARM)? >> >> Well, it depends what you mean by 'irqchip' (part of the point of >> this series being that there isn't a coherent architecture >> independent definition of that and so we shouldn't use the term >> in architecture-independent code). >> On ARM we will use KVM_IRQ_LINE whether we have an in-kernel VGIC >> or not, because we always use async interrupt injection. >> >> (That is, the same arguments for "why should this function be >> guarded by kvm_async_interrupt_injection() rather than >> kvm_irqchip_in_kernel()?" apply to "why should this function not >> have 'irqchip' in the function name.) > > Wasn't Avi's point that you do have an irqchip in your KVM support? Not in the sense of "you need to KVM_CREATE_IRQCHIP it", or in the sense of "this might be a useful thing to expose to the user as a togglable option", which are the main interesting architecture-independent bits of kernel_irqchip=on. IIRC Alex said that PPC has a similar setup -- interrupts are always sent asynchronously whether you've created the kernel irqchip or not. In any case I think it's much clearer to actually name things based on what they're really testing for rather than things that might or might not be correlated with that, even in the (hypothetical) case that all architectures had an irqchip if and only if they did async interrupt delivery. -- PMM
On 25 July 2012 17:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> Not in the sense of "you need to KVM_CREATE_IRQCHIP it",
...incidentally I was thinking about maybe moving kvm_irqchip_create()
from being called by kvm_init() to being called by the device
init function for the relevant irqchip (particularly we'll need
to do that if we adopt Avi's suggestion of having a parameter
to KVM_CREATE_IRQCHIP to specify a particular kind of irqchip).
But that's more invasive surgery so I didn't want to do it yet.
-- PMM
On 2012-07-25 18:24, Peter Maydell wrote: > On 25 July 2012 17:18, Peter Maydell <peter.maydell@linaro.org> wrote: >> Not in the sense of "you need to KVM_CREATE_IRQCHIP it", > > ...incidentally I was thinking about maybe moving kvm_irqchip_create() > from being called by kvm_init() to being called by the device > init function for the relevant irqchip (particularly we'll need > to do that if we adopt Avi's suggestion of having a parameter > to KVM_CREATE_IRQCHIP to specify a particular kind of irqchip). > But that's more invasive surgery so I didn't want to do it yet. This won't fly as irchip affects the whole orchestra (vcpus & irqchip stubs in user space), at least on x86, and has to be called in the current order. That's also why kernel_irqchip is a machine options, not an option of one of the many device models. Jan
On 07/25/2012 07:28 PM, Jan Kiszka wrote: > On 2012-07-25 18:24, Peter Maydell wrote: >> On 25 July 2012 17:18, Peter Maydell <peter.maydell@linaro.org> wrote: >>> Not in the sense of "you need to KVM_CREATE_IRQCHIP it", >> >> ...incidentally I was thinking about maybe moving kvm_irqchip_create() >> from being called by kvm_init() to being called by the device >> init function for the relevant irqchip (particularly we'll need >> to do that if we adopt Avi's suggestion of having a parameter >> to KVM_CREATE_IRQCHIP to specify a particular kind of irqchip). >> But that's more invasive surgery so I didn't want to do it yet. > > This won't fly as irchip affects the whole orchestra (vcpus & irqchip > stubs in user space), at least on x86, and has to be called in the > current order. That's also why kernel_irqchip is a machine options, not > an option of one of the many device models. Yes, to elaborate, KVM_CREATE_IRQCHIP creates N+3 devices: N local APICs (deferred until N vcpus are created), one IOAPIC, and two PICs. We debated decoupling those devices, but since there are a lot of intercommunication among those devices, it was deemed to difficult (plus these were the early kvm days when we had different get it in/get it right tradeoffs).
On 25 July 2012 17:28, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2012-07-25 18:24, Peter Maydell wrote: >> ...incidentally I was thinking about maybe moving kvm_irqchip_create() >> from being called by kvm_init() to being called by the device >> init function for the relevant irqchip (particularly we'll need >> to do that if we adopt Avi's suggestion of having a parameter >> to KVM_CREATE_IRQCHIP to specify a particular kind of irqchip). >> But that's more invasive surgery so I didn't want to do it yet. > > This won't fly as irchip affects the whole orchestra (vcpus & irqchip > stubs in user space), at least on x86, and has to be called in the > current order. That's also why kernel_irqchip is a machine options, not > an option of one of the many device models. Yes, one of the things you'd need to do is move actual creation of the vcpus (as opposed to the QEMU CPU QOM objects) to rather later in the sequence than they are now. Where you have multiple devices which all need to go the same way you can put that in the machine model code and then have all the devices take an option the machine model sets to say which way they go. (Oddities in the x86 specific bits of the KVM kernel code ought to result in oddities in x86 specific bits of QEMU, not in generic bits :-)) -- PMM
On 2012-07-25 18:41, Peter Maydell wrote: > On 25 July 2012 17:28, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> On 2012-07-25 18:24, Peter Maydell wrote: >>> ...incidentally I was thinking about maybe moving kvm_irqchip_create() >>> from being called by kvm_init() to being called by the device >>> init function for the relevant irqchip (particularly we'll need >>> to do that if we adopt Avi's suggestion of having a parameter >>> to KVM_CREATE_IRQCHIP to specify a particular kind of irqchip). >>> But that's more invasive surgery so I didn't want to do it yet. >> >> This won't fly as irchip affects the whole orchestra (vcpus & irqchip >> stubs in user space), at least on x86, and has to be called in the >> current order. That's also why kernel_irqchip is a machine options, not >> an option of one of the many device models. > > Yes, one of the things you'd need to do is move actual creation > of the vcpus (as opposed to the QEMU CPU QOM objects) to rather > later in the sequence than they are now. KVM VCPU creation is bound to the QOM object creation phase if we want hotplugging support. > > Where you have multiple devices which all need to go the same way > you can put that in the machine model code and then have all the > devices take an option the machine model sets to say which way > they go. > > (Oddities in the x86 specific bits of the KVM kernel code ought > to result in oddities in x86 specific bits of QEMU, not in > generic bits :-)) I dreamed of this as well before porting in-kernel irqchip support upstream. ;) But the point of this service is not that arch-specific in fact: By the time you have a set of kernel irqchips that cannot reasonably be instantiated / enabled separately, a single service helps to avoid that userspace tries to do this mistake at all. Not all archs may have this requirement, but a generic service needs to address it if it wants to be generic. Jan
diff --git a/hw/kvm/i8259.c b/hw/kvm/i8259.c index 94d1b9a..ab970db 100644 --- a/hw/kvm/i8259.c +++ b/hw/kvm/i8259.c @@ -94,7 +94,7 @@ static void kvm_pic_set_irq(void *opaque, int irq, int level) { int delivered; - delivered = kvm_irqchip_set_irq(kvm_state, irq, level); + delivered = kvm_inject_async_irq(kvm_state, irq, level); apic_report_irq_delivered(delivered); } diff --git a/hw/kvm/ioapic.c b/hw/kvm/ioapic.c index 3ae3175..d7add35 100644 --- a/hw/kvm/ioapic.c +++ b/hw/kvm/ioapic.c @@ -82,7 +82,7 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level) KVMIOAPICState *s = opaque; int delivered; - delivered = kvm_irqchip_set_irq(kvm_state, s->kvm_gsi_base + irq, level); + delivered = kvm_inject_async_irq(kvm_state, s->kvm_gsi_base + irq, level); apic_report_irq_delivered(delivered); } diff --git a/kvm-all.c b/kvm-all.c index 3354c6f..9f14274 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -853,7 +853,7 @@ static void kvm_handle_interrupt(CPUArchState *env, int mask) } } -int kvm_irqchip_set_irq(KVMState *s, int irq, int level) +int kvm_inject_async_irq(KVMState *s, int irq, int level) { struct kvm_irq_level event; int ret; @@ -864,7 +864,7 @@ int kvm_irqchip_set_irq(KVMState *s, int irq, int level) event.irq = irq; ret = kvm_vm_ioctl(s, s->irqchip_inject_ioctl, &event); if (ret < 0) { - perror("kvm_set_irqchip_line"); + perror("kvm_inject_async_irq"); abort(); } @@ -1089,7 +1089,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg) assert(route->kroute.type == KVM_IRQ_ROUTING_MSI); - return kvm_irqchip_set_irq(s, route->kroute.gsi, 1); + return kvm_inject_async_irq(s, route->kroute.gsi, 1); } int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg) diff --git a/kvm.h b/kvm.h index 00abe36..cfdc95e 100644 --- a/kvm.h +++ b/kvm.h @@ -144,7 +144,7 @@ int kvm_arch_on_sigbus(int code, void *addr); void kvm_arch_init_irq_routing(KVMState *s); -int kvm_irqchip_set_irq(KVMState *s, int irq, int level); +int kvm_inject_async_irq(KVMState *s, int irq, int level); int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg); void kvm_irqchip_add_irq_route(KVMState *s, int gsi, int irqchip, int pin);
Rename the function kvm_irqchip_set_irq() to kvm_inject_async_irq(), since it can be used for asynchronous interrupt injection whether there is a full irqchip model in the kernel or not. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/kvm/i8259.c | 2 +- hw/kvm/ioapic.c | 2 +- kvm-all.c | 6 +++--- kvm.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-)