page ref/type count overflows

page ref/type count overflows

[Jan Beulich]

With pretty trivial user mode programs being able to crash the kernel due to
the ref counter widths in Xen being more narrow than in Linux, I started an
attempt to put together a kernel side fix. While addressing the plain
hypercalls is pretty strait forward, dealing with multicalls (both when using
them for lazy mmu mode batching and when explicitly using them in e.g.
netback - the backends are susceptible to this too since a malicious guest
could pass grant references to pages that cannot have additional
references obtained) isn't. I'm therefore trying to find alternatives,
preferably ones mostly transparent to the kernel.

The way I was intending to address this on the kernel side in the general
accessors was to re-issue failed hypercalls (or page table writes) with
_PAGE_PRESENT replaced by _PAGE_PROTNONE, thus converting (non-
recoverable) kernel mode faults (under the right circumstances bringing
down the whole kernel) into user mode faults. In order to avoid the
complications of handling failed multicall elements I'm now considering to
add a mechanism for the kernel to tell Xen to
- automatically do a fallback operation like this at least on failed L1 table
writes
- continue handling mmu updates when one failed (at least in the case
where the guest obviously is not prepared to pick up the pieces, i.e. when
the 'pdone' argument is NULL, or alternatively by [dynamically] altering
the meaning of that pointer so that it could point to a bitmap).

The backend drivers in my opinion have no alternative to getting taught
to do full error checking in order to avoid the respective DomU-induced
problems.

Thanks for any thoughts or opinions,
Jan

Re: page ref/type count overflows

[Keir Fraser]

On 26/01/2009 13:10, "Jan Beulich" <jbeulich@novell.com> wrote:

> The backend drivers in my opinion have no alternative to getting taught
> to do full error checking in order to avoid the respective DomU-induced
> problems.

Backend drivers, which get their mappings via grants, have sufficient
checking already don't they?

As for the general issue, why fudge around it when for any modern system we
can just fix it? By which I mean, x64 systems with CMPXCHG16 support
(everything but ancient Opterons?) can have a full domain pointer and a long
count_info in struct page_info, and still update both atomically.

It'd be a smaller patch, and it'd be less kludgy. Disadvantages are extra 8
bytes per page (which I think is okay, particularly to fix this nasty issue
in a clean way) and only fixes the issue for x64 (I personally don't care
about non-regressions for i386, especially when the cost of the fix in this
case would be IMO high in code complexity and ugliness).

Already there is no reason why type_info (an unsigned long) could not have a
wider count. It's just no use until count_info is widened.

What do you say to that then? :-)

 -- Keir

Re: page ref/type count overflows

[Keir Fraser]

On 26/01/2009 13:33, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> As for the general issue, why fudge around it when for any modern system we
> can just fix it? By which I mean, x64 systems with CMPXCHG16 support
> (everything but ancient Opterons?) can have a full domain pointer and a long
> count_info in struct page_info, and still update both atomically.

Actually I think we could do without CX16. I'm not sure count_info and
domain need to be checked/updated at the same time. I think it would suffice
to update count_info, then check domain and undo on mismatch. That'd get rid
of the existing ugly open-coded cmpxchg8b operations too.

I think I'll look into this bit at least...

 -- Keir

Re: page ref/type count overflows

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 26.01.09 14:33 >>>
>On 26/01/2009 13:10, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>> The backend drivers in my opinion have no alternative to getting taught
>> to do full error checking in order to avoid the respective DomU-induced
>> problems.
>
>Backend drivers, which get their mappings via grants, have sufficient
>checking already don't they?

Actually, after some checking, almost. There's one leak in netback, but
that's trivial to fix.

