Re: [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code

From: "Jan Beulich" <JBeulich@suse.com>
To: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: wei.liu2@citrix.com, andrew.cooper3@citrix.com,
	ian.jackson@eu.citrix.com, xen-devel@lists.xen.org,
	julien.grall@arm.com, zhaoshenglong@huawei.com,
	roger.pau@citrix.com
Subject: Re: [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code
Date: Fri, 08 Jul 2016 04:10:49 -0600	[thread overview]
Message-ID: <577F984902000078000FC87F@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1467745519-9868-3-git-send-email-boris.ostrovsky@oracle.com>

>>> On 05.07.16 at 21:05, <boris.ostrovsky@oracle.com> wrote:
> --- a/tools/firmware/hvmloader/acpi/acpi2_0.h
> +++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
> @@ -449,17 +449,6 @@ struct acpi_20_slit {
>  #define ACPI_2_0_SRAT_REVISION 0x01
>  #define ACPI_2_0_SLIT_REVISION 0x01
>  
> -#pragma pack ()

This must not be removed from here.

> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config, unsigned int physical)
>                   offsetof(struct acpi_20_rsdp, extended_checksum),
>                   sizeof(struct acpi_20_rsdp));
>  
> -    if ( !new_vm_gid(acpi_info) )
> +    if ( !new_vm_gid(&config->ainfo) )
>          goto oom;
>  
> -    acpi_info->com1_present = uart_exists(0x3f8);
> -    acpi_info->com2_present = uart_exists(0x2f8);
> -    acpi_info->lpt1_present = lpt_exists(0x378);
> -    acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
> -    acpi_info->pci_min = pci_mem_start;
> -    acpi_info->pci_len = pci_mem_end - pci_mem_start;
> -    if ( pci_hi_mem_end > pci_hi_mem_start )
> -    {
> -        acpi_info->pci_hi_min = pci_hi_mem_start;
> -        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
> -    }
> +    *(struct acpi_info *)config->ainfop = config->ainfo;

With your new separation of responsibilities - does this really
belong here rather than in the caller? And if it is to stay here, then
I think the pointer field should be made of pointer type, leaving it
to the caller to do whatever casting is necessary when filling in that
field.

> --- /dev/null
> +++ b/tools/firmware/hvmloader/acpi/libacpi.h
> @@ -0,0 +1,80 @@
> +/******************************************************************************
> + * libacpi.h
> + * 
> + * libacpi interfaces
> + * 
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + */
> +
> +
> +#ifndef __LIBACPI_H__
> +#define __LIBACPI_H__
> +
> +#pragma pack ()

I guess this is going to be unneeded once the removal from acpi2_0.h
gets undone?

> +/*
> + * This must match the Field("BIOS"....) definition in the DSDT.
> + */

This is a single line comment. And considering what this comment
says I wonder whether the following structure definition wouldn't
better be wrapped in a #pragma pack(1) / #pragma pack() pair,
with ...

> +struct acpi_info {
> +    uint8_t  com1_present:1;    /* 0[0] - System has COM1? */
> +    uint8_t  com2_present:1;    /* 0[1] - System has COM2? */
> +    uint8_t  lpt1_present:1;    /* 0[2] - System has LPT1? */
> +    uint8_t  hpet_present:1;    /* 0[3] - System has HPET? */

explicit (unnamed) padding added here.

> +struct acpi_config {
> +    const unsigned char *dsdt_anycpu;
> +    unsigned int dsdt_anycpu_len;
> +    const unsigned char *dsdt_15cpu;
> +    unsigned int dsdt_15cpu_len;
> +
> +    /* May have some fields updated by acpi_build_table() */
> +    struct acpi_info ainfo;
> +    /*
> +     * Address where acpi_info should be placed.
> +     * This must match the OperationRegion(BIOS, SystemMemory, ....)
> +     * definition in the DSDT
> +     */
> +    unsigned int ainfop;

What is the "a" prefix of these two fields good for? Considering the
name of the structure I don't see a need for any prefix. But if you
absolutely want one, then acpi_ please.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel