Re: [PATCH v2 3/9] provide in-kernel ioapic

Re: [PATCH v2 3/9] provide in-kernel ioapic

[Anthony Liguori]

Glauber Costa wrote:
> This patch provides kvm with an in-kernel ioapic. We are currently not enabling it.
> The code is heavily based on what's in qemu-kvm.git.
>   

It really ought to be it's own file and own device model.  Having the 
code mixed in with ioapic.c is confusing because it's unclear what code 
is in use when the in-kernel model is used.

> @@ -193,6 +194,79 @@ static void ioapic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t va
>      }
>  }
>
> +static int kvm_kernel_ioapic_load_from_user(IOAPICState *s)
> +{
> +    int r = 0;
> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
>   

No point in checking the KVM_CAP_IRQCHIP.  Just require it during 
build.  Otherwise, !KVM_CAP_IRQCHIP is dead code since I'm sure noone is 
actually testing kernels that old with modern qemu.

There's no point in restricting to I386 either.

> +    struct kvm_irqchip chip;
> +    struct kvm_ioapic_state *kioapic;
> +    int i;
> +
> +    if (!(kvm_enabled() && kvm_irqchip_in_kernel()))
> +        return 0;
> +
> +    chip.chip_id = KVM_IRQCHIP_IOAPIC;
> +    kioapic = &chip.chip.ioapic;
> +
> +    kioapic->id = s->id;
> +    kioapic->ioregsel = s->ioregsel;
> +    kioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
> +    kioapic->irr = s->irr;
> +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> +        kioapic->redirtbl[i].bits = s->ioredtbl[i];
> +    }
> +
> +    r = kvm_set_irqchip(&chip);
> +#endif
> +    return r;
> +}
> +
> +static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
> +{
> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> +    struct kvm_irqchip chip;
> +    struct kvm_ioapic_state *kioapic;
> +    int i;
> +
> +    if (!(kvm_enabled() && kvm_irqchip_in_kernel()))
> +        return;
> +    chip.chip_id = KVM_IRQCHIP_IOAPIC;
> +    kvm_get_irqchip(&chip);
> +    kioapic = &chip.chip.ioapic;
> +
> +    s->id = kioapic->id;
> +    s->ioregsel = kioapic->ioregsel;
> +    s->irr = kioapic->irr;
> +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> +        s->ioredtbl[i] = kioapic->redirtbl[i].bits;
> +    }
> +#endif
> +}
> +
> +static void ioapic_pre_save(void *opaque)
> +{
> +    IOAPICState *s = (void *)opaque;
> +
> +    kvm_kernel_ioapic_save_to_user(s);
> +}
> +
> +static int ioapic_pre_load(void *opaque)
> +{
> +    IOAPICState *s = opaque;
> +
> +    /* in case we are doing version 1, we just set these to sane values */
> +    s->irr = 0;
> +    return 0;
> +}
> +
> +static int ioapic_post_load(void *opaque, int version_id)
> +{
> +    IOAPICState *s = opaque;
> +
> +    return kvm_kernel_ioapic_load_from_user(s);
> +}
> +
> +
>  static const VMStateDescription vmstate_ioapic = {
>      .name = "ioapic",
>      .version_id = 2,
> @@ -205,7 +279,10 @@ static const VMStateDescription vmstate_ioapic = {
>          VMSTATE_UINT32_V(irr, IOAPICState, 2),
>          VMSTATE_UINT64_ARRAY(ioredtbl, IOAPICState, IOAPIC_NUM_PINS),
>          VMSTATE_END_OF_LIST()
> -    }
> +    },
> +    .pre_load = ioapic_pre_load,
> +    .post_load = ioapic_post_load,
> +    .pre_save = ioapic_pre_save,
>  };
>   

The in kernel apic should be a separate device model with a separate 
savevm section.  They are different devices and there's no real 
advantage to pretending like they're the same device.

>  static CPUReadMemoryFunc * const ioapic_mem_read[3] = {
> diff --git a/kvm-all.c b/kvm-all.c
> index 48ae26c..d795285 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -411,6 +411,26 @@ int kvm_check_extension(KVMState *s, unsigned int extension)
>      return ret;
>  }
>
> +#ifdef KVM_CAP_IRQCHIP
> +int kvm_set_irqchip(struct kvm_irqchip *chip)
> +{
> +    if (!kvm_state->irqchip_in_kernel) {
> +        return 0;
> +    }
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_SET_IRQCHIP, chip);
> +}
>   

irqchip_in_kernel ought to disappear.

-- 
Regards,

Anthony Liguori

Re: [PATCH v2 3/9] provide in-kernel ioapic

[Avi Kivity]

On 10/08/2009 03:49 PM, Anthony Liguori wrote:
> Glauber Costa wrote:
>> This patch provides kvm with an in-kernel ioapic. We are currently 
>> not enabling it.
>> The code is heavily based on what's in qemu-kvm.git.
>
> It really ought to be it's own file and own device model.  Having the 
> code mixed in with ioapic.c is confusing because it's unclear what 
> code is in use when the in-kernel model is used.

I disagree.  It's the same device with the same guest-visible interface 
and the same host-visible interface (save/restore, 'info ioapic' if we 
write one).  Splitting it into two files will only result in code 
duplication.

Think of it as an ioapic accelerator.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2 3/9] provide in-kernel ioapic

[Jan Kiszka]

Avi Kivity wrote:
> On 10/08/2009 03:49 PM, Anthony Liguori wrote:
>> Glauber Costa wrote:
>>> This patch provides kvm with an in-kernel ioapic. We are currently
>>> not enabling it.
>>> The code is heavily based on what's in qemu-kvm.git.
>>
>> It really ought to be it's own file and own device model.  Having the
>> code mixed in with ioapic.c is confusing because it's unclear what
>> code is in use when the in-kernel model is used.
> 
> I disagree.  It's the same device with the same guest-visible interface
> and the same host-visible interface (save/restore, 'info ioapic' if we
> write one).  Splitting it into two files will only result in code
> duplication.

Shared functions can be pushed into a commonly used module
(ioapic-common.c). And everyone touching it should realize then that it
could affect more than one user.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Jamie Lokier]

