Re: [Xen-devel] [PATCH v2] dm_op: Add xendevicemodel_modified_memory_bulk.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.03.17 at 19:44, <Jennifer.Herbert@xxxxxxxxxx> wrote:

> 
> On 23/03/17 15:58, Jan Beulich wrote:
>>>>>>> On 22.03.17 at 20:55, <Jennifer.Herbert@xxxxxxxxxx> wrote:
>>>>>>> --- a/xen/arch/x86/hvm/dm.c
>>>>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>>>>> @@ -119,56 +119,89 @@ static int set_isa_irq_level(struct domain *d, 
>>>>>>> uint8_t
>>>
>>>>>>> --- a/xen/include/public/hvm/dm_op.h
>>>>>>> +++ b/xen/include/public/hvm/dm_op.h
>>>>>>> @@ -237,13 +237,24 @@ struct xen_dm_op_set_pci_link_route {
>>>>>>>      * XEN_DMOP_modified_memory: Notify that a set of pages were 
>>>>>>> modified by
>>>>>>>      *                           an emulator.
>>>>>>>      *
>>>>>>> - * NOTE: In the event of a continuation, the @first_pfn is set to the
>>>>>>> - *       value of the pfn of the remaining set of pages and @nr reduced
>>>>>>> - *       to the size of the remaining set.
>>>>>>> + * DMOP buf 1 contains an array of xen_dm_op_modified_memory_extent 
>>>>>>> with
>>>>>>> + * @nr_extent entries.  The value of  @nr_extent will be reduced on
>>>>>>> + *  continuation.
>>>>>>> + *
>>>>>>> + * @pfns_processed is used for continuation, and not intended to be 
>>>>>>> usefull
>>>>>>> + * to the caller.  It gives the number of pfns already processed (and 
>>>>>>> can
>>>>>>> + * be skipped) in the last extent. This should always be set to zero.
>>>>>>>      */
>>>>>>>     #define XEN_DMOP_modified_memory 11
>>>>>>>     
>>>>>>>     struct xen_dm_op_modified_memory {
>>>>>>> +    /* IN - number of extents. */
>>>>>>> +    uint32_t nr_extents;
>>>>>>> +    /* IN - should be set to 0, and is updated on continuation. */
>>>>>>> +    uint32_t pfns_processed;
>>>>>> I'd prefer if the field (if to be kept) wasn't named after its current
>>>>>> purpose, nor should we state anything beyond it needing to be
>>>>>> zero when first invoking the call. These are implementation details
>>>>>> which callers should not rely on.
>>>>> Assuming we keep it, how about calling it "reserved", with a comment in
>>>>> dm.c explaining what its actually for?
>>>> Elsewhere we use "opaque", but "reserved" is probably fine too.
>>>> However, we may want to name it as an OUT value, for the
>>>> error-on-extent indication mentioned above (of course another
>>>> option would be to make nr_extent IN/OUT). As an OUT, we're
>>>> free to use it for whatever intermediate value, just so long as
>>>> upon final return to the caller it has the intended value. (It's
>>>> debatable whether it should be IN/OUT, due to the need for it
>>>> to be set to zero.)
>>> Having an indication of which extent failed seem a sensible idea. We'd
>>> need that
>>> parameter to be initially set to something that can represent none of
>>> the extents,
>>> such that if there is an error before we get to precessing the extents,
>>> this is clear.
>> I don't think this is a requirement - failure outside of any extent
>> processing can reasonably be attached to the first extent. The
>> more that the actual processing of the extent (after reading the
>> coordinates from guest memory) can't actually fail. With that
>> observation I'm in fact no longer convinced we really need such
>> an indication - I simply didn't pay attention to this aspect when
>> first suggesting it. The more that your backwards processing
>> would invalidate a common conclusion a caller might draw: Error
>> on extent N means all extents lower than N were processed
>> successfully.
>>
>> So if you wanted to stick to providing this information I'd say
>> use the opaque (or whatever it's going to be named) field to
>> provide that status. Switch back to processing extents forwards,
>> having the opaque field starting out as zero point to the first
>> extent as the failed one initially. Initial processing during
>> continuation handling can't fail unless the caller has fiddled with
>> the hypercall arguments, so I wouldn't see anything wrong with
>> not providing a reliable error indicator in that case.
>>
> 
> I was mostly considering it, from a debugging perspective.  Any failure 
> would be due to bad
> arguments, which would indicate a serious bug, and trying to continue 
> after such a bug
> was discovered would seem most unwise.
> If I copied back the buffer on event of error, then we could state that 
> nr_extent would point
> to one after extent that had the error, but say it is not defined which 
> if any extents would have
> succeeded.  This would be a trivial change, but aid with any debugging.
> 
> I can continue to count extents backwards, saving one parameter.
> I can then have an opaque, which i say nothing about, other then it 
> should be zero.
> This i'd use for for pfns_processed.
> 
> How does that sound?

Reasonable. Let's see how that ends up looking.

Jan

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