* [PATCH v4 1/3] qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event
2021-07-07 0:33 [PATCH v4 0/3] DEVICE_UNPLUG_ERROR QAPI event Daniel Henrique Barboza
@ 2021-07-07 0:33 ` Daniel Henrique Barboza
2021-07-07 9:26 ` Greg Kurz
2021-07-08 13:01 ` Markus Armbruster
2021-07-07 0:33 ` [PATCH v4 2/3] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors Daniel Henrique Barboza
` (2 subsequent siblings)
3 siblings, 2 replies; 18+ messages in thread
From: Daniel Henrique Barboza @ 2021-07-07 0:33 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel Henrique Barboza, armbru, groug, qemu-ppc, eblake, david
At this moment we only provide one event to report a hotunplug error,
MEM_UNPLUG_ERROR. As of Linux kernel 5.12 and QEMU 6.0.0, the pseries
machine is now able to report unplug errors for other device types, such
as CPUs.
Instead of creating a (device_type)_UNPLUG_ERROR for each new device,
create a generic DEVICE_UNPLUG_ERROR event that can be used by all
unplug errors in the future.
With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
docs/system/deprecated.rst | 10 ++++++++++
qapi/machine.json | 6 +++++-
qapi/qdev.json | 27 ++++++++++++++++++++++++++-
3 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 70e08baff6..ca6c7f9d43 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -204,6 +204,16 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
(the ISA has never been upstreamed to a compiler toolchain). Therefore
this CPU is also deprecated.
+
+QEMU API (QAPI) events
+----------------------
+
+``MEM_UNPLUG_ERROR`` (since 6.1)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Use the more generic event ``DEVICE_UNPLUG_ERROR`` instead.
+
+
System emulator machines
------------------------
diff --git a/qapi/machine.json b/qapi/machine.json
index c3210ee1fb..a595c753d2 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1271,6 +1271,9 @@
#
# @msg: Informative message
#
+# Features:
+# @deprecated: This event is deprecated. Use @DEVICE_UNPLUG_ERROR instead.
+#
# Since: 2.4
#
# Example:
@@ -1283,7 +1286,8 @@
#
##
{ 'event': 'MEM_UNPLUG_ERROR',
- 'data': { 'device': 'str', 'msg': 'str' } }
+ 'data': { 'device': 'str', 'msg': 'str' },
+ 'features': ['deprecated'] }
##
# @SMPConfiguration:
diff --git a/qapi/qdev.json b/qapi/qdev.json
index b83178220b..349d7439fa 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -84,7 +84,9 @@
# This command merely requests that the guest begin the hot removal
# process. Completion of the device removal process is signaled with a
# DEVICE_DELETED event. Guest reset will automatically complete removal
-# for all devices.
+# for all devices. If an error in the hot removal process is detected,
+# the device will not be removed and a DEVICE_UNPLUG_ERROR event is
+# sent.
#
# Since: 0.14
#
@@ -124,3 +126,26 @@
##
{ 'event': 'DEVICE_DELETED',
'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @DEVICE_UNPLUG_ERROR:
+#
+# Emitted when a device hot unplug error occurs.
+#
+# @device: device name
+#
+# @msg: Informative message
+#
+# Since: 6.1
+#
+# Example:
+#
+# <- { "event": "DEVICE_UNPLUG_ERROR"
+# "data": { "device": "dimm1",
+# "msg": "Memory hotunplug rejected by the guest for device dimm1"
+# },
+# "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
+#
+##
+{ 'event': 'DEVICE_UNPLUG_ERROR',
+ 'data': { 'device': 'str', 'msg': 'str' } }
--
2.31.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 1/3] qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event
2021-07-07 0:33 ` [PATCH v4 1/3] qapi/qdev.json: add " Daniel Henrique Barboza
@ 2021-07-07 9:26 ` Greg Kurz
2021-07-08 13:01 ` Markus Armbruster
1 sibling, 0 replies; 18+ messages in thread
From: Greg Kurz @ 2021-07-07 9:26 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: armbru, qemu-ppc, qemu-devel, eblake, david
On Tue, 6 Jul 2021 21:33:12 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> At this moment we only provide one event to report a hotunplug error,
> MEM_UNPLUG_ERROR. As of Linux kernel 5.12 and QEMU 6.0.0, the pseries
> machine is now able to report unplug errors for other device types, such
> as CPUs.
>
> Instead of creating a (device_type)_UNPLUG_ERROR for each new device,
> create a generic DEVICE_UNPLUG_ERROR event that can be used by all
> unplug errors in the future.
>
> With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> docs/system/deprecated.rst | 10 ++++++++++
> qapi/machine.json | 6 +++++-
> qapi/qdev.json | 27 ++++++++++++++++++++++++++-
> 3 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 70e08baff6..ca6c7f9d43 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -204,6 +204,16 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
> (the ISA has never been upstreamed to a compiler toolchain). Therefore
> this CPU is also deprecated.
>
> +
> +QEMU API (QAPI) events
> +----------------------
> +
> +``MEM_UNPLUG_ERROR`` (since 6.1)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Use the more generic event ``DEVICE_UNPLUG_ERROR`` instead.
> +
> +
> System emulator machines
> ------------------------
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index c3210ee1fb..a595c753d2 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1271,6 +1271,9 @@
> #
> # @msg: Informative message
> #
> +# Features:
> +# @deprecated: This event is deprecated. Use @DEVICE_UNPLUG_ERROR instead.
> +#
> # Since: 2.4
> #
> # Example:
> @@ -1283,7 +1286,8 @@
> #
> ##
> { 'event': 'MEM_UNPLUG_ERROR',
> - 'data': { 'device': 'str', 'msg': 'str' } }
> + 'data': { 'device': 'str', 'msg': 'str' },
> + 'features': ['deprecated'] }
>
> ##
> # @SMPConfiguration:
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index b83178220b..349d7439fa 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -84,7 +84,9 @@
> # This command merely requests that the guest begin the hot removal
> # process. Completion of the device removal process is signaled with a
> # DEVICE_DELETED event. Guest reset will automatically complete removal
> -# for all devices.
> +# for all devices. If an error in the hot removal process is detected,
> +# the device will not be removed and a DEVICE_UNPLUG_ERROR event is
> +# sent.
> #
> # Since: 0.14
> #
> @@ -124,3 +126,26 @@
> ##
> { 'event': 'DEVICE_DELETED',
> 'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @DEVICE_UNPLUG_ERROR:
> +#
> +# Emitted when a device hot unplug error occurs.
> +#
> +# @device: device name
> +#
> +# @msg: Informative message
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# <- { "event": "DEVICE_UNPLUG_ERROR"
> +# "data": { "device": "dimm1",
> +# "msg": "Memory hotunplug rejected by the guest for device dimm1"
> +# },
> +# "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
> +#
> +##
> +{ 'event': 'DEVICE_UNPLUG_ERROR',
> + 'data': { 'device': 'str', 'msg': 'str' } }
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 1/3] qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event
2021-07-07 0:33 ` [PATCH v4 1/3] qapi/qdev.json: add " Daniel Henrique Barboza
2021-07-07 9:26 ` Greg Kurz
@ 2021-07-08 13:01 ` Markus Armbruster
2021-07-08 14:20 ` Daniel Henrique Barboza
2021-07-12 2:26 ` David Gibson
1 sibling, 2 replies; 18+ messages in thread
From: Markus Armbruster @ 2021-07-08 13:01 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: eblake, groug, qemu-ppc, qemu-devel, david
Daniel Henrique Barboza <danielhb413@gmail.com> writes:
> At this moment we only provide one event to report a hotunplug error,
> MEM_UNPLUG_ERROR. As of Linux kernel 5.12 and QEMU 6.0.0, the pseries
> machine is now able to report unplug errors for other device types, such
> as CPUs.
>
> Instead of creating a (device_type)_UNPLUG_ERROR for each new device,
> create a generic DEVICE_UNPLUG_ERROR event that can be used by all
> unplug errors in the future.
>
> With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> docs/system/deprecated.rst | 10 ++++++++++
> qapi/machine.json | 6 +++++-
> qapi/qdev.json | 27 ++++++++++++++++++++++++++-
> 3 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 70e08baff6..ca6c7f9d43 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -204,6 +204,16 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
> (the ISA has never been upstreamed to a compiler toolchain). Therefore
> this CPU is also deprecated.
>
> +
> +QEMU API (QAPI) events
> +----------------------
> +
> +``MEM_UNPLUG_ERROR`` (since 6.1)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Use the more generic event ``DEVICE_UNPLUG_ERROR`` instead.
> +
> +
> System emulator machines
> ------------------------
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index c3210ee1fb..a595c753d2 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1271,6 +1271,9 @@
> #
> # @msg: Informative message
> #
> +# Features:
> +# @deprecated: This event is deprecated. Use @DEVICE_UNPLUG_ERROR instead.
> +#
> # Since: 2.4
> #
> # Example:
> @@ -1283,7 +1286,8 @@
> #
> ##
> { 'event': 'MEM_UNPLUG_ERROR',
> - 'data': { 'device': 'str', 'msg': 'str' } }
> + 'data': { 'device': 'str', 'msg': 'str' },
> + 'features': ['deprecated'] }
>
> ##
> # @SMPConfiguration:
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index b83178220b..349d7439fa 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -84,7 +84,9 @@
> # This command merely requests that the guest begin the hot removal
> # process. Completion of the device removal process is signaled with a
> # DEVICE_DELETED event. Guest reset will automatically complete removal
> -# for all devices.
> +# for all devices. If an error in the hot removal process is detected,
> +# the device will not be removed and a DEVICE_UNPLUG_ERROR event is
> +# sent.
"If an error ... is detected" kind of implies that some errors may go
undetected. Let's spell this out more clearly. Perhaps append "Some
errors cannot be detected."
DEVICE_UNPLUG_ERROR's unrelability is awkward. Best we can do in the
general case. Can we do better in special cases, and would it be
worthwhile? If yes, it should probably be done on top.
Two spaces between sentences for consistency with the existing text, please.
> #
> # Since: 0.14
> #
> @@ -124,3 +126,26 @@
> ##
> { 'event': 'DEVICE_DELETED',
> 'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @DEVICE_UNPLUG_ERROR:
> +#
> +# Emitted when a device hot unplug error occurs.
> +#
> +# @device: device name
> +#
> +# @msg: Informative message
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# <- { "event": "DEVICE_UNPLUG_ERROR"
> +# "data": { "device": "dimm1",
> +# "msg": "Memory hotunplug rejected by the guest for device dimm1"
> +# },
> +# "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
> +#
> +##
> +{ 'event': 'DEVICE_UNPLUG_ERROR',
> + 'data': { 'device': 'str', 'msg': 'str' } }
Hmm.
DEVICE_DELETED provides the device ID if the device has one, and the QOM
path. Documentation is less than clear for both (not your patch's
fault).
DEVICE_UNPLUG_ERROR provides the device ID unconditionally, and doesn't
provide the QOM path. What if the device doesn't have a device ID?
I suspect DEVICE_UNPLUG_ERROR should match DEVICE_DELETED exactly.
Bonus (for me, not for you): improving the unclear documentation becomes
your patch's problem. Here's my attempt:
# @device: the device's ID if it has one
#
# @path: the device's path within the object model
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 1/3] qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event
2021-07-08 13:01 ` Markus Armbruster
@ 2021-07-08 14:20 ` Daniel Henrique Barboza
2021-07-12 2:26 ` David Gibson
1 sibling, 0 replies; 18+ messages in thread
From: Daniel Henrique Barboza @ 2021-07-08 14:20 UTC (permalink / raw)
To: Markus Armbruster; +Cc: eblake, groug, qemu-ppc, qemu-devel, david
On 7/8/21 10:01 AM, Markus Armbruster wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>
>> At this moment we only provide one event to report a hotunplug error,
>> MEM_UNPLUG_ERROR. As of Linux kernel 5.12 and QEMU 6.0.0, the pseries
>> machine is now able to report unplug errors for other device types, such
>> as CPUs.
>>
>> Instead of creating a (device_type)_UNPLUG_ERROR for each new device,
>> create a generic DEVICE_UNPLUG_ERROR event that can be used by all
>> unplug errors in the future.
>>
>> With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
>>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> docs/system/deprecated.rst | 10 ++++++++++
>> qapi/machine.json | 6 +++++-
>> qapi/qdev.json | 27 ++++++++++++++++++++++++++-
>> 3 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
>> index 70e08baff6..ca6c7f9d43 100644
>> --- a/docs/system/deprecated.rst
>> +++ b/docs/system/deprecated.rst
>> @@ -204,6 +204,16 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
>> (the ISA has never been upstreamed to a compiler toolchain). Therefore
>> this CPU is also deprecated.
>>
>> +
>> +QEMU API (QAPI) events
>> +----------------------
>> +
>> +``MEM_UNPLUG_ERROR`` (since 6.1)
>> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''
>> +
>> +Use the more generic event ``DEVICE_UNPLUG_ERROR`` instead.
>> +
>> +
>> System emulator machines
>> ------------------------
>>
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index c3210ee1fb..a595c753d2 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -1271,6 +1271,9 @@
>> #
>> # @msg: Informative message
>> #
>> +# Features:
>> +# @deprecated: This event is deprecated. Use @DEVICE_UNPLUG_ERROR instead.
>> +#
>> # Since: 2.4
>> #
>> # Example:
>> @@ -1283,7 +1286,8 @@
>> #
>> ##
>> { 'event': 'MEM_UNPLUG_ERROR',
>> - 'data': { 'device': 'str', 'msg': 'str' } }
>> + 'data': { 'device': 'str', 'msg': 'str' },
>> + 'features': ['deprecated'] }
>>
>> ##
>> # @SMPConfiguration:
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index b83178220b..349d7439fa 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -84,7 +84,9 @@
>> # This command merely requests that the guest begin the hot removal
>> # process. Completion of the device removal process is signaled with a
>> # DEVICE_DELETED event. Guest reset will automatically complete removal
>> -# for all devices.
>> +# for all devices. If an error in the hot removal process is detected,
>> +# the device will not be removed and a DEVICE_UNPLUG_ERROR event is
>> +# sent.
>
> "If an error ... is detected" kind of implies that some errors may go
> undetected. Let's spell this out more clearly. Perhaps append "Some
> errors cannot be detected."
>
> DEVICE_UNPLUG_ERROR's unrelability is awkward. Best we can do in the
> general case. Can we do better in special cases, and would it be
> worthwhile? If yes, it should probably be done on top.
>
> Two spaces between sentences for consistency with the existing text, please.
Ok!
>
>> #
>> # Since: 0.14
>> #
>> @@ -124,3 +126,26 @@
>> ##
>> { 'event': 'DEVICE_DELETED',
>> 'data': { '*device': 'str', 'path': 'str' } }
>> +
>> +##
>> +# @DEVICE_UNPLUG_ERROR:
>> +#
>> +# Emitted when a device hot unplug error occurs.
>> +#
>> +# @device: device name
>> +#
>> +# @msg: Informative message
>> +#
>> +# Since: 6.1
>> +#
>> +# Example:
>> +#
>> +# <- { "event": "DEVICE_UNPLUG_ERROR"
>> +# "data": { "device": "dimm1",
>> +# "msg": "Memory hotunplug rejected by the guest for device dimm1"
>> +# },
>> +# "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
>> +#
>> +##
>> +{ 'event': 'DEVICE_UNPLUG_ERROR',
>> + 'data': { 'device': 'str', 'msg': 'str' } }
>
> Hmm.
>
> DEVICE_DELETED provides the device ID if the device has one, and the QOM
> path. Documentation is less than clear for both (not your patch's
> fault).
Now that you mentioned I realized that I was seeing both 'device' and 'path'
being propagated all this time in this event without noticing it. E.g.:
{"timestamp": {"seconds": 1625617532, "microseconds": 50228}, "event": "DEVICE_DELETED", "data": {"device": "core1", "path": "/machine/peripheral/core1"}}
>
> DEVICE_UNPLUG_ERROR provides the device ID unconditionally, and doesn't
> provide the QOM path. What if the device doesn't have a device ID?
>
> I suspect DEVICE_UNPLUG_ERROR should match DEVICE_DELETED exactly.
Agree. That will allow us to send DEVICE_UNPLUG_ERROR events even if dev->id is
NULL since we're also providing the path.
DEVICE_UNPLUG_ERROR was inspired by MEM_UNPLUG_ERROR since the usage was similar,
but I guess we're better of basing the new event on DEVICE_DELETED API instead.
This will also fix most of your inquiries in patches 2 and 3 as well.
I'll do the proper adjustments and re-send.
>
> Bonus (for me, not for you): improving the unclear documentation becomes
> your patch's problem. Here's my attempt:
>
> # @device: the device's ID if it has one
> #
> # @path: the device's path within the object model
>
I can make a pre-patch that add this information in DEVICE_DELETED documentation if
you prefer, instead of putting everything into the same patch (since the amended
DEVICE_DELETED docs are useful regardless of this work).
Thanks,
Daniel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 1/3] qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event
2021-07-08 13:01 ` Markus Armbruster
2021-07-08 14:20 ` Daniel Henrique Barboza
@ 2021-07-12 2:26 ` David Gibson
1 sibling, 0 replies; 18+ messages in thread
From: David Gibson @ 2021-07-12 2:26 UTC (permalink / raw)
To: Markus Armbruster
Cc: eblake, Daniel Henrique Barboza, qemu-ppc, qemu-devel, groug
[-- Attachment #1: Type: text/plain, Size: 3931 bytes --]
On Thu, Jul 08, 2021 at 03:01:20PM +0200, Markus Armbruster wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>
> > At this moment we only provide one event to report a hotunplug error,
> > MEM_UNPLUG_ERROR. As of Linux kernel 5.12 and QEMU 6.0.0, the pseries
> > machine is now able to report unplug errors for other device types, such
> > as CPUs.
> >
> > Instead of creating a (device_type)_UNPLUG_ERROR for each new device,
> > create a generic DEVICE_UNPLUG_ERROR event that can be used by all
> > unplug errors in the future.
> >
> > With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
> >
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> > docs/system/deprecated.rst | 10 ++++++++++
> > qapi/machine.json | 6 +++++-
> > qapi/qdev.json | 27 ++++++++++++++++++++++++++-
> > 3 files changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> > index 70e08baff6..ca6c7f9d43 100644
> > --- a/docs/system/deprecated.rst
> > +++ b/docs/system/deprecated.rst
> > @@ -204,6 +204,16 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
> > (the ISA has never been upstreamed to a compiler toolchain). Therefore
> > this CPU is also deprecated.
> >
> > +
> > +QEMU API (QAPI) events
> > +----------------------
> > +
> > +``MEM_UNPLUG_ERROR`` (since 6.1)
> > +''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> > +
> > +Use the more generic event ``DEVICE_UNPLUG_ERROR`` instead.
> > +
> > +
> > System emulator machines
> > ------------------------
> >
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index c3210ee1fb..a595c753d2 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1271,6 +1271,9 @@
> > #
> > # @msg: Informative message
> > #
> > +# Features:
> > +# @deprecated: This event is deprecated. Use @DEVICE_UNPLUG_ERROR instead.
> > +#
> > # Since: 2.4
> > #
> > # Example:
> > @@ -1283,7 +1286,8 @@
> > #
> > ##
> > { 'event': 'MEM_UNPLUG_ERROR',
> > - 'data': { 'device': 'str', 'msg': 'str' } }
> > + 'data': { 'device': 'str', 'msg': 'str' },
> > + 'features': ['deprecated'] }
> >
> > ##
> > # @SMPConfiguration:
> > diff --git a/qapi/qdev.json b/qapi/qdev.json
> > index b83178220b..349d7439fa 100644
> > --- a/qapi/qdev.json
> > +++ b/qapi/qdev.json
> > @@ -84,7 +84,9 @@
> > # This command merely requests that the guest begin the hot removal
> > # process. Completion of the device removal process is signaled with a
> > # DEVICE_DELETED event. Guest reset will automatically complete removal
> > -# for all devices.
> > +# for all devices. If an error in the hot removal process is detected,
> > +# the device will not be removed and a DEVICE_UNPLUG_ERROR event is
> > +# sent.
>
> "If an error ... is detected" kind of implies that some errors may go
> undetected. Let's spell this out more clearly. Perhaps append "Some
> errors cannot be detected."
>
> DEVICE_UNPLUG_ERROR's unrelability is awkward. Best we can do in the
> general case. Can we do better in special cases, and would it be
> worthwhile? If yes, it should probably be done on top.
I can't rule out such a special case entirely, but it's pretty hard to
imagine. If we need any kind of acknowledgement from the guest to
complete the unplug, then the unplug failing but the guest never
reporting anything is going to be indistinguishable from the guest
working on the unplug but being super slow.
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 2/3] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors
2021-07-07 0:33 [PATCH v4 0/3] DEVICE_UNPLUG_ERROR QAPI event Daniel Henrique Barboza
2021-07-07 0:33 ` [PATCH v4 1/3] qapi/qdev.json: add " Daniel Henrique Barboza
@ 2021-07-07 0:33 ` Daniel Henrique Barboza
2021-07-07 9:28 ` Greg Kurz
2021-07-08 13:08 ` Markus Armbruster
2021-07-07 0:33 ` [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write() Daniel Henrique Barboza
2021-07-08 0:49 ` [PATCH v4 0/3] DEVICE_UNPLUG_ERROR QAPI event David Gibson
3 siblings, 2 replies; 18+ messages in thread
From: Daniel Henrique Barboza @ 2021-07-07 0:33 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel Henrique Barboza, armbru, groug, qemu-ppc, eblake, david
Linux Kernel 5.12 is now unisolating CPU DRCs in the device_removal
error path, signalling that the hotunplug process wasn't successful.
This allow us to send a DEVICE_UNPLUG_ERROR in drc_unisolate_logical()
to signal this error to the management layer.
We also have another error path in spapr_memory_unplug_rollback() for
configured LMB DRCs. Kernels older than 5.13 will not unisolate the LMBs
in the hotunplug error path, but it will reconfigure them. Let's send
the DEVICE_UNPLUG_ERROR event in that code path as well to cover the
case of older kernels.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
hw/ppc/spapr.c | 8 ++++++++
hw/ppc/spapr_drc.c | 15 +++++++++------
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4dd90b75cc..fc071a1767 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -29,6 +29,7 @@
#include "qemu/datadir.h"
#include "qapi/error.h"
#include "qapi/qapi-events-machine.h"
+#include "qapi/qapi-events-qdev.h"
#include "qapi/visitor.h"
#include "sysemu/sysemu.h"
#include "sysemu/hostmem.h"
@@ -3641,7 +3642,14 @@ void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
*/
qapi_error = g_strdup_printf("Memory hotunplug rejected by the guest "
"for device %s", dev->id);
+
+ /*
+ * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
+ * while the deprecation of MEM_UNPLUG_ERROR is
+ * pending.
+ */
qapi_event_send_mem_unplug_error(dev->id, qapi_error);
+ qapi_event_send_device_unplug_error(dev->id, qapi_error);
}
/* Callback to be called during DRC release. */
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index a2f2634601..45a7b1aa16 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -17,6 +17,8 @@
#include "hw/ppc/spapr_drc.h"
#include "qom/object.h"
#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qapi/qapi-events-qdev.h"
#include "qapi/visitor.h"
#include "qemu/error-report.h"
#include "hw/ppc/spapr.h" /* for RTAS return codes */
@@ -160,6 +162,10 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
* means that the kernel is refusing the removal.
*/
if (drc->unplug_requested && drc->dev) {
+ const char qapi_error_fmt[] = "Device hotunplug rejected by the "
+ "guest for device %s";
+ g_autofree char *qapi_error = NULL;
+
if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
spapr = SPAPR_MACHINE(qdev_get_machine());
@@ -167,13 +173,10 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
}
drc->unplug_requested = false;
- error_report("Device hotunplug rejected by the guest "
- "for device %s", drc->dev->id);
+ error_report(qapi_error_fmt, drc->dev->id);
- /*
- * TODO: send a QAPI DEVICE_UNPLUG_ERROR event when
- * it is implemented.
- */
+ qapi_error = g_strdup_printf(qapi_error_fmt, drc->dev->id);
+ qapi_event_send_device_unplug_error(drc->dev->id, qapi_error);
}
return RTAS_OUT_SUCCESS; /* Nothing to do */
--
2.31.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/3] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors
2021-07-07 0:33 ` [PATCH v4 2/3] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors Daniel Henrique Barboza
@ 2021-07-07 9:28 ` Greg Kurz
2021-07-08 13:08 ` Markus Armbruster
1 sibling, 0 replies; 18+ messages in thread
From: Greg Kurz @ 2021-07-07 9:28 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: armbru, qemu-ppc, qemu-devel, eblake, david
On Tue, 6 Jul 2021 21:33:13 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> Linux Kernel 5.12 is now unisolating CPU DRCs in the device_removal
> error path, signalling that the hotunplug process wasn't successful.
> This allow us to send a DEVICE_UNPLUG_ERROR in drc_unisolate_logical()
> to signal this error to the management layer.
>
> We also have another error path in spapr_memory_unplug_rollback() for
> configured LMB DRCs. Kernels older than 5.13 will not unisolate the LMBs
> in the hotunplug error path, but it will reconfigure them. Let's send
> the DEVICE_UNPLUG_ERROR event in that code path as well to cover the
> case of older kernels.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/ppc/spapr.c | 8 ++++++++
> hw/ppc/spapr_drc.c | 15 +++++++++------
> 2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4dd90b75cc..fc071a1767 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -29,6 +29,7 @@
> #include "qemu/datadir.h"
> #include "qapi/error.h"
> #include "qapi/qapi-events-machine.h"
> +#include "qapi/qapi-events-qdev.h"
> #include "qapi/visitor.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/hostmem.h"
> @@ -3641,7 +3642,14 @@ void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
> */
> qapi_error = g_strdup_printf("Memory hotunplug rejected by the guest "
> "for device %s", dev->id);
> +
> + /*
> + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> + * while the deprecation of MEM_UNPLUG_ERROR is
> + * pending.
> + */
> qapi_event_send_mem_unplug_error(dev->id, qapi_error);
> + qapi_event_send_device_unplug_error(dev->id, qapi_error);
> }
>
> /* Callback to be called during DRC release. */
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a2f2634601..45a7b1aa16 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -17,6 +17,8 @@
> #include "hw/ppc/spapr_drc.h"
> #include "qom/object.h"
> #include "migration/vmstate.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-events-qdev.h"
> #include "qapi/visitor.h"
> #include "qemu/error-report.h"
> #include "hw/ppc/spapr.h" /* for RTAS return codes */
> @@ -160,6 +162,10 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
> * means that the kernel is refusing the removal.
> */
> if (drc->unplug_requested && drc->dev) {
> + const char qapi_error_fmt[] = "Device hotunplug rejected by the "
> + "guest for device %s";
> + g_autofree char *qapi_error = NULL;
> +
> if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> spapr = SPAPR_MACHINE(qdev_get_machine());
>
> @@ -167,13 +173,10 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
> }
>
> drc->unplug_requested = false;
> - error_report("Device hotunplug rejected by the guest "
> - "for device %s", drc->dev->id);
> + error_report(qapi_error_fmt, drc->dev->id);
>
> - /*
> - * TODO: send a QAPI DEVICE_UNPLUG_ERROR event when
> - * it is implemented.
> - */
> + qapi_error = g_strdup_printf(qapi_error_fmt, drc->dev->id);
> + qapi_event_send_device_unplug_error(drc->dev->id, qapi_error);
> }
>
> return RTAS_OUT_SUCCESS; /* Nothing to do */
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/3] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors
2021-07-07 0:33 ` [PATCH v4 2/3] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors Daniel Henrique Barboza
2021-07-07 9:28 ` Greg Kurz
@ 2021-07-08 13:08 ` Markus Armbruster
1 sibling, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2021-07-08 13:08 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: eblake, groug, qemu-ppc, qemu-devel, david
Daniel Henrique Barboza <danielhb413@gmail.com> writes:
> Linux Kernel 5.12 is now unisolating CPU DRCs in the device_removal
> error path, signalling that the hotunplug process wasn't successful.
> This allow us to send a DEVICE_UNPLUG_ERROR in drc_unisolate_logical()
> to signal this error to the management layer.
>
> We also have another error path in spapr_memory_unplug_rollback() for
> configured LMB DRCs. Kernels older than 5.13 will not unisolate the LMBs
> in the hotunplug error path, but it will reconfigure them. Let's send
> the DEVICE_UNPLUG_ERROR event in that code path as well to cover the
> case of older kernels.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/spapr.c | 8 ++++++++
> hw/ppc/spapr_drc.c | 15 +++++++++------
> 2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4dd90b75cc..fc071a1767 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -29,6 +29,7 @@
> #include "qemu/datadir.h"
> #include "qapi/error.h"
> #include "qapi/qapi-events-machine.h"
> +#include "qapi/qapi-events-qdev.h"
> #include "qapi/visitor.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/hostmem.h"
> @@ -3641,7 +3642,14 @@ void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
> */
> qapi_error = g_strdup_printf("Memory hotunplug rejected by the guest "
> "for device %s", dev->id);
> +
> + /*
> + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> + * while the deprecation of MEM_UNPLUG_ERROR is
> + * pending.
> + */
> qapi_event_send_mem_unplug_error(dev->id, qapi_error);
> + qapi_event_send_device_unplug_error(dev->id, qapi_error);
Can dev->id be null here?
Such devices exist, but maybe not here.
If dev->id can be null, we pass null to
qapi_event_send_device_unplug_error(), which is not okay. The output
visitor papers over by replacing with "". Let's not rely on this
misfeature.
> }
>
> /* Callback to be called during DRC release. */
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a2f2634601..45a7b1aa16 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -17,6 +17,8 @@
> #include "hw/ppc/spapr_drc.h"
> #include "qom/object.h"
> #include "migration/vmstate.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-events-qdev.h"
> #include "qapi/visitor.h"
> #include "qemu/error-report.h"
> #include "hw/ppc/spapr.h" /* for RTAS return codes */
> @@ -160,6 +162,10 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
> * means that the kernel is refusing the removal.
> */
> if (drc->unplug_requested && drc->dev) {
> + const char qapi_error_fmt[] = "Device hotunplug rejected by the "
> + "guest for device %s";
> + g_autofree char *qapi_error = NULL;
> +
> if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> spapr = SPAPR_MACHINE(qdev_get_machine());
>
> @@ -167,13 +173,10 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
> }
>
> drc->unplug_requested = false;
> - error_report("Device hotunplug rejected by the guest "
> - "for device %s", drc->dev->id);
> + error_report(qapi_error_fmt, drc->dev->id);
>
> - /*
> - * TODO: send a QAPI DEVICE_UNPLUG_ERROR event when
> - * it is implemented.
> - */
> + qapi_error = g_strdup_printf(qapi_error_fmt, drc->dev->id);
> + qapi_event_send_device_unplug_error(drc->dev->id, qapi_error);
> }
>
> return RTAS_OUT_SUCCESS; /* Nothing to do */
Likewise, only here we also pass it to g_strdup_printf() and possibly
vprintf() via error_report(). Null arguments to %s crash on some
systems. The issue predates your patch.
I'm not sure reporting failed unplug with error_report() is a good idea,
but it's not your patch's idea.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()
2021-07-07 0:33 [PATCH v4 0/3] DEVICE_UNPLUG_ERROR QAPI event Daniel Henrique Barboza
2021-07-07 0:33 ` [PATCH v4 1/3] qapi/qdev.json: add " Daniel Henrique Barboza
2021-07-07 0:33 ` [PATCH v4 2/3] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors Daniel Henrique Barboza
@ 2021-07-07 0:33 ` Daniel Henrique Barboza
2021-07-07 9:28 ` Greg Kurz
2021-07-08 13:08 ` Markus Armbruster
2021-07-08 0:49 ` [PATCH v4 0/3] DEVICE_UNPLUG_ERROR QAPI event David Gibson
3 siblings, 2 replies; 18+ messages in thread
From: Daniel Henrique Barboza @ 2021-07-07 0:33 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, Daniel Henrique Barboza, armbru, groug,
qemu-ppc, Igor Mammedov, eblake, david
MEM_UNPLUG_ERROR is deprecated since the introduction of
DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
MEM_UNPLUG_ERROR is pending.
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
hw/acpi/memory_hotplug.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index af37889423..fb9f4d2de7 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -8,6 +8,7 @@
#include "qapi/error.h"
#include "qapi/qapi-events-acpi.h"
#include "qapi/qapi-events-machine.h"
+#include "qapi/qapi-events-qdev.h"
#define MEMORY_SLOTS_NUMBER "MDNR"
#define MEMORY_HOTPLUG_IO_REGION "HPMR"
@@ -177,9 +178,17 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
/* call pc-dimm unplug cb */
hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
if (local_err) {
+ const char *error_pretty = error_get_pretty(local_err);
+
trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
- qapi_event_send_mem_unplug_error(dev->id,
- error_get_pretty(local_err));
+
+ /*
+ * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
+ * while the deprecation of MEM_UNPLUG_ERROR is
+ * pending.
+ */
+ qapi_event_send_mem_unplug_error(dev->id, error_pretty);
+ qapi_event_send_device_unplug_error(dev->id, error_pretty);
error_free(local_err);
break;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()
2021-07-07 0:33 ` [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write() Daniel Henrique Barboza
@ 2021-07-07 9:28 ` Greg Kurz
2021-07-08 13:08 ` Markus Armbruster
1 sibling, 0 replies; 18+ messages in thread
From: Greg Kurz @ 2021-07-07 9:28 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Michael S . Tsirkin, armbru, qemu-devel, qemu-ppc, Igor Mammedov,
eblake, david
On Tue, 6 Jul 2021 21:33:14 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> MEM_UNPLUG_ERROR is deprecated since the introduction of
> DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
> MEM_UNPLUG_ERROR is pending.
>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/acpi/memory_hotplug.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index af37889423..fb9f4d2de7 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -8,6 +8,7 @@
> #include "qapi/error.h"
> #include "qapi/qapi-events-acpi.h"
> #include "qapi/qapi-events-machine.h"
> +#include "qapi/qapi-events-qdev.h"
>
> #define MEMORY_SLOTS_NUMBER "MDNR"
> #define MEMORY_HOTPLUG_IO_REGION "HPMR"
> @@ -177,9 +178,17 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
> /* call pc-dimm unplug cb */
> hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
> if (local_err) {
> + const char *error_pretty = error_get_pretty(local_err);
> +
> trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
> - qapi_event_send_mem_unplug_error(dev->id,
> - error_get_pretty(local_err));
> +
> + /*
> + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> + * while the deprecation of MEM_UNPLUG_ERROR is
> + * pending.
> + */
> + qapi_event_send_mem_unplug_error(dev->id, error_pretty);
> + qapi_event_send_device_unplug_error(dev->id, error_pretty);
> error_free(local_err);
> break;
> }
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()
2021-07-07 0:33 ` [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write() Daniel Henrique Barboza
2021-07-07 9:28 ` Greg Kurz
@ 2021-07-08 13:08 ` Markus Armbruster
2021-07-09 8:39 ` Igor Mammedov
1 sibling, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2021-07-08 13:08 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Michael S . Tsirkin, groug, qemu-devel, qemu-ppc, Igor Mammedov,
eblake, david
Daniel Henrique Barboza <danielhb413@gmail.com> writes:
> MEM_UNPLUG_ERROR is deprecated since the introduction of
> DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
> MEM_UNPLUG_ERROR is pending.
>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/acpi/memory_hotplug.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index af37889423..fb9f4d2de7 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -8,6 +8,7 @@
> #include "qapi/error.h"
> #include "qapi/qapi-events-acpi.h"
> #include "qapi/qapi-events-machine.h"
> +#include "qapi/qapi-events-qdev.h"
>
> #define MEMORY_SLOTS_NUMBER "MDNR"
> #define MEMORY_HOTPLUG_IO_REGION "HPMR"
> @@ -177,9 +178,17 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
> /* call pc-dimm unplug cb */
> hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
> if (local_err) {
> + const char *error_pretty = error_get_pretty(local_err);
> +
> trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
> - qapi_event_send_mem_unplug_error(dev->id,
> - error_get_pretty(local_err));
> +
> + /*
> + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> + * while the deprecation of MEM_UNPLUG_ERROR is
> + * pending.
> + */
> + qapi_event_send_mem_unplug_error(dev->id, error_pretty);
> + qapi_event_send_device_unplug_error(dev->id, error_pretty);
> error_free(local_err);
> break;
> }
Same question as for PATCH 2: can dev->id be null?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()
2021-07-08 13:08 ` Markus Armbruster
@ 2021-07-09 8:39 ` Igor Mammedov
2021-07-09 11:25 ` Markus Armbruster
0 siblings, 1 reply; 18+ messages in thread
From: Igor Mammedov @ 2021-07-09 8:39 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael S . Tsirkin, Daniel Henrique Barboza, groug, qemu-devel,
qemu-ppc, eblake, david
On Thu, 08 Jul 2021 15:08:57 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>
> > MEM_UNPLUG_ERROR is deprecated since the introduction of
> > DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
> > MEM_UNPLUG_ERROR is pending.
> >
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > CC: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> > hw/acpi/memory_hotplug.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > index af37889423..fb9f4d2de7 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -8,6 +8,7 @@
> > #include "qapi/error.h"
> > #include "qapi/qapi-events-acpi.h"
> > #include "qapi/qapi-events-machine.h"
> > +#include "qapi/qapi-events-qdev.h"
> >
> > #define MEMORY_SLOTS_NUMBER "MDNR"
> > #define MEMORY_HOTPLUG_IO_REGION "HPMR"
> > @@ -177,9 +178,17 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
> > /* call pc-dimm unplug cb */
> > hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
> > if (local_err) {
> > + const char *error_pretty = error_get_pretty(local_err);
> > +
> > trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
> > - qapi_event_send_mem_unplug_error(dev->id,
> > - error_get_pretty(local_err));
> > +
> > + /*
> > + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> > + * while the deprecation of MEM_UNPLUG_ERROR is
> > + * pending.
> > + */
> > + qapi_event_send_mem_unplug_error(dev->id, error_pretty);
> > + qapi_event_send_device_unplug_error(dev->id, error_pretty);
> > error_free(local_err);
> > break;
> > }
>
> Same question as for PATCH 2: can dev->id be null?
only theoretically (if memory device were created directly without
using device_add), which as far as I know is not the case as all
memory devices are created using -device/device_add so far.
( for device_add case see qdev_device_add->qdev_set_id where
'id' is set to user provided or to generated "device[%d]" value)
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()
2021-07-09 8:39 ` Igor Mammedov
@ 2021-07-09 11:25 ` Markus Armbruster
2021-07-09 13:38 ` Igor Mammedov
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2021-07-09 11:25 UTC (permalink / raw)
To: Igor Mammedov
Cc: Michael S . Tsirkin, Daniel Henrique Barboza, groug, qemu-devel,
qemu-ppc, eblake, david
Igor Mammedov <imammedo@redhat.com> writes:
> On Thu, 08 Jul 2021 15:08:57 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>>
>> > MEM_UNPLUG_ERROR is deprecated since the introduction of
>> > DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
>> > MEM_UNPLUG_ERROR is pending.
>> >
>> > CC: Michael S. Tsirkin <mst@redhat.com>
>> > CC: Igor Mammedov <imammedo@redhat.com>
>> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> > ---
>> > hw/acpi/memory_hotplug.c | 13 +++++++++++--
>> > 1 file changed, 11 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> > index af37889423..fb9f4d2de7 100644
>> > --- a/hw/acpi/memory_hotplug.c
>> > +++ b/hw/acpi/memory_hotplug.c
>> > @@ -8,6 +8,7 @@
>> > #include "qapi/error.h"
>> > #include "qapi/qapi-events-acpi.h"
>> > #include "qapi/qapi-events-machine.h"
>> > +#include "qapi/qapi-events-qdev.h"
>> >
>> > #define MEMORY_SLOTS_NUMBER "MDNR"
>> > #define MEMORY_HOTPLUG_IO_REGION "HPMR"
>> > @@ -177,9 +178,17 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>> > /* call pc-dimm unplug cb */
>> > hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
>> > if (local_err) {
>> > + const char *error_pretty = error_get_pretty(local_err);
>> > +
>> > trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
>> > - qapi_event_send_mem_unplug_error(dev->id,
>> > - error_get_pretty(local_err));
>> > +
>> > + /*
>> > + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
>> > + * while the deprecation of MEM_UNPLUG_ERROR is
>> > + * pending.
>> > + */
>> > + qapi_event_send_mem_unplug_error(dev->id, error_pretty);
>> > + qapi_event_send_device_unplug_error(dev->id, error_pretty);
>> > error_free(local_err);
>> > break;
>> > }
>>
>> Same question as for PATCH 2: can dev->id be null?
> only theoretically (if memory device were created directly without
> using device_add), which as far as I know is not the case as all
> memory devices are created using -device/device_add so far.
>
> ( for device_add case see qdev_device_add->qdev_set_id where
> 'id' is set to user provided or to generated "device[%d]" value)
Something is set to a generated value, but it's not dev->id :)
void qdev_set_id(DeviceState *dev, const char *id)
@id is the value of id=... It may be null.
dev->id still is null here.
{
if (id) {
dev->id = id;
}
dev->id is now the value of id=... It may be null.
if (dev->id) {
object_property_add_child(qdev_get_peripheral(), dev->id,
OBJECT(dev));
If the user specified id=..., add @dev as child of /peripheral. The
child's name is the (non-null) value of id=...
} else {
static int anon_count;
gchar *name = g_strdup_printf("device[%d]", anon_count++);
object_property_add_child(qdev_get_peripheral_anon(), name,
OBJECT(dev));
g_free(name);
Else, add @dev as child of /peripheral-anon. The child's name is made
up.
}
}
dev->id is still the value of id=..., i.e. it may be null.
Sure dereferencing dev->id in acpi_memory_hotplug_write() is safe?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()
2021-07-09 11:25 ` Markus Armbruster
@ 2021-07-09 13:38 ` Igor Mammedov
2021-07-10 6:57 ` Markus Armbruster
0 siblings, 1 reply; 18+ messages in thread
From: Igor Mammedov @ 2021-07-09 13:38 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael S . Tsirkin, Daniel Henrique Barboza, groug, qemu-devel,
qemu-ppc, eblake, david
On Fri, 09 Jul 2021 13:25:43 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
> > On Thu, 08 Jul 2021 15:08:57 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
> >>
> >> > MEM_UNPLUG_ERROR is deprecated since the introduction of
> >> > DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
> >> > MEM_UNPLUG_ERROR is pending.
> >> >
> >> > CC: Michael S. Tsirkin <mst@redhat.com>
> >> > CC: Igor Mammedov <imammedo@redhat.com>
> >> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> >> > ---
> >> > hw/acpi/memory_hotplug.c | 13 +++++++++++--
> >> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> >> > index af37889423..fb9f4d2de7 100644
> >> > --- a/hw/acpi/memory_hotplug.c
> >> > +++ b/hw/acpi/memory_hotplug.c
> >> > @@ -8,6 +8,7 @@
> >> > #include "qapi/error.h"
> >> > #include "qapi/qapi-events-acpi.h"
> >> > #include "qapi/qapi-events-machine.h"
> >> > +#include "qapi/qapi-events-qdev.h"
> >> >
> >> > #define MEMORY_SLOTS_NUMBER "MDNR"
> >> > #define MEMORY_HOTPLUG_IO_REGION "HPMR"
> >> > @@ -177,9 +178,17 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
> >> > /* call pc-dimm unplug cb */
> >> > hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
> >> > if (local_err) {
> >> > + const char *error_pretty = error_get_pretty(local_err);
> >> > +
> >> > trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
> >> > - qapi_event_send_mem_unplug_error(dev->id,
> >> > - error_get_pretty(local_err));
> >> > +
> >> > + /*
> >> > + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> >> > + * while the deprecation of MEM_UNPLUG_ERROR is
> >> > + * pending.
> >> > + */
> >> > + qapi_event_send_mem_unplug_error(dev->id, error_pretty);
> >> > + qapi_event_send_device_unplug_error(dev->id, error_pretty);
> >> > error_free(local_err);
> >> > break;
> >> > }
> >>
> >> Same question as for PATCH 2: can dev->id be null?
> > only theoretically (if memory device were created directly without
> > using device_add), which as far as I know is not the case as all
> > memory devices are created using -device/device_add so far.
> >
> > ( for device_add case see qdev_device_add->qdev_set_id where
> > 'id' is set to user provided or to generated "device[%d]" value)
>
> Something is set to a generated value, but it's not dev->id :)
>
> void qdev_set_id(DeviceState *dev, const char *id)
>
> @id is the value of id=... It may be null.
>
> dev->id still is null here.
>
> {
> if (id) {
> dev->id = id;
> }
>
> dev->id is now the value of id=... It may be null.
>
> if (dev->id) {
> object_property_add_child(qdev_get_peripheral(), dev->id,
> OBJECT(dev));
>
> If the user specified id=..., add @dev as child of /peripheral. The
> child's name is the (non-null) value of id=...
>
> } else {
> static int anon_count;
> gchar *name = g_strdup_printf("device[%d]", anon_count++);
> object_property_add_child(qdev_get_peripheral_anon(), name,
> OBJECT(dev));
> g_free(name);
>
> Else, add @dev as child of /peripheral-anon. The child's name is made
> up.
>
>
> }
> }
>
> dev->id is still the value of id=..., i.e. it may be null.
yep, I was wrong and confused it child name in QOM tree.
> Sure dereferencing dev->id in acpi_memory_hotplug_write() is safe?
it aren't safe since guest may trigger this error when
memory-device is created without id.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()
2021-07-09 13:38 ` Igor Mammedov
@ 2021-07-10 6:57 ` Markus Armbruster
2021-07-11 8:49 ` Daniel Henrique Barboza
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2021-07-10 6:57 UTC (permalink / raw)
To: Igor Mammedov
Cc: Michael S . Tsirkin, Daniel Henrique Barboza, groug, qemu-devel,
qemu-ppc, eblake, david
Igor Mammedov <imammedo@redhat.com> writes:
> On Fri, 09 Jul 2021 13:25:43 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Igor Mammedov <imammedo@redhat.com> writes:
>>
>> > On Thu, 08 Jul 2021 15:08:57 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>> >>
>> >> > MEM_UNPLUG_ERROR is deprecated since the introduction of
>> >> > DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
>> >> > MEM_UNPLUG_ERROR is pending.
>> >> >
>> >> > CC: Michael S. Tsirkin <mst@redhat.com>
>> >> > CC: Igor Mammedov <imammedo@redhat.com>
>> >> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> >> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> >> > ---
>> >> > hw/acpi/memory_hotplug.c | 13 +++++++++++--
>> >> > 1 file changed, 11 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> >> > index af37889423..fb9f4d2de7 100644
>> >> > --- a/hw/acpi/memory_hotplug.c
>> >> > +++ b/hw/acpi/memory_hotplug.c
>> >> > @@ -8,6 +8,7 @@
>> >> > #include "qapi/error.h"
>> >> > #include "qapi/qapi-events-acpi.h"
>> >> > #include "qapi/qapi-events-machine.h"
>> >> > +#include "qapi/qapi-events-qdev.h"
>> >> >
>> >> > #define MEMORY_SLOTS_NUMBER "MDNR"
>> >> > #define MEMORY_HOTPLUG_IO_REGION "HPMR"
>> >> > @@ -177,9 +178,17 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>> >> > /* call pc-dimm unplug cb */
>> >> > hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
>> >> > if (local_err) {
>> >> > + const char *error_pretty = error_get_pretty(local_err);
>> >> > +
>> >> > trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
>> >> > - qapi_event_send_mem_unplug_error(dev->id,
>> >> > - error_get_pretty(local_err));
>> >> > +
>> >> > + /*
>> >> > + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
>> >> > + * while the deprecation of MEM_UNPLUG_ERROR is
>> >> > + * pending.
>> >> > + */
>> >> > + qapi_event_send_mem_unplug_error(dev->id, error_pretty);
>> >> > + qapi_event_send_device_unplug_error(dev->id, error_pretty);
>> >> > error_free(local_err);
>> >> > break;
>> >> > }
>> >>
>> >> Same question as for PATCH 2: can dev->id be null?
>> > only theoretically (if memory device were created directly without
>> > using device_add), which as far as I know is not the case as all
>> > memory devices are created using -device/device_add so far.
>> >
>> > ( for device_add case see qdev_device_add->qdev_set_id where
>> > 'id' is set to user provided or to generated "device[%d]" value)
>>
>> Something is set to a generated value, but it's not dev->id :)
>>
>> void qdev_set_id(DeviceState *dev, const char *id)
>>
>> @id is the value of id=... It may be null.
>>
>> dev->id still is null here.
>>
>> {
>> if (id) {
>> dev->id = id;
>> }
>>
>> dev->id is now the value of id=... It may be null.
>>
>> if (dev->id) {
>> object_property_add_child(qdev_get_peripheral(), dev->id,
>> OBJECT(dev));
>>
>> If the user specified id=..., add @dev as child of /peripheral. The
>> child's name is the (non-null) value of id=...
>>
>> } else {
>> static int anon_count;
>> gchar *name = g_strdup_printf("device[%d]", anon_count++);
>> object_property_add_child(qdev_get_peripheral_anon(), name,
>> OBJECT(dev));
>> g_free(name);
>>
>> Else, add @dev as child of /peripheral-anon. The child's name is made
>> up.
>>
>>
>> }
>> }
>>
>> dev->id is still the value of id=..., i.e. it may be null.
> yep, I was wrong and confused it child name in QOM tree.
>
>> Sure dereferencing dev->id in acpi_memory_hotplug_write() is safe?
>
> it aren't safe since guest may trigger this error when
> memory-device is created without id.
Thanks!
Daniel, the issue predates your series, but your series adds instances.
We need a patch fixing the existing instances before your series, and
fix up your series. Can you take care of that?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()
2021-07-10 6:57 ` Markus Armbruster
@ 2021-07-11 8:49 ` Daniel Henrique Barboza
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Henrique Barboza @ 2021-07-11 8:49 UTC (permalink / raw)
To: Markus Armbruster, Igor Mammedov
Cc: Michael S . Tsirkin, groug, qemu-devel, qemu-ppc, eblake, david
On 7/10/21 3:57 AM, Markus Armbruster wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
>> On Fri, 09 Jul 2021 13:25:43 +0200
>> Markus Armbruster <armbru@redhat.com> wrote:
>>
>>> Igor Mammedov <imammedo@redhat.com> writes:
>>>
>>>> On Thu, 08 Jul 2021 15:08:57 +0200
>>>> Markus Armbruster <armbru@redhat.com> wrote:
>>>>
>>>>> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>>>>>
>>>>>> MEM_UNPLUG_ERROR is deprecated since the introduction of
>>>>>> DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
>>>>>> MEM_UNPLUG_ERROR is pending.
>>>>>>
>>>>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>>>>> CC: Igor Mammedov <imammedo@redhat.com>
>>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>>> ---
>>>>>> hw/acpi/memory_hotplug.c | 13 +++++++++++--
>>>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>>>>>> index af37889423..fb9f4d2de7 100644
>>>>>> --- a/hw/acpi/memory_hotplug.c
>>>>>> +++ b/hw/acpi/memory_hotplug.c
>>>>>> @@ -8,6 +8,7 @@
>>>>>> #include "qapi/error.h"
>>>>>> #include "qapi/qapi-events-acpi.h"
>>>>>> #include "qapi/qapi-events-machine.h"
>>>>>> +#include "qapi/qapi-events-qdev.h"
>>>>>>
>>>>>> #define MEMORY_SLOTS_NUMBER "MDNR"
>>>>>> #define MEMORY_HOTPLUG_IO_REGION "HPMR"
>>>>>> @@ -177,9 +178,17 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>>>>>> /* call pc-dimm unplug cb */
>>>>>> hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
>>>>>> if (local_err) {
>>>>>> + const char *error_pretty = error_get_pretty(local_err);
>>>>>> +
>>>>>> trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
>>>>>> - qapi_event_send_mem_unplug_error(dev->id,
>>>>>> - error_get_pretty(local_err));
>>>>>> +
>>>>>> + /*
>>>>>> + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
>>>>>> + * while the deprecation of MEM_UNPLUG_ERROR is
>>>>>> + * pending.
>>>>>> + */
>>>>>> + qapi_event_send_mem_unplug_error(dev->id, error_pretty);
>>>>>> + qapi_event_send_device_unplug_error(dev->id, error_pretty);
>>>>>> error_free(local_err);
>>>>>> break;
>>>>>> }
>>>>>
>>>>> Same question as for PATCH 2: can dev->id be null?
>>>> only theoretically (if memory device were created directly without
>>>> using device_add), which as far as I know is not the case as all
>>>> memory devices are created using -device/device_add so far.
>>>>
>>>> ( for device_add case see qdev_device_add->qdev_set_id where
>>>> 'id' is set to user provided or to generated "device[%d]" value)
>>>
>>> Something is set to a generated value, but it's not dev->id :)
>>>
>>> void qdev_set_id(DeviceState *dev, const char *id)
>>>
>>> @id is the value of id=... It may be null.
>>>
>>> dev->id still is null here.
>>>
>>> {
>>> if (id) {
>>> dev->id = id;
>>> }
>>>
>>> dev->id is now the value of id=... It may be null.
>>>
>>> if (dev->id) {
>>> object_property_add_child(qdev_get_peripheral(), dev->id,
>>> OBJECT(dev));
>>>
>>> If the user specified id=..., add @dev as child of /peripheral. The
>>> child's name is the (non-null) value of id=...
>>>
>>> } else {
>>> static int anon_count;
>>> gchar *name = g_strdup_printf("device[%d]", anon_count++);
>>> object_property_add_child(qdev_get_peripheral_anon(), name,
>>> OBJECT(dev));
>>> g_free(name);
>>>
>>> Else, add @dev as child of /peripheral-anon. The child's name is made
>>> up.
>>>
>>>
>>> }
>>> }
>>>
>>> dev->id is still the value of id=..., i.e. it may be null.
>> yep, I was wrong and confused it child name in QOM tree.
>>
>>> Sure dereferencing dev->id in acpi_memory_hotplug_write() is safe?
>>
>> it aren't safe since guest may trigger this error when
>> memory-device is created without id.
>
> Thanks!
>
> Daniel, the issue predates your series, but your series adds instances.
> We need a patch fixing the existing instances before your series, and
> fix up your series. Can you take care of that?
Sure. I'll add a patch to handle the dev->id == NULL case before calling
qapi_event_send_mem_unplug_error() in acpi_memory_hotplug_write().
Daniel
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 0/3] DEVICE_UNPLUG_ERROR QAPI event
2021-07-07 0:33 [PATCH v4 0/3] DEVICE_UNPLUG_ERROR QAPI event Daniel Henrique Barboza
` (2 preceding siblings ...)
2021-07-07 0:33 ` [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write() Daniel Henrique Barboza
@ 2021-07-08 0:49 ` David Gibson
3 siblings, 0 replies; 18+ messages in thread
From: David Gibson @ 2021-07-08 0:49 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: armbru, qemu-ppc, qemu-devel, eblake, groug
[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]
On Tue, Jul 06, 2021 at 09:33:11PM -0300, Daniel Henrique Barboza wrote:
> Hi,
>
> This new version is rebased with current master (9aef0954195cc),
> hopefully an adequate format of patch 1, and David's R-b on all
> patches.
Markus do you want to take this, or will you ack and I'll take it
through the ppc tree?
If you want to take it then, for the ppc parts:
Acked-by: David Gibson <david@gibson.dropbear.id.au>
>
> changes from v3:
> - patch 1:
> * fixed format
> - all patches:
> * rebased with master
> * added David's R-b
> - v3 link: https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg05842.html
>
> changes from v2:
> - patch 1:
> * moved DEVICE_UNPLUG_ERROR declaration to qapi/qdev.json
> * updated 'device_del' description
> * added 'deprecated' notice on MEM_UNPLUG_ERROR
> * added MEM_UNPLUG_ERROR 'deprecated' info in docs/system/deprecated.rst
> - patch 2:
> * send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> - patch 3 (new):
> * send DEVICE_UNPLUG_ERROR in acpi/memory_hotplug.c
> - v2 link: https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg01304.html
>
> changes from v1:
> - former patches 1 and 2: dropped
> - patch 1 (former 3): changed the version to '6.1'
> - patch 2 (former 4): add a DEVICE_UNPLUG_ERROR event in the device
> unplug error path of CPUs and DIMMs
> - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg04682.html
>
> *** BLURB HERE ***
>
> Daniel Henrique Barboza (3):
> qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event
> spapr: use DEVICE_UNPLUG_ERROR to report unplug errors
> memory_hotplug.c: send DEVICE_UNPLUG_ERROR in
> acpi_memory_hotplug_write()
>
> docs/system/deprecated.rst | 10 ++++++++++
> hw/acpi/memory_hotplug.c | 13 +++++++++++--
> hw/ppc/spapr.c | 8 ++++++++
> hw/ppc/spapr_drc.c | 15 +++++++++------
> qapi/machine.json | 6 +++++-
> qapi/qdev.json | 27 ++++++++++++++++++++++++++-
> 6 files changed, 69 insertions(+), 10 deletions(-)
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread