Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-08 Thread Thomas Huth

On 08/02/2023 17.48, Andrea Bolognani wrote:

On Tue, Feb 07, 2023 at 08:20:31PM +0100, Andrew Jones wrote:

On Tue, Feb 07, 2023 at 06:38:15AM -0800, Andrea Bolognani wrote:

Well, it looks like -no-acpi will come for free after all, thanks to
the code you pasted above, as long as the machine property follows
the convention established by x86, arm and (I just noticed this one)
loongarch.


Not quite, because we also have

$ grep -A1 '"no-acpi"' qemu-options.hx
DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
"-no-acpidisable ACPI\n", QEMU_ARCH_I386 | QEMU_ARCH_ARM)

So that means neither riscv nor loongarch get -no-acpi just by adding
the "acpi" machine property.

If the plan is to deprecate -no-acpi, then riscv can be like loongarch
and only have the "acpi" property from the start.


Makes sense.


For the libvirt integration, do I understand correctly that the
machine property was added by commit

   commit 17e89077b7e3bc1d96540e21ddc7451c3e077049
   Author: Gerd Hoffmann 
   Date:   Fri Mar 20 11:01:36 2020 +0100

 acpi: add acpi=OnOffAuto machine property to x86 and arm virt

 Remove the global acpi_enabled bool and replace it with an
 acpi OnOffAuto machine property.

 qemu throws an error now if you use -no-acpi while the machine
 type you are using doesn't support acpi in the first place.

 Signed-off-by: Gerd Hoffmann 
 Message-Id: <20200320100136.11717-1-kra...@redhat.com>
 Reviewed-by: Philippe Mathieu-Daudé 
 Acked-by: Paolo Bonzini 
 Reviewed-by: Michael S. Tsirkin 
 Signed-off-by: Michael S. Tsirkin 

included in QEMU 5.0? libvirt still officially supports QEMU 4.2, so
we'll have to make the use of the machine property conditional.


Sounds right, and testing for the machine option should be possible with the 
upcoming QEMU 8.0. FWIW, I assume this is similar to the -no-hpet option 
which has been turned into a machine property, too:


https://gitlab.com/libvirt/libvirt/-/commit/3c508e7d4310aeb8

 Thomas




Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-08 Thread Andrea Bolognani
On Tue, Feb 07, 2023 at 08:20:31PM +0100, Andrew Jones wrote:
> On Tue, Feb 07, 2023 at 06:38:15AM -0800, Andrea Bolognani wrote:
> > Well, it looks like -no-acpi will come for free after all, thanks to
> > the code you pasted above, as long as the machine property follows
> > the convention established by x86, arm and (I just noticed this one)
> > loongarch.
>
> Not quite, because we also have
>
> $ grep -A1 '"no-acpi"' qemu-options.hx
> DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
>"-no-acpidisable ACPI\n", QEMU_ARCH_I386 | QEMU_ARCH_ARM)
>
> So that means neither riscv nor loongarch get -no-acpi just by adding
> the "acpi" machine property.
>
> If the plan is to deprecate -no-acpi, then riscv can be like loongarch
> and only have the "acpi" property from the start.

Makes sense.


For the libvirt integration, do I understand correctly that the
machine property was added by commit

  commit 17e89077b7e3bc1d96540e21ddc7451c3e077049
  Author: Gerd Hoffmann 
  Date:   Fri Mar 20 11:01:36 2020 +0100

acpi: add acpi=OnOffAuto machine property to x86 and arm virt

Remove the global acpi_enabled bool and replace it with an
acpi OnOffAuto machine property.

qemu throws an error now if you use -no-acpi while the machine
type you are using doesn't support acpi in the first place.

Signed-off-by: Gerd Hoffmann 
Message-Id: <20200320100136.11717-1-kra...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé 
Acked-by: Paolo Bonzini 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 

included in QEMU 5.0? libvirt still officially supports QEMU 4.2, so
we'll have to make the use of the machine property conditional.

