Re: [PATCH 5/6] device assignment: support for assigning PCI devices to guests

[Muli Ben-Yehuda <muli@il.ibm.com>]

On Tue, Oct 28, 2008 at 10:45:57AM -0500, Anthony Liguori wrote:

>> +ifeq ($(USE_KVM), 1)
>> +OBJS+= device-assignment.o
>> +endif
>
> I don't think you want to build this on PPC so I think you need a
> stronger check.

Good point. How about checking TARGET_BASE_ARCH = i386?

>> +static void assigned_dev_ioport_writel(void *opaque, uint32_t addr,
>> +                       uint32_t value)
>> +{
>> +    AssignedDevRegion *r_access = opaque;
>> +    uint32_t r_pio = guest_to_host_ioport(r_access, addr);
>> +
>> +    DEBUG("%s: r_pio=%08x e_physbase=%08x r_virtbase=%08lx value=%08x\n",
>> +	  r_pio, (int)r_access->e_physbase,
>> +          (unsigned long)r_access->r_virtbase, value);
>>   
>
> The format doesn't match the parameter count.

Yep, already fixed.

>> +static void assigned_dev_ioport_map(PCIDevice *pci_dev, int region_num,
>> +                                    uint32_t addr, uint32_t size, int 
>> type)
>> +{
>> +    AssignedDevice *r_dev = (AssignedDevice *) pci_dev;
>> +    AssignedDevRegion *region = &r_dev->v_addrs[region_num];
>> +    uint32_t old_port = region->u.r_baseport;
>> +    uint32_t old_num = region->e_size;
>> +    int first_map = (old_num == 0);
>> +    struct ioperm_data data;
>> +    int i;
>> +
>> +    region->e_physbase = addr;
>> +    region->e_size = size;
>> +
>> +    DEBUG("e_phys=0x%x r_baseport=%x type=0x%x len=%d region_num=%d \n",
>> +          addr, region->u.r_baseport, type, size, region_num);
>> +
>> +    memset(&data, 0, sizeof(data));
>> +
>> +    if (!first_map) {
>> +	data.start_port = old_port;
>> +	data.num = old_num; +	data.turn_on = 0;
>> +
>> +	for (i = 0; i < smp_cpus; ++i)
>> +	    kvm_ioperm(qemu_kvm_cpu_env(i), &data);
>>   
>
> How does this interact with VCPU hot-plug?

I have no idea. Weidong?

>> +#ifdef KVM_CAP_IOMMU
>> +        /* We always enable the IOMMU if present
>> +         * (or when not disabled on the command line)
>> +         */
>> +        r = kvm_check_extension(kvm_context, KVM_CAP_IOMMU);
>> +        if (r && !disable_iommu)
>> +            assigned_dev_data.flags |= KVM_DEV_ASSIGN_ENABLE_IOMMU;
>> +#endif
>> +        r = kvm_assign_pci_device(kvm_context, &assigned_dev_data);
>> +        if (r < 0) {
>> +            fprintf(stderr, "Could not notify kernel about "
>> +                "assigned device \"%s\"\n", e_dev_name);
>> +            perror("register_real_device");
>> +            goto out;
>> +        }
>> +    }
>>   
>
> You still succeed if KVM_CAP_DEVICE_ASSIGNMENT isn't defined?  That
> means a newer userspace compiled on an older kernel will silently
> fail if they try to do device assignment.  There's probably no
> reason to build this file if KVM_CAP_DEVICE_ASSIGNMENT isn't defined
> (see how the in-kernel PIT gets conditionally build depending on
> whether that cap is available).

Ok, I'll take a look at this.

>> +#endif
>> +    term_printf("Registered host PCI device %02x:%02x.%1x "
>> +		"(\"%s\") as guest device %02x:%02x.%1x\n",
>> +		r_bus, r_dev, r_func, e_dev_name,
>> +		pci_bus_num(e_bus), e_device, r_func);
>>
>>   
>
> If I read the code correctly, this term_printf() happens regardless
> of whether this is being done for PCI hotplug or for command-line
> assignment?  That's a problem as it'll print garbage on the monitor
> when you start QEMU which could break management applications.

Is there a more suitable alternative or shall I just nuke it?

>> diff --git a/qemu/hw/pc.c b/qemu/hw/pc.c
>> index d559f0c..5fdb726 100644
>> --- a/qemu/hw/pc.c
>> +++ b/qemu/hw/pc.c
>> @@ -33,6 +33,7 @@
>>  #include "boards.h"
>>  #include "console.h"
>>  #include "fw_cfg.h"
>> +#include "device-assignment.h"
>>   #include "qemu-kvm.h"
>>  @@ -1157,6 +1158,21 @@ static void pc_init1(ram_addr_t ram_size, int 
>> vga_ram_size,
>>       if (pci_enabled)
>>          virtio_balloon_init(pci_bus);
>> +
>> +    if (kvm_enabled() && device_assignment_enabled) {
>> +	int i;
>>   
>
> Stray tab.

Grrr. Silly emacs.

>
>> +        for (i = 0; i < assigned_devices_index; i++) {
>> +            if (add_assigned_device(assigned_devices[i]) < 0) {
>> +                fprintf(stderr, "Warning: could not add assigned device 
>> %s\n",
>> +                        assigned_devices[i]);
>> +            }
>> +        }
>> +
>> +	if (init_all_assigned_devices(pci_bus)) {
>> +	    fprintf(stderr, "Failed to initialize assigned devices\n");
>> +	    exit (1);
>> +	}
>> +    }
>>  }
>>  +#if defined(TARGET_I386) || defined(TARGET_X86_64) || defined(__linux__)
>> +            case QEMU_OPTION_pcidevice:
>> +		device_assignment_enabled = 1;
>> +		if (assigned_devices_index >= MAX_DEV_ASSIGN_CMDLINE) {
>> +                    fprintf(stderr, "Too many assigned devices\n");
>> +                    exit(1);
>> +		}
>> +		assigned_devices[assigned_devices_index] = optarg;
>> +		assigned_devices_index++;
>> +                break;
>>   
>
> Tab damage.

Thanks, will fix in the next revision.

Cheers,
Muli
-- 
The First Workshop on I/O Virtualization (WIOV '08)
Dec 2008, San Diego, CA, http://www.usenix.org/wiov08/
                       <->
SYSTOR 2009---The Israeli Experimental Systems Conference
http://www.haifa.il.ibm.com/conferences/systor2009/