qemu-devel
[Top][All Lists]
Advanced

[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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]