[v4] board-qemu: add private hcall to inform host on "phandle" update

Thomas,

What is this business with phandle replacements all about? I looked at the
history which says "SLOF has a different concept of phandles" but it does
not say what would break if SLOF kept using phandles from QEMU.

I applied the chunk below and could not see any difference in how SLOF or a
guest kernel behave (slof can boot from virtio-net, guest can use mellanox
iov vfio-pci device):



What am I missing here?


ps. the patch is correct assuming we still need the phandle replacement
trick :)


On 24/07/17 22:34, Greg Kurz wrote:
> The "interrupt-map" property in each PHB node references the "phandle"
> property of the "interrupt-controller" node. This is used by the guest
> OS to setup IRQs for any PCI device plugged into the PHB. QEMU sets this
> property to an arbitrary value in the flattened DT passed to SLOF.
> 
> Since commit 82954d4c1088, SLOF has some generic code to convert all
> references to any "phandle" property to a SLOF specific value.
> 
> This is is perfectly okay for coldplug devices, since the guest OS only
> sees the converted value in "interrupt-map". It is a problem though for
> hotplug devices. Since they don't go through SLOF, the guest OS receives
> the arbitrary value set by QEMU and fails to setup IRQs.
> 
> In order to support PHB hotplug, this patch introduces a new private
> hcall, which allows SLOF to tell QEMU that a "phandle" was converted
> from an old value to a new value.
> 
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v4: - fix bad stack usage in fdt-hv-update-phandle error path
> 
> v3: - drop "<> 0" before IF
> 
> v2: - use ?dup
>     - switch the order of the parameters of hv-update-phandle
>     - added stack comment to hv-update-phandle
> ---
>  board-qemu/slof/fdt.fs    |   14 ++++++++++++--
>  lib/libhvcall/hvcall.code |    7 +++++++
>  lib/libhvcall/hvcall.in   |    1 +
>  lib/libhvcall/libhvcall.h |    1 +
>  4 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
> index 851645eae299..a24e3449e7c1 100644
> --- a/board-qemu/slof/fdt.fs
> +++ b/board-qemu/slof/fdt.fs
> @@ -308,18 +308,28 @@ fdt-claim-reserve
>     3drop
>  ;
>  
> +\ Tell QEMU about the updated phandle:
> +: fdt-hv-update-phandle ( old new -- )
> +   hv-update-phandle ?dup IF
> +      \ Ignore hcall not implemented error, print error otherwise
> +      dup -2 <> IF ." HV-UPDATE-PHANDLE error: " . cr ELSE drop THEN
> +   THEN
> +;
> +
>  \ Replace one FDT phandle "val" with a OF1275 phandle "node" in the
>  \ whole tree:
>  : fdt-update-phandle ( val node -- )
>     >r
>     FALSE TO (fdt-phandle-replaced)
> -   r@ s" /" find-node               ( val node root )
> -   fdt-replace-all-phandles
> +   r@ 2dup s" /" find-node          ( val node val node root )
> +   fdt-replace-all-phandles         ( val node )
>     (fdt-phandle-replaced) IF
> +      fdt-hv-update-phandle
>        r@ set-node
>        s" phandle" delete-property
>        s" linux,phandle" delete-property
>     ELSE
> +      2drop
>        diagnostic-mode? IF
>           cr ." Warning: Did not replace phandle in " r@ node>path type cr
>        THEN
> diff --git a/lib/libhvcall/hvcall.code b/lib/libhvcall/hvcall.code
> index 0ff50f27e8a9..834974862ef5 100644
> --- a/lib/libhvcall/hvcall.code
> +++ b/lib/libhvcall/hvcall.code
> @@ -129,3 +129,10 @@ PRIM(check_X2d_and_X2d_patch_X2d_sc1)
>  
>  	patch_broken_sc1((void*)start, (void*)end, (void*)patch_ins);
>  MIRP
> +
> +// : hv-update-phandle ( old_phandle new_phandle -- res )
> +PRIM(hv_X2d_update_X2d_phandle)
> +	uint32_t new_phandle = TOS.u; POP;
> +	uint32_t old_phandle = TOS.u;
> +	TOS.u = hv_generic(KVMPPC_H_UPDATE_PHANDLE, old_phandle, new_phandle);
> +MIRP
> diff --git a/lib/libhvcall/hvcall.in b/lib/libhvcall/hvcall.in
> index 4437b77f001d..ab7513af8977 100644
> --- a/lib/libhvcall/hvcall.in
> +++ b/lib/libhvcall/hvcall.in
> @@ -31,4 +31,5 @@ cod(RX!)
>  cod(hv-logical-memop)
>  cod(hv-cas)
>  cod(hv-rtas-update)
> +cod(hv-update-phandle)
>  cod(get-print-version)
> diff --git a/lib/libhvcall/libhvcall.h b/lib/libhvcall/libhvcall.h
> index b2ea3f6bf944..5776a2b772f7 100644
> --- a/lib/libhvcall/libhvcall.h
> +++ b/lib/libhvcall/libhvcall.h
> @@ -25,6 +25,7 @@
>  /* Client Architecture support */
>  #define KVMPPC_H_CAS            (KVMPPC_HCALL_BASE + 0x2)
>  #define KVMPPC_H_RTAS_UPDATE    (KVMPPC_HCALL_BASE + 0x3)
> +#define KVMPPC_H_UPDATE_PHANDLE (KVMPPC_HCALL_BASE + 0x4)
>  
>  #ifndef __ASSEMBLY__
>  
> 
> _______________________________________________
> SLOF mailing list
> SLOF@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/slof
>

