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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200804292128.52268.amit.shah@qumranet.com>
Date:	Tue, 29 Apr 2008 21:28:51 +0530
From:	Amit Shah <amit.shah@...ranet.com>
To:	Glauber Costa <gcosta@...hat.com>
Cc:	kvm-devel@...ts.sourceforge.net,
	virtualization@...ts.linux-foundation.org, muli@...ibm.com,
	BENAMI@...ibm.com, chrisw@...hat.com, dor.laor@...ranet.com,
	allen.m.kay@...el.com, avi@...ranet.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM x86: Handle hypercalls for assigned PCI devices

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