Avi Kivity wrote:
> On 10/08/2009 03:49 PM, Anthony Liguori wrote:
> >Glauber Costa wrote:
> >>This patch provides kvm with an in-kernel ioapic. We are currently 
> >>not enabling it.
> >>The code is heavily based on what's in qemu-kvm.git.
> >
> >It really ought to be it's own file and own device model.  Having the 
> >code mixed in with ioapic.c is confusing because it's unclear what 
> >code is in use when the in-kernel model is used.
> 
> I disagree.  It's the same device with the same guest-visible interface 
> and the same host-visible interface (save/restore, 'info ioapic' if we 
> write one).  Splitting it into two files will only result in code 
> duplication.
> 
> Think of it as an ioapic accelerator.

Haven't we already confirmed that it *isn't* just an ioapic accelerator
because you can't migrate between in-kernel iopic and qemu's ioapic?

Imho, if they cannot be swapped transparently, they are different
device emulations.

OF course there's nothing wrong with sharing lots of code.

Maybe ioapic.c and ioapic-kvm.c, with shared code in ioapic-common.c?

-- Jamie

Re: [Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Anthony Liguori]

Jamie Lokier wrote:
> Avi Kivity wrote:
>   
>> On 10/08/2009 03:49 PM, Anthony Liguori wrote:
>>     
>>> Glauber Costa wrote:
>>>       
>>>> This patch provides kvm with an in-kernel ioapic. We are currently 
>>>> not enabling it.
>>>> The code is heavily based on what's in qemu-kvm.git.
>>>>         
>>> It really ought to be it's own file and own device model.  Having the 
>>> code mixed in with ioapic.c is confusing because it's unclear what 
>>> code is in use when the in-kernel model is used.
>>>       
>> I disagree.  It's the same device with the same guest-visible interface 
>> and the same host-visible interface (save/restore, 'info ioapic' if we 
>> write one).  Splitting it into two files will only result in code 
>> duplication.
>>
>> Think of it as an ioapic accelerator.
>>     
>
> Haven't we already confirmed that it *isn't* just an ioapic accelerator
> because you can't migrate between in-kernel iopic and qemu's ioapic?
>
> Imho, if they cannot be swapped transparently, they are different
> device emulations.
>
> OF course there's nothing wrong with sharing lots of code.
>   

If you avoid having a common save format, you get an overall reduction 
in code size and there's virtually no code to share.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Avi Kivity]

On 10/08/2009 06:07 PM, Jamie Lokier wrote:
> Haven't we already confirmed that it *isn't* just an ioapic accelerator
> because you can't migrate between in-kernel iopic and qemu's ioapic?
>    

We haven't confirmed it.  Both implement the same spec, and if you can't 
migrate between them, one of them has a bug (for example, qemu ioapic 
doesn't implement polarity - but it's still just a bug).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Gleb Natapov]

On Thu, Oct 08, 2009 at 06:17:57PM +0200, Avi Kivity wrote:
> On 10/08/2009 06:07 PM, Jamie Lokier wrote:
> >Haven't we already confirmed that it *isn't* just an ioapic accelerator
> >because you can't migrate between in-kernel iopic and qemu's ioapic?
> 
> We haven't confirmed it.  Both implement the same spec, and if you
> can't migrate between them, one of them has a bug (for example, qemu
> ioapic doesn't implement polarity - but it's still just a bug).
> 
Are you saying that HW spec (that only describes software visible behavior)
completely defines implementation? No other internal state is needed
that may be done differently by different implementations?

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Avi Kivity]

On 10/08/2009 06:22 PM, Gleb Natapov wrote:
> On Thu, Oct 08, 2009 at 06:17:57PM +0200, Avi Kivity wrote:
>    
>> On 10/08/2009 06:07 PM, Jamie Lokier wrote:
>>      
>>> Haven't we already confirmed that it *isn't* just an ioapic accelerator
>>> because you can't migrate between in-kernel iopic and qemu's ioapic?
>>>        
>> We haven't confirmed it.  Both implement the same spec, and if you
>> can't migrate between them, one of them has a bug (for example, qemu
>> ioapic doesn't implement polarity - but it's still just a bug).
>>
>>      
> Are you saying that HW spec (that only describes software visible behavior)
> completely defines implementation? No other internal state is needed
> that may be done differently by different implementations?
>    

It may be done differently (for example, selecting the cpu to deliver 
the interrupt to), but as the guest cannot rely on the differences, 
there's no need to save the state that can cause these differences.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Gleb Natapov]

On Thu, Oct 08, 2009 at 06:29:53PM +0200, Avi Kivity wrote:
> On 10/08/2009 06:22 PM, Gleb Natapov wrote:
> >On Thu, Oct 08, 2009 at 06:17:57PM +0200, Avi Kivity wrote:
> >>On 10/08/2009 06:07 PM, Jamie Lokier wrote:
> >>>Haven't we already confirmed that it *isn't* just an ioapic accelerator
> >>>because you can't migrate between in-kernel iopic and qemu's ioapic?
> >>We haven't confirmed it.  Both implement the same spec, and if you
> >>can't migrate between them, one of them has a bug (for example, qemu
> >>ioapic doesn't implement polarity - but it's still just a bug).
> >>
> >Are you saying that HW spec (that only describes software visible behavior)
> >completely defines implementation? No other internal state is needed
> >that may be done differently by different implementations?
> 
> It may be done differently (for example, selecting the cpu to
> deliver the interrupt to), but as the guest cannot rely on the
> differences, there's no need to save the state that can cause these
> differences.
> 
So suppose I have simple watchdog device that required to be poked every
second, otherwise it resets a computer. On migration we have to migrate
time elapsed since last poke, but if device doesn't expose it to
software in any way you are saying we can recreate is some other way?

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Avi Kivity]

On 10/08/2009 06:34 PM, Gleb Natapov wrote:
> So suppose I have simple watchdog device that required to be poked every
> second, otherwise it resets a computer. On migration we have to migrate
> time elapsed since last poke, but if device doesn't expose it to
> software in any way you are saying we can recreate is some other way?
>    

The time is exposed (you can measure it by poking the device and 
measuring the time till reset) and will be described in the spec 
(otherwise users will be surprised when their machine resets).

You don't have to migrate all exposed state, just exposed state that the 
guest may rely on.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Gleb Natapov]

On Thu, Oct 08, 2009 at 06:42:07PM +0200, Avi Kivity wrote:
> On 10/08/2009 06:34 PM, Gleb Natapov wrote:
> >So suppose I have simple watchdog device that required to be poked every
> >second, otherwise it resets a computer. On migration we have to migrate
> >time elapsed since last poke, but if device doesn't expose it to
> >software in any way you are saying we can recreate is some other way?
> 
> The time is exposed (you can measure it by poking the device and
The time yes, not its internal representation. What if one implementation 
stores how much time passed and another how much time left. One counts
in wall clack another only when guest runs. etc... and this is a device
with only one write only register.

> measuring the time till reset) and will be described in the spec
> (otherwise users will be surprised when their machine resets).
> 
> You don't have to migrate all exposed state, just exposed state that
> the guest may rely on.

--
			Gleb.

Re: Re: [PATCH v2 3/9] provide in-kernel ioapic

[Jamie Lokier]

Gleb Natapov wrote:
> On Thu, Oct 08, 2009 at 06:42:07PM +0200, Avi Kivity wrote:
> > On 10/08/2009 06:34 PM, Gleb Natapov wrote:
> > >So suppose I have simple watchdog device that required to be poked every
> > >second, otherwise it resets a computer. On migration we have to migrate
> > >time elapsed since last poke, but if device doesn't expose it to
> > >software in any way you are saying we can recreate is some other way?
> > 
> > The time is exposed (you can measure it by poking the device and
> The time yes, not its internal representation. What if one implementation 
> stores how much time passed and another how much time left.
> One counts in wall clack another only when guest runs. etc... and
> this is a device with only one write only register.

In that case you can decide between calling it two different devices
(which have the same guest-visible behaviour but are not
interchangable), or calling them different implementations of one
device - by adding a little more code to save state in a common format.

(Although they may have to be different devices for qemu
configuration, it's ok for them to have the same PCI IDs and for the
guest not to know the difference)

For your watchdog example, it's not hard to pick a saved state which
works for both.

ioapic will be harder to find a useful common saved state, and there
might need to be an *optional hint* section (e.g. for selecting the
next CPU to get an interrupt), but I think it would be worth it in
this case.  Being able to load a KVM image into TCG and vice versa is
quite useful sometimes.  E.g. I've had to do some OS installs using
TCG at first, then switch to KVM later for performance.

-- Jamie

Re: [Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Gleb Natapov]

On Fri, Oct 09, 2009 at 11:02:31AM +0100, Jamie Lokier wrote:
> Gleb Natapov wrote:
> > On Thu, Oct 08, 2009 at 06:42:07PM +0200, Avi Kivity wrote:
> > > On 10/08/2009 06:34 PM, Gleb Natapov wrote:
> > > >So suppose I have simple watchdog device that required to be poked every
> > > >second, otherwise it resets a computer. On migration we have to migrate
> > > >time elapsed since last poke, but if device doesn't expose it to
> > > >software in any way you are saying we can recreate is some other way?
> > > 
> > > The time is exposed (you can measure it by poking the device and
> > The time yes, not its internal representation. What if one implementation 
> > stores how much time passed and another how much time left.
> > One counts in wall clack another only when guest runs. etc... and
> > this is a device with only one write only register.
> 
> In that case you can decide between calling it two different devices
> (which have the same guest-visible behaviour but are not
> interchangable), or calling them different implementations of one
> device - by adding a little more code to save state in a common format.
> 
That is what currency done for in-kernel/out-of-kernel irq chips. Save
state transformation. The problem begins if one of the devices has more
state (not just the same state but in a different format). You need to
drop info on migration.

> (Although they may have to be different devices for qemu
> configuration, it's ok for them to have the same PCI IDs and for the
> guest not to know the difference)
> 
> For your watchdog example, it's not hard to pick a saved state which
> works for both.
If you can't migrate from one to the other why even bother? In my
example if one device counts wallclock time and another guest cpu time
you can't migrate from one implementation to another.

> 
> ioapic will be harder to find a useful common saved state, and there
> might need to be an *optional hint* section (e.g. for selecting the
> next CPU to get an interrupt), but I think it would be worth it in
> this case.  Being able to load a KVM image into TCG and vice versa is
> quite useful sometimes.  E.g. I've had to do some OS installs using
> TCG at first, then switch to KVM later for performance.
> 
Reboot :)

--
			Gleb.

Re: Re: [PATCH v2 3/9] provide in-kernel ioapic

[Glauber Costa]

On Thu, Oct 08, 2009 at 06:22:48PM +0200, Gleb Natapov wrote:
> On Thu, Oct 08, 2009 at 06:17:57PM +0200, Avi Kivity wrote:
> > On 10/08/2009 06:07 PM, Jamie Lokier wrote:
> > >Haven't we already confirmed that it *isn't* just an ioapic accelerator
> > >because you can't migrate between in-kernel iopic and qemu's ioapic?
> > 
> > We haven't confirmed it.  Both implement the same spec, and if you
> > can't migrate between them, one of them has a bug (for example, qemu
> > ioapic doesn't implement polarity - but it's still just a bug).
> > 
> Are you saying that HW spec (that only describes software visible behavior)
> completely defines implementation? No other internal state is needed
> that may be done differently by different implementations?
Most specifications leaves a lot as implementation specific.

It's not hard to imagine a case in which both devices will follow the spec correctly,
(no bugs involved), and yet differ in the implementation.

Re: [Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Jamie Lokier]

Glauber Costa wrote:
> On Thu, Oct 08, 2009 at 06:22:48PM +0200, Gleb Natapov wrote:
> > On Thu, Oct 08, 2009 at 06:17:57PM +0200, Avi Kivity wrote:
> > > On 10/08/2009 06:07 PM, Jamie Lokier wrote:
> > > >Haven't we already confirmed that it *isn't* just an ioapic accelerator
> > > >because you can't migrate between in-kernel iopic and qemu's ioapic?
> > > 
> > > We haven't confirmed it.  Both implement the same spec, and if you
> > > can't migrate between them, one of them has a bug (for example, qemu
> > > ioapic doesn't implement polarity - but it's still just a bug).
> > > 
> > Are you saying that HW spec (that only describes software visible behavior)
> > completely defines implementation? No other internal state is needed
> > that may be done differently by different implementations?
> Most specifications leaves a lot as implementation specific.
> 
> It's not hard to imagine a case in which both devices will follow
> the spec correctly, (no bugs involved), and yet differ in the
> implementation.

