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

Re: [PATCH V3 19/23] xen/arm: io: Abstract sign-extension





On 01/12/2020 12:24, Oleksandr wrote:

On 01.12.20 14:13, Julien Grall wrote:
Hi Oleksandr,

Hi Julien.



--- a/xen/include/asm-arm/traps.h
+++ b/xen/include/asm-arm/traps.h
@@ -83,6 +83,30 @@ static inline bool VABORT_GEN_BY_GUEST(const struct cpu_user_regs *regs)
           (unsigned long)abort_guest_exit_end == regs->pc;
   }
   +/* Check whether the sign extension is required and perform it */ +static inline register_t sign_extend(const struct hsr_dabt dabt, register_t r)
+{
+    uint8_t size = (1 << dabt.size) * 8;
+
+    /*
+     * Sign extend if required.
+     * Note that we expect the read handler to have zeroed the bits
+     * outside the requested access size.
+     */
+    if ( dabt.sign && (r & (1UL << (size - 1))) )
+    {
+        /*
+         * We are relying on register_t using the same as
+         * an unsigned long in order to keep the 32-bit assembly
+         * code smaller.
+         */
+        BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long));
+        r |= (~0UL) << size;
If `size` is 64, you will get undefined behavior there.
I think, we don't need to worry about undefined behavior here. Having
size=64 would be possible with doubleword (dabt.size=3). But if "r"
adjustment gets called (I mean Syndrome Sign Extend bit is set) then
we deal with byte, halfword or word operations (dabt.size<3). Or I
missed something?

At which point please put in a respective ASSERT(), possibly amended
by a brief comment.

ASSERT()s are only meant to catch programatic error. However, in this case, the bigger risk is an hardware bug such as advertising a sign extension for either 64-bit (or 32-bit) on Arm64 (resp. Arm32).

Actually the Armv8 spec is a bit more blurry when running in AArch32 state because they suggest that the sign extension can be set even for 32-bit access. I think this is a spelling mistake, but it is probably better to be cautious here.

Therefore, I would recommend to rework the code so it is only called when len < sizeof(register_t).

I am not sure I understand the recommendation, could you please clarify (also I don't see 'len' being used here).

Sorry I meant 'size'. I think something like:

if ( dabt.sign && (size < sizeof(register_t)) &&
     (r & (1UL << (size - 1)) )
{
}

Another posibility would be:

if ( dabt.sign && (size < sizeof(register_t)) )
{
   /* find whether the sign bit is set and propagate it */
}

I have a slight preference for the latter as the "if" is easier to read.

In any case, I think this change should be done in a separate patch (I don't mint whether this is done after or before this one).

ok, I got it, thank you for the clarification. Of course, I will do that in a separate patch, since the current one is to avoid code duplication only. BTW, do you have comments on this patch itself?

The series is in my TODO list. I will have a look once in a bit :).

Cheers,

--
Julien Grall



 


Rackspace

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