[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: |
David Gibson |
Subject: |
Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core |
Date: |
Thu, 10 Dec 2020 14:53:02 +1100 |
On Wed, Dec 09, 2020 at 01:26:17PM -0500, Eduardo Habkost 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().
Well, also machine reset, but the point is it's not the resetfn() of
the cpu.
Basically what this is doing is machine specific (rather than just cpu
specific) initialization of the cpu state - we need that because the
pseries machine is implementing an explicitly paravirtualized platform
which starts things off in a state a bit different from the "raw" cpu
behaviour.
So, although it's working on a CPU's state, this function actually
belongs to the machine, rather than the cpu.
> 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.
But, yeah, this...
I've applied some of the later patches in this series, but I'm not
convinced on this one or 2/6. It seems like they're just replacing
one ugly (access to qdev_machine_state() as a global) with a different
ugly (duplicating something which has to equal the global machine
pointer as properties in a bunch of other objects).
Both 1/6 and 2/6 are altering explicitly spapr specific devices, they
have interactions with the overall platform model which mean they have
to sit in that environment, so I think trying to add a property here
implies an abstraction that can't actually be used in practice.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
- [PATCH 4/6] spapr: Don't use qdev_get_machine() in spapr_msi_write(), (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, 2020/12/09
- Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core,
David Gibson <=
- 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