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

quoted text
> > +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.

quoted text
> > +	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.

quoted text
> > +	/* 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.

quoted text
> > +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.

quoted text
> > +	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.

quoted text
> > +	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.
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/