Re: [Xen-devel] [RFC v3 5/6] xen/arm: Add log_dirty support for ARM

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Sun, 2014-05-11 at 16:28 +0100, Julien Grall wrote:

> [..]
> 
> > +/* Return start and end addr of guest RAM. Note this function only reports
> > + * regular RAM. It does not cover other areas such as foreign mapped
> > + * pages or MMIO space. */
> > +void domain_get_ram_range(struct domain *d, paddr_t *start, paddr_t *end)
> > +{
> > +    if ( start )
> > +        *start = GUEST_RAM_BASE;
> > +
> > +    if ( end )
> > +        *end = GUEST_RAM_BASE + ((paddr_t) d->max_pages << PAGE_SHIFT);
> > +}
> 
> As said on V1 this solution won't work.
> 
> Ian plans to add multiple banks support for the guest very soon. With this
> solution there is a 1GB hole between the 2 banks. Your function will therefore
> stop to work.
> 
> Furthermore, Xen should not assume that the layout of the guest will always 
> start
> at GUEST_RAM_BASE.

Actually, for the time being that is fine if it is internal to the
hypervisor, although it might be storing up pain for later.

> > +        for ( i2 = second_index; i2 < LPAE_ENTRIES; ++i2 )
> > +        {
> > +            lpae_walk_t second_pte = second[i2].walk;
> > +
> > +            if ( !second_pte.valid || !second_pte.table )
> > +                goto out;
> 
> With Ian's multiple bank support, the RAM region (as returned by 
> domain_get_range)
> can contain a hole. Rather than leaving the loop, you should continue.

Even without that patch you'd need to be careful of ballooned out
regions.
> > @@ -341,6 +343,27 @@ static inline void put_page_and_type(struct page_info 
> > *page)
> >       put_page(page);
> >   }
> >
> > +enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> 
> Please describe this enum. Also mg is too generic.

This is just code motion.

> > @@ -41,6 +42,7 @@ typedef enum {
> >       p2m_invalid = 0,    /* Nothing mapped here */
> >       p2m_ram_rw,         /* Normal read/write guest RAM */
> >       p2m_ram_ro,         /* Read-only; writes are silently dropped */
> > +    p2m_ram_logdirty,   /* Read-only: special mode for log dirty */
> 
> You should at the new type at the end of the enum.

Why? Keeping p2m_ram_* together doesn't seem wrong to me.

Ian.


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