On Fri, 2013-10-25 at 11:51 +0100, Stefano Stabellini wrote:

This breaks domU ballooning (for some reason only on 64-bit, I've no
clue why not 32-bit).

decrease_reservation does pfn_to_mfn in order to release the page, and
this ends up passing INVALID_P2M_ENTRY, which the hypervisor rightly
rejects.

+static inline unsigned long mfn_to_pfn(unsigned long mfn)
+{
+	unsigned long pfn;
+
+	if (phys_to_mach.rb_node != NULL) {
+		pfn = __mfn_to_pfn(mfn);
+		if (pfn != INVALID_P2M_ENTRY)
+			return pfn;
+	}
+
+	if (xen_initial_domain())
+		return mfn;
+	else
+		return INVALID_P2M_ENTRY;

Same here I think.

Both of these should unconditionally return their inputs for !
xen_initial_domain() I think. I presume hys_to_mach.rb_node != NULL
should never trigger for a domU?

+static int xen_add_phys_to_mach_entry(struct xen_p2m_entry *new)
+{
+	struct rb_node **link = &phys_to_mach.rb_node;
+	struct rb_node *parent = NULL;
+	struct xen_p2m_entry *entry;
+	int rc = 0;
+
+	while (*link) {
+		parent = *link;
+		entry = rb_entry(parent, struct xen_p2m_entry, rbnode_phys);
+
+		if (new->mfn == entry->mfn)
+			goto err_out;
+		if (new->pfn == entry->pfn)
+			goto err_out;
+
+		if (new->pfn < entry->pfn)
+			link = &(*link)->rb_left;
+		else
+			link = &(*link)->rb_right;
+	}

Are there really no helpers for walking an rbtree?

+	rb_link_node(&new->rbnode_phys, parent, link);
+	rb_insert_color(&new->rbnode_phys, &phys_to_mach);
+	goto out;
+
+err_out:
+	rc = -EINVAL;
+	pr_warn("%s: cannot add pfn=%pa -> mfn=%pa: pfn=%pa -> mfn=%pa already exists\n",
+			__func__, &new->pfn, &new->mfn, &entry->pfn, &entry->mfn);
+out:
+	return rc;
+}
+
+unsigned long __pfn_to_mfn(unsigned long pfn)
+{
+	struct rb_node *n = phys_to_mach.rb_node;
+	struct xen_p2m_entry *entry;
+	unsigned long irqflags;
+
+	read_lock_irqsave(&p2m_lock, irqflags);
+	while (n) {
+		entry = rb_entry(n, struct xen_p2m_entry, rbnode_phys);
+		if (entry->pfn <= pfn &&
+				entry->pfn + entry->nr_pages > pfn) {
+			read_unlock_irqrestore(&p2m_lock, irqflags);
+			return entry->mfn + (pfn - entry->pfn);
+		}
+		if (pfn < entry->pfn)
+			n = n->rb_left;
+		else
+			n = n->rb_right;
+	}
+	read_unlock_irqrestore(&p2m_lock, irqflags);
+
+	return INVALID_P2M_ENTRY;
+}
+EXPORT_SYMBOL_GPL(__pfn_to_mfn);
+
+static int xen_add_mach_to_phys_entry(struct xen_p2m_entry *new)
+{
+	struct rb_node **link = &mach_to_phys.rb_node;
+	struct rb_node *parent = NULL;
+	struct xen_p2m_entry *entry;
+	int rc = 0;
+
+	while (*link) {
+		parent = *link;
+		entry = rb_entry(parent, struct xen_p2m_entry, rbnode_mach);
+
+		if (new->mfn == entry->mfn)
+			goto err_out;
+		if (new->pfn == entry->pfn)
+			goto err_out;
+
+		if (new->mfn < entry->mfn)
+			link = &(*link)->rb_left;
+		else
+			link = &(*link)->rb_right;
+	}

This looks close to identical to the one in xen_add_phys_to_mach_entry.
You could combine them with a simple "lookup mfn" boolean.

+	rb_link_node(&new->rbnode_mach, parent, link);
+	rb_insert_color(&new->rbnode_mach, &mach_to_phys);
+	goto out;
+
+err_out:
+	rc = -EINVAL;
+	pr_warn("%s: cannot add pfn=%pa -> mfn=%pa: pfn=%pa -> mfn=%pa already exists\n",
+			__func__, &new->pfn, &new->mfn, &entry->pfn, &entry->mfn);
+out:
+	return rc;
+}
+
+unsigned long __mfn_to_pfn(unsigned long mfn)
+{
+	struct rb_node *n = mach_to_phys.rb_node;
+	struct xen_p2m_entry *entry;
+	unsigned long irqflags;
+
+	read_lock_irqsave(&p2m_lock, irqflags);
+	while (n) {
+		entry = rb_entry(n, struct xen_p2m_entry, rbnode_mach);
+		if (entry->mfn <= mfn &&
+				entry->mfn + entry->nr_pages > mfn) {
+			read_unlock_irqrestore(&p2m_lock, irqflags);
+			return entry->pfn + (mfn - entry->mfn);
+		}
+		if (mfn < entry->mfn)
+			n = n->rb_left;
+		else
+			n = n->rb_right;
+	}

and this looks basically identical to __pfn_to_mfn in the same way.

+	read_unlock_irqrestore(&p2m_lock, irqflags);
+
+	return INVALID_P2M_ENTRY;
+}
+EXPORT_SYMBOL_GPL(__mfn_to_pfn);
+

Ian.