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

[Christopher Clark <christopher.w.clark@gmail.com>]

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.
>
> 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.

I'm not certain that is an enormous win though; it looks like it would be ok
to drop that logic and reestablish the mappings as the ring is used, as per
other cases.

> And there's no support for S4 (and I can't see it appearing any time soon).

OK. oh well.

>
> >> > +static struct argo_ring_info *
> >> > +argo_ring_find_info(const struct domain *d, const struct argo_ring_id *id)
> >> > +{
> >> > +    uint16_t hash;
> >> > +    struct hlist_node *node;
> >>
> >> const?
> >
> > I couldn't determine exactly what you were pointing towards with this one.
> > I've applied 'const' in a lot further place in the next version; please
> > let me know if I've missed where you intended.
>
> This is a pretty general rule: const should be applied to pointer
> target types whenever no modification is intended, to make
> this read-only aspect very obvious (and force people to think
> twice if they alter such a property).
>
> >> > +    uint64_t dst_domain_cookie = 0;
> >> > +
> >> > +    if ( !(guest_handle_is_aligned(ring_hnd, ~PAGE_MASK)) )
> >> > +        return -EINVAL;
> >>
> >> Why? You don't store the handle for later use (and you shouldn't).
> >> If there really is a need for a full page's worth of memory, it
> >> would better be passed in as GFN.
> >
> > I've added this comment for this behaviour in v2:
> >
> > +    /*
> > +     * Verify the alignment of the ring data structure supplied with the
> > +     * understanding that the ring handle supplied points to the same memory as
> > +     * the first entry in the array of pages provided via pg_descr_hnd, where
> > +     * the head of the ring will reside.
> > +     * See argo_update_tx_ptr where the location of the tx_ptr is accessed at a
> > +     * fixed offset from head of the first page in the mfn array.
> > +     */
>
> Well, this then suggests that you don't want to verify alignment,
> but instead you want to verify addresses match.

ack. I'll take a look at doing that.

>
> >> > @@ -253,6 +723,34 @@ do_argo_message_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> >> >
> >> >      switch (cmd)
> >> >      {
> >> > +    case ARGO_MESSAGE_OP_register_ring:
> >> > +    {
> >> > +        XEN_GUEST_HANDLE_PARAM(argo_ring_t) ring_hnd =
> >> > +            guest_handle_cast(arg1, argo_ring_t);
> >> > +        XEN_GUEST_HANDLE_PARAM(argo_pfn_t) pfn_hnd =
> >> > +            guest_handle_cast(arg2, argo_pfn_t);
> >> > +        uint32_t npage = arg3;
> >> > +        bool fail_exist = arg4 & ARGO_REGISTER_FLAG_FAIL_EXIST;
> >> > +
> >> > +        if ( unlikely(!guest_handle_okay(ring_hnd, 1)) )
> >> > +            break;
> >>
> >> I don't understand the need for this and ...
> >>
> >> > +        if ( unlikely(npage > (ARGO_MAX_RING_SIZE >> PAGE_SHIFT)) )
> >> > +        {
> >> > +            rc = -EINVAL;
> >> > +            break;
> >> > +        }
> >> > +        if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) )
> >> > +            break;
> >>
> >> ... perhaps also this, when you use copy_from_guest() upon access.
> >
> > This is the one piece of feedback on version 1 of this series that I haven't
> > taken the time to address yet. The code is evidently safe, with only a possible
> > performance decrease a concern, so I'd like to study it further before removing
> > any of the checks rather than delay posting version two of this series.
>
> Hmm, re-posting without all comments addressed is not ideal.
> It means extra work for the reviewers (unless you've clearly
> marked respective code fragments with some sort of TBD
> comment).

Understood.

Christopher

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