[v5,01/23] memattrs: add debug attribute

Message ID 20171206200346.116537-2-brijesh.singh@amd.com
State New
Headers show
Series
  • x86: Secure Encrypted Virtualization (AMD)
Related show

Commit Message

Brijesh Singh Dec. 6, 2017, 8:03 p.m.
The debug attribute will be set when qemu attempts to access the guest
memory for debug (e.g memory access from gdbstub, memory dump commands
etc).

When guest memory is encrypted, the debug access will need to go through
the memory encryption APIs.

Cc: Alistair Francis <alistair.francis@xilinx.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 include/exec/memattrs.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Peter Maydell Dec. 6, 2017, 10:03 p.m. | #1
On 6 December 2017 at 20:03, Brijesh Singh <brijesh.singh@amd.com> wrote:
> The debug attribute will be set when qemu attempts to access the guest
> memory for debug (e.g memory access from gdbstub, memory dump commands
> etc).
>
> When guest memory is encrypted, the debug access will need to go through
> the memory encryption APIs.
>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  include/exec/memattrs.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index d4a16420984b..721362e06292 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -37,6 +37,8 @@ typedef struct MemTxAttrs {
>      unsigned int user:1;
>      /* Requester ID (for MSI for example) */
>      unsigned int requester_id:16;
> +    /* Debug memory access for encrypted guest */
> +    unsigned int debug:1;
>  } MemTxAttrs;

Can we have some more detailed semantics for this please?
For instance, if a device gets a debug=1 transaction
should it refuse to do things like read-clears-bits
semantics or other side-effects you wouldn't expect
of debugger accesses?

thanks
-- PMM
Brijesh Singh Dec. 7, 2017, 9:20 p.m. | #2
On 12/06/2017 04:03 PM, Peter Maydell wrote:
> On 6 December 2017 at 20:03, Brijesh Singh <brijesh.singh@amd.com> wrote:
>> The debug attribute will be set when qemu attempts to access the guest
>> memory for debug (e.g memory access from gdbstub, memory dump commands
>> etc).
>>
>> When guest memory is encrypted, the debug access will need to go through
>> the memory encryption APIs.
>>
>> Cc: Alistair Francis <alistair.francis@xilinx.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   include/exec/memattrs.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
>> index d4a16420984b..721362e06292 100644
>> --- a/include/exec/memattrs.h
>> +++ b/include/exec/memattrs.h
>> @@ -37,6 +37,8 @@ typedef struct MemTxAttrs {
>>       unsigned int user:1;
>>       /* Requester ID (for MSI for example) */
>>       unsigned int requester_id:16;
>> +    /* Debug memory access for encrypted guest */
>> +    unsigned int debug:1;
>>   } MemTxAttrs;
> 
> Can we have some more detailed semantics for this please?


Sure, I will add more details. The flag is mainly used to distinguish 
the debug vs non debug access when using address_space_read/write/rw().

If a debug=1 is set then flatview_read(), flatview_write(), 
flatview_rw() will use the encryption APIs when accessing the guest RAM 
case. Since the MMIO regions are not encrypted with guest-specific keys 
hence we don't need to do anything special for the MMIO cases.


> For instance, if a device gets a debug=1 transaction
> should it refuse to do things like read-clears-bits
> semantics or other side-effects you wouldn't expect
> of debugger accesses?
> 

Sorry I am not able to understand "if a device gets a debug=1 
transition" comment, Can you please explain me a bit more. If you give 
me example on how to trigger this type of request with debug=1 then I 
can look into the code and see what we can do when memory encryption is 
enabled. The things like read-clears-bits semantics will be tricky.


> thanks
> -- PMM
>
Peter Maydell Dec. 8, 2017, 9:55 a.m. | #3
On 7 December 2017 at 21:20, Brijesh Singh <brijesh.singh@amd.com> wrote:
> On 12/06/2017 04:03 PM, Peter Maydell wrote:
>> For instance, if a device gets a debug=1 transaction
>> should it refuse to do things like read-clears-bits
>> semantics or other side-effects you wouldn't expect
>> of debugger accesses?
>>
>
> Sorry I am not able to understand "if a device gets a debug=1 transition"
> comment, Can you please explain me a bit more.

A device model (eg a UART) can be written to look at the
MemTxAttrs that it's passed and behave differently if debug=1.

> If you give me example on how
> to trigger this type of request with debug=1 then I can look into the code
> and see what we can do when memory encryption is enabled. The things like
> read-clears-bits semantics will be tricky.

The question was really whether we want to make this a general
indicator of "this operation was triggered by a debugger" and
expand that to mean "don't do things that mess with the state
of the simulation unexpectedly", or if this is really a very
encrypted-memory specific thing.