-- 
Andrea Bolognani / Red Hat / Virtualization




Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-07 Thread Andrew Jones
On Tue, Feb 07, 2023 at 06:38:15AM -0800, Andrea Bolognani wrote:
> On Tue, Feb 07, 2023 at 03:02:19PM +0100, Thomas Huth wrote:
> > On 07/02/2023 14.56, Andrea Bolognani wrote:
> > > It looks like i440fx and q35 both have an 'acpi' machine property. Is
> > > -no-acpi just sugar for acpi=off?
> >
> > Yes, it is, see softmmu/vl.c:
> >
> > case QEMU_OPTION_no_acpi:
> > qdict_put_str(machine_opts_dict, "acpi", "off");
> > break;
> >
> > > Is it considered desirable to get rid of -no-acpi?
> >
> > Sounds like a good idea, indeed!
> >
> > > If so, we should follow the usual deprecation
> > > process and get rid of it after libvirt has had a chance to adapt.
> > >
> > > In the scenario described above, it would make sense for RISC-V to
> > > only offer the machine type option (assuming that -no-acpi doesn't
> > > come for free with that) instead of putting additional effort into
> > > implementing an interface that is already on its way out.
> >
> > I agree. IMHO the machine parameter should be enough, no need to introduce
> > "-no-acpi" here.
> 
> Well, it looks like -no-acpi will come for free after all, thanks to
> the code you pasted above, as long as the machine property follows
> the convention established by x86, arm and (I just noticed this one)
> loongarch.

Not quite, because we also have

$ grep -A1 '"no-acpi"' qemu-options.hx
DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
   "-no-acpidisable ACPI\n", QEMU_ARCH_I386 | QEMU_ARCH_ARM)

So that means neither riscv nor loongarch get -no-acpi just by adding
the "acpi" machine property.

If the plan is to deprecate -no-acpi, then riscv can be like loongarch
and only have the "acpi" property from the start.

Thanks,
drew



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-07 Thread Andrea Bolognani
On Tue, Feb 07, 2023 at 03:02:19PM +0100, Thomas Huth wrote:
> On 07/02/2023 14.56, Andrea Bolognani wrote:
> > It looks like i440fx and q35 both have an 'acpi' machine property. Is
> > -no-acpi just sugar for acpi=off?
>
> Yes, it is, see softmmu/vl.c:
>
> case QEMU_OPTION_no_acpi:
> qdict_put_str(machine_opts_dict, "acpi", "off");
> break;
>
> > Is it considered desirable to get rid of -no-acpi?
>
> Sounds like a good idea, indeed!
>
> > If so, we should follow the usual deprecation
> > process and get rid of it after libvirt has had a chance to adapt.
> >
> > In the scenario described above, it would make sense for RISC-V to
> > only offer the machine type option (assuming that -no-acpi doesn't
> > come for free with that) instead of putting additional effort into
> > implementing an interface that is already on its way out.
>
> I agree. IMHO the machine parameter should be enough, no need to introduce
> "-no-acpi" here.

Well, it looks like -no-acpi will come for free after all, thanks to
the code you pasted above, as long as the machine property follows
the convention established by x86, arm and (I just noticed this one)
loongarch.

So I guess the -no-acpi deprecation can be handled separately, and
the only thing that needs changing in the current patch is making
sure that ACPI is opt-out rather than opt-in, as is the case for
other architectures :)

-- 
Andrea Bolognani / Red Hat / Virtualization




Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-07 Thread Thomas Huth

On 07/02/2023 14.56, Andrea Bolognani wrote:

On Tue, Feb 07, 2023 at 11:23:53AM +0100, Andrew Jones wrote:

On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:

On 6/2/23 11:54, Andrea Bolognani wrote:

On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:

+object_class_property_add(oc, "acpi", "OnOffAuto",
+  virt_get_acpi, virt_set_acpi,
+  NULL, NULL);
+object_class_property_set_description(oc, "acpi",
+  "Enable ACPI");


The way this works on other architectures (x86_64, aarch64) is that
you get ACPI by default and can use -no-acpi to disable it if
desired. Can we have the same on RISC-V, for consistency?


-no-acpi rather seems a x86-specific hack for the ISA PC machine, and
has a high maintenance cost / burden.


Can you elaborate on this? RISCV doesn't need '-no-acpi' specifically.
If -no-acpi is problematic for some reason, then something like
'-machine virt,acpi=off' would be sufficient for switching to DT too.


I would greatly prefer it if the command line interface could be kept
consistent across architectures.

It looks like i440fx and q35 both have an 'acpi' machine property. Is
-no-acpi just sugar for acpi=off?


