On Fri, 21 Aug 2020, Simon Leiner wrote:
On 20.08.20 20:35, Stefano Stabellini wrote:
Thank for the well-written analysis of the problem. The following
should
work to translate the virtual address properly in xenbus_grant_ring:
if (is_vmalloc_addr(vaddr))
page = vmalloc_to_page(vaddr);
else
page = virt_to_page(vaddr);
Great idea, thanks! I modified it lightly (see below) and it did indeed
work! I'm wondering though whether the check for vmalloc'd addresses
should be included directly in the ARM implementation of virt_to_gfn.
As far as I see, this should not break anything, but might impose a
small performance overhead in cases where it is known for sure that we
are dealing with directly mapped memory. What do you think?
Thanks for testing!
We could ask the relevant maintainers for feedback, but I think it is
probably intended that virt_to_gfn doesn't work on vmalloc addresses.
That's because vmalloc addresses are not typically supposed to be used
like that.
diff --git a/drivers/xen/xenbus/xenbus_client.c
b/drivers/xen/xenbus/xenbus_client.c
index e17ca8156171..d7a97f946f2f 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -344,6 +344,21 @@ static void xenbus_switch_fatal(struct xenbus_device *dev,
int depth, int err,
__xenbus_switch_state(dev, XenbusStateClosing, 1);
}
+/**
+ * vaddr_to_gfn
+ * @vaddr: any virtual address
+ *
+ * Returns the guest frame number (GFN) corresponding to vaddr.
+ */
+static inline unsigned long vaddr_to_gfn(void *vaddr)
+{
+ if (is_vmalloc_addr(vaddr)) {
+ return pfn_to_gfn(vmalloc_to_pfn(vaddr));
+ } else {
+ return virt_to_gfn(vaddr);
+ }
+}
+
For the same reason as above, I would rather have the check inside
xenbus_grant_ring, rather than above in a generic function:
- if this is a special case the check should be inside xenbus_grant_ring
- if this is not a special case, then the fix should be in virt_to_gfn
as you mentioned
either way, I wouldn't introduce this function here
Juergen, do you agree with this?
