search

tianocore / edk2

EDK II
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II
Other
4.58k stars 2.47k forks source link

OvmfPkg/QemuVideoDxe: add feature PCD to remap framebuffer W/C #5786

Closed ardbiesheuvel closed 3 months ago

ardbiesheuvel commented 3 months ago

============ RFC ============= QEMU no longer permits misaligned access to device memory, which breaks QemuVideoDxe on SbsaQemu.

This is one potential work-around - it is quick and dirty hack, and I'm open to alternative approaches.

ardbiesheuvel commented 3 months ago

@hrw

bcran commented 3 months ago

I had a discussion with @hrw on Twitter and he linked me to https://lore.kernel.org/qemu-devel/20240301204110.656742-1-richard.henderson@linaro.org/ which is where the change in Qemu behavior happened.

I haven't had a chance to understand the change: is this a bug in Qemu? Or is it just more correctly emulating AArch64?

ardbiesheuvel commented 3 months ago

I had a discussion with @hrw on Twitter and he linked me to https://lore.kernel.org/qemu-devel/20240301204110.656742-1-richard.henderson@linaro.org/ which is where the change in Qemu behavior happened.

I haven't had a chance to understand the change: is this a bug in Qemu? Or is it just more correctly emulating AArch64?

The latter.

QemuVideoDxe was already broken in the same way when running under KVM, but it was never a problem because there are other reasons (related to memory coherency) that prevent us from using that driver on ArmVirtQemu, so it was removed from the primary ARM-related virtual platforms.

SbsaQemu is intentionally more geared towards emulation of a system topology that resembles what you might encounter on an actual bare metal platform, and so VGA PCIe controllers are supported too, by means of the QemuVideoDxe driver, which ignores memory attributes for framebuffers entirely.

leiflindholm commented 3 months ago

I'm happy with this change. I guess on x86 this ends up architecturally as a no-op?

kraxel commented 3 months ago

I'm happy with this change. I guess on x86 this ends up architecturally as a no-op?

Nope. x86 tries (and fails) to setup MTRR registers. Which is I guess the reason why @ardbiesheuvel has added PCD to enable this behaviour.

ardbiesheuvel commented 3 months ago

I'm happy with this change. I guess on x86 this ends up architecturally as a no-op?

I'll let @kraxel answer that, but I think the answer is no. In any case, this is behind a feature PCD so OVMF will not be affected by this change.

kraxel commented 3 months ago

I'd suggest to apply the attribute to the complete pci bar, i.e. use FrameBufDesc->AddrRangeMin + FrameBufDesc->AddrLen. Otherwise looks good to me.

bcran commented 3 months ago

I removed the 'push' label because someone added a merge commit instead of rebasing the branch.

bcran commented 3 months ago

@ardbiesheuvel Could you remove the merge commit please?

Merge branch 'master' into rfc-qemu-pci-framebuffer-remap-wc

ardbiesheuvel commented 3 months ago

@ardbiesheuvel Could you remove the merge commit please?

Merge branch 'master' into rfc-qemu-pci-framebuffer-remap-wc

Mergify creates these automatically, but somehow, they don't end up on the main branch. You can just ignore them.

bcran commented 3 months ago

@ardbiesheuvel Could you remove the merge commit please?

Merge branch 'master' into rfc-qemu-pci-framebuffer-remap-wc

Mergify creates these automatically, but somehow, they don't end up on the main branch. You can just ignore them.

Oh I didn't realize - sorry for canceling the push!

ardbiesheuvel commented 3 months ago

@ardbiesheuvel Could you remove the merge commit please?

Merge branch 'master' into rfc-qemu-pci-framebuffer-remap-wc

Mergify creates these automatically, but somehow, they don't end up on the main branch. You can just ignore them.

Oh I didn't realize - sorry for canceling the push!

No worries