qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH hack dontapply v2 7/7] pc: HACK: acpi: tie in _C


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PATCH hack dontapply v2 7/7] pc: HACK: acpi: tie in _CST object to Processor
Date: Sat, 28 Jul 2018 23:53:12 +0300

On Fri, Jul 27, 2018 at 05:02:06PM +0200, Igor Mammedov wrote:
> On Thu, 26 Jul 2018 18:49:57 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Wed, Jul 25, 2018 at 04:49:08PM +0200, Igor Mammedov wrote:
> > > On Wed, 25 Jul 2018 15:50:09 +0300
> > > "Michael S. Tsirkin" <address@hidden> wrote:
> > >   
> > > > On Wed, Jul 25, 2018 at 02:37:24PM +0200, Igor Mammedov wrote:  
> > > > > On Tue, 10 Jul 2018 03:01:34 +0300
> > > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > > >     
> > > > > > Based on patch by Igor Mammedov.
> > > > > > 
> > > > > > This is a hack: we definitely shouldn't do it
> > > > > > unconditionally, and initialization should be handled
> > > > > > differently (through an isa device?). io port to use
> > > > > > should depend on the PC type and should be documented.
> > > > > > 
> > > > > > Notifications should be supported.
> > > > > > 
> > > > > > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > > > > > ---
> > > > > >  include/hw/i386/pc.h |  5 +++++
> > > > > >  hw/acpi/cpu.c        |  5 +++++
> > > > > >  hw/i386/acpi-build.c | 10 +++++++++-
> > > > > >  3 files changed, 19 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > > > index 316230e570..83b3a84322 100644
> > > > > > --- a/include/hw/i386/pc.h
> > > > > > +++ b/include/hw/i386/pc.h
> > > > > > @@ -20,6 +20,11 @@
> > > > > >  
> > > > > >  #define HPET_INTCAP "hpet-intcap"
> > > > > >  
> > > > > > +typedef struct PCCstate {
> > > > > > +    uint32_t latency;
> > > > > > +    uint32_t hint;
> > > > > > +} PCCstate;
> > > > > > +
> > > > > >  /**
> > > > > >   * PCMachineState:
> > > > > >   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug 
> > > > > > handling
> > > > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > > > > > index 5ae595ecbe..e9e207b033 100644
> > > > > > --- a/hw/acpi/cpu.c
> > > > > > +++ b/hw/acpi/cpu.c
> > > > > > @@ -561,6 +561,11 @@ void build_cpus_aml(Aml *table, MachineState 
> > > > > > *machine, CPUHotplugFeatures opts,
> > > > > >                             
> > > > > > aml_int(arch_ids->cpus[i].props.node_id)));
> > > > > >              }
> > > > > >  
> > > > > > +            if (1) {
> > > > > > +                method = aml_method("_CST", 0, AML_NOTSERIALIZED);
> > > > > > +                aml_append(method, aml_return(aml_call0("CCST"))); 
> > > > > >    
> > > > > in case of not loaded CCST it would be unresolved reference.
> > > > > It would be better to have a package /SB.CPUS.CCST filled with some 
> > > > > startup values
> > > > > and than that could be updated on the fly with new package.    
> > > > 
> > > > I think it will conflict if we do. But yes we could load \SB.CCST and
> > > > then \SB.CPUS.CCST will override that. Would it help though?
> > > > ACPI spec does not describe what happens on load failure.
> > > > For linux it does not matter much -
> > > > it just handles the error. What happens on windows with an unresolved
> > > > reference? Is failure to load the object any better?  
> > > I was thinking more in a align of assigning a new temporary package value 
> > > to
> > > a statically named CCST:
> > > 
> > >   update_cst_METHOD()
> > >      build package in local0
> > >      \SB.CPUS.CCST = local0
> > > 
> > > so guest gets valid initial CCST with values on source host at boot time
> > > and on target values are replaced on update.
> > > 
> > > It doesn't have to be named variable/could be a method but with always
> > > valid values.  
> > 
> > Right but what are we trying to achieve here?
> have a valid package not only on update but on boot up as well


So my patches basically do:

(1)
on interrupt:
        notify OSPM
on _CST
        get CST from host and return

What you propose:

(2)
on interrupt:
        get CST from host and store in CCST
        notify OSPM
on _CST
        return CCST

Is this a fair summary? If so I'd like to understand what do you mean
when you say "have a valid package on boot up as well".  In both
approaches _CST always returns a valid package *on each call*.

Is it that in absence of migration the configuration is easier to debug
since you can just dump SSDT to see the _CST value?

I think that's fair - unfortunately with (2) it is by comparison to (1)
harder to debug and test _CST updates since that flow looks quite
different from how normal _CST works. E.g. testing is harder as you need
migration to trigger DMA.  I would prefer a single path that works the
same with and without migration.

Let me know if I understand your concern correctly, if yes I'll try to
think of solutions.


> > 
> > > >   
> > > > > > +                aml_append(dev, method);
> > > > > > +            }
> > > > > >              aml_append(cpus_dev, dev);
> > > > > >          }
> > > > > >      }
> > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > index fff1059a31..da2c830db7 100644
> > > > > > --- a/hw/i386/acpi-build.c
> > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > @@ -64,6 +64,7 @@
> > > > > >  #include "hw/i386/intel_iommu.h"
> > > > > >  
> > > > > >  #include "hw/acpi/ipmi.h"
> > > > > > +#include "hw/acpi/cst.h"
> > > > > >  
> > > > > >  /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
> > > > > >   * -M pc-i440fx-2.0.  Even if the actual amount of AML generated 
> > > > > > grows
> > > > > > @@ -1840,7 +1841,7 @@ build_dsdt(GArray *table_data, BIOSLinker 
> > > > > > *linker,
> > > > > >          build_legacy_cpu_hotplug_aml(dsdt, machine, 
> > > > > > pm->cpu_hp_io_base);
> > > > > >      } else {
> > > > > >          CPUHotplugFeatures opts = {
> > > > > > -            .apci_1_compatible = true, .has_legacy_cphp = true
> > > > > > +            .apci_1_compatible = true, .has_legacy_cphp = true,    
> > > > > unrelated change???    
> > > > 
> > > > Good catch.
> > > >   
> > > > > >          };
> > > > > >          build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > > > > >                         "\\_SB.PCI0", "\\_GPE._E02");
> > > > > > @@ -2693,6 +2694,10 @@ void acpi_build(AcpiBuildTables *tables, 
> > > > > > MachineState *machine)
> > > > > >                             tables->vmgenid, tables->linker);
> > > > > >      }
> > > > > >  
> > > > > > +    /* TODO: get a free ioport. This one is PIIX specific. */
> > > > > > +    acpi_add_table(table_offsets, tables_blob);
> > > > > > +    cst_build_acpi(tables_blob, tables->linker, 0xaf20);
> > > > > > +
> > > > > >      if (misc.has_hpet) {
> > > > > >          acpi_add_table(table_offsets, tables_blob);
> > > > > >          build_hpet(tables_blob, tables->linker);
> > > > > > @@ -2891,6 +2896,9 @@ void acpi_setup(void)
> > > > > >  
> > > > > >      build_state = g_malloc0(sizeof *build_state);
> > > > > >  
> > > > > > +    /* TODO: this is not the best place to do it */
> > > > > > +    cst_register(pcms->fw_cfg, 0xaf20);
> > > > > > +
> > > > > >      acpi_build_tables_init(&tables);
> > > > > >      acpi_build(&tables, MACHINE(pcms));
> > > > > >      



reply via email to

[Prev in Thread] Current Thread [Next in Thread]