Yes, it is, see softmmu/vl.c:

case QEMU_OPTION_no_acpi:
qdict_put_str(machine_opts_dict, "acpi", "off");
break;


Is it considered desirable to get rid of -no-acpi?


Sounds like a good idea, indeed!


If so, we should follow the usual deprecation
process and get rid of it after libvirt has had a chance to adapt.

In the scenario described above, it would make sense for RISC-V to
only offer the machine type option (assuming that -no-acpi doesn't
come for free with that) instead of putting additional effort into
implementing an interface that is already on its way out.


I agree. IMHO the machine parameter should be enough, no need to introduce 
"-no-acpi" here.


 Thomas




Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-07 Thread Andrea Bolognani
On Tue, Feb 07, 2023 at 11:23:53AM +0100, Andrew Jones wrote:
> On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > +object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > +  virt_get_acpi, virt_set_acpi,
> > > > +  NULL, NULL);
> > > > +object_class_property_set_description(oc, "acpi",
> > > > +  "Enable ACPI");
> > >
> > > The way this works on other architectures (x86_64, aarch64) is that
> > > you get ACPI by default and can use -no-acpi to disable it if
> > > desired. Can we have the same on RISC-V, for consistency?
> >
> > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> > has a high maintenance cost / burden.
>
> Can you elaborate on this? RISCV doesn't need '-no-acpi' specifically.
> If -no-acpi is problematic for some reason, then something like
> '-machine virt,acpi=off' would be sufficient for switching to DT too.

I would greatly prefer it if the command line interface could be kept
consistent across architectures.

It looks like i440fx and q35 both have an 'acpi' machine property. Is
-no-acpi just sugar for acpi=off? Is it considered desirable to get
rid of -no-acpi? If so, we should follow the usual deprecation
process and get rid of it after libvirt has had a chance to adapt.

In the scenario described above, it would make sense for RISC-V to
only offer the machine type option (assuming that -no-acpi doesn't
come for free with that) instead of putting additional effort into
implementing an interface that is already on its way out.

-- 
Andrea Bolognani / Red Hat / Virtualization




Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-07 Thread Sunil V L
On Tue, Feb 07, 2023 at 11:57:29AM +0800, Bin Meng wrote:
> On Mon, Feb 6, 2023 at 8:36 PM Andrew Jones  wrote:
> >
> > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > > +object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > > +  virt_get_acpi, virt_set_acpi,
> > > > > +  NULL, NULL);
> > > > > +object_class_property_set_description(oc, "acpi",
> > > > > +  "Enable ACPI");
> > > >
> > > > The way this works on other architectures (x86_64, aarch64) is that
> > > > you get ACPI by default and can use -no-acpi to disable it if
> > > > desired. Can we have the same on RISC-V, for consistency?
> >
> > Default on, with a user control to turn off, can be done with a boolean.
> > I'm not sure why/if Auto is needed for acpi. Auto is useful when a
> > configuration doesn't support a default setting for a feature. If the
> > user hasn't explicitly requested the feature to be on or off, then the
> > configuration can silently select what works. If, however, the user
> > explicitly chooses what doesn't work, then qemu will fail with an error
> > instead.
> 
> I have a confusion about "OnOffAuto" vs. "bool" type.
> 
> Both "OnOffAuto" vs. "bool" type property can have a default value if
> user does not assign a value to it from command line. The default
> value is:
> 
> - ON_OFF_AUTO_AUTO for "OnOffAuto"
> - false for "bool"
> 
> But the default value can be overridden in the machine's init
> function, like in this patch.
> 
> So I am not really sure how these 2 types of properties are different.
> Why did we introduce a "OnOffAuto" type, and how is that type supposed
> to be used in which scenario?
> 

I don't know either. Since it is the same property across architectures,
I used the OnOffAuto instead of a bool. 

May be Gerd and other qemu experts can help us to understand better.
https://github.com/qemu/qemu/commit/17e89077b7e3bc1d96540e21ddc7451c3e077049