By the way, I don't think this:

> /* Access the guest memory for debug purposes */
> #define MEMTXATTRS_DEBUG ((MemTxAttrs) { .debug = 1 })

is a great idea. Callers that care about the transaction
attributes should just specify them properly. MEMTXATTRS_UNSPECIFIED
is a fallback for the large set of places that don't care at all.

thanks
-- PMM
Edgar E. Iglesias Dec. 8, 2017, 10:24 a.m. | #4
On Fri, Dec 08, 2017 at 09:55:26AM +0000, Peter Maydell wrote:
> On 7 December 2017 at 21:20, Brijesh Singh <brijesh.singh@amd.com> wrote:
> > On 12/06/2017 04:03 PM, Peter Maydell wrote:
> >> For instance, if a device gets a debug=1 transaction
> >> should it refuse to do things like read-clears-bits
> >> semantics or other side-effects you wouldn't expect
> >> of debugger accesses?
> >>
> >
> > Sorry I am not able to understand "if a device gets a debug=1 transition"
> > comment, Can you please explain me a bit more.
> 
> A device model (eg a UART) can be written to look at the
> MemTxAttrs that it's passed and behave differently if debug=1.
> 
> > If you give me example on how
> > to trigger this type of request with debug=1 then I can look into the code
> > and see what we can do when memory encryption is enabled. The things like
> > read-clears-bits semantics will be tricky.
> 
> The question was really whether we want to make this a general
> indicator of "this operation was triggered by a debugger" and
> expand that to mean "don't do things that mess with the state
> of the simulation unexpectedly", or if this is really a very
> encrypted-memory specific thing.


IMO, It sounds like a good idea to make it a general indicator of
debugger access. We may not need to be very strict about an
exact global meaning, but individual device models may for example
choose to expose additional debug registers and features or
to avoid certain register access side-effects.

In some cases it may be confusing to change the behaviour of
existing regs with side-effects based on debug access, so it may
not always make sense.

Cheers,
Edgar


> 
> By the way, I don't think this:
> 
> > /* Access the guest memory for debug purposes */
> > #define MEMTXATTRS_DEBUG ((MemTxAttrs) { .debug = 1 })
> 
> is a great idea. Callers that care about the transaction
> attributes should just specify them properly. MEMTXATTRS_UNSPECIFIED
> is a fallback for the large set of places that don't care at all.
> 
> thanks
> -- PMM
Brijesh Singh Dec. 8, 2017, 10:57 p.m. | #5
On 12/8/17 3:55 AM, Peter Maydell wrote:
>> If you give me example on how
>> to trigger this type of request with debug=1 then I can look into the code
>> and see what we can do when memory encryption is enabled. The things like
>> read-clears-bits semantics will be tricky.
> The question was really whether we want to make this a general
> indicator of "this operation was triggered by a debugger" and
> expand that to mean "don't do things that mess with the state
> of the simulation unexpectedly", or if this is really a very
> encrypted-memory specific thing.

It can be used as a generic indicator that operation was triggered by a
debugger. There is not anything encryption specific. Having said that,
in the current patch series I have been only focused on making it work
from the gdbstub and HMP point of view. The debug=1 from gdbstub and HMP
is tested on both encrypted and non-encrypted guest. If we decide to
extend to support other callers (device model etc) then we may need to
update memory load/store functions defined in memory_ldst_inc.c to work
with debug=1.

> By the way, I don't think this:
>
>> /* Access the guest memory for debug purposes */
>> #define MEMTXATTRS_DEBUG ((MemTxAttrs) { .debug = 1 })
> is a great idea. Callers that care about the transaction
> attributes should just specify them properly. MEMTXATTRS_UNSPECIFIED
> is a fallback for the large set of places that don't care at all.

OK, I will drop the macro and update the patches in the series to set
MemTxAttr.debug = 1 when we do debugger request.  thanks

--Brijesh

Patch

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index d4a16420984b..721362e06292 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -37,6 +37,8 @@  typedef struct MemTxAttrs {
     unsigned int user:1;
     /* Requester ID (for MSI for example) */
     unsigned int requester_id:16;
+    /* Debug memory access for encrypted guest */
+    unsigned int debug:1;
 } MemTxAttrs;
 
 /* Bus masters which don't specify any attributes will get this,
@@ -56,4 +58,6 @@  typedef struct MemTxAttrs {
 #define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
 typedef uint32_t MemTxResult;
 
+/* Access the guest memory for debug purposes */
+#define MEMTXATTRS_DEBUG ((MemTxAttrs) { .debug = 1 })
 #endif