Re: [PATCH] kvm-userspace: Load PCI option ROMs

[Avi Kivity <avi@redhat.com>]

Liu, Kechao wrote:
> Load assigned devices' PCI option ROMs to the RAM of
> guest OS. And pass the corresponding devfns to BIOS.
>
>   

Sorry about the late review.

> +
> +    addr = qemu_ram_alloc(size);
> +    phys_addr = addr + phys_ram_base;
> +
> +    /* Write ROM data and devfn to phys_addr */
> +    memcpy((void *)phys_addr, rom, rom->rom_size * 512);
> +    *(uint8_t *)(phys_addr + rom->rom_size * 512) = devfn;
>   

phys_ram_base is dangerous.  It works in this case, but we'd like to 
avoid it.  Please use cpu_physical_memory_write_rom().

> +
> +    cpu_register_physical_memory(0xd0000 + offset, size, addr);
>   

Best to register this as a rom, though kvm will ignore it (we don't 
implement read-only memory at this time).

> +
> +/*
> + * Scan the assigned devices for the devices that have an option ROM, 
> + * and then load the corresponding ROM data to RAM.
> + */
> +ram_addr_t assigned_dev_load_option_roms(ram_addr_t rom_base_offset)
> +{
> +    ram_addr_t offset = rom_base_offset;
> +    AssignedDevInfo *adev;
> +
> +    LIST_FOREACH(adev, &adev_head, next) {
> +        int size;
> +        void *buf;
> +        FILE *fp;
> +        char rom_file[64]; 
> +        char cmd[64];
> +
> +        snprintf(rom_file, sizeof(rom_file), 
> +                 "/sys/bus/pci/devices/0000:%02x:%02x.%01x/rom", 
> +                 adev->bus, adev->dev, adev->func);
> +   
> +        if (access(rom_file, F_OK))
> +            continue;
> +
> +        /* Write something to the ROM file to enable it */
> +        snprintf(cmd, sizeof(cmd), "echo 1 > %s", rom_file);
> +        system(cmd);
>   

system()?!  Please just write to the file.

> +
> +        fp = fopen(rom_file, "rb");
> +        if (fp == NULL)
> +            continue;
> +
> +        /* Read the data of the ROM file to the buffer */
> +        fseek(fp, 0, SEEK_END);
> +        size = ftell(fp);
> +        fseek(fp, 0, SEEK_SET);
> +        buf = malloc(size);
> +        fread(buf, size, 1, fp);
> +
>   

Missing error checking.

> +        /* Scan the buffer for suitable ROMs and increase the offset */
> +        offset += scan_option_rom(adev->assigned_dev->dev.devfn, buf, offset);
> +        
> +        free(buf);
> +        fclose(fp);
> +    }
> +
> +    return offset;
> +}
> diff --git a/qemu/hw/device-assignment.h b/qemu/hw/device-assignment.h
>   

-- 
error compiling committee.c: too many arguments to function