Re: [Xen-devel] [RFC PATCH 5/7] x86/m2b: Add a tracking structure for mfn to bfn mappings per page

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Feb 10, 2016 at 10:10 AM, Malcolm Crossley
<malcolm.crossley@xxxxxxxxxx> wrote:
> Add a pointer to the page struct which refers to the head of
> m2b tracking structure for that page.
>
> Atomically add a PGC bit to the page struct when setting the pointer to
> the m2b tracking structure.
>
> Adding elements to the per page m2b tracking structure is done via a
> RCU protected linked list.
>
> Callers of add_m2b and del_m2b must hold the page lock for the page.
> This prevents the m2b code racing with itself and allows for fine
> gain locking. References to the page are also expected to be taken and
> dropped by the callers to the m2b code.
>
> m2b references are forcibly removed as part of domain destroy to ensure 
> malicious
> guests do not cause memory

Editing mistake?

[snip]

> +struct m2b_entry *lookup_m2b_entry(struct page_info *page, struct domain *d,
> +                                   ioservid_t ioserver, unsigned long bfn)
> +{
> +    struct m2b_entry *m2b_e = NULL;
> +    struct list_head *entry;
> +    domid_t domain = d->domain_id;
> +
> +    if ( !test_bit(_PGC_foreign_map, &page->count_info) )
> +        return NULL;
> +
> +    rcu_read_lock(&m2b_rcu);
> +    list_for_each_rcu(entry, &page->pv_iommu->head)
> +    {
> +        m2b_e = list_entry(entry, struct m2b_entry, list);
> +        if ( m2b_e->domain == domain && m2b_e->ioserver == ioserver &&
> +                m2b_e->bfn == bfn )
> +                    goto done;
> +        else if ( ioserver == IOSERVER_ANY && m2b_e->domain == domain &&
> +                  m2b_e->bfn == bfn )
> +                    goto done;
> +        else if ( bfn == BFN_ANY && m2b_e->domain == domain &&
> +                  m2b_e->ioserver == ioserver )
> +                    goto done;
> +        else if ( bfn == BFN_ANY && ioserver == IOSERVER_ANY &&
> +                  m2b_e->domain == domain )
> +                    goto done;
> +    }

Just a minor note while I'm thinking about it -- since every single
one of these have "m2b_e->domain == domain", wouldn't it be easier to
read if you just had "if (m2b_e->domain != domain) continue" first,
and then the series of if statements without mentioning the domain?

 -George

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