Xen devel - Oct 2009 - [PATCH 00 of 10] Xen-side netchannel2 patches

This patch series includes all of the Xen and tools bits of netchannel2.
That means:

-- V2 grant tables
-- Patches to xend to create and destroy NC2 interfaces
-- Various bits of hotplug script gubbins.

There are more detailed descriptions in the individual patch comments.

By itself, none of this is terribly useful.  I''ll be following up
shortly
with two more patch series, one to Jeremy''s git tree and one for the
XCI
patchqueue, which contain the actual interesting bits.

As far as I''m aware, this is now ready to go upstream.  The series
should
apply cleanly to xen-unstable changeset 20270:6f63970032a3.

Steven.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/10/2009 16:00, "Steven Smith" <Steven.Smith@eu.citrix.com>
wrote:
> This patch series includes all of the Xen and tools bits of netchannel2.
> That means:
> 
> -- V2 grant tables
> -- Patches to xend to create and destroy NC2 interfaces
> -- Various bits of hotplug script gubbins.
> 
> There are more detailed descriptions in the individual patch comments.
> 
> By itself, none of this is terribly useful.  I''ll be following up
shortly
> with two more patch series, one to Jeremy''s git tree and one for
the XCI
> patchqueue, which contain the actual interesting bits.
> 
> As far as I''m aware, this is now ready to go upstream.  The series
should
> apply cleanly to xen-unstable changeset 20270:6f63970032a3.
Thanks Steve. I''ll plan to check these in at the end of the week, as
long as
no significant points are raised by reviewers.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> <steven.smith@citrix.com> 04.10.09 17:00 >>>
>+struct gnttab_get_status_frames {
>+    /* IN parameters. */
>+    uint32_t nr_frames;
>+    domid_t  dom;
>+    /* OUT parameters. */
>+    int16_t  status;              /* GNTST_* */
>+    uint64_t frame_list;
I would seem that this should be a GUEST_HANDLE, or is there any
particular reason to not declare it so?
>+};
Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> <steven.smith@citrix.com> 04.10.09 17:00 >>>
>--- a/xen/include/xen/grant_table.h	Mon Sep 22 13:09:07 2008 +0100
>+++ b/xen/include/xen/grant_table.h	Fri Nov 28 15:43:42 2008 +0000
>@@ -32,7 +32,9 @@
> struct active_grant_entry {
>     u32           pin;    /* Reference count information.             */
>     domid_t       domid;  /* Domain being granted access.             */
>-    unsigned long frame;  /* Frame being granted.                     */
>+    domid_t       trans_dom;
>+    uint32_t      trans_gref;
>+    uint32_t      frame;  /* Frame being granted.                     */
Using a uint32_t for a frame number is not valid anymore after the recent
sparse physical memory support changes (and I don''t think it would have
been valid on ia64 even before).
>     unsigned long gfn;    /* Guest''s idea of the frame being
granted. */
This field I would think is much more reasonable to constrain to 32 bits,
if you need to save space here.
>     unsigned      is_sub_page:1; /* True if this is a sub-page grant. */
>     unsigned      start:15; /* For sub-page grants, the start offset
Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> <steven.smith@citrix.com> 04.10.09 17:00 >>>
Defining CONFIG_* stuff in unmodified_drivers/linux-2.6/overrides.mk
seems certainly bogus, so I think the patch would at least need an
explanation of why this cannot be done differently.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 05/10/2009 09:02, "Jan Beulich" <JBeulich@novell.com> wrote:
>> -    unsigned long frame;  /* Frame being granted.                    
*/
>> +    domid_t       trans_dom;
>> +    uint32_t      trans_gref;
>> +    uint32_t      frame;  /* Frame being granted.                    
*/
> 
> Using a uint32_t for a frame number is not valid anymore after the recent
> sparse physical memory support changes (and I don''t think it would
have
> been valid on ia64 even before).
What about the ''uint32_t frame'' in grant_entry_v2_t --
I''m surprised you
didn''t mention that also? It''s most critical we get the sizing
of that
correct, while we''re rev''ing a public interface. Given the
definition of
__spacer[], it looks like there''s space to have a full
''uint64_t frame'' in
that structure too, no problem. A bit of field reordering might be needed to
make everything align naturally yet compactly, is all, I think?

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Keir Fraser <keir.fraser@eu.citrix.com> 05.10.09 10:23
>>>
>On 05/10/2009 09:02, "Jan Beulich" <JBeulich@novell.com>
wrote:
>
>>> -    unsigned long frame;  /* Frame being granted.                 
*/
>>> +    domid_t       trans_dom;
>>> +    uint32_t      trans_gref;
>>> +    uint32_t      frame;  /* Frame being granted.                 
*/
>> 
>> Using a uint32_t for a frame number is not valid anymore after the
recent
>> sparse physical memory support changes (and I don''t think it
would have
>> been valid on ia64 even before).
>
>What about the ''uint32_t frame'' in grant_entry_v2_t --
I''m surprised you
>didn''t mention that also? It''s most critical we get the
sizing of that
Oops, that one slipped my attention.
>correct, while we''re rev''ing a public interface. Given the
definition of
>__spacer[], it looks like there''s space to have a full
''uint64_t frame'' in
>that structure too, no problem. A bit of field reordering might be needed to
>make everything align naturally yet compactly, is all, I think?
Agreed. Not sure though whether uintXX_t is the right thing to use -
after all we have xen_pfn_t for exactly this purpose.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >+struct gnttab_get_status_frames {
> >+    /* IN parameters. */
> >+    uint32_t nr_frames;
> >+    domid_t  dom;
> >+    /* OUT parameters. */
> >+    int16_t  status;              /* GNTST_* */
> >+    uint64_t frame_list;
> It would seem that this should be a GUEST_HANDLE, or is there any
> particular reason to not declare it so?
Mostly that GUEST_HANDLE isn''t 32/64 invariant, and I didn''t
want to
have to introduce yet more compat shims.

If there''s a strong consensus that GUEST_HANDLE is preferable then it
could be switched easily enough, but it seems kind of silly to
continue introducing more ABIs which aren''t 64-bit clean.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >>> -    unsigned long frame;  /* Frame being granted.            
*/
> >>> +    domid_t       trans_dom;
> >>> +    uint32_t      trans_gref;
> >>> +    uint32_t      frame;  /* Frame being granted.            
*/
> >> 
> >> Using a uint32_t for a frame number is not valid anymore after the
recent
> >> sparse physical memory support changes (and I don''t think
it would have
> >> been valid on ia64 even before).
Yes, agreed, I''ll put it back to unsigned long.
> >What about the ''uint32_t frame'' in grant_entry_v2_t
-- I''m surprised you
> >didn''t mention that also? It''s most critical we get
the sizing of that
> 
> Oops, that one slipped my attention.
> 
> >correct, while we''re rev''ing a public interface.
Given the definition of
> >__spacer[], it looks like there''s space to have a full
''uint64_t frame'' in
> >that structure too, no problem. A bit of field reordering might be
needed to
> >make everything align naturally yet compactly, is all, I think?
> Agreed. Not sure though whether uintXX_t is the right thing to use -
> after all we have xen_pfn_t for exactly this purpose.
Maybe.  xen_pfn_t probably would be the right type, but, again, it''s
not 32/64 clean, which means it''d need some compat stuff.  Not
impossible, but kind of annoying.

Would something like this be acceptable?

struct grant_entry_v2 {
    grant_entry_header_t hdr;
    union {
        /*
         * The frame to which we are granting access.  This field has
         * the same meaning as the grant_entry_v1 field of the same
         * name.
         */
        struct {
            uint32_t pad0;
            uint64_t frame;
        } __attribute__((packed));

        /*
         * If the grant type is GTF_grant_access and GTF_sub_page is
         * set, @domid is allowed to access bytes
         * [@page_off,@page_off+@length) in frame @frame.
         */
        struct {
            uint16_t page_off;
            uint16_t length;
            uint64_t frame;
        } __attribute__((packed)) sub_page;

        /*
         * If the grant is GTF_transitive, @domid is allowed to use
         * the grant @gref in domain @trans_domid, as if it was the
         * local domain.  Obviously, the transitive access must be
         * compatible with the original grant.
         *
         * The current version of Xen does not allow transitive grants
         * to be mapped.
         */
        struct {
            domid_t trans_domid;
            uint16_t pad0;
            grant_ref_t gref;
        } transitive;

        uint32_t __spacer[3]; /* Pad to a power of two */
    };
};
typedef struct grant_entry_v2 grant_entry_v2_t;



The __attribute__((packed))s are very much not a thing of beauty, but
it''s at least fairly self-contained.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >>> <steven.smith@citrix.com> 04.10.09 17:00 >>>
> Defining CONFIG_* stuff in unmodified_drivers/linux-2.6/overrides.mk
> seems certainly bogus, so I think the patch would at least need an
> explanation of why this cannot be done differently.
I didn''t really give this a great deal of thought, to be honest.  They
need to go somewhere, and overrides.mk seemed like the least bad place
(they''d normally go in the kernel .config, but that doesn''t
work in an
unmodified drivers build).

Where would you prefer I put them?

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 05/10/2009 12:15, "Steven Smith" <Steven.Smith@eu.citrix.com>
wrote:
>>> correct, while we''re rev''ing a public interface.
Given the definition of
>>> __spacer[], it looks like there''s space to have a full
''uint64_t frame'' in
>>> that structure too, no problem. A bit of field reordering might be
needed to
>>> make everything align naturally yet compactly, is all, I think?
>> Agreed. Not sure though whether uintXX_t is the right thing to use -
>> after all we have xen_pfn_t for exactly this purpose.
> Maybe.  xen_pfn_t probably would be the right type, but, again,
it''s
> not 32/64 clean, which means it''d need some compat stuff.  Not
> impossible, but kind of annoying.
> 
> Would something like this be acceptable?
Yes, that would be fine by me. I think continuing with xen_pfn_t and making
another shared grant structure be 32/64-bit unclean would be a mistake. A
uint64_t is clearly going to be big enough.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 05/10/2009 10:36, "Steven Smith" <Steven.Smith@eu.citrix.com>
wrote:
>> It would seem that this should be a GUEST_HANDLE, or is there any
>> particular reason to not declare it so?
> Mostly that GUEST_HANDLE isn''t 32/64 invariant, and I
didn''t want to
> have to introduce yet more compat shims.
> 
> If there''s a strong consensus that GUEST_HANDLE is preferable then
it
> could be switched easily enough, but it seems kind of silly to
> continue introducing more ABIs which aren''t 64-bit clean.
I think now we have guest handles we should just use them, and the guest
access macros, consistently. Despite it requiring us to have a (pretty
trivial) compat shim.

This is despite choosing the opposite for the grant_entry_v2 struct and
xen_pfn_t versus uint64_t. There''s less machinery and magic around
xen_pfn_t, and also compat shims for shared-memory structures are just plain
annoying.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Steven Smith <steven.smith@citrix.com> 05.10.09 13:15
>>>
>Would something like this be acceptable?
>...
>The __attribute__((packed))s are very much not a thing of beauty, but
>it''s at least fairly self-contained.
Hmm, this being a public header, I''m afraid using gcc extensions
isn''t
appropriate.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Steven Smith <steven.smith@citrix.com> 05.10.09 13:19
>>>
>> >>> <steven.smith@citrix.com> 04.10.09 17:00
>>>
>> Defining CONFIG_* stuff in unmodified_drivers/linux-2.6/overrides.mk
>> seems certainly bogus, so I think the patch would at least need an
>> explanation of why this cannot be done differently.
>I didn''t really give this a great deal of thought, to be honest. 
They
>need to go somewhere, and overrides.mk seemed like the least bad place
>(they''d normally go in the kernel .config, but that
doesn''t work in an
>unmodified drivers build).
>
>Where would you prefer I put them?
I can''t see why you need them in the first place - all other frontends
get away without. They simply hard-code their values in the respective
Kbuild files (which ought to be the only place they''re needed).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 05/10/2009 12:55, "Jan Beulich" <JBeulich@novell.com> wrote:
>> Would something like this be acceptable?
>> ...
>> The __attribute__((packed))s are very much not a thing of beauty, but
>> it''s at least fairly self-contained.
> 
> Hmm, this being a public header, I''m afraid using gcc extensions
isn''t
> appropriate.
Oh, I missed those. I obviously didn''t look closely enough! Indeed - no
gcc
extensions in public headers.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >>> Steven Smith <steven.smith@citrix.com> 05.10.09 13:15
>>>
> >Would something like this be acceptable?
> >...
> >The __attribute__((packed))s are very much not a thing of beauty, but
> >it''s at least fairly self-contained.
> 
> Hmm, this being a public header, I''m afraid using gcc extensions
isn''t
> appropriate.
Okay.  How about this one?

union grant_entry_v2 {
    grant_entry_header_t hdr;

    /*
     * The frame to which we are granting access.  This field has the
     * same meaning as the grant_entry_v1 field of the same name.
     */
    struct {
        grant_entry_header_t hdr;
        uint32_t pad0;
        uint64_t frame;
    } full_page;

    /*
     * If the grant type is GTF_grant_access and GTF_sub_page is set,
     * @domid is allowed to access bytes [@page_off,@page_off+@length)
     * in frame @frame.
     */
    struct {
        grant_entry_header_t hdr;
        uint16_t page_off;
        uint16_t length;
        uint64_t frame;
    } sub_page;

    /*
     * If the grant is GTF_transitive, @domid is allowed to use the
     * grant @gref in domain @trans_domid, as if it was the local
     * domain.  Obviously, the transitive access must be compatible
     * with the original grant.
     *
     * The current version of Xen does not allow transitive grants to
     * be mapped.
     */
    struct {
        grant_entry_header_t hdr;
        domid_t trans_domid;
        uint16_t pad0;
        grant_ref_t gref;
    } transitive;

    uint32_t __spacer[4]; /* Pad to a power of two */
};


Duplicating the header into every field is kind of icky, but I can''t
see any other way of avoiding unfortunate padding without using gcc
extensions.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Steven Smith <steven.smith@citrix.com> 05.10.09 15:19
>>>
>> >>> Steven Smith <steven.smith@citrix.com> 05.10.09
13:15 >>>
>> >Would something like this be acceptable?
>> >...
>> >The __attribute__((packed))s are very much not a thing of beauty,
but
>> >it''s at least fairly self-contained.
>> 
>> Hmm, this being a public header, I''m afraid using gcc
extensions isn''t
>> appropriate.
>Okay.  How about this one?
Yes, this looks fine to me.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >>> Steven Smith <steven.smith@citrix.com> 05.10.09 13:19
>>>
> >> >>> <steven.smith@citrix.com> 04.10.09 17:00
>>>
> >> Defining CONFIG_* stuff in
unmodified_drivers/linux-2.6/overrides.mk
> >> seems certainly bogus, so I think the patch would at least need an
> >> explanation of why this cannot be done differently.
> >I didn''t really give this a great deal of thought, to be
honest.  They
> >need to go somewhere, and overrides.mk seemed like the least bad place
> >(they''d normally go in the kernel .config, but that
doesn''t work in an
> >unmodified drivers build).
> >
> >Where would you prefer I put them?
> I can''t see why you need them in the first place - all other
frontends
> get away without. They simply hard-code their values in the respective
> Kbuild files (which ought to be the only place they''re needed).
The original intent was to share Kbuild files between the unmodified
drivers build and the normal Linux one, and the 2.6.18 version of the
driver did just that.  That''s not particularly interesting, though,
because I''m not going to submit the 2.6.18 driver and the later
versions'' Makefiles can''t be used that way.  I guess I should
change
this.

Although, now I look, the unmodified drivers don''t build for a 2.6.27
kernel even without the NC2 patches applied.  That kind of suggests
that nobody''s using them anymore, in which case this patch (as well as
the previous one) should probably be dropped.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Steven Smith <steven.smith@citrix.com> 05.10.09 18:11
>>>
>Although, now I look, the unmodified drivers don''t build for a
2.6.27
>kernel even without the NC2 patches applied.  That kind of suggests
>that nobody''s using them anymore, in which case this patch (as well
as
>the previous one) should probably be dropped.
We continue to use them, and I just checked - we don''t have any fixup
patches in the (3.3.1-based) Xen sources that would work around issues
found there. So unless something got broken post-3.3, I would think such
builds should work, as they do for us.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >> It would seem that this should be a GUEST_HANDLE, or is there any
> >> particular reason to not declare it so?
> > Mostly that GUEST_HANDLE isn''t 32/64 invariant, and I
didn''t want to
> > have to introduce yet more compat shims.
> > 
> > If there''s a strong consensus that GUEST_HANDLE is preferable
then it
> > could be switched easily enough, but it seems kind of silly to
> > continue introducing more ABIs which aren''t 64-bit clean.
> I think now we have guest handles we should just use them, and the guest
> access macros, consistently. Despite it requiring us to have a (pretty
> trivial) compat shim.
Okay.

I''ll post another series which includes those changes (plus the other
ones suggested in review) in a couple of seconds.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel