[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 4/6] provide in-kernel i8259 chip
From: |
Juan Quintela |
Subject: |
[Qemu-devel] Re: [PATCH 4/6] provide in-kernel i8259 chip |
Date: |
Tue, 29 Sep 2009 00:39:43 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Glauber Costa <address@hidden> wrote:
> On Tue, Sep 29, 2009 at 12:04:54AM +0200, Juan Quintela wrote:
>> Glauber Costa <address@hidden> wrote:
>> > This patch provides kvm with an in-kernel i8259 chip. We are currently not
>> > enabling it.
>> > The code is heavily based on what's in qemu-kvm.git.
>> >
>> > Signed-off-by: Glauber Costa <address@hidden>
>> > ---
>> > hw/i8259.c | 103
>> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> > hw/pc.h | 1 +
>> > kvm-all.c | 24 ++++++++++++++
>> > kvm.h | 2 +
>> > 4 files changed, 130 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/hw/i8259.c b/hw/i8259.c
>> > index 3de22e3..31524f5 100644
>> > --- a/hw/i8259.c
>> > +++ b/hw/i8259.c
>> > @@ -26,6 +26,7 @@
>> > #include "isa.h"
>> > #include "monitor.h"
>> > #include "qemu-timer.h"
>> > +#include "kvm.h"
>> >
>> > /* debug PIC */
>> > //#define DEBUG_PIC
>> > @@ -446,9 +447,77 @@ static uint32_t elcr_ioport_read(void *opaque,
>> > uint32_t addr1)
>> > return s->elcr;
>> > }
>> >
>> > +static int kvm_kernel_pic_load_from_user(void *opaque)
>> > +{
>> > +#if defined(TARGET_I386)
>> > + PicState *s = (void *)opaque;
>> > + struct kvm_irqchip chip;
>> > + struct kvm_pic_state *kpic;
>>
>> It miss:
>> if (!kvm_enabled() && !kvm_irqchip_enabled()) {
>> return 0;
>> }
>>
>> Or similar logic, otherwise kvm_set_irqchip() is called when kvm_irqchip
>> is not enabled. Same for save_to_user.
>>
>> > + chip.chip_id = (&s->pics_state->pics[0] == s) ?
>> > + KVM_IRQCHIP_PIC_MASTER :
>> > + KVM_IRQCHIP_PIC_SLAVE;
>> > + kpic = &chip.chip.pic;
>> > +
>> > + kpic->last_irr = s->last_irr;
>> > + kpic->irr = s->irr;
>> > + kpic->imr = s->imr;
>> > + kpic->isr = s->isr;
>> > + kpic->priority_add = s->priority_add;
>> > + kpic->irq_base = s->irq_base;
>> > + kpic->read_reg_select = s->read_reg_select;
>> > + kpic->poll = s->poll;
>> > + kpic->special_mask = s->special_mask;
>> > + kpic->init_state = s->init_state;
>> > + kpic->auto_eoi = s->auto_eoi;
>> > + kpic->rotate_on_auto_eoi = s->rotate_on_auto_eoi;
>> > + kpic->special_fully_nested_mode = s->special_fully_nested_mode;
>> > + kpic->init4 = s->init4;
>> > + kpic->elcr = s->elcr;
>> > + kpic->elcr_mask = s->elcr_mask;
>> > +
>> > + kvm_set_irqchip(&chip);
>> > +#endif
>> > + return 0;
>> > +}
>> ....
>> > static const VMStateDescription vmstate_pic = {
>> > .name = "i8259",
>> > .version_id = 1,
>> > + .pre_save = kvm_kernel_pic_save_to_user,
>> > + .post_load = kvm_kernel_pic_load_from_user,
>>
>> Let the three version_id fields together, please.
>>
>>
>> > +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
>> > +static void kvm_i8259_set_irq(void *opaque, int irq, int level)
>> > +{
>> > + int pic_ret;
>> > + if (kvm_set_irq(irq, level, &pic_ret)) {
>> > + if (pic_ret != 0)
>> > + apic_set_irq_delivered();
>> > + return;
>> > + }
>> > +}
>> > +
>> > +static void kvm_pic_init1(int io_addr, PicState *s)
>> > +{
>> > + vmstate_register(io_addr, &vmstate_pic, s);
>> > + qemu_register_reset(pic_reset, s);
>> > +}
>> > +
>> > +qemu_irq *kvm_i8259_init(qemu_irq parent_irq)
>> > +{
>> > + PicState2 *s;
>> > +
>> > + s = qemu_mallocz(sizeof(PicState2));
>> > +
>> > + kvm_pic_init1(0x20, &s->pics[0]);
>> > + kvm_pic_init1(0xa0, &s->pics[1]);
>> > + s->parent_irq = parent_irq;
>> > + s->pics[0].pics_state = s;
>> > + s->pics[1].pics_state = s;
>> > + isa_pic = s;
>> > + return qemu_allocate_irqs(kvm_i8259_set_irq, s, 24);
>> > +}
>> > +#endif
>>
>> I think everything would be nicer if this three functions where merged
>> with the _non_ kvm ones with a kvm_enable() test. They only differ in
>> 2-3 lines.
> I disagree. I think it is a better solution long term to provide irqchips
> that are completely free of kvm code.
Solutions:
- you copy the file and lives synchronizing the changes
- you export the needed funtions and then implement in the other file
the kvm bits.
- you merge the kvm and non kvm bits.
I see here a very bad mix :( The error showed before is due to the bad mix.
I showed 1 error, 1 question of style and 1 suggestion, you only
answered to the suggestion.
Later, Juan.
Message not available
Re: [Qemu-devel] [PATCH 0/6] in kernel irqchip support, Jamie Lokier, 2009/09/28
Re: [Qemu-devel] [PATCH 0/6] in kernel irqchip support, Avi Kivity, 2009/09/29
Re: [Qemu-devel] [PATCH 0/6] in kernel irqchip support, Anthony Liguori, 2009/09/30