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

[Julien Grall <julien.grall@xxxxxxx>]

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) &
> > > lpae_entry_mask(gran))
> > > +#define
> > > GUEST_TABLE_OFFSET_HELPERS(gran)
> > > \
> > > +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.
Cheers,

--
Julien Grall

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