Thanks,
Sunil



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-07 Thread Andrew Jones
On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> On 6/2/23 11:54, Andrea Bolognani wrote:
> > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > +object_class_property_add(oc, "acpi", "OnOffAuto",
> > > +  virt_get_acpi, virt_set_acpi,
> > > +  NULL, NULL);
> > > +object_class_property_set_description(oc, "acpi",
> > > +  "Enable ACPI");
> > 
> > The way this works on other architectures (x86_64, aarch64) is that
> > you get ACPI by default and can use -no-acpi to disable it if
> > desired. Can we have the same on RISC-V, for consistency?
> 
> -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> has a high maintenance cost / burden.

Can you elaborate on this? RISCV doesn't need '-no-acpi' specifically.
If -no-acpi is problematic for some reason, then something like
'-machine virt,acpi=off' would be sufficient for switching to DT too.

Thanks,
drew



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-07 Thread Andrew Jones
On Tue, Feb 07, 2023 at 09:50:29AM +0100, Philippe Mathieu-Daudé wrote:
> On 6/2/23 13:56, Gerd Hoffmann wrote:
> > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > > +object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > > +  virt_get_acpi, virt_set_acpi,
> > > > > +  NULL, NULL);
> > > > > +object_class_property_set_description(oc, "acpi",
> > > > > +  "Enable ACPI");
> > > > 
> > > > The way this works on other architectures (x86_64, aarch64) is that
> > > > you get ACPI by default and can use -no-acpi to disable it if
> > > > desired. Can we have the same on RISC-V, for consistency?
> > > 
> > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> > > has a high maintenance cost / burden.
> > 
> > Under the hood it is actually a OnOffAuto machine property and -no-acpi
> > is just a shortcut to set that.
> > 
> > > Actually, what is the value added by '-no-acpi'?
> > 
> > On arm(64) the linux kernel can use either device trees or ACPI to find
> > the hardware.  Historical reasons mostly, when the platform started
> > there was no ACPI support.  Also the edk2 firmware uses Device Trees
> > for hardware discovery, likewise for historical reasons.
> > 
> > When ACPI is available for a platform right from the start I see little
> > reason to offer an option to turn it off though ...
> 
> Yeah I concur. There is no point in disabling ACPI on the RISCV virt
> machine IMO.

edk2 will only present DT or ACPI to the guest, not both. However, RISCV
Linux supports both. If we don't offer '-no-acpi' as a way to switch to
DT, then edk2+DT users will need to configure the varstore to select it.
And, since testing needs to be done with both, that varstore change will
need to be added to all the testcases which need DT (or a varstore with
DT already selected will need to be maintained and used by the testsuites)

IMO, the generation of the ACPI tables should be 'on' by default, but then
the, already present, '-no-acpi' command line option should be made
available in order to easily inform edk2 to present DT instead of ACPI.

Thanks,
drew



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-07 Thread Sunil V L
On Tue, Feb 07, 2023 at 09:50:29AM +0100, Philippe Mathieu-Daudé wrote:
> On 6/2/23 13:56, Gerd Hoffmann wrote:
> > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > > +object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > > +  virt_get_acpi, virt_set_acpi,
> > > > > +  NULL, NULL);
> > > > > +object_class_property_set_description(oc, "acpi",
> > > > > +  "Enable ACPI");
> > > > 
> > > > The way this works on other architectures (x86_64, aarch64) is that
> > > > you get ACPI by default and can use -no-acpi to disable it if
> > > > desired. Can we have the same on RISC-V, for consistency?
> > > 
> > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> > > has a high maintenance cost / burden.
> > 
> > Under the hood it is actually a OnOffAuto machine property and -no-acpi
> > is just a shortcut to set that.
> > 
> > > Actually, what is the value added by '-no-acpi'?
> > 
> > On arm(64) the linux kernel can use either device trees or ACPI to find
> > the hardware.  Historical reasons mostly, when the platform started
> > there was no ACPI support.  Also the edk2 firmware uses Device Trees
> > for hardware discovery, likewise for historical reasons.
> > 
> > When ACPI is available for a platform right from the start I see little
> > reason to offer an option to turn it off though ...
> 
> Yeah I concur. There is no point in disabling ACPI on the RISCV virt
> machine IMO.

Thank you all for the inputs. I am fine to keep it enabled by default.
Do you mean we don't need the switch at all even for some
testing/debugging purpose?

Thanks,
Sunil



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-07 Thread Philippe Mathieu-Daudé

On 6/2/23 13:56, Gerd Hoffmann wrote:

On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:

