Re: [PATCH qemu v6 1/1] Honor guest PAT on x86, absent Bochs display

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

Hi,

On 10/10/25 05:43, ~myrslint wrote:
> From: myrslint <qemu.haziness801@passinbox.com>
> 
> On x86_64, where most CPUs support self-snoop, it is preferrable to
> always honor guest PAT. Not doing so is a quirk. There is a default
> enabled KVM quirk flag which enforces not doing so due to a former bug
> in Bochs display driver.
> 
> The bug has been fixed but not enough has yet passed since so we only
> disable said quirk flag if a Bochs display is not configured for the
> virtual machine.
> 
> This commit also moves around a bit of code that would be called when
> the initialization of a VM is done.
> 
> Signed-off-by: myrslint <qemu.haziness801@passinbox.com>
> ---
>  accel/kvm/kvm-all.c        |  1 +
>  accel/stubs/kvm-stub.c     |  1 +
>  hw/display/bochs-display.c |  5 +++++
>  include/system/kvm.h       |  9 ++++++++
>  target/i386/kvm/kvm.c      | 42 +++++++++++++++++++++++++++++++-------
>  5 files changed, 51 insertions(+), 7 deletions(-)
> 

...
> +#include "system/kvm.h"
> +
>  typedef struct BochsDisplayMode {
>      pixman_format_code_t format;
>      uint32_t             bytepp;
> @@ -261,6 +263,7 @@ static const GraphicHwOps bochs_display_gfx_ops = {
>      .gfx_update = bochs_display_update,
>  };
>  
> +

Nit: please remove the extra newline

>  static void bochs_display_realize(PCIDevice *dev, Error **errp)
>  {
>      BochsDisplayState *s = BOCHS_DISPLAY(dev);
> @@ -309,6 +312,8 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
>      }
>  
>      memory_region_set_log(&s->vram, true, DIRTY_MEMORY_VGA);
> +
> +    kvm_bochs_drm = true;

The patch looks good, only minor suggestions from me:

1. rename kvm_bochs_drm -> kvm_bochs_drm_quirk for more clarity

2. add clarifying comment to the code explaining what this quirk does
and why it's needed, basically repeating the commit msg

...
> +static void handle_machine_done(Notifier *n, void *unused)
> +{
> +    if (kvm_x86_smm_enabled()) {
> +        register_smram_listener();
> +    }
> +    if (!kvm_has_bochs_drm() && \
> +        (kvm_x86_disable_quirsk2_mask() & KVM_X86_QUIRK_IGNORE_GUEST_PAT)) {
> +        if (kvm_disable_ignore_guest_pat()) {
> +            error_report("KVM_X86_QUIRK_IGNORE_GUEST_PAT available and "
> +                         "modifiable but we failed to disable it\n");

Nit: the extra "\n" shouldn't be needed for error_report()


-- 
Best regards,
Dmitry