[PATCH for-6.2] hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE

[PATCH for-6.2] hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE

[Thomas Huth]

Configuring a drive with "if=none" is meant for creation of a backend
only, it should not get automatically assigned to a device frontend.
Use "if=pflash" for the One-Time-Programmable device instead (like
it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).

Since the old way of configuring the device has already been published
with the previous QEMU versions, we cannot remove this immediately, but
have to deprecate it and support it for at least two more releases.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 docs/about/deprecated.rst | 6 ++++++
 hw/misc/sifive_u_otp.c    | 9 ++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index c03fcf951f..ff7488cb63 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -192,6 +192,12 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``.
 However, short-form booleans are deprecated and full explicit ``arg_name=on``
 form is preferred.
 
+``-drive if=none`` for the sifive_u OTP device (since 6.2)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Using ``-drive if=none`` to configure the OTP device of the sifive_u
+RISC-V machine is deprecated. Use ``-drive if=pflash`` instead.
+
 
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
index 18aa0bd55d..cf6098ff2c 100644
--- a/hw/misc/sifive_u_otp.c
+++ b/hw/misc/sifive_u_otp.c
@@ -209,7 +209,14 @@ static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
                           TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
 
-    dinfo = drive_get_next(IF_NONE);
+    dinfo = drive_get_next(IF_PFLASH);
+    if (!dinfo) {
+        dinfo = drive_get_next(IF_NONE);
+        if (dinfo) {
+            warn_report("using \"-drive if=none\" for the OTP is deprecated, "
+                        "use \"-drive if=pflash\" instead.");
+        }
+    }
     if (dinfo) {
         int ret;
         uint64_t perm;
-- 
2.27.0

Re: [PATCH for-6.2] hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE

[Philippe Mathieu-Daudé]

On 11/19/21 11:25, Thomas Huth wrote:
> Configuring a drive with "if=none" is meant for creation of a backend
> only, it should not get automatically assigned to a device frontend.
> Use "if=pflash" for the One-Time-Programmable device instead (like
> it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).
> 
> Since the old way of configuring the device has already been published
> with the previous QEMU versions, we cannot remove this immediately, but
> have to deprecate it and support it for at least two more releases.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  docs/about/deprecated.rst | 6 ++++++
>  hw/misc/sifive_u_otp.c    | 9 ++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)

> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
> index 18aa0bd55d..cf6098ff2c 100644
> --- a/hw/misc/sifive_u_otp.c
> +++ b/hw/misc/sifive_u_otp.c
> @@ -209,7 +209,14 @@ static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
>                            TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
>  
> -    dinfo = drive_get_next(IF_NONE);
> +    dinfo = drive_get_next(IF_PFLASH);
> +    if (!dinfo) {
> +        dinfo = drive_get_next(IF_NONE);

Isn't it a bug to call drive_get_next() from DeviceRealize()?

Shouldn't drive_get_next() be restricted to the MachineClass?

Re: [PATCH for-6.2] hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE

[Thomas Huth]

On 19/11/2021 11.40, Philippe Mathieu-Daudé wrote:
> On 11/19/21 11:25, Thomas Huth wrote:
>> Configuring a drive with "if=none" is meant for creation of a backend
>> only, it should not get automatically assigned to a device frontend.
>> Use "if=pflash" for the One-Time-Programmable device instead (like
>> it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).
>>
>> Since the old way of configuring the device has already been published
>> with the previous QEMU versions, we cannot remove this immediately, but
>> have to deprecate it and support it for at least two more releases.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   docs/about/deprecated.rst | 6 ++++++
>>   hw/misc/sifive_u_otp.c    | 9 ++++++++-
>>   2 files changed, 14 insertions(+), 1 deletion(-)
> 
>> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
>> index 18aa0bd55d..cf6098ff2c 100644
>> --- a/hw/misc/sifive_u_otp.c
>> +++ b/hw/misc/sifive_u_otp.c
>> @@ -209,7 +209,14 @@ static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
>>                             TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
>>       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
>>   
>> -    dinfo = drive_get_next(IF_NONE);
>> +    dinfo = drive_get_next(IF_PFLASH);
>> +    if (!dinfo) {
>> +        dinfo = drive_get_next(IF_NONE);
> 
> Isn't it a bug to call drive_get_next() from DeviceRealize()?
> 
> Shouldn't drive_get_next() be restricted to the MachineClass?

Yes, that would certainly be better - but considering that we are already 
past RC1 of the 6.2 release, I'd rather prefer to keep this patch rather as 
small as possible and do such refactorings during the next development cycle 
instead.

  Thomas

Re: [PATCH for-6.2] hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE

[Philippe Mathieu-Daudé]

On 11/19/21 12:02, Thomas Huth wrote:
> On 19/11/2021 11.40, Philippe Mathieu-Daudé wrote:
>> On 11/19/21 11:25, Thomas Huth wrote:
>>> Configuring a drive with "if=none" is meant for creation of a backend
>>> only, it should not get automatically assigned to a device frontend.
>>> Use "if=pflash" for the One-Time-Programmable device instead (like
>>> it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).
>>>
>>> Since the old way of configuring the device has already been published
>>> with the previous QEMU versions, we cannot remove this immediately, but
>>> have to deprecate it and support it for at least two more releases.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   docs/about/deprecated.rst | 6 ++++++
>>>   hw/misc/sifive_u_otp.c    | 9 ++++++++-
>>>   2 files changed, 14 insertions(+), 1 deletion(-)
>>
>>> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
>>> index 18aa0bd55d..cf6098ff2c 100644
>>> --- a/hw/misc/sifive_u_otp.c
>>> +++ b/hw/misc/sifive_u_otp.c
>>> @@ -209,7 +209,14 @@ static void sifive_u_otp_realize(DeviceState
>>> *dev, Error **errp)
>>>                             TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
>>>       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
>>>   -    dinfo = drive_get_next(IF_NONE);
>>> +    dinfo = drive_get_next(IF_PFLASH);
>>> +    if (!dinfo) {
>>> +        dinfo = drive_get_next(IF_NONE);
>>
>> Isn't it a bug to call drive_get_next() from DeviceRealize()?
>>
>> Shouldn't drive_get_next() be restricted to the MachineClass?
> 
> Yes, that would certainly be better - but considering that we are
> already past RC1 of the 6.2 release, I'd rather prefer to keep this
> patch rather as small as possible and do such refactorings during the
> next development cycle instead.

OK. For pflash:
Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH for-6.2] hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 19/11/2021 11.40, Philippe Mathieu-Daudé wrote:
>> On 11/19/21 11:25, Thomas Huth wrote:
>>> Configuring a drive with "if=none" is meant for creation of a backend
>>> only, it should not get automatically assigned to a device frontend.
>>> Use "if=pflash" for the One-Time-Programmable device instead (like
>>> it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).
>>>
>>> Since the old way of configuring the device has already been published
>>> with the previous QEMU versions, we cannot remove this immediately, but
>>> have to deprecate it and support it for at least two more releases.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   docs/about/deprecated.rst | 6 ++++++
>>>   hw/misc/sifive_u_otp.c    | 9 ++++++++-
>>>   2 files changed, 14 insertions(+), 1 deletion(-)
>> 
>>> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
>>> index 18aa0bd55d..cf6098ff2c 100644
>>> --- a/hw/misc/sifive_u_otp.c
>>> +++ b/hw/misc/sifive_u_otp.c
>>> @@ -209,7 +209,14 @@ static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
>>>                             TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
>>>       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
>>>   -    dinfo = drive_get_next(IF_NONE);
>>> +    dinfo = drive_get_next(IF_PFLASH);
>>> +    if (!dinfo) {
>>> +        dinfo = drive_get_next(IF_NONE);
>> 
>> Isn't it a bug to call drive_get_next() from DeviceRealize()?
>> Shouldn't drive_get_next() be restricted to the MachineClass?

drive_get_next() needs to die:

    Subject: [PATCH v2 00/13] Eliminate drive_get_next()
    Message-Id: <20211117163409.3587705-1-armbru@redhat.com>

Not for 6.2.

> Yes, that would certainly be better - but considering that we are
> already past RC1 of the 6.2 release, I'd rather prefer to keep this
> patch rather as small as possible and do such refactorings during the
> next development cycle instead.

Concur.

Your patch conflicts with mine.  No worries, I'll rebase.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH for-6.2] hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE

[Alistair Francis]

On Fri, Nov 19, 2021 at 8:27 PM Thomas Huth <thuth@redhat.com> wrote:
>
> Configuring a drive with "if=none" is meant for creation of a backend
> only, it should not get automatically assigned to a device frontend.
> Use "if=pflash" for the One-Time-Programmable device instead (like
> it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).
>
> Since the old way of configuring the device has already been published
> with the previous QEMU versions, we cannot remove this immediately, but
> have to deprecate it and support it for at least two more releases.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  docs/about/deprecated.rst | 6 ++++++
>  hw/misc/sifive_u_otp.c    | 9 ++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index c03fcf951f..ff7488cb63 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -192,6 +192,12 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``.
>  However, short-form booleans are deprecated and full explicit ``arg_name=on``
>  form is preferred.
>
> +``-drive if=none`` for the sifive_u OTP device (since 6.2)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Using ``-drive if=none`` to configure the OTP device of the sifive_u
> +RISC-V machine is deprecated. Use ``-drive if=pflash`` instead.
> +
>
>  QEMU Machine Protocol (QMP) commands
>  ------------------------------------
> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
> index 18aa0bd55d..cf6098ff2c 100644
> --- a/hw/misc/sifive_u_otp.c
> +++ b/hw/misc/sifive_u_otp.c
> @@ -209,7 +209,14 @@ static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
>                            TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
>
> -    dinfo = drive_get_next(IF_NONE);
> +    dinfo = drive_get_next(IF_PFLASH);
> +    if (!dinfo) {
> +        dinfo = drive_get_next(IF_NONE);
> +        if (dinfo) {
> +            warn_report("using \"-drive if=none\" for the OTP is deprecated, "
> +                        "use \"-drive if=pflash\" instead.");
> +        }
> +    }
>      if (dinfo) {
>          int ret;
>          uint64_t perm;
> --
> 2.27.0
>
>

Re: [PATCH for-6.2] hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE

[Alistair Francis]

On Fri, Nov 19, 2021 at 8:27 PM Thomas Huth <thuth@redhat.com> wrote:
>
> Configuring a drive with "if=none" is meant for creation of a backend
> only, it should not get automatically assigned to a device frontend.
> Use "if=pflash" for the One-Time-Programmable device instead (like
> it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).
>
> Since the old way of configuring the device has already been published
> with the previous QEMU versions, we cannot remove this immediately, but
> have to deprecate it and support it for at least two more releases.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  docs/about/deprecated.rst | 6 ++++++
>  hw/misc/sifive_u_otp.c    | 9 ++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index c03fcf951f..ff7488cb63 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -192,6 +192,12 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``.
>  However, short-form booleans are deprecated and full explicit ``arg_name=on``
>  form is preferred.
>
> +``-drive if=none`` for the sifive_u OTP device (since 6.2)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Using ``-drive if=none`` to configure the OTP device of the sifive_u
> +RISC-V machine is deprecated. Use ``-drive if=pflash`` instead.
> +
>
>  QEMU Machine Protocol (QMP) commands
>  ------------------------------------
> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
> index 18aa0bd55d..cf6098ff2c 100644
> --- a/hw/misc/sifive_u_otp.c
> +++ b/hw/misc/sifive_u_otp.c
> @@ -209,7 +209,14 @@ static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
>                            TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
>
> -    dinfo = drive_get_next(IF_NONE);
> +    dinfo = drive_get_next(IF_PFLASH);
> +    if (!dinfo) {
> +        dinfo = drive_get_next(IF_NONE);
> +        if (dinfo) {
> +            warn_report("using \"-drive if=none\" for the OTP is deprecated, "
> +                        "use \"-drive if=pflash\" instead.");
> +        }
> +    }
>      if (dinfo) {
>          int ret;
>          uint64_t perm;
> --
> 2.27.0
>
>