Avi's not saying the implementations won't differ.  I believe he's
saying that implementation-specific states don't need to be saved if
they have no effect on guest visible behaviour.

-- Jamie

Re: [PATCH v2 3/9] provide in-kernel ioapic

[Juan Quintela]

Jamie Lokier <jamie@shareable.org> wrote:
> Glauber Costa wrote:
>> On Thu, Oct 08, 2009 at 06:22:48PM +0200, Gleb Natapov wrote:
>> > On Thu, Oct 08, 2009 at 06:17:57PM +0200, Avi Kivity wrote:
>> > > On 10/08/2009 06:07 PM, Jamie Lokier wrote:
>> > > >Haven't we already confirmed that it *isn't* just an ioapic accelerator
>> > > >because you can't migrate between in-kernel iopic and qemu's ioapic?
>> > > 
>> > > We haven't confirmed it.  Both implement the same spec, and if you
>> > > can't migrate between them, one of them has a bug (for example, qemu
>> > > ioapic doesn't implement polarity - but it's still just a bug).
>> > > 
>> > Are you saying that HW spec (that only describes software visible behavior)
>> > completely defines implementation? No other internal state is needed
>> > that may be done differently by different implementations?
>> Most specifications leaves a lot as implementation specific.
>> 
>> It's not hard to imagine a case in which both devices will follow
>> the spec correctly, (no bugs involved), and yet differ in the
>> implementation.
>
> Avi's not saying the implementations won't differ.  I believe he's
> saying that implementation-specific states don't need to be saved if
> they have no effect on guest visible behaviour.

