From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32889) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab10V-0007hu-OQ for qemu-devel@nongnu.org; Wed, 02 Mar 2016 02:15:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ab10Q-0008Lc-N9 for qemu-devel@nongnu.org; Wed, 02 Mar 2016 02:15:39 -0500 Received: from mga03.intel.com ([134.134.136.65]:38646) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab10Q-0008LV-BN for qemu-devel@nongnu.org; Wed, 02 Mar 2016 02:15:34 -0500 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> From: Xiao Guangrong Message-ID: <56D69307.6040902@linux.intel.com> Date: Wed, 2 Mar 2016 15:15:19 +0800 MIME-Version: 1.0 In-Reply-To: <20160302081841-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" 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 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. 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. > > >>>> + } 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!