Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
Alex Williamson writes:
> On Mon, 2013-06-17 at 13:56 +1000, Benjamin Herrenschmidt wrote:
>> On Sun, 2013-06-16 at 21:13 -0600, Alex Williamson wrote:
>>
>> > IOMMU groups themselves don't provide security, they're accessed by
>> > interfaces like VFIO, which provide the security. Given a brief look, I
>> > agree, this looks like a possible backdoor. The typical VFIO way to
>> > handle this would be to pass a VFIO file descriptor here to prove that
>> > the process has access to the IOMMU group. This is how /dev/vfio/vfio
>> > gains the ability to setup an IOMMU domain an do mappings with the
>> > SET_CONTAINER ioctl using a group fd. Thanks,
>>
>> How do you envision that in the kernel ? IE. I'm in KVM code, gets that
>> vfio fd, what do I do with it ?
>>
>> Basically, KVM needs to know that the user is allowed to use that iommu
>> group. I don't think we want KVM however to call into VFIO directly
>> right ?
>
> Right, we don't want to create dependencies across modules. I don't
> have a vision for how this should work. This is effectively a complete
> side-band to vfio, so we're really just dealing in the iommu group
> space. Maybe there needs to be some kind of registration of ownership
> for the group using some kind of token. It would need to include some
> kind of notification when that ownership ends. That might also be a
> convenient tag to toggle driver probing off for devices in the group.
> Other ideas? Thanks,
It's actually not that bad.
eg.
struct vfio_container *vfio_container_from_file(struct file *filp)
{
if (filp->f_op != &vfio_device_fops)
return ERR_PTR(-EINVAL);
/* OK it really is a vfio fd, return the data. */
}
EXPORT_SYMBOL_GPL(vfio_container_from_file);
...
inside KVM_CREATE_SPAPR_TCE_IOMMU:
struct file *vfio_filp;
struct vfio_container *(lookup)(struct file *filp);
vfio_filp = fget(create_tce_iommu.fd);
if (!vfio)
ret = -EBADF;
lookup = symbol_get(vfio_container_from_file);
if (!lookup)
ret = -EINVAL;
else {
container = lookup(vfio_filp);
if (IS_ERR(container))
ret = PTR_ERR(container);
else
...
symbol_put(vfio_container_from_file);
}
symbol_get() won't try to load a module; it'll just fail. This is what
you want, since they must have vfio in the kernel to get a valid fd...
Hope that helps,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] BUILD_BUG_ON: make it handle more cases
On Thu, 5 Nov 2009 05:08:42 pm Stephen Rothwell wrote: > Hi Rusty, > > On Thu, 5 Nov 2009 16:58:36 +1030 Rusty Russell wrote: > > > > Huh? virtio_has_feature does: > > > > if (__builtin_constant_p(fbit)) > > BUILD_BUG_ON(fbit >= 32); > > else > > BUG_ON(fbit >= 32); > > In Linus' tree (and linux-next) it looks like this: Ah. My patch series fixes that as part of removing MAYBE_BUILD_BUG_ON. I've put both in for linux-next. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] BUILD_BUG_ON: make it handle more cases
BUILD_BUG_ON used to use the optimizer to do code elimination or fail
at link time; it was changed to first the size of a negative array (a
nicer compile time error), then (in
8c87df457cb58fe75b9b893007917cf8095660a0) to a bitfield.
bitfields: needs a literal constant at parse time, and can't be put under
"if (__builtin_constant_p(x))" for example.
negative array: can handle anything, but if the compiler can't tell it's
a constant, silently has no effect.
link time: breaks link if the compiler can't determine the value, but the
linker output is not usually as informative as a compiler error.
If we use the negative-array-size method *and* the link time trick,
we get the ability to use BUILD_BUG_ON() under __builtin_constant_p()
branches, and maximal ability for the compiler to detect errors at
build time.
Signed-off-by: Rusty Russell
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -683,12 +683,6 @@ struct sysinfo {
char _f[20-2*sizeof(long)-sizeof(int)]; /* Padding: libc5 uses this.. */
};
-/* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))
-
-/* Force a compilation error if condition is constant and true */
-#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))
-
/* Force a compilation error if condition is true, but also produce a
result (of value 0 and type size_t), so the expression can be used
e.g. in a structure initializer (or where-ever else comma expressions
@@ -696,6 +690,33 @@ struct sysinfo {
#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
+/**
+ * BUILD_BUG_ON - break compile if a condition is true.
+ * @cond: the condition which the compiler should know is false.
+ *
+ * If you have some code which relies on certain constants being equal, or
+ * other compile-time-evaluated condition, you should use BUILD_BUG_ON to
+ * detect if someone changes it.
+ *
+ * The implementation uses gcc's reluctance to create a negative array, but
+ * gcc (as of 4.4) only emits that error for obvious cases (eg. not arguments
+ * to inline functions). So as a fallback we use the optimizer; if it can't
+ * prove the condition is false, it will cause a link error on the undefined
+ * "__build_bug_on_failed". This error message can be harder to track down
+ * though, hence the two different methods.
+ */
+#ifndef __OPTIMIZE__
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#else
+extern int __build_bug_on_failed;
+#define BUILD_BUG_ON(condition)\
+ do {\
+ ((void)sizeof(char[1 - 2*!!(condition)])); \
+ if (condition) __build_bug_on_failed = 1; \
+ } while(0)
+#endif
+#define MAYBE_BUILD_BUG_ON(condition) BUILD_BUG_ON(condition)
+
/* Trap pasters of __FUNCTION__ at compile-time */
#define __FUNCTION__ (__func__)
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: linux-next: tree build failure
On Tue, 20 Oct 2009 04:49:29 am Hollis Blanchard wrote: > On Thu, 2009-10-15 at 08:27 +0100, Jan Beulich wrote: > > My perspective is that it just uncovered already existing brokenness. > > Sorry, I thought it was clear, but to be more explicit: I propose the > following patch, which replaces the current BUILD_BUG_ON implementation > with Rusty's version. OK, I switched my brain back on. Yeah, I agree: we may still want BUILD_OR_RUNTIME_BUG_ON one day, but I like this. It's just missing the giant comment that it needs :) /** * BUILD_BUG_ON - break compile if a condition is true. * @cond: the condition which the compiler should know is false. * * If you have some code which relies on certain constants being equal, or * other compile-time-evaluated condition, you should use BUILD_BUG_ON to * detect if someone changes it. * * The implementation uses gcc's reluctance to create a negative array, but * gcc (as of 4.4) only emits that error for obvious cases (eg. not arguments * to inline functions). So as a fallback we use the optimizer; if it can't * prove the condition is false, it will cause a link error on the undefined * "__build_bug_on_failed". This error is less neat, and can be harder to * track down. */ Thanks! Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [v2] linux: virtio: Standardize virtio's concept of "page size"
On Friday 14 November 2008 08:18:33 Hollis Blanchard wrote:
> On Thu, 2008-11-13 at 08:44 +1030, Rusty Russell wrote:
> > Note that I still don't have a balloon patch: want to send me one?
>
> linux: virtio-balloon: avoid implicit use of Linux page size in balloon
> interface
Thanks, applied with following diff:
Use tabs to indent, and put BUILD_BUG_ON pagesize assumption.
Signed-off-by: Rusty Russell <[EMAIL PROTECTED]>
diff -r 50e970613233 drivers/virtio/virtio_balloon.c
--- a/drivers/virtio/virtio_balloon.c Fri Nov 14 11:40:38 2008 +1030
+++ b/drivers/virtio/virtio_balloon.c Fri Nov 14 11:41:19 2008 +1030
@@ -58,10 +58,11 @@
static u32 page_to_balloon_pfn(struct page *page)
{
-unsigned long pfn = page_to_pfn(page);
+ unsigned long pfn = page_to_pfn(page);
-/* Convert pfn from Linux page size to balloon page size. */
-return pfn >> (PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT);
+ BUILD_BUG_ON(PAGE_SHIFT < VIRTIO_BALLOON_PFN_SHIFT);
+ /* Convert pfn from Linux page size to balloon page size. */
+ return pfn >> (PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT);
}
static void balloon_ack(struct virtqueue *vq)
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [v2] linux: virtio: Standardize virtio's concept of "page size"
On Thursday 13 November 2008 02:46:31 Hollis Blanchard wrote: > On Wed, 2008-11-12 at 22:51 +1030, Rusty Russell wrote: > > On Tuesday 11 November 2008 10:07:09 Hollis Blanchard wrote: > > > Both sides of the virtio interface must agree about how big a pfn > > > really is. This is particularly an issue on architectures where the > > > page size is configurable (e.g. PowerPC, IA64) -- the interface must be > > > independent of PAGE_SHIFT. > > > > > > Currently there are three distinct problems: > > > * The shift count used when passing the physical address of the ring to > > > a PCI-based back end. > > > * The ring layout itself is padded to span at least two "pages". > > > * The balloon driver operates in units of "pages". > > > > Hi Hollis, > > > >The more I thought about this, the more I think we're not solving this > > as neatly as we could. The trigger was noting that we're breaking the > > userspace API (vring_size and vring_init are exposed to userspace): I > > know that qemu cut & pastes, but that's no excuse. > > > >So instead, I've introduced separate constants for each use. Yes, > > all these constants are 12/4096. But just to be contrary, at the end > > is a patch to change lguest to 128. And there's no reason this > > couldn't change in future using some guest detection scheme. > > OK. I thought it was simpler to just say "4KB everywhere" in all aspects > of the virtio interface, but I'm happy as long as we solve the problem > somehow. :) It is simpler, yes, but we can take this opportunity to deconflate them and make things clearer and better than the current code, not just "fix" it. Note that I still don't have a balloon patch: want to send me one? Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [v2] linux: virtio: Standardize virtio's concept of "page size"
On Tuesday 11 November 2008 10:07:09 Hollis Blanchard wrote: > Both sides of the virtio interface must agree about how big a pfn really > is. This is particularly an issue on architectures where the page size is > configurable (e.g. PowerPC, IA64) -- the interface must be independent of > PAGE_SHIFT. > > Currently there are three distinct problems: > * The shift count used when passing the physical address of the ring to a > PCI-based back end. > * The ring layout itself is padded to span at least two "pages". > * The balloon driver operates in units of "pages". Hi Hollis, The more I thought about this, the more I think we're not solving this as neatly as we could. The trigger was noting that we're breaking the userspace API (vring_size and vring_init are exposed to userspace): I know that qemu cut & pastes, but that's no excuse. So instead, I've introduced separate constants for each use. Yes, all these constants are 12/4096. But just to be contrary, at the end is a patch to change lguest to 128. And there's no reason this couldn't change in future using some guest detection scheme. Patch stream to follow... Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: large page size virtio issues
On Wednesday 05 November 2008 09:14:20 Hollis Blanchard wrote: > Hi Rusty, I'm using a patch that changes the Linux base page size to > 64K. (This is actually pretty common in ppc64 world, but I happen to be > trying it on ppc32.) > > I'm seeing a problem with virtio. I think at least part of it can be > explained by qemu's TARGET_PAGE_BITS==12, and the guest's > PAGE_SHIFT==16. The guest allocates the queue, then passes the pfn (pa > > >> PAGE_SHIFT) to the virtio backend (vp_find_vq()). The backend then > > calculates the pa as pfn << TARGET_PAGE_BITS. > > I have to run right now, but quickly changing qemu TARGET_PAGE_BITS to > 16 got me a little further but still didn't work. Any thoughts? I see Anthony hardwired page size into the queue activation ABI for virtio_pci. I think that this should be an actual 4096 (or 12) rather than depending on guest page size: virtio_pci.c:247: iowrite32(virt_to_phys(info->queue) >> PAGE_SHIFT, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN); Anthony? Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