Just to re-state.  I would also prefer to have a single device.  Reasons
(majority already told in the thread):
- We can switch between devices more easily
- They are emulating the same device.
- At the moment that you have two different devices, one of them will
  rot :(
- Adding state to the save/load format that is used only from one device
  is not a problem.

I notice that discussion is going nowhere, basically we are in the
state:
- people that want one device
  * they emulate the same hardware
  * lots of code is shared
  * they should be interchageable
  * if they are not interchageable, it is a bug
  * once that they are split, it is basically imposible to join then
    again.
- people that want 2 devices:
  * The devices can more easily diverge if they are two devices
  * They are not interchageable now
  * It allows you more freedom in changing any of them if they are
    separate.

As you can see, none of the proposals is a clear winner.  And what is
worse, we have the two maintainers (avi and anthony), the two with more
experience having to deal with this kind of situation disagreeing.

How to fix the impass?

Later, Juan.

Re: [PATCH v2 3/9] provide in-kernel ioapic

[Glauber Costa]

On Fri, Oct 09, 2009 at 09:55:13PM +0200, Juan Quintela wrote:
> Jamie Lokier <jamie@shareable.org> wrote:
> > Glauber Costa wrote:
> >> On Thu, Oct 08, 2009 at 06:22:48PM +0200, Gleb Natapov wrote:
> >> > On Thu, Oct 08, 2009 at 06:17:57PM +0200, Avi Kivity wrote:
> >> > > On 10/08/2009 06:07 PM, Jamie Lokier wrote:
> >> > > >Haven't we already confirmed that it *isn't* just an ioapic accelerator
> >> > > >because you can't migrate between in-kernel iopic and qemu's ioapic?
> >> > > 
> >> > > We haven't confirmed it.  Both implement the same spec, and if you
> >> > > can't migrate between them, one of them has a bug (for example, qemu
> >> > > ioapic doesn't implement polarity - but it's still just a bug).
> >> > > 
> >> > Are you saying that HW spec (that only describes software visible behavior)
> >> > completely defines implementation? No other internal state is needed
> >> > that may be done differently by different implementations?
> >> Most specifications leaves a lot as implementation specific.
> >> 
> >> It's not hard to imagine a case in which both devices will follow
> >> the spec correctly, (no bugs involved), and yet differ in the
> >> implementation.
> >
> > Avi's not saying the implementations won't differ.  I believe he's
> > saying that implementation-specific states don't need to be saved if
> > they have no effect on guest visible behaviour.
> 
> Just to re-state.  I would also prefer to have a single device.  Reasons
> (majority already told in the thread):
> - We can switch between devices more easily
> - They are emulating the same device.
> - At the moment that you have two different devices, one of them will
>   rot :(
> - Adding state to the save/load format that is used only from one device
>   is not a problem.
> 
> I notice that discussion is going nowhere, basically we are in the
> state:
> - people that want one device
>   * they emulate the same hardware
>   * lots of code is shared
>   * they should be interchageable
>   * if they are not interchageable, it is a bug
>   * once that they are split, it is basically imposible to join then
>     again.
> - people that want 2 devices:
>   * The devices can more easily diverge if they are two devices
>   * They are not interchageable now
>   * It allows you more freedom in changing any of them if they are
>     separate.
> 
> As you can see, none of the proposals is a clear winner.  And what is
> worse, we have the two maintainers (avi and anthony), the two with more
> experience having to deal with this kind of situation disagreeing.
> 
> How to fix the impass?
a deathmatch?

Re: [PATCH v2 3/9] provide in-kernel ioapic

[Anthony Liguori]

Juan Quintela wrote:
> I notice that discussion is going nowhere, basically we are in the
> state:
> - people that want one device
>   * they emulate the same hardware
>   * lots of code is shared
>   * they should be interchageable
>   * if they are not interchageable, it is a bug
>   * once that they are split, it is basically imposible to join then
>     again.
> - people that want 2 devices:
>   * The devices can more easily diverge if they are two devices
>   * They are not interchageable now
>   * It allows you more freedom in changing any of them if they are
>     separate.
>
> As you can see, none of the proposals is a clear winner.  And what is
> worse, we have the two maintainers (avi and anthony), the two with more
> experience having to deal with this kind of situation disagreeing.
>
> How to fix the impass?
>   

We already have the single device model implementation and the 
limitations are well known.  The best way to move forward is for someone 
to send out patches implementing separate device models.

At that point, it becomes a discussion of two concrete pieces of code 
verses hand waving.

> Later, Juan.
>   


-- 
Regards,

Anthony Liguori

Re: [PATCH v2 3/9] provide in-kernel ioapic

[Jamie Lokier]

Anthony Liguori wrote:
> We already have the single device model implementation and the 
> limitations are well known.  The best way to move forward is for someone 
> to send out patches implementing separate device models.
> 
> At that point, it becomes a discussion of two concrete pieces of code 
> verses hand waving.

Out of curiosity now, what _are_ the behavioural differences between
the in-kernel irqchip and the qemu one?

Are the differences significant to guests, such that it might be
necessary to disable the in-kernel irqchip for some guests, or
conversely, necessary to use KVM for some guests?

Thanks,
-- Jamie

Re: [PATCH v2 3/9] provide in-kernel ioapic

[Anthony Liguori]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>> We already have the single device model implementation and the 
>> limitations are well known.  The best way to move forward is for someone 
>> to send out patches implementing separate device models.
>>
>> At that point, it becomes a discussion of two concrete pieces of code 
>> verses hand waving.
>>     
>
> Out of curiosity now, what _are_ the behavioural differences between
> the in-kernel irqchip and the qemu one?
>
> Are the differences significant to guests, such that it might be
> necessary to disable the in-kernel irqchip for some guests, or
> conversely, necessary to use KVM for some guests?
>   

No, the behavior differences are not terribly significant for the apic.  
Disabling it is really most useful for debugging purposes.

Regards,

Anthony Liguori

Re: [PATCH v2 4/9] provide in-kernel apic

[Anthony Liguori]

Glauber Costa wrote:
> This patch provides kvm with an in-kernel apic. We are currently not enabling it.
> The code is heavily based on what's in qemu-kvm.git.
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/apic.c         |  135 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  kvm.h             |    3 +
>  target-i386/kvm.c |   18 +++++++
>  3 files changed, 152 insertions(+), 4 deletions(-)
>
> diff --git a/hw/apic.c b/hw/apic.c
> index c89008e..5635607 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -299,7 +299,11 @@ void cpu_set_apic_base(CPUState *env, uint64_t val)
>  #endif
>      if (!s)
>          return;
> -    s->apicbase = (val & 0xfffff000) |
> +
> +    if (kvm_enabled() && kvm_irqchip_in_kernel())
> +        s->apicbase = val;
> +    else
> +        s->apicbase = (val & 0xfffff000) |
>          (s->apicbase & (MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE));
>      /* if disabled, cannot be enabled again */
>      if (!(val & MSR_IA32_APICBASE_ENABLE)) {
> @@ -497,6 +501,13 @@ void apic_init_reset(CPUState *env)
>      s->wait_for_sipi = 1;
>
>      env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
> +
> +#ifdef KVM_CAP_MP_STATE
> +    if (kvm_enabled() && kvm_irqchip_in_kernel())
> +        env->mp_state
> +            = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
> +#endif
>   

I don't think CAP_MP_STATE should be treated as an optional feature.

> +static int kvm_kernel_lapic_load_from_user(APICState *s)
> +{
> +    int r = 0;
> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> +    struct kvm_lapic_state apic;
> +    struct kvm_lapic_state *klapic = &apic;
> +    int i;
> +
> +    if (!(kvm_enabled() && kvm_irqchip_in_kernel()))
> +        return 0;
> +
> +    memset(klapic, 0, sizeof apic);
> +    kapic_set_reg(klapic, 0x2, s->id << 24);
> +    kapic_set_reg(klapic, 0x8, s->tpr);
> +    kapic_set_reg(klapic, 0xd, s->log_dest << 24);
> +    kapic_set_reg(klapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
> +    kapic_set_reg(klapic, 0xf, s->spurious_vec);
> +    for (i = 0; i < 8; i++) {
> +        kapic_set_reg(klapic, 0x10 + i, s->isr[i]);
> +        kapic_set_reg(klapic, 0x18 + i, s->tmr[i]);
> +        kapic_set_reg(klapic, 0x20 + i, s->irr[i]);
> +    }
> +    kapic_set_reg(klapic, 0x28, s->esr);
> +    kapic_set_reg(klapic, 0x30, s->icr[0]);
> +    kapic_set_reg(klapic, 0x31, s->icr[1]);
> +    for (i = 0; i < APIC_LVT_NB; i++)
> +        kapic_set_reg(klapic, 0x32 + i, s->lvt[i]);
> +    kapic_set_reg(klapic, 0x38, s->initial_count);
> +    kapic_set_reg(klapic, 0x3e, s->divide_conf);
> +
> +    r = kvm_set_lapic(s->cpu_env, klapic);
> +#endif
> +    return r;
> +}
>   

You should probably just setup VMState such that it directly saves 
kvm_lapic_state and then have the pre/post functions call the kernel 
ioctls to sync it.  There's not a whole lot of point switching the state 
between two different structures.

>  static const VMStateDescription vmstate_apic = {
>      .name = "apic",
>      .version_id = 3,
>      .minimum_version_id = 3,
>      .minimum_version_id_old = 1,
>      .load_state_old = apic_load_old,
> +    .pre_save = apic_pre_save,
> +    .post_load = apic_post_load,
>      .fields      = (VMStateField []) {
>          VMSTATE_UINT32(apicbase, APICState),
>          VMSTATE_UINT8(id, APICState),
> @@ -933,9 +1052,8 @@ static const VMStateDescription vmstate_apic = {
>      }
>  };
>   

Same applies here as ioapic.  Should be a separate device.

-- 
Regards,

Anthony Liguori

Re: Re: [PATCH v2 4/9] provide in-kernel apic

[Avi Kivity]

On 10/08/2009 03:55 PM, Anthony Liguori wrote:
> Glauber Costa wrote:
>> This patch provides kvm with an in-kernel apic. We are currently not 
>> enabling it.
>> The code is heavily based on what's in qemu-kvm.git.
>>
>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>> ---
>>  hw/apic.c         |  135 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  kvm.h             |    3 +
>>  target-i386/kvm.c |   18 +++++++
>>  3 files changed, 152 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index c89008e..5635607 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -299,7 +299,11 @@ void cpu_set_apic_base(CPUState *env, uint64_t val)
>>  #endif
>>      if (!s)
>>          return;
>> -    s->apicbase = (val & 0xfffff000) |
>> +
>> +    if (kvm_enabled() && kvm_irqchip_in_kernel())
>> +        s->apicbase = val;
>> +    else
>> +        s->apicbase = (val & 0xfffff000) |
>>          (s->apicbase & (MSR_IA32_APICBASE_BSP | 
>> MSR_IA32_APICBASE_ENABLE));
>>      /* if disabled, cannot be enabled again */
>>      if (!(val & MSR_IA32_APICBASE_ENABLE)) {
>> @@ -497,6 +501,13 @@ void apic_init_reset(CPUState *env)
>>      s->wait_for_sipi = 1;
>>
>>      env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
>> +
>> +#ifdef KVM_CAP_MP_STATE
>> +    if (kvm_enabled() && kvm_irqchip_in_kernel())
>> +        env->mp_state
>> +            = env->halted ? KVM_MP_STATE_UNINITIALIZED : 
>> KVM_MP_STATE_RUNNABLE;
>> +#endif
>
> I don't think CAP_MP_STATE should be treated as an optional feature.
>
>> +static int kvm_kernel_lapic_load_from_user(APICState *s)
>> +{
>> +    int r = 0;
>> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
>> +    struct kvm_lapic_state apic;
>> +    struct kvm_lapic_state *klapic = &apic;
>> +    int i;
>> +
>> +    if (!(kvm_enabled() && kvm_irqchip_in_kernel()))
>> +        return 0;
>> +
>> +    memset(klapic, 0, sizeof apic);
>> +    kapic_set_reg(klapic, 0x2, s->id << 24);
>> +    kapic_set_reg(klapic, 0x8, s->tpr);
>> +    kapic_set_reg(klapic, 0xd, s->log_dest << 24);
>> +    kapic_set_reg(klapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
>> +    kapic_set_reg(klapic, 0xf, s->spurious_vec);
>> +    for (i = 0; i < 8; i++) {
>> +        kapic_set_reg(klapic, 0x10 + i, s->isr[i]);
>> +        kapic_set_reg(klapic, 0x18 + i, s->tmr[i]);
>> +        kapic_set_reg(klapic, 0x20 + i, s->irr[i]);
>> +    }
>> +    kapic_set_reg(klapic, 0x28, s->esr);
>> +    kapic_set_reg(klapic, 0x30, s->icr[0]);
>> +    kapic_set_reg(klapic, 0x31, s->icr[1]);
>> +    for (i = 0; i < APIC_LVT_NB; i++)
>> +        kapic_set_reg(klapic, 0x32 + i, s->lvt[i]);
>> +    kapic_set_reg(klapic, 0x38, s->initial_count);
>> +    kapic_set_reg(klapic, 0x3e, s->divide_conf);
>> +
>> +    r = kvm_set_lapic(s->cpu_env, klapic);
>> +#endif
>> +    return r;
>> +}
>
> You should probably just setup VMState such that it directly saves 
> kvm_lapic_state and then have the pre/post functions call the kernel 
> ioctls to sync it.  There's not a whole lot of point switching the 
> state between two different structures.

It ensures the two models are compatible.  Since they're the same device 
from the point of view of the guest, there's no reason for them to have 
different representations or to be incompatible.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v2 4/9] provide in-kernel apic

[Glauber Costa]

On Thu, Oct 08, 2009 at 04:09:27PM +0200, Avi Kivity wrote:
> On 10/08/2009 03:55 PM, Anthony Liguori wrote:
>> Glauber Costa wrote:
>>> This patch provides kvm with an in-kernel apic. We are currently not  
>>> enabling it.
>>> The code is heavily based on what's in qemu-kvm.git.
>>>
>>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>> ---
>>>  hw/apic.c         |  135  
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>  kvm.h             |    3 +
>>>  target-i386/kvm.c |   18 +++++++
>>>  3 files changed, 152 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/apic.c b/hw/apic.c
>>> index c89008e..5635607 100644
>>> --- a/hw/apic.c
>>> +++ b/hw/apic.c
>>> @@ -299,7 +299,11 @@ void cpu_set_apic_base(CPUState *env, uint64_t val)
>>>  #endif
>>>      if (!s)
>>>          return;
>>> -    s->apicbase = (val & 0xfffff000) |
>>> +
>>> +    if (kvm_enabled() && kvm_irqchip_in_kernel())
>>> +        s->apicbase = val;
>>> +    else
>>> +        s->apicbase = (val & 0xfffff000) |
>>>          (s->apicbase & (MSR_IA32_APICBASE_BSP |  
>>> MSR_IA32_APICBASE_ENABLE));
>>>      /* if disabled, cannot be enabled again */
>>>      if (!(val & MSR_IA32_APICBASE_ENABLE)) {
>>> @@ -497,6 +501,13 @@ void apic_init_reset(CPUState *env)
>>>      s->wait_for_sipi = 1;
>>>
>>>      env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
>>> +
>>> +#ifdef KVM_CAP_MP_STATE
>>> +    if (kvm_enabled() && kvm_irqchip_in_kernel())
>>> +        env->mp_state
>>> +            = env->halted ? KVM_MP_STATE_UNINITIALIZED :  
>>> KVM_MP_STATE_RUNNABLE;
>>> +#endif
>>
>> I don't think CAP_MP_STATE should be treated as an optional feature.
>>
>>> +static int kvm_kernel_lapic_load_from_user(APICState *s)
>>> +{
>>> +    int r = 0;
>>> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
>>> +    struct kvm_lapic_state apic;
>>> +    struct kvm_lapic_state *klapic = &apic;
>>> +    int i;
>>> +
>>> +    if (!(kvm_enabled() && kvm_irqchip_in_kernel()))
>>> +        return 0;
>>> +
>>> +    memset(klapic, 0, sizeof apic);
>>> +    kapic_set_reg(klapic, 0x2, s->id << 24);
>>> +    kapic_set_reg(klapic, 0x8, s->tpr);
>>> +    kapic_set_reg(klapic, 0xd, s->log_dest << 24);
>>> +    kapic_set_reg(klapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
>>> +    kapic_set_reg(klapic, 0xf, s->spurious_vec);
>>> +    for (i = 0; i < 8; i++) {
>>> +        kapic_set_reg(klapic, 0x10 + i, s->isr[i]);
>>> +        kapic_set_reg(klapic, 0x18 + i, s->tmr[i]);
>>> +        kapic_set_reg(klapic, 0x20 + i, s->irr[i]);
>>> +    }
>>> +    kapic_set_reg(klapic, 0x28, s->esr);
>>> +    kapic_set_reg(klapic, 0x30, s->icr[0]);
>>> +    kapic_set_reg(klapic, 0x31, s->icr[1]);
>>> +    for (i = 0; i < APIC_LVT_NB; i++)
>>> +        kapic_set_reg(klapic, 0x32 + i, s->lvt[i]);
>>> +    kapic_set_reg(klapic, 0x38, s->initial_count);
>>> +    kapic_set_reg(klapic, 0x3e, s->divide_conf);
>>> +
>>> +    r = kvm_set_lapic(s->cpu_env, klapic);
>>> +#endif
>>> +    return r;
>>> +}
>>
>> You should probably just setup VMState such that it directly saves  
>> kvm_lapic_state and then have the pre/post functions call the kernel  
>> ioctls to sync it.  There's not a whole lot of point switching the  
>> state between two different structures.
>
> It ensures the two models are compatible.  Since they're the same device  
> from the point of view of the guest, there's no reason for them to have  
> different representations or to be incompatible.

live migration between something that has in-kernel irqchip and something that
has not is likely to be a completely untested thing. And this is the only reason
we might think of it as the same device. I don't see any value in supporting this
combination

Re: Re: [PATCH v2 4/9] provide in-kernel apic

[Jamie Lokier]

Glauber Costa wrote:
> > It ensures the two models are compatible.  Since they're the same device  
> > from the point of view of the guest, there's no reason for them to have  
> > different representations or to be incompatible.
> 
> live migration between something that has in-kernel irqchip and
> something that has not is likely to be a completely untested
> thing. And this is the only reason we might think of it as the same
> device. I don't see any value in supporting this combination

Not just live migration.  ACPI sleep + savevm + loadvm + ACPI resume,
for example.

-- Jamie

Re: Re: [PATCH v2 4/9] provide in-kernel apic

[Glauber Costa]

On Fri, Oct 09, 2009 at 11:06:41AM +0100, Jamie Lokier wrote:
> Glauber Costa wrote:
> > > It ensures the two models are compatible.  Since they're the same device  
> > > from the point of view of the guest, there's no reason for them to have  
> > > different representations or to be incompatible.
> > 
> > live migration between something that has in-kernel irqchip and
> > something that has not is likely to be a completely untested
> > thing. And this is the only reason we might think of it as the same
> > device. I don't see any value in supporting this combination
> 
> Not just live migration.  ACPI sleep + savevm + loadvm + ACPI resume,
> for example.
Yes, but in this case too, I'd expect the irqchipness of qemu not to change.

Re: [Qemu-devel] Re: [PATCH v2 4/9] provide in-kernel apic

[Jamie Lokier]

Glauber Costa wrote:
> On Fri, Oct 09, 2009 at 11:06:41AM +0100, Jamie Lokier wrote:
> > Glauber Costa wrote:
> > > > It ensures the two models are compatible.  Since they're the same device  
> > > > from the point of view of the guest, there's no reason for them to have  
> > > > different representations or to be incompatible.
> > > 
> > > live migration between something that has in-kernel irqchip and
> > > something that has not is likely to be a completely untested
> > > thing. And this is the only reason we might think of it as the same
> > > device. I don't see any value in supporting this combination
> > 
> > Not just live migration.  ACPI sleep + savevm + loadvm + ACPI resume,
> > for example.
> Yes, but in this case too, I'd expect the irqchipness of qemu not to change.

If I've just been sent an image produced by someone running KVM, and
my machine is not KVM-capable, or I cannot upgrade the KVM kernel
module because it's in use by other VMs (had this problem a few
times), there's no choice but to change the irqchipness.

-- Jamie

Re: Re: [PATCH v2 4/9] provide in-kernel apic

[Glauber Costa]

On Fri, Oct 09, 2009 at 05:48:31PM +0100, Jamie Lokier wrote:
> Glauber Costa wrote:
> > On Fri, Oct 09, 2009 at 11:06:41AM +0100, Jamie Lokier wrote:
> > > Glauber Costa wrote:
> > > > > It ensures the two models are compatible.  Since they're the same device  
> > > > > from the point of view of the guest, there's no reason for them to have  
> > > > > different representations or to be incompatible.
> > > > 
> > > > live migration between something that has in-kernel irqchip and
> > > > something that has not is likely to be a completely untested
> > > > thing. And this is the only reason we might think of it as the same
> > > > device. I don't see any value in supporting this combination
> > > 
> > > Not just live migration.  ACPI sleep + savevm + loadvm + ACPI resume,
> > > for example.
> > Yes, but in this case too, I'd expect the irqchipness of qemu not to change.
> 
> If I've just been sent an image produced by someone running KVM, and
> my machine is not KVM-capable, or I cannot upgrade the KVM kernel
> module because it's in use by other VMs (had this problem a few
> times), there's no choice but to change the irqchipness.
As gleb mentioned, requiring such a change to happen offline (across a reboot)
is not that much of a pain.

There are thousands of scenarios in which it will have to happen anyway,
including major bumps in qemu version. 

Re: [Qemu-devel] Re: [PATCH v2 4/9] provide in-kernel apic

[Anthony Liguori]

Jamie Lokier wrote:
> Glauber Costa wrote:
>   
>> On Fri, Oct 09, 2009 at 11:06:41AM +0100, Jamie Lokier wrote:
>>     
>>> Glauber Costa wrote:
>>>       
>>>>> It ensures the two models are compatible.  Since they're the same device  
>>>>> from the point of view of the guest, there's no reason for them to have  
>>>>> different representations or to be incompatible.
>>>>>           
>>>> live migration between something that has in-kernel irqchip and
>>>> something that has not is likely to be a completely untested
>>>> thing. And this is the only reason we might think of it as the same
>>>> device. I don't see any value in supporting this combination
>>>>         
>>> Not just live migration.  ACPI sleep + savevm + loadvm + ACPI resume,
>>> for example.
>>>       
>> Yes, but in this case too, I'd expect the irqchipness of qemu not to change.
>>     
>
> If I've just been sent an image produced by someone running KVM, and
> my machine is not KVM-capable, or I cannot upgrade the KVM kernel
> module because it's in use by other VMs (had this problem a few
> times), there's no choice but to change the irqchipness.
>   

You cannot migrate from KVM to TCG so this use-case is irrelevant.


-- 
Regards,

Anthony Liguori

Re: Re: [PATCH v2 4/9] provide in-kernel apic

[Avi Kivity]

On 10/09/2009 09:49 PM, Anthony Liguori wrote:
>> If I've just been sent an image produced by someone running KVM, and
>> my machine is not KVM-capable, or I cannot upgrade the KVM kernel
>> module because it's in use by other VMs (had this problem a few
>> times), there's no choice but to change the irqchipness.
>
>
> You cannot migrate from KVM to TCG so this use-case is irrelevant.

I agree it's mostly irrelevant, however nothing in principle prevents 
such a migration, as long as the cpuid feature bits are implemented on 
both sides.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v2 4/9] provide in-kernel apic

[Anthony Liguori]

Avi Kivity wrote:
> On 10/09/2009 09:49 PM, Anthony Liguori wrote:
>>> If I've just been sent an image produced by someone running KVM, and
>>> my machine is not KVM-capable, or I cannot upgrade the KVM kernel
>>> module because it's in use by other VMs (had this problem a few
>>> times), there's no choice but to change the irqchipness.
>>
>>
>> You cannot migrate from KVM to TCG so this use-case is irrelevant.
>
> I agree it's mostly irrelevant, however nothing in principle prevents 
> such a migration, as long as the cpuid feature bits are implemented on 
> both sides.

Sure, in principle it's certainly possible but in practice, it isn't 
today and I don't see anything that's likely to happen in the near term 
future that would make it.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH v2 4/9] provide in-kernel apic

