[1/7] x86/HVM: drop stdvga's "cache" struct member

Commit Message

Jan Beulich Sept. 10, 2024, 2:39 p.m. UTC

As of 68e1183411be ("libxc: introduce a xc_dom_arch for hvm-3.0-x86_32
guests") caching mode is disabled for HVM domains from start-of-day, due
the disabling being unconditional in hvm/save.c:arch_hvm_load(). With
that the field is useless, and can be dropped. Drop the helper functions
manipulating / checking as well right away, but leave the use sites of
stdvga_cache_is_enabled() with the hard-coded result the function would
have produced, to aid validation of subsequent dropping of further code.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Andrew Cooper Sept. 10, 2024, 4:44 p.m. UTC | #1

On 10/09/2024 3:39 pm, Jan Beulich wrote:
> As of 68e1183411be ("libxc: introduce a xc_dom_arch for hvm-3.0-x86_32
> guests") caching mode is disabled for HVM domains from start-of-day, due
> the disabling being unconditional in hvm/save.c:arch_hvm_load(). With
> that the field is useless, and can be dropped. Drop the helper functions
> manipulating / checking as well right away, but leave the use sites of
> stdvga_cache_is_enabled() with the hard-coded result the function would
> have produced, to aid validation of subsequent dropping of further code.
>
> Signed-off-by: Jan Beulich <[e-mail]>

This only applies to VMs constructed with libxenguest, which isn't all
VMs.  But, it's probably close enough to ~100% of VMs to count.

Personally I think it would be clearer to say "Since 68e1183411be, HVM
guests are built using XEN_DOMCTL_sethvmcontext, which intentionally
disables stdvga caching in arch_hvm_load()".

As a minor tangent, this is yet another casualty of nothing being wired
into the migration stream.  Rightly or wrongly, that mindset has caused
an immense amount of problems in Xen.


How does this interact with X86_EMU_VGA?

stdvga_init() is called unconditionally for HVM domains, exiting early
for !EMU_VGA (skipping the pointless re-zero of the structure). 
Nevertheless, the cache is UNINITIALISED for all configurations at this
point.

And we won't hit the stdvga_try_cache_enable() calls in any of
stdvga_{outb,mem_{write,accept}}() prior to XEN_DOMCTL_sethvmcontext,
which will unconditionally set DISABLED.

So yes, I think this is for-all-intents-and-purposes dead logic.

Reviewed-by: Andrew Cooper <[e-mail]>

Jan Beulich Sept. 11, 2024, 9 a.m. UTC | #2

On 10.09.2024 18:44, Andrew Cooper wrote:
> On 10/09/2024 3:39 pm, Jan Beulich wrote:
>> As of 68e1183411be ("libxc: introduce a xc_dom_arch for hvm-3.0-x86_32
>> guests") caching mode is disabled for HVM domains from start-of-day, due
>> the disabling being unconditional in hvm/save.c:arch_hvm_load(). With
>> that the field is useless, and can be dropped. Drop the helper functions
>> manipulating / checking as well right away, but leave the use sites of
>> stdvga_cache_is_enabled() with the hard-coded result the function would
>> have produced, to aid validation of subsequent dropping of further code.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
> 
> This only applies to VMs constructed with libxenguest, which isn't all
> VMs.  But, it's probably close enough to ~100% of VMs to count.
> 
> Personally I think it would be clearer to say "Since 68e1183411be, HVM
> guests are built using XEN_DOMCTL_sethvmcontext, which intentionally
> disables stdvga caching in arch_hvm_load()".

I'll derive something intermediate. I don't think it was intentional that
caching got disabled this way. It's just that no-one noticed or paid
attention.

> As a minor tangent, this is yet another casualty of nothing being wired
> into the migration stream.  Rightly or wrongly, that mindset has caused
> an immense amount of problems in Xen.

Indeed.

> How does this interact with X86_EMU_VGA?
> 
> stdvga_init() is called unconditionally for HVM domains, exiting early
> for !EMU_VGA (skipping the pointless re-zero of the structure). 
> Nevertheless, the cache is UNINITIALISED for all configurations at this
> point.
> 
> And we won't hit the stdvga_try_cache_enable() calls in any of
> stdvga_{outb,mem_{write,accept}}() prior to XEN_DOMCTL_sethvmcontext,
> which will unconditionally set DISABLED.
> 
> So yes, I think this is for-all-intents-and-purposes dead logic.
> 
> Reviewed-by: Andrew Cooper <[e-mail]>

Thanks.

Jan

Patch

--- a/xen/arch/x86/hvm/save.c
+++ b/xen/arch/x86/hvm/save.c
@@ -69,9 +69,6 @@  static void arch_hvm_load(struct domain
 
     /* Time when restore started  */
     d->arch.hvm.sync_tsc = rdtsc();
-
-    /* VGA state is not saved/restored, so we nobble the cache. */
-    d->arch.hvm.stdvga.cache = STDVGA_CACHE_DISABLED;
 }
 
 /* List of handlers for various HVM save and restore types */
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -100,37 +100,6 @@  static void vram_put(struct hvm_hw_stdvg
     unmap_domain_page(p);
 }
 
-static void stdvga_try_cache_enable(struct hvm_hw_stdvga *s)
-{
-    /*
-     * Caching mode can only be enabled if the the cache has
-     * never been used before. As soon as it is disabled, it will
-     * become out-of-sync with the VGA device model and since no
-     * mechanism exists to acquire current VRAM state from the
-     * device model, re-enabling it would lead to stale data being
-     * seen by the guest.
-     */
-    if ( s->cache != STDVGA_CACHE_UNINITIALIZED )
-        return;
-
-    gdprintk(XENLOG_INFO, "entering caching mode\n");
-    s->cache = STDVGA_CACHE_ENABLED;
-}
-
-static void stdvga_cache_disable(struct hvm_hw_stdvga *s)
-{
-    if ( s->cache != STDVGA_CACHE_ENABLED )
-        return;
-
-    gdprintk(XENLOG_INFO, "leaving caching mode\n");
-    s->cache = STDVGA_CACHE_DISABLED;
-}
-
-static bool stdvga_cache_is_enabled(const struct hvm_hw_stdvga *s)
-{
-    return s->cache == STDVGA_CACHE_ENABLED;
-}
-
 static int stdvga_outb(uint64_t addr, uint8_t val)
 {
     struct hvm_hw_stdvga *s = &current->domain->arch.hvm.stdvga;
@@ -170,7 +139,6 @@  static int stdvga_outb(uint64_t addr, ui
     if ( !prev_stdvga && s->stdvga )
     {
         gdprintk(XENLOG_INFO, "entering stdvga mode\n");
-        stdvga_try_cache_enable(s);
     }
     else if ( prev_stdvga && !s->stdvga )
     {
@@ -468,7 +436,7 @@  static int cf_check stdvga_mem_write(
     };
     struct ioreq_server *srv;
 
-    if ( !stdvga_cache_is_enabled(s) || !s->stdvga )
+    if ( true || !s->stdvga )
         goto done;
 
     /* Intercept mmio write */
@@ -536,18 +504,12 @@  static bool cf_check stdvga_mem_accept(
          * We cannot return X86EMUL_UNHANDLEABLE on anything other then the
          * first cycle of an I/O. So, since we cannot guarantee to always be
          * able to send buffered writes, we have to reject any multi-cycle
-         * I/O and, since we are rejecting an I/O, we must invalidate the
-         * cache.
-         * Single-cycle write transactions are accepted even if the cache is
-         * not active since we can assert, when in stdvga mode, that writes
-         * to VRAM have no side effect and thus we can try to buffer them.
+         * I/O.
          */
-        stdvga_cache_disable(s);
-
         goto reject;
     }
     else if ( p->dir == IOREQ_READ &&
-              (!stdvga_cache_is_enabled(s) || !s->stdvga) )
+              (true || !s->stdvga) )
         goto reject;
 
     /* s->lock intentionally held */
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -110,19 +110,12 @@  struct vpci_arch_msix_entry {
     int pirq;
 };
 
-enum stdvga_cache_state {
-    STDVGA_CACHE_UNINITIALIZED,
-    STDVGA_CACHE_ENABLED,
-    STDVGA_CACHE_DISABLED
-};
-
 struct hvm_hw_stdvga {
     uint8_t sr_index;
     uint8_t sr[8];
     uint8_t gr_index;
     uint8_t gr[9];
     bool stdvga;
-    enum stdvga_cache_state cache;
     uint32_t latch;
     struct page_info *vram_page[64];  /* shadow of 0xa0000-0xaffff */
     spinlock_t lock;