* [PATCH] x86/vLAPIC: adjust types in internal read/write handling
@ 2015-06-22 11:49 Jan Beulich
2015-06-22 12:15 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2015-06-22 11:49 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 6944 bytes --]
- use 32-bit types where possible (produces slightly better code)
- drop (now) unnecessary casts
- avoid indirection where not needed
- avoid duplicate log messages in vlapic_write()
- minor other cleanup
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -556,52 +556,42 @@ static void vlapic_set_tdcr(struct vlapi
"timer_divisor: %d", vlapic->hw.timer_divisor);
}
-static void vlapic_read_aligned(
- struct vlapic *vlapic, unsigned int offset, unsigned int *result)
+static uint32_t vlapic_read_aligned(struct vlapic *vlapic, unsigned int offset)
{
switch ( offset )
{
case APIC_PROCPRI:
- *result = vlapic_get_ppr(vlapic);
- break;
+ return vlapic_get_ppr(vlapic);
case APIC_TMCCT: /* Timer CCR */
if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
- {
- *result = 0;
break;
- }
- *result = vlapic_get_tmcct(vlapic);
- break;
+ return vlapic_get_tmcct(vlapic);
case APIC_TMICT: /* Timer ICR */
if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
- {
- *result = 0;
break;
- }
+ /* fall through */
default:
- *result = vlapic_get_reg(vlapic, offset);
- break;
+ return vlapic_get_reg(vlapic, offset);
}
+
+ return 0;
}
static int vlapic_read(
struct vcpu *v, unsigned long address,
unsigned long len, unsigned long *pval)
{
- unsigned int alignment;
- unsigned int tmp;
- unsigned long result = 0;
struct vlapic *vlapic = vcpu_vlapic(v);
unsigned int offset = address - vlapic_base_address(vlapic);
+ unsigned int alignment = offset & 3, tmp, result = 0;
if ( offset > (APIC_TDCR + 0x3) )
goto out;
- alignment = offset & 0x3;
+ tmp = vlapic_read_aligned(vlapic, offset & ~3);
- vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
switch ( len )
{
case 1:
@@ -627,7 +617,7 @@ static int vlapic_read(
}
HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "offset %#x with length %#lx, "
- "and the result is %#lx", offset, len, result);
+ "and the result is %#x", offset, len, result);
out:
*pval = result;
@@ -657,19 +647,17 @@ int hvm_x2apic_msr_read(struct vcpu *v,
#undef REGBLOCK
};
struct vlapic *vlapic = vcpu_vlapic(v);
- uint32_t low, high = 0, reg = msr - MSR_IA32_APICBASE_MSR,
- offset = reg << 4;
+ uint32_t high = 0, reg = msr - MSR_IA32_APICBASE_MSR, offset = reg << 4;
if ( !vlapic_x2apic_mode(vlapic) ||
(reg >= sizeof(readable) * 8) || !test_bit(reg, readable) )
return X86EMUL_UNHANDLEABLE;
if ( offset == APIC_ICR )
- vlapic_read_aligned(vlapic, APIC_ICR2, &high);
-
- vlapic_read_aligned(vlapic, offset, &low);
+ high = vlapic_read_aligned(vlapic, APIC_ICR2);
- *msr_content = (((uint64_t)high) << 32) | low;
+ *msr_content = ((uint64_t)high << 32) |
+ vlapic_read_aligned(vlapic, offset);
return X86EMUL_OKAY;
}
@@ -687,7 +675,7 @@ static void vlapic_tdt_pt_cb(struct vcpu
}
static int vlapic_reg_write(struct vcpu *v,
- unsigned int offset, unsigned long val)
+ unsigned int offset, uint32_t val)
{
struct vlapic *vlapic = vcpu_vlapic(v);
int rc = X86EMUL_OKAY;
@@ -797,8 +785,7 @@ static int vlapic_reg_write(struct vcpu
break;
}
- period = ((uint64_t)APIC_BUS_CYCLE_NS *
- (uint32_t)val * vlapic->hw.timer_divisor);
+ period = (uint64_t)APIC_BUS_CYCLE_NS * val * vlapic->hw.timer_divisor;
TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(period),
TRC_PAR_LONG(vlapic_lvtt_period(vlapic) ? period : 0LL),
vlapic->pt.irq);
@@ -811,7 +798,7 @@ static int vlapic_reg_write(struct vcpu
HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
"bus cycle is %uns, "
- "initial count %lu, period %"PRIu64"ns",
+ "initial count %u, period %"PRIu64"ns",
APIC_BUS_CYCLE_NS, val, period);
}
break;
@@ -847,47 +834,41 @@ static int vlapic_write(struct vcpu *v,
* According to the IA32 Manual, all accesses should be 32 bits.
* Some OSes do 8- or 16-byte accesses, however.
*/
- val = (uint32_t)val;
- if ( len != 4 )
+ if ( unlikely(len != 4) )
{
- unsigned int tmp;
- unsigned char alignment;
-
- gdprintk(XENLOG_INFO, "Notice: Local APIC write with len = %lx\n",len);
-
- alignment = offset & 0x3;
- (void)vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
+ unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
+ unsigned char alignment = (offset & 3) * 8;
switch ( len )
{
case 1:
- val = ((tmp & ~(0xff << (8*alignment))) |
- ((val & 0xff) << (8*alignment)));
+ val = ((tmp & ~(0xff << alignment)) |
+ ((val & 0xff) << alignment));
break;
case 2:
if ( alignment & 1 )
goto unaligned_exit_and_crash;
- val = ((tmp & ~(0xffff << (8*alignment))) |
- ((val & 0xffff) << (8*alignment)));
+ val = ((tmp & ~(0xffff << alignment)) |
+ ((val & 0xffff) << alignment));
break;
default:
- gdprintk(XENLOG_ERR, "Local APIC write with len = %lx, "
- "should be 4 instead\n", len);
+ gprintk(XENLOG_ERR, "LAPIC write with len %lx\n", len);
goto exit_and_crash;
}
+
+ gdprintk(XENLOG_INFO, "Notice: LAPIC write with len %lx\n", len);
+ offset &= ~3;
}
- else if ( (offset & 0x3) != 0 )
+ else if ( unlikely(offset & 3) )
goto unaligned_exit_and_crash;
- offset &= ~0x3;
-
return vlapic_reg_write(v, offset, val);
unaligned_exit_and_crash:
- gdprintk(XENLOG_ERR, "Unaligned LAPIC write len=%#lx at offset=%#x.\n",
- len, offset);
+ gprintk(XENLOG_ERR, "Unaligned LAPIC write: len=%#lx offset=%#x.\n",
+ len, offset);
exit_and_crash:
domain_crash(v->domain);
return rc;
@@ -983,7 +964,7 @@ int hvm_x2apic_msr_write(struct vcpu *v,
return X86EMUL_UNHANDLEABLE;
}
- return vlapic_reg_write(v, offset, (uint32_t)msr_content);
+ return vlapic_reg_write(v, offset, msr_content);
}
static int vlapic_range(struct vcpu *v, unsigned long addr)
[-- Attachment #2: x86-vLAPIC-read-write-types.patch --]
[-- Type: text/plain, Size: 7000 bytes --]
x86/vLAPIC: adjust types in internal read/write handling
- use 32-bit types where possible (produces slightly better code)
- drop (now) unnecessary casts
- avoid indirection where not needed
- avoid duplicate log messages in vlapic_write()
- minor other cleanup
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -556,52 +556,42 @@ static void vlapic_set_tdcr(struct vlapi
"timer_divisor: %d", vlapic->hw.timer_divisor);
}
-static void vlapic_read_aligned(
- struct vlapic *vlapic, unsigned int offset, unsigned int *result)
+static uint32_t vlapic_read_aligned(struct vlapic *vlapic, unsigned int offset)
{
switch ( offset )
{
case APIC_PROCPRI:
- *result = vlapic_get_ppr(vlapic);
- break;
+ return vlapic_get_ppr(vlapic);
case APIC_TMCCT: /* Timer CCR */
if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
- {
- *result = 0;
break;
- }
- *result = vlapic_get_tmcct(vlapic);
- break;
+ return vlapic_get_tmcct(vlapic);
case APIC_TMICT: /* Timer ICR */
if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
- {
- *result = 0;
break;
- }
+ /* fall through */
default:
- *result = vlapic_get_reg(vlapic, offset);
- break;
+ return vlapic_get_reg(vlapic, offset);
}
+
+ return 0;
}
static int vlapic_read(
struct vcpu *v, unsigned long address,
unsigned long len, unsigned long *pval)
{
- unsigned int alignment;
- unsigned int tmp;
- unsigned long result = 0;
struct vlapic *vlapic = vcpu_vlapic(v);
unsigned int offset = address - vlapic_base_address(vlapic);
+ unsigned int alignment = offset & 3, tmp, result = 0;
if ( offset > (APIC_TDCR + 0x3) )
goto out;
- alignment = offset & 0x3;
+ tmp = vlapic_read_aligned(vlapic, offset & ~3);
- vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
switch ( len )
{
case 1:
@@ -627,7 +617,7 @@ static int vlapic_read(
}
HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "offset %#x with length %#lx, "
- "and the result is %#lx", offset, len, result);
+ "and the result is %#x", offset, len, result);
out:
*pval = result;
@@ -657,19 +647,17 @@ int hvm_x2apic_msr_read(struct vcpu *v,
#undef REGBLOCK
};
struct vlapic *vlapic = vcpu_vlapic(v);
- uint32_t low, high = 0, reg = msr - MSR_IA32_APICBASE_MSR,
- offset = reg << 4;
+ uint32_t high = 0, reg = msr - MSR_IA32_APICBASE_MSR, offset = reg << 4;
if ( !vlapic_x2apic_mode(vlapic) ||
(reg >= sizeof(readable) * 8) || !test_bit(reg, readable) )
return X86EMUL_UNHANDLEABLE;
if ( offset == APIC_ICR )
- vlapic_read_aligned(vlapic, APIC_ICR2, &high);
-
- vlapic_read_aligned(vlapic, offset, &low);
+ high = vlapic_read_aligned(vlapic, APIC_ICR2);
- *msr_content = (((uint64_t)high) << 32) | low;
+ *msr_content = ((uint64_t)high << 32) |
+ vlapic_read_aligned(vlapic, offset);
return X86EMUL_OKAY;
}
@@ -687,7 +675,7 @@ static void vlapic_tdt_pt_cb(struct vcpu
}
static int vlapic_reg_write(struct vcpu *v,
- unsigned int offset, unsigned long val)
+ unsigned int offset, uint32_t val)
{
struct vlapic *vlapic = vcpu_vlapic(v);
int rc = X86EMUL_OKAY;
@@ -797,8 +785,7 @@ static int vlapic_reg_write(struct vcpu
break;
}
- period = ((uint64_t)APIC_BUS_CYCLE_NS *
- (uint32_t)val * vlapic->hw.timer_divisor);
+ period = (uint64_t)APIC_BUS_CYCLE_NS * val * vlapic->hw.timer_divisor;
TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(period),
TRC_PAR_LONG(vlapic_lvtt_period(vlapic) ? period : 0LL),
vlapic->pt.irq);
@@ -811,7 +798,7 @@ static int vlapic_reg_write(struct vcpu
HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
"bus cycle is %uns, "
- "initial count %lu, period %"PRIu64"ns",
+ "initial count %u, period %"PRIu64"ns",
APIC_BUS_CYCLE_NS, val, period);
}
break;
@@ -847,47 +834,41 @@ static int vlapic_write(struct vcpu *v,
* According to the IA32 Manual, all accesses should be 32 bits.
* Some OSes do 8- or 16-byte accesses, however.
*/
- val = (uint32_t)val;
- if ( len != 4 )
+ if ( unlikely(len != 4) )
{
- unsigned int tmp;
- unsigned char alignment;
-
- gdprintk(XENLOG_INFO, "Notice: Local APIC write with len = %lx\n",len);
-
- alignment = offset & 0x3;
- (void)vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
+ unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
+ unsigned char alignment = (offset & 3) * 8;
switch ( len )
{
case 1:
- val = ((tmp & ~(0xff << (8*alignment))) |
- ((val & 0xff) << (8*alignment)));
+ val = ((tmp & ~(0xff << alignment)) |
+ ((val & 0xff) << alignment));
break;
case 2:
if ( alignment & 1 )
goto unaligned_exit_and_crash;
- val = ((tmp & ~(0xffff << (8*alignment))) |
- ((val & 0xffff) << (8*alignment)));
+ val = ((tmp & ~(0xffff << alignment)) |
+ ((val & 0xffff) << alignment));
break;
default:
- gdprintk(XENLOG_ERR, "Local APIC write with len = %lx, "
- "should be 4 instead\n", len);
+ gprintk(XENLOG_ERR, "LAPIC write with len %lx\n", len);
goto exit_and_crash;
}
+
+ gdprintk(XENLOG_INFO, "Notice: LAPIC write with len %lx\n", len);
+ offset &= ~3;
}
- else if ( (offset & 0x3) != 0 )
+ else if ( unlikely(offset & 3) )
goto unaligned_exit_and_crash;
- offset &= ~0x3;
-
return vlapic_reg_write(v, offset, val);
unaligned_exit_and_crash:
- gdprintk(XENLOG_ERR, "Unaligned LAPIC write len=%#lx at offset=%#x.\n",
- len, offset);
+ gprintk(XENLOG_ERR, "Unaligned LAPIC write: len=%#lx offset=%#x.\n",
+ len, offset);
exit_and_crash:
domain_crash(v->domain);
return rc;
@@ -983,7 +964,7 @@ int hvm_x2apic_msr_write(struct vcpu *v,
return X86EMUL_UNHANDLEABLE;
}
- return vlapic_reg_write(v, offset, (uint32_t)msr_content);
+ return vlapic_reg_write(v, offset, msr_content);
}
static int vlapic_range(struct vcpu *v, unsigned long addr)
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/vLAPIC: adjust types in internal read/write handling
2015-06-22 11:49 [PATCH] x86/vLAPIC: adjust types in internal read/write handling Jan Beulich
@ 2015-06-22 12:15 ` Andrew Cooper
2015-06-22 12:55 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2015-06-22 12:15 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 22/06/15 12:49, Jan Beulich wrote:
> - use 32-bit types where possible (produces slightly better code)
> - drop (now) unnecessary casts
> - avoid indirection where not needed
> - avoid duplicate log messages in vlapic_write()
> - minor other cleanup
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with two
suggestions.
>
> @@ -847,47 +834,41 @@ static int vlapic_write(struct vcpu *v,
> * According to the IA32 Manual, all accesses should be 32 bits.
> * Some OSes do 8- or 16-byte accesses, however.
> */
> - val = (uint32_t)val;
> - if ( len != 4 )
> + if ( unlikely(len != 4) )
> {
> - unsigned int tmp;
> - unsigned char alignment;
> -
> - gdprintk(XENLOG_INFO, "Notice: Local APIC write with len = %lx\n",len);
> -
> - alignment = offset & 0x3;
> - (void)vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
> + unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
> + unsigned char alignment = (offset & 3) * 8;
>
> switch ( len )
> {
> case 1:
> - val = ((tmp & ~(0xff << (8*alignment))) |
> - ((val & 0xff) << (8*alignment)));
> + val = ((tmp & ~(0xff << alignment)) |
> + ((val & 0xff) << alignment));
These should probably be explicitly unsigned constants, to avoid issues
with shifting a 1 into the sign bit. (I can't quite decide whether 0xff
will be interpreted as signed or unsigned, given the integer promotion
rules.)
> break;
>
> case 2:
> if ( alignment & 1 )
> goto unaligned_exit_and_crash;
> - val = ((tmp & ~(0xffff << (8*alignment))) |
> - ((val & 0xffff) << (8*alignment)));
> + val = ((tmp & ~(0xffff << alignment)) |
> + ((val & 0xffff) << alignment));
> break;
>
> default:
> - gdprintk(XENLOG_ERR, "Local APIC write with len = %lx, "
> - "should be 4 instead\n", len);
> + gprintk(XENLOG_ERR, "LAPIC write with len %lx\n", len);
> goto exit_and_crash;
> }
> +
> + gdprintk(XENLOG_INFO, "Notice: LAPIC write with len %lx\n", len);
Probably better using %lu.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/vLAPIC: adjust types in internal read/write handling
2015-06-22 12:15 ` Andrew Cooper
@ 2015-06-22 12:55 ` Jan Beulich
2015-06-22 13:02 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2015-06-22 12:55 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 22.06.15 at 14:15, <andrew.cooper3@citrix.com> wrote:
> On 22/06/15 12:49, Jan Beulich wrote:
>> @@ -847,47 +834,41 @@ static int vlapic_write(struct vcpu *v,
>> * According to the IA32 Manual, all accesses should be 32 bits.
>> * Some OSes do 8- or 16-byte accesses, however.
>> */
>> - val = (uint32_t)val;
>> - if ( len != 4 )
>> + if ( unlikely(len != 4) )
>> {
>> - unsigned int tmp;
>> - unsigned char alignment;
>> -
>> - gdprintk(XENLOG_INFO, "Notice: Local APIC write with len = %lx\n",len);
>> -
>> - alignment = offset & 0x3;
>> - (void)vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
>> + unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
>> + unsigned char alignment = (offset & 3) * 8;
>>
>> switch ( len )
>> {
>> case 1:
>> - val = ((tmp & ~(0xff << (8*alignment))) |
>> - ((val & 0xff) << (8*alignment)));
>> + val = ((tmp & ~(0xff << alignment)) |
>> + ((val & 0xff) << alignment));
>
> These should probably be explicitly unsigned constants, to avoid issues
> with shifting a 1 into the sign bit.
I don't see what harm the sign bit would do here - even if the shift
operation is one on signed int, the & converts the operand to
unsigned int anyway (and with them being the same size, the
binary representation doesn't change).
> (I can't quite decide whether 0xff
> will be interpreted as signed or unsigned, given the integer promotion
> rules.)
Literal numbers representable as int will always be "promoted to"
int.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/vLAPIC: adjust types in internal read/write handling
2015-06-22 12:55 ` Jan Beulich
@ 2015-06-22 13:02 ` Andrew Cooper
2015-06-22 13:27 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2015-06-22 13:02 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
On 22/06/15 13:55, Jan Beulich wrote:
>>>> On 22.06.15 at 14:15, <andrew.cooper3@citrix.com> wrote:
>> On 22/06/15 12:49, Jan Beulich wrote:
>>> @@ -847,47 +834,41 @@ static int vlapic_write(struct vcpu *v,
>>> * According to the IA32 Manual, all accesses should be 32 bits.
>>> * Some OSes do 8- or 16-byte accesses, however.
>>> */
>>> - val = (uint32_t)val;
>>> - if ( len != 4 )
>>> + if ( unlikely(len != 4) )
>>> {
>>> - unsigned int tmp;
>>> - unsigned char alignment;
>>> -
>>> - gdprintk(XENLOG_INFO, "Notice: Local APIC write with len = %lx\n",len);
>>> -
>>> - alignment = offset & 0x3;
>>> - (void)vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
>>> + unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
>>> + unsigned char alignment = (offset & 3) * 8;
>>>
>>> switch ( len )
>>> {
>>> case 1:
>>> - val = ((tmp & ~(0xff << (8*alignment))) |
>>> - ((val & 0xff) << (8*alignment)));
>>> + val = ((tmp & ~(0xff << alignment)) |
>>> + ((val & 0xff) << alignment));
>> These should probably be explicitly unsigned constants, to avoid issues
>> with shifting a 1 into the sign bit.
> I don't see what harm the sign bit would do here - even if the shift
> operation is one on signed int, the & converts the operand to
> unsigned int anyway (and with them being the same size, the
> binary representation doesn't change).
The problem is with 0xff << 24, which where the sign bit will change
given the shift.
If 0xff is interpreted as signed, then shifted, then promoted to
unsigned by the ~ operation, then the result is undefined behaviour
(altering the sign bit of a number with a shift).
If 0xff is interpreted as unsigned straight away, then everything is
fine, as 0xffu << 24 is completely defined behaviour.
>
>> (I can't quite decide whether 0xff
>> will be interpreted as signed or unsigned, given the integer promotion
>> rules.)
> Literal numbers representable as int will always be "promoted to"
> int.
Which suggested that the code above does demonstrate UB.
~Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/vLAPIC: adjust types in internal read/write handling
2015-06-22 13:02 ` Andrew Cooper
@ 2015-06-22 13:27 ` Jan Beulich
2015-06-22 14:06 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2015-06-22 13:27 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 22.06.15 at 15:02, <andrew.cooper3@citrix.com> wrote:
> On 22/06/15 13:55, Jan Beulich wrote:
>>>>> On 22.06.15 at 14:15, <andrew.cooper3@citrix.com> wrote:
>>> On 22/06/15 12:49, Jan Beulich wrote:
>>>> @@ -847,47 +834,41 @@ static int vlapic_write(struct vcpu *v,
>>>> * According to the IA32 Manual, all accesses should be 32 bits.
>>>> * Some OSes do 8- or 16-byte accesses, however.
>>>> */
>>>> - val = (uint32_t)val;
>>>> - if ( len != 4 )
>>>> + if ( unlikely(len != 4) )
>>>> {
>>>> - unsigned int tmp;
>>>> - unsigned char alignment;
>>>> -
>>>> - gdprintk(XENLOG_INFO, "Notice: Local APIC write with len =
> %lx\n",len);
>>>> -
>>>> - alignment = offset & 0x3;
>>>> - (void)vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
>>>> + unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
>>>> + unsigned char alignment = (offset & 3) * 8;
>>>>
>>>> switch ( len )
>>>> {
>>>> case 1:
>>>> - val = ((tmp & ~(0xff << (8*alignment))) |
>>>> - ((val & 0xff) << (8*alignment)));
>>>> + val = ((tmp & ~(0xff << alignment)) |
>>>> + ((val & 0xff) << alignment));
>>> These should probably be explicitly unsigned constants, to avoid issues
>>> with shifting a 1 into the sign bit.
>> I don't see what harm the sign bit would do here - even if the shift
>> operation is one on signed int, the & converts the operand to
>> unsigned int anyway (and with them being the same size, the
>> binary representation doesn't change).
>
> The problem is with 0xff << 24, which where the sign bit will change
> given the shift.
>
> If 0xff is interpreted as signed, then shifted, then promoted to
> unsigned by the ~ operation, then the result is undefined behaviour
> (altering the sign bit of a number with a shift).
Okay, while I can buy that, I suppose we've got many more of these
throughout the tree (and the compiler is treating them quite fine).
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/vLAPIC: adjust types in internal read/write handling
2015-06-22 13:27 ` Jan Beulich
@ 2015-06-22 14:06 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2015-06-22 14:06 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
On 22/06/15 14:27, Jan Beulich wrote:
>>>> On 22.06.15 at 15:02, <andrew.cooper3@citrix.com> wrote:
>> On 22/06/15 13:55, Jan Beulich wrote:
>>>>>> On 22.06.15 at 14:15, <andrew.cooper3@citrix.com> wrote:
>>>> On 22/06/15 12:49, Jan Beulich wrote:
>>>>> @@ -847,47 +834,41 @@ static int vlapic_write(struct vcpu *v,
>>>>> * According to the IA32 Manual, all accesses should be 32 bits.
>>>>> * Some OSes do 8- or 16-byte accesses, however.
>>>>> */
>>>>> - val = (uint32_t)val;
>>>>> - if ( len != 4 )
>>>>> + if ( unlikely(len != 4) )
>>>>> {
>>>>> - unsigned int tmp;
>>>>> - unsigned char alignment;
>>>>> -
>>>>> - gdprintk(XENLOG_INFO, "Notice: Local APIC write with len =
>> %lx\n",len);
>>>>> -
>>>>> - alignment = offset & 0x3;
>>>>> - (void)vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
>>>>> + unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
>>>>> + unsigned char alignment = (offset & 3) * 8;
>>>>>
>>>>> switch ( len )
>>>>> {
>>>>> case 1:
>>>>> - val = ((tmp & ~(0xff << (8*alignment))) |
>>>>> - ((val & 0xff) << (8*alignment)));
>>>>> + val = ((tmp & ~(0xff << alignment)) |
>>>>> + ((val & 0xff) << alignment));
>>>> These should probably be explicitly unsigned constants, to avoid issues
>>>> with shifting a 1 into the sign bit.
>>> I don't see what harm the sign bit would do here - even if the shift
>>> operation is one on signed int, the & converts the operand to
>>> unsigned int anyway (and with them being the same size, the
>>> binary representation doesn't change).
>> The problem is with 0xff << 24, which where the sign bit will change
>> given the shift.
>>
>> If 0xff is interpreted as signed, then shifted, then promoted to
>> unsigned by the ~ operation, then the result is undefined behaviour
>> (altering the sign bit of a number with a shift).
> Okay, while I can buy that, I suppose we've got many more of these
> throughout the tree (and the compiler is treating them quite fine).
We likely have. I try my best to fix them as I find them.
As for the compiler, it is probably easiest to ignore the potential
problem which ends up generating correct code (and is a legitimate
action to take with UB). It is more likely to be a problem with extreme
levels of optimisations enabled, as the compiler tries harder and harder
to throw operations away.
~Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-06-22 14:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-22 11:49 [PATCH] x86/vLAPIC: adjust types in internal read/write handling Jan Beulich
2015-06-22 12:15 ` Andrew Cooper
2015-06-22 12:55 ` Jan Beulich
2015-06-22 13:02 ` Andrew Cooper
2015-06-22 13:27 ` Jan Beulich
2015-06-22 14:06 ` Andrew Cooper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).