From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51957) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xefcz-0001jO-2p for qemu-devel@nongnu.org; Thu, 16 Oct 2014 03:37:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xefcu-00021q-6U for qemu-devel@nongnu.org; Thu, 16 Oct 2014 03:37:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17970) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xefct-00021N-Ux for qemu-devel@nongnu.org; Thu, 16 Oct 2014 03:37:36 -0400 Message-ID: <543F75AA.6050701@redhat.com> Date: Thu, 16 Oct 2014 09:37:14 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1413364599-7582-1-git-send-email-paul.durrant@citrix.com> <1413364599-7582-3-git-send-email-paul.durrant@citrix.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , Paul Durrant Cc: Olaf Hering , Stefano Stabellini , Alexey Kardashevskiy , Stefan Weil , Michael Tokarev , QEMU Developers , Alexander Graf , Gerd Hoffmann , Stefan Hajnoczi , xen-devel@lists.xenproject.org Il 15/10/2014 19:30, Peter Maydell ha scritto: > On 15 October 2014 11:16, Paul Durrant wrote: >> The ioreq-server API added to Xen 4.5 offers better security than >> the existing Xen/QEMU interface because the shared pages that are >> used to pass emulation request/results back and forth are removed >> from the guest's memory space before any requests are serviced. >> This prevents the guest from mapping these pages (they are in a >> well known location) and attempting to attack QEMU by synthesizing >> its own request structures. Hence, this patch modifies configure >> to detect whether the API is available, and adds the necessary >> code to use the API if it is. > > This commit message doesn't mention it, but presumably this is > all x86-specific given it's in a file which is only used for > x86 Xen? > >> +static void xen_hvm_pre_save(void *opaque) >> +{ >> + XenIOState *state = opaque; >> + >> + /* Stop servicing emulation requests */ >> + xen_set_ioreq_server_state(xen_xc, xen_domid, state->ioservid, 0); >> + xen_destroy_ioreq_server(xen_xc, xen_domid, state->ioservid); >> +} >> + >> +static const VMStateDescription vmstate_xen_hvm = { >> + .name = "xen-hvm", >> + .version_id = 4, >> + .minimum_version_id = 4, > > This is new in upstream so why's it starting at version 4? > >> + .pre_save = xen_hvm_pre_save, >> + .fields = (VMStateField[]) { >> + VMSTATE_END_OF_LIST() >> + }, > > A vmstate which doesn't actually save any state? This seems > rather suspicious... > >> @@ -1060,12 +1185,19 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size, >> xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size, ram_memory); >> >> qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state); >> + vmstate_register(NULL, 0, &vmstate_xen_hvm, state); > > Is the new use of vmstate_register() really necessary? > Usually the state you're saving corresponds to some QOM > device whose vmsd field you can use instead. In this case, it seems like a job for a vmstate change handler. Paolo