[Anthony Liguori]

Avi Kivity wrote:
> On 10/08/2009 03:55 PM, Anthony Liguori wrote:
>>
>> You should probably just setup VMState such that it directly saves 
>> kvm_lapic_state and then have the pre/post functions call the kernel 
>> ioctls to sync it.  There's not a whole lot of point switching the 
>> state between two different structures.
>
> It ensures the two models are compatible.  Since they're the same 
> device from the point of view of the guest, there's no reason for them 
> to have different representations or to be incompatible.

The problem is, the in-kernel apic is not part of the qemu source tree.  
If we add a field to the qemu apic, then we would break the in-kernel 
apic and vice-versa.  It's far easier to just have the in-kernel apic as 
a separate model.

Most importantly though, the code should reflect how things behave.  
It's extremely difficult to look at the apic code and figure out what 
bits are used and what bits aren't used when the in-kernel apic is enabled.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH v2 4/9] provide in-kernel apic

[Avi Kivity]

On 10/08/2009 04:26 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 10/08/2009 03:55 PM, Anthony Liguori wrote:
>>>
>>> You should probably just setup VMState such that it directly saves 
>>> kvm_lapic_state and then have the pre/post functions call the kernel 
>>> ioctls to sync it.  There's not a whole lot of point switching the 
>>> state between two different structures.
>>
>> It ensures the two models are compatible.  Since they're the same 
>> device from the point of view of the guest, there's no reason for 
>> them to have different representations or to be incompatible.
>
> The problem is, the in-kernel apic is not part of the qemu source 
> tree.  If we add a field to the qemu apic, then we would break the 
> in-kernel apic and vice-versa.  It's far easier to just have the 
> in-kernel apic as a separate model.
>

