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

[Alex Williamson]

On Tue, 2014-03-04 at 18:24 -0600, Kim Phillips wrote:
> On Fri, 28 Feb 2014 11:03:18 -0700
> Alex Williamson <address@hidden> 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?

I think hw/misc/vfio/{pci.c,common.c,platform.c,etc}  Thanks,

Alex