* Jan Kiszka <[email protected]> [2017-09-14 06:42:38 +0000]: > On 2017-09-14 01:37, Gustavo Lima Chaves wrote: > > Those are: > > > > X86_OP_MOV_IMMEDIATE_TO_MEM 0xc7 > > X86_OP_MOV_MEM_TO_RAX 0xa1 > > X86_OP_MOV_RAX_TO_MEM 0xa3 > > Let's call this AX_TO_MEM / MEM_TO_AX - the register width depends on > other parameters. > > Can we split them up? I suppose handling of the immediate access differs > from the AX accesses.
Yes, for both. > > > > > The reason to bring them in is that the code is simple enough and will > > enable one more guest OS (Zephyr OS) to function in xAPIC mode as well. > > > > Signed-off-by: Gustavo Lima Chaves <[email protected]> > > --- > > hypervisor/arch/x86/apic.c | 6 ++++- > > hypervisor/arch/x86/include/asm/mmio.h | 3 +++ > > hypervisor/arch/x86/include/asm/processor.h | 3 +++ > > hypervisor/arch/x86/mmio.c | 35 > > ++++++++++++++++++++++++++++- > > 4 files changed, 45 insertions(+), 2 deletions(-) > > > > diff --git a/hypervisor/arch/x86/apic.c b/hypervisor/arch/x86/apic.c > > index f09d093..73e240b 100644 > > --- a/hypervisor/arch/x86/apic.c > > +++ b/hypervisor/arch/x86/apic.c > > @@ -504,7 +504,11 @@ unsigned int apic_mmio_access(unsigned long rip, > > return 0; > > } > > if (is_write) { > > - val = this_cpu_data()->guest_regs.by_index[inst.reg_num]; > > + if (inst.has_immediate) > > + val = inst.immediate; > > + else > > + val = > > this_cpu_data()->guest_regs.by_index[inst.reg_num]; > > + > > if (apic_accessing_reserved_bits(reg, val)) > > return 0; > > > > diff --git a/hypervisor/arch/x86/include/asm/mmio.h > > b/hypervisor/arch/x86/include/asm/mmio.h > > index 72aecab..5c26c5c 100644 > > --- a/hypervisor/arch/x86/include/asm/mmio.h > > +++ b/hypervisor/arch/x86/include/asm/mmio.h > > @@ -28,6 +28,9 @@ struct mmio_instruction { > > /** Number of the register that holds the output value or should > > * receive the input. */ > > unsigned int reg_num; > > + /** Immediate value, when due */ > > + signed int immediate; > > Just "int". Fair. > > > + bool has_immediate; > > We could also extend this in different way: use reg_num only for input > and define an output_value that is initialized by the parser to hold the > register value or the immediate. Saves the bool here and its testing above. Fair as well, will do. > > > }; > > > > /** > > diff --git a/hypervisor/arch/x86/include/asm/processor.h > > b/hypervisor/arch/x86/include/asm/processor.h > > index a658039..e3c36ed 100644 > > --- a/hypervisor/arch/x86/include/asm/processor.h > > +++ b/hypervisor/arch/x86/include/asm/processor.h > > @@ -148,6 +148,9 @@ > > #define X86_OP_MOVB_TO_MEM 0x88 > > #define X86_OP_MOV_TO_MEM 0x89 > > #define X86_OP_MOV_FROM_MEM 0x8b > > +#define X86_OP_MOV_IMMEDIATE_TO_MEM 0xc7 > > +#define X86_OP_MOV_MEM_TO_RAX 0xa1 > > +#define X86_OP_MOV_RAX_TO_MEM 0xa3 > > > > #define DB_VECTOR 1 > > #define NMI_VECTOR 2 > > diff --git a/hypervisor/arch/x86/mmio.c b/hypervisor/arch/x86/mmio.c > > index 7ca2ba8..7d3a3ab 100644 > > --- a/hypervisor/arch/x86/mmio.c > > +++ b/hypervisor/arch/x86/mmio.c > > @@ -78,7 +78,8 @@ struct mmio_instruction x86_mmio_parse(unsigned long pc, > > const struct guest_paging_structures *pg_structs, bool is_write) > > { > > struct parse_context ctx = { .remaining = X86_MAX_INST_LEN }; > > - struct mmio_instruction inst = { .inst_len = 0 }; > > + struct mmio_instruction inst = { .inst_len = 0, .has_immediate = false > > }; > > Overlong line, and strictly spoken unneeded (all fields will be > zero-initialized). OK. > > > + const u8 *inst_start = NULL; > > union opcode op[4] = { }; > > bool does_write = false; > > bool has_rex_w = false; > > @@ -88,6 +89,9 @@ restart: > > if (!ctx_maybe_get_bytes(&ctx, &pc, pg_structs)) > > goto error_noinst; > > > > + if (!inst_start) > > + inst_start = ctx.inst; > > + > > op[0].raw = *(ctx.inst); > > if (op[0].rex.code == X86_REX_CODE) { > > if (op[0].rex.w) > > @@ -126,6 +130,23 @@ restart: > > inst.inst_len += 2; > > inst.access_size = 4; > > break; > > + case X86_OP_MOV_IMMEDIATE_TO_MEM: > > + inst.inst_len += 2; > > + inst.access_size = 4; > > + inst.has_immediate = true; > > + does_write = true; > > + break; > > + case X86_OP_MOV_MEM_TO_RAX: > > + inst.inst_len += 5; > > + inst.access_size = 4; > > + inst.reg_num = 15; > > + goto final; > > + case X86_OP_MOV_RAX_TO_MEM: > > + inst.inst_len += 5; > > + inst.access_size = 4; > > + inst.reg_num = 15; > > + does_write = true; > > + goto final; > > default: > > goto error_unsupported; > > } > > @@ -138,6 +159,8 @@ restart: > > case 0: > > if (op[2].modrm.rm == 5) { /* 32-bit displacement */ > > inst.inst_len += 4; > > + if (inst.has_immediate) > > + inst.inst_len += 4; > > inst.inst_len += inst.has_immediate ? 8 : 4; OK. > > > break; > > } else if (op[2].modrm.rm != 4) { /* no SIB */ > > break; > > @@ -168,6 +191,16 @@ restart: > > else > > inst.reg_num = 15 - op[2].modrm.reg; > > > > +final: > > + /* FIXME: what if an instruction with immediate spans two > > + * pages? */ > > That has to be addressed (another reason to split the patch, because we > can then already handle the other two instructions). Just look at how we > do this for the instruction so far and follow the same path. I suspect this is not done in this file (before one would collect all the state needed progressively), but I'll sure search better how to achieve that. Thanks a lot! > > > + if (inst.has_immediate) { > > + if (ctx.inst - inst_start + ctx.size < inst.inst_len) > > + goto error_noinst; > > + inst.immediate = *(typeof(inst.immediate)*) > > + (inst_start + inst.inst_len - sizeof(inst.immediate)); > > + } > > + > > if (does_write != is_write) > > goto error_inconsitent; > > > > > > Jan > > -- > Siemens AG, Corporate Technology, CT RDA ITP SES-DE > Corporate Competence Center Embedded Linux > > -- > You received this message because you are subscribed to the Google Groups > "Jailhouse" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/d/optout. -- Gustavo Lima Chaves Intel - Open Source Technology Center -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
