Re: [Qemu-devel] [PATCH 23/27] pc: ACPI BIOS: implement memory hotplug interface

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 16 Dec 2013 21:50:53 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Nov 25, 2013 at 03:39:13PM +0100, Igor Mammedov wrote:
> > On Thu, 21 Nov 2013 11:37:01 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, Nov 21, 2013 at 03:38:44AM +0100, Igor Mammedov wrote:
> > > > - provides static SSDT object for memory hotplug
> > > > - SSDT template for memory devices and runtime generator
> > > >   of them in SSDT table.
> > > > 
> > > > Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > [...]
> > 
> > > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> > > > index a4484b8..b0c581e 100644
> > > > --- a/hw/i386/ssdt-misc.dsl
> > > > +++ b/hw/i386/ssdt-misc.dsl
> > > > @@ -116,4 +116,183 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> > > >              }
> > > >          }
> > > >      }
> > > > +
> > > > +    External(MTFY, MethodObj)
> > > > +
> > > > +    Scope(\_SB) {
> > > > +        Device(MHPD) {
> > > > +            Name(_HID, EISAID("PNP0C08"))
> > > > +
> > > > +            ACPI_EXTRACT_NAME_WORD_CONST ssdt_mctrl_port
> > > > +            Name(MHPP, 0xFFFF)
> > > > +
> > > > +            ACPI_EXTRACT_NAME_DWORD_CONST ssdt_mctrl_nr_slots
> > > > +            Name(MDNR, 0x12345678)
> > > > +
> > > > +            /* Memory hotplug IO registers */
> > > > +            OperationRegion(HPMR, SystemIO, MHPP, 24)
> > > > +
> > > > +            Method(_CRS, 0, Serialized) {
> > > > +                Name(CRS, ResourceTemplate() {
> > > > +                    IO(Decode16, 0x00, 0x00, 0x01, 24, IO)
> > > > +                })
> > > 
> > > 
> > > Declaring name makes us serialize it.
> > > Can't we use a local variable?
> > If that works, I'll change it.
> > 
> > But I have a question of my own perhaps to Paolo or Gerd,
> > Do we really need this _CRS, because if you look at next hunk
> > _STA should report not present but functioning to avoid windows BSOD.
> > So there is not guaranties that OSPM would care or even query it
> > and honor _CRS provided range.
> > 
> 
> Yes this worries me too.
> Making _STA non present looks wrong.
I've made status as not present and enabled/decoding and functioning, but that
doesn't mean that OSPM respects its _CRS.
 
> I suspect we need to look for some other way to make
> windows not crash that will make it respect the _CRS.
> Also which windows? All of them?
I've tested with WS2012DCx64,

PV panic doesn't crash windows because it's showed as Unknown device, which
also doesn't guaranty that windows would respects it's _CRS.

OSPM already knows that range is used by OperationRegion.
Question is do we really need to expose OperationRegion as _CRS?


> 
> 
> > [...]
> > > > +
> > > > +            Method(_STA, 0) {
> > > > +                If (LEqual(MDNR, Zero)) {
> > > > +                    Return(0x0)
> > > > +                }
> > > > +                /* Leave bit 0 cleared to avoid Windows BSOD */
> > > > +                Return(0xA)
> > > > +            }
> > [...]


-- 
Regards,
  Igor