Re: [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash

[Jordan Justen]

On Mon, Jan 9, 2012 at 01:28, Kevin Wolf <address@hidden> wrote:
> Am 19.12.2011 23:19, schrieb Anthony Liguori:
>> On 12/19/2011 03:25 PM, Jordan Justen wrote:
>>> On Mon, Dec 19, 2011 at 11:41, Anthony Liguori<address@hidden>  wrote:
>>>> On 12/15/2011 02:51 PM, Jordan Justen wrote:
>>>>> +static void pc_system_rom_init(MemoryRegion *rom_memory,
>>>>> +                               DriveInfo *pflash_drv)
>>>>> +{
>>>>> +    BlockDriverState *bdrv;
>>>>> +    int64_t size;
>>>>> +    target_phys_addr_t phys_addr;
>>>>> +    int sector_bits, sector_size;
>>>>> +    MemoryRegion *sys_rom;
>>>>> +    void *buffer;
>>>>> +    int ret;
>>>>> +
>>>>> +    bdrv = pflash_drv->bdrv;
>>>>> +    size = bdrv_getlength(pflash_drv->bdrv);
>>>>> +    sector_bits = 9;
>>>>> +    sector_size = 1<<    sector_bits;
>>>>> +
>>>>> +    if ((size % sector_size) != 0) {
>>>>> +        fprintf(stderr,
>>>>> +                "qemu: PC system rom (pflash) must be a multiple of
>>>>> 0x%x\n",
>>>>> +                sector_size);
>>>>> +        exit(1);
>>>>> +    }
>>>>> +
>>>>> +    phys_addr = 0x100000000ULL - size;
>>>>> +    sys_rom = g_malloc(sizeof(*sys_rom));
>>>>> +    memory_region_init_ram(sys_rom, NULL, "system.rom", size);
>>>>> +    buffer = memory_region_get_ram_ptr(sys_rom);
>>>>> +    memory_region_add_subregion(rom_memory, phys_addr, sys_rom);
>>>>> +
>>>>> +    /* read the rom content */
>>>>> +    ret = bdrv_read(bdrv, 0, buffer, size>>    sector_bits);
>>>>
>>>>
>>>> I think we're trying to get rid of synchronous block I/O in machine
>>>> initialization for a number of reasons.
>>>>
>>>> Kevin/Stefan, care to comment?  Will this be problematic in the future?
>>>
>>> I was hoping pc-1.1 with and without kvm could be as close as
>>> possible, but I guess I can make pc-1.1 with kvm behave the same as
>>> pc-1.0.  Then I can delete pc_system_rom_init.
>>
>> I think your general approach is right, I'm just not sure what we're going 
>> to do
>> short term about synchronous I/O in the machine init routines.  It may just 
>> be a
>> matter of structuring this in such a way that you can use an async interface.
>
> I don't think there is a problem with using the synchronous interface
> here. But I'm not sure if having any I/O in the machine init is a good
> idea with respect to migration. Didn't we already have to move some code
> there to a later stage to make sure that the destination doesn't use
> outdated data?

The pc_system_rom_init function is only used when kvm is enabled.
Since kvm cannot currently execute code from device memory, we have to
create a ram region, and transfer the pflash contents into the ram
region.

Instead of bdrv_read, should I get the pflash memory region buffer,
and use memcpy?  Or, would still suffer from the same issue of getting
outdated data?

Thanks,

-Jordan