Re: [PATCH] x86/emul: Use existing X86_EXC_* constants
On 06/04/2023 1:50 pm, Jan Beulich wrote: > On 06.04.2023 14:17, Andrew Cooper wrote: >> ... rather than having separate definitions locally. EXC_HAS_EC in >> particular >> is missing #CP, #VC and #SX vs X86_EXC_HAVE_EC. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper > Acked-by: Jan Beulich Thanks. > Yet more re-basing for me to do ... And me... I'm somewhat alarmed with how may branch collisions I've had with the TRAP_* change. > But yes, it needs to happen at some > point, and I guess there never really is a good time. Yeah, but doing it together with the TRAP_* change is going to be least disruptive (overall) to others. ~Andrew
Re: [PATCH] x86/emul: Use existing X86_EXC_* constants
On 06.04.2023 14:17, Andrew Cooper wrote: > ... rather than having separate definitions locally. EXC_HAS_EC in particular > is missing #CP, #VC and #SX vs X86_EXC_HAVE_EC. > > No functional change. > > Signed-off-by: Andrew Cooper Acked-by: Jan Beulich Yet more re-basing for me to do ... But yes, it needs to happen at some point, and I guess there never really is a good time. Jan
[PATCH] x86/emul: Use existing X86_EXC_* constants
... rather than having separate definitions locally. EXC_HAS_EC in particular
is missing #CP, #VC and #SX vs X86_EXC_HAVE_EC.
No functional change.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
---
xen/arch/x86/x86_emulate/x86_emulate.c | 608 -
1 file changed, 294 insertions(+), 314 deletions(-)
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
b/xen/arch/x86/x86_emulate/x86_emulate.c
index c69f7c65f526..8aa23b929c07 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -135,28 +135,6 @@ static const uint8_t sse_prefix[] = { 0x66, 0xf3, 0xf2 };
/* MXCSR bit definitions. */
#define MXCSR_MM (1U << 17)
-/* Exception definitions. */
-#define EXC_DE 0
-#define EXC_DB 1
-#define EXC_BP 3
-#define EXC_OF 4
-#define EXC_BR 5
-#define EXC_UD 6
-#define EXC_NM 7
-#define EXC_DF 8
-#define EXC_TS 10
-#define EXC_NP 11
-#define EXC_SS 12
-#define EXC_GP 13
-#define EXC_PF 14
-#define EXC_MF 16
-#define EXC_AC 17
-#define EXC_XM 19
-
-#define EXC_HAS_EC \
-((1u << EXC_DF) | (1u << EXC_TS) | (1u << EXC_NP) | \
- (1u << EXC_SS) | (1u << EXC_GP) | (1u << EXC_PF) | (1u << EXC_AC))
-
/* Segment selector error code bits. */
#define ECODE_EXT (1 << 0)
#define ECODE_IDT (1 << 1)
@@ -360,11 +338,11 @@ do {
\
#define validate_far_branch(cs, ip) ({ \
if ( sizeof(ip) <= 4 ) {\
ASSERT(!ctxt->lma); \
-generate_exception_if((ip) > (cs)->limit, EXC_GP, 0); \
+generate_exception_if((ip) > (cs)->limit, X86_EXC_GP, 0); \
} else \
generate_exception_if(ctxt->lma && (cs)->l \
? !is_canonical_address(ip) \
- : (ip) > (cs)->limit, EXC_GP, 0); \
+ : (ip) > (cs)->limit, X86_EXC_GP, 0); \
})
#define commit_far_branch(cs, newip) ({ \
@@ -431,7 +409,7 @@ int x86emul_get_fpu(
return rc;
generate_exception_if(!(cr4 & ((type == X86EMUL_FPU_xmm)
? X86_CR4_OSFXSR :
X86_CR4_OSXSAVE)),
- EXC_UD);
+ X86_EXC_UD);
}
rc = ops->read_cr(0, , ctxt);
@@ -444,13 +422,13 @@ int x86emul_get_fpu(
}
if ( cr0 & X86_CR0_EM )
{
-generate_exception_if(type == X86EMUL_FPU_fpu, EXC_NM);
-generate_exception_if(type == X86EMUL_FPU_mmx, EXC_UD);
-generate_exception_if(type == X86EMUL_FPU_xmm, EXC_UD);
+generate_exception_if(type == X86EMUL_FPU_fpu, X86_EXC_NM);
+generate_exception_if(type == X86EMUL_FPU_mmx, X86_EXC_UD);
+generate_exception_if(type == X86EMUL_FPU_xmm, X86_EXC_UD);
}
generate_exception_if((cr0 & X86_CR0_TS) &&
(type != X86EMUL_FPU_wait || (cr0 & X86_CR0_MP)),
- EXC_NM);
+ X86_EXC_NM);
}
done:
@@ -776,7 +754,7 @@ static int ioport_access_check(
return rc == X86EMUL_DONE ? X86EMUL_OKAY : rc;
/* Ensure the TSS has an io-bitmap-offset field. */
-generate_exception_if(tr.type != 0xb, EXC_GP, 0);
+generate_exception_if(tr.type != 0xb, X86_EXC_GP, 0);
switch ( rc = read_ulong(x86_seg_tr, 0x66, , 2, ctxt, ops) )
{
@@ -784,7 +762,7 @@ static int ioport_access_check(
break;
case X86EMUL_EXCEPTION:
-generate_exception_if(!ctxt->event_pending, EXC_GP, 0);
+generate_exception_if(!ctxt->event_pending, X86_EXC_GP, 0);
/* fallthrough */
default:
@@ -799,7 +777,7 @@ static int ioport_access_check(
break;
case X86EMUL_EXCEPTION:
-generate_exception_if(!ctxt->event_pending, EXC_GP, 0);
+generate_exception_if(!ctxt->event_pending, X86_EXC_GP, 0);
/* fallthrough */
default:
@@ -807,7 +785,7 @@ static int ioport_access_check(
}
generate_exception_if(iobmp & (((1 << bytes) - 1) << (first_port & 7)),
- EXC_GP, 0);
+ X86_EXC_GP, 0);
done:
return rc;
@@ -854,7 +832,7 @@ protmode_load_seg(
uint8_t dpl, rpl;
int cpl = x86emul_get_cpl(ctxt, ops);
uint32_t a_flag = 0x100;
-int rc, fault_type = EXC_GP;
+int rc, fault_type = X86_EXC_GP;
if ( cpl < 0 )
return X86EMUL_UNHANDLEABLE;
@@ -982,7 +960,7 @@ protmode_load_seg(
/* Segment present in memory? */
if ( !(desc.b & (1 << 15)) && seg !=
Re: [PATCH] x86/emul: Use existing X86_EXC_* constants
On 27.04.2021 20:56, Andrew Cooper wrote:
> On 27/04/2021 08:09, Jan Beulich wrote:
>> On 26.04.2021 14:45, Andrew Cooper wrote:
>>> ... rather than having separate definitions locally. EXC_HAS_EC in
>>> particular
>>> is missing #CP, #VC and #SX vs X86_EXC_HAVE_EC.
>>>
>>> Also switch a raw 14 for X86_EXC_PF in x86_emul_pagefault().
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper
>>> ---
>>> CC: Jan Beulich
>>> CC: Roger Pau Monné
>>> CC: Wei Liu
>>> ---
>>> xen/arch/x86/x86_emulate/x86_emulate.c | 739
>>> -
>>> xen/arch/x86/x86_emulate/x86_emulate.h | 4 +-
>>> 2 files changed, 361 insertions(+), 382 deletions(-)
>> This is a lot of code churn (almost all some slight growth) for this
>> kind of a change.
>
> Interestingly, no lines needed re-wrapping as a consequence.
>
> [Answering out of order]
>
>> The other option,
>> not reducing churn but reducing rather than increasing code volume (and
>> hence imo helping readability), would be to have shorthands for at
>> least some frequently raised exceptions like #UD and #GP - e.g.
>> generate_ud_if(). Thougths?
>
> #UD and #GP[0] are the overwhelming majority of cases. If you want to
> reduce code volume further, I've always found the "generate" on the
> front rather gratuitous. "raise" is a more common expression to use
> with exceptions.
Indeed I, too, have been wondering whether "raise" wouldn't be better
(but see also below). "generate_exception" predates my participation
in development, iirc.
>> I'm not opposed, but I'd like to explore alternatives
>> first. I know you often dislike token concatenation in macros, which
>> would be my first suggestion to get churn down here.
>
> Outside of a few specific cases, yes, but as you can see in XTF,
> exception handling is one area where IMO clarity can be improved with
> certain limited macro magic.
>
> In the emulator, I could get behind a single #define RAISE() along the
> lines of:
>
> #define RAISE(e, ...)
> do {
> BUILD_BUG_ON(((X86_EXC_HAS_EC & (1u << X86_EXC_##e)) !=
> __count_args(__VA_ARGS__));
> x86_emul_hw_exception(X86_EXC_##e, __count_args(__VA_ARGS__) ?
> __VA_ARGS__ : X86_EVENT_NO_EC, ctxt);
> rc = X86EMUL_EXCEPTION;
> goto done;
> } while ( 0 )
Looks good to me at a first glance, except that I think the left
side of the != in the BUILD_BUG_ON() needs converting to bool
(either explicitly, or by shifting right X86_EXC_HAS_EC instead
of shifting left 1u).
> It's obviously playing behind your back, unlike generate_exception(),
Playing behind our backs? I don't think I see what you mean.
> and does build-time checking that error codes are handled correctly
> (unlike mkec() which has a substantial quantity of code bloat to not
> actually handle it correctly at runtime).
Are you telling me that the compiler doesn't do enough constant
folding there? And I'm also not seeing what doesn't get handled
correctly there.
> I dislike _if() suffixed macros, again for obfuscation reasons.
>
> if ( foo )
> RAISE(UD);
>
> is far more normal C than
>
> RAISE_IF(UD, foo);
> or
> RAISE_IF(foo, UD);
>
> neither if which is a natural reading order. The reduction in line
> count does not equate to improved code clarity.
Not sure here. I wouldn't object the change you suggest, but I've
never had a problem with generate_exception_if(). I guess it's
less clear with RAISE_IF(), but that may be an entirely personal
perception. It was for that reason though that I suggested (now
adapting to the alternative naming) RAISE_UD_IF() as a possible
shorthand. As a good example where I think all the extra if()
would make the code harder to read, please see e.g. the pending
"x86emul: support tile multiplication insns".
> Frankly, areas of
> x86_emulate() would benefit greatly from extra spacing per our normal
> coding style.
I wonder what you refer to by "normal" here: If I guess correctly
what you mean, I don't think ./CODING_STYLE mandates anything.
Not even blank lines between non-fall-through case label blocks.
In recent work I've been trying to separate logical chunks, but I
guess I may not have gone far enough for your taste ...
Really what I'm still hoping to have a good idea for is how to
split up the gigantic switch() statement in there, without having
to introduce dozens of functions with very long parameter lists,
and without sacrificing the centralized SIMD handling immediately
after that switch(). Ideally I'd want to even split up the
monolithic source file, as - especially with older gcc - its
compilation has been dominating build time particularly when
passing make a sufficiently high -j.
Jan
Re: [PATCH] x86/emul: Use existing X86_EXC_* constants
On 27/04/2021 08:09, Jan Beulich wrote:
> On 26.04.2021 14:45, Andrew Cooper wrote:
>> ... rather than having separate definitions locally. EXC_HAS_EC in
>> particular
>> is missing #CP, #VC and #SX vs X86_EXC_HAVE_EC.
>>
>> Also switch a raw 14 for X86_EXC_PF in x86_emul_pagefault().
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper
>> ---
>> CC: Jan Beulich
>> CC: Roger Pau Monné
>> CC: Wei Liu
>> ---
>> xen/arch/x86/x86_emulate/x86_emulate.c | 739
>> -
>> xen/arch/x86/x86_emulate/x86_emulate.h | 4 +-
>> 2 files changed, 361 insertions(+), 382 deletions(-)
> This is a lot of code churn (almost all some slight growth) for this
> kind of a change.
Interestingly, no lines needed re-wrapping as a consequence.
[Answering out of order]
> The other option,
> not reducing churn but reducing rather than increasing code volume (and
> hence imo helping readability), would be to have shorthands for at
> least some frequently raised exceptions like #UD and #GP - e.g.
> generate_ud_if(). Thougths?
#UD and #GP[0] are the overwhelming majority of cases. If you want to
reduce code volume further, I've always found the "generate" on the
front rather gratuitous. "raise" is a more common expression to use
with exceptions.
> I'm not opposed, but I'd like to explore alternatives
> first. I know you often dislike token concatenation in macros, which
> would be my first suggestion to get churn down here.
Outside of a few specific cases, yes, but as you can see in XTF,
exception handling is one area where IMO clarity can be improved with
certain limited macro magic.
In the emulator, I could get behind a single #define RAISE() along the
lines of:
#define RAISE(e, ...)
do {
BUILD_BUG_ON(((X86_EXC_HAS_EC & (1u << X86_EXC_##e)) !=
__count_args(__VA_ARGS__));
x86_emul_hw_exception(X86_EXC_##e, __count_args(__VA_ARGS__) ?
__VA_ARGS__ : X86_EVENT_NO_EC, ctxt);
rc = X86EMUL_EXCEPTION;
goto done;
} while ( 0 )
It's obviously playing behind your back, unlike generate_exception(),
and does build-time checking that error codes are handled correctly
(unlike mkec() which has a substantial quantity of code bloat to not
actually handle it correctly at runtime).
I dislike _if() suffixed macros, again for obfuscation reasons.
if ( foo )
RAISE(UD);
is far more normal C than
RAISE_IF(UD, foo);
or
RAISE_IF(foo, UD);
neither if which is a natural reading order. The reduction in line
count does not equate to improved code clarity. Frankly, areas of
x86_emulate() would benefit greatly from extra spacing per our normal
coding style.
~Andrew
Re: [PATCH] x86/emul: Use existing X86_EXC_* constants
On 26.04.2021 14:45, Andrew Cooper wrote: > ... rather than having separate definitions locally. EXC_HAS_EC in particular > is missing #CP, #VC and #SX vs X86_EXC_HAVE_EC. > > Also switch a raw 14 for X86_EXC_PF in x86_emul_pagefault(). > > No functional change. > > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Roger Pau Monné > CC: Wei Liu > --- > xen/arch/x86/x86_emulate/x86_emulate.c | 739 > - > xen/arch/x86/x86_emulate/x86_emulate.h | 4 +- > 2 files changed, 361 insertions(+), 382 deletions(-) This is a lot of code churn (almost all some slight growth) for this kind of a change. I'm not opposed, but I'd like to explore alternatives first. I know you often dislike token concatenation in macros, which would be my first suggestion to get churn down here. The other option, not reducing churn but reducing rather than increasing code volume (and hence imo helping readability), would be to have shorthands for at least some frequently raised exceptions like #UD and #GP - e.g. generate_ud_if(). Thougths? Jan
[PATCH] x86/emul: Use existing X86_EXC_* constants
... rather than having separate definitions locally. EXC_HAS_EC in particular
is missing #CP, #VC and #SX vs X86_EXC_HAVE_EC.
Also switch a raw 14 for X86_EXC_PF in x86_emul_pagefault().
No functional change.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
---
xen/arch/x86/x86_emulate/x86_emulate.c | 739 -
xen/arch/x86/x86_emulate/x86_emulate.h | 4 +-
2 files changed, 361 insertions(+), 382 deletions(-)
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
b/xen/arch/x86/x86_emulate/x86_emulate.c
index 4a33fe9613..138b85c4bf 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1005,28 +1005,6 @@ struct x86_fxsr {
/* MXCSR bit definitions. */
#define MXCSR_MM (1U << 17)
-/* Exception definitions. */
-#define EXC_DE 0
-#define EXC_DB 1
-#define EXC_BP 3
-#define EXC_OF 4
-#define EXC_BR 5
-#define EXC_UD 6
-#define EXC_NM 7
-#define EXC_DF 8
-#define EXC_TS 10
-#define EXC_NP 11
-#define EXC_SS 12
-#define EXC_GP 13
-#define EXC_PF 14
-#define EXC_MF 16
-#define EXC_AC 17
-#define EXC_XM 19
-
-#define EXC_HAS_EC \
-((1u << EXC_DF) | (1u << EXC_TS) | (1u << EXC_NP) | \
- (1u << EXC_SS) | (1u << EXC_GP) | (1u << EXC_PF) | (1u << EXC_AC))
-
/* Segment selector error code bits. */
#define ECODE_EXT (1 << 0)
#define ECODE_IDT (1 << 1)
@@ -1240,7 +1218,7 @@ do {\
static inline int mkec(uint8_t e, int32_t ec, ...)
{
-return (e < 32 && ((1u << e) & EXC_HAS_EC)) ? ec : X86_EVENT_NO_EC;
+return (e < 32 && ((1u << e) & X86_EXC_HAVE_EC)) ? ec : X86_EVENT_NO_EC;
}
#define generate_exception_if(p, e, ec...)\
@@ -1293,7 +1271,7 @@ static inline int mkec(uint8_t e, int32_t ec, ...)
state->ip += (_size); /* real hardware doesn't truncate */ \
generate_exception_if((uint8_t)(state->ip - \
ctxt->regs->r(ip)) > MAX_INST_LEN, \
- EXC_GP, 0);\
+ X86_EXC_GP, 0);\
rc = ops->insn_fetch(x86_seg_cs, _ip, &_x, (_size), ctxt); \
if ( rc ) goto done; \
_x; \
@@ -1371,11 +1349,11 @@ do {
\
#define validate_far_branch(cs, ip) ({ \
if ( sizeof(ip) <= 4 ) {\
ASSERT(!ctxt->lma); \
-generate_exception_if((ip) > (cs)->limit, EXC_GP, 0); \
+generate_exception_if((ip) > (cs)->limit, X86_EXC_GP, 0); \
} else \
generate_exception_if(ctxt->lma && (cs)->l \
? !is_canonical_address(ip) \
- : (ip) > (cs)->limit, EXC_GP, 0); \
+ : (ip) > (cs)->limit, X86_EXC_GP, 0); \
})
#define commit_far_branch(cs, newip) ({ \
@@ -1442,7 +1420,7 @@ static int _get_fpu(
return rc;
generate_exception_if(!(cr4 & ((type == X86EMUL_FPU_xmm)
? X86_CR4_OSFXSR :
X86_CR4_OSXSAVE)),
- EXC_UD);
+ X86_EXC_UD);
}
rc = ops->read_cr(0, , ctxt);
@@ -1455,13 +1433,13 @@ static int _get_fpu(
}
if ( cr0 & X86_CR0_EM )
{
-generate_exception_if(type == X86EMUL_FPU_fpu, EXC_NM);
-generate_exception_if(type == X86EMUL_FPU_mmx, EXC_UD);
-generate_exception_if(type == X86EMUL_FPU_xmm, EXC_UD);
+generate_exception_if(type == X86EMUL_FPU_fpu, X86_EXC_NM);
+generate_exception_if(type == X86EMUL_FPU_mmx, X86_EXC_UD);
+generate_exception_if(type == X86EMUL_FPU_xmm, X86_EXC_UD);
}
generate_exception_if((cr0 & X86_CR0_TS) &&
(type != X86EMUL_FPU_wait || (cr0 & X86_CR0_MP)),
- EXC_NM);
+ X86_EXC_NM);
}
done:
@@ -1861,7 +1839,7 @@ static int ioport_access_check(
return rc == X86EMUL_DONE ? X86EMUL_OKAY : rc;
/* Ensure the TSS has an io-bitmap-offset field. */
-generate_exception_if(tr.type != 0xb, EXC_GP, 0);
+generate_exception_if(tr.type != 0xb, X86_EXC_GP, 0);
switch ( rc = read_ulong(x86_seg_tr, 0x66, , 2, ctxt, ops) )
{
@@ -1869,7 +1847,7 @@ static int ioport_access_check(
