From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36555) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WF3Rn-0002T1-RY for qemu-devel@nongnu.org; Sun, 16 Feb 2014 10:16:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WF3Rh-00016G-Rr for qemu-devel@nongnu.org; Sun, 16 Feb 2014 10:15:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:19823) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WF3Rh-00016B-Ie for qemu-devel@nongnu.org; Sun, 16 Feb 2014 10:15:53 -0500 Date: Sun, 16 Feb 2014 17:20:50 +0200 From: "Michael S. Tsirkin" Message-ID: <20140216152050.GE30056@redhat.com> References: <1391777496-3882-1-git-send-email-imammedo@redhat.com> <1391777496-3882-7-git-send-email-imammedo@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1391777496-3882-7-git-send-email-imammedo@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC 6/9] acpi: consume GPE0 IO resources in PNP0C02 device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, anthony@codemonkey.ws, kraxel@redhat.com On Fri, Feb 07, 2014 at 01:51:33PM +0100, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov > --- > hw/i386/acpi-build.c | 62 ++++++++++++++++++++++++++++++++++++++++++= ++++++++ > 1 files changed, 62 insertions(+), 0 deletions(-) >=20 > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index f0bedbd..ce5f715 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -230,8 +230,13 @@ static void acpi_get_pci_info(PcPciInfo *info) > =20 > #define NameOp 0x08 > #define ScopeOp 0x10 > +#define BufferOp 0x11 > #define DeviceOp 0x82 > =20 > +#define EndTag 0x79 I would say we should use the values from Table 6-162 Small Resource Items. Wrap them in a function to get the full resource. > +#define Decode16 0x1 > +#define Decode10 0x0 > + This is the name from ASL, it's really _DEC value. > static void > build_header(GArray *linker, GArray *table_data, > AcpiTableHeader *h, uint32_t sig, int len, uint8_t rev) > @@ -406,6 +411,25 @@ static void build_append_int(GArray *table, uint32= _t value) > } > } > =20 > +static void build_prepend_int(GArray *array, uint32_t value) > +{ > + GArray *data =3D build_alloc_array(); > + > + build_append_int(data, value); > + g_array_prepend_vals(array, data->data, data->len); > + build_free_array(data); > +} > + > +static void build_buffer(GArray *package, unsigned BufferSize) > +{ > + uint32_t len =3D package->len > BufferSize ? package->len : Buffer= Size; > + > + /* TODO: buffer padding if BufferSize > actual buffer length */ Not sure what this means. So assert here? Or just make it work ... > + build_prepend_int(package, len); > + build_prepend_package_length(package, 0); > + build_prepend_byte(package, BufferOp); prepend is confusing. Just do it like we do for methods: build_append_and_cleanup_buffer(template, buffer); > +} > + > static GArray *build_alloc_method(const char *name, uint8_t arg_count) > { > GArray *method =3D build_alloc_array(); > @@ -523,6 +547,14 @@ static uint32_t encodeEisaId(const char *str) > build_free_array(name); \ > } > =20 > +#define ACPI_BUFFER(ctx, name, min_size, ...) { \ Why pass in min_size? the only reason we have it in existing code was I wanted ACPI to be bit for bit compatible with what seabios generated. We can drop minsize everywhere ... > + GArray *name =3D build_alloc_array(); \ > + __VA_ARGS__; \ > + build_buffer(name, min_size); \ > + build_append_array(ctx, name); \ > + build_free_array(name); \ > +} > + > #define ACPI_NAME(ctx, name) { \ > build_append_byte(ctx, NameOp); \ > build_append_nameseg(ctx, name); \ > @@ -541,6 +573,29 @@ static uint32_t encodeEisaId(const char *str) > build_free_array(name); \ > } > =20 > +#define ACPI_ENDTAG(ctx) { \ > + build_append_byte(ctx, EndTag); \ > + build_append_byte(ctx, 0); \ Confused. what's going on with the checksum here? What fills it in? why don't we add in the correct byte straight away? > +} > + > +#define ACPI_RESOURCE_TEMPLATE(ctx, name, ...) { \ > + ACPI_BUFFER(ctx, name, 0, \ > + __VA_ARGS__; \ > + ACPI_ENDTAG(name); \ Ugh. Not worth the ugliness in my opinion. Just add end tag explicitly. > + ) \ > +} > + > +#define ACPI_IO(ctx, _DEC, _MIN_BASE, _MAX_BASE, _ALN, _LEN) { \ C spec says =E2=80=94 All identifiers that begin with an underscore and either an upp= ercase letter or another underscore are always reserved for any use. =E2=80=94 All identifiers that begin with an underscore are always reserv= ed for use as identifiers so we try to avoid these. > + build_append_byte(ctx, 0x47 /* IO port descriptor */); \ > + build_append_byte(ctx, _DEC); \ > + build_append_byte(ctx, _MIN_BASE & 0xff); \ > + build_append_byte(ctx, (_MIN_BASE >> 8) & 0xff); \ > + build_append_byte(ctx, _MAX_BASE & 0xff); \ > + build_append_byte(ctx, (_MAX_BASE >> 8) & 0xff); \ > + build_append_byte(ctx, _ALN); \ > + build_append_byte(ctx, _LEN); \ > +} > + > /* FACS */ > static void > build_facs(GArray *table_data, GArray *linker, PcGuestInfo *guest_info= ) > @@ -1084,6 +1139,13 @@ build_ssdt(GArray *table_data, GArray *linker, > ACPI_SCOPE(sb_scope, PCI0, > ACPI_DEVICE(PCI0, MRES, > ACPI_NAME(MRES, "_HID"); ACPI_EISAID(MRES, "PNP0C02"); > + ACPI_NAME(MRES, "_CRS"); ACPI_RESOURCE_TEMPLATE(MRES, = RESBUF, > + ACPI_IO(RESBUF, Decode16, > + pm->gpe0_blk, /* _MIN */ > + pm->gpe0_blk, /* _MAX */ > + 0x0, /* _ALN */ > + pm->gpe0_blk_len); /* _LEN */ > + ); > ); > ); Ugh, that's too tricky I'm afraid. how about: crs =3D build_alloc_array(); buf =3D build_alloc_buffer(); build_append_io(buf, .... ); build_append_and_cleanup_buffer(crs, buf); =20 make everything use static functions, not macros. > =20 > --=20 > 1.7.1