Re: [PATCH] KVM x86: Handle hypercalls for assigned PCI devices

[Amit Shah <amit.shah@qumranet.com>]

On Tuesday 29 April 2008 20:14:16 Glauber Costa wrote:
> Amit Shah wrote:

> > +static struct kvm_pv_dma_map*
> > +find_pci_pv_dmap(struct list_head *head, dma_addr_t dma)
> > +{
>
> might be better to prefix those functions with kvm? Even though they are
> static, it seems to be the current practice.

The function names are long enough already. Prefixing everything with kvm_ 
could hurt the eye as well.

> > +	host_page = gfn_to_page(vcpu->kvm, page_gfn);
>
> you need mmap_sem held for read to use gfn_to_page.

Yes; it's going to trickle down soon.

> > +	/* FIXME: guest should send the direction */
> > +	r = dma_ops->map_sg(NULL, sg, npages, PCI_DMA_BIDIRECTIONAL);
> > +	if (r) {
> > +		r = npages;
> > +		*hcall_page = sg[0].dma_address | (*hcall_page & ~PAGE_MASK);
> > +	}
> > +
> > + out_unmap:
> > +	if (!r)
> > +		*hcall_page = bad_dma_address;
> > +	kunmap(host_page);
> > + out:
> > +	++vcpu->stat.hypercall_map;
> > +	return r;
> > + out_unmap_sg_dmap:
> > +	kfree(dmap);
> > + out_unmap_sg:
> > +	kfree(sg);
> > +	goto out_unmap;
>
> those backwards goto are very clumsy. Might be better to give it a
> further attention in order to avoid id.

It does keep everything nicely in one place though. You're right though. Some 
more attention is needed.

> > +static int free_dmap(struct kvm_pv_dma_map *dmap, struct list_head
> > *head) +{
> > +	int i;
> > +
> > +	if (!dmap)
> > +		return 1;
>
> that's ugly.
>
> it's better to keep the free function with free-like semantics: just a
> void function that plainly returns if !dmap, and check in the caller.

I was lazy and used the return value from here to propagate it down further. 
But this kicked me to modify that.

> > +	if (is_error_page(host_page)) {
> > +		printk(KERN_INFO "%s: gfn %p not valid\n",
> > +		       __func__, (void *)page_gfn);
> > +		r = -1;
>
> r = -1 is not really informative. Better use some meaningful error.

The error's going to the guest. The guest, as we know, has already done a 
successful DMA allocation. Something went wrong in the hypercall, and we 
don't know why (bad page). Any kind of error here isn't going to be 
intelligible to the guest anyway. It's mostly a host thing if we ever hit 
this.

> > +	if (find_pci_pt_dev(&vcpu->kvm->arch.pci_pt_dev_head,
> > +			    &pci_pt_info, 0, KVM_PT_SOURCE_ASSIGN))
> > +		r++; /* We have assigned the device */
> > +
> > +	kunmap(host_page);
>
> better use atomic mappings here.

We can't use atomic mappings for guest pages. They can be swapped out.