On 6/2/23 11:54, Andrea Bolognani wrote:

On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:

+object_class_property_add(oc, "acpi", "OnOffAuto",
+  virt_get_acpi, virt_set_acpi,
+  NULL, NULL);
+object_class_property_set_description(oc, "acpi",
+  "Enable ACPI");


The way this works on other architectures (x86_64, aarch64) is that
you get ACPI by default and can use -no-acpi to disable it if
desired. Can we have the same on RISC-V, for consistency?


-no-acpi rather seems a x86-specific hack for the ISA PC machine, and
has a high maintenance cost / burden.


Under the hood it is actually a OnOffAuto machine property and -no-acpi
is just a shortcut to set that.


Actually, what is the value added by '-no-acpi'?


On arm(64) the linux kernel can use either device trees or ACPI to find
the hardware.  Historical reasons mostly, when the platform started
there was no ACPI support.  Also the edk2 firmware uses Device Trees
for hardware discovery, likewise for historical reasons.

When ACPI is available for a platform right from the start I see little
reason to offer an option to turn it off though ...


Yeah I concur. There is no point in disabling ACPI on the RISCV virt
machine IMO.



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-06 Thread Andrew Jones
On Tue, Feb 07, 2023 at 11:57:29AM +0800, Bin Meng wrote:
> On Mon, Feb 6, 2023 at 8:36 PM Andrew Jones  wrote:
> >
> > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > > +object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > > +  virt_get_acpi, virt_set_acpi,
> > > > > +  NULL, NULL);
> > > > > +object_class_property_set_description(oc, "acpi",
> > > > > +  "Enable ACPI");
> > > >
> > > > The way this works on other architectures (x86_64, aarch64) is that
> > > > you get ACPI by default and can use -no-acpi to disable it if
> > > > desired. Can we have the same on RISC-V, for consistency?
> >
> > Default on, with a user control to turn off, can be done with a boolean.
> > I'm not sure why/if Auto is needed for acpi. Auto is useful when a
> > configuration doesn't support a default setting for a feature. If the
> > user hasn't explicitly requested the feature to be on or off, then the
> > configuration can silently select what works. If, however, the user
> > explicitly chooses what doesn't work, then qemu will fail with an error
> > instead.
> 
> I have a confusion about "OnOffAuto" vs. "bool" type.
> 
> Both "OnOffAuto" vs. "bool" type property can have a default value if
> user does not assign a value to it from command line. The default
> value is:
> 
> - ON_OFF_AUTO_AUTO for "OnOffAuto"
> - false for "bool"
> 
> But the default value can be overridden in the machine's init
> function, like in this patch.
> 
> So I am not really sure how these 2 types of properties are different.
> Why did we introduce a "OnOffAuto" type, and how is that type supposed
> to be used in which scenario?

Auto makes sense for options which have dependencies on other options.
For example, if we have two options, A and B, where A is an Auto and
B is a boolean, then, when A is initialized to AUTO and has a dependency
on B being X, then have the following

B=X A=AUTO -> T (works)
B=!XA=AUTO -> F (works)

(This is the same whether B was set to X by default or explicitly by the
user.)

Now, if the user explicitly sets A, we have

B=X A=T (works)
B=X A=F (works)
B=!XA=T (emit error about B!=X with A=T and fail)
B=!XA=F (works)

We can't have that behavior with A just being a boolean, defaulting to
true, because we don't know if it's true because of the default or
because it was explicitly set

B=X A=T (works, by default or by user)
B=X A=F (works)
B=!XA=T (doesn't work, but if the user didn't
 select A=T, then we could have silently
 flipped it to F and continued)
B=!XA=F (works)


IOW, Auto just adds one more bit of info, allowing default vs. user
selection to be determined, which can then be used for different
behaviors.

Back to the "acpi" property, I'm not sure what it depends on that
requires it to be an Auto vs. a boolean.

