Re: [Xen-devel] [PATCH 2/4] xen/arm: implement page reference and grant table functions needed by grant_table.c

[Tim Deegan <tim@xxxxxxx>]

At 12:05 +0100 on 04 Jul (1341403532), Stefano Stabellini wrote:
> The implementation is strongly "inspired" by their x86 counterparts,
> except that we assume paging_mode_external and paging_mode_translate.
> 
> TODO: read_only mappings and gnttab_mark_dirty.

Do we actually need these refcounts on ARM?  Or was it just the
typecount we thought we could do without?

> +struct domain *page_get_owner_and_reference(struct page_info *page)
> +{
> +    unsigned long x, y = page->count_info;
> +
> +    do {
> +        x = y;
> +        /*
> +         * Count ==  0: Page is not allocated, so we cannot take a reference.
> +         * Count == -1: Reference count would wrap, which is invalid. 
> +         * Count == -2: Remaining unused ref is reserved for 
> get_page_light().
> +         */
> +        if ( unlikely(((x + 2) & PGC_count_mask) <= 2) )
> +            return NULL;
> +    }
> +    while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );

get_page_light() is an x86-ism, so we don't need to leave room for it here. 

> +void gnttab_clear_flag(unsigned long nr, uint16_t *addr)
> +{
> +     /*
> +      * Note that this cannot be clear_bit(), as the access must be
> +      * confined to the specified 2 bytes.
> +      */
> +     uint16_t mask = ~(1 << nr), old;
> +
> +     do {
> +             old = *addr;

A hard tab has snuck in here.  

> +     } while (cmpxchg(addr, old, old & mask) != old);

This could be done more efficiently with the underlying LRREXH/STREXH
instructions but maybe not worth it?  Is this on any hot paths?

> +}
> +
> +void gnttab_mark_dirty(struct domain *d, unsigned long l)
> +{
> +    /* XXX: mark dirty */
> +}
> +
> +int create_grant_host_mapping(unsigned long addr, unsigned long frame, 
> +                              unsigned int flags, unsigned int cache_flags)
> +{
> +    int rc;
> +
> +    if ( cache_flags  || (flags & ~GNTMAP_readonly) != GNTMAP_host_map )
> +        return GNTST_general_error;
> +
> +    /* XXX: read only mappings
> +    if ( flags & GNTMAP_readonly )
> +        p2mt = p2m_grant_map_ro;
> +    else
> +        p2mt = p2m_grant_map_rw;
> +    */
> +    rc = guest_physmap_add_page(current->domain,
> +                                 addr >> PAGE_SHIFT, frame, 0);

I'm a little worried about taking this in, in case we never get round to
making the read-only grants read-only.  Could we get away with making
that an error case or are they needed right now?

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel