| Message ID | abbd917c-d13d-4572-ae9e-1c413c7d4cf2@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/HVM: drop stdvga caching mode | expand |
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
--- 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);
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.