From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33782) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab15R-0002SR-Vy for qemu-devel@nongnu.org; Wed, 02 Mar 2016 02:20:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ab15N-0001He-7K for qemu-devel@nongnu.org; Wed, 02 Mar 2016 02:20:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54988) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab15M-0001Ha-TJ for qemu-devel@nongnu.org; Wed, 02 Mar 2016 02:20:41 -0500 Date: Wed, 2 Mar 2016 09:20:33 +0200 From: "Michael S. Tsirkin" Message-ID: <20160302091830-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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56D69307.6040902@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: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? how exactly does 0 indicate no command is supported? is it a bitmask of supported commands? > 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? > > > > > > >>>>+ } else { > >>>>+ /* No function is supported yet. */ > >>>>+ build_append_int_noprefix(out, 1 /* Not Supported */, > >>>>+ sizeof(uint8_t)); > >>>>+ } > >>>>+ > >>>>+ buf_size = cpu_to_le32(out->len); > >>>>+ cpu_physical_memory_write(dsm_mem_addr, &buf_size, sizeof(buf_size)); > >>> > >>>is there a race here? > >>>can guest read this before data is written? > >> > >>I think no. > >> > >>It is the SERIALIZED DSM so there is no race in guest. And the CPU has exited > >>from guest mode when we fill the buffer in the same CPU-context so the guest > >>can not read the buffer at this point also memory-barrier is not needed here. > >> > >>> > >>>>+ cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data, > >>>>+ out->len); > >>> > >>>What is this doing? > >>>Is this actually writing AML bytecode into guest memory? > >> > >>The layout of result written into the buffer is like this: > >>struct NvdimmDsmOut { > >> /* the size of buffer filled by QEMU. */ > >> uint32_t len; > >> uint8_t data[0]; > >>} QEMU_PACKED; > >>typedef struct NvdimmDsmOut NvdimmDsmOut; > >> > >>So the first cpu_physical_memory_write() writes the @len and the second one you > >>pointed out writes the real payload. > > > > > >So either write a function that gets parameters and formats > >buffer, or use a structure to do this. > >Do not open-code formatting and don't mess with > >offsets. > > > >E.g. > > > >struct NvdimmDsmFunc0Out { > > /* the size of buffer filled by QEMU. */ > > uint32_t len; > > uint8_t supported; > >} QEMU_PACKED; > >typedef struct NvdimmDsmFunc0Out NvdimmDsmFunc0Out; > > > > > >And now > > > >NvdimmDsmFunc0Out func0 = { .len = cpu_to_le32(sizeof(func0)); suppported = func == 0; }; > > > >cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof func0); > > > > > >Or if you really insist on using GArray: > > > >build_dsm_out_func0(int function...) > >{ > > uint32_t len; > > uint8_t result; > > > > len = sizeof result; > > if (function == 0) { > > result = 0 /* No function Supported */; > > } else { > > /* No function is supported yet. */ > > result = 1 /* Not Supported */; > > } > > > > build_append_int_noprefix(out, len, sizeof len); > > build_append_int_noprefix(out, result, sizeof result); > > > > assert(out->len < PAGE_SIZE); - is this right? > > cpu_physical_memory_write(dsm_mem_addr, out->data, > > out->len); > >} > > > > > >but I prefer the former ... > > > > Okay, i prefer the former too ;). > > Thank you, Michael! >