On Fri, 2014-03-28 at 14:42 +1100, Alexey Kardashevskiy wrote:
> On 03/20/2014 06:57 AM, Alex Williamson wrote:
> > On Wed, 2014-03-12 at 16:52 +1100, Alexey Kardashevskiy wrote:
> >> From: David Gibson <da...@gibson.dropbear.id.au>
> >>
> >> The only model so far supported for VFIO passthrough devices is the model
> >> usually used on x86, where all of the guest's RAM is mapped into the
> >> (host) IOMMU and there is no IOMMU visible in the guest.
> >>
> >> This patch begins to relax this model, introducing the notion of a
> >> VFIOAddressSpace. This represents a logical DMA address space which will
> >> be visible to one or more VFIO devices by appropriate mapping in the (host)
> >> IOMMU. Thus the currently global list of containers becomes local to
> >> a VFIOAddressSpace, and we verify that we don't attempt to add a VFIO
> >> group to multiple address spaces.
> >>
> >> For now, only one VFIOAddressSpace is created and used, corresponding to
> >> main system memory, that will change in future patches.
> >>
> >> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> >> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
> >>
> >> ---
> >> Changes:
> >> v5:
> >> * vfio_get_group() now receives AddressSpace*
> >>
> >> v4:
> >> * removed redundant checks and asserts
> >> * fixed some return error codes
> >> ---
> >> hw/misc/vfio.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
> >> 1 file changed, 40 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >> index 6a04c2a..c8236c3 100644
> >> --- a/hw/misc/vfio.c
> >> +++ b/hw/misc/vfio.c
> >> @@ -133,6 +133,22 @@ enum {
> >> VFIO_INT_MSIX = 3,
> >> };
> >>
> >> +typedef struct VFIOAddressSpace {
> >> + AddressSpace *as;
> >> + QLIST_HEAD(, VFIOContainer) containers;
> >> + QLIST_ENTRY(VFIOAddressSpace) list;
> >> +} VFIOAddressSpace;
> >> +
> >> +QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces;
> >
> > Why isn't this static and initialized with QLIST_HEAD_INITIALIZER like
> > the qlist it replaces?
>
>
> I'll use QLIST_HEAD_INITIALIZER, ok.
>
>
> It is not static because otherwise it does not compile - the actual use of
> vfio_address_spaces happens in the next patches. Should I make it static
> and move to the next patch? Thanks.
So you're working around an unused variable compile failure by making it
non-static? Why not just move declaring it until the next patch?
Thanks,
Alex
> >> +
> >> +static VFIOAddressSpace vfio_address_space_memory;
> >> +
> >> +static void vfio_address_space_init(VFIOAddressSpace *space, AddressSpace
> >> *as)
> >> +{
> >> + space->as = as;
> >> + QLIST_INIT(&space->containers);
> >> +}
> >> +
> >> struct VFIOGroup;
> >>
> >> typedef struct VFIOType1 {
> >> @@ -142,6 +158,7 @@ typedef struct VFIOType1 {
> >> } VFIOType1;
> >>
> >> typedef struct VFIOContainer {
> >> + VFIOAddressSpace *space;
> >> int fd; /* /dev/vfio/vfio, empowered by the attached groups */
> >> struct {
> >> /* enable abstraction to support various iommu backends */
> >> @@ -234,9 +251,6 @@ static const VFIORomBlacklistEntry romblacklist[] = {
> >>
> >> #define MSIX_CAP_LENGTH 12
> >>
> >> -static QLIST_HEAD(, VFIOContainer)
> >> - container_list = QLIST_HEAD_INITIALIZER(container_list);
> >> -
> >> static QLIST_HEAD(, VFIOGroup)
> >> group_list = QLIST_HEAD_INITIALIZER(group_list);
> >>
> >> @@ -3280,16 +3294,15 @@ static void vfio_kvm_device_del_group(VFIOGroup
> >> *group)
> >> #endif
> >> }
> >>
> >> -static int vfio_connect_container(VFIOGroup *group)
> >> +static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
> >> {
> >> VFIOContainer *container;
> >> int ret, fd;
> >> + VFIOAddressSpace *space;
> >>
> >> - if (group->container) {
> >> - return 0;
> >> - }
> >> + space = &vfio_address_space_memory;
> >>
> >> - QLIST_FOREACH(container, &container_list, next) {
> >> + QLIST_FOREACH(container, &space->containers, next) {
> >> if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
> >> group->container = container;
> >> QLIST_INSERT_HEAD(&container->group_list, group,
> >> container_next);
> >> @@ -3312,6 +3325,7 @@ static int vfio_connect_container(VFIOGroup *group)
> >> }
> >>
> >> container = g_malloc0(sizeof(*container));
> >> + container->space = space;
> >> container->fd = fd;
> >>
> >> if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU)) {
> >> @@ -3349,7 +3363,7 @@ static int vfio_connect_container(VFIOGroup *group)
> >> }
> >>
> >> QLIST_INIT(&container->group_list);
> >> - QLIST_INSERT_HEAD(&container_list, container, next);
> >> + QLIST_INSERT_HEAD(&space->containers, container, next);
> >>
> >> group->container = container;
> >> QLIST_INSERT_HEAD(&container->group_list, group, container_next);
> >> @@ -3392,7 +3406,7 @@ static void vfio_disconnect_container(VFIOGroup
> >> *group)
> >> }
> >> }
> >>
> >> -static VFIOGroup *vfio_get_group(int groupid)
> >> +static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
> >> {
> >> VFIOGroup *group;
> >> char path[32];
> >> @@ -3400,7 +3414,14 @@ static VFIOGroup *vfio_get_group(int groupid)
> >>
> >> QLIST_FOREACH(group, &group_list, next) {
> >> if (group->groupid == groupid) {
> >> - return group;
> >> + /* Found it. Now is it already in the right context? */
> >> + if (group->container->space->as == as) {
> >> + return group;
> >> + } else {
> >> + error_report("vfio: group %d used in multiple address
> >> spaces",
> >> + group->groupid);
> >> + return NULL;
> >> + }
> >> }
> >> }
> >>
> >> @@ -3428,7 +3449,7 @@ static VFIOGroup *vfio_get_group(int groupid)
> >> group->groupid = groupid;
> >> QLIST_INIT(&group->device_list);
> >>
> >> - if (vfio_connect_container(group)) {
> >> + if (vfio_connect_container(group, as)) {
> >> error_report("vfio: failed to setup container for group %d",
> >> groupid);
> >> goto close_fd_exit;
> >> }
> >> @@ -3780,7 +3801,12 @@ static int vfio_initfn(PCIDevice *pdev)
> >> DPRINTF("%s(%04x:%02x:%02x.%x) group %d\n", __func__,
> >> vdev->host.domain,
> >> vdev->host.bus, vdev->host.slot, vdev->host.function,
> >> groupid);
> >>
> >> - group = vfio_get_group(groupid);
> >> + if (pci_device_iommu_address_space(pdev) != &address_space_memory) {
> >> + error_report("vfio: DMA address space must be system memory");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + group = vfio_get_group(groupid, &address_space_memory);
> >> if (!group) {
> >> error_report("vfio: failed to get group %d", groupid);
> >> return -ENOENT;
> >> @@ -3994,6 +4020,7 @@ static const TypeInfo vfio_pci_dev_info = {
> >>
> >> static void register_vfio_pci_dev_type(void)
> >> {
> >> + vfio_address_space_init(&vfio_address_space_memory,
> >> &address_space_memory);
> >> type_register_static(&vfio_pci_dev_info);
> >> }
> >>
> >
> >
> >
>
>
