[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and
From: |
Anthony Liguori |
Subject: |
Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c |
Date: |
Wed, 01 Aug 2012 15:02:52 -0500 |
User-agent: |
Notmuch/0.13.2+93~ged93d79 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) |
Andreas Färber <address@hidden> writes:
> Am 01.08.2012 20:25, schrieb Anthony Liguori:
>> Andreas Färber <address@hidden> writes:
>>
>>> Am 01.08.2012 17:43, schrieb Anthony Liguori:
>>>> Igor Mammedov <address@hidden> writes:
>>>>
>>>>> v2:
>>>>> ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>>>>> to keep cpu_init implementation in -softmmu and -user targets the same
>>>>> in single place and maintanable.
>>>>>
>>>>> v3:
>>>>> reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit
>>>>> set
>>>>>
>>>>> tree for testing:
>>>>> https://github.com/imammedo/qemu/tree/x86_reset_v3
>>>>>
>>>>> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
>>>>>
>>>>> Igor Mammedov (2):
>>>>> target-i386: move cpu halted decision into x86_cpu_reset
>>>>> target-i386: move cpu_reset and reset callback to cpu.c
>>>>
>>>> Applied all. Thanks.
>>>
>>> So do you intend to refactor all machines accordingly or leave it
>>> inconsistent now?
>>
>> Are you asking me?
>>
>> No, I have no intention of touching any other machine. We're not going
>> to limit cleaning up target-i386 unless every other machine is cleaned
>> up too.
>>
>> Reset logic should live in the CPU. Seems like a no-brainer to me.
>
> Yes, I'm asking you, since you replied and applied the series without
> responding to my review comment on patch 2/2. You probably applied it
> locally before reading my comments but then I would still have expected
> a reply on how to proceed in light of those comments:
No, I saw your comment, although I had already decided to apply it by
then.
> Before applying this, as I've pointed out to Igor at least once before,
> all machines do such reset handling themselves. Patch 2/2 that you
> applied makes target-i386 break away from that scheme. (I wonder that
> Peter hasn't protested yet...)
Devices manage their own reset. CPUs are just another type of device.
It's completely logically that CPUs handle their own reset.
> Anyway, that being the last patch in this series, I see no value in
> doing this on its own for target-i386 only.
There's obvious value. You would prefer all targets get refactored
too. But that's an unrealistic expectation to place on contributors.
> So now we should either
> revert that patch and later replace it with one that does a touch-all
> change across the boards, or someone needs to volunteer (and you agree,
> during the Freeze) to refactor all other machines accordingly, which
> will take a while to get Acked-bys from machine maintainers... Or just
> defer touching reset callbacks until we have the CPU as a device and
> then drop the callbacks instead of moving them.
Sorry, but no, this is completely unreasonable. Fighting against
improvements because you want more to be improved is
counter-productive. No step in the right direction is too small.
> Note the point of disagreement here is not "reset logic" - it's great
> that the APIC BSP fiddling is gone from PC with patch 1/2 - but the
> registration of system-level callbacks in cpu.c in patch 2/2. I thought
> we all agreed that we want to make CPU a device and have it reset as a
> device? No such callback in cpu.c will be needed then and we thus seem
> to be, in absence of follow-ups for 1.2, needlessly moving to-be-dead
> code around. Not doing that seems like a no-brainer to me.
Devices do one of two things today:
1) register a reset callback
2) implement a reset method that is invoked through it's parent bus
Since I don't expect CPUs to exist on a bus, it's not immediately clear
to me that (1) isn't going to be what we do for quite some time.
Regards,
Anthony Liguori
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Anthony Liguori, 2012/08/01
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Andreas Färber, 2012/08/01
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Anthony Liguori, 2012/08/01
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Andreas Färber, 2012/08/01
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c,
Anthony Liguori <=
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Andreas Färber, 2012/08/01
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Anthony Liguori, 2012/08/01
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Andreas Färber, 2012/08/01
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Peter Maydell, 2012/08/01
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Andreas Färber, 2012/08/01
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Igor Mammedov, 2012/08/02
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Andreas Färber, 2012/08/01
- Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c, Anthony Liguori, 2012/08/01