Re: [PATCH v2 3/9] acpi/core: always set SCI_EN when SMM isn't supported

[Igor Mammedov]

On Mon,  8 Feb 2021 13:57:22 -0800
isaku.yamahata@gmail.com wrote:

> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> If SMM is not supported, ACPI fixed hardware doesn't support
> legacy-mode. ACPI-only platform. Where SCI_EN in PM1_CNT register is
> always set.
> The bit tells OS legacy mode(SCI_EN cleared) or ACPI mode(SCI_EN set).

does it break some specific software?

> ACPI spec 4.8.10.1 PM1 Event Grouping
> PM1 Eanble Registers
> > For ACPI-only platforms (where SCI_EN is always set)  
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
it changes guest ABI for old machine types but it seems to me that
it's harmless (in typical use-cases backward and forward migrated
guest should work fine).

The only thing that is broken is transitioning to legacy mode
when guest was started on old QEMU and then migrated to the new one
where disable op will be NOP and qemu always stays in ACPI mode
(so guest will hang while it waits for transition to happen).

Can you test this scenario with various guest OSes (old/new/MS Windows)
to check if it won't break them.

if we are to be conservative, we need to disable this compliance fix
on old machine types.

other than that patch looks good to me.

> ---
>  hw/acpi/core.c         | 11 ++++++++++-
>  hw/acpi/ich9.c         |  2 +-
>  hw/acpi/piix4.c        |  3 ++-
>  hw/isa/vt82c686.c      |  2 +-
>  include/hw/acpi/acpi.h |  4 +++-
>  5 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 7170bff657..1e004d0078 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -579,6 +579,10 @@ void acpi_pm1_cnt_update(ACPIREGS *ar,
>                           bool sci_enable, bool sci_disable)
>  {
>      /* ACPI specs 3.0, 4.7.2.5 */
> +    if (ar->pm1.cnt.acpi_only) {
> +        return;
> +    }
> +
>      if (sci_enable) {
>          ar->pm1.cnt.cnt |= ACPI_BITMASK_SCI_ENABLE;
>      } else if (sci_disable) {
> @@ -608,11 +612,13 @@ static const MemoryRegionOps acpi_pm_cnt_ops = {
>  };
>  
>  void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
> -                       bool disable_s3, bool disable_s4, uint8_t s4_val)
> +                       bool disable_s3, bool disable_s4, uint8_t s4_val,
> +                       bool acpi_only)
>  {
>      FWCfgState *fw_cfg;
>  
>      ar->pm1.cnt.s4_val = s4_val;
> +    ar->pm1.cnt.acpi_only = acpi_only;
>      ar->wakeup.notify = acpi_notify_wakeup;
>      qemu_register_wakeup_notifier(&ar->wakeup);
>  
> @@ -638,6 +644,9 @@ void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
>  void acpi_pm1_cnt_reset(ACPIREGS *ar)
>  {
>      ar->pm1.cnt.cnt = 0;
> +    if (ar->pm1.cnt.acpi_only) {
> +        ar->pm1.cnt.cnt |= ACPI_BITMASK_SCI_ENABLE;
> +    }
>  }
>  
>  /* ACPI GPE */
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 5ff4e01c36..1a34d7f621 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -282,7 +282,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>      acpi_pm_tmr_init(&pm->acpi_regs, ich9_pm_update_sci_fn, &pm->io);
>      acpi_pm1_evt_init(&pm->acpi_regs, ich9_pm_update_sci_fn, &pm->io);
>      acpi_pm1_cnt_init(&pm->acpi_regs, &pm->io, pm->disable_s3, 
> pm->disable_s4,
> -                      pm->s4_val);
> +                      pm->s4_val, !smm_enabled);
>  
>      acpi_gpe_init(&pm->acpi_regs, ICH9_PMIO_GPE0_LEN);
>      memory_region_init_io(&pm->io_gpe, OBJECT(lpc_pci), &ich9_gpe_ops, pm,
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 669be5bbf6..0cddf91de5 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -496,7 +496,8 @@ static void piix4_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, s->disable_s3, s->disable_s4, 
> s->s4_val);
> +    acpi_pm1_cnt_init(&s->ar, &s->io, s->disable_s3, s->disable_s4, 
> s->s4_val,
> +                      !s->smm_enabled);
>      acpi_gpe_init(&s->ar, GPE_LEN);
>  
>      s->powerdown_notifier.notify = piix4_pm_powerdown_req;
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index a6f5a0843d..071b64b497 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -240,7 +240,7 @@ static void vt82c686b_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);
> +    acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false);
>  }
>  
>  static Property via_pm_properties[] = {
> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index 22b0b65bb2..9e8a76f2e2 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -128,6 +128,7 @@ struct ACPIPM1CNT {
>      MemoryRegion io;
>      uint16_t cnt;
>      uint8_t s4_val;
> +    bool acpi_only;
>  };
>  
>  struct ACPIGPE {
> @@ -163,7 +164,8 @@ void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn 
> update_sci,
>  
>  /* PM1a_CNT: piix and ich9 don't implement PM1b CNT. */
>  void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
> -                       bool disable_s3, bool disable_s4, uint8_t s4_val);
> +                       bool disable_s3, bool disable_s4, uint8_t s4_val,
> +                       bool acpi_only);
>  void acpi_pm1_cnt_update(ACPIREGS *ar,
>                           bool sci_enable, bool sci_disable);
>  void acpi_pm1_cnt_reset(ACPIREGS *ar);