On 25.07.2017 07:37, Alexey Kardashevskiy wrote:
> Thomas,
> 
> What is this business with phandle replacements all about? I looked at the
> history which says "SLOF has a different concept of phandles" but it does
> not say what would break if SLOF kept using phandles from QEMU.
> 
> I applied the chunk below and could not see any difference in how SLOF or a
> guest kernel behave (slof can boot from virtio-net, guest can use mellanox
> iov vfio-pci device):
> 
> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
> index a24e344..54a4bf8 100644
> --- a/board-qemu/slof/fdt.fs
> +++ b/board-qemu/slof/fdt.fs
> @@ -449,4 +449,4 @@ r> drop
>      fdt-cas-fix?
>  ;
> 
> -s" /" find-node fdt-fix-phandles
> +\ s" /" find-node fdt-fix-phandles
> 
> What am I missing here?

QEMU uses the FDT concept of phandles, i.e. it puts a "phandle" and
"linux,phandle" property into the corresponding device tree nodes. SLOF
uses the concept of Open Firmware phandles, i.e. the normal code does
not know about these properties and uses its own way of managing
phandles - which are pointer to data structures here. So if you leave
the properties in the device tree, you present Linux with a device tree
where a node suddenly has two different phandles - the FDT phandle from
QEMU and the Open Firmware phandle from SLOF. Not sure how Linux reacts
to such a device tree ... it could work by accident, but it could also
confuse the kernel completely. It's certainly wrong to do this, so I
think Greg's patch is the right way to go here, even if it looks a
little bit more complicated at a first glance.

 Thomas

