[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: |
Jan Kiszka |
Subject: |
[Qemu-devel] Re: [PATCH 4/6] provide in-kernel i8259 chip |
Date: |
Fri, 02 Oct 2009 22:33:47 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
Juan Quintela wrote:
> 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.
>
I agree with Juan here. And I would recommend to go the second path:
factor out shared code, e.g. into apic_common.c, and implement the
different variants in different files, maybe even as separate qdev
devices. The current situation is fairly unfortunate IMO.
Jan
signature.asc
Description: OpenPGP digital signature
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- [Qemu-devel] Re: [PATCH 4/6] provide in-kernel i8259 chip,
Jan Kiszka <=