Re: [Qemu-devel] [RFC PATCH] vfio: VFIO Driver core framework

[Alex Williamson]

On Wed, 2011-11-16 at 11:47 -0600, Scott Wood wrote:
> On 11/11/2011 04:10 PM, Alex Williamson wrote:
> > 
> > Thanks Konrad!  Comments inline.
> > 
> > On Fri, 2011-11-11 at 12:51 -0500, Konrad Rzeszutek Wilk wrote:
> >> On Thu, Nov 03, 2011 at 02:12:24PM -0600, Alex Williamson wrote:
> >>> +When supported, as indicated by the device flags, reset the device.
> >>> +
> >>> +#define VFIO_DEVICE_RESET               _IO(';', 116)
> >>
> >> Does it disable the 'count'? Err, does it disable the IRQ on the
> >> device after this and one should call VFIO_DEVICE_SET_IRQ_EVENTFDS
> >> to set new eventfds? Or does it re-use the eventfds and the device
> >> is enabled after this?
> > 
> > It doesn't affect the interrupt programming.  Should it?
> 
> It should probably clear any currently pending interrupts, as if the
> unmask IOCTL were called.

Sounds reasonable.

> >>> +device tree properties of the device:
> >>> +
> >>> +struct vfio_dtpath {
> >>> +        __u32   len;            /* length of structure */
> >>> +        __u32   index;
> >>
> >> 0 based I presume?
> > 
> > Everything else is, I would assume so/
> 
> Yes, it should be zero-based -- this matches how such indices are done
> in the kernel device tree APIs.
> 
> >>> +        __u64   flags;
> >>> +#define VFIO_DTPATH_FLAGS_REGION        (1 << 0)
> >>
> >> What is region in this context?? Or would this make much more sense
> >> if I knew what Device Tree actually is.
> > 
> > Powerpc guys, any comments?  This was their suggestion.  These are
> > effectively the first device specific extension, available when
> > VFIO_DEVICE_FLAGS_DT is set.
> 
> An assigned device may consist of an entire subtree of the device tree,
> and both register banks and interrupts can come from any node in the
> tree.  Region versus IRQ here indicates the context in which to
> interpret index, in order to retrieve the path of the node that supplied
> this particular region or IRQ.

Ok.  Thanks for the clarification.  We'll wait for the vfio-dt bus
driver before actually including this.

> >>> +};
> >>> +#define VFIO_DEVICE_GET_DTPATH          _IOWR(';', 117, struct 
> >>> vfio_dtpath)
> >>> +
> >>> +struct vfio_dtindex {
> >>> +        __u32   len;            /* length of structure */
> >>> +        __u32   index;
> >>> +        __u32   prop_type;
> >>
> >> Is that an enum type? Is this definied somewhere?
> >>> +        __u32   prop_index;
> >>
> >> What is the purpose of this field?
> > 
> > Need input from powerpc folks here
> 
> To identify what this resource (register bank or IRQ) this is, we need
> both the path to the node and the index into the reg or interrupts
> property within the node.
> 
> We also need to distinguish reg from ranges, and interrupts from
> interrupt-map.  As you suggested elsewhere in the thread, the device
> tree API should probably be left out for now, and added later along with
> the device tree "bus" driver.

Yep, I'll do that.

> >>> +static void __vfio_iommu_detach_dev(struct vfio_iommu *iommu,
> >>> +                             struct vfio_device *device)
> >>> +{
> >>> + BUG_ON(!iommu->domain && device->attached);
> >>
> >> Whoa. Heavy hammer there.
> >>
> >> Perhaps WARN_ON as you do check it later on.
> > 
> > I think it's warranted, internal consistency is broken if we have a
> > device that thinks it's attached to an iommu domain that doesn't exist.
> > It should, of course, never happen and this isn't a performance path.
> > 
> [snip]
> >>> +static int __vfio_iommu_attach_dev(struct vfio_iommu *iommu,
> >>> +                            struct vfio_device *device)
> >>> +{
> >>> + int ret;
> >>> +
> >>> + BUG_ON(device->attached);
> >>
> >> How about:
> >>
> >> WARN_ON(device->attached, "The engineer who wrote the user-space device 
> >> driver is trying to register
> >> the device again! Tell him/her to stop please.\n");
> > 
> > I would almost demote this one to a WARN_ON, but userspace isn't in
> > control of attaching and detaching devices from the iommu.  That's a
> > side effect of getting the iommu or device file descriptor.  So again,
> > this is an internal consistency check and it should never happen,
> > regardless of userspace.
> 
> The rule isn't to use BUG for internal consistency checks and WARN for
> stuff userspace can trigger, but rather to use BUG if you cannot
> reasonably continue, WARN for "significant issues that need prompt
> attention" that are reasonably recoverable.  Most instances of WARN are
> internal consistency checks.

That makes sense.

> From include/asm-generic/bug.h:
> > If you're tempted to BUG(), think again:  is completely giving up
> > really the *only* solution?  There are usually better options, where
> > users don't need to reboot ASAP and can mostly shut down cleanly.

Ok, I'll make a cleanup pass of demoting BUG_ONs to WARN_ONs.  Thanks,

Alex