Re: [PATCH 05 of 10] Introduce a grant_entry_v2 structure

Re: [PATCH 05 of 10] Introduce a grant_entry_v2 structure

[Jan Beulich]

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

Re: [PATCH 07 of 10] Transitive grant support

[Jan Beulich]

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

Re: [PATCH 10 of 10] Hook NC2 up to the unmodified drivers build

[Jan Beulich]

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

Re: [PATCH 07 of 10] Transitive grant support

[Keir Fraser]

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

Re: [PATCH 07 of 10] Transitive grant support

[Jan Beulich]

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

Re: [PATCH 05 of 10] Introduce a grant_entry_v2 structure

[Steven Smith]

[-- Attachment #1.1: Type: text/plain, Size: 649 bytes --]

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 07 of 10] Transitive grant support

[Steven Smith]

[-- Attachment #1.1: Type: text/plain, Size: 2838 bytes --]

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 10 of 10] Hook NC2 up to the unmodified drivers build

[Steven Smith]

[-- Attachment #1.1: Type: text/plain, Size: 530 bytes --]

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 07 of 10] Transitive grant support

[Keir Fraser]

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

Re: [PATCH 05 of 10] Introduce a grant_entry_v2 structure

[Keir Fraser]

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

Re: [PATCH 07 of 10] Transitive grant support

[Jan Beulich]

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

Re: [PATCH 10 of 10] Hook NC2 up to the unmodified drivers build

[Jan Beulich]

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

Re: [PATCH 07 of 10] Transitive grant support

[Keir Fraser]

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

Re: [PATCH 07 of 10] Transitive grant support

[Steven Smith]

[-- Attachment #1.1: Type: text/plain, Size: 1749 bytes --]

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 07 of 10] Transitive grant support

[Jan Beulich]

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

Re: [PATCH 10 of 10] Hook NC2 up to the unmodified drivers build

[Steven Smith]

[-- Attachment #1.1: Type: text/plain, Size: 1438 bytes --]

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 10 of 10] Hook NC2 up to the unmodified drivers build

[Jan Beulich]

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

Re: [PATCH 05 of 10] Introduce a grant_entry_v2 structure

[Steven Smith]

[-- Attachment #1.1: Type: text/plain, Size: 767 bytes --]

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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