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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 15/10/2014 19:30, Peter Maydell ha scritto:
> On 15 October 2014 11:16, Paul Durrant <paul.durrant@citrix.com> 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