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
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
