Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On Wed, 12 Dec 2018 11:29:55 +1100
David Gibson wrote:
> On Tue, Dec 11, 2018 at 10:55:59AM +0100, Greg Kurz wrote:
> > On Tue, 11 Dec 2018 14:53:32 +1100
> > Alexey Kardashevskiy wrote:
> >
> > > On 10/12/2018 20:30, Greg Kurz wrote:
> > > > On Mon, 10 Dec 2018 17:20:43 +1100
> > > > David Gibson wrote:
> > > >
> > > >> On Mon, Nov 12, 2018 at 03:12:26PM +1100, Alexey Kardashevskiy wrote:
> > > >>
> > > >>>
> > > >>>
> > > >>> On 12/11/2018 05:10, Greg Kurz wrote:
> > > Hi Alexey,
> > >
> > > Just a few remarks. See below.
> > >
> > > On Thu, 8 Nov 2018 12:44:06 +1100
> > > Alexey Kardashevskiy wrote:
> > >
> > > > SLOF receives a device tree and updates it with various properties
> > > > before switching to the guest kernel and QEMU is not aware of any
> > > > changes
> > > > made by SLOF. Since there is no real RTAS (QEMU implements it), it
> > > > makes
> > > > sense to pass the SLOF final device tree to QEMU to let it implement
> > > > RTAS related tasks better, such as PCI host bus adapter hotplug.
> > > >
> > > > Specifially, now QEMU can find out the actual XICS phandle (for PHB
> > > > hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> > > > assisted NMI - FWNMI).
> > > >
> > > > This stores the initial DT blob in the sPAPR machine and replaces it
> > > > in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> > > >
> > > > This adds an @update_dt_enabled machine property to allow backward
> > > > migration.
> > > >
> > > > SLOF already has a hypercall since
> > > > https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> > > >
> > > > Signed-off-by: Alexey Kardashevskiy
> > > > ---
> > > > include/hw/ppc/spapr.h | 7 ++-
> > > > hw/ppc/spapr.c | 29 -
> > > > hw/ppc/spapr_hcall.c | 32
> > > > hw/ppc/trace-events| 2 ++
> > > > 4 files changed, 68 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > index ad4d7cfd97..f5dcaf44cb 100644
> > > > --- a/include/hw/ppc/spapr.h
> > > > +++ b/include/hw/ppc/spapr.h
> > > > @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
> > > >
> > > > /*< public >*/
> > > > bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug
> > > > of LMBs */
> > > > +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> > > > bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > > > bool pre_2_10_has_unused_icps;
> > > > bool legacy_irq_allocation;
> > > > @@ -136,6 +137,9 @@ struct sPAPRMachineState {
> > > > int vrma_adjust;
> > > > ssize_t rtas_size;
> > > > void *rtas_blob;
> > > > +uint32_t fdt_size;
> > > > +uint32_t fdt_initial_size;
> > >
> > > I don't quite see the purpose of fdt_initial_size... it seems to be
> > > only
> > > used to print a trace.
> > > >>>
> > > >>>
> > > >>> Ah, lost in rebase. The purpose was to test if the new device tree has
> > > >>> not grown too much.
> > > >>>
> > > >>>
> > > >>>
> > >
> > > > +void *fdt_blob;
> > > > long kernel_size;
> > > > bool kernel_le;
> > > > uint32_t initrd_base;
> > > > @@ -462,7 +466,8 @@ struct sPAPRMachineState {
> > > > #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> > > > /* Client Architecture support */
> > > > #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> > > > -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> > > > +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> > > > +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
> > > >
> > > > typedef struct sPAPRDeviceTreeUpdateHeader {
> > > > uint32_t version_id;
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index c08130facb..5e2d4d211c 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
> > > > /* Load the fdt */
> > > > qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > > > cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> > > > -g_free(fdt);
> > > > +g_free(spapr->fdt_blob);
> > > > +spapr->fdt_size = fdt_totalsize(fdt);
> > > > +spapr->fdt_initial_size = spapr->fdt_size;
> > > > +spapr->fdt_blob = fdt;
> > >
> > > Hmm... It looks weird to store state in a reset handler. I'd rather
> > > zeroe
> > > both fdt_blob and fdt_size here.
> > > >>>
> > > >>> The device tree is built from the reset handler and the idea is that
> > > >>> we
> > > >>> want to always have
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On Tue, Dec 11, 2018 at 02:36:09PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 10/12/2018 17:20, David Gibson wrote:
> > On Mon, Nov 12, 2018 at 03:12:26PM +1100, Alexey Kardashevskiy wrote:
> >>
> >>
> >> On 12/11/2018 05:10, Greg Kurz wrote:
> >>> Hi Alexey,
> >>>
> >>> Just a few remarks. See below.
> >>>
> >>> On Thu, 8 Nov 2018 12:44:06 +1100
> >>> Alexey Kardashevskiy wrote:
> >>>
> SLOF receives a device tree and updates it with various properties
> before switching to the guest kernel and QEMU is not aware of any changes
> made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> sense to pass the SLOF final device tree to QEMU to let it implement
> RTAS related tasks better, such as PCI host bus adapter hotplug.
>
> Specifially, now QEMU can find out the actual XICS phandle (for PHB
> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> assisted NMI - FWNMI).
>
> This stores the initial DT blob in the sPAPR machine and replaces it
> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
>
> This adds an @update_dt_enabled machine property to allow backward
> migration.
>
> SLOF already has a hypercall since
> https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
>
> Signed-off-by: Alexey Kardashevskiy
> ---
> include/hw/ppc/spapr.h | 7 ++-
> hw/ppc/spapr.c | 29 -
> hw/ppc/spapr_hcall.c | 32
> hw/ppc/trace-events| 2 ++
> 4 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ad4d7cfd97..f5dcaf44cb 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
>
> /*< public >*/
> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of
> LMBs */
> +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> bool pre_2_10_has_unused_icps;
> bool legacy_irq_allocation;
> @@ -136,6 +137,9 @@ struct sPAPRMachineState {
> int vrma_adjust;
> ssize_t rtas_size;
> void *rtas_blob;
> +uint32_t fdt_size;
> +uint32_t fdt_initial_size;
> >>>
> >>> I don't quite see the purpose of fdt_initial_size... it seems to be only
> >>> used to print a trace.
> >>
> >>
> >> Ah, lost in rebase. The purpose was to test if the new device tree has
> >> not grown too much.
> >>
> >>
> >>
> >>>
> +void *fdt_blob;
> long kernel_size;
> bool kernel_le;
> uint32_t initrd_base;
> @@ -462,7 +466,8 @@ struct sPAPRMachineState {
> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> /* Client Architecture support */
> #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
>
> typedef struct sPAPRDeviceTreeUpdateHeader {
> uint32_t version_id;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c08130facb..5e2d4d211c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
> /* Load the fdt */
> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> -g_free(fdt);
> +g_free(spapr->fdt_blob);
> +spapr->fdt_size = fdt_totalsize(fdt);
> +spapr->fdt_initial_size = spapr->fdt_size;
> +spapr->fdt_blob = fdt;
> >>>
> >>> Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
> >>> both fdt_blob and fdt_size here.
> >>
> >> The device tree is built from the reset handler and the idea is that we
> >> want to always have some tree in the machine.
> >
> > Yes, I think the approach here is fine. Otherwise when we want to
> > look up the current fdt state in RTAS calls or whatever we'd always
> > have to do
> > if (fdt_blob)
> > look up that
> > else
> > look up qemu created fdt.
> >
> > Incidentally 'fdt' and 'fdt_blob' names do a terrible job of
> > distinguishing what the difference is. Renaming fdt to fdt_initial
> > (to match fdt_initial_size) and fdt_blob to fdt should make that
> > clearer.
>
> There is just one fdt in sPAPRMachineState - it is fdt_blob as it is
> flattened. The "fdt" symbol above is local to spapr_machine_reset() and
> when the tree is built - it is stored in fdt_blob.
Uh, sorry, I misread. I'll look more carefully at the next spin.
--
David Gibson| I'll have my
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On Tue, Dec 11, 2018 at 10:55:59AM +0100, Greg Kurz wrote:
> On Tue, 11 Dec 2018 14:53:32 +1100
> Alexey Kardashevskiy wrote:
>
> > On 10/12/2018 20:30, Greg Kurz wrote:
> > > On Mon, 10 Dec 2018 17:20:43 +1100
> > > David Gibson wrote:
> > >
> > >> On Mon, Nov 12, 2018 at 03:12:26PM +1100, Alexey Kardashevskiy wrote:
> > >>>
> > >>>
> > >>> On 12/11/2018 05:10, Greg Kurz wrote:
> > Hi Alexey,
> >
> > Just a few remarks. See below.
> >
> > On Thu, 8 Nov 2018 12:44:06 +1100
> > Alexey Kardashevskiy wrote:
> >
> > > SLOF receives a device tree and updates it with various properties
> > > before switching to the guest kernel and QEMU is not aware of any
> > > changes
> > > made by SLOF. Since there is no real RTAS (QEMU implements it), it
> > > makes
> > > sense to pass the SLOF final device tree to QEMU to let it implement
> > > RTAS related tasks better, such as PCI host bus adapter hotplug.
> > >
> > > Specifially, now QEMU can find out the actual XICS phandle (for PHB
> > > hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> > > assisted NMI - FWNMI).
> > >
> > > This stores the initial DT blob in the sPAPR machine and replaces it
> > > in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> > >
> > > This adds an @update_dt_enabled machine property to allow backward
> > > migration.
> > >
> > > SLOF already has a hypercall since
> > > https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> > >
> > > Signed-off-by: Alexey Kardashevskiy
> > > ---
> > > include/hw/ppc/spapr.h | 7 ++-
> > > hw/ppc/spapr.c | 29 -
> > > hw/ppc/spapr_hcall.c | 32
> > > hw/ppc/trace-events| 2 ++
> > > 4 files changed, 68 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index ad4d7cfd97..f5dcaf44cb 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
> > >
> > > /*< public >*/
> > > bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of
> > > LMBs */
> > > +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> > > bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > > bool pre_2_10_has_unused_icps;
> > > bool legacy_irq_allocation;
> > > @@ -136,6 +137,9 @@ struct sPAPRMachineState {
> > > int vrma_adjust;
> > > ssize_t rtas_size;
> > > void *rtas_blob;
> > > +uint32_t fdt_size;
> > > +uint32_t fdt_initial_size;
> >
> > I don't quite see the purpose of fdt_initial_size... it seems to be
> > only
> > used to print a trace.
> > >>>
> > >>>
> > >>> Ah, lost in rebase. The purpose was to test if the new device tree has
> > >>> not grown too much.
> > >>>
> > >>>
> > >>>
> >
> > > +void *fdt_blob;
> > > long kernel_size;
> > > bool kernel_le;
> > > uint32_t initrd_base;
> > > @@ -462,7 +466,8 @@ struct sPAPRMachineState {
> > > #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> > > /* Client Architecture support */
> > > #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> > > -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> > > +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> > > +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
> > >
> > > typedef struct sPAPRDeviceTreeUpdateHeader {
> > > uint32_t version_id;
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index c08130facb..5e2d4d211c 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
> > > /* Load the fdt */
> > > qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > > cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> > > -g_free(fdt);
> > > +g_free(spapr->fdt_blob);
> > > +spapr->fdt_size = fdt_totalsize(fdt);
> > > +spapr->fdt_initial_size = spapr->fdt_size;
> > > +spapr->fdt_blob = fdt;
> >
> > Hmm... It looks weird to store state in a reset handler. I'd rather
> > zeroe
> > both fdt_blob and fdt_size here.
> > >>>
> > >>> The device tree is built from the reset handler and the idea is that we
> > >>> want to always have some tree in the machine.
> > >>
> > >> Yes, I think the approach here is fine. Otherwise when we want to
> > >> look up the current fdt state in RTAS calls or whatever we'd always
> > >> have to do
> > >> if (fdt_blob)
> > >> look up that
> > >> else
> > >> look up qemu created fdt.
> > >>
> > >
>
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On Tue, 11 Dec 2018 14:53:32 +1100
Alexey Kardashevskiy wrote:
> On 10/12/2018 20:30, Greg Kurz wrote:
> > On Mon, 10 Dec 2018 17:20:43 +1100
> > David Gibson wrote:
> >
> >> On Mon, Nov 12, 2018 at 03:12:26PM +1100, Alexey Kardashevskiy wrote:
> >>>
> >>>
> >>> On 12/11/2018 05:10, Greg Kurz wrote:
> Hi Alexey,
>
> Just a few remarks. See below.
>
> On Thu, 8 Nov 2018 12:44:06 +1100
> Alexey Kardashevskiy wrote:
>
> > SLOF receives a device tree and updates it with various properties
> > before switching to the guest kernel and QEMU is not aware of any
> > changes
> > made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> > sense to pass the SLOF final device tree to QEMU to let it implement
> > RTAS related tasks better, such as PCI host bus adapter hotplug.
> >
> > Specifially, now QEMU can find out the actual XICS phandle (for PHB
> > hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> > assisted NMI - FWNMI).
> >
> > This stores the initial DT blob in the sPAPR machine and replaces it
> > in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> >
> > This adds an @update_dt_enabled machine property to allow backward
> > migration.
> >
> > SLOF already has a hypercall since
> > https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> >
> > Signed-off-by: Alexey Kardashevskiy
> > ---
> > include/hw/ppc/spapr.h | 7 ++-
> > hw/ppc/spapr.c | 29 -
> > hw/ppc/spapr_hcall.c | 32
> > hw/ppc/trace-events| 2 ++
> > 4 files changed, 68 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index ad4d7cfd97..f5dcaf44cb 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
> >
> > /*< public >*/
> > bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of
> > LMBs */
> > +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> > bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > bool pre_2_10_has_unused_icps;
> > bool legacy_irq_allocation;
> > @@ -136,6 +137,9 @@ struct sPAPRMachineState {
> > int vrma_adjust;
> > ssize_t rtas_size;
> > void *rtas_blob;
> > +uint32_t fdt_size;
> > +uint32_t fdt_initial_size;
>
> I don't quite see the purpose of fdt_initial_size... it seems to be only
> used to print a trace.
> >>>
> >>>
> >>> Ah, lost in rebase. The purpose was to test if the new device tree has
> >>> not grown too much.
> >>>
> >>>
> >>>
>
> > +void *fdt_blob;
> > long kernel_size;
> > bool kernel_le;
> > uint32_t initrd_base;
> > @@ -462,7 +466,8 @@ struct sPAPRMachineState {
> > #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> > /* Client Architecture support */
> > #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> > -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> > +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> > +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
> >
> > typedef struct sPAPRDeviceTreeUpdateHeader {
> > uint32_t version_id;
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index c08130facb..5e2d4d211c 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
> > /* Load the fdt */
> > qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> > -g_free(fdt);
> > +g_free(spapr->fdt_blob);
> > +spapr->fdt_size = fdt_totalsize(fdt);
> > +spapr->fdt_initial_size = spapr->fdt_size;
> > +spapr->fdt_blob = fdt;
>
> Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
> both fdt_blob and fdt_size here.
> >>>
> >>> The device tree is built from the reset handler and the idea is that we
> >>> want to always have some tree in the machine.
> >>
> >> Yes, I think the approach here is fine. Otherwise when we want to
> >> look up the current fdt state in RTAS calls or whatever we'd always
> >> have to do
> >>if (fdt_blob)
> >>look up that
> >>else
> >>look up qemu created fdt.
> >>
> >
> > No. We only have one fdt blob: the initial one, I'd rather
> > call reset time one, or the updated one.
>
> There is one fdt in the machine, always. Either initial or from cas.
>
Yeah, reset time fdt is either the initial one, either cas... and I'm now
wandering what happens if migration occurs between cas that
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On 10/12/2018 20:30, Greg Kurz wrote:
> On Mon, 10 Dec 2018 17:20:43 +1100
> David Gibson wrote:
>
>> On Mon, Nov 12, 2018 at 03:12:26PM +1100, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 12/11/2018 05:10, Greg Kurz wrote:
Hi Alexey,
Just a few remarks. See below.
On Thu, 8 Nov 2018 12:44:06 +1100
Alexey Kardashevskiy wrote:
> SLOF receives a device tree and updates it with various properties
> before switching to the guest kernel and QEMU is not aware of any changes
> made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> sense to pass the SLOF final device tree to QEMU to let it implement
> RTAS related tasks better, such as PCI host bus adapter hotplug.
>
> Specifially, now QEMU can find out the actual XICS phandle (for PHB
> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> assisted NMI - FWNMI).
>
> This stores the initial DT blob in the sPAPR machine and replaces it
> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
>
> This adds an @update_dt_enabled machine property to allow backward
> migration.
>
> SLOF already has a hypercall since
> https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
>
> Signed-off-by: Alexey Kardashevskiy
> ---
> include/hw/ppc/spapr.h | 7 ++-
> hw/ppc/spapr.c | 29 -
> hw/ppc/spapr_hcall.c | 32
> hw/ppc/trace-events| 2 ++
> 4 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ad4d7cfd97..f5dcaf44cb 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
>
> /*< public >*/
> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of
> LMBs */
> +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> bool pre_2_10_has_unused_icps;
> bool legacy_irq_allocation;
> @@ -136,6 +137,9 @@ struct sPAPRMachineState {
> int vrma_adjust;
> ssize_t rtas_size;
> void *rtas_blob;
> +uint32_t fdt_size;
> +uint32_t fdt_initial_size;
I don't quite see the purpose of fdt_initial_size... it seems to be only
used to print a trace.
>>>
>>>
>>> Ah, lost in rebase. The purpose was to test if the new device tree has
>>> not grown too much.
>>>
>>>
>>>
> +void *fdt_blob;
> long kernel_size;
> bool kernel_le;
> uint32_t initrd_base;
> @@ -462,7 +466,8 @@ struct sPAPRMachineState {
> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> /* Client Architecture support */
> #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
>
> typedef struct sPAPRDeviceTreeUpdateHeader {
> uint32_t version_id;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c08130facb..5e2d4d211c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
> /* Load the fdt */
> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> -g_free(fdt);
> +g_free(spapr->fdt_blob);
> +spapr->fdt_size = fdt_totalsize(fdt);
> +spapr->fdt_initial_size = spapr->fdt_size;
> +spapr->fdt_blob = fdt;
Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
both fdt_blob and fdt_size here.
>>>
>>> The device tree is built from the reset handler and the idea is that we
>>> want to always have some tree in the machine.
>>
>> Yes, I think the approach here is fine. Otherwise when we want to
>> look up the current fdt state in RTAS calls or whatever we'd always
>> have to do
>> if (fdt_blob)
>> look up that
>> else
>> look up qemu created fdt.
>>
>
> No. We only have one fdt blob: the initial one, I'd rather
> call reset time one, or the updated one.
There is one fdt in the machine, always. Either initial or from cas.
>> Incidentally 'fdt' and 'fdt_blob' names do a terrible job of
>> distinguishing what the difference is. Renaming fdt to fdt_initial
>> (to match fdt_initial_size) and fdt_blob to fdt should make that
>> clearer.
>>
>
> As mentioned earlier in this thread, spapr->fdt_initial_size is only used
> for tracing if the received fdt blob fails fdt_check_full()...
>
> $ git grep -H fdt_initial_size
> hw/ppc/spapr.c:spapr->fdt_initial_size = spapr->fdt_size;
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On 10/12/2018 17:20, David Gibson wrote:
> On Mon, Nov 12, 2018 at 03:12:26PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> On 12/11/2018 05:10, Greg Kurz wrote:
>>> Hi Alexey,
>>>
>>> Just a few remarks. See below.
>>>
>>> On Thu, 8 Nov 2018 12:44:06 +1100
>>> Alexey Kardashevskiy wrote:
>>>
SLOF receives a device tree and updates it with various properties
before switching to the guest kernel and QEMU is not aware of any changes
made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
sense to pass the SLOF final device tree to QEMU to let it implement
RTAS related tasks better, such as PCI host bus adapter hotplug.
Specifially, now QEMU can find out the actual XICS phandle (for PHB
hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
assisted NMI - FWNMI).
This stores the initial DT blob in the sPAPR machine and replaces it
in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
This adds an @update_dt_enabled machine property to allow backward
migration.
SLOF already has a hypercall since
https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
Signed-off-by: Alexey Kardashevskiy
---
include/hw/ppc/spapr.h | 7 ++-
hw/ppc/spapr.c | 29 -
hw/ppc/spapr_hcall.c | 32
hw/ppc/trace-events| 2 ++
4 files changed, 68 insertions(+), 2 deletions(-)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index ad4d7cfd97..f5dcaf44cb 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -100,6 +100,7 @@ struct sPAPRMachineClass {
/*< public >*/
bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs
*/
+bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
bool pre_2_10_has_unused_icps;
bool legacy_irq_allocation;
@@ -136,6 +137,9 @@ struct sPAPRMachineState {
int vrma_adjust;
ssize_t rtas_size;
void *rtas_blob;
+uint32_t fdt_size;
+uint32_t fdt_initial_size;
>>>
>>> I don't quite see the purpose of fdt_initial_size... it seems to be only
>>> used to print a trace.
>>
>>
>> Ah, lost in rebase. The purpose was to test if the new device tree has
>> not grown too much.
>>
>>
>>
>>>
+void *fdt_blob;
long kernel_size;
bool kernel_le;
uint32_t initrd_base;
@@ -462,7 +466,8 @@ struct sPAPRMachineState {
#define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
/* Client Architecture support */
#define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
-#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
+#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
+#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
typedef struct sPAPRDeviceTreeUpdateHeader {
uint32_t version_id;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c08130facb..5e2d4d211c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
/* Load the fdt */
qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
-g_free(fdt);
+g_free(spapr->fdt_blob);
+spapr->fdt_size = fdt_totalsize(fdt);
+spapr->fdt_initial_size = spapr->fdt_size;
+spapr->fdt_blob = fdt;
>>>
>>> Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
>>> both fdt_blob and fdt_size here.
>>
>> The device tree is built from the reset handler and the idea is that we
>> want to always have some tree in the machine.
>
> Yes, I think the approach here is fine. Otherwise when we want to
> look up the current fdt state in RTAS calls or whatever we'd always
> have to do
> if (fdt_blob)
> look up that
> else
> look up qemu created fdt.
>
> Incidentally 'fdt' and 'fdt_blob' names do a terrible job of
> distinguishing what the difference is. Renaming fdt to fdt_initial
> (to match fdt_initial_size) and fdt_blob to fdt should make that
> clearer.
There is just one fdt in sPAPRMachineState - it is fdt_blob as it is
flattened. The "fdt" symbol above is local to spapr_machine_reset() and
when the tree is built - it is stored in fdt_blob.
--
Alexey
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On Mon, 10 Dec 2018 17:20:43 +1100
David Gibson wrote:
> On Mon, Nov 12, 2018 at 03:12:26PM +1100, Alexey Kardashevskiy wrote:
> >
> >
> > On 12/11/2018 05:10, Greg Kurz wrote:
> > > Hi Alexey,
> > >
> > > Just a few remarks. See below.
> > >
> > > On Thu, 8 Nov 2018 12:44:06 +1100
> > > Alexey Kardashevskiy wrote:
> > >
> > >> SLOF receives a device tree and updates it with various properties
> > >> before switching to the guest kernel and QEMU is not aware of any changes
> > >> made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> > >> sense to pass the SLOF final device tree to QEMU to let it implement
> > >> RTAS related tasks better, such as PCI host bus adapter hotplug.
> > >>
> > >> Specifially, now QEMU can find out the actual XICS phandle (for PHB
> > >> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> > >> assisted NMI - FWNMI).
> > >>
> > >> This stores the initial DT blob in the sPAPR machine and replaces it
> > >> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> > >>
> > >> This adds an @update_dt_enabled machine property to allow backward
> > >> migration.
> > >>
> > >> SLOF already has a hypercall since
> > >> https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> > >>
> > >> Signed-off-by: Alexey Kardashevskiy
> > >> ---
> > >> include/hw/ppc/spapr.h | 7 ++-
> > >> hw/ppc/spapr.c | 29 -
> > >> hw/ppc/spapr_hcall.c | 32
> > >> hw/ppc/trace-events| 2 ++
> > >> 4 files changed, 68 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > >> index ad4d7cfd97..f5dcaf44cb 100644
> > >> --- a/include/hw/ppc/spapr.h
> > >> +++ b/include/hw/ppc/spapr.h
> > >> @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
> > >>
> > >> /*< public >*/
> > >> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of
> > >> LMBs */
> > >> +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> > >> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > >> bool pre_2_10_has_unused_icps;
> > >> bool legacy_irq_allocation;
> > >> @@ -136,6 +137,9 @@ struct sPAPRMachineState {
> > >> int vrma_adjust;
> > >> ssize_t rtas_size;
> > >> void *rtas_blob;
> > >> +uint32_t fdt_size;
> > >> +uint32_t fdt_initial_size;
> > >
> > > I don't quite see the purpose of fdt_initial_size... it seems to be only
> > > used to print a trace.
> >
> >
> > Ah, lost in rebase. The purpose was to test if the new device tree has
> > not grown too much.
> >
> >
> >
> > >
> > >> +void *fdt_blob;
> > >> long kernel_size;
> > >> bool kernel_le;
> > >> uint32_t initrd_base;
> > >> @@ -462,7 +466,8 @@ struct sPAPRMachineState {
> > >> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> > >> /* Client Architecture support */
> > >> #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> > >> -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> > >> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> > >> +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
> > >>
> > >> typedef struct sPAPRDeviceTreeUpdateHeader {
> > >> uint32_t version_id;
> > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > >> index c08130facb..5e2d4d211c 100644
> > >> --- a/hw/ppc/spapr.c
> > >> +++ b/hw/ppc/spapr.c
> > >> @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
> > >> /* Load the fdt */
> > >> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > >> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> > >> -g_free(fdt);
> > >> +g_free(spapr->fdt_blob);
> > >> +spapr->fdt_size = fdt_totalsize(fdt);
> > >> +spapr->fdt_initial_size = spapr->fdt_size;
> > >> +spapr->fdt_blob = fdt;
> > >
> > > Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
> > > both fdt_blob and fdt_size here.
> >
> > The device tree is built from the reset handler and the idea is that we
> > want to always have some tree in the machine.
>
> Yes, I think the approach here is fine. Otherwise when we want to
> look up the current fdt state in RTAS calls or whatever we'd always
> have to do
> if (fdt_blob)
> look up that
> else
> look up qemu created fdt.
>
No. We only have one fdt blob: the initial one, I'd rather
call reset time one, or the updated one.
> Incidentally 'fdt' and 'fdt_blob' names do a terrible job of
> distinguishing what the difference is. Renaming fdt to fdt_initial
> (to match fdt_initial_size) and fdt_blob to fdt should make that
> clearer.
>
As mentioned earlier in this thread, spapr->fdt_initial_size is only used
for tracing if the received fdt blob fails fdt_check_full()...
$ git grep -H fdt_initial_size
hw/ppc/spapr.c:spapr->fdt_initial_size = spapr->fdt_size;
hw/ppc/spapr.c:
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On Mon, Nov 12, 2018 at 03:12:26PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 12/11/2018 05:10, Greg Kurz wrote:
> > Hi Alexey,
> >
> > Just a few remarks. See below.
> >
> > On Thu, 8 Nov 2018 12:44:06 +1100
> > Alexey Kardashevskiy wrote:
> >
> >> SLOF receives a device tree and updates it with various properties
> >> before switching to the guest kernel and QEMU is not aware of any changes
> >> made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> >> sense to pass the SLOF final device tree to QEMU to let it implement
> >> RTAS related tasks better, such as PCI host bus adapter hotplug.
> >>
> >> Specifially, now QEMU can find out the actual XICS phandle (for PHB
> >> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> >> assisted NMI - FWNMI).
> >>
> >> This stores the initial DT blob in the sPAPR machine and replaces it
> >> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> >>
> >> This adds an @update_dt_enabled machine property to allow backward
> >> migration.
> >>
> >> SLOF already has a hypercall since
> >> https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> >>
> >> Signed-off-by: Alexey Kardashevskiy
> >> ---
> >> include/hw/ppc/spapr.h | 7 ++-
> >> hw/ppc/spapr.c | 29 -
> >> hw/ppc/spapr_hcall.c | 32
> >> hw/ppc/trace-events| 2 ++
> >> 4 files changed, 68 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index ad4d7cfd97..f5dcaf44cb 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
> >>
> >> /*< public >*/
> >> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs
> >> */
> >> +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> >> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> >> bool pre_2_10_has_unused_icps;
> >> bool legacy_irq_allocation;
> >> @@ -136,6 +137,9 @@ struct sPAPRMachineState {
> >> int vrma_adjust;
> >> ssize_t rtas_size;
> >> void *rtas_blob;
> >> +uint32_t fdt_size;
> >> +uint32_t fdt_initial_size;
> >
> > I don't quite see the purpose of fdt_initial_size... it seems to be only
> > used to print a trace.
>
>
> Ah, lost in rebase. The purpose was to test if the new device tree has
> not grown too much.
>
>
>
> >
> >> +void *fdt_blob;
> >> long kernel_size;
> >> bool kernel_le;
> >> uint32_t initrd_base;
> >> @@ -462,7 +466,8 @@ struct sPAPRMachineState {
> >> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> >> /* Client Architecture support */
> >> #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> >> -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> >> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> >> +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
> >>
> >> typedef struct sPAPRDeviceTreeUpdateHeader {
> >> uint32_t version_id;
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index c08130facb..5e2d4d211c 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
> >> /* Load the fdt */
> >> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> >> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> >> -g_free(fdt);
> >> +g_free(spapr->fdt_blob);
> >> +spapr->fdt_size = fdt_totalsize(fdt);
> >> +spapr->fdt_initial_size = spapr->fdt_size;
> >> +spapr->fdt_blob = fdt;
> >
> > Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
> > both fdt_blob and fdt_size here.
>
> The device tree is built from the reset handler and the idea is that we
> want to always have some tree in the machine.
Yes, I think the approach here is fine. Otherwise when we want to
look up the current fdt state in RTAS calls or whatever we'd always
have to do
if (fdt_blob)
look up that
else
look up qemu created fdt.
Incidentally 'fdt' and 'fdt_blob' names do a terrible job of
distinguishing what the difference is. Renaming fdt to fdt_initial
(to match fdt_initial_size) and fdt_blob to fdt should make that
clearer.
--
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
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On 12/11/2018 20:05, Greg Kurz wrote:
> On Mon, 12 Nov 2018 15:12:26 +1100
> Alexey Kardashevskiy wrote:
>
>> On 12/11/2018 05:10, Greg Kurz wrote:
>>> Hi Alexey,
>>>
>>> Just a few remarks. See below.
>>>
>>> On Thu, 8 Nov 2018 12:44:06 +1100
>>> Alexey Kardashevskiy wrote:
>>>
SLOF receives a device tree and updates it with various properties
before switching to the guest kernel and QEMU is not aware of any changes
made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
sense to pass the SLOF final device tree to QEMU to let it implement
RTAS related tasks better, such as PCI host bus adapter hotplug.
Specifially, now QEMU can find out the actual XICS phandle (for PHB
hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
assisted NMI - FWNMI).
This stores the initial DT blob in the sPAPR machine and replaces it
in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
This adds an @update_dt_enabled machine property to allow backward
migration.
SLOF already has a hypercall since
https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
Signed-off-by: Alexey Kardashevskiy
---
include/hw/ppc/spapr.h | 7 ++-
hw/ppc/spapr.c | 29 -
hw/ppc/spapr_hcall.c | 32
hw/ppc/trace-events| 2 ++
4 files changed, 68 insertions(+), 2 deletions(-)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index ad4d7cfd97..f5dcaf44cb 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -100,6 +100,7 @@ struct sPAPRMachineClass {
/*< public >*/
bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs
*/
+bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
bool pre_2_10_has_unused_icps;
bool legacy_irq_allocation;
@@ -136,6 +137,9 @@ struct sPAPRMachineState {
int vrma_adjust;
ssize_t rtas_size;
void *rtas_blob;
+uint32_t fdt_size;
+uint32_t fdt_initial_size;
>>>
>>> I don't quite see the purpose of fdt_initial_size... it seems to be only
>>> used to print a trace.
>>
>>
>> Ah, lost in rebase. The purpose was to test if the new device tree has
>> not grown too much.
>>
>
> Ok, makes sense during development.
>
>>
>>
>>>
+void *fdt_blob;
long kernel_size;
bool kernel_le;
uint32_t initrd_base;
@@ -462,7 +466,8 @@ struct sPAPRMachineState {
#define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
/* Client Architecture support */
#define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
-#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
+#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
+#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
typedef struct sPAPRDeviceTreeUpdateHeader {
uint32_t version_id;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c08130facb..5e2d4d211c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
/* Load the fdt */
qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
-g_free(fdt);
+g_free(spapr->fdt_blob);
+spapr->fdt_size = fdt_totalsize(fdt);
+spapr->fdt_initial_size = spapr->fdt_size;
+spapr->fdt_blob = fdt;
>>>
>>> Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
>>> both fdt_blob and fdt_size here.
>>
>>
>> The device tree is built from the reset handler and the idea is that we
>> want to always have some tree in the machine.
>>
>
> Yes of course, I forgot that we need to keep the fdt to be kept
> somewhere so that we can use it :). My remark has more to do
> with migration actually: the fdt built at reset time is supposed
> to derive from the command line and hot-(un)plugged devices, ie,
> identical in source and destination. This isn't state we should
> migrate IIUC.
Having some device tree all the time seems more convenient than managing
the state when we do have one and when we do not.
It is not a big deal though, I'd wait and see what David thinks. Thanks,
> Maybe add a boolean field that tells that the fdt was updated, use
> it in spapr_dtb_needed() and reset it in spapr_machine_reset() ?
>
>>
>>
>>>
/* Set up the entry state */
spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
@@ -1887,6 +1890,27 @@ static const VMStateDescription
vmstate_spapr_irq_map = {
},
};
+static bool spapr_dtb_needed(void *opaque)
+{
+sPAPRMachineClass
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On Mon, 12 Nov 2018 15:12:26 +1100
Alexey Kardashevskiy wrote:
> On 12/11/2018 05:10, Greg Kurz wrote:
> > Hi Alexey,
> >
> > Just a few remarks. See below.
> >
> > On Thu, 8 Nov 2018 12:44:06 +1100
> > Alexey Kardashevskiy wrote:
> >
> >> SLOF receives a device tree and updates it with various properties
> >> before switching to the guest kernel and QEMU is not aware of any changes
> >> made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> >> sense to pass the SLOF final device tree to QEMU to let it implement
> >> RTAS related tasks better, such as PCI host bus adapter hotplug.
> >>
> >> Specifially, now QEMU can find out the actual XICS phandle (for PHB
> >> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> >> assisted NMI - FWNMI).
> >>
> >> This stores the initial DT blob in the sPAPR machine and replaces it
> >> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> >>
> >> This adds an @update_dt_enabled machine property to allow backward
> >> migration.
> >>
> >> SLOF already has a hypercall since
> >> https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> >>
> >> Signed-off-by: Alexey Kardashevskiy
> >> ---
> >> include/hw/ppc/spapr.h | 7 ++-
> >> hw/ppc/spapr.c | 29 -
> >> hw/ppc/spapr_hcall.c | 32
> >> hw/ppc/trace-events| 2 ++
> >> 4 files changed, 68 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index ad4d7cfd97..f5dcaf44cb 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
> >>
> >> /*< public >*/
> >> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs
> >> */
> >> +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> >> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> >> bool pre_2_10_has_unused_icps;
> >> bool legacy_irq_allocation;
> >> @@ -136,6 +137,9 @@ struct sPAPRMachineState {
> >> int vrma_adjust;
> >> ssize_t rtas_size;
> >> void *rtas_blob;
> >> +uint32_t fdt_size;
> >> +uint32_t fdt_initial_size;
> >
> > I don't quite see the purpose of fdt_initial_size... it seems to be only
> > used to print a trace.
>
>
> Ah, lost in rebase. The purpose was to test if the new device tree has
> not grown too much.
>
Ok, makes sense during development.
>
>
> >
> >> +void *fdt_blob;
> >> long kernel_size;
> >> bool kernel_le;
> >> uint32_t initrd_base;
> >> @@ -462,7 +466,8 @@ struct sPAPRMachineState {
> >> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> >> /* Client Architecture support */
> >> #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> >> -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> >> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> >> +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
> >>
> >> typedef struct sPAPRDeviceTreeUpdateHeader {
> >> uint32_t version_id;
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index c08130facb..5e2d4d211c 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
> >> /* Load the fdt */
> >> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> >> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> >> -g_free(fdt);
> >> +g_free(spapr->fdt_blob);
> >> +spapr->fdt_size = fdt_totalsize(fdt);
> >> +spapr->fdt_initial_size = spapr->fdt_size;
> >> +spapr->fdt_blob = fdt;
> >
> > Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
> > both fdt_blob and fdt_size here.
>
>
> The device tree is built from the reset handler and the idea is that we
> want to always have some tree in the machine.
>
Yes of course, I forgot that we need to keep the fdt to be kept
somewhere so that we can use it :). My remark has more to do
with migration actually: the fdt built at reset time is supposed
to derive from the command line and hot-(un)plugged devices, ie,
identical in source and destination. This isn't state we should
migrate IIUC.
Maybe add a boolean field that tells that the fdt was updated, use
it in spapr_dtb_needed() and reset it in spapr_machine_reset() ?
>
>
> >
> >>
> >> /* Set up the entry state */
> >> spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
> >> @@ -1887,6 +1890,27 @@ static const VMStateDescription
> >> vmstate_spapr_irq_map = {
> >> },
> >> };
> >>
> >> +static bool spapr_dtb_needed(void *opaque)
> >> +{
> >> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
> >> +
> >> +return smc->update_dt_enabled;
> >
> > This means we always migrate the fdt, even if migration occurs before
> > SLOF could call KVMPPC_H_UPDATE_DT.
> >
> > With spapr->fdt_blob set to NULL on reset, a better check
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On 12/11/2018 05:10, Greg Kurz wrote:
> Hi Alexey,
>
> Just a few remarks. See below.
>
> On Thu, 8 Nov 2018 12:44:06 +1100
> Alexey Kardashevskiy wrote:
>
>> SLOF receives a device tree and updates it with various properties
>> before switching to the guest kernel and QEMU is not aware of any changes
>> made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
>> sense to pass the SLOF final device tree to QEMU to let it implement
>> RTAS related tasks better, such as PCI host bus adapter hotplug.
>>
>> Specifially, now QEMU can find out the actual XICS phandle (for PHB
>> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
>> assisted NMI - FWNMI).
>>
>> This stores the initial DT blob in the sPAPR machine and replaces it
>> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
>>
>> This adds an @update_dt_enabled machine property to allow backward
>> migration.
>>
>> SLOF already has a hypercall since
>> https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
>>
>> Signed-off-by: Alexey Kardashevskiy
>> ---
>> include/hw/ppc/spapr.h | 7 ++-
>> hw/ppc/spapr.c | 29 -
>> hw/ppc/spapr_hcall.c | 32
>> hw/ppc/trace-events| 2 ++
>> 4 files changed, 68 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index ad4d7cfd97..f5dcaf44cb 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
>>
>> /*< public >*/
>> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
>> +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
>> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
>> bool pre_2_10_has_unused_icps;
>> bool legacy_irq_allocation;
>> @@ -136,6 +137,9 @@ struct sPAPRMachineState {
>> int vrma_adjust;
>> ssize_t rtas_size;
>> void *rtas_blob;
>> +uint32_t fdt_size;
>> +uint32_t fdt_initial_size;
>
> I don't quite see the purpose of fdt_initial_size... it seems to be only
> used to print a trace.
Ah, lost in rebase. The purpose was to test if the new device tree has
not grown too much.
>
>> +void *fdt_blob;
>> long kernel_size;
>> bool kernel_le;
>> uint32_t initrd_base;
>> @@ -462,7 +466,8 @@ struct sPAPRMachineState {
>> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
>> /* Client Architecture support */
>> #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
>> -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
>> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
>> +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
>>
>> typedef struct sPAPRDeviceTreeUpdateHeader {
>> uint32_t version_id;
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index c08130facb..5e2d4d211c 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
>> /* Load the fdt */
>> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
>> -g_free(fdt);
>> +g_free(spapr->fdt_blob);
>> +spapr->fdt_size = fdt_totalsize(fdt);
>> +spapr->fdt_initial_size = spapr->fdt_size;
>> +spapr->fdt_blob = fdt;
>
> Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
> both fdt_blob and fdt_size here.
The device tree is built from the reset handler and the idea is that we
want to always have some tree in the machine.
>
>>
>> /* Set up the entry state */
>> spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
>> @@ -1887,6 +1890,27 @@ static const VMStateDescription vmstate_spapr_irq_map
>> = {
>> },
>> };
>>
>> +static bool spapr_dtb_needed(void *opaque)
>> +{
>> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
>> +
>> +return smc->update_dt_enabled;
>
> This means we always migrate the fdt, even if migration occurs before
> SLOF could call KVMPPC_H_UPDATE_DT.
>
> With spapr->fdt_blob set to NULL on reset, a better check would be:
>
> sPAPRMachineState *spapr = SPAPR_MACHINE(opaque);
>
> return smc->update_dt_enabled && spapr->fdt_blob;
>
>> +}
>> +
>> +static const VMStateDescription vmstate_spapr_dtb = {
>> +.name = "spapr_dtb",
>> +.version_id = 1,
>> +.minimum_version_id = 1,
>> +.needed = spapr_dtb_needed,
>> +.fields = (VMStateField[]) {
>> +VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
>> +VMSTATE_UINT32(fdt_size, sPAPRMachineState),
>> +VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0, NULL,
>> + fdt_size),
>> +VMSTATE_END_OF_LIST()
>> +},
>> +};
>> +
>> static const VMStateDescription vmstate_spapr = {
>> .name = "spapr",
>> .version_id = 3,
>> @@ -1915,6 +1939,7 @@ static const VMStateDescription
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
Hi Alexey,
Just a few remarks. See below.
On Thu, 8 Nov 2018 12:44:06 +1100
Alexey Kardashevskiy wrote:
> SLOF receives a device tree and updates it with various properties
> before switching to the guest kernel and QEMU is not aware of any changes
> made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> sense to pass the SLOF final device tree to QEMU to let it implement
> RTAS related tasks better, such as PCI host bus adapter hotplug.
>
> Specifially, now QEMU can find out the actual XICS phandle (for PHB
> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> assisted NMI - FWNMI).
>
> This stores the initial DT blob in the sPAPR machine and replaces it
> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
>
> This adds an @update_dt_enabled machine property to allow backward
> migration.
>
> SLOF already has a hypercall since
> https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
>
> Signed-off-by: Alexey Kardashevskiy
> ---
> include/hw/ppc/spapr.h | 7 ++-
> hw/ppc/spapr.c | 29 -
> hw/ppc/spapr_hcall.c | 32
> hw/ppc/trace-events| 2 ++
> 4 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ad4d7cfd97..f5dcaf44cb 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
>
> /*< public >*/
> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
> +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> bool pre_2_10_has_unused_icps;
> bool legacy_irq_allocation;
> @@ -136,6 +137,9 @@ struct sPAPRMachineState {
> int vrma_adjust;
> ssize_t rtas_size;
> void *rtas_blob;
> +uint32_t fdt_size;
> +uint32_t fdt_initial_size;
I don't quite see the purpose of fdt_initial_size... it seems to be only
used to print a trace.
> +void *fdt_blob;
> long kernel_size;
> bool kernel_le;
> uint32_t initrd_base;
> @@ -462,7 +466,8 @@ struct sPAPRMachineState {
> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> /* Client Architecture support */
> #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
>
> typedef struct sPAPRDeviceTreeUpdateHeader {
> uint32_t version_id;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c08130facb..5e2d4d211c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
> /* Load the fdt */
> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> -g_free(fdt);
> +g_free(spapr->fdt_blob);
> +spapr->fdt_size = fdt_totalsize(fdt);
> +spapr->fdt_initial_size = spapr->fdt_size;
> +spapr->fdt_blob = fdt;
Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
both fdt_blob and fdt_size here.
>
> /* Set up the entry state */
> spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
> @@ -1887,6 +1890,27 @@ static const VMStateDescription vmstate_spapr_irq_map
> = {
> },
> };
>
> +static bool spapr_dtb_needed(void *opaque)
> +{
> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
> +
> +return smc->update_dt_enabled;
This means we always migrate the fdt, even if migration occurs before
SLOF could call KVMPPC_H_UPDATE_DT.
With spapr->fdt_blob set to NULL on reset, a better check would be:
sPAPRMachineState *spapr = SPAPR_MACHINE(opaque);
return smc->update_dt_enabled && spapr->fdt_blob;
> +}
> +
> +static const VMStateDescription vmstate_spapr_dtb = {
> +.name = "spapr_dtb",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = spapr_dtb_needed,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
> +VMSTATE_UINT32(fdt_size, sPAPRMachineState),
> +VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0, NULL,
> + fdt_size),
> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
> static const VMStateDescription vmstate_spapr = {
> .name = "spapr",
> .version_id = 3,
> @@ -1915,6 +1939,7 @@ static const VMStateDescription vmstate_spapr = {
> _spapr_cap_sbbc,
> _spapr_cap_ibs,
> _spapr_irq_map,
> +_spapr_dtb,
This needs to be rebased.
<<<
_spapr_cap_nested_kvm_hv,
===
_spapr_dtb,
>>>
I'll try to find some time to respin the PHB hotplug series and I'll happily
give a try to this patch.
> NULL
> }
> };
> @@ -3849,6 +3874,7 @@ static void
