[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: |
Eduardo Habkost |
Subject: |
Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core |
Date: |
Wed, 9 Dec 2020 15:54:04 -0500 |
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?
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.
--
Eduardo
- Re: [PATCH 5/6] spapr: Pass sPAPR machine state to some RTAS events handling functions, (continued)
- [PATCH 4/6] spapr: Don't use qdev_get_machine() in spapr_msi_write(), 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 <=
- 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
[PATCH 6/6] target/ppc: Add mce_req_event() handler to PPCVirtualHypervisorClass, Greg Kurz, 2020/12/09
[PATCH 3/6] spapr: Pass sPAPR machine state down to spapr_pci_switch_vga(), Greg Kurz, 2020/12/09
[PATCH 2/6] spapr: Add an "spapr" property to sPAPR PHB, Greg Kurz, 2020/12/09