From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52899) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab2OG-0003fu-8f for qemu-devel@nongnu.org; Wed, 02 Mar 2016 03:44:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ab2OD-0005FD-12 for qemu-devel@nongnu.org; Wed, 02 Mar 2016 03:44:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37550) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab2OC-0005F2-Pf for qemu-devel@nongnu.org; Wed, 02 Mar 2016 03:44:12 -0500 Date: Wed, 2 Mar 2016 10:44:06 +0200 From: "Michael S. Tsirkin" Message-ID: <20160302103829-mutt-send-email-mst@redhat.com> References: <1456829771-71553-1-git-send-email-guangrong.xiao@linux.intel.com> <1456829771-71553-9-git-send-email-guangrong.xiao@linux.intel.com> <20160301190240-mutt-send-email-mst@redhat.com> <56D65E42.1050108@linux.intel.com> <20160302081841-mutt-send-email-mst@redhat.com> <56D69307.6040902@linux.intel.com> <20160302091830-mutt-send-email-mst@redhat.com> <56D6965D.9010906@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56D6965D.9010906@linux.intel.com> Subject: Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong Cc: ehabkost@redhat.com, kvm@vger.kernel.org, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On Wed, Mar 02, 2016 at 03:29:33PM +0800, Xiao Guangrong wrote: > > > On 03/02/2016 03:20 PM, Michael S. Tsirkin wrote: > >On Wed, Mar 02, 2016 at 03:15:19PM +0800, Xiao Guangrong wrote: > >> > >> > >>On 03/02/2016 02:36 PM, Michael S. Tsirkin wrote: > >>>On Wed, Mar 02, 2016 at 11:30:10AM +0800, Xiao Guangrong wrote: > >>>> > >>>> > >>>>On 03/02/2016 01:09 AM, Michael S. Tsirkin wrote: > >>>> > >>>>> > >>>>>Can't guest trigger this? > >>>>>If yes, don't put such code in production please: > >>>>>this will fill up disk on the host. > >>>>> > >>>> > >>>>Okay, the evil guest can read the IO port freely. I will use nvdimm_debug() instead. > >>>> > >>>>> > >>>>>> > >>>>>> static void > >>>>>> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > >>>>>> { > >>>>>>+ NvdimmDsmIn *in; > >>>>>>+ GArray *out; > >>>>>>+ uint32_t buf_size; > >>>>>>+ hwaddr dsm_mem_addr = val; > >>>>>>+ > >>>>>>+ nvdimm_debug("dsm memory address %#lx.\n", dsm_mem_addr); > >>>>>>+ > >>>>>>+ /* > >>>>>>+ * The DSM memory is mapped to guest address space so an evil guest > >>>>>>+ * can change its content while we are doing DSM emulation. Avoid > >>>>>>+ * this by copying DSM memory to QEMU local memory. > >>>>>>+ */ > >>>>>>+ in = g_malloc(TARGET_PAGE_SIZE); > >>> > >>>ugh. manual memory management :( > >>> > >> > >>Hmm... Or use GArray? But it is :) > >> > >>>>>>+ cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE); > >>> > >>>is there a requirement address is aligned? > >>>if not this might cross page and crash qemu. > >>>better read just what you need. > >>> > >> > >>Yes, this memory is allocated by BIOS and we asked it to align the memory > >>with PAGE_SIZE: > >> > >> bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE, > >> false /* high memory */); > >> > >>>>>>+ > >>>>>>+ le32_to_cpus(&in->revision); > >>>>>>+ le32_to_cpus(&in->function); > >>>>>>+ le32_to_cpus(&in->handle); > >>>>>>+ > >>>>>>+ nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision, > >>>>>>+ in->handle, in->function); > >>>>>>+ > >>>>>>+ out = g_array_new(false, true /* clear */, 1); > >>> > >>>export build_alloc_array then, and reuse? > >> > >>It is good to me, but as your suggestions, this code will be removed. > >> > >>> > >>>>>>+ > >>>>>>+ /* > >>>>>>+ * function 0 is called to inquire what functions are supported by > >>>>>>+ * OSPM > >>>>>>+ */ > >>>>>>+ if (in->function == 0) { > >>>>>>+ build_append_int_noprefix(out, 0 /* No function Supported */, > >>>>>>+ sizeof(uint8_t)); > >>> > >>>What does this mean? Same comment here and below ... > >> > >>If its the function 0, we return 0 that indicates no command is supported yet. > > > >first comment says no function supported. > >clearly function 0 is supported, is it not? > > Yep, the comment is not clear here. It should be "No function Supported other > than function 0 " > > Function 0 is the common function supported by all DSMs to inquire what functions are > supported by this DSM. > > >how exactly does 0 indicate no command is supported? > >is it a bitmask of supported commands? > > It is a bitmask. The spec said: > > If Function Index is zero, the return is a buffer containing one bit for each function > index, starting with zero. Why not start from 1? So 0x1 - function 1 supported, 0x2 - function 2, 0x4 - function 3 etc. > Bit 0 indicates whether there is support for any functions other > than function 0 for the specified UUID and Revision ID. If set to zero, no functions are > supported (other than function zero) for the specified UUID and Revision ID. > > > >>Other wise, it is a command request from a evil guest regardless of the result > >>returned by function 0, we return the status code 1 to indicates this command > >>is not supported. > > > >is command same as function? > > Yes.