[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core
From: |
Cédric Le Goater |
Subject: |
Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core |
Date: |
Thu, 10 Dec 2020 09:23:58 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 |
On 12/9/20 9:54 PM, Eduardo Habkost wrote:
> On Wed, Dec 09, 2020 at 09:24:36PM +0100, Greg Kurz wrote:
>> On Wed, 9 Dec 2020 13:26:17 -0500
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>>> On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote:
>>> [...]
>>>>>>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
>>>>>>>> int i;
>>>>>>>>
>>>>>>>> for (i = 0; i < cc->nr_threads; i++) {
>>>>>>>> - spapr_reset_vcpu(sc->threads[i]);
>>>>>>>> + spapr_reset_vcpu(sc->threads[i], sc->spapr);
>>>>>>>
>>>>>>> Why reset() needs access to the machine state, don't
>>>>>>> you have it in realize()?
>>>>>>>
>>>>>>
>>>>>> This is for the vCPU threads of the sPAPR CPU core. They don't have the
>>>>>> link to the machine state.
>>>>>
>>>>> They are created by spapr_create_vcpu() + spapr_realize_vcpu() in
>>>>> spapr_cpu_core_realize(), which has sc->spapr set... Am I missing
>>>>> something?
>>>>
>>>> Anyhow, from a QOM design point of view, resetfn() is not the correct
>>>> place to set a property IMHO, so Cc'ing Eduardo.
>>>
>>> This patch is not setting the property on resetfn(), it is
>>> setting it on CPU core pre_plug().
>>>
>>> This is more complex than simply using qdev_get_machine() and I
>>> don't see why it would be better, but I don't think it's wrong.
>>>
>>
>> The reference to the machine state is basically needed to
>> setup/reset/teardown interrupt presenters in the IRQ chip
>> backend. It is a bit unfortunate to express this dependency
>> at realize(), reset() and unrealize(). Maybe having an
>> "irq_chip" property linked to the IRQ chip backend would
>> make more sense ?
>>
>
> Considering that the spapr_irq_*() functions get a
> SpaprMachineState argument and deal with two interrupt
> controllers, maybe you won't be able to save what you need in a
> single irq_chip field?
The sPAPR machine needs to operate on all available interrupt
controllers and the array is built under the spapr_irq* routines
with :
SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
We could add the array under the machine and use a link to that
instead but we need to keep the existing interrupt controllers
anyway because of migration compatibility.
> I don't have a strong opinion here. It feels weird to me to save
> a reference to the global machine object that is always
> available, but I don't think that's a problem if you believe the
> resulting code looks better.
I think it's a good cleanup to remove globals. QEMU might emulate
multiple "machines" in a single binary one day.
C.
- [PATCH 5/6] spapr: Pass sPAPR machine state to some RTAS events handling functions, (continued)
- [PATCH 5/6] spapr: Pass sPAPR machine state to some RTAS events handling functions, Greg Kurz, 2020/12/09
- [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core, Greg Kurz, 2020/12/09
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core, Philippe Mathieu-Daudé, 2020/12/09
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core, Greg Kurz, 2020/12/09
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core, Philippe Mathieu-Daudé, 2020/12/09
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core, Philippe Mathieu-Daudé, 2020/12/09
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core, Eduardo Habkost, 2020/12/09
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core, Greg Kurz, 2020/12/09
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core, Eduardo Habkost, 2020/12/09
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core,
Cédric Le Goater <=
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core, David Gibson, 2020/12/09
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core, Greg Kurz, 2020/12/10