Thanks,
drew



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-06 Thread Bin Meng
On Mon, Feb 6, 2023 at 8:36 PM Andrew Jones  wrote:
>
> On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > +object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > +  virt_get_acpi, virt_set_acpi,
> > > > +  NULL, NULL);
> > > > +object_class_property_set_description(oc, "acpi",
> > > > +  "Enable ACPI");
> > >
> > > The way this works on other architectures (x86_64, aarch64) is that
> > > you get ACPI by default and can use -no-acpi to disable it if
> > > desired. Can we have the same on RISC-V, for consistency?
>
> Default on, with a user control to turn off, can be done with a boolean.
> I'm not sure why/if Auto is needed for acpi. Auto is useful when a
> configuration doesn't support a default setting for a feature. If the
> user hasn't explicitly requested the feature to be on or off, then the
> configuration can silently select what works. If, however, the user
> explicitly chooses what doesn't work, then qemu will fail with an error
> instead.

I have a confusion about "OnOffAuto" vs. "bool" type.

Both "OnOffAuto" vs. "bool" type property can have a default value if
user does not assign a value to it from command line. The default
value is:

- ON_OFF_AUTO_AUTO for "OnOffAuto"
- false for "bool"

But the default value can be overridden in the machine's init
function, like in this patch.

So I am not really sure how these 2 types of properties are different.
Why did we introduce a "OnOffAuto" type, and how is that type supposed
to be used in which scenario?

>
> >
> > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> > has a high maintenance cost / burden.
> >
> > If hardware provides ACPI support, QEMU should expose it to the guest.
> >
> > Actually, what is the value added by '-no-acpi'?
>
> IIRC, when booting, at least arm guests, with edk2 and ACPI tables,
> then edk2 will provide the guest ACPI tables instead of DT. To ensure
> we can boot with edk2, but still allow the guest to boot with DT, we
> need a way to disable the generation of ACPI tables.
>

Regards,
Bin



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-06 Thread Bernhard Beschow



Am 6. Februar 2023 11:18:06 UTC schrieb "Philippe Mathieu-Daudé" 
:
>On 6/2/23 11:54, Andrea Bolognani wrote:
>> On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
>>> +object_class_property_add(oc, "acpi", "OnOffAuto",
>>> +  virt_get_acpi, virt_set_acpi,
>>> +  NULL, NULL);
>>> +object_class_property_set_description(oc, "acpi",
>>> +  "Enable ACPI");
>> 
>> The way this works on other architectures (x86_64, aarch64) is that
>> you get ACPI by default and can use -no-acpi to disable it if
>> desired. Can we have the same on RISC-V, for consistency?
>
>-no-acpi rather seems a x86-specific hack for the ISA PC machine,

... for the i440FX/PIIX machine, to be precise. There it controls the presence 
of the PIIX ACPI controller and surely also the generation of ACPI tables. ACPI 
wasn't a thing in pure ISA times, so the switch doesn't make much sense for the 
ISA machine.

Here, for RISC-V, the "acpi" switch seems to just control the generation of 
ACPI tables which has quite different semantics than -no-acpi.

>and
>has a high maintenance cost / burden.
>
>If hardware provides ACPI support, QEMU should expose it to the guest.

Yes, I fully agree with both points.

>
>Actually, what is the value added by '-no-acpi'?

IIUC, it allows the PC machine to emulate a PCI PC without an ACPI bios. 
Unfortunately, it also omits the instantiation of the... erm... Frankenstein 
PIIX4_ACPI device which seems quite unnecessary to achieve that goal. Just 
always instantiating it seems much simpler.



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-06 Thread Sunil V L
On Mon, Feb 06, 2023 at 01:35:20PM +0100, Andrew Jones wrote:
> On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > +object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > +  virt_get_acpi, virt_set_acpi,
> > > > +  NULL, NULL);
> > > > +object_class_property_set_description(oc, "acpi",
> > > > +  "Enable ACPI");
> > > 
> > > The way this works on other architectures (x86_64, aarch64) is that
> > > you get ACPI by default and can use -no-acpi to disable it if
> > > desired. Can we have the same on RISC-V, for consistency?
> 
> Default on, with a user control to turn off, can be done with a boolean.
> I'm not sure why/if Auto is needed for acpi. Auto is useful when a
> configuration doesn't support a default setting for a feature. If the
> user hasn't explicitly requested the feature to be on or off, then the
> configuration can silently select what works. If, however, the user
> explicitly chooses what doesn't work, then qemu will fail with an error
> instead.
> 

Since all other architectures use Auto instead of a simple bool, I opted
for the same to keep it consistent.

However, default AUTO looked ambiguous to me. Since we still need to
support external interrupt controllers (IMSIC/APLIC/PLIC), I chose to
keep it OFF by default for now.

Thanks
Sunil

> > 
> > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> > has a high maintenance cost / burden.
> > 
> > If hardware provides ACPI support, QEMU should expose it to the guest.
> > 
> > Actually, what is the value added by '-no-acpi'?
> 
> IIRC, when booting, at least arm guests, with edk2 and ACPI tables,
> then edk2 will provide the guest ACPI tables instead of DT. To ensure
> we can boot with edk2, but still allow the guest to boot with DT, we
> need a way to disable the generation of ACPI tables.
> 
> Thanks,
> drew



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-06 Thread Gerd Hoffmann
On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> On 6/2/23 11:54, Andrea Bolognani wrote:
> > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > +object_class_property_add(oc, "acpi", "OnOffAuto",
> > > +  virt_get_acpi, virt_set_acpi,
> > > +  NULL, NULL);
> > > +object_class_property_set_description(oc, "acpi",
> > > +  "Enable ACPI");
> > 
> > The way this works on other architectures (x86_64, aarch64) is that
> > you get ACPI by default and can use -no-acpi to disable it if
> > desired. Can we have the same on RISC-V, for consistency?
> 
> -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> has a high maintenance cost / burden.

Under the hood it is actually a OnOffAuto machine property and -no-acpi
is just a shortcut to set that.

> Actually, what is the value added by '-no-acpi'?

On arm(64) the linux kernel can use either device trees or ACPI to find
the hardware.  Historical reasons mostly, when the platform started
there was no ACPI support.  Also the edk2 firmware uses Device Trees
for hardware discovery, likewise for historical reasons.

When ACPI is available for a platform right from the start I see little
reason to offer an option to turn it off though ...

take care,
  Gerd




Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-06 Thread Andrew Jones
On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> On 6/2/23 11:54, Andrea Bolognani wrote:
> > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > +object_class_property_add(oc, "acpi", "OnOffAuto",
> > > +  virt_get_acpi, virt_set_acpi,
> > > +  NULL, NULL);
> > > +object_class_property_set_description(oc, "acpi",
> > > +  "Enable ACPI");
> > 
> > The way this works on other architectures (x86_64, aarch64) is that
> > you get ACPI by default and can use -no-acpi to disable it if
> > desired. Can we have the same on RISC-V, for consistency?

Default on, with a user control to turn off, can be done with a boolean.
I'm not sure why/if Auto is needed for acpi. Auto is useful when a
configuration doesn't support a default setting for a feature. If the
user hasn't explicitly requested the feature to be on or off, then the
configuration can silently select what works. If, however, the user
explicitly chooses what doesn't work, then qemu will fail with an error
instead.

> 
> -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> has a high maintenance cost / burden.
> 
> If hardware provides ACPI support, QEMU should expose it to the guest.
> 
> Actually, what is the value added by '-no-acpi'?

IIRC, when booting, at least arm guests, with edk2 and ACPI tables,
then edk2 will provide the guest ACPI tables instead of DT. To ensure
we can boot with edk2, but still allow the guest to boot with DT, we
need a way to disable the generation of ACPI tables.

Thanks,
drew



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-06 Thread Philippe Mathieu-Daudé

On 6/2/23 11:54, Andrea Bolognani wrote:

On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:

+object_class_property_add(oc, "acpi", "OnOffAuto",
+  virt_get_acpi, virt_set_acpi,
+  NULL, NULL);
+object_class_property_set_description(oc, "acpi",
+  "Enable ACPI");


The way this works on other architectures (x86_64, aarch64) is that
you get ACPI by default and can use -no-acpi to disable it if
desired. Can we have the same on RISC-V, for consistency?


-no-acpi rather seems a x86-specific hack for the ISA PC machine, and
has a high maintenance cost / burden.

If hardware provides ACPI support, QEMU should expose it to the guest.

Actually, what is the value added by '-no-acpi'?



Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-06 Thread Andrea Bolognani
On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> +object_class_property_add(oc, "acpi", "OnOffAuto",
> +  virt_get_acpi, virt_set_acpi,
> +  NULL, NULL);
> +object_class_property_set_description(oc, "acpi",
> +  "Enable ACPI");