On 25/07/17 16:14, Thomas Huth wrote:
> On 25.07.2017 07:37, Alexey Kardashevskiy wrote:
>> Thomas,
>>
>> What is this business with phandle replacements all about? I looked at the
>> history which says "SLOF has a different concept of phandles" but it does
>> not say what would break if SLOF kept using phandles from QEMU.
>>
>> I applied the chunk below and could not see any difference in how SLOF or a
>> guest kernel behave (slof can boot from virtio-net, guest can use mellanox
>> iov vfio-pci device):
>>
>> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
>> index a24e344..54a4bf8 100644
>> --- a/board-qemu/slof/fdt.fs
>> +++ b/board-qemu/slof/fdt.fs
>> @@ -449,4 +449,4 @@ r> drop
>>      fdt-cas-fix?
>>  ;
>>
>> -s" /" find-node fdt-fix-phandles
>> +\ s" /" find-node fdt-fix-phandles
>>
>> What am I missing here?
> 
> QEMU uses the FDT concept of phandles, i.e. it puts a "phandle" and
> "linux,phandle" property into the corresponding device tree nodes. SLOF
> uses the concept of Open Firmware phandles, i.e. the normal code does
> not know about these properties and uses its own way of managing
> phandles - which are pointer to data structures here. So if you leave
> the properties in the device tree, you present Linux with a device tree
> where a node suddenly has two different phandles - the FDT phandle from
> QEMU and the Open Firmware phandle from SLOF.

SLOF does not create phandle properties (or does it?), whatever QEMU
provides stays there, linux creates linux,phandle properties but if it is
already in the tree, it remains unchanged.

May be there is some use of these phandles like RTAS which I am not aware
of? Or the guest kernel assumes something about the nature of phandles
(unlikely)?


> Not sure how Linux reacts to such a device tree ...
> it could work by accident, but it could also
> confuse the kernel completely. It's certainly wrong to do this, so I
> think Greg's patch is the right way to go here, even if it looks a
> little bit more complicated at a first glance.

On 25/07/17 16:30, Alexey Kardashevskiy wrote:
> On 25/07/17 16:14, Thomas Huth wrote:
>> On 25.07.2017 07:37, Alexey Kardashevskiy wrote:
>>> Thomas,
>>>
>>> What is this business with phandle replacements all about? I looked at the
>>> history which says "SLOF has a different concept of phandles" but it does
>>> not say what would break if SLOF kept using phandles from QEMU.
>>>
>>> I applied the chunk below and could not see any difference in how SLOF or a
>>> guest kernel behave (slof can boot from virtio-net, guest can use mellanox
>>> iov vfio-pci device):
>>>
>>> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
>>> index a24e344..54a4bf8 100644
>>> --- a/board-qemu/slof/fdt.fs
>>> +++ b/board-qemu/slof/fdt.fs
>>> @@ -449,4 +449,4 @@ r> drop
>>>      fdt-cas-fix?
>>>  ;
>>>
>>> -s" /" find-node fdt-fix-phandles
>>> +\ s" /" find-node fdt-fix-phandles
>>>
>>> What am I missing here?
>>
>> QEMU uses the FDT concept of phandles, i.e. it puts a "phandle" and
>> "linux,phandle" property into the corresponding device tree nodes. SLOF
>> uses the concept of Open Firmware phandles, i.e. the normal code does
>> not know about these properties and uses its own way of managing
>> phandles - which are pointer to data structures here. So if you leave
>> the properties in the device tree, you present Linux with a device tree
>> where a node suddenly has two different phandles - the FDT phandle from
>> QEMU and the Open Firmware phandle from SLOF.
> 
> SLOF does not create phandle properties (or does it?), whatever QEMU
> provides stays there, linux creates linux,phandle properties but if it is
> already in the tree, it remains unchanged.
> 
> May be there is some use of these phandles like RTAS which I am not aware
> of? Or the guest kernel assumes something about the nature of phandles
> (unlikely)?


Ah, figured it out, it is all my ignorance :(

The guest's arch/powerpc/kernel/prom_init.c traverses the tree using
call_prom()  and this is where these SLOF's nodes come up so yes, we are
better off staying in sync with that.

> 
> 
>> Not sure how Linux reacts to such a device tree ...
>> it could work by accident, but it could also
>> confuse the kernel completely. It's certainly wrong to do this, so I
>> think Greg's patch is the right way to go here, even if it looks a
>> little bit more complicated at a first glance.