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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 12 Jul 2012, Tim Deegan wrote:
> 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?

Nope, we need them for the grant_table.


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

I'll remove it


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

I'll fix


> > +   } 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?

It is called whem mapping/unmapping grant references, I don't think it
counts as hot path.


> > +}
> > +
> > +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?

Good idea, I'll return an error.

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