[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 03/12] arm/mem_access: Add defines supporting PTs with varying page sizes

On 04/07/17 22:33, Sergej Proskurin wrote:
Hi Julien,

Hi Sergej,

On 07/04/2017 06:15 PM, Julien Grall wrote:
Hi Sergej,


+#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) &
+static inline vaddr_t third_guest_table_offset_##gran##K(vaddr_t
gva)                   \

Sorry I haven't spot it before. This is not going to work properly on
32-bit if you use vaddr_t. Indeed, input for stage-2 page-table (i.e
IPA) will be 40-bit. But vaddr_t is 32-bit. So you to use paddr_t here
and in all the helpers below.

I agree that IPAs won't work properly on AArch32. However, we don't walk
the second stage translation tables with the introduced code (yet?). In
fact, second stage translation walks in software are not supported at
the moment. I understand why you would think in this direction, with
ARM's nested virtualization support coming up, where we might need to
walk the second stage translation tables in sw. Yet, with the current
implementation, we work on on GVAs (not IPAs) and hence the vaddr_t
should not present an issue (except that the now missing CONFIG_ARM_64
#ifdef's in the long-descriptor translation table walk create compile
issues as we need to support both different page granularities and
zeroeth-level offsets which work on gva's > 32bit on AArch64).

If you wish to see the implementation extended in the future to support
walking the 2nd stage address translation, then I will gladly change
vaddr_t to paddr_t.

Rather than justifying with: "We don't use like that today, so it is fine to keep the bug", you should think: "How can it be used in the future?". And when I see a cast from vaddr_t to paddr_t in the code, then I can directly tell something is wrong.

We already have code to walk stage-2 page-table (see p2m_lookup) and are using similar macro a bit everywhere in the p2m code. I was actually thinking to make *_table_offset an alias to *_table_offset_4k because they are the same. Note I am not asking modify the p2m code...

Even though the stage-2 code does not use those helpers today, I don't see any valid reasons to keep a known latent bug in the code. It will likely be forgotten and I wish good luck of the developer who will have the issue.

BTW, I think you can drop "guest" in the name because those new helps are very generic.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.