>As for the general issue, why fudge around it when for any modern system we
>can just fix it? By which I mean, x64 systems with CMPXCHG16 support
>(everything but ancient Opterons?) can have a full domain pointer and a long
>count_info in struct page_info, and still update both atomically.
>
>It'd be a smaller patch, and it'd be less kludgy. Disadvantages are extra 8
>bytes per page (which I think is okay, particularly to fix this nasty issue
>in a clean way) and only fixes the issue for x64 (I personally don't care
>about non-regressions for i386, especially when the cost of the fix in this
>case would be IMO high in code complexity and ugliness).
>
>Already there is no reason why type_info (an unsigned long) could not have a
>wider count. It's just no use until count_info is widened.
>
>What do you say to that then? :-)

I considered all of this first, but afair it's not only ancient Opterons that
don't have cmpxchg16b.

But (just having got your second response) if we can do without that ugly
cmpxchg8b altogether, then of course this is the much preferred solution.
The growing of struct page_info of course isn't very fortunate, but pretty
much unavoidable.

Jan

Re: page ref/type count overflows

[Keir Fraser]

On 26/01/2009 14:10, "Jan Beulich" <jbeulich@novell.com> wrote:

> But (just having got your second response) if we can do without that ugly
> cmpxchg8b altogether, then of course this is the much preferred solution.
> The growing of struct page_info of course isn't very fortunate, but pretty
> much unavoidable.

I'm having at the cmpxchg8b right now. I'll also widen the fields I guess,
since that's pretty easy.

 -- Keir

Re: page ref/type count overflows

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 26.01.09 15:19 >>>
>On 26/01/2009 14:10, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>> But (just having got your second response) if we can do without that ugly
>> cmpxchg8b altogether, then of course this is the much preferred solution.
>> The growing of struct page_info of course isn't very fortunate, but pretty
>> much unavoidable.
>
>I'm having at the cmpxchg8b right now. I'll also widen the fields I guess,
>since that's pretty easy.

The count_info one only, or are you also intending to change _domain?
With struct page_info pretty large already, I'd want to avoid growing it
needlessly.

Partly related to this and partly to the recent heap changes - the need to
constrain the Xen heap to the lower 4G is purely due pickle_domptr(), right?
Wouldn't it make sense to change alloc_domain_struct() then to allocate
a page directly, have pickle_domptr() convert to pfn, and remove the
restriction? Apart from the (auto-ballooning) issues pointed out before, I
realize that such a restriction may make it impossible to create domains
altogether, since there continues to be no way to control what specific
pages can be ballooned out of Dom0 (which as we know has been limiting
the ability to create 32-bit domains).

And in order to possibly buy back the amount the structure will grow now,
wouldn't it make sense to have specialized linked list handling for the
page_info structures that also uses pfn encoding (rather than a full
pointer)?

Jan

Re: page ref/type count overflows

[Keir Fraser]

On 26/01/2009 14:38, "Jan Beulich" <jbeulich@novell.com> wrote:

>> I'm having at the cmpxchg8b right now. I'll also widen the fields I guess,
>> since that's pretty easy.
> 
> The count_info one only, or are you also intending to change _domain?
> With struct page_info pretty large already, I'd want to avoid growing it
> needlessly.

I'm going to change both and do it the easy way, growing the structure from
40 to 48 bytes. You can shrink it again with the tricks you describe if
you're keen. I'm not sure how ugly the list stuff would end up, which would
be my main concern, but I suppose you can hide it behind list.h-style
macros. I don't see there's much duplication of effort to phase the work
like this.

 -- Keir

Re: page ref/type count overflows

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 26.01.09 15:54 >>>
>On 26/01/2009 14:38, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>>> I'm having at the cmpxchg8b right now. I'll also widen the fields I guess,
>>> since that's pretty easy.
>> 
>> The count_info one only, or are you also intending to change _domain?
>> With struct page_info pretty large already, I'd want to avoid growing it
>> needlessly.
>
>I'm going to change both and do it the easy way, growing the structure from
>40 to 48 bytes. You can shrink it again with the tricks you describe if
>you're keen. I'm not sure how ugly the list stuff would end up, which would
>be my main concern, but I suppose you can hide it behind list.h-style
>macros. I don't see there's much duplication of effort to phase the work
>like this.

No word on the question regarding the below-4G restriction of the Xen heap?
Also, what's the purpose of those (_domain & 1) checks in unpickle_domptr()?

