[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH 4/8] hw: arm: Carry RSDP specific data through Acp
From: |
Samuel Ortiz |
Subject: |
Re: [Qemu-arm] [PATCH 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData |
Date: |
Wed, 28 Nov 2018 11:05:36 +0100 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
Hi Michael,
On Tue, Nov 27, 2018 at 10:26:30PM -0500, Michael S. Tsirkin wrote:
> On Tue, Nov 27, 2018 at 05:27:49PM +0100, Igor Mammedov wrote:
> > On Tue, 27 Nov 2018 16:42:18 +0100
> > Samuel Ortiz <address@hidden> wrote:
> >
> > > Hi Igor,
> > >
> > > On Tue, Nov 27, 2018 at 04:25:51PM +0100, Igor Mammedov wrote:
> > > > On Mon, 26 Nov 2018 17:29:37 +0100
> > > > Samuel Ortiz <address@hidden> wrote:
> > > >
> > > > > That will allow us to generalize the ARM build_rsdp() routine to
> > > > > support
> > > > > both legacy RSDP (The current i386 implementation) and extended RSDP
> > > > > (The ARM implementation).
> > > > >
> > > > > Signed-off-by: Samuel Ortiz <address@hidden>
> > > > > ---
> > > > > include/hw/acpi/acpi-defs.h | 11 +++++++++++
> > > > > hw/arm/virt-acpi-build.c | 27 ++++++++++++++++++++++-----
> > > > > 2 files changed, 33 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > > > index af8e023968..e7fd24c6c5 100644
> > > > > --- a/include/hw/acpi/acpi-defs.h
> > > > > +++ b/include/hw/acpi/acpi-defs.h
> > > > > @@ -53,6 +53,17 @@ struct AcpiRsdpDescriptor { /* Root System
> > > > > Descriptor Pointer */
> > > > > } QEMU_PACKED;
> > > > > typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
> > > > >
> > > > > +typedef struct AcpiRsdpData {
> > > > > + uint8_t oem_id[6]; /* OEM identification */
> > > > > + uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */
> > > > > +
> > > > > + unsigned *rsdt_tbl_offset;
> > > > > + unsigned *xsdt_tbl_offset;
> > > > > +} AcpiRsdpData;
> > > > > +
> > > >
> > > > > +#define ACPI_RSDP_REV_1 0
> > > > > +#define ACPI_RSDP_REV_2 2
> > > > it's one time used spec defined values so just use values directly
> > > > in place with a comment, so reader won't have to jump around code
> > > > when comparing to spec.
> > > It's also used in the ACPI tests fix patch.
> > it's better to use in test it's own version (we just opencode them there)
> > see fadt_fetch_facs_and_dsdt_ptrs()/sanitize_fadt_ptrs()
> > same applies for length.
> > that way if we break it in qemu's code test would catch the thing
> >
> > > Also the 0 for revision 1 is a little confusing, I feel the above
> > > definition is clearer.
> > that's confusion is in the spec, so we just mimic it, no need to add more
> > on top
>
> To be more precise, there is a huge number of constants in ACPI
> such that adding defines for them all would be a huge burden,
I find that defining a set of well named constants is a lot less painful
than maintaining code with at least the same amount of hard coded
constants. That's a personal opinion, for sure.
> and will not make it easy to check values against the
> spec at all (case in point ACPI_RSDP_REV_2 is actually wrong,
> 2 is version 3 and up).
I may be misreading the spec, but I understand 0 is for ACPI 1.0 and 2
is for ACPI 2.0+. The latest spec is a little confusing with regard to
this field, but when looking at the 2.0a ACPI spec for RSDP:
"The ACPI version 1.0 revision number of this table is zero. The ACPI 2.0
value for this field is 2."
> Thus the preferred style is to add a comment near the value
> matching spec name verbatim, so one can copy it and
> look it up in the spec. Sometimes one needs to reference
> specific spec version.
>
> 0 /* Revision: ACPI version 1.0 */
>
> and
>
> 1 /* Revision: ACPI 2.0 */
>
> and
>
> 2 /* Revision: ACPI 3.0a */
>
> For style consistency, if the value is used multiple times, we avoid
> duplication by using an inline function and not a macro.
Not entirely sure how this materializes. Do you mean that e.g. if I want
to check for an RSDP revision I'd have to define inline functions of
that kind:
bool is_rsdp_revision_0(uint8_t *rsdp_table);
bool is_rsdp_revision_2(uint8_t *rsdp_table);
or do you have something else in mind?
Cheers,
Samuel.
- Re: [Qemu-arm] [PATCH 2/8] hw: arm: acpi: Fix incorrect checksums in RSDP, (continued)
- [Qemu-arm] [PATCH 3/8] hw: i386: Use correct RSDT length for checksum, Samuel Ortiz, 2018/11/26
- [Qemu-arm] [PATCH 5/8] hw: arm: Convert the RSDP build to the buid_append_foo() API, Samuel Ortiz, 2018/11/26
- [Qemu-arm] [PATCH 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData, Samuel Ortiz, 2018/11/26
- Re: [Qemu-arm] [Qemu-devel] [PATCH 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData, Philippe Mathieu-Daudé, 2018/11/26
- Re: [Qemu-arm] [PATCH 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData, Igor Mammedov, 2018/11/27
- Re: [Qemu-arm] [PATCH 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData, Samuel Ortiz, 2018/11/28
- Re: [Qemu-arm] [Qemu-devel] [PATCH 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData, Samuel Ortiz, 2018/11/28
- Re: [Qemu-arm] [Qemu-devel] [PATCH 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData, Igor Mammedov, 2018/11/28
[Qemu-arm] [PATCH 7/8] hw: acpi: Export and share the ARM RSDP build, Samuel Ortiz, 2018/11/26
[Qemu-arm] [PATCH 6/8] hw: arm: Support both legacy and current RSDP build, Samuel Ortiz, 2018/11/26
[Qemu-arm] [PATCH 8/8] hw: acpi: Remove AcpiRsdpDescriptor and fix tests, Samuel Ortiz, 2018/11/26