From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34141) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fZyFu-0005UB-2d for qemu-devel@nongnu.org; Mon, 02 Jul 2018 08:48:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fZyFq-00064h-4U for qemu-devel@nongnu.org; Mon, 02 Jul 2018 08:48:34 -0400 References: <20180702093755.7384-1-david@redhat.com> <20180702093755.7384-5-david@redhat.com> <20180702123141.4a499198@redhat.com> <1bb74240-d1ce-7a1e-69df-f2c7c70d1fd9@redhat.com> <20180702144422.18f451c9@redhat.com> From: David Hildenbrand Message-ID: <823aa15f-c73b-4e15-4562-3f0bf7007dc4@redhat.com> Date: Mon, 2 Jul 2018 14:47:38 +0200 MIME-Version: 1.0 In-Reply-To: <20180702144422.18f451c9@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 4/4] pc-dimm: assign and verify the "addr" property during pre_plug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: Eduardo Habkost , "Michael S . Tsirkin" , Stefan Weil , Xiao Guangrong , qemu-devel@nongnu.org, Alexander Graf , qemu-ppc@nongnu.org, Paolo Bonzini , David Gibson , Richard Henderson On 02.07.2018 14:44, Igor Mammedov wrote: > On Mon, 2 Jul 2018 12:39:43 +0200 > David Hildenbrand wrote: > >> On 02.07.2018 12:31, Igor Mammedov wrote: >>> On Mon, 2 Jul 2018 11:37:55 +0200 >>> David Hildenbrand wrote: >>> >>>> We can assign and verify the slot before realizing and trying to plug. >>> s/slot/"addr"/ >>> >>>> reading/writing the address property should never fail for DIMMs, so let's >>>> reduce error handling a bit by using &error_abort. Getting access to the >>>> memory region now might however fail. So forward errors from >>>> get_memory_region() properly. >>>> >>>> As all memory devices should use the alignment of the underlying memory >>>> region for guest physical address asignment, do detection of the >>>> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the >>>> alignment for compatibility handling. >>>> >>>> Signed-off-by: David Hildenbrand >>> with commit message fixup, >>> >>> Reviewed-by: Igor Mammedov >>> --- >>> For future reference, I don't really like 2 things about patch >>> 1: mixes both error handling and functional changes (should be separate patches) >>> 2: that property setter may crash QEMU at preplug stage >>> where it should gracefully error out (so put proper error check >>> as follow up patch or respin this one maybe taking in account #1.) >> >> Thanks for the review. >> >> 1. could have been factored out into a separate patch. >> >> Regarding 2, I won't respin as long there is nothing else to take care >> of. As long as we are in the pc-dimm world and we have static >> properties, I don't see any reason to add error handling that cannot >> happen. It will be different once we factor out more stuff into memory >> device code. I assume you have a different opinion about that, but I >> consider these nits. > it's not really nits. it happens to work now but static properties are > just current impl. detail which can easily change without amending > *plug handlers. > It's cleaner/safer to handle errors properly and keeping device model > separate from machine helpers as much as possible from maintainer pov, > even at expense of extra boiler plate. > > I expect you to re-introduce proper error checking when you > factor out memory_device_pre_plug() part /it's condition for my ack > on this patch/. > Yes, already contained. -- Thanks, David / dhildenb