Jan

Re: page ref/type count overflows

[Keir Fraser]

On 26/01/2009 16:15, "Jan Beulich" <jbeulich@novell.com> wrote:

>> I'm going to change both and do it the easy way, growing the structure from
>> 40 to 48 bytes. You can shrink it again with the tricks you describe if
>> you're keen. I'm not sure how ugly the list stuff would end up, which would
>> be my main concern, but I suppose you can hide it behind list.h-style
>> macros. I don't see there's much duplication of effort to phase the work
>> like this.
> 
> No word on the question regarding the below-4G restriction of the Xen heap?

Got rid of it. It was only needed for the pickle_domptr() thing, as you
suspected.

> Also, what's the purpose of those (_domain & 1) checks in unpickle_domptr()?

Introduced by shadow2 code, and I think it's never been needed. I'm going to
nuke it, but I will be checking some other things with the shadow code
maintainers so I'll double check that too.

 -- Keir

Re: page ref/type count overflows

[Keir Fraser]

On 26/01/2009 14:54, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> The count_info one only, or are you also intending to change _domain?
>> With struct page_info pretty large already, I'd want to avoid growing it
>> needlessly.
> 
> I'm going to change both and do it the easy way, growing the structure from
> 40 to 48 bytes. You can shrink it again with the tricks you describe if
> you're keen. I'm not sure how ugly the list stuff would end up, which would
> be my main concern, but I suppose you can hide it behind list.h-style
> macros. I don't see there's much duplication of effort to phase the work
> like this.

By the way, unless you can see some really clever way to shrink page_info to
32 bytes then I think it is only worth doing compression tricks on the
list_head field, to save 8 bytes (struct becomes 40 bytes). Compressing the
domain field won't get you down to another multiple-of-eight size. It may be
a trick to keep in mind for future though...

And all my stuff is in, as of changeset 19093.

 -- Keir

Re: page ref/type count overflows

[Tim Deegan]

At 16:30 +0000 on 26 Jan (1232987412), Keir Fraser wrote:
> Introduced by shadow2 code, and I think it's never been needed. 

Certainly not needed now; at one point it was used to recover more
usable bits in the shadow page_info struct.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

Re: page ref/type count overflows

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 26.01.09 18:01 >>>
>On 26/01/2009 14:54, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
>
>>> The count_info one only, or are you also intending to change _domain?
>>> With struct page_info pretty large already, I'd want to avoid growing it
>>> needlessly.
>> 
>> I'm going to change both and do it the easy way, growing the structure from
>> 40 to 48 bytes. You can shrink it again with the tricks you describe if
>> you're keen. I'm not sure how ugly the list stuff would end up, which would
>> be my main concern, but I suppose you can hide it behind list.h-style
>> macros. I don't see there's much duplication of effort to phase the work
>> like this.
>
>By the way, unless you can see some really clever way to shrink page_info to
>32 bytes then I think it is only worth doing compression tricks on the
>list_head field, to save 8 bytes (struct becomes 40 bytes). Compressing the
>domain field won't get you down to another multiple-of-eight size. It may be
>a trick to keep in mind for future though...

Yes, I too realized that on my way home yesterday. The only thing I see as
viable for consideration would be to remove the embedded spinlock again, and
use the same bit-lock as x86-32 does.

And of course I'd like to see the cpumask gone, but that's gonna be more
tricky (if possible at all).

>And all my stuff is in, as of changeset 19093.

Thanks! One thing though that puzzled me regarding c/s 19093: How can
the mbz field have changed from being an overlay of u.inuse._domain to
being one of count_info? And if this was indeed intended that way, then
the comment prior to shadow_check_page_struct_offsets() should be
updated accordingly.

And shouldn't shadow's count field also be widened to BITS_PER_LONG-6?

Jan

Re: page ref/type count overflows

[Keir Fraser]

On 27/01/2009 10:16, "Jan Beulich" <jbeulich@novell.com> wrote:

