| Message ID | 20200605041733.415188-7-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target-arm: Implement ARMv8.5-MemTag, user mode | expand |
On Fri, 5 Jun 2020 at 05:17, Richard Henderson <richard.henderson@linaro.org> wrote: > > This is the only use of guest_addr_valid that does not begin > with a guest address, but a host address being transformed to > a guest address. > > We will shortly adjust guest_addr_valid to handle guest memory > tags, and the host address should not be subjected to that. > > Move h2g_valid adjacent to the other h2g macros. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/cpu_ldst.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h > index c14a48f65e..3930362e20 100644 > --- a/include/exec/cpu_ldst.h > +++ b/include/exec/cpu_ldst.h > @@ -77,15 +77,16 @@ typedef uint64_t abi_ptr; > #else > #define guest_addr_valid(x) ((x) <= GUEST_ADDR_MAX) > #endif > -#define h2g_valid(x) guest_addr_valid((unsigned long)(x) - guest_base) > > static inline int guest_range_valid(unsigned long start, unsigned long len) > { > return len - 1 <= GUEST_ADDR_MAX && start <= GUEST_ADDR_MAX - len + 1; > } > > +#define h2g_valid(x) ((uintptr_t)(x) - guest_base <= GUEST_ADDR_MAX) The old implementation returns true for HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS (because there's a different definition of guest_addr_valid() there) but this one does a range check even in that case. > + > #define h2g_nocheck(x) ({ \ > - unsigned long __ret = (unsigned long)(x) - guest_base; \ > + uintptr_t __ret = (uintptr_t)(x) - guest_base; \ > (abi_ptr)__ret; \ > }) Why the type change? This seems unrelated. thanks -- PMM
On 6/25/20 9:34 AM, Peter Maydell wrote: > On Fri, 5 Jun 2020 at 05:17, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> This is the only use of guest_addr_valid that does not begin >> with a guest address, but a host address being transformed to >> a guest address. >> >> We will shortly adjust guest_addr_valid to handle guest memory >> tags, and the host address should not be subjected to that. >> >> Move h2g_valid adjacent to the other h2g macros. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> include/exec/cpu_ldst.h | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h >> index c14a48f65e..3930362e20 100644 >> --- a/include/exec/cpu_ldst.h >> +++ b/include/exec/cpu_ldst.h >> @@ -77,15 +77,16 @@ typedef uint64_t abi_ptr; >> #else >> #define guest_addr_valid(x) ((x) <= GUEST_ADDR_MAX) >> #endif >> -#define h2g_valid(x) guest_addr_valid((unsigned long)(x) - guest_base) >> >> static inline int guest_range_valid(unsigned long start, unsigned long len) >> { >> return len - 1 <= GUEST_ADDR_MAX && start <= GUEST_ADDR_MAX - len + 1; >> } >> >> +#define h2g_valid(x) ((uintptr_t)(x) - guest_base <= GUEST_ADDR_MAX) > > The old implementation returns true for > HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS > (because there's a different definition of guest_addr_valid() there) > but this one does a range check even in that case. It's part and parcel with patch 1, wherein we are in fact attempting to limit the guest address space to GUEST_ADDR_MAX. That's why I put patch 1 first, so the behaviour change happens there. >> #define h2g_nocheck(x) ({ \ >> - unsigned long __ret = (unsigned long)(x) - guest_base; \ >> + uintptr_t __ret = (uintptr_t)(x) - guest_base; \ >> (abi_ptr)__ret; \ >> }) > > Why the type change? This seems unrelated. Dropped. Though at some point we should purge unsigned long, as there is always a clearer type to use. r~
On 7/11/20 12:30 PM, Richard Henderson wrote: >> The old implementation returns true for >> HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS >> (because there's a different definition of guest_addr_valid() there) >> but this one does a range check even in that case. > > It's part and parcel with patch 1, wherein we are in fact attempting to limit > the guest address space to GUEST_ADDR_MAX. > > That's why I put patch 1 first, so the behaviour change happens there. Ho hum. I've just realized the messages are sorted oddly in the mbox here, and that the behaviour change is actually coming later in patch 7. So, to summarize, I am intending a change here, it's just a matter of sorting things so that one thing happens at a time. r~
diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h index c14a48f65e..3930362e20 100644 --- a/include/exec/cpu_ldst.h +++ b/include/exec/cpu_ldst.h @@ -77,15 +77,16 @@ typedef uint64_t abi_ptr; #else #define guest_addr_valid(x) ((x) <= GUEST_ADDR_MAX) #endif -#define h2g_valid(x) guest_addr_valid((unsigned long)(x) - guest_base) static inline int guest_range_valid(unsigned long start, unsigned long len) { return len - 1 <= GUEST_ADDR_MAX && start <= GUEST_ADDR_MAX - len + 1; } +#define h2g_valid(x) ((uintptr_t)(x) - guest_base <= GUEST_ADDR_MAX) + #define h2g_nocheck(x) ({ \ - unsigned long __ret = (unsigned long)(x) - guest_base; \ + uintptr_t __ret = (uintptr_t)(x) - guest_base; \ (abi_ptr)__ret; \ })
This is the only use of guest_addr_valid that does not begin with a guest address, but a host address being transformed to a guest address. We will shortly adjust guest_addr_valid to handle guest memory tags, and the host address should not be subjected to that. Move h2g_valid adjacent to the other h2g macros. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/exec/cpu_ldst.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)