Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paul Durrant]

> -----Original Message-----
> From: Stefano Stabellini [mailto:address@hidden
> Sent: 15 October 2014 15:38
> To: Paul Durrant
> Cc: address@hidden; address@hidden; 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 <address@hidden>
> 
> 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 <address@hidden>
> > Cc: Peter Maydell <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> > Cc: Michael Tokarev <address@hidden>
> > Cc: Stefan Hajnoczi <address@hidden>
> > Cc: Stefan Weil <address@hidden>
> > Cc: Olaf Hering <address@hidden>
> > Cc: Gerd Hoffmann <address@hidden>
> > Cc: Alexey Kardashevskiy <address@hidden>
> > Cc: Alexander Graf <address@hidden>
> > ---
> >  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.

> 
> 
> >      }
> >
> > @@ -480,6 +491,7 @@ static void xen_region_add(MemoryListener
> *listener,
> >                             MemoryRegionSection *section)
> >  {
> >      memory_region_ref(section->mr);
> > +
> >      xen_set_memory(listener, section, true);
> >  }
> >
> > @@ -487,9 +499,54 @@ static void xen_region_del(MemoryListener
> *listener,
> >                             MemoryRegionSection *section)
> >  {
> >      xen_set_memory(listener, section, false);
> > +
> >      memory_region_unref(section->mr);
> >  }
> 
> Useless changes?

Yes. Unfortunately I only noticed after posting. I will definitely clean up if 
I re-post.

  Paul