From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Lalancette Subject: Re: [PATCH]: Fix memory corruption in-kernel IOAPIC emulation Date: Wed, 30 Jan 2008 11:39:12 -0500 Message-ID: <47A0A830.8040900@redhat.com> References: <479FB5C6.6060204@redhat.com> <47A04BB3.7020302@qumranet.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------090005010905010609060709" Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Avi Kivity Return-path: In-Reply-To: <47A04BB3.7020302-atKUWr5tajBWk0Htik3J/w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org This is a multi-part message in MIME format. --------------090005010905010609060709 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Avi Kivity wrote: > > Excellent catch, but the fix is wrong. Instead of partially restoring > the ioapic state in the kernel, you should fully save it in qemu. > > This is a trap that many fall into: considering kvm and qemu as one > entity and making sure they work well together. We need to make sure > that kvm and libkvm are useful for other userspace programs as well. > Actually, let me ask a question here. It seems to me that there are two ways I could go about this: 1) Change the save protocol so that it saves the relevant information (i.e. base_address and irr), and then the restore protocol so it actually pulls this stuff off the wire. This solution seems more "right" to me, but it has the downside that we are changing the over-the-wire information, which will break migration between old and new versions of qemu. 2) Just change the restore protocol so that we properly fill in the missing fields with real, hard-coded values instead of just random memory. This has the benefit that it doesn't change the protocol, but has the downside that we won't reflect changes to base_address or irr 100% properly. In practice, this doesn't seem like a big deal since there doesn't currently seem to be a way to change base_address anyway, and losing the irr doesn't seem to be catastrophic (although I'm not 100% certain about that). The attached patch implements 2); does anyone have an opinion on which way to go here? Chris Lalancette --------------090005010905010609060709 Content-Type: text/x-patch; name="kvm-60-fix-ioapic-in-userland.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="kvm-60-fix-ioapic-in-userland.patch" diff --git a/qemu/hw/apic.c b/qemu/hw/apic.c index c26a18d..6b41d7a 100644 --- a/qemu/hw/apic.c +++ b/qemu/hw/apic.c @@ -60,6 +60,7 @@ /* FIXME: it's now hard coded to be equal with KVM_IOAPIC_NUM_PINS */ #define IOAPIC_NUM_PINS 0x18 +#define IOAPIC_DEFAULT_BASE_ADDRESS 0xfec00000 #define ESR_ILLEGAL_ADDRESS (1 << 7) @@ -1143,8 +1144,11 @@ static void kvm_kernel_ioapic_load_from_user(IOAPICState *s) chip.chip_id = KVM_IRQCHIP_IOAPIC; kioapic = &chip.chip.ioapic; - kioapic->id = s->id; + kioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS; kioapic->ioregsel = s->ioregsel; + kioapic->id = s->id; + kioapic->irr = 0; + kioapic->pad = 0; for (i = 0; i < IOAPIC_NUM_PINS; i++) { kioapic->redirtbl[i].bits = s->ioredtbl[i]; } --------------090005010905010609060709 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ --------------090005010905010609060709 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ kvm-devel mailing list kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org https://lists.sourceforge.net/lists/listinfo/kvm-devel --------------090005010905010609060709--