> Yes, I too realized that on my way home yesterday. The only thing I see as
> viable for consideration would be to remove the embedded spinlock again, and
> use the same bit-lock as x86-32 does.

Makes page_unlock() more expensive on x64. Don't know how much that matters.

> And of course I'd like to see the cpumask gone, but that's gonna be more
> tricky (if possible at all).

Given that page allocations tend to be bursty, we could tlbflush-check all
CPUs? Or make it a groups-of-CPUs mask? I suppose until we really care about
NR_CPUS > 64 it's not an important thing to get rid of anyhow.

>> And all my stuff is in, as of changeset 19093.
> 
> Thanks! One thing though that puzzled me regarding c/s 19093: How can
> the mbz field have changed from being an overlay of u.inuse._domain to
> being one of count_info? And if this was indeed intended that way, then
> the comment prior to shadow_check_page_struct_offsets() should be
> updated accordingly.

It was deliberate because of how get_page() now works. Zero count_info is
properly how we determine 'ordinary' allocated pages from free pages and
shadow pages. I think keeping the owner field as mbz instead was a bit
fragile. Yes the comment needs updating.

> And shouldn't shadow's count field also be widened to BITS_PER_LONG-6?

Would be nice. Hopefully either Tim or Gianluca will see to that.

 -- Keir

Re: page ref/type count overflows

[Keir Fraser]

On 27/01/2009 10:24, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> On 27/01/2009 10:16, "Jan Beulich" <jbeulich@novell.com> wrote:
> 
>> Yes, I too realized that on my way home yesterday. The only thing I see as
>> viable for consideration would be to remove the embedded spinlock again, and
>> use the same bit-lock as x86-32 does.
> 
> Makes page_unlock() more expensive on x64. Don't know how much that matters.

I think we could perhaps merge it into type_info and take/release it at the
same time as get_page_type/put_page_type, to avoid extra atomic ops. I'll
look into that.

For the cpumask, I have some ideas there too...

 -- Keir

Re: page ref/type count overflows

[Keir Fraser]

On 27/01/2009 11:22, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> Makes page_unlock() more expensive on x64. Don't know how much that matters.
> 
> I think we could perhaps merge it into type_info and take/release it at the
> same time as get_page_type/put_page_type, to avoid extra atomic ops. I'll look
> into that.
> 
> For the cpumask, I have some ideas there too...

Jan,

I have a patch for the lock field. It doesn't work yet but I should have it
done quite soon. It cleans up various races at the same time. I will also
look into the cpumask (not so important right now, but I think it will be
quite easy to get rid of anyway).

So... For the rest I thought I would leave it in your court. Since you might
be looking into it already. :-) What I see needing still to be done:
 * Shrink list_head
 * Shrink u.inuse._domain
 * Also will need to shrink sh_page_info (currently 48 bytes). It contains a
few chain pointers which you could probably compress down like page_info's
list_head.

So, overall nothing very hard, but probably a bunch of code to have to
modify! But it should get us down to 32 bytes, I think.
 
 -- Keir

Re: page ref/type count overflows

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 27.01.09 16:38 >>>
>So... For the rest I thought I would leave it in your court. Since you might
>be looking into it already. :-) What I see needing still to be done:
> * Shrink list_head
> * Shrink u.inuse._domain
> * Also will need to shrink sh_page_info (currently 48 bytes). It contains a
>few chain pointers which you could probably compress down like page_info's
>list_head.

Yes, I think I already have about half of the needed changes done.

Jan

Re: page ref/type count overflows

[Keir Fraser]

On 27/01/2009 15:49, "Jan Beulich" <jbeulich@novell.com> wrote:

>>>> Keir Fraser <keir.fraser@eu.citrix.com> 27.01.09 16:38 >>>
>> So... For the rest I thought I would leave it in your court. Since you might
>> be looking into it already. :-) What I see needing still to be done:
>> * Shrink list_head
>> * Shrink u.inuse._domain
>> * Also will need to shrink sh_page_info (currently 48 bytes). It contains a
>> few chain pointers which you could probably compress down like page_info's
>> list_head.
> 
> Yes, I think I already have about half of the needed changes done.

