Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Feb 18, 2014 at 12:10:27PM +0100, Igor Mammedov wrote:
> On Mon, 17 Feb 2014 13:02:18 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Feb 17, 2014 at 11:33:23AM +0100, Igor Mammedov wrote:
> > > On Sun, 16 Feb 2014 17:53:45 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > > > > Since introduction of PCIHP, it became problematic to
> > > > > punch hole in PCI0._CRS statically since PCI hotplug
> > > > > region size became runtime changeable.
> > > > 
> > > > What makes it runtime changeable?
> > > piix machine "acpi-pci-hotplug-with-bridge-support=on" effectively
> > > changes size of pcihp MMIO region
> > 
> > It adds 4 bytes. Let's just reserve a reasonably sized region,
> > like 512 bytes.
> Reserve where and for how log it will be enough?
> Eventually we will continue punching holes or increasing size of
> this one big hole.

I agree this might become necessary.  But why not keep it simple while
we can? The advantage of ACPI in qemu is we can fix it without
touching guest code. Let's use it.

So if there's just pm_io_base, we can just write
it in ASL and patch in that value.

> 
> Q35 adds more to the problem with guest programmable pm_io_base,
> we can't statically punch hole for it.

Aha. That's a good point.
But I still worry how we'll handle e.g. pvpanic.
Paolo had an idea to scan the memory range,
and punch holes for each section, making an
exception for regions owned by PCI devices.
I thought it would work originally too, but it
seems that e.g. VGA range is also owned by pci
devices sometimes.


> > 
> > > > 
> > > > > So replace static hole punching with dynamically consumed
> > > > > resources in a child device on PCI0 bus. i.e generate
> > > > > PNP0C02 device as a child of PCI0 bus at runtime and
> > > > > consume GPE0, PCI/CPU hotplug IO resources in it instead
> > > > > of punching holes in static PCI0._CRS.
> > > > 
> > > > It seems that we are being too exact with
> > > > IO resources here.
> > > > Can't we roughly reserve 0xae00 to 0xafff
> > > > and be done with it?
> > > that would be easiest way for this specific case if we could agree
> > > for ranges on PIIX/Q35 machines.
> > > 
> > > But I also use it as excuse to introduce ASL like macros so that
> > > building dynamic SSDT would be easier i.e. replace template patching
> > > with single place where dynamic device is defined and its values
> > > are set in simple and straightforward manner.
> > 
> > We don't need excuses really :)
> > But the approach itself needs some work IMHO, in particular
> > ASL like syntax by using macros and varargs is not something to strive
> > for IMHO :)
> > 
> > Why don't we just pass GArrays around?
> > 
> > crs = build_alloc_crs();
> > build_append_resource(crs, ....)
> 
> I don't like much macros myself, but compared to alternative
> approach ^^^ (I've tried it) it's still harder to read/write
> ASL constructs right.
> 
> I think that creating AML using ASL like flow and primitives
> is better than building it using custom APIs, since it looks
> like it was documented in spec.
> Macros+vararg provide a necessary degree of flexibility to
> create AML using ASL like flow and constructs.

Only you lose all type safety. When compiling ASL with IASL
at least it does some type checking.

> > 
> > 
> > Or if you really want this, using array of GArray:
> > 
> > build_append_crs(ssdt,
> > 		{
> > 			build_alloc_resource(...),
> > 			build_alloc_resource(...),
> > 			build_alloc_resource(...),
> > 			build_alloc_resource(...),
> > 			NULL
> > 		})
> that would work if there was a way to specify arbitrary statements
> inside embedded array (like in the last hunk of [9/9]).

In C we have functions for this :)
Last chunk:

-                    if (pm->pcihp_io_base) {
-                        ACPI_IO(RESBUF, Decode16,
-                                pm->pcihp_io_base, /* _MIN */
-                                pm->pcihp_io_base, /* _MAX */
-                                0x0,               /* _ALN */
-                                pm->pcihp_io_len); /* _LEN */
-                    }


+	build_io_resource_16(pm->pcihp_io_base, pm->pcihp_io_base,
			     0x0, pm->pcihp_io_len);


Which actually makes one think. Isn't _MAX wrong here?
I'd expect pm->pcihp_io_base + pm->pcihp_io_len - 1

I would also key this off pcihp_io_len.

> > 
> > 
> > > > > Tested with Windows XPsp3, Vista, Windows Server 2003, 2008, 2012r2.
> > > > > 
> > > > > PS:
> > > > > Series adds several ASL like macros to simplify
> > > > > code for dynamic generation of AML structures.
> > > > > 
> > > > > Igor Mammedov (9):
> > > > >   Revert "pc: Q35 DSDT: exclude CPU hotplug IO range from PCI bus
> > > > >     resources"
> > > > >   Revert "pc: PIIX DSDT: exclude CPU/PCI hotplug & GPE0 IO range from
> > > > >     PCI bus resources"
> > > > >   Partial revert "pc: ACPI: expose PRST IO range via _CRS"
> > > > >   acpi: replace opencoded opcodes with defines
> > > > >   acpi: add PNP0C02 to PCI0 bus
> > > > >   acpi: consume GPE0 IO resources in PNP0C02 device
> > > > >   acpi: consume CPU hotplug IO resource in PNP0C02 device
> > > > >   pcihp: expose PCI hotplug MMIO base/length as properties of piix4pm
> > > > >   acpi: consume PCIHP IO resource in PNP0C02 device
> > > > > 
> > > > >  hw/acpi/pcihp.c                   |   28 ++++++
> > > > >  hw/acpi/piix4.c                   |    1 +
> > > > >  hw/i386/acpi-build.c              |  177 ++++++++++++++++++++++++++++++++++--
> > > > >  hw/i386/acpi-dsdt-cpu-hotplug.dsl |   11 ---
> > > > >  hw/i386/acpi-dsdt-pci-crs.dsl     |   15 +++-
> > > > >  hw/i386/acpi-dsdt.dsl             |   39 --------
> > > > >  hw/i386/q35-acpi-dsdt.dsl         |   16 ----
> > > > >  include/hw/acpi/pcihp.h           |    4 +
> > > > >  8 files changed, 214 insertions(+), 77 deletions(-)
> > > 
> > > 
> > > -- 
> > > Regards,
> > >   Igor