Re: [Xen-devel] [PATCH v7 5/5] x86/ioreq server: Synchronously reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.03.17 at 09:42, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> On 3/11/2017 12:59 AM, Andrew Cooper wrote:
>> On 08/03/17 15:33, Yu Zhang wrote:
>>> --- a/xen/arch/x86/hvm/dm.c
>>> +++ b/xen/arch/x86/hvm/dm.c
>>> @@ -288,6 +288,7 @@ static int inject_event(struct domain *d,
>>>       return 0;
>>>   }
>>>   
>>> +#define DMOP_op_mask 0xff
>>>   static int dm_op(domid_t domid,
>>>                    unsigned int nr_bufs,
>>>                    xen_dm_op_buf_t bufs[])
>>> @@ -315,10 +316,8 @@ static int dm_op(domid_t domid,
>>>       }
>>>   
>>>       rc = -EINVAL;
>>> -    if ( op.pad )
>>> -        goto out;
>>>   
>>> -    switch ( op.op )
>>> +    switch ( op.op & DMOP_op_mask )
>> Nack to changes like this.  HVMop continuations only existed in this
>> form because we had to retrofit it to longterm-stable ABIs in the past,
>> and there were literally no free bits anywhere else.
> 
> Thank you, Andrew. Frankly, I'm not surprised to see disagreement from 
> you and Jan
> on this interface. :)
> 
> Using the HVMop like continuation is a hard choice for me. I saw you 
> removed these
> from the DMops, and I also agree that the new interface is much cleaner.
> 
> I noticed there are other 2 DMops to continuable, the set_mem_type and 
> the modified_mem.
> Both definitions of their structure have fields like first_gfn and nr 
> which can be updated to
> be used in the hypercall continuation.
> But for map_mem_type_to_ioreq_server, however we do not need a gfn 
> number exposed in
> this interface(and I do not think exposing a gfn is correct), it is only 
> used when an ioreq server
> detaches from p2m_ioreq_server and the p2m sweeping is not finished. So 
> I changed definition
> of the xen_dm_op, removed the pad field and extend the size of op to 64 
> bit(so that we can have
> free bits). But I do not think this is an optimal solution either.
> 
>> Currently, the DMOP ABI isn't set in stone, so you have until code
>> freeze in 4.9 to make changes.  (i.e. soon now.)  We should also
>> consider which other DMops might potentially need to become continuable,
>> and take preventative action before the freeze.
> 
> I can only see set_mem_type and modified_mem need to become continuable, 
> and they does
> this quite elegantly now.
> 
>>
>> If we need to make similar changes once the ABI truely is frozen, there
>> are plenty of free bits in the end of the union which can be used
>> without breaking the ABI.
> 
> Another propose is to change the definition of 
> xen_dm_op_map_mem_type_to_ioreq_server,
> and extend the flags field from uint32_t to uint64_t and use the upper 
> bits to store the gfn.

Well, you introduce a brand new sub-op in patch 2. Why would
you even try to re-use part of some other field in such a
situation, when you can right away add an opaque 64-bit field
you require the caller to set to zero upon first call? The caller
not playing by this would - afaict - only harm the guest it controls.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel