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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/10/14 15:51, Paul Durrant wrote:
>> -----Original Message-----
>> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
>> Sent: 15 October 2014 15:38
>> To: Paul Durrant
>> Cc: qemu-devel@nongnu.org; xen-devel@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.durrant@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.stabellini@eu.citrix.com>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Michael Tokarev <mjt@tls.msk.ru>
>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>> Cc: Stefan Weil <sw@weilnetz.de>
>>> Cc: Olaf Hering <olaf@aepfle.de>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> Cc: Alexander Graf <agraf@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.

~Andrew