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

[Paolo Bonzini]

On 15/09/2016 16:13, Brijesh Singh wrote:
> 1) something like this
> 
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index a9d8aef..6322265 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1379,13 +1379,22 @@ void x86_cpu_exec_exit(CPUState *cs)
>  }
> 
>  #ifndef CONFIG_USER_ONLY
> +static inline MemTxAttrs get_mem_debug_attrs(CPUX86State *env)
> +{
> +    MemTxAttrs attrs = cpu_get_mem_attrs(env);
> +
> +    attrs.debug = MEMTXATTRS_DEBUG;
> +
> +    return attrs;
> +}
> +
>  uint8_t x86_ldub_phys(CPUState *cs, hwaddr addr)
>  {
>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
> 
>      return address_space_ldub(cs->as, addr,
> -                              cpu_get_mem_attrs(env),
> +                              get_mem_debug_attrs(env),
>                                NULL);
>  }

This changes the semantics of x86_ld*_phys so it's not acceptable.

You need new exec.c functions that wrap
cpu_physical_memory_rw_debug_internal so that they end up calling your
hooks.  Like this:

x86_cpu_get_phys_page_debug
  -> calls ldl_phys_debug
      -> calls cpu_physical_memory_rw_debug_internal

uint32_t ldl_phys_debug(CPUState *cs, hwaddr addr)
{
    /* This really should be a little more complex to support
     * SMRAM, but this is enough for now.  I'll provide you with a
     * diff when you post v2.
     */
    MemTxAttrs attrs = { .debug = 1 };
    int asidx = cpu_asidx_from_attrs(cpu, attrs);
    uint32_t val;

    cpu_physical_memory_rw_debug_internal(cpu->cpu_ases[asidx].as,
                                          addr, (void *) &val,
                                          4, READ_DATA);
    return tswap32(val);
}

/* and the same for ldq_phys_debug */

These two new functions can replace x86_ld*_phys in
x86_cpu_get_phys_page_debug.

Technically I'm not even sure you need the new .debug attribute now that
the hooks are never called by the device emulation DMA path.  Still,
asserting something like assert(attrs->debug || sev_info.state ==
SEV_LAUNCH_START) in your hooks is probably a good idea.  Even if you
only need the new attribute for that assertion, keep it.  It's just a
couple lines of code.

Paolo