solo5 icon indicating copy to clipboard operation
solo5 copied to clipboard

Remove -m-no-red-zone on AMD64

Open mato opened this issue 6 years ago • 4 comments

We have been carrying -m-no-red-zone for AMD64 for all code (bindings themselves and "user" code) since the dawn of time. Since then, the CPU interrupt handling and trap handling code has been refactored to use ISTn, i.e. separate stacks for interrupt and trap handlers.

We should consider removing -m-no-red-zone. There are two options:

  1. Remove it from "user" code only, i.e. for the MirageOS case, do not pass it down to OPAM via pkg-config.
  2. Remove it from the bindings as well.

The first option seems entirely safe -- "user" code will never run in an interrupt context, and any trap/interrupt handlers in bindings currently run on a separate stack via ISTn.

The second option should be possible with a minor tweak to our usage of ISTn to accomodate virtio (hvt does not use hardware interrupts). Currently, we have (from bindings/cpu_x86_64.c):

IST1 (cpu_intr_stack): All hardware interrupts except #NMI (virtio only).
IST2 (cpu_trap_stack): All traps except #DF.
IST3 (cpu_nmi_stack): #NMI, #DF.

Given that on virtio it is theoretically possible for IRQ0 (timer) to nest with IRQx (virtio_net), I think we want something like this:

IST1 (cpu_intr1_stack): IRQ0 (timer, virtio only)
IST2 (cpu_intr2_stack): All other hardware interrupts (i.e. only virtio-net).
IST3 ... IST5 (unused, reserved for future expansion)
IST6 (cpu_trap_stack): All traps except #DF.
IST7 (cpu_nmi_stack): #NMI, #DF.

References: System V Application Binary Interface x86-64 Architecture Processor Supplement, section 3.2.2, also this stackoverflow answer.

Any opinions? @djwillia @ricarkol?

mato avatar Nov 22 '18 13:11 mato

Interesting, the red zone business seems very subtle, which is why I guess most kernels just avoid it. But you are right, we certainly don't need it in the hvt case. I would lean towards removing it entirely (option 2 above). It seems the only issue is the virtio case.

A couple of questions: in virtio, does linking code that has been built with and without red zone (option 1) cause any issues, in other words would you need the IST partitioning scheme even if you only changed flags for "user" code? And second, would it be easy to see if there are issues with the IST partitioning scheme? It seems pretty racy and I'm not sure how often you get an interrupt clobber when not being careful about the red zone.

djwillia avatar Nov 29 '18 15:11 djwillia

@djwillia I think I explained the situation on our call today, let me know if you need any recap here?

mato avatar Nov 29 '18 16:11 mato

See also https://www.kernel.org/doc/Documentation/x86/kernel-stacks, which gives a good explanation of the IST mechanism on x86_64.

Notably, what I did not realise but makes sense in hindsight, IST-using interrupt or trap handlers cannot nest safely. This needs some more thought about the implications for virtio; it's possible we have a race condition between virtio-net and timer interrupts even in the current setup, although it must be extremely rare since no one has reported it. In order to be 100% safe, it would be best for virtio to always use separate ISTs for the timer and virtio-net.

mato avatar Dec 11 '18 15:12 mato

#345 rationalized the various build flags, and in the process implements option 1 here, so -mno-red-zone is no longer passed down to OPAM consumers. I'm keeping this open for now, but don't plan to do any further red-zone/IST related changes in the short term.

mato avatar May 20 '19 12:05 mato