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

[Brijesh Singh]

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.