You need to handle it anyway due to save/restore; that is the new field 
and whatever functionality it has must be optional.

> Most importantly though, the code should reflect how things behave.  
> It's extremely difficult to look at the apic code and figure out what 
> bits are used and what bits aren't used when the in-kernel apic is 
> enabled.

That doesn't mean they need to be separate devices.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v2 4/9] provide in-kernel apic

[Anthony Liguori]

Avi Kivity wrote:
>> The problem is, the in-kernel apic is not part of the qemu source 
>> tree.  If we add a field to the qemu apic, then we would break the 
>> in-kernel apic and vice-versa.  It's far easier to just have the 
>> in-kernel apic as a separate model.
>>
>
> You need to handle it anyway due to save/restore; that is the new 
> field and whatever functionality it has must be optional.

Not necessarily.  If it's a bug fix, we may disable the older versions 
of savevm (which we have to do from time to time).

> That doesn't mean they need to be separate devices.

But they are.  The in-kernel device models are not mere accelerations.  
There are features that are only enabled with in-kernel device models 
(like PCI passthrough).  In fact, in-kernel PIT is not at all an 
acceleration, it's there because it's functionally different.

The sync stuff is really ugly too.  It would be much cleaner to have a 
separate state for the in-kernel device models that saved the structures 
from the kernel directly instead of having to translate between 
formats.  More straight forward code means it's all easier to 
understand, easier to debug, etc.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH v2 4/9] provide in-kernel apic

