On Wed, 2008-07-30 at 11:33 +0530, Amit Shah wrote:
> * On Tuesday 29 July 2008 15:08:27 Ben-Ami Yassour wrote:
> > On Tue, 2008-07-29 at 14:49 +0530, Amit Shah wrote:
> > > * On Monday 28 Jul 2008 21:56:26 Ben-Ami Yassour wrote:
> > > > +static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
> > > > + struct kvm_assigned_pci_dev
> > > > *assigned_dev)
> > > > +{
> > > >
> > > >
> > > > + if (pci_enable_device(dev)) {
> > > > + printk(KERN_INFO "%s: Could not enable PCI device\n",
> > > > __func__);
> > > > + r = -EBUSY;
> > > > + goto out_put;
> > > > + }
> > > > + r = pci_request_regions(dev, "kvm_assigned_device");
> > > > + if (r) {
> > > > + printk(KERN_INFO "%s: Could not get access to device
> > > > regions\n",
> > > > + __func__);
> > > > + goto out_disable;
> > >
> > > Shouldn't disable here unconditionally (see my comment earlier to the
> > > previous patch).
> >
> > Why? the device should not be used by the host at the same time.
> > What is the condition that you were thinking of?
> >
> > > > +static void kvm_free_assigned_devices(struct kvm *kvm)
> > > > +{
> > > > + struct list_head *ptr, *ptr2;
> > > > + struct kvm_assigned_dev_kernel *assigned_dev;
> > > > +
> > > > + list_for_each_safe(ptr, ptr2, &kvm->arch.assigned_dev_head) {
> > > > + assigned_dev = list_entry(ptr,
> > > > + struct
> > > > kvm_assigned_dev_kernel,
> > > > + list);
> > > > +
> > > > + if (irqchip_in_kernel(kvm) &&
> > > > assigned_dev->irq_requested) {
> > > > + free_irq(assigned_dev->host_irq,
> > > > + (void *)assigned_dev);
> > > > +
> > > > + kvm_unregister_irq_ack_notifier(kvm,
> > > > + &assigned_dev->
> > > > + ack_notifier);
> > > > + }
> > >
> > > Unregister the notifier before freeing irq
> >
> > I don't think that there is any importance to the order in this case,
> > but just in case, the order should be in reverse to the initialization
> > order, which is the case here.
>
> Just that we shouldn't accept any new interrupts to ack because we're going
> to
> free the irq as the next step. This is more relevant now that we don't have
> the rwlock for the device assignment structures. If the work gets scheduled
> after we free the resources, it's going to bomb:
These are two separate things.
The ack notifier is called when the guest is signaling the virutal
IOAPIC with EOI, and it has no direct relation with the host interrupt
handler.
The connection is that when the interrupt is generated, the host inject
it to the guest, and then the guest will eventually do EOI. But we are
not going to run the guest after we started to run this termination code
so this is irrelevant. Even if it was, then the right order is first to
make sure that there are no interrupts received in the host and then
cancel the notifier (which is the reverse order of the initialization).
Regards,
Ben
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
