From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Date: Wed, 29 Apr 2009 18:08:48 +0000 Subject: Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management Message-Id: <49F897B0.4050405@siemens.com> List-Id: References: <20090411172025.32383.77687.stgit@mchn012c.ww002.siemens.net> <20090411172026.32383.7492.stgit@mchn012c.ww002.siemens.net> <0A1FE637C2C7E148B9573BB60CC630E5210713@zch01exm26.fsl.freescale.net> <49F82E09.3070702@siemens.com> <1241025000.24990.51.camel@slate.austin.ibm.com> <49F88ECC.8080005@siemens.com> <1241026635.24990.53.camel@slate.austin.ibm.com> In-Reply-To: <1241026635.24990.53.camel@slate.austin.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Hollis Blanchard Cc: Liu Yu-B13201 , qemu-devel@nongnu.org, kvm-ppc@vger.kernel.org Hollis Blanchard wrote: > On Wed, 2009-04-29 at 19:30 +0200, Jan Kiszka wrote: >> Hollis Blanchard wrote: >>> On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote: >>>> Liu Yu-B13201 wrote: >>>>>> -----Original Message----- >>>>>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org >>>>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] >>>>>> On Behalf Of Jan Kiszka >>>>>> Sent: Sunday, April 12, 2009 1:20 AM >>>>>> To: qemu-devel@nongnu.org >>>>>> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to >>>>>> slot management >>>>>> >>>>>> Fail loudly if we run out of memory slot. >>>>>> >>>>>> Make sure that dirty log start/stop works with consistent >>>>>> memory regions >>>>>> by reporting invalid parameters. This reveals several >>>>>> inconsistencies in >>>>>> the vga code, patch to fix them follows later in this series. >>>>>> >>>>>> And, for simplicity reasons, also catch and report unaligned memory >>>>>> regions passed to kvm_set_phys_mem (KVM works on page basis). >>>>>> >>>>> Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc >>>>> The alignment check in kvm_set_phys_mem prevents pci controller and mpic initializing mmio regions. >>>> What is the alignment of those regions then? None? And do regions of >>>> different types overlap even on the same page? Maybe the check reveals >>>> some deeper conflict /wrt KVM. Can you point me to the involved code files? >>> These PCI controllers make separate calls to >>> cpu_register_physical_memory() for separate callbacks. Reading >>> ppce500_pci_init(), for example: >>> 0xe0008000 -> CFGADDR (4 bytes) >>> 0xe0008004 -> CFGDATA (4 bytes) >>> 0xe0008c00 -> other registers >>> >>> The loop in cpu_register_physical_memory_offset() handles "subpage" >>> registration. However, kvm_set_phys_mem() is called outside that loop, >>> so it gets the non-page-aligned addresses. >>> >> Half-blind shot: >> >> diff --git a/kvm-all.c b/kvm-all.c >> index 32cd636..c2c760e 100644 >> --- a/kvm-all.c >> +++ b/kvm-all.c >> @@ -583,6 +583,9 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr, >> int err; >> >> if (start_addr & ~TARGET_PAGE_MASK) { >> + if (flags >= IO_MEM_UNASSIGNED) { >> + return; >> + } >> fprintf(stderr, "Only page-aligned memory slots supported\n"); >> abort(); >> } >> >> If it works, it likely needs a cleaner approach to handle all cases. > > I don't understand the point. kvm_set_phys_mem() already works without > this new abort() check. This new check is there to catch those cases where someone tries to register regions that are actually incompatible with KVM. IO-MEM regions do not belong into this category (unless they would split existing KVM slots in a non-align way), and so the test likely overshoots here. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LzEDM-00005w-Ca for qemu-devel@nongnu.org; Wed, 29 Apr 2009 14:09:00 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LzEDL-00005Q-MB for qemu-devel@nongnu.org; Wed, 29 Apr 2009 14:08:59 -0400 Received: from [199.232.76.173] (port=37489 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LzEDL-00005K-IU for qemu-devel@nongnu.org; Wed, 29 Apr 2009 14:08:59 -0400 Received: from gecko.sbs.de ([194.138.37.40]:18591) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1LzEDK-0007hU-Sz for qemu-devel@nongnu.org; Wed, 29 Apr 2009 14:08:59 -0400 Message-ID: <49F897B0.4050405@siemens.com> Date: Wed, 29 Apr 2009 20:08:48 +0200 From: Jan Kiszka MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management References: <20090411172025.32383.77687.stgit@mchn012c.ww002.siemens.net> <20090411172026.32383.7492.stgit@mchn012c.ww002.siemens.net> <0A1FE637C2C7E148B9573BB60CC630E5210713@zch01exm26.fsl.freescale.net> <49F82E09.3070702@siemens.com> <1241025000.24990.51.camel@slate.austin.ibm.com> <49F88ECC.8080005@siemens.com> <1241026635.24990.53.camel@slate.austin.ibm.com> In-Reply-To: <1241026635.24990.53.camel@slate.austin.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hollis Blanchard Cc: Liu Yu-B13201 , qemu-devel@nongnu.org, kvm-ppc@vger.kernel.org Hollis Blanchard wrote: > On Wed, 2009-04-29 at 19:30 +0200, Jan Kiszka wrote: >> Hollis Blanchard wrote: >>> On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote: >>>> Liu Yu-B13201 wrote: >>>>>> -----Original Message----- >>>>>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org >>>>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] >>>>>> On Behalf Of Jan Kiszka >>>>>> Sent: Sunday, April 12, 2009 1:20 AM >>>>>> To: qemu-devel@nongnu.org >>>>>> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to >>>>>> slot management >>>>>> >>>>>> Fail loudly if we run out of memory slot. >>>>>> >>>>>> Make sure that dirty log start/stop works with consistent >>>>>> memory regions >>>>>> by reporting invalid parameters. This reveals several >>>>>> inconsistencies in >>>>>> the vga code, patch to fix them follows later in this series. >>>>>> >>>>>> And, for simplicity reasons, also catch and report unaligned memory >>>>>> regions passed to kvm_set_phys_mem (KVM works on page basis). >>>>>> >>>>> Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc >>>>> The alignment check in kvm_set_phys_mem prevents pci controller and mpic initializing mmio regions. >>>> What is the alignment of those regions then? None? And do regions of >>>> different types overlap even on the same page? Maybe the check reveals >>>> some deeper conflict /wrt KVM. Can you point me to the involved code files? >>> These PCI controllers make separate calls to >>> cpu_register_physical_memory() for separate callbacks. Reading >>> ppce500_pci_init(), for example: >>> 0xe0008000 -> CFGADDR (4 bytes) >>> 0xe0008004 -> CFGDATA (4 bytes) >>> 0xe0008c00 -> other registers >>> >>> The loop in cpu_register_physical_memory_offset() handles "subpage" >>> registration. However, kvm_set_phys_mem() is called outside that loop, >>> so it gets the non-page-aligned addresses. >>> >> Half-blind shot: >> >> diff --git a/kvm-all.c b/kvm-all.c >> index 32cd636..c2c760e 100644 >> --- a/kvm-all.c >> +++ b/kvm-all.c >> @@ -583,6 +583,9 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr, >> int err; >> >> if (start_addr & ~TARGET_PAGE_MASK) { >> + if (flags >= IO_MEM_UNASSIGNED) { >> + return; >> + } >> fprintf(stderr, "Only page-aligned memory slots supported\n"); >> abort(); >> } >> >> If it works, it likely needs a cleaner approach to handle all cases. > > I don't understand the point. kvm_set_phys_mem() already works without > this new abort() check. This new check is there to catch those cases where someone tries to register regions that are actually incompatible with KVM. IO-MEM regions do not belong into this category (unless they would split existing KVM slots in a non-align way), and so the test likely overshoots here. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux