From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH 1/2] Clean up MADT Table Creation
Date: Sun, 17 May 2009 21:40:57 +0300
Message-ID: <4A105A39.2080001@redhat.com>
References: <1242443800-22686-1-git-send-email-eak@us.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org, mtosatti@redhat.com,
	vincent@vincent-minet.net, gleb@redhat.com, anthony@codemonkey.ws
To: Beth Kon <eak@us.ibm.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:53601 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752250AbZEQSlG (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 17 May 2009 14:41:06 -0400
In-Reply-To: <1242443800-22686-1-git-send-email-eak@us.ibm.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Beth Kon wrote:
> This patch is based on the recent patch from Vincent Minet. I split Vincent's
> changes into 2 patches (to separate MADT and RSDT table cleanup, as suggested by
> Marcelo) and added a bit to them. And to give credit where it is due, this
> cleanup is also related to the patch Marcelo provided when the HPET addition 
> tripped over the same problem. (Thanks again Marcelo :-) 
>
> This patch moves all the table layout calculations to the same area of
> acpi_bios_init. This prevents corruption problems when, in the middle of
> filling in the tables, the MADT table size grows. The idea is to do all the 
> layout in one section, then fill things in afterwards. It also corrects a 
> problem where the madt table was memset to 0 before the final size of the 
> table had been determined.
>
> Signed-off-by: Beth Kon <eak@us.ibm.com>
>
> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
> index cbd5f15..7f62e4f 100755
> --- a/kvm/bios/rombios32.c
> +++ b/kvm/bios/rombios32.c
> @@ -1665,6 +1665,7 @@ void acpi_bios_init(void)
>  
>      addr = (addr + 7) & ~7;
>      madt_addr = addr;
> +    madt = (void *)(addr);
>      madt_size = sizeof(*madt) +
>          sizeof(struct madt_processor_apic) * MAX_CPUS +
>  #ifdef BX_QEMU
> @@ -1672,7 +1673,11 @@ void acpi_bios_init(void)
>  #else
>          sizeof(struct madt_io_apic);
>  #endif
> -    madt = (void *)(addr);
> +    for ( i = 0; i < 16; i++ ) {
> +        if ( PCI_ISA_IRQ_MASK & (1U << i) ) {
> +            madt_size += sizeof(struct madt_int_override);
> +        }
> +    }
>      addr += madt_size;
>  
>   

You're just duplicating the override creation loop (with its internal 
if); if we update it, we'll have to update this too.

Why not set madt_end = int_override and calculate madt_size = madt_end - 
madt?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.