Excellent. Mine is now in as c/s 19103. You'll need that to get down to 32
bytes.

 -- Keir

Re: page ref/type count overflows

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 27.01.09 11:24 >>>
>On 27/01/2009 10:16, "Jan Beulich" <jbeulich@novell.com> wrote:
>> And shouldn't shadow's count field also be widened to BITS_PER_LONG-6?
>
>Would be nice. Hopefully either Tim or Gianluca will see to that.

Actually, I'd like to go a step further: Is there any reason why struct
shadow_page_info must be separate from struct page_info (rather than
sharing the definition, requiring some re-ordering of its elements)?

Thanks, Jan

Re: page ref/type count overflows

[Keir Fraser]

On 29/01/2009 08:34, "Jan Beulich" <jbeulich@novell.com> wrote:

>>>> Keir Fraser <keir.fraser@eu.citrix.com> 27.01.09 11:24 >>>
>> On 27/01/2009 10:16, "Jan Beulich" <jbeulich@novell.com> wrote:
>>> And shouldn't shadow's count field also be widened to BITS_PER_LONG-6?
>> 
>> Would be nice. Hopefully either Tim or Gianluca will see to that.
> 
> Actually, I'd like to go a step further: Is there any reason why struct
> shadow_page_info must be separate from struct page_info (rather than
> sharing the definition, requiring some re-ordering of its elements)?

Not really, apart from wanting to keep shadow stuff in one place in a
private header file, I suppose. Would it risk turning page_info's definition
into crazy union soup? If it could be done as something like:
 unsigned long count_info
 union {
  struct { page_info stuff }; // anonymous
  struct { sh_page_info stuff }; // anonymous
 } // anonymous
That would be nicer than what we currently have, I'd agree. And the more
stuff we can pull out of the anonymous union (e.g., perhaps a list_head?)
then the better, of course.

 -- Keir

Re: page ref/type count overflows

[Tim Deegan]

At 08:34 +0000 on 29 Jan (1233218074), Jan Beulich wrote:
> >>> Keir Fraser <keir.fraser@eu.citrix.com> 27.01.09 11:24 >>>
> >On 27/01/2009 10:16, "Jan Beulich" <jbeulich@novell.com> wrote:
> >> And shouldn't shadow's count field also be widened to BITS_PER_LONG-6?
> >
> >Would be nice. Hopefully either Tim or Gianluca will see to that.
> 
> Actually, I'd like to go a step further: Is there any reason why struct
> shadow_page_info must be separate from struct page_info (rather than
> sharing the definition, requiring some re-ordering of its elements)?

Well, when it _did_ use struct page_info the code was full of ugly hacks
to wedge information into fields with misleading names. :) I also like
the type-safety of not having the two structs anonymous-unioned
together; it's already confusing which field names are valid at any
time.

I've no objection to having the fields merged if it can be done without
hoicking lots of internal shadow-code definitions back out into common
code (it took me ages to separate it all!), and if it gets some real
benefit (like sharing most of the existing fields).

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

Re: page ref/type count overflows

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 29.01.09 09:45 >>>
>On 29/01/2009 08:34, "Jan Beulich" <jbeulich@novell.com> wrote:
>> Actually, I'd like to go a step further: Is there any reason why struct
>> shadow_page_info must be separate from struct page_info (rather than
>> sharing the definition, requiring some re-ordering of its elements)?
>
>Not really, apart from wanting to keep shadow stuff in one place in a
>private header file, I suppose. Would it risk turning page_info's definition
>into crazy union soup? If it could be done as something like:
> unsigned long count_info
> union {
>  struct { page_info stuff }; // anonymous
>  struct { sh_page_info stuff }; // anonymous
> } // anonymous
>That would be nicer than what we currently have, I'd agree. And the more
>stuff we can pull out of the anonymous union (e.g., perhaps a list_head?)
>then the better, of course.