[Glauber Costa]

On Thu, Oct 08, 2009 at 09:39:13AM -0500, Anthony Liguori wrote:
> The sync stuff is really ugly too.  It would be much cleaner to have a  
> separate state for the in-kernel device models that saved the structures  
> from the kernel directly instead of having to translate between formats.  
> More straight forward code means it's all easier to understand, easier to 
> debug, etc.
>
One may arguee that I am stupid, but I have caught myself reading code that
was totally unused in the quest for irqchip related bugs...

Re: [Qemu-devel] Re: [PATCH v2 4/9] provide in-kernel apic

[Glauber Costa]

On Thu, Oct 08, 2009 at 04:31:57PM +0200, Avi Kivity wrote:
> On 10/08/2009 04:26 PM, Anthony Liguori wrote:
>> Avi Kivity wrote:
>>> On 10/08/2009 03:55 PM, Anthony Liguori wrote:
>>>>
>>>> You should probably just setup VMState such that it directly saves  
>>>> kvm_lapic_state and then have the pre/post functions call the 
>>>> kernel ioctls to sync it.  There's not a whole lot of point 
>>>> switching the state between two different structures.
>>>
>>> It ensures the two models are compatible.  Since they're the same  
>>> device from the point of view of the guest, there's no reason for  
>>> them to have different representations or to be incompatible.
>>
>> The problem is, the in-kernel apic is not part of the qemu source  
>> tree.  If we add a field to the qemu apic, then we would break the  
>> in-kernel apic and vice-versa.  It's far easier to just have the  
>> in-kernel apic as a separate model.
>>
>
> You need to handle it anyway due to save/restore; that is the new field  
> and whatever functionality it has must be optional.
Not necessarily. You can grab the structures directly from the kernel definition
, copy that over, issue the ioctl, and just make sure the source and destination
have compatible kernels.