Re: [PATCH v2] dm_op: Add xendevicemodel_modified_memory_bulk.

[Jennifer Herbert <Jennifer.Herbert@citrix.com>]

On 23/03/17 15:58, Jan Beulich wrote:
>>>>>> On 22.03.17 at 20:55, <Jennifer.Herbert@citrix.com> 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?


-jenny



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