qemu-devel
[Top][All Lists]
Advanced

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

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


From: Igor Mammedov
Subject: Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources
Date: Tue, 18 Feb 2014 17:30:24 +0100

On Tue, 18 Feb 2014 13:33:17 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> 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" <address@hidden> 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" <address@hidden> 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.
from ACPI pov, I think we need to punch holes or define
_CRS in respective consuming device explicitly, doing it
automatically looping through MemoryRegions going to
be tricky.

but for SeaBIOS to find out what ports are reserved
looping might be a right way, if it needs this information.

> 
> > > 
> > > > > 
> > > > > > 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.
short of embedded IASL solution, it's the same type safety as we
could provide with C, with extra checks inside primitives.
So that API user won't have to worry about it.

> 
> > > 
> > > 
> > > 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 :)
I don't say it's impossible to write using C functions to express the same
logic, but it would be harder to read/write code that way. For example below
snippet would look like:

GArray *return_array_if(bool, garray){
 if (bool == true) return garray;
 return empty_garray;
}

build_append_crs(ssdt,
                {
                        build_alloc_resource(...),
                        ...,
                        return_array_if(have_pcihp,
                                        build_alloc_resource(...)
                        ),

and that will turn out in explosive growth of "return_array_if" like
functions to accommodate every tweak we would to do.
On contrary, macros with inline code make it simpler/easier
to write/read code, almost like embedded ASL.

> 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);
That's not the same, Do you mean:

build_io_resource_16_IF(have_pcihp, ....);

and returning empty GArray if have_pcihp == false?

If it's the case than that's what I'm trying to avoid,
in favor of pretty well described in spec ASL like primitives
with some C snippets to tweak generated AML object content.

If we use C++11 lambda feature we could achieve almost
the same simple to use API as macro based, but I guess it's
not an option yet.

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

nope,
"
19.5.62 IO (IO Resource Descriptor Macro)
...
AddressMax evaluates to a 16-bit integer that specifies the maximum acceptable 
STARTING address for
the I/O range. 
"

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




reply via email to

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