Hi,

Boolean properties are easier to deal with in the protocol side (e.g.
QMP) since they don't require string parsing. We should always use them
when applicable.

This series adds 3 new riscv-aia bool options for the KVM accel driver,
each one representing the possible values (emul, hwaccel and auto).
We're also deprecating the existing 'riscv-aia' string option. 

The idea is to use the new properties to enable AIA support in libvirt.

Patches based on riscv-to-apply.next.

Daniel Henrique Barboza (4):
  target/riscv/kvm: set 'aia_mode' to default in error path
  target/riscv/kvm: clarify how 'riscv-aia' default works
  target/riscv/kvm: add kvm-aia bools props
  target/riscv/kvm: deprecate riscv-aia string prop

 docs/about/deprecated.rst  |   8 +++
 target/riscv/kvm/kvm-cpu.c | 100 +++++++++++++++++++++++++++++++++----
 2 files changed, 98 insertions(+), 10 deletions(-)

-- 
2.45.2

Hi,

I had a change of heart w.r.t this work. I still believe that the boolean properties
are better to deal with since we don't have to deal with string parsing, and that we
should avoid creating new string props in the future.

But as far as the user API goes it doesn't matter that much. Having to do

-accel kvm,riscv-aia=emul

or

-accel kvm,riscv-aia-emul=on

is basically the same thing. Deprecate properties always creates some form of hassle
for existing scripts and whatnot and we should avoid it.

String properties aren't that great to report to APIs though, so what we can do is to
create internal bools to track the string value and then use it for QMP.


Long story short, I'll re-send this series with only patches 1 and 2. Thanks,


Daniel



On 9/24/24 9:44 AM, Daniel Henrique Barboza wrote:
> Hi,
> 
> Boolean properties are easier to deal with in the protocol side (e.g.
> QMP) since they don't require string parsing. We should always use them
> when applicable.
> 
> This series adds 3 new riscv-aia bool options for the KVM accel driver,
> each one representing the possible values (emul, hwaccel and auto).
> We're also deprecating the existing 'riscv-aia' string option.
> 
> The idea is to use the new properties to enable AIA support in libvirt.
> 
> Patches based on riscv-to-apply.next.
> 
> Daniel Henrique Barboza (4):
>    target/riscv/kvm: set 'aia_mode' to default in error path
>    target/riscv/kvm: clarify how 'riscv-aia' default works
>    target/riscv/kvm: add kvm-aia bools props
>    target/riscv/kvm: deprecate riscv-aia string prop
> 
>   docs/about/deprecated.rst  |   8 +++
>   target/riscv/kvm/kvm-cpu.c | 100 +++++++++++++++++++++++++++++++++----
>   2 files changed, 98 insertions(+), 10 deletions(-)
>

On Tue, Oct 29, 2024 at 4:01 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> I had a change of heart w.r.t this work. I still believe that the boolean properties
> are better to deal with since we don't have to deal with string parsing, and that we
> should avoid creating new string props in the future.
>
> But as far as the user API goes it doesn't matter that much. Having to do
>
> -accel kvm,riscv-aia=emul
>
> or
>
> -accel kvm,riscv-aia-emul=on
>
> is basically the same thing. Deprecate properties always creates some form of hassle
> for existing scripts and whatnot and we should avoid it.
>
> String properties aren't that great to report to APIs though, so what we can do is to
> create internal bools to track the string value and then use it for QMP.
>
>
> Long story short, I'll re-send this series with only patches 1 and 2. Thanks,

Ah, I should have read this before responding to your other patch.

Sounds good to me. Although I don't have the same dislike of string
properties as you, but I guess I'm also not using APIs :)

Alistair

>
>
> Daniel
>
>
>
> On 9/24/24 9:44 AM, Daniel Henrique Barboza wrote:
> > Hi,
> >
> > Boolean properties are easier to deal with in the protocol side (e.g.
> > QMP) since they don't require string parsing. We should always use them
> > when applicable.
> >
> > This series adds 3 new riscv-aia bool options for the KVM accel driver,
> > each one representing the possible values (emul, hwaccel and auto).
> > We're also deprecating the existing 'riscv-aia' string option.
> >
> > The idea is to use the new properties to enable AIA support in libvirt.
> >
> > Patches based on riscv-to-apply.next.
> >
> > Daniel Henrique Barboza (4):
> >    target/riscv/kvm: set 'aia_mode' to default in error path
> >    target/riscv/kvm: clarify how 'riscv-aia' default works
> >    target/riscv/kvm: add kvm-aia bools props
> >    target/riscv/kvm: deprecate riscv-aia string prop
> >
> >   docs/about/deprecated.rst  |   8 +++
> >   target/riscv/kvm/kvm-cpu.c | 100 +++++++++++++++++++++++++++++++++----
> >   2 files changed, 98 insertions(+), 10 deletions(-)
> >
>

On Wed, Oct 30, 2024 at 11:44:19AM +1000, Alistair Francis wrote:
> On Tue, Oct 29, 2024 at 4:01 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> > Hi,
> >
> > I had a change of heart w.r.t this work. I still believe that the boolean properties
> > are better to deal with since we don't have to deal with string parsing, and that we
> > should avoid creating new string props in the future.
> >
> > But as far as the user API goes it doesn't matter that much. Having to do
> >
> > -accel kvm,riscv-aia=emul
> >
> > or
> >
> > -accel kvm,riscv-aia-emul=on
> >
> > is basically the same thing. Deprecate properties always creates some form of hassle
> > for existing scripts and whatnot and we should avoid it.
> >
> > String properties aren't that great to report to APIs though, so what we can do is to
> > create internal bools to track the string value and then use it for QMP.
> >
> >
> > Long story short, I'll re-send this series with only patches 1 and 2. Thanks,
> 
> Ah, I should have read this before responding to your other patch.
> 
> Sounds good to me. Although I don't have the same dislike of string
> properties as you, but I guess I'm also not using APIs :)

libvirt and other upper layers which use qmp would need to learn about
each property's possible values, possibly requiring QEMU to provide
different APIs for each different property type. With only boolean
properties, all an object's properties can be queried and modified in the
same way, which also allows immediately knowing how to enable and disable
new properties which QEMU adds without the need to update the upper layers
at all.

Thanks,
drew