On Tue, 2014-03-04 at 18:24 -0600, Kim Phillips wrote:
> On Fri, 28 Feb 2014 11:03:18 -0700
> Alex Williamson <alex.williamson@xxxxxxxxxx> 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
