Re: [PATCH 13/25] argo: implement the register op

["Jan Beulich" <JBeulich@suse.com>]

>>> On 21.12.18 at 09:16, <christopher.w.clark@gmail.com> wrote:
> On Thu, Dec 20, 2018 at 11:28 PM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 21.12.18 at 02:25, <christopher.w.clark@gmail.com> wrote:
>> > On Thu, Dec 20, 2018 at 12:29 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >>
>> >> >>> On 20.12.18 at 06:29, <christopher.w.clark@gmail.com> wrote:
>> >> > On Wed, Dec 12, 2018 at 1:48 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>
>> >> >> > +static int
>> >> >> > +argo_find_ring_mfns(struct domain *d, struct argo_ring_info *ring_info,
>> >> >> > +                    uint32_t npage, XEN_GUEST_HANDLE_PARAM(argo_pfn_t) pfn_hnd,
>> >> >> > +                    uint32_t len)
>> >> >> > +{
>> >> >> > +    int i;
>> >> >> > +    int ret = 0;
>> >> >> > +
>> >> >> > +    if ( (npage << PAGE_SHIFT) < len )
>> >> >> > +        return -EINVAL;
>> >> >> > +
>> >> >> > +    if ( ring_info->mfns )
>> >> >> > +    {
>> >> >> > +        /*
>> >> >> > +         * Ring already existed. Check if it's the same ring,
>> >> >> > +         * i.e. same number of pages and all translated gpfns still
>> >> >> > +         * translating to the same mfns
>> >> >> > +         */
>> >> >>
>> >> >> This comment makes me wonder whether the translations are
>> >> >> permitted to change at other times. If so I'm not sure what
>> >> >> value verification here has. If not, this probably would want to
>> >> >> be debugging-only code.
>> >> >
>> >> > My understanding is that the gfn->mfn translation is not necessarily stable
>> >> > across entry and exit from host power state S4, suspend to disk.

Note this ^^^ (and see below).

>> Now I'm afraid there's some confusion here: Originally you've
>> said "host".
>>
>> >> How would that be? It's not stable across guest migration (or
>> >> its non-live save/restore equivalent),
>> >
>> > Right, that's clear.
>> >
>> >> but how would things change across S3?
>> >
>> > I don't think that they do change in that case.
>> >
>> > From studying the code involved above, a related item: the guest runs the same
>> > suspend and resume kernel code before entering into/exiting from either guest
>> > S3 or S4, so the guest kernel resume code needs to re-register the rings, to
>> > cover the case where it is coming up in an environment where they were dropped
>> > - so that's what it does.
>> >
>> > This relates to the code section above: if guest entry to S3 is aborted at the
>> > final step (eg. error or platform refuses, eg. maybe a physical device
>> > interaction with passthrough) then the hypervisor has not torn down the rings,
>> > the guest remains running within the same domain, and the guest resume logic
>> > runs, which runs through re-registration for all its rings. The check in the
>> > logic above allows the existing ring mappings within the hypervisor to be
>> > preserved.
>>
>> Yet now you suddenly talk about guest S3.
> 
> Well, the context is that you did just ask about S3, without
> specifying host or guest.

I'm sorry to be picky, but no, I don't think I did. You did expicitly
say "host", making me further think only about that case.

> Host S3 doesn't involve much at all, so I
> went and studied the code in both the Linux driver and the hypervisor
> to determine what it does in the case of guest S3, and then replied
> with the above since it is relevant to the code in question. I hope I
> was clear about referring to guest S3 above in my last reply.
> 
> That logic aims to make ring registration idempotent, to avoid the
> teardown of established mappings of the ring pages in the case where
> doing so isn't needed.

You treat complexity in the kernel for complexity in the hypervisor.
I'm not sure this is appropriate, as I can't judge how much more
difficult it would be for the guest to look after itself. But let's look
at both cases again:
- For guest S3, afaik, the domain doesn't change, and hence
  memory assignment remains the same. No re-registration
  necessary then afaict.
- For guest S4, aiui, the domain gets destroyed and a new one
  built upon resume. Re-registration would be needed, but due
  to the domain re-construction no leftovers ought to exist in
  Xen.
Hence to me it would seem more natural to have the guest deal
with the situation, and have no extra logic for this in Xen. You
want the guest to re-register anyway, yet simply avoiding to
do so in the S3 case ought to be a single (or very few)
conditional(s), i.e. not a whole lot of complexity.

Jan


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