From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35694) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab1EG-0006ud-Fl for qemu-devel@nongnu.org; Wed, 02 Mar 2016 02:29:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ab1EC-0003GL-DF for qemu-devel@nongnu.org; Wed, 02 Mar 2016 02:29:52 -0500 Received: from mga11.intel.com ([192.55.52.93]:52834) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab1EC-0003Fx-3W for qemu-devel@nongnu.org; Wed, 02 Mar 2016 02:29:48 -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> <56D69307.6040902@linux.intel.com> <20160302091830-mutt-send-email-mst@redhat.com> From: Xiao Guangrong Message-ID: <56D6965D.9010906@linux.intel.com> Date: Wed, 2 Mar 2016 15:29:33 +0800 MIME-Version: 1.0 In-Reply-To: <20160302091830-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 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. 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.