diff mbox series

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

Message ID b21a1b211ad4dc99aaf5f19d803f96dfa88b3fb1.1612821109.git.isaku.yamahata@intel.com (mailing list archive)
State New, archived
Headers show
Series ACPI related fixes | expand

Commit Message

Isaku Yamahata Feb. 8, 2021, 9:57 p.m. UTC 
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).

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>
---
 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(-)

Comments

Igor Mammedov Feb. 9, 2021, 3:05 p.m. UTC | #1 
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);
Isaku Yamahata Feb. 9, 2021, 7:23 p.m. UTC | #2 
On Tue, Feb 09, 2021 at 04:05:14PM +0100,
Igor Mammedov <imammedo@redhat.com> wrote:

> 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?

With the next patch (setting fadt.smi_cmd = 0 when smm isn't supported),
guest Linux tries to switch to ACPI mode, finds smi_cmd = 0, and then
fails to initialize acpi subsystem.
will update the commit message in next respin.


> > 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).

The patch affects guests only when SMM isn't supported.
Concretely
- user explicitly specified to disable smm by -machine smm=off
or
- underlying kvm doesn't have KVM_CAP_X86_SMM (smm=auto: default)
Please refer to x86_machine_is_smm_enabled().
Also Libvirt checks if guest bios requires SMI and enables smm even
when user disabling SMM. qemuFirmwareEnableFeatures()

If smm is disabled and legacy-mode is enabled without this patch,
SMI won't be injected to guest anyway. Thus guest breaks already.


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

Unless -machine smm=off is explicitly passed, this patch won't break
guests. And such case is rare as I described above.
My motivation for this patch series is preparation for TDX which disallows
SMM mode and SMI injection.


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

I'm fine with adding one more knob to be on safe side.
-machine smm=off is such knob, though.

Thanks,
Igor Mammedov Feb. 10, 2021, 1:17 p.m. UTC | #3 
On Tue, 9 Feb 2021 11:23:05 -0800
Isaku Yamahata <yamahata.qemudev@gmail.com> wrote:

> On Tue, Feb 09, 2021 at 04:05:14PM +0100,
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > 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?  
> 
> With the next patch (setting fadt.smi_cmd = 0 when smm isn't supported),
> guest Linux tries to switch to ACPI mode, finds smi_cmd = 0, and then
> fails to initialize acpi subsystem.
> will update the commit message in next respin.
> 
> 
> > > 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).  
> 
> The patch affects guests only when SMM isn't supported.
> Concretely
> - user explicitly specified to disable smm by -machine smm=off
> or
> - underlying kvm doesn't have KVM_CAP_X86_SMM (smm=auto: default)
> Please refer to x86_machine_is_smm_enabled().
> Also Libvirt checks if guest bios requires SMI and enables smm even
> when user disabling SMM. qemuFirmwareEnableFeatures()


> If smm is disabled and legacy-mode is enabled without this patch,
> SMI won't be injected to guest anyway. Thus guest breaks already.
can you point to code in QEMU that prevents SMI being triggered?
(what see is QEMU faking SMM being configured when smm_enable=false,
and seabios skipping its smm code based on that, other guest code
may behave differently though).

But that's beside point, guest started on old QEMU may believe that
it runs on Legacy/ACPI platform even with (smm=off), and hence can try
to enable/disable ACPI mode explicitly. Combined with migration to QEMU
with this patches it might turn into problem (which is hard to
troubleshoot in the wild).


> > Can you test this scenario with various guest OSes (old/new/MS Windows)
> > to check if it won't break them.  
> 
> Unless -machine smm=off is explicitly passed, this patch won't break
> guests. And such case is rare as I described above.

yes, it's a corner case but it doesn't guarantee that someone isn't using
it it in the wild.

> My motivation for this patch series is preparation for TDX which disallows
> SMM mode and SMI injection.
> 
> 
> > if we are to be conservative, we need to disable this compliance fix
> > on old machine types.  
> 
> I'm fine with adding one more knob to be on safe side.
> -machine smm=off is such knob, though.

you can use 2ebc21216f as an example of compat knob that changes behavior
depending on machine version. (i.e. to keep old behavior in piix4/ich9 on
exiting machine versions)

> Thanks,
diff mbox series

Patch

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);