Re: [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation

["Ефимов Василий" <real@ispras.ru>]

16.07.2015 20:52, Paolo Bonzini пишет:
>
>
> On 16/07/2015 16:41, Ефимов Василий wrote:
>> The main problem is rendering memory tree to FlatView.
>
> I don't believe it's necessary to render a memory tree to the FlatView.
>   You can use existing AddressSpaces.
>
>>> +    /* Read from RAM and write to PCI */
>>> +    memory_region_init_io(&pam->region[1], OBJECT(dev), &pam_ops, pam,
>>> +            "pam-r-ram-w-pci", size);
>>>
>>> This can be done with memory_region_set_readonly on the RAM region.  You
>>> need to set mr->ops in order to point to pam_ops; for a first proof of
>>> concept you can just set the field directly.
>> The idea is to read directly from system RAM region and to write
>> to PCI using I/O (because I do not see another way to emulate
>> access type driven redirection with existent API). If we create RAM
>> and make it read only then new useless RAM block will be created.
>
> Don't create RAM; modify the existing one.
>
>> It is useless because of ram_addr of new region will be set to
>> one within system RAM block. Hence, cleaner way is to create I/O region.
>
> You can use the existing RAM region and modify its properties (i.e.
> toggle mr->readonly) after setting special mr->ops.  The special mr->ops
> will be used for writes when mr->readonly = true.
The existing RAMs are ether whole pc.ram or pc.rom and pc.bios beyond
PCI. So, I think we have not invade them. Also, we only need the small 
subrange to be read only. Moreover, if -pflash is used then there is
ROM device instead of pc.bios with its own mr->ops.

You have suggested to create AddressSpace for PCI below. It is flexible
solution which is transparent for existing regions. I suggests to create
AddressSpace for RAM subtree too.  I think, both AS have to be PAM
implementation private for complete transparency for another QEMU parts.

I will try it in next patch version.
>
>>> Writes to the PCI memory space can use the PCI address space, with
>>> address_space_st*.
>> There is no PCI AddressSpace (only MemoryRegion). But
>> address_space_st* requires AddressSpace as argument.
>
> Then create one with address_space_init.
>
> However, can the guest see the difference between "real" mode 1 and the
> "fake" mode 1 that QEMU implements?  Perhaps mode 1 can be left as is.
Yes, guest can.
If -pfalsh is used then BIOS becomes a programmable ROM device. Then
guest can switch to mode 1 and copy data by reading and writing it
at same location. After that guest can switch to mode 0 (or 3) and
copy data to some another place. Then it switches to mode 3 (or 0) and
compares the data.

I just check it with SeaBIOS and original PAM adding code listed below
to fw/shadow.c: __make_bios_writable_intel.

     dprintf(1, "PAM mode 1 test begin\n");
     unsigned *m = (unsigned *) BUILD_ROM_START;

     pci_config_writeb(bdf, pam0 + 1, 0x33);
     *m = 0xdeafbeef;

     pci_config_writeb(bdf, pam0 + 1, 0x11);
     volatile unsigned t = *m;
     *m = t;

     pci_config_writeb(bdf, pam0 + 1, 0x33);
     t = *m;

     pci_config_writeb(bdf, pam0 + 1, 0x00);
     unsigned t2 = *m;

     dprintf(1, "t = 0x%x, t2 = 0x%x\n", t, t2);

     dprintf(1, "PAM mode 1 test end\n");

The output is:

PAM mode 1 test begin
t = 0xdeafbeef, t2 = 0x0
PAM mode 1 test end

With new PAM the output is:

PAM mode 1 test begin
t = 0xdeafbeef, t2 = 0xdeafbeef
PAM mode 1 test end

Note BUILD_ROM_START is 0xc0000 which is pc.rom with write permission
according to info mtree output. The -pflash is not needed.

>
>>> +    /* Read from PCI and write to RAM */
>>> +    memory_region_init_io(&pam->region[2], OBJECT(dev), &pam_ops, pam,
>>> +            "pam-r-pci-w-ram", size);
>>>
>>> Here you cannot run code from ROM, so it can be a pure MMIO region.
>>> Reads can use address_space_ld*, while writes can use
>>> memory_region_get_ram_ptr.
>>
>> Even in this mode it is possible for code to be executed from ROM. This
>> can happen when particular PCI address is within ROM device connected
>> to PCI bus.
>
> If it's just for pc.rom and isa-bios, introduce a new function
> pam_create_pci_region that creates pc.rom with
> memory_region_init_rom_device.  The mr->ops can write to RAM (mode 2) or
> discard the write (mode 0).
>
> They you can make pc.rom 256K instead of 128K, and instead of an alias,
> you can manually copy the last 128K of the BIOS into the last 128K of
> pc.rom.
>
> Some adjustment will be necessary in order to support migration (perhaps
> creating two 128K regions pc.rom and pc.rom.mirror), but for a proof of
> concept the above should be enough.
I will try solution based on address spaces as stated above. It is 
cleaner and more generic.
>
> Paolo
>
Vasily