Re: [PATCH 1 of 3] Use kvm_x86 to hold x86 specific kvm fields

[Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

On Wed, 2007-11-28 at 15:20 -0600, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > diff --git a/drivers/kvm/ioapic.c b/drivers/kvm/ioapic.c
> > --- a/drivers/kvm/ioapic.c
> > +++ b/drivers/kvm/ioapic.c
> > @@ -276,7 +276,9 @@ static int get_eoi_gsi(struct kvm_ioapic
> >  
> >  void kvm_ioapic_update_eoi(struct kvm *kvm, int vector)
> >  {
> > -     struct kvm_ioapic *ioapic = kvm->vioapic;
> > +     struct kvm_x86 *kvm_x86 = to_kvm_x86(kvm);
> > +
> > +     struct kvm_ioapic *ioapic = kvm_x86->vioapic;
> >       union ioapic_redir_entry *ent;
> >       int gsi;
> >  
> > @@ -386,11 +388,12 @@ int kvm_ioapic_init(struct kvm *kvm)
> >  int kvm_ioapic_init(struct kvm *kvm)
> >  {
> >       struct kvm_ioapic *ioapic;
> > +     struct kvm_x86 *kvm_x86 = to_kvm_x86(kvm);
> >  
> >       ioapic = kzalloc(sizeof(struct kvm_ioapic), GFP_KERNEL);
> >       if (!ioapic)
> >               return -ENOMEM;
> > -     kvm->vioapic = ioapic;
> > +     kvm_x86->vioapic = ioapic;
> >       kvm_ioapic_reset(ioapic);
> >       ioapic->dev.read = ioapic_mmio_read;
> >       ioapic->dev.write = ioapic_mmio_write;
> >   
> 
> If the ioapic depends on x86_specific information, why is it taking a 
> generic kvm structure instead of the x86 one?

Fine, I can move the conversion up into x86.c .

> > diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> > --- a/drivers/kvm/kvm_main.c
> > +++ b/drivers/kvm/kvm_main.c
> > @@ -232,6 +232,7 @@ int __kvm_set_memory_region(struct kvm *
> >       unsigned long i;
> >       struct kvm_memory_slot *memslot;
> >       struct kvm_memory_slot old, new;
> > +     struct kvm_x86 *kvm_x86 = to_kvm_x86(kvm);
> >  
> >       r = -EINVAL;
> >       /* General sanity checks */
> > @@ -332,18 +333,18 @@ int __kvm_set_memory_region(struct kvm *
> >       if (mem->slot >= kvm->nmemslots)
> >               kvm->nmemslots = mem->slot + 1;
> >  
> > -     if (!kvm->n_requested_mmu_pages) {
> > +     if (!kvm_x86->n_requested_mmu_pages) {
> >   
> 
> Why is anything in kvm_main.c accessing a memory of kvm_x86?
> Something is broken in the abstraction if that's the case.

kvm_main.c was chock full of x86 code. The fact that there's still a
little bit remaining (even after all our work so far) shouldn't be
surprising.

> >               unsigned int n_pages;
> >  
> >               if (npages) {
> >                       n_pages = npages * KVM_PERMILLE_MMU_PAGES /
> 1000;
> > -                     kvm_mmu_change_mmu_pages(kvm,
> kvm->n_alloc_mmu_pages +
> > -                                              n_pages);
> > +                     kvm_mmu_change_mmu_pages(kvm,
> > +                                       kvm_x86->n_alloc_mmu_pages +
> n_pages);
> >               } else {
> >                       unsigned int nr_mmu_pages;
> >  
> >                       n_pages = old.npages *
> KVM_PERMILLE_MMU_PAGES / 1000;
> > -                     nr_mmu_pages = kvm->n_alloc_mmu_pages -
> n_pages;
> > +                     nr_mmu_pages = kvm_x86->n_alloc_mmu_pages -
> n_pages;
> >                       nr_mmu_pages = max(nr_mmu_pages,
> >                                       (unsigned int)
> KVM_MIN_ALLOC_MMU_PAGES);
> >                       kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
> > diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
> > --- a/drivers/kvm/mmu.c
> > +++ b/drivers/kvm/mmu.c
> > @@ -526,12 +526,14 @@ static void kvm_mmu_free_page(struct kvm
> >  static void kvm_mmu_free_page(struct kvm *kvm,
> >                             struct kvm_mmu_page *page_head)
> >  {
> > +     struct kvm_x86 *kvm_x86 = to_kvm_x86(kvm);
> > +
> >       ASSERT(is_empty_shadow_page(page_head->spt));
> >       list_del(&page_head->link);
> >       __free_page(virt_to_page(page_head->spt));
> >       __free_page(virt_to_page(page_head->gfns));
> >       kfree(page_head);
> > -     ++kvm->n_free_mmu_pages;
> > +     ++kvm_x86->n_free_mmu_pages;
> >  }
> 
> The mmu functions should probably take kvm_x86 since this won't be 
> shared with any other architecture.

Sure, and that can be addressed in the future. However, changing all of
that code at once results in a far larger patch, and this patch works
as-is.

-- 
Hollis Blanchard
IBM Linux Technology Center


-------------------------------------------------------------------------
SF.Net email is sponsored by: The Future of Linux Business White Paper
from Novell.  From the desktop to the data center, Linux is going
mainstream.  Let it simplify your IT future.
http://altfarm.mediaplex.com/ad/ck/8857-50307-18918-4