| Message ID | 147377816100.11859.1924921034992764815.stgit@brijesh-build-machine |
|---|---|
| State | New |
| Headers | show |
On 13/09/2016 16:49, Brijesh Singh wrote: > > + /* Register SEV read/write ops for the guest RAM */ > + if (kvm_sev_enabled()) > + memory_region_set_ram_ops(ram, kvm_sev_get_ram_ops()); If you don't actually need this one except for -kernel it would be very nice, because then the hooks could be limited to cpu_memory_rw_debug. address_space_write and address_space_read are the central entry point for device DMA, and calling mr->ram_ops->write from there seems very wrong. I'd rather make those hooks *ROM* read/write ops rather than RAM read/write ops. Paolo
Hi Paolo, On 09/13/2016 06:05 PM, Paolo Bonzini wrote: > > > On 13/09/2016 16:49, Brijesh Singh wrote: >> >> + /* Register SEV read/write ops for the guest RAM */ >> + if (kvm_sev_enabled()) >> + memory_region_set_ram_ops(ram, kvm_sev_get_ram_ops()); > > If you don't actually need this one except for -kernel it would be very > nice, because then the hooks could be limited to cpu_memory_rw_debug. > Yes so far i see that we needing this only for -kernel option. > address_space_write and address_space_read are the central entry point > for device DMA, and calling mr->ram_ops->write from there seems very > wrong. I'd rather make those hooks *ROM* read/write ops rather than RAM > read/write ops. > 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? -Brijesh
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. Paolo
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 cpu_physical_memory_rw address_space_rw address_space_write/read How do you want me to handle these cases? Having SEV RAM callback taking care this internally was my simplest solution, I am certainly open to new ideas.
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. Paolo
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.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 022dd1b..1471df4 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -46,6 +46,7 @@ #include "sysemu/sysemu.h" #include "sysemu/numa.h" #include "sysemu/kvm.h" +#include "sysemu/sev.h" #include "sysemu/qtest.h" #include "kvm_i386.h" #include "hw/xen/xen.h" @@ -1387,6 +1388,10 @@ void pc_memory_init(PCMachineState *pcms, e820_add_entry(0x100000000ULL, pcms->above_4g_mem_size, E820_RAM); } + /* Register SEV read/write ops for the guest RAM */ + if (kvm_sev_enabled()) + memory_region_set_ram_ops(ram, kvm_sev_get_ram_ops()); + if (!pcmc->has_reserved_memory && (machine->ram_slots || (machine->maxram_size > machine->ram_size))) { diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index f915ad0..95b1006 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -35,6 +35,7 @@ #include "sysemu/sysemu.h" #include "hw/block/flash.h" #include "sysemu/kvm.h" +#include "sysemu/sev.h" #define BIOS_FILENAME "bios.bin" @@ -228,6 +229,11 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) memory_region_add_subregion(rom_memory, (uint32_t)(-bios_size), bios); + + /* Register SEV read/write callback */ + if (kvm_sev_enabled()) { + memory_region_set_ram_ops(bios, kvm_sev_get_ram_ops()); + } } void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
If guest is launched into SEV-enabled mode then read/write to the BIOS and RAM memory regions should be performed using the SEV commands. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- hw/i386/pc.c | 5 +++++ hw/i386/pc_sysfw.c | 6 ++++++ 2 files changed, 11 insertions(+)