No uses are left, hence its setup, teardown, and the field itself can
also go away. stdvga_deinit() is then empty and can be dropped as well.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I have no idea whether in the tool stack some memory calculations would
want adjusting, to account for the 256k less that a HVM domain now
needs.

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -700,7 +700,6 @@ int hvm_domain_initialise(struct domain
     return 0;
 
  fail2:
-    stdvga_deinit(d);
     vioapic_deinit(d);
  fail1:
     if ( is_hardware_domain(d) )
@@ -763,7 +762,6 @@ void hvm_domain_destroy(struct domain *d
     if ( hvm_funcs.domain_destroy )
         alternative_vcall(hvm_funcs.domain_destroy, d);
 
-    stdvga_deinit(d);
     vioapic_deinit(d);
 
     XFREE(d->arch.hvm.pl_time);
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -119,8 +119,7 @@ static const struct hvm_io_ops stdvga_me
 void stdvga_init(struct domain *d)
 {
     struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
-    struct page_info *pg;
-    unsigned int i;
+    struct hvm_io_handler *handler;
 
     if ( !has_vvga(d) )
         return;
@@ -128,47 +127,15 @@ void stdvga_init(struct domain *d)
     memset(s, 0, sizeof(*s));
     spin_lock_init(&s->lock);
     
-    for ( i = 0; i != ARRAY_SIZE(s->vram_page); i++ )
+    /* VGA memory */
+    handler = hvm_next_io_handler(d);
+    if ( handler )
     {
-        pg = alloc_domheap_page(d, MEMF_no_owner);
-        if ( pg == NULL )
-            break;
-        s->vram_page[i] = pg;
-        clear_domain_page(page_to_mfn(pg));
-    }
-
-    if ( i == ARRAY_SIZE(s->vram_page) )
-    {
-        struct hvm_io_handler *handler;
-
-        /* VGA memory */
-        handler = hvm_next_io_handler(d);
-
-        if ( handler == NULL )
-            return;
-
         handler->type = IOREQ_TYPE_COPY;
         handler->ops = &stdvga_mem_ops;
     }
 }
 
-void stdvga_deinit(struct domain *d)
-{
-    struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
-    int i;
-
-    if ( !has_vvga(d) )
-        return;
-
-    for ( i = 0; i != ARRAY_SIZE(s->vram_page); i++ )
-    {
-        if ( s->vram_page[i] == NULL )
-            continue;
-        free_domheap_page(s->vram_page[i]);
-        s->vram_page[i] = NULL;
-    }
-}
-
 /*
  * Local variables:
  * mode: C
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -111,12 +111,10 @@ struct vpci_arch_msix_entry {
 };
 
 struct hvm_hw_stdvga {
-    struct page_info *vram_page[64];  /* shadow of 0xa0000-0xaffff */
     spinlock_t lock;
 };
 
 void stdvga_init(struct domain *d);
-void stdvga_deinit(struct domain *d);
 
 extern void hvm_dpci_msi_eoi(struct domain *d, int vector);

On 10/09/2024 3:41 pm, Jan Beulich wrote:
> No uses are left, hence its setup, teardown, and the field itself can
> also go away. stdvga_deinit() is then empty and can be dropped as well.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although I think
there's more to do.

> ---
> I have no idea whether in the tool stack some memory calculations would
> want adjusting, to account for the 256k less that a HVM domain now
> needs.

I honestly don't know.  We need dalloc() and friends, simply to figure
out whether it's a significant number or not.

> --- a/xen/arch/x86/include/asm/hvm/io.h
> +++ b/xen/arch/x86/include/asm/hvm/io.h
> @@ -111,12 +111,10 @@ struct vpci_arch_msix_entry {
>  };
>  
>  struct hvm_hw_stdvga {
> -    struct page_info *vram_page[64];  /* shadow of 0xa0000-0xaffff */
>      spinlock_t lock;
>  };

I'm pretty sure you can drop the lock too.  It's taken in accept(), and
dropped in complete(), but there's no state at all to be protected.

stdvga_mem_accept()'s return value is a simple expression of p.


With that dropped, the complete() handler disappears, and it's the only
hvm_io_ops->complete() handler in Xen so the whole field can go.

So I'm pretty sure there are 2 more patches that ought to be part of
this series, which go in a further negative direction.

~Andrew

On 10.09.2024 20:28, Andrew Cooper wrote:
> On 10/09/2024 3:41 pm, Jan Beulich wrote:
>> No uses are left, hence its setup, teardown, and the field itself can
>> also go away. stdvga_deinit() is then empty and can be dropped as well.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>,

Thanks.

> although I think there's more to do.
> 
>> --- a/xen/arch/x86/include/asm/hvm/io.h
>> +++ b/xen/arch/x86/include/asm/hvm/io.h
>> @@ -111,12 +111,10 @@ struct vpci_arch_msix_entry {
>>  };
>>  
>>  struct hvm_hw_stdvga {
>> -    struct page_info *vram_page[64];  /* shadow of 0xa0000-0xaffff */
>>      spinlock_t lock;
>>  };
> 
> I'm pretty sure you can drop the lock too.  It's taken in accept(), and
> dropped in complete(), but there's no state at all to be protected.
> 
> stdvga_mem_accept()'s return value is a simple expression of p.

I think you're right. Previously I was assuming the lock was (also) about
serializing the bufioreq sending, yet ioreq_send_buffered() has its own
serialization. And hence yes, with all other state gone, the lock can go
too, as can ...

> With that dropped, the complete() handler disappears, and it's the only
> hvm_io_ops->complete() handler in Xen so the whole field can go.

... this hook.

> So I'm pretty sure there are 2 more patches that ought to be part of
> this series, which go in a further negative direction.

Will do.

Jan