Below is what I currently have it at. I'm afraid it won't get much simpler,
but I think it reasonably expresses the individual overlays. There are three
more transformations I plan to make:
- _domain -> unsigned int
- next_shadow -> __mfn_t
- split u into two unions (one having type_info, type/pinned/count, and
cpumask, the other having _domain, back, and order).

That last step is to avoid having to re-add __attribute__ ((__packed__)),
so that other (future) changes to the structure won't risk mis-aligning any
fields again.

Does this look acceptable?

Jan

/*
 * This definition is solely for the use in struct page_info (and
 * struct page_list_head), intended to allow easy adjustment once x86-64
 * wants to support more than 16Tb.
 * 'unsigned long' should be used for MFNs everywhere else.
 */
#define __mfn_t unsigned int

#ifndef __i386__
#undef page_list_entry
struct page_list_entry
{
    __mfn_t next, prev;
};
#endif

struct page_info
{
    union {
        /* Each frame can be threaded onto a doubly-linked list.
         *
         * For unused shadow pages, a list of pages of this order; for
         * pinnable shadows, if pinned, a list of other pinned shadows
         * (see sh_type_is_pinnable() below for the definition of
         * "pinnable" shadow types).
         */
        struct page_list_entry list;
        /* For non-pinnable shadows, a higher entry that points at us. */
        paddr_t up;
    };

    /* Reference count and various PGC_xxx flags and fields. */
    unsigned long count_info;

    /* Context-dependent fields follow... */
    union {

        /* Page is in use: ((count_info & PGC_count_mask) != 0). */
        struct {
            /* Owner of this page (NULL if page is anonymous). */
            unsigned long _domain; /* pickled format */
            /* Type reference count and various PGT_xxx flags and fields. */
            unsigned long type_info;
        } inuse;

        /* Page is in use by shadow code: count_info == 0. */
        struct {
            unsigned long type:5;   /* What kind of shadow is this? */
            unsigned long pinned:1; /* Is the shadow pinned? */
            unsigned long count:26; /* Reference count */
            union {
                /* When in use, GMFN of guest page we're a shadow of. */
                __mfn_t back;
                /* When free, order of the freelist we're on. */
                unsigned int order;
            };
        } sh;

        /* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
        struct {
            /* Order-size of the free chunk this page is the head of. */
            u32 order;
            /* Mask of possibly-tainted TLBs. */
            cpumask_t cpumask;
        } free;

    } u;

    union {
        /*
         * Timestamp from 'TLB clock', used to avoid extra safety flushes.
         * Only valid for: a) free pages, and b) pages with zero type count
         * (except page table pages when the guest is in shadow mode).
         */
        u32 tlbflush_timestamp;

        /*
         * When PGT_partial is true then this field is valid and indicates
         * that PTEs in the range [0, @nr_validated_ptes) have been validated.
         * An extra page reference must be acquired (or not dropped) whenever
         * PGT_partial gets set, and it must be dropped when the flag gets
         * cleared. This is so that a get() leaving a page in partially
         * validated state (where the caller would drop the reference acquired
         * due to the getting of the type [apparently] failing [-EAGAIN])
         * would not accidentally result in a page left with zero general
         * reference count, but non-zero type reference count (possible when
         * the partial get() is followed immediately by domain destruction).
         * Likewise, the ownership of the single type reference for partially
         * (in-)validated pages is tied to this flag, i.e. the instance
         * setting the flag must not drop that reference, whereas the instance
         * clearing it will have to.
         *
         * If @partial_pte is positive then PTE at @nr_validated_ptes+1 has
         * been partially validated. This implies that the general reference
         * to the page (acquired from get_page_from_lNe()) would be dropped
         * (again due to the apparent failure) and hence must be re-acquired
         * when resuming the validation, but must not be dropped when picking
         * up the page for invalidation.
         *
         * If @partial_pte is negative then PTE at @nr_validated_ptes+1 has
         * been partially invalidated. This is basically the opposite case of
         * above, i.e. the general reference to the page was not dropped in
         * put_page_from_lNe() (due to the apparent failure), and hence it
         * must be dropped when the put operation is resumed (and completes),
         * but it must not be acquired if picking up the page for validation.
         */
        struct {
            u16 nr_validated_ptes;
            s8 partial_pte;
        };

        /*
         * Guest pages with a shadow.  This does not conflict with
         * tlbflush_timestamp since page table pages are explicitly not
         * tracked for TLB-flush avoidance when a guest runs in shadow mode.
         */
        u32 shadow_flags;

        /* When in use, next shadow in this hash chain */
        struct page_info *next_shadow;
    };
};

Re: page ref/type count overflows

[Jan Beulich]

>>> Tim Deegan <Tim.Deegan@citrix.com> 29.01.09 11:12 >>>
>Well, when it _did_ use struct page_info the code was full of ugly hacks
>to wedge information into fields with misleading names. :) I also like

It has mostly clear names now (I mostly kept their original shadow names),
but it avoids redundancy on fields that really have the same purpose in
and outside of shadow code.

>the type-safety of not having the two structs anonymous-unioned
>together; it's already confusing which field names are valid at any
>time.

If type-safety is a concern, then shadow_page_info could of course be
made a secondary definition. But I think with properly named fields that
may not be as much of a concern.

>I've no objection to having the fields merged if it can be done without
>hoicking lots of internal shadow-code definitions back out into common
>code (it took me ages to separate it all!), and if it gets some real
>benefit (like sharing most of the existing fields).

For this, see my other reply.

Jan

Re: page ref/type count overflows

[Tim Deegan]

At 10:54 +0000 on 29 Jan (1233226449), Jan Beulich wrote:
> Below is what I currently have it at. I'm afraid it won't get much simpler,
> but I think it reasonably expresses the individual overlays. There are three
> more transformations I plan to make:
> - _domain -> unsigned int
> - next_shadow -> __mfn_t
> - split u into two unions (one having type_info, type/pinned/count, and
> cpumask, the other having _domain, back, and order).
> 
> That last step is to avoid having to re-add __attribute__ ((__packed__)),
> so that other (future) changes to the structure won't risk mis-aligning any
> fields again.
> 
> Does this look acceptable?

Seems fine, which is to say I don't see much advantage but have no 
objection. :)  Please try to make it clear in the comments which fields
belong to a page which _has_ a shadow and which to a page that _is_ a
shadow.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

Re: page ref/type count overflows

[Keir Fraser]

On 29/01/2009 11:07, "Tim Deegan" <Tim.Deegan@citrix.com> wrote:

>> Does this look acceptable?
> 
> Seems fine, which is to say I don't see much advantage but have no
> objection. :)  Please try to make it clear in the comments which fields
> belong to a page which _has_ a shadow and which to a page that _is_ a
> shadow.

I'd agree with Tim. I perhaps have a slight preference for keeping it all in
one struct (i.e., the new approach) but I'm happy either way.

 -- Keir

Re: page ref/type count overflows

[Gianluca Guida]

Jan Beulich wrote:
> Below is what I currently have it at. I'm afraid it won't get much simpler,
> but I think it reasonably expresses the individual overlays. There are three
> more transformations I plan to make:
> - _domain -> unsigned int
> - next_shadow -> __mfn_t
> - split u into two unions (one having type_info, type/pinned/count, and
> cpumask, the other having _domain, back, and order).
> 
> That last step is to avoid having to re-add __attribute__ ((__packed__)),
> so that other (future) changes to the structure won't risk mis-aligning any
> fields again.
> 
> Does this look acceptable?

Personally, while I don't have (and I can't have) anything against this, 
I think that this kind of unified struct would make the shadow code more 
error-prone, by making shadow pages and guest pages being casted by the 
same type.
Of course, every bug is fixable, and good programmers shouldn't do this 
kind of errors. But I think that being able to differentiate between 
shadow's page_info and normal page_info by something more than just a 
variable name would help a lot the clarity of this already complex code.

Thanks,
Gianluca