Re: [Qemu-devel] [RFC 2/2] hw/misc/vfio: add vfio-platform support

[Kim Phillips <kim.phillips@linaro.org>]

On Fri, 28 Feb 2014 11:03:18 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Tue, 2014-02-25 at 20:37 -0600, Kim Phillips wrote:
> > - support allocating a variable number of regions
> >   - VFIODevice's bars[] become dynamically allocated *regions
> >   - VFIOBAR's device fd replaced with parent VFIODevice ptr,
> >     to facilitate BAR r/w ops calling vfio_eoi()
> 
> On one hand, I had assumed we'd create a hw/misc/vfio/ directory and
> perhaps split things into pci, common, vga, and platform.  We already
> need the pci vs vga split as there's lots of irrelevant and complicated
> vga quirks that most people don't want to see.  On the other hand, I'm
> surprised how much of the PCI code you can re-use.  Still, I think it
> would be cleaner to create a VFIOPCIDevice and a VFIOPlatformDevice.

sounds good, had started down that path but reverted it because I too
realized how much code was indeed being reused.  I'm ok with
VFIOPCIDevice and a separate VFIOPlatformDevice for now, thanks.

> >  static void vfio_map_bars(VFIODevice *vdev)
> >  {
> >      int i;
> >  
> > +    if (!vdev->config_size) {
> > +        /* platform device */
> > +        for (i = 0; i < vdev->nr_regions; i++) {
> > +            vfio_map_region(vdev, i);
> > +        }
> > +        return;
> > +    }
> 
> I don't understand this, vfio_map_region() calls vfio_mmap_bar(), but
> vfio_mmap_bar() has been gutted so that it only initializes the mmap
> subregion, but never adds it.  vfio_map_bar() does both the
> initialization of the slow, read/write, memory region as well as adding
> the mmap sub-region.  I don't see where platform devices get a memory
> region inserted anywhere into the guest address space.

it's being done by the board code via the first RFC/patch ("hw/arm/virt:
add a Calxeda XGMAC device"), and vfio_mmap_bar()'s call to
memory_region_init_ram_ptr().

Yeah, the slow, r/w ops region path isn't used, and I'm not sure how to
make it work without addressing the problems the first RFC presents.

> > -    if (dev_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
> > +    if (dev_info.num_regions > PCI_NUM_REGIONS) ||
> > +        ((dev_info.flags & VFIO_DEVICE_FLAGS_PCI) &&
> > +         (dev_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1)) {
> 
> Now we start to have platform devices error out if they have more
> regions than PCI knows about...  That doesn't make much sense.

right, I should have made it more clear that the multiple-regions code
is sketchy in the "support allocating a variable number of regions"
blurb in the commit text of this RFC.

> > +static void register_vfio_platform_dev_type(void)
> > +{
> > +    type_register_static(&vfio_platform_dev_info);
> > +}
> > +
> > +type_init(register_vfio_platform_dev_type)
> 
> This all looks reasonable, but I suspect it would be cleaner if
> vfio_find_get_group() was in a common file along with basic mmap and
> read/write access functions.  Thanks,

so rename existing hw/misc/vfio.c to its original name vfio-pci.c, and
load all common functions back into a vfio.c?

Kim