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?
Kim
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
