Re: [Qemu-devel] [RFC PATCH v1 15/22] i386: sev: register RAM read/write ops for BIOS and PC.RAM region

[Brijesh Singh <brijesh.singh@amd.com>]

On 09/14/2016 04:52 PM, Paolo Bonzini wrote:
>
>
> On 14/09/2016 23:47, Brijesh Singh wrote:
>>
>>
>> On 09/14/2016 04:00 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 14/09/2016 22:59, Brijesh Singh wrote:
>>>> I will look into hooking up the callback into ROM read/write ops. I was
>>>> thinking about adding a new argument in
>>>> cpu_physical_memory_write_rom_internal()
>>>>
>>>> void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
>>>>                                    const uint8_t *buf, int len,
>>>>                                    WriteCB *cb)
>>>> {
>>>>    ....
>>>>    ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
>>>>
>>>>    if (cb)
>>>>      cb(ptr, buf, len)
>>>>    else
>>>>      memcpy(ptr, buf, len)
>>>> ....
>>>>
>>>> }
>>>>
>>>> In case of SEV, we pass a CB function pointer which calls SEV API's to
>>>> encrypt memory. Does this make sense?
>>>
>>> I think a global as you have it in this series is just fine---just don't
>>> hook it into address_space_read and address_space_write.
>>>
>>
>> Actually in SEV RAM callback I check the Attrs, if attr.sev_debug flag
>> set then use SEV debug command otherwise default to memcpy so that DMA
>> and everything else works. I guest the main reason why i choose to hook
>> this up in address_space_read/write was that I found that
>> address_space_write and address_space_read is used in debug path. e.g
>>
>> cpu_memory_rw_debug
>>   address_space_rw
>>     address_space_write/read
>
> Right, but if you change this to a ROM hook only, cpu_memory_rw_debug
> will go through cpu_physical_memory_write_rom instead.  This will invoke
> the hook properly, won't it?  It will break -kernel unless fw_cfg DMA is
> disabled, of course.
>

maybe I am missing something.

here is what I see:

int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
                         uint8_t *buf, int len, int is_write)
{
   ............

    if (is_write)
        cpu_physical_memory_write_rom_internal()
     else
        address_space_rw()

    .....

}

So looking at code, i have impression that write will go through the 
cpu_physical_memory_write_rom but the read will still go through 
address_space_rw which will eventually invoke address_space_read.

Also when user tries to read or write to a physical address through qemu 
monitor then it will invoke cpu_physical_memory_rw which will eventually 
use address_space_write and address_space_read to read/write the guest 
memory.