Re: [Xen-devel] [PATCH v3 3/3] xen: rework paging_log_dirty_op to work with hvm guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.04.15 at 19:29, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -397,6 +397,53 @@ int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn)
>      return rv;
>  }
>  
> +static inline void *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
> +                                     unsigned long pages,
> +                                     struct page_info **page,
> +                                     unsigned long *mapped_page)
> +{
> +    p2m_type_t p2mt;
> +    uint32_t pfec;
> +    unsigned long gfn;
> +
> +    gfn = paging_gva_to_gfn(current,
> +                            (paddr_t)(((char *)dirty_bitmap.p) + (pages >> 
> 3)),

I'd suggest dropping the parentheses around the inner cast - they
make this more difficult to read, and I think any C programmer
should know that unary ops have precedence over binary ones.

> +                            &pfec);
> +    if ( gfn == INVALID_GFN )
> +        return NULL;
> +
> +    *page = get_page_from_gfn(current->domain, gfn, &p2mt, P2M_UNSHARE);
> +
> +    if ( !p2m_is_ram(p2mt) )
> +    {
> +        put_page(*page);
> +        return NULL;
> +    }
> +    if ( p2m_is_paging(p2mt) )
> +    {
> +        put_page(*page);
> +        p2m_mem_paging_populate(current->domain, gfn);
> +        return NULL;
> +    }
> +    if ( p2m_is_shared(p2mt) || p2m_is_discard_write(p2mt) )
> +    {
> +        put_page(*page);
> +        return NULL;
> +    }
> +
> +    *mapped_page = pages;

You only ever store the passed in value of "pages" into
"*mapped_pages" - what's the point of this? If the caller needs
to track the value it passes here, it should simple make a copy
itself if so needed.

Apart from that both parameter names don't really seem to
express their purpose.

> @@ -409,9 +456,23 @@ static int paging_log_dirty_op(struct domain *d,
>      mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
>      unsigned long *l1 = NULL;
>      int i4, i3, i2;
> +    uint8_t *dirty_bitmap = NULL;

Pointless initializer.

> +    struct page_info *page;
> +    unsigned long index_mapped = 0;
>  
>      if ( !resuming )
>          domain_pause(d);
> +
> +    dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap,
> +                                    resuming ?
> +                                        
> d->arch.paging.preempt.log_dirty.done :
> +                                        0,
> +                                    &page, &index_mapped);
> +    if ( dirty_bitmap == NULL )
> +    {
> +        domain_unpause(d);
> +        return -EFAULT;
> +    }
>      paging_lock(d);

Blank line above that one please.

> @@ -471,15 +534,29 @@ static int paging_log_dirty_op(struct domain *d,
>                      bytes = (unsigned int)((sc->pages - pages + 7) >> 3);
>                  if ( likely(peek) )
>                  {
> -                    if ( (l1 ? copy_to_guest_offset(sc->dirty_bitmap,
> -                                                    pages >> 3, (uint8_t 
> *)l1,
> -                                                    bytes)
> -                             : clear_guest_offset(sc->dirty_bitmap,
> -                                                  pages >> 3, bytes)) != 0 )
> +                    if ( pages >> (3 + PAGE_SHIFT) !=
> +                         index_mapped >> (3 + PAGE_SHIFT) )
>                      {
> -                        rv = -EFAULT;
> -                        goto out;
> +                        /* We need to map next page */
> +                        paging_unlock(d);
> +                        unmap_dirty_bitmap(dirty_bitmap, page);
> +                        dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, 
> pages,
> +                                                        &page, 
> &index_mapped);
> +                        paging_lock(d);
> +                        if ( dirty_bitmap == NULL )
> +                        {
> +                            rv = -EFAULT;
> +                            goto out;
> +                        }
> +                        goto again;

This won't work: The paging lock protects all of
d->arch.paging.preempt.log_dirty, of which you hold cached values
in local variables.

> +                    BUG_ON(((pages >> 3) % PAGE_SIZE) + bytes > PAGE_SIZE);

I don't seem to be able to spot the original for this one. If there
was none, please make this an ASSERT() instead.

> +                    if ( l1 )
> +                        memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
> +                               (uint8_t *)l1, bytes);

Pointless cast.

Jan

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