The way this works on other architectures (x86_64, aarch64) is that
you get ACPI by default and can use -no-acpi to disable it if
desired. Can we have the same on RISC-V, for consistency?

-- 
Andrea Bolognani / Red Hat / Virtualization




Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

2023-02-06 Thread Bin Meng
On Thu, Feb 2, 2023 at 12:54 PM Sunil V L  wrote:
>
> ACPI is optional. So, add a switch to toggle.
>
> Signed-off-by: Sunil V L 
> ---
>  hw/riscv/virt.c | 38 ++
>  include/hw/riscv/virt.h |  2 ++
>  2 files changed, 40 insertions(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 7ad9fda20c..84962962ff 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -50,6 +50,7 @@
>  #include "hw/pci-host/gpex.h"
>  #include "hw/display/ramfb.h"
>  #include "hw/acpi/aml-build.h"
> +#include "qapi/qapi-visit-common.h"
>
>  /*
>   * The virt machine physical address space used by some of the devices
> @@ -1525,6 +1526,10 @@ static void virt_machine_init(MachineState *machine)
>
>  static void virt_machine_instance_init(Object *obj)
>  {
> +MachineState *ms = MACHINE(obj);

Drop this

> +RISCVVirtState *s = RISCV_VIRT_MACHINE(ms);
> +
> +s->acpi = ON_OFF_AUTO_OFF;

Is this needed? I believe the purpose of an auto/on/off property is to
have an "auto" value, which is ON_OFF_AUTO_AUTO.

>  }
>
>  static char *virt_get_aia_guests(Object *obj, Error **errp)
> @@ -1601,6 +1606,34 @@ static void virt_set_aclint(Object *obj, bool value, 
> Error **errp)
>  s->have_aclint = value;
>  }
>
> +bool virt_is_acpi_enabled(RISCVVirtState *s)
> +{
> +if (s->acpi == ON_OFF_AUTO_OFF) {
> +return false;
> +}
> +return true;
> +}
> +
> +static void virt_get_acpi(Object *obj, Visitor *v, const char *name,
> +  void *opaque, Error **errp)
> +{
> +MachineState *ms = MACHINE(obj);

ditto

> +RISCVVirtState *s = RISCV_VIRT_MACHINE(ms);
> +
> +OnOffAuto acpi = s->acpi;
> +
> +visit_type_OnOffAuto(v, name, &acpi, errp);
> +}
> +
> +static void virt_set_acpi(Object *obj, Visitor *v, const char *name,
> +  void *opaque, Error **errp)
> +{
> +MachineState *ms = MACHINE(obj);

ditto

> +RISCVVirtState *s = RISCV_VIRT_MACHINE(ms);
> +
> +visit_type_OnOffAuto(v, name, &s->acpi, errp);
> +}
> +
>  static HotplugHandler *virt_machine_get_hotplug_handler(MachineState 
> *machine,
>  DeviceState *dev)
>  {
> @@ -1672,6 +1705,11 @@ static void virt_machine_class_init(ObjectClass *oc, 
> void *data)
>  sprintf(str, "Set number of guest MMIO pages for AIA IMSIC. Valid value "
>   "should be between 0 and %d.", VIRT_IRQCHIP_MAX_GUESTS);
>  object_class_property_set_description(oc, "aia-guests", str);
> +object_class_property_add(oc, "acpi", "OnOffAuto",
> +  virt_get_acpi, virt_set_acpi,
> +  NULL, NULL);

I am not sure about "OnOffAuto" vs. "bool" type. It seems the only
difference is that with "OnOffAuto" type we may silently change the
interpretation of "auto" value across different QEMU versions?

> +object_class_property_set_description(oc, "acpi",
> +  "Enable ACPI");
>  }
>
>  static const TypeInfo virt_machine_typeinfo = {
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 6c7885bf89..62efebaa32 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -58,6 +58,7 @@ struct RISCVVirtState {
>  int aia_guests;
>  char *oem_id;
>  char *oem_table_id;
> +OnOffAuto acpi;
>  };
>
>  enum {
> @@ -123,4 +124,5 @@ enum {
>  #define FDT_APLIC_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \
>   1 + FDT_APLIC_INT_CELLS)
>
> +bool virt_is_acpi_enabled(RISCVVirtState *s);
>  #endif
> --

Regards,
Bin