Re: [PATCH 3/3] hw/isa/vt82c686: Implement ACPI powerdown

[Bernhard Beschow]

Am 31. Januar 2023 14:58:01 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 29 Jan 2023, Bernhard Beschow wrote:
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index b0765d4ed8..2db54d1649 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -33,8 +33,10 @@
>> #include "qapi/error.h"
>> #include "qemu/log.h"
>> #include "qemu/module.h"
>> +#include "qemu/notify.h"
>> #include "qemu/range.h"
>> #include "qemu/timer.h"
>> +#include "sysemu/runstate.h"
>> #include "trace.h"
>> 
>> #define ACPI_ENABLE 0xf1
>> @@ -50,6 +52,8 @@ struct ViaPMState {
>>     APMState apm;
>>     PMSMBus smb;
>> 
>> +    Notifier powerdown_notifier;
>> +
>>     qemu_irq irq;
>
>Is there a reason to leave blank lines here? Do they separate any logical 
>blocks? If not please just drop them to allow mire lines to fit in one screen.

I could stick closer to piix4 here indeed.

>> };
>> 
>> @@ -198,6 +202,14 @@ static void via_pm_reset(DeviceState *d)
>>     smb_io_space_update(s);
>> }
>> 
>> +static void via_pm_powerdown_req(Notifier *n, void *opaque)
>> +{
>> +    ViaPMState *s = container_of(n, ViaPMState, powerdown_notifier);
>> +
>> +    assert(s != NULL);
>
>Only piix4 seems to assert in this callback, all others assume this will work 
>and indeed you register it from the realize method of the same object with its 
>already type checked state struct so this should not need to check again so I 
>think asserting here is overcautiousness.

Yes, I took a lot of inspitation from piix4. Piix4 also sets up the notifier in 
its realize method, so the situation here and there should be the same. I'd 
therefore tend to keep the code consistent. Also, an assert does communicate 
intention.

>
>As said in the cover all these are just small things I came across, sorry I 
>can't give a better review of this.

Thanks still!

Best regards,
Bernhard

P.S.: Any news from the via-ac97 side? Mind rebasing onto 7.2, even if it's 
still work in progress?

>
>Regards,
>BALATON Zoltan
>
>> +    acpi_pm1_evt_power_down(&s->ar);
>> +}
>> +
>> static void via_pm_realize(PCIDevice *dev, Error **errp)
>> {
>>     ViaPMState *s = VIA_PM(dev);
>> @@ -218,6 +230,9 @@ static void via_pm_realize(PCIDevice *dev, Error **errp)
>>     acpi_pm_tmr_init(&s->ar, pm_tmr_timer, &s->io);
>>     acpi_pm1_evt_init(&s->ar, pm_tmr_timer, &s->io);
>>     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false);
>> +
>> +    s->powerdown_notifier.notify = via_pm_powerdown_req;
>> +    qemu_register_powerdown_notifier(&s->powerdown_notifier);
>> }
>> 
>> static void via_pm_init(Object *obj)
>>