On Wed, 15 Oct 2014, Andrew Cooper wrote:
> On 15/10/14 15:51, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Stefano Stabellini [mailto:stefano.stabell...@eu.citrix.com]
> >> Sent: 15 October 2014 15:38
> >> To: Paul Durrant
> >> Cc: qemu-devel@nongnu.org; xen-de...@lists.xenproject.org; Stefano
> >> Stabellini; Peter Maydell; Paolo Bonzini; Michael Tokarev; Stefan Hajnoczi;
> >> Stefan Weil; Olaf Hering; Gerd Hoffmann; Alexey Kardashevskiy; Alexander
> >> Graf
> >> Subject: Re: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
> >>
> >> On Wed, 15 Oct 2014, 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.
> >>>
> >>> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> >> The patch is OK, so you can add my Acked-by.
> >> I have a couple of minor comments below. If you need to repost it then
> >> would be nice if you could address them.
> >>
> >>
> >>> Cc: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
> >>> Cc: Peter Maydell <peter.mayd...@linaro.org>
> >>> Cc: Paolo Bonzini <pbonz...@redhat.com>
> >>> Cc: Michael Tokarev <m...@tls.msk.ru>
> >>> Cc: Stefan Hajnoczi <stefa...@redhat.com>
> >>> Cc: Stefan Weil <s...@weilnetz.de>
> >>> Cc: Olaf Hering <o...@aepfle.de>
> >>> Cc: Gerd Hoffmann <kra...@redhat.com>
> >>> Cc: Alexey Kardashevskiy <a...@ozlabs.ru>
> >>> Cc: Alexander Graf <ag...@suse.de>
> >>> ---
> >>> configure | 29 ++++++
> >>> include/hw/xen/xen_common.h | 222
> >> +++++++++++++++++++++++++++++++++++++++++++
> >>> trace-events | 8 ++
> >>> xen-hvm.c | 174 +++++++++++++++++++++++++++++----
> >>> 4 files changed, 412 insertions(+), 21 deletions(-)
> >>>
> >> [...]
> >>
> >>> diff --git a/xen-hvm.c b/xen-hvm.c
> >>> index 05e522c..0bbbf2a 100644
> >>> --- a/xen-hvm.c
> >>> +++ b/xen-hvm.c
> >>> @@ -62,9 +62,6 @@ static inline ioreq_t
> >> *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
> >>> }
> >>> # define FMT_ioreq_size "u"
> >>> #endif
> >>> -#ifndef HVM_PARAM_BUFIOREQ_EVTCHN
> >>> -#define HVM_PARAM_BUFIOREQ_EVTCHN 26
> >>> -#endif
> >>>
> >>> #define BUFFER_IO_MAX_DELAY 100
> >>>
> >>> @@ -78,6 +75,7 @@ typedef struct XenPhysmap {
> >>> } XenPhysmap;
> >>>
> >>> typedef struct XenIOState {
> >>> + ioservid_t ioservid;
> >>> shared_iopage_t *shared_page;
> >>> buffered_iopage_t *buffered_io_page;
> >>> QEMUTimer *buffered_io_timer;
> >>> @@ -92,6 +90,8 @@ typedef struct XenIOState {
> >>>
> >>> struct xs_handle *xenstore;
> >>> MemoryListener memory_listener;
> >>> + MemoryListener io_listener;
> >>> + DeviceListener device_listener;
> >>> QLIST_HEAD(, XenPhysmap) physmap;
> >>> hwaddr free_phys_offset;
> >>> const XenPhysmap *log_for_dirtybit;
> >>> @@ -442,12 +442,23 @@ static void xen_set_memory(struct
> >> MemoryListener *listener,
> >>> bool log_dirty = memory_region_is_logging(section->mr);
> >>> hvmmem_type_t mem_type;
> >>>
> >>> + if (section->mr == &ram_memory) {
> >>> + return;
> >>> + } else {
> >>> + if (add) {
> >>> + xen_map_memory_section(xen_xc, xen_domid, state->ioservid,
> >>> + section);
> >>> + } else {
> >>> + xen_unmap_memory_section(xen_xc, xen_domid, state->ioservid,
> >>> + section);
> >>> + }
> >>> + }
> >>> if (!memory_region_is_ram(section->mr)) {
> >>> return;
> >>> }
> >>>
> >>> - if (!(section->mr != &ram_memory
> >>> - && ( (log_dirty && add) || (!log_dirty && !add)))) {
> >>> + if (!(log_dirty && add) && !(!log_dirty && !add)) {
> >>> return;
> >> if (!((log_dirty && add) || (!log_dirty && !add)))
> >>
> > I'm not sure which is better TBH.
>
> I set simplification problems like this to my Part 1a digital
> electronics supervisees.
>
> This is "if (!(log_dirty ^ add))" as they are both booleans, and reads
> rather more easily that either of the above.
you are funny
