Modeling safety of UART or is it bug?

[efenniht]

현재 Uart 구현은, UART interrupt가 발생했을 때에는, Uart 에 대한 lock을 걸지 않고 그냥 UART 의 MMIO register를 읽고 씁니다. xv6 spec 과 riscv interrupt 의 설명에 따르면, device interrupt 는 임의의 CPU에 전해지므로, CPU A 가 putc() 를 하는 도중에 CPU B 가 UART interrupt를 받아서 getc() 를 호출, MMIO register 에 대한 data race 가 발생할 수 있습니다. 자세하게 설명하자면 상태 레지스터 LSR 에 대해서는 multi read가 있을 수 있고, recieve / transmit register 가 주소를 공유하기 때문에 해당 주소에 대해서는 concurrent read / write 이 존재할 수 있습니다.

여기서 발생하는 문제가 두 가지 있는데,

• Rust memory model 상에서 concurrent nonatomic read / write 은 UB 입니다. 하드웨어 레벨에서는 아니지만. 관련해서 What about: volatile, concurrency, and interaction with untrusted threads rust-lang/unsafe-code-guidelines#152 (comment) 이런 논의가 있습니다.
• Concurrent getc()s 도 순서가 보장되지 않게 읽어오므로 옳은 모델은 getc() 를 호출할 때 &mut Uart 를 받는 것이라고 생각하는데, 그 경우 UART interrupt가 Uart 에 대한 exclusive ownership을 가진다는 것을 나타내기 어렵습니다. (UART interrupt가 걸려서 UART interrupt가 disable 되었다는 것이 exclusiveness 를 보장.) 이걸 잘 말아서 타입으로 wrapping 하는 게 맞을 지, getc() 가 그냥 &self 를 받도록 하고, 해당 문제는 직접적인 memory safety 와 상관이 없으므로 괜찮다고 하는 게 맞을 지 궁금합니다.

어떻게 생각하시나요? @jeehoonkang

[jeehoonkang]

• 요는 volatile concurrent access를 한다는 의미이군요. 시스템 프로그래밍에서 이러한 경우가 자주 있을 것 같고 Rust에서 이 아이디어를 어떻게 표현하는지 궁금합니다. 링크해주신 논의에 결론이 있나요?

• getc()가 (interrupt에 의해 발동된) putc()와 함께 불릴 수 있으므로 &self를 받는게 맞다고 생각합니다. interrupt 내에서 불리는 putc()도 마찬가지이구요. 우리가 이들 사이의 synchronization을 (unsafe하게) 분석할 필요가 있습니다.

• 혹시 xv6에 버그는 없나요?

[efenniht]

• 해당 링크를 요약했습니다: Concurrent volatile access 를 UB로 지정한 것은 실수이고, document를 수정해야 합니다.
  • C++ spec proposal 중 data race 의 정의에서 volatile load / store 가 involve 된 경우는 빼자는 proposal이 있습니다.
  • CompCert에서 volatile access *addr = val 은 extern function write_volatile(addr, SIZE, val) 을 호출하는 것으로 모델링합니다. 이때 이 함수에 대한 assumption은 addr..addr+size 외의 메모리가 수정되지 않는다라는 것만 존재합니다.
  • Rust에서 이 이상의 specification을 더 추가할 지는 아직 논의 중입니다.
  • 따라서 지금은 이 문제를 넘어가도 될 것 같습니다.
• getc() 와 putc() 가 &self 를 받도록 하겠습니다.
• 개인적으로 다음은 xv6의 버그였다고 생각합니다:
  • Refactor Spinlock and related functions #166 에서 고친 cpu 가 atomic 변수가 아니었던 것
  • Refactoring console.rs and printf.rs #186 에서 수정 중인 locking 이 atomic 이 변수가 아니었던 것

[efenniht]

getc() 관련해서 아직 문제가 남아있는 데 (#186 (comment)) trap 관련 코드를 리팩토링할 때 다시 보도록 하겠습니다.

[coolofficials]

Xv6 upstream의 변화와 #286 에서의 변화가 본 이슈와 어떤 연관성이 있는지 논의가 필요할 것 같습니다.

[coolofficials]

@Medowhill 최근 변화들에서 이 이슈 관련 변화가 있었나요? Trap refactoring 이후에 다시 visit 하기로 했었습니다.

[Medowhill]

#503 에서 진행한 아래와 같은 수정에 따라 이 이슈는 닫도록 하겠습니다.

• Uart의 메서드들이 &self를 요구함으로써 Uart가 MMIO 레지스터들을 소유함이 표현됨
• Console이 Uart를 소유하게 됨

Volatile concurrent access가 허용되어야 하는지 따지는 것은 이 일의 범위가 아니라고 생각되므로, 해당 PR에서는 volatile concurrent access가 허용된다고 가정했습니다.