* [Qemu-devel] [PULL 01/20] mc146818rtc: update periodic timer only if it is needed
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
@ 2017-05-19 11:20 ` Paolo Bonzini
2017-05-19 11:20 ` [Qemu-devel] [PULL 02/20] mc146818rtc: precisely count the clock for periodic timer Paolo Bonzini
` (20 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
Currently, the timer is updated whenever RegA or RegB is written
even if the periodic timer related configuration is not changed
This patch optimizes it slightly to make the update happen only
if its period or enable-status is changed, also later patches are
depend on this optimization
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
Message-Id: <20170510083259.3900-2-xiaoguangrong@tencent.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/timer/mc146818rtc.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 93de3e1..7d78391 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -391,6 +391,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
uint64_t data, unsigned size)
{
RTCState *s = opaque;
+ bool update_periodic_timer;
if ((addr & 1) == 0) {
s->cmos_index = data & 0x7f;
@@ -423,6 +424,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
}
break;
case RTC_REG_A:
+ update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & 0x0f;
+
if ((data & 0x60) == 0x60) {
if (rtc_running(s)) {
rtc_update_time(s);
@@ -445,10 +448,17 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
/* UIP bit is read only */
s->cmos_data[RTC_REG_A] = (data & ~REG_A_UIP) |
(s->cmos_data[RTC_REG_A] & REG_A_UIP);
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+
+ if (update_periodic_timer) {
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+ }
+
check_update_timer(s);
break;
case RTC_REG_B:
+ update_periodic_timer = (s->cmos_data[RTC_REG_B] ^ data)
+ & REG_B_PIE;
+
if (data & REG_B_SET) {
/* update cmos to when the rtc was stopping */
if (rtc_running(s)) {
@@ -475,7 +485,11 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
qemu_irq_lower(s->irq);
}
s->cmos_data[RTC_REG_B] = data;
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+
+ if (update_periodic_timer) {
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+ }
+
check_update_timer(s);
break;
case RTC_REG_C:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 02/20] mc146818rtc: precisely count the clock for periodic timer
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
2017-05-19 11:20 ` [Qemu-devel] [PULL 01/20] mc146818rtc: update periodic timer only if it is needed Paolo Bonzini
@ 2017-05-19 11:20 ` Paolo Bonzini
2017-05-19 11:20 ` [Qemu-devel] [PULL 03/20] mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386 Paolo Bonzini
` (19 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Tai Yunfang, Xiao Guangrong
From: Tai Yunfang <yunfangtai@tencent.com>
There are two issues in current code:
1) If the period is changed by re-configuring RegA, the coalesced
irq will be scaled to reflect the new period, however, it
calculates the new interrupt number like this:
s->irq_coalesced = (s->irq_coalesced * s->period) / period;
There are some clocks will be lost if they are not enough to
be squeezed to a single new period that will cause the VM clock
slower
In order to fix the issue, we calculate the interrupt window
based on the precise clock rather than period, then the clocks
lost during period is scaled can be compensated properly
2) If periodic_timer_update() is called due to RegA reconfiguration,
i.e, the period is updated, current time is not the start point
for the next periodic timer, instead, which should start from the
last interrupt, otherwise, the clock in VM will become slow
This patch takes the clocks from last interrupt to current clock
into account and compensates the clocks for the next interrupt,
especially if a complete interrupt was lost in this window, the
time can be caught up by LOST_TICK_POLICY_SLEW
Signed-off-by: Tai Yunfang <yunfangtai@tencent.com>
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
Message-Id: <20170510083259.3900-3-xiaoguangrong@tencent.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/timer/mc146818rtc.c | 120 +++++++++++++++++++++++++++++++++++++++----------
1 file changed, 97 insertions(+), 23 deletions(-)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 7d78391..aeb60cc 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -146,31 +146,100 @@ static void rtc_coalesced_timer(void *opaque)
}
#endif
-/* handle periodic timer */
-static void periodic_timer_update(RTCState *s, int64_t current_time)
+static uint32_t rtc_periodic_clock_ticks(RTCState *s)
{
- int period_code, period;
- int64_t cur_clock, next_irq_clock;
+ int period_code;
+
+ if (!(s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
+ return 0;
+ }
period_code = s->cmos_data[RTC_REG_A] & 0x0f;
- if (period_code != 0
- && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
- if (period_code <= 2)
- period_code += 7;
- /* period in 32 Khz cycles */
- period = 1 << (period_code - 1);
-#ifdef TARGET_I386
- if (period != s->period) {
- s->irq_coalesced = (s->irq_coalesced * s->period) / period;
- DPRINTF_C("cmos: coalesced irqs scaled to %d\n", s->irq_coalesced);
- }
- s->period = period;
-#endif
+ if (!period_code) {
+ return 0;
+ }
+
+ if (period_code <= 2) {
+ period_code += 7;
+ }
+
+ /* period in 32 Khz cycles */
+ return 1 << (period_code - 1);
+}
+
+/*
+ * handle periodic timer. @old_period indicates the periodic timer update
+ * is just due to period adjustment.
+ */
+static void
+periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
+{
+ uint32_t period;
+ int64_t cur_clock, next_irq_clock, lost_clock = 0;
+
+ period = rtc_periodic_clock_ticks(s);
+
+ if (period) {
/* compute 32 khz clock */
cur_clock =
muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
- next_irq_clock = (cur_clock & ~(period - 1)) + period;
+ /*
+ * if the periodic timer's update is due to period re-configuration,
+ * we should count the clock since last interrupt.
+ */
+ if (old_period) {
+ int64_t last_periodic_clock, next_periodic_clock;
+
+ next_periodic_clock = muldiv64(s->next_periodic_time,
+ RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
+ last_periodic_clock = next_periodic_clock - old_period;
+ lost_clock = cur_clock - last_periodic_clock;
+ assert(lost_clock >= 0);
+ }
+
+#ifdef TARGET_I386
+ /*
+ * s->irq_coalesced can change for two reasons:
+ *
+ * a) if one or more periodic timer interrupts have been lost,
+ * lost_clock will be more that a period.
+ *
+ * b) when the period may be reconfigured, we expect the OS to
+ * treat delayed tick as the new period. So, when switching
+ * from a shorter to a longer period, scale down the missing,
+ * because the OS will treat past delayed ticks as longer
+ * (leftovers are put back into lost_clock). When switching
+ * to a shorter period, scale up the missing ticks since the
+ * OS handler will treat past delayed ticks as shorter.
+ */
+ if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+ uint32_t old_irq_coalesced = s->irq_coalesced;
+
+ s->period = period;
+ lost_clock += old_irq_coalesced * old_period;
+ s->irq_coalesced = lost_clock / s->period;
+ lost_clock %= s->period;
+ if (old_irq_coalesced != s->irq_coalesced ||
+ old_period != s->period) {
+ DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, "
+ "period scaled from %d to %d\n", old_irq_coalesced,
+ s->irq_coalesced, old_period, s->period);
+ rtc_coalesced_timer_update(s);
+ }
+ } else
+#endif
+ {
+ /*
+ * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
+ * is not used, we should make the time progress anyway.
+ */
+ lost_clock = MIN(lost_clock, period);
+ }
+
+ assert(lost_clock >= 0 && lost_clock <= period);
+
+ next_irq_clock = cur_clock + period - lost_clock;
s->next_periodic_time = muldiv64(next_irq_clock, NANOSECONDS_PER_SECOND,
RTC_CLOCK_RATE) + 1;
timer_mod(s->periodic_timer, s->next_periodic_time);
@@ -186,7 +255,7 @@ static void rtc_periodic_timer(void *opaque)
{
RTCState *s = opaque;
- periodic_timer_update(s, s->next_periodic_time);
+ periodic_timer_update(s, s->next_periodic_time, 0);
s->cmos_data[RTC_REG_C] |= REG_C_PF;
if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
@@ -391,6 +460,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
uint64_t data, unsigned size)
{
RTCState *s = opaque;
+ uint32_t old_period;
bool update_periodic_timer;
if ((addr & 1) == 0) {
@@ -425,6 +495,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
break;
case RTC_REG_A:
update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & 0x0f;
+ old_period = rtc_periodic_clock_ticks(s);
if ((data & 0x60) == 0x60) {
if (rtc_running(s)) {
@@ -450,7 +521,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
(s->cmos_data[RTC_REG_A] & REG_A_UIP);
if (update_periodic_timer) {
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
+ old_period);
}
check_update_timer(s);
@@ -458,6 +530,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
case RTC_REG_B:
update_periodic_timer = (s->cmos_data[RTC_REG_B] ^ data)
& REG_B_PIE;
+ old_period = rtc_periodic_clock_ticks(s);
if (data & REG_B_SET) {
/* update cmos to when the rtc was stopping */
@@ -487,7 +560,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
s->cmos_data[RTC_REG_B] = data;
if (update_periodic_timer) {
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
+ old_period);
}
check_update_timer(s);
@@ -757,7 +831,7 @@ static int rtc_post_load(void *opaque, int version_id)
uint64_t now = qemu_clock_get_ns(rtc_clock);
if (now < s->next_periodic_time ||
now > (s->next_periodic_time + get_max_clock_jump())) {
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), 0);
}
}
@@ -822,7 +896,7 @@ static void rtc_notify_clock_reset(Notifier *notifier, void *data)
int64_t now = *(int64_t *)data;
rtc_set_date_from_host(ISA_DEVICE(s));
- periodic_timer_update(s, now);
+ periodic_timer_update(s, now, 0);
check_update_timer(s);
#ifdef TARGET_I386
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 03/20] mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
2017-05-19 11:20 ` [Qemu-devel] [PULL 01/20] mc146818rtc: update periodic timer only if it is needed Paolo Bonzini
2017-05-19 11:20 ` [Qemu-devel] [PULL 02/20] mc146818rtc: precisely count the clock for periodic timer Paolo Bonzini
@ 2017-05-19 11:20 ` Paolo Bonzini
2017-05-19 11:20 ` [Qemu-devel] [PULL 04/20] mc146818rtc: drop unnecessary '#ifdef TARGET_I386' Paolo Bonzini
` (18 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
Any tick policy specified on other platforms rather on TARGET_I386
will fall back to LOST_TICK_POLICY_DISCARD silently, this patch makes
sure only TARGET_I386 can enable LOST_TICK_POLICY_SLEW
After that, we can enable LOST_TICK_POLICY_SLEW in the common code
which need not use '#ifdef TARGET_I386' to make these code be x86
specific anymore
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
Message-Id: <20170510083259.3900-4-xiaoguangrong@tencent.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/timer/mc146818rtc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index aeb60cc..4870a72 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -974,19 +974,19 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
rtc_set_date_from_host(isadev);
-#ifdef TARGET_I386
switch (s->lost_tick_policy) {
+#ifdef TARGET_I386
case LOST_TICK_POLICY_SLEW:
s->coalesced_timer =
timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
break;
+#endif
case LOST_TICK_POLICY_DISCARD:
break;
default:
error_setg(errp, "Invalid lost tick policy.");
return;
}
-#endif
s->periodic_timer = timer_new_ns(rtc_clock, rtc_periodic_timer, s);
s->update_timer = timer_new_ns(rtc_clock, rtc_update_timer, s);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 04/20] mc146818rtc: drop unnecessary '#ifdef TARGET_I386'
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (2 preceding siblings ...)
2017-05-19 11:20 ` [Qemu-devel] [PULL 03/20] mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386 Paolo Bonzini
@ 2017-05-19 11:20 ` Paolo Bonzini
2017-05-19 11:20 ` [Qemu-devel] [PULL 05/20] mc146818rtc: embrace all x86 specific code Paolo Bonzini
` (17 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
If the code purely depends on LOST_TICK_POLICY_SLEW, we can simply
drop '#ifdef TARGET_I386' as only x86 can enable this tick policy
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
Message-Id: <20170510083259.3900-5-xiaoguangrong@tencent.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/timer/mc146818rtc.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 4870a72..f9d6181 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -112,7 +112,6 @@ static uint64_t get_guest_rtc_ns(RTCState *s)
guest_clock - s->last_update + s->offset;
}
-#ifdef TARGET_I386
static void rtc_coalesced_timer_update(RTCState *s)
{
if (s->irq_coalesced == 0) {
@@ -126,6 +125,7 @@ static void rtc_coalesced_timer_update(RTCState *s)
}
}
+#ifdef TARGET_I386
static void rtc_coalesced_timer(void *opaque)
{
RTCState *s = opaque;
@@ -198,7 +198,6 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
assert(lost_clock >= 0);
}
-#ifdef TARGET_I386
/*
* s->irq_coalesced can change for two reasons:
*
@@ -227,9 +226,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
s->irq_coalesced, old_period, s->period);
rtc_coalesced_timer_update(s);
}
- } else
-#endif
- {
+ } else {
/*
* no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
* is not used, we should make the time progress anyway.
@@ -244,9 +241,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
RTC_CLOCK_RATE) + 1;
timer_mod(s->periodic_timer, s->next_periodic_time);
} else {
-#ifdef TARGET_I386
s->irq_coalesced = 0;
-#endif
timer_del(s->periodic_timer);
}
}
@@ -835,13 +830,11 @@ static int rtc_post_load(void *opaque, int version_id)
}
}
-#ifdef TARGET_I386
if (version_id >= 2) {
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
rtc_coalesced_timer_update(s);
}
}
-#endif
return 0;
}
@@ -898,11 +891,10 @@ static void rtc_notify_clock_reset(Notifier *notifier, void *data)
rtc_set_date_from_host(ISA_DEVICE(s));
periodic_timer_update(s, now, 0);
check_update_timer(s);
-#ifdef TARGET_I386
+
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
rtc_coalesced_timer_update(s);
}
-#endif
}
/* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
@@ -923,12 +915,10 @@ static void rtc_reset(void *opaque)
qemu_irq_lower(s->irq);
-#ifdef TARGET_I386
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
s->irq_coalesced = 0;
s->irq_reinject_on_ack_count = 0;
}
-#endif
}
static const MemoryRegionOps cmos_ops = {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 05/20] mc146818rtc: embrace all x86 specific code
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (3 preceding siblings ...)
2017-05-19 11:20 ` [Qemu-devel] [PULL 04/20] mc146818rtc: drop unnecessary '#ifdef TARGET_I386' Paolo Bonzini
@ 2017-05-19 11:20 ` Paolo Bonzini
2017-05-19 11:20 ` [Qemu-devel] [PULL 06/20] kvm: irqchip: trace changes on msi add/remove Paolo Bonzini
` (16 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
Introduce a function, rtc_policy_slew_deliver_irq(), which delivers
irq if LOST_TICK_POLICY_SLEW is used, as which is only supported on
x86, other platforms call it will trigger a assert
After that, we can move the x86 specific code to the common place
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
Message-Id: <20170510083259.3900-6-xiaoguangrong@tencent.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/timer/mc146818rtc.c | 60 ++++++++++++++++++++++++++------------------------
1 file changed, 31 insertions(+), 29 deletions(-)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index f9d6181..542cd09 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -125,17 +125,34 @@ static void rtc_coalesced_timer_update(RTCState *s)
}
}
+static QLIST_HEAD(, RTCState) rtc_devices =
+ QLIST_HEAD_INITIALIZER(rtc_devices);
+
#ifdef TARGET_I386
+void qmp_rtc_reset_reinjection(Error **errp)
+{
+ RTCState *s;
+
+ QLIST_FOREACH(s, &rtc_devices, link) {
+ s->irq_coalesced = 0;
+ }
+}
+
+static bool rtc_policy_slew_deliver_irq(RTCState *s)
+{
+ apic_reset_irq_delivered();
+ qemu_irq_raise(s->irq);
+ return apic_get_irq_delivered();
+}
+
static void rtc_coalesced_timer(void *opaque)
{
RTCState *s = opaque;
if (s->irq_coalesced != 0) {
- apic_reset_irq_delivered();
s->cmos_data[RTC_REG_C] |= 0xc0;
DPRINTF_C("cmos: injecting from timer\n");
- qemu_irq_raise(s->irq);
- if (apic_get_irq_delivered()) {
+ if (rtc_policy_slew_deliver_irq(s)) {
s->irq_coalesced--;
DPRINTF_C("cmos: coalesced irqs decreased to %d\n",
s->irq_coalesced);
@@ -144,6 +161,12 @@ static void rtc_coalesced_timer(void *opaque)
rtc_coalesced_timer_update(s);
}
+#else
+static bool rtc_policy_slew_deliver_irq(RTCState *s)
+{
+ assert(0);
+ return false;
+}
#endif
static uint32_t rtc_periodic_clock_ticks(RTCState *s)
@@ -254,21 +277,17 @@ static void rtc_periodic_timer(void *opaque)
s->cmos_data[RTC_REG_C] |= REG_C_PF;
if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
-#ifdef TARGET_I386
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
if (s->irq_reinject_on_ack_count >= RTC_REINJECT_ON_ACK_COUNT)
- s->irq_reinject_on_ack_count = 0;
- apic_reset_irq_delivered();
- qemu_irq_raise(s->irq);
- if (!apic_get_irq_delivered()) {
+ s->irq_reinject_on_ack_count = 0;
+ if (!rtc_policy_slew_deliver_irq(s)) {
s->irq_coalesced++;
rtc_coalesced_timer_update(s);
DPRINTF_C("cmos: coalesced irqs increased to %d\n",
s->irq_coalesced);
}
} else
-#endif
- qemu_irq_raise(s->irq);
+ qemu_irq_raise(s->irq);
}
}
@@ -612,20 +631,6 @@ static void rtc_get_time(RTCState *s, struct tm *tm)
rtc_from_bcd(s, s->cmos_data[RTC_CENTURY]) * 100 - 1900;
}
-static QLIST_HEAD(, RTCState) rtc_devices =
- QLIST_HEAD_INITIALIZER(rtc_devices);
-
-#ifdef TARGET_I386
-void qmp_rtc_reset_reinjection(Error **errp)
-{
- RTCState *s;
-
- QLIST_FOREACH(s, &rtc_devices, link) {
- s->irq_coalesced = 0;
- }
-}
-#endif
-
static void rtc_set_time(RTCState *s)
{
struct tm tm;
@@ -745,22 +750,19 @@ static uint64_t cmos_ioport_read(void *opaque, hwaddr addr,
if (ret & (REG_C_UF | REG_C_AF)) {
check_update_timer(s);
}
-#ifdef TARGET_I386
+
if(s->irq_coalesced &&
(s->cmos_data[RTC_REG_B] & REG_B_PIE) &&
s->irq_reinject_on_ack_count < RTC_REINJECT_ON_ACK_COUNT) {
s->irq_reinject_on_ack_count++;
s->cmos_data[RTC_REG_C] |= REG_C_IRQF | REG_C_PF;
- apic_reset_irq_delivered();
DPRINTF_C("cmos: injecting on ack\n");
- qemu_irq_raise(s->irq);
- if (apic_get_irq_delivered()) {
+ if (rtc_policy_slew_deliver_irq(s)) {
s->irq_coalesced--;
DPRINTF_C("cmos: coalesced irqs decreased to %d\n",
s->irq_coalesced);
}
}
-#endif
break;
default:
ret = s->cmos_data[s->cmos_index];
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 06/20] kvm: irqchip: trace changes on msi add/remove
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (4 preceding siblings ...)
2017-05-19 11:20 ` [Qemu-devel] [PULL 05/20] mc146818rtc: embrace all x86 specific code Paolo Bonzini
@ 2017-05-19 11:20 ` Paolo Bonzini
2017-05-19 11:20 ` [Qemu-devel] [PULL 07/20] msix: trace control bit write op Paolo Bonzini
` (15 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
From: Peter Xu <peterx@redhat.com>
It'll be nice to know which virq belongs to which device/vector when
adding msi routes, so adding two more parameters for the add trace.
Meanwhile, releasing virq has no tracing before. Add one for it.
Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <1494309644-18743-2-git-send-email-peterx@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
kvm-all.c | 4 +++-
trace-events | 3 ++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c
index 90b8573..2598b1f 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1144,6 +1144,7 @@ void kvm_irqchip_release_virq(KVMState *s, int virq)
}
clear_gsi(s, virq);
kvm_arch_release_virq_post(virq);
+ trace_kvm_irqchip_release_virq(virq);
}
static unsigned int kvm_hash_msi(uint32_t data)
@@ -1287,7 +1288,8 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
return -EINVAL;
}
- trace_kvm_irqchip_add_msi_route(virq);
+ trace_kvm_irqchip_add_msi_route(dev ? dev->name : (char *)"N/A",
+ vector, virq);
kvm_add_routing_entry(s, &kroute);
kvm_arch_add_msi_route_post(&kroute, vector, dev);
diff --git a/trace-events b/trace-events
index e582d63..f01ec05 100644
--- a/trace-events
+++ b/trace-events
@@ -69,8 +69,9 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
kvm_irqchip_commit_routes(void) ""
-kvm_irqchip_add_msi_route(int virq) "Adding MSI route virq=%d"
+kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
+kvm_irqchip_release_virq(int virq) "virq %d"
# TCG related tracing (mostly disabled by default)
# cpu-exec.c
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 07/20] msix: trace control bit write op
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (5 preceding siblings ...)
2017-05-19 11:20 ` [Qemu-devel] [PULL 06/20] kvm: irqchip: trace changes on msi add/remove Paolo Bonzini
@ 2017-05-19 11:20 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 08/20] kvm: irqchip: skip update msi when disabled Paolo Bonzini
` (14 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
From: Peter Xu <peterx@redhat.com>
Meanwhile, abstract a function to detect msix masked bit.
Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <1494309644-18743-3-git-send-email-peterx@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/pci/msix.c | 11 +++++++++--
hw/pci/trace-events | 3 +++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index bb54e8b..fc5fe51 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -22,6 +22,7 @@
#include "hw/xen/xen.h"
#include "qemu/range.h"
#include "qapi/error.h"
+#include "trace.h"
#define MSIX_CAP_LENGTH 12
@@ -130,10 +131,14 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked)
}
}
+static bool msix_masked(PCIDevice *dev)
+{
+ return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
+}
+
static void msix_update_function_masked(PCIDevice *dev)
{
- dev->msix_function_masked = !msix_enabled(dev) ||
- (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK);
+ dev->msix_function_masked = !msix_enabled(dev) || msix_masked(dev);
}
/* Handle MSI-X capability config write. */
@@ -148,6 +153,8 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
return;
}
+ trace_msix_write_config(dev->name, msix_enabled(dev), msix_masked(dev));
+
was_masked = dev->msix_function_masked;
msix_update_function_masked(dev);
diff --git a/hw/pci/trace-events b/hw/pci/trace-events
index 2b9cf24..83c8f5a 100644
--- a/hw/pci/trace-events
+++ b/hw/pci/trace-events
@@ -7,3 +7,6 @@ pci_update_mappings_add(void *d, uint32_t bus, uint32_t slot, uint32_t func, int
# hw/pci/pci_host.c
pci_cfg_read(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x -> 0x%x"
pci_cfg_write(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x <- 0x%x"
+
+# hw/pci/msix.c
+msix_write_config(char *name, bool enabled, bool masked) "dev %s enabled %d masked %d"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 08/20] kvm: irqchip: skip update msi when disabled
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (6 preceding siblings ...)
2017-05-19 11:20 ` [Qemu-devel] [PULL 07/20] msix: trace control bit write op Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 09/20] Check the return value of fcntl in qemu_set_cloexec Paolo Bonzini
` (13 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
From: Peter Xu <peterx@redhat.com>
It's possible that one device kept its irqfd/virq there even when
MSI/MSIX was disabled globally for that device. One example is
virtio-net-pci (see commit f1d0f15a6 and virtio_pci_vq_vector_mask()).
It is used as a fast path to avoid allocate/release irqfd/virq
frequently when guest enables/disables MSIX.
However, this fast path brought a problem to msi_route_list, that the
device MSIRouteEntry is still dangling there even if MSIX disabled -
then we cannot know which message to fetch, even if we can, the messages
are meaningless. In this case, we can just simply ignore this entry.
It's safe, since when MSIX is enabled again, we'll rebuild them no
matter what.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1448813
Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <1494309644-18743-4-git-send-email-peterx@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 011d4a5..82c72d2 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -43,6 +43,7 @@
#include "standard-headers/asm-x86/hyperv.h"
#include "hw/pci/pci.h"
#include "hw/pci/msi.h"
+#include "hw/pci/msix.h"
#include "migration/blocker.h"
#include "exec/memattrs.h"
#include "trace.h"
@@ -3510,12 +3511,17 @@ static void kvm_update_msi_routes_all(void *private, bool global,
int cnt = 0;
MSIRouteEntry *entry;
MSIMessage msg;
+ PCIDevice *dev;
+
/* TODO: explicit route update */
QLIST_FOREACH(entry, &msi_route_list, list) {
cnt++;
- msg = pci_get_msi_message(entry->dev, entry->vector);
- kvm_irqchip_update_msi_route(kvm_state, entry->virq,
- msg, entry->dev);
+ dev = entry->dev;
+ if (!msix_enabled(dev) && !msi_enabled(dev)) {
+ continue;
+ }
+ msg = pci_get_msi_message(dev, entry->vector);
+ kvm_irqchip_update_msi_route(kvm_state, entry->virq, msg, dev);
}
kvm_irqchip_commit_routes(kvm_state);
trace_kvm_x86_update_msi_routes(cnt);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 09/20] Check the return value of fcntl in qemu_set_cloexec
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (7 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 08/20] kvm: irqchip: skip update msi when disabled Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 10/20] nbd: strict nbd_wr_syncv Paolo Bonzini
` (12 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Stefano Stabellini, groug, pbonzini, Eric Blake
From: Stefano Stabellini <sstabellini@kernel.org>
Assert that the return value is not an error. This issue was found by
Coverity.
CID: 1374831
Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
CC: groug@kaod.org
CC: pbonzini@redhat.com
CC: Eric Blake <eblake@redhat.com>
Message-Id: <1494356693-13190-2-git-send-email-sstabellini@kernel.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
util/oslib-posix.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 4d9189e..16894ad 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -182,7 +182,9 @@ void qemu_set_cloexec(int fd)
{
int f;
f = fcntl(fd, F_GETFD);
- fcntl(fd, F_SETFD, f | FD_CLOEXEC);
+ assert(f != -1);
+ f = fcntl(fd, F_SETFD, f | FD_CLOEXEC);
+ assert(f != -1);
}
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 10/20] nbd: strict nbd_wr_syncv
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (8 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 09/20] Check the return value of fcntl in qemu_set_cloexec Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 11/20] nbd: read_sync and friends: return 0 on success Paolo Bonzini
` (11 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
nbd_wr_syncv is called either from coroutine or from client negotiation
code, when socket is in blocking mode. So, -EAGAIN is impossible.
Furthermore, EAGAIN is confusing, as, what to read/write again? With
EAGAIN as a return code we don't know how much data is already
read or written by the function, so in case of EAGAIN the whole
communication is broken.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20170516094533.6160-2-vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd/common.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/nbd/common.c b/nbd/common.c
index dccbb8e..4db45b3 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -20,6 +20,10 @@
#include "qapi/error.h"
#include "nbd-internal.h"
+/* nbd_wr_syncv
+ * The function may be called from coroutine or from non-coroutine context.
+ * When called from non-coroutine context @ioc must be in blocking mode.
+ */
ssize_t nbd_wr_syncv(QIOChannel *ioc,
struct iovec *iov,
size_t niov,
@@ -42,11 +46,8 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
len = qio_channel_writev(ioc, local_iov, nlocal_iov, &local_err);
}
if (len == QIO_CHANNEL_ERR_BLOCK) {
- if (qemu_in_coroutine()) {
- qio_channel_yield(ioc, do_read ? G_IO_IN : G_IO_OUT);
- } else {
- return -EAGAIN;
- }
+ assert(qemu_in_coroutine());
+ qio_channel_yield(ioc, do_read ? G_IO_IN : G_IO_OUT);
continue;
}
if (len < 0) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 11/20] nbd: read_sync and friends: return 0 on success
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (9 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 10/20] nbd: strict nbd_wr_syncv Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 12/20] nbd: add errp parameter to nbd_wr_syncv() Paolo Bonzini
` (10 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
functions read_sync, drop_sync, write_sync, and also
nbd_negotiate_write, nbd_negotiate_read, nbd_negotiate_drop_sync
returns number of processed bytes. But what this number can be,
except requested number of bytes?
Actually, underlying nbd_wr_syncv function returns a value >= 0 and
!= requested_bytes only on eof on read operation. So, firstly, it is
impossible on write (let's add an assert) and on read it actually
means, that communication is broken (except nbd_receive_reply, see
below).
Most of callers operate like this:
if (func(..., size) != size) {
/* error path */
}
, i.e.:
1. They are not interested in partial success
2. Extra duplications in code (especially bad are duplications of
magic numbers)
3. User doesn't see actual error message, as return code is lost.
(this patch doesn't fix this point, but it makes fixing easier)
Several callers handles ret >= 0 and != requested-size separately, by
just returning EINVAL in this case. This patch makes read_sync and
friends return EINVAL in this case, so final behavior is the same.
And only one caller - nbd_receive_reply() does something not so
obvious. It returns EINVAL for ret > 0 and != requested-size, like
previous group, but for ret == 0 it returns 0. The only caller of
nbd_receive_reply() - nbd_read_reply_entry() handles ret == 0 in the
same way as ret < 0, so for now it doesn't matter. However, in
following commits error path handling will be improved and we'll need
to distinguish success from fail in this case too. So, this patch adds
separate helper for this case - read_sync_eof.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20170516094533.6160-3-vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd/client.c | 63 ++++++++++++++++------------------------
nbd/nbd-internal.h | 34 +++++++++++++++++++---
nbd/server.c | 84 +++++++++++++++++++++---------------------------------
3 files changed, 88 insertions(+), 93 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index a58fb02..6b74a62 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -86,9 +86,9 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
*/
-/* Discard length bytes from channel. Return -errno on failure, or
- * the amount of bytes consumed. */
-static ssize_t drop_sync(QIOChannel *ioc, size_t size)
+/* Discard length bytes from channel. Return -errno on failure and 0 on
+ * success*/
+static int drop_sync(QIOChannel *ioc, size_t size)
{
ssize_t ret = 0;
char small[1024];
@@ -96,14 +96,13 @@ static ssize_t drop_sync(QIOChannel *ioc, size_t size)
buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));
while (size > 0) {
- ssize_t count = read_sync(ioc, buffer, MIN(65536, size));
+ ssize_t count = MIN(65536, size);
+ ret = read_sync(ioc, buffer, MIN(65536, size));
- if (count <= 0) {
+ if (ret < 0) {
goto cleanup;
}
- assert(count <= size);
size -= count;
- ret += count;
}
cleanup:
@@ -136,12 +135,12 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt,
stl_be_p(&req.option, opt);
stl_be_p(&req.length, len);
- if (write_sync(ioc, &req, sizeof(req)) != sizeof(req)) {
+ if (write_sync(ioc, &req, sizeof(req)) < 0) {
error_setg(errp, "Failed to send option request header");
return -1;
}
- if (len && write_sync(ioc, (char *) data, len) != len) {
+ if (len && write_sync(ioc, (char *) data, len) < 0) {
error_setg(errp, "Failed to send option request data");
return -1;
}
@@ -170,7 +169,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
nbd_opt_reply *reply, Error **errp)
{
QEMU_BUILD_BUG_ON(sizeof(*reply) != 20);
- if (read_sync(ioc, reply, sizeof(*reply)) != sizeof(*reply)) {
+ if (read_sync(ioc, reply, sizeof(*reply)) < 0) {
error_setg(errp, "failed to read option reply");
nbd_send_opt_abort(ioc);
return -1;
@@ -219,7 +218,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, nbd_opt_reply *reply,
goto cleanup;
}
msg = g_malloc(reply->length + 1);
- if (read_sync(ioc, msg, reply->length) != reply->length) {
+ if (read_sync(ioc, msg, reply->length) < 0) {
error_setg(errp, "failed to read option error message");
goto cleanup;
}
@@ -321,7 +320,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
nbd_send_opt_abort(ioc);
return -1;
}
- if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
+ if (read_sync(ioc, &namelen, sizeof(namelen)) < 0) {
error_setg(errp, "failed to read option name length");
nbd_send_opt_abort(ioc);
return -1;
@@ -334,7 +333,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
return -1;
}
if (namelen != strlen(want)) {
- if (drop_sync(ioc, len) != len) {
+ if (drop_sync(ioc, len) < 0) {
error_setg(errp, "failed to skip export name with wrong length");
nbd_send_opt_abort(ioc);
return -1;
@@ -343,14 +342,14 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
}
assert(namelen < sizeof(name));
- if (read_sync(ioc, name, namelen) != namelen) {
+ if (read_sync(ioc, name, namelen) < 0) {
error_setg(errp, "failed to read export name");
nbd_send_opt_abort(ioc);
return -1;
}
name[namelen] = '\0';
len -= namelen;
- if (drop_sync(ioc, len) != len) {
+ if (drop_sync(ioc, len) < 0) {
error_setg(errp, "failed to read export description");
nbd_send_opt_abort(ioc);
return -1;
@@ -477,7 +476,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}
- if (read_sync(ioc, buf, 8) != 8) {
+ if (read_sync(ioc, buf, 8) < 0) {
error_setg(errp, "Failed to read data");
goto fail;
}
@@ -503,7 +502,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}
- if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+ if (read_sync(ioc, &magic, sizeof(magic)) < 0) {
error_setg(errp, "Failed to read magic");
goto fail;
}
@@ -515,8 +514,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
uint16_t globalflags;
bool fixedNewStyle = false;
- if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
- sizeof(globalflags)) {
+ if (read_sync(ioc, &globalflags, sizeof(globalflags)) < 0) {
error_setg(errp, "Failed to read server flags");
goto fail;
}
@@ -534,8 +532,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
}
/* client requested flags */
clientflags = cpu_to_be32(clientflags);
- if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
- sizeof(clientflags)) {
+ if (write_sync(ioc, &clientflags, sizeof(clientflags)) < 0) {
error_setg(errp, "Failed to send clientflags field");
goto fail;
}
@@ -573,13 +570,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
}
/* Read the response */
- if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
+ if (read_sync(ioc, &s, sizeof(s)) < 0) {
error_setg(errp, "Failed to read export length");
goto fail;
}
*size = be64_to_cpu(s);
- if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
+ if (read_sync(ioc, flags, sizeof(*flags)) < 0) {
error_setg(errp, "Failed to read export flags");
goto fail;
}
@@ -596,14 +593,14 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}
- if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
+ if (read_sync(ioc, &s, sizeof(s)) < 0) {
error_setg(errp, "Failed to read export length");
goto fail;
}
*size = be64_to_cpu(s);
TRACE("Size is %" PRIu64, *size);
- if (read_sync(ioc, &oldflags, sizeof(oldflags)) != sizeof(oldflags)) {
+ if (read_sync(ioc, &oldflags, sizeof(oldflags)) < 0) {
error_setg(errp, "Failed to read export flags");
goto fail;
}
@@ -619,7 +616,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
}
TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
- if (zeroes && drop_sync(ioc, 124) != 124) {
+ if (zeroes && drop_sync(ioc, 124) < 0) {
error_setg(errp, "Failed to read reserved block");
goto fail;
}
@@ -744,7 +741,6 @@ int nbd_disconnect(int fd)
ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
{
uint8_t buf[NBD_REQUEST_SIZE];
- ssize_t ret;
TRACE("Sending request to server: "
"{ .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64
@@ -759,16 +755,7 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
stq_be_p(buf + 16, request->from);
stl_be_p(buf + 24, request->len);
- ret = write_sync(ioc, buf, sizeof(buf));
- if (ret < 0) {
- return ret;
- }
-
- if (ret != sizeof(buf)) {
- LOG("writing to socket failed");
- return -EINVAL;
- }
- return 0;
+ return write_sync(ioc, buf, sizeof(buf));
}
ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
@@ -777,7 +764,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
uint32_t magic;
ssize_t ret;
- ret = read_sync(ioc, buf, sizeof(buf));
+ ret = read_sync_eof(ioc, buf, sizeof(buf));
if (ret <= 0) {
return ret;
}
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index f43d990..e6bbc7c 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -94,7 +94,13 @@
#define NBD_ENOSPC 28
#define NBD_ESHUTDOWN 108
-static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size)
+/* read_sync_eof
+ * Tries to read @size bytes from @ioc. Returns number of bytes actually read.
+ * May return a value >= 0 and < size only on EOF, i.e. when iteratively called
+ * qio_channel_readv() returns 0. So, there are no needs to call read_sync_eof
+ * iteratively.
+ */
+static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size)
{
struct iovec iov = { .iov_base = buffer, .iov_len = size };
/* Sockets are kept in blocking mode in the negotiation phase. After
@@ -105,12 +111,32 @@ static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size)
return nbd_wr_syncv(ioc, &iov, 1, size, true);
}
-static inline ssize_t write_sync(QIOChannel *ioc, const void *buffer,
- size_t size)
+/* read_sync
+ * Reads @size bytes from @ioc. Returns 0 on success.
+ */
+static inline int read_sync(QIOChannel *ioc, void *buffer, size_t size)
+{
+ ssize_t ret = read_sync_eof(ioc, buffer, size);
+
+ if (ret >= 0 && ret != size) {
+ ret = -EINVAL;
+ }
+
+ return ret < 0 ? ret : 0;
+}
+
+/* write_sync
+ * Writes @size bytes to @ioc. Returns 0 on success.
+ */
+static inline int write_sync(QIOChannel *ioc, const void *buffer, size_t size)
{
struct iovec iov = { .iov_base = (void *) buffer, .iov_len = size };
- return nbd_wr_syncv(ioc, &iov, 1, size, false);
+ ssize_t ret = nbd_wr_syncv(ioc, &iov, 1, size, false);
+
+ assert(ret < 0 || ret == size);
+
+ return ret < 0 ? ret : 0;
}
struct NBDTLSHandshakeData {
diff --git a/nbd/server.c b/nbd/server.c
index 924a1fe..1e1096c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -112,7 +112,7 @@ static gboolean nbd_negotiate_continue(QIOChannel *ioc,
return TRUE;
}
-static ssize_t nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
+static int nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
{
ssize_t ret;
guint watch;
@@ -130,8 +130,7 @@ static ssize_t nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
}
-static ssize_t nbd_negotiate_write(QIOChannel *ioc, const void *buffer,
- size_t size)
+static int nbd_negotiate_write(QIOChannel *ioc, const void *buffer, size_t size)
{
ssize_t ret;
guint watch;
@@ -148,24 +147,24 @@ static ssize_t nbd_negotiate_write(QIOChannel *ioc, const void *buffer,
return ret;
}
-static ssize_t nbd_negotiate_drop_sync(QIOChannel *ioc, size_t size)
+static int nbd_negotiate_drop_sync(QIOChannel *ioc, size_t size)
{
- ssize_t ret, dropped = size;
+ ssize_t ret;
uint8_t *buffer = g_malloc(MIN(65536, size));
while (size > 0) {
- ret = nbd_negotiate_read(ioc, buffer, MIN(65536, size));
+ size_t count = MIN(65536, size);
+ ret = nbd_negotiate_read(ioc, buffer, count);
if (ret < 0) {
g_free(buffer);
return ret;
}
- assert(ret <= size);
- size -= ret;
+ size -= count;
}
g_free(buffer);
- return dropped;
+ return 0;
}
/* Basic flow for negotiation
@@ -206,22 +205,22 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
type, opt, len);
magic = cpu_to_be64(NBD_REP_MAGIC);
- if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+ if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) < 0) {
LOG("write failed (rep magic)");
return -EINVAL;
}
opt = cpu_to_be32(opt);
- if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
+ if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) < 0) {
LOG("write failed (rep opt)");
return -EINVAL;
}
type = cpu_to_be32(type);
- if (nbd_negotiate_write(ioc, &type, sizeof(type)) != sizeof(type)) {
+ if (nbd_negotiate_write(ioc, &type, sizeof(type)) < 0) {
LOG("write failed (rep type)");
return -EINVAL;
}
len = cpu_to_be32(len);
- if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
+ if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) {
LOG("write failed (rep data length)");
return -EINVAL;
}
@@ -256,7 +255,7 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
if (ret < 0) {
goto out;
}
- if (nbd_negotiate_write(ioc, msg, len) != len) {
+ if (nbd_negotiate_write(ioc, msg, len) < 0) {
LOG("write failed (error message)");
ret = -EIO;
} else {
@@ -287,15 +286,15 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
}
len = cpu_to_be32(name_len);
- if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
+ if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) {
LOG("write failed (name length)");
return -EINVAL;
}
- if (nbd_negotiate_write(ioc, name, name_len) != name_len) {
+ if (nbd_negotiate_write(ioc, name, name_len) < 0) {
LOG("write failed (name buffer)");
return -EINVAL;
}
- if (nbd_negotiate_write(ioc, desc, desc_len) != desc_len) {
+ if (nbd_negotiate_write(ioc, desc, desc_len) < 0) {
LOG("write failed (description buffer)");
return -EINVAL;
}
@@ -309,7 +308,7 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
NBDExport *exp;
if (length) {
- if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
+ if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
return -EIO;
}
return nbd_negotiate_send_rep_err(client->ioc,
@@ -340,7 +339,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
LOG("Bad length received");
goto fail;
}
- if (nbd_negotiate_read(client->ioc, name, length) != length) {
+ if (nbd_negotiate_read(client->ioc, name, length) < 0) {
LOG("read failed");
goto fail;
}
@@ -373,7 +372,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
TRACE("Setting up TLS");
ioc = client->ioc;
if (length) {
- if (nbd_negotiate_drop_sync(ioc, length) != length) {
+ if (nbd_negotiate_drop_sync(ioc, length) < 0) {
return NULL;
}
nbd_negotiate_send_rep_err(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS,
@@ -437,8 +436,7 @@ static int nbd_negotiate_options(NBDClient *client)
... Rest of request
*/
- if (nbd_negotiate_read(client->ioc, &flags, sizeof(flags)) !=
- sizeof(flags)) {
+ if (nbd_negotiate_read(client->ioc, &flags, sizeof(flags)) < 0) {
LOG("read failed");
return -EIO;
}
@@ -464,8 +462,7 @@ static int nbd_negotiate_options(NBDClient *client)
uint32_t clientflags, length;
uint64_t magic;
- if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) !=
- sizeof(magic)) {
+ if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) < 0) {
LOG("read failed");
return -EINVAL;
}
@@ -476,14 +473,14 @@ static int nbd_negotiate_options(NBDClient *client)
}
if (nbd_negotiate_read(client->ioc, &clientflags,
- sizeof(clientflags)) != sizeof(clientflags)) {
+ sizeof(clientflags)) < 0)
+ {
LOG("read failed");
return -EINVAL;
}
clientflags = be32_to_cpu(clientflags);
- if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) !=
- sizeof(length)) {
+ if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) < 0) {
LOG("read failed");
return -EINVAL;
}
@@ -513,7 +510,7 @@ static int nbd_negotiate_options(NBDClient *client)
return -EINVAL;
default:
- if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
+ if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
return -EIO;
}
ret = nbd_negotiate_send_rep_err(client->ioc,
@@ -551,7 +548,7 @@ static int nbd_negotiate_options(NBDClient *client)
return nbd_negotiate_handle_export_name(client, length);
case NBD_OPT_STARTTLS:
- if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
+ if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
return -EIO;
}
if (client->tlscreds) {
@@ -570,7 +567,7 @@ static int nbd_negotiate_options(NBDClient *client)
}
break;
default:
- if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
+ if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
return -EIO;
}
ret = nbd_negotiate_send_rep_err(client->ioc,
@@ -659,12 +656,12 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
TRACE("TLS cannot be enabled with oldstyle protocol");
goto fail;
}
- if (nbd_negotiate_write(client->ioc, buf, sizeof(buf)) != sizeof(buf)) {
+ if (nbd_negotiate_write(client->ioc, buf, sizeof(buf)) < 0) {
LOG("write failed");
goto fail;
}
} else {
- if (nbd_negotiate_write(client->ioc, buf, 18) != 18) {
+ if (nbd_negotiate_write(client->ioc, buf, 18) < 0) {
LOG("write failed");
goto fail;
}
@@ -679,7 +676,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
stq_be_p(buf + 18, client->exp->size);
stw_be_p(buf + 26, client->exp->nbdflags | myflags);
len = client->no_zeroes ? 10 : sizeof(buf) - 18;
- if (nbd_negotiate_write(client->ioc, buf + 18, len) != len) {
+ if (nbd_negotiate_write(client->ioc, buf + 18, len) < 0) {
LOG("write failed");
goto fail;
}
@@ -702,11 +699,6 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
return ret;
}
- if (ret != sizeof(buf)) {
- LOG("read failed");
- return -EINVAL;
- }
-
/* Request
[ 0 .. 3] magic (NBD_REQUEST_MAGIC)
[ 4 .. 5] flags (NBD_CMD_FLAG_FUA, ...)
@@ -737,7 +729,6 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
static ssize_t nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
{
uint8_t buf[NBD_REPLY_SIZE];
- ssize_t ret;
reply->error = system_errno_to_nbd_errno(reply->error);
@@ -754,16 +745,7 @@ static ssize_t nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
stl_be_p(buf + 4, reply->error);
stq_be_p(buf + 8, reply->handle);
- ret = write_sync(ioc, buf, sizeof(buf));
- if (ret < 0) {
- return ret;
- }
-
- if (ret != sizeof(buf)) {
- LOG("writing to socket failed");
- return -EINVAL;
- }
- return 0;
+ return write_sync(ioc, buf, sizeof(buf));
}
#define MAX_NBD_REQUESTS 16
@@ -1067,7 +1049,7 @@ static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
rc = nbd_send_reply(client->ioc, reply);
if (rc >= 0) {
ret = write_sync(client->ioc, req->data, len);
- if (ret != len) {
+ if (ret < 0) {
rc = -EIO;
}
}
@@ -1141,7 +1123,7 @@ static ssize_t nbd_co_receive_request(NBDRequestData *req,
if (request->type == NBD_CMD_WRITE) {
TRACE("Reading %" PRIu32 " byte(s)", request->len);
- if (read_sync(client->ioc, req->data, request->len) != request->len) {
+ if (read_sync(client->ioc, req->data, request->len) < 0) {
LOG("reading from socket failed");
rc = -EIO;
goto out;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 12/20] nbd: add errp parameter to nbd_wr_syncv()
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (10 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 11/20] nbd: read_sync and friends: return 0 on success Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 13/20] nbd: add errp to read_sync, write_sync and drop_sync Paolo Bonzini
` (9 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Will be used in following patch to provide actual error message in
some cases.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20170516094533.6160-4-vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/nbd-client.c | 4 ++--
include/block/nbd.h | 3 ++-
nbd/common.c | 12 +++++-------
nbd/nbd-internal.h | 4 ++--
4 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 1e2952f..538d95e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -136,7 +136,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
rc = nbd_send_request(s->ioc, request);
if (rc >= 0) {
ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov, request->len,
- false);
+ false, NULL);
if (ret != request->len) {
rc = -EIO;
}
@@ -165,7 +165,7 @@ static void nbd_co_receive_reply(NBDClientSession *s,
} else {
if (qiov && reply->error == 0) {
ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov, request->len,
- true);
+ true, NULL);
if (ret != request->len) {
reply->error = EIO;
}
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 0ed0775..9d385ea 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -127,7 +127,8 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
struct iovec *iov,
size_t niov,
size_t length,
- bool do_read);
+ bool do_read,
+ Error **errp);
int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
QCryptoTLSCreds *tlscreds, const char *hostname,
QIOChannel **outioc,
diff --git a/nbd/common.c b/nbd/common.c
index 4db45b3..bd81637 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -28,10 +28,10 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
struct iovec *iov,
size_t niov,
size_t length,
- bool do_read)
+ bool do_read,
+ Error **errp)
{
ssize_t done = 0;
- Error *local_err = NULL;
struct iovec *local_iov = g_new(struct iovec, niov);
struct iovec *local_iov_head = local_iov;
unsigned int nlocal_iov = niov;
@@ -41,19 +41,17 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
while (nlocal_iov > 0) {
ssize_t len;
if (do_read) {
- len = qio_channel_readv(ioc, local_iov, nlocal_iov, &local_err);
+ len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
} else {
- len = qio_channel_writev(ioc, local_iov, nlocal_iov, &local_err);
+ len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
}
if (len == QIO_CHANNEL_ERR_BLOCK) {
+ /* errp should not be set */
assert(qemu_in_coroutine());
qio_channel_yield(ioc, do_read ? G_IO_IN : G_IO_OUT);
continue;
}
if (len < 0) {
- TRACE("I/O error: %s", error_get_pretty(local_err));
- error_free(local_err);
- /* XXX handle Error objects */
done = -EIO;
goto cleanup;
}
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index e6bbc7c..1d479fe 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -108,7 +108,7 @@ static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size)
* our request/reply. Synchronization is done with recv_coroutine, so
* that this is coroutine-safe.
*/
- return nbd_wr_syncv(ioc, &iov, 1, size, true);
+ return nbd_wr_syncv(ioc, &iov, 1, size, true, NULL);
}
/* read_sync
@@ -132,7 +132,7 @@ static inline int write_sync(QIOChannel *ioc, const void *buffer, size_t size)
{
struct iovec iov = { .iov_base = (void *) buffer, .iov_len = size };
- ssize_t ret = nbd_wr_syncv(ioc, &iov, 1, size, false);
+ ssize_t ret = nbd_wr_syncv(ioc, &iov, 1, size, false, NULL);
assert(ret < 0 || ret == size);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 13/20] nbd: add errp to read_sync, write_sync and drop_sync
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (11 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 12/20] nbd: add errp parameter to nbd_wr_syncv() Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 14/20] nbd/client.c: use errp instead of LOG Paolo Bonzini
` (8 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
There a lot of calls of these functions, which already have errp, which
they are filling themselves. On the other hand, nbd_wr_syncv has errp
parameter too, so it would be great to connect them.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20170516094533.6160-5-vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd/client.c | 76 +++++++++++++++++++++++++++---------------------------
nbd/nbd-internal.h | 16 +++++++-----
nbd/server.c | 12 ++++-----
3 files changed, 54 insertions(+), 50 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 6b74a62..f102375 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -88,7 +88,7 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
/* Discard length bytes from channel. Return -errno on failure and 0 on
* success*/
-static int drop_sync(QIOChannel *ioc, size_t size)
+static int drop_sync(QIOChannel *ioc, size_t size, Error **errp)
{
ssize_t ret = 0;
char small[1024];
@@ -97,7 +97,7 @@ static int drop_sync(QIOChannel *ioc, size_t size)
buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));
while (size > 0) {
ssize_t count = MIN(65536, size);
- ret = read_sync(ioc, buffer, MIN(65536, size));
+ ret = read_sync(ioc, buffer, MIN(65536, size), errp);
if (ret < 0) {
goto cleanup;
@@ -135,13 +135,13 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt,
stl_be_p(&req.option, opt);
stl_be_p(&req.length, len);
- if (write_sync(ioc, &req, sizeof(req)) < 0) {
- error_setg(errp, "Failed to send option request header");
+ if (write_sync(ioc, &req, sizeof(req), errp) < 0) {
+ error_prepend(errp, "Failed to send option request header");
return -1;
}
- if (len && write_sync(ioc, (char *) data, len) < 0) {
- error_setg(errp, "Failed to send option request data");
+ if (len && write_sync(ioc, (char *) data, len, errp) < 0) {
+ error_prepend(errp, "Failed to send option request data");
return -1;
}
@@ -169,8 +169,8 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
nbd_opt_reply *reply, Error **errp)
{
QEMU_BUILD_BUG_ON(sizeof(*reply) != 20);
- if (read_sync(ioc, reply, sizeof(*reply)) < 0) {
- error_setg(errp, "failed to read option reply");
+ if (read_sync(ioc, reply, sizeof(*reply), errp) < 0) {
+ error_prepend(errp, "failed to read option reply");
nbd_send_opt_abort(ioc);
return -1;
}
@@ -218,8 +218,8 @@ static int nbd_handle_reply_err(QIOChannel *ioc, nbd_opt_reply *reply,
goto cleanup;
}
msg = g_malloc(reply->length + 1);
- if (read_sync(ioc, msg, reply->length) < 0) {
- error_setg(errp, "failed to read option error message");
+ if (read_sync(ioc, msg, reply->length, errp) < 0) {
+ error_prepend(errp, "failed to read option error message");
goto cleanup;
}
msg[reply->length] = '\0';
@@ -320,8 +320,8 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
nbd_send_opt_abort(ioc);
return -1;
}
- if (read_sync(ioc, &namelen, sizeof(namelen)) < 0) {
- error_setg(errp, "failed to read option name length");
+ if (read_sync(ioc, &namelen, sizeof(namelen), errp) < 0) {
+ error_prepend(errp, "failed to read option name length");
nbd_send_opt_abort(ioc);
return -1;
}
@@ -333,8 +333,8 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
return -1;
}
if (namelen != strlen(want)) {
- if (drop_sync(ioc, len) < 0) {
- error_setg(errp, "failed to skip export name with wrong length");
+ if (drop_sync(ioc, len, errp) < 0) {
+ error_prepend(errp, "failed to skip export name with wrong length");
nbd_send_opt_abort(ioc);
return -1;
}
@@ -342,15 +342,15 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
}
assert(namelen < sizeof(name));
- if (read_sync(ioc, name, namelen) < 0) {
- error_setg(errp, "failed to read export name");
+ if (read_sync(ioc, name, namelen, errp) < 0) {
+ error_prepend(errp, "failed to read export name");
nbd_send_opt_abort(ioc);
return -1;
}
name[namelen] = '\0';
len -= namelen;
- if (drop_sync(ioc, len) < 0) {
- error_setg(errp, "failed to read export description");
+ if (drop_sync(ioc, len, errp) < 0) {
+ error_prepend(errp, "failed to read export description");
nbd_send_opt_abort(ioc);
return -1;
}
@@ -476,8 +476,8 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}
- if (read_sync(ioc, buf, 8) < 0) {
- error_setg(errp, "Failed to read data");
+ if (read_sync(ioc, buf, 8, errp) < 0) {
+ error_prepend(errp, "Failed to read data");
goto fail;
}
@@ -502,8 +502,8 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}
- if (read_sync(ioc, &magic, sizeof(magic)) < 0) {
- error_setg(errp, "Failed to read magic");
+ if (read_sync(ioc, &magic, sizeof(magic), errp) < 0) {
+ error_prepend(errp, "Failed to read magic");
goto fail;
}
magic = be64_to_cpu(magic);
@@ -514,8 +514,8 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
uint16_t globalflags;
bool fixedNewStyle = false;
- if (read_sync(ioc, &globalflags, sizeof(globalflags)) < 0) {
- error_setg(errp, "Failed to read server flags");
+ if (read_sync(ioc, &globalflags, sizeof(globalflags), errp) < 0) {
+ error_prepend(errp, "Failed to read server flags");
goto fail;
}
globalflags = be16_to_cpu(globalflags);
@@ -532,8 +532,8 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
}
/* client requested flags */
clientflags = cpu_to_be32(clientflags);
- if (write_sync(ioc, &clientflags, sizeof(clientflags)) < 0) {
- error_setg(errp, "Failed to send clientflags field");
+ if (write_sync(ioc, &clientflags, sizeof(clientflags), errp) < 0) {
+ error_prepend(errp, "Failed to send clientflags field");
goto fail;
}
if (tlscreds) {
@@ -570,14 +570,14 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
}
/* Read the response */
- if (read_sync(ioc, &s, sizeof(s)) < 0) {
- error_setg(errp, "Failed to read export length");
+ if (read_sync(ioc, &s, sizeof(s), errp) < 0) {
+ error_prepend(errp, "Failed to read export length");
goto fail;
}
*size = be64_to_cpu(s);
- if (read_sync(ioc, flags, sizeof(*flags)) < 0) {
- error_setg(errp, "Failed to read export flags");
+ if (read_sync(ioc, flags, sizeof(*flags), errp) < 0) {
+ error_prepend(errp, "Failed to read export flags");
goto fail;
}
be16_to_cpus(flags);
@@ -593,15 +593,15 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}
- if (read_sync(ioc, &s, sizeof(s)) < 0) {
- error_setg(errp, "Failed to read export length");
+ if (read_sync(ioc, &s, sizeof(s), errp) < 0) {
+ error_prepend(errp, "Failed to read export length");
goto fail;
}
*size = be64_to_cpu(s);
TRACE("Size is %" PRIu64, *size);
- if (read_sync(ioc, &oldflags, sizeof(oldflags)) < 0) {
- error_setg(errp, "Failed to read export flags");
+ if (read_sync(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
+ error_prepend(errp, "Failed to read export flags");
goto fail;
}
be32_to_cpus(&oldflags);
@@ -616,8 +616,8 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
}
TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
- if (zeroes && drop_sync(ioc, 124) < 0) {
- error_setg(errp, "Failed to read reserved block");
+ if (zeroes && drop_sync(ioc, 124, errp) < 0) {
+ error_prepend(errp, "Failed to read reserved block");
goto fail;
}
rc = 0;
@@ -755,7 +755,7 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
stq_be_p(buf + 16, request->from);
stl_be_p(buf + 24, request->len);
- return write_sync(ioc, buf, sizeof(buf));
+ return write_sync(ioc, buf, sizeof(buf), NULL);
}
ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
@@ -764,7 +764,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
uint32_t magic;
ssize_t ret;
- ret = read_sync_eof(ioc, buf, sizeof(buf));
+ ret = read_sync_eof(ioc, buf, sizeof(buf), NULL);
if (ret <= 0) {
return ret;
}
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 1d479fe..d607164 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -100,7 +100,8 @@
* qio_channel_readv() returns 0. So, there are no needs to call read_sync_eof
* iteratively.
*/
-static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size)
+static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size,
+ Error **errp)
{
struct iovec iov = { .iov_base = buffer, .iov_len = size };
/* Sockets are kept in blocking mode in the negotiation phase. After
@@ -108,18 +109,20 @@ static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size)
* our request/reply. Synchronization is done with recv_coroutine, so
* that this is coroutine-safe.
*/
- return nbd_wr_syncv(ioc, &iov, 1, size, true, NULL);
+ return nbd_wr_syncv(ioc, &iov, 1, size, true, errp);
}
/* read_sync
* Reads @size bytes from @ioc. Returns 0 on success.
*/
-static inline int read_sync(QIOChannel *ioc, void *buffer, size_t size)
+static inline int read_sync(QIOChannel *ioc, void *buffer, size_t size,
+ Error **errp)
{
- ssize_t ret = read_sync_eof(ioc, buffer, size);
+ ssize_t ret = read_sync_eof(ioc, buffer, size, errp);
if (ret >= 0 && ret != size) {
ret = -EINVAL;
+ error_setg(errp, "End of file");
}
return ret < 0 ? ret : 0;
@@ -128,11 +131,12 @@ static inline int read_sync(QIOChannel *ioc, void *buffer, size_t size)
/* write_sync
* Writes @size bytes to @ioc. Returns 0 on success.
*/
-static inline int write_sync(QIOChannel *ioc, const void *buffer, size_t size)
+static inline int write_sync(QIOChannel *ioc, const void *buffer, size_t size,
+ Error **errp)
{
struct iovec iov = { .iov_base = (void *) buffer, .iov_len = size };
- ssize_t ret = nbd_wr_syncv(ioc, &iov, 1, size, false, NULL);
+ ssize_t ret = nbd_wr_syncv(ioc, &iov, 1, size, false, errp);
assert(ret < 0 || ret == size);
diff --git a/nbd/server.c b/nbd/server.c
index 1e1096c..ee59e5d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -124,7 +124,7 @@ static int nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
nbd_negotiate_continue,
qemu_coroutine_self(),
NULL);
- ret = read_sync(ioc, buffer, size);
+ ret = read_sync(ioc, buffer, size, NULL);
g_source_remove(watch);
return ret;
@@ -142,7 +142,7 @@ static int nbd_negotiate_write(QIOChannel *ioc, const void *buffer, size_t size)
nbd_negotiate_continue,
qemu_coroutine_self(),
NULL);
- ret = write_sync(ioc, buffer, size);
+ ret = write_sync(ioc, buffer, size, NULL);
g_source_remove(watch);
return ret;
}
@@ -694,7 +694,7 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
uint32_t magic;
ssize_t ret;
- ret = read_sync(ioc, buf, sizeof(buf));
+ ret = read_sync(ioc, buf, sizeof(buf), NULL);
if (ret < 0) {
return ret;
}
@@ -745,7 +745,7 @@ static ssize_t nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
stl_be_p(buf + 4, reply->error);
stq_be_p(buf + 8, reply->handle);
- return write_sync(ioc, buf, sizeof(buf));
+ return write_sync(ioc, buf, sizeof(buf), NULL);
}
#define MAX_NBD_REQUESTS 16
@@ -1048,7 +1048,7 @@ static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
qio_channel_set_cork(client->ioc, true);
rc = nbd_send_reply(client->ioc, reply);
if (rc >= 0) {
- ret = write_sync(client->ioc, req->data, len);
+ ret = write_sync(client->ioc, req->data, len, NULL);
if (ret < 0) {
rc = -EIO;
}
@@ -1123,7 +1123,7 @@ static ssize_t nbd_co_receive_request(NBDRequestData *req,
if (request->type == NBD_CMD_WRITE) {
TRACE("Reading %" PRIu32 " byte(s)", request->len);
- if (read_sync(client->ioc, req->data, request->len) < 0) {
+ if (read_sync(client->ioc, req->data, request->len, NULL) < 0) {
LOG("reading from socket failed");
rc = -EIO;
goto out;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 14/20] nbd/client.c: use errp instead of LOG
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (12 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 13/20] nbd: add errp to read_sync, write_sync and drop_sync Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-26 11:09 ` [Qemu-devel] [PATCH v2] " Vladimir Sementsov-Ogievskiy
2017-05-19 11:21 ` [Qemu-devel] [PULL 15/20] exec: simplify phys_page_find() params Paolo Bonzini
` (7 subsequent siblings)
21 siblings, 1 reply; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Move to modern errp scheme from just LOGging errors.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20170516094533.6160-6-vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/nbd-client.c | 7 ++++++-
include/block/nbd.h | 5 +++--
nbd/client.c | 30 +++++++++++++++++-------------
qemu-nbd.c | 3 ++-
4 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 538d95e..073032b 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -28,6 +28,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "nbd-client.h"
#define HANDLE_TO_INDEX(bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs))
@@ -70,10 +71,14 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
NBDClientSession *s = opaque;
uint64_t i;
int ret;
+ Error *local_err;
for (;;) {
assert(s->reply.handle == 0);
- ret = nbd_receive_reply(s->ioc, &s->reply);
+ ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
if (ret <= 0) {
break;
}
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9d385ea..416257a 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -133,9 +133,10 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
QCryptoTLSCreds *tlscreds, const char *hostname,
QIOChannel **outioc,
off_t *size, Error **errp);
-int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size);
+int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
+ Error **errp);
ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request);
-ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply);
+ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp);
int nbd_client(int fd);
int nbd_disconnect(int fd);
diff --git a/nbd/client.c b/nbd/client.c
index f102375..595d99e 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -627,11 +627,13 @@ fail:
}
#ifdef __linux__
-int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
+ Error **errp)
{
unsigned long sectors = size / BDRV_SECTOR_SIZE;
if (size / BDRV_SECTOR_SIZE != sectors) {
- LOG("Export size %lld too large for 32-bit kernel", (long long) size);
+ error_setg(errp, "Export size %lld too large for 32-bit kernel",
+ (long long) size);
return -E2BIG;
}
@@ -639,7 +641,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, NBD_SET_SOCK, (unsigned long) sioc->fd) < 0) {
int serrno = errno;
- LOG("Failed to set NBD socket");
+ error_setg(errp, "Failed to set NBD socket");
return -serrno;
}
@@ -647,7 +649,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, NBD_SET_BLKSIZE, (unsigned long)BDRV_SECTOR_SIZE) < 0) {
int serrno = errno;
- LOG("Failed setting NBD block size");
+ error_setg(errp, "Failed setting NBD block size");
return -serrno;
}
@@ -659,7 +661,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, NBD_SET_SIZE_BLOCKS, sectors) < 0) {
int serrno = errno;
- LOG("Failed setting size (in blocks)");
+ error_setg(errp, "Failed setting size (in blocks)");
return -serrno;
}
@@ -670,12 +672,12 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, BLKROSET, (unsigned long) &read_only) < 0) {
int serrno = errno;
- LOG("Failed setting read-only attribute");
+ error_setg(errp, "Failed setting read-only attribute");
return -serrno;
}
} else {
int serrno = errno;
- LOG("Failed setting flags");
+ error_setg(errp, "Failed setting flags");
return -serrno;
}
}
@@ -723,8 +725,10 @@ int nbd_disconnect(int fd)
}
#else
-int nbd_init(int fd, QIOChannelSocket *ioc, uint16_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *ioc, uint16_t flags, off_t size,
+ Error **errp)
{
+ error_setg(errp, "nbd_init is only supported on Linux");
return -ENOTSUP;
}
@@ -758,19 +762,19 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
return write_sync(ioc, buf, sizeof(buf), NULL);
}
-ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
+ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
{
uint8_t buf[NBD_REPLY_SIZE];
uint32_t magic;
ssize_t ret;
- ret = read_sync_eof(ioc, buf, sizeof(buf), NULL);
+ ret = read_sync_eof(ioc, buf, sizeof(buf), errp);
if (ret <= 0) {
return ret;
}
if (ret != sizeof(buf)) {
- LOG("read failed");
+ error_setg(errp, "read failed");
return -EINVAL;
}
@@ -788,7 +792,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
if (reply->error == ESHUTDOWN) {
/* This works even on mingw which lacks a native ESHUTDOWN */
- LOG("server shutting down");
+ error_setg(errp, "server shutting down");
return -EINVAL;
}
TRACE("Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32
@@ -796,7 +800,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
magic, reply->error, reply->handle);
if (magic != NBD_REPLY_MAGIC) {
- LOG("invalid magic (got 0x%" PRIx32 ")", magic);
+ error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
return -EINVAL;
}
return sizeof(buf);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index b7ab86b..f60842f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -288,8 +288,9 @@ static void *nbd_client_thread(void *arg)
goto out_socket;
}
- ret = nbd_init(fd, sioc, nbdflags, size);
+ ret = nbd_init(fd, sioc, nbdflags, size, &local_error);
if (ret < 0) {
+ error_report_err(local_error);
goto out_fd;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v2] nbd/client.c: use errp instead of LOG
2017-05-19 11:21 ` [Qemu-devel] [PULL 14/20] nbd/client.c: use errp instead of LOG Paolo Bonzini
@ 2017-05-26 11:09 ` Vladimir Sementsov-Ogievskiy
2017-05-26 13:43 ` Eric Blake
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-05-26 11:09 UTC (permalink / raw)
To: qemu-devel, pbonzini; +Cc: vsementsov
Move to modern errp scheme from just LOGging errors.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
Fixes:
- local_err initialized to NULL
- 083 iotest ajusted
\I feel like an idiot...
block/nbd-client.c | 7 ++++++-
include/block/nbd.h | 5 +++--
nbd/client.c | 30 +++++++++++++++++-------------
qemu-nbd.c | 3 ++-
tests/qemu-iotests/083.out | 2 ++
5 files changed, 30 insertions(+), 17 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 538d95e031..09d955bc4d 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -28,6 +28,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "nbd-client.h"
#define HANDLE_TO_INDEX(bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs))
@@ -70,10 +71,14 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
NBDClientSession *s = opaque;
uint64_t i;
int ret;
+ Error *local_err = NULL;
for (;;) {
assert(s->reply.handle == 0);
- ret = nbd_receive_reply(s->ioc, &s->reply);
+ ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
if (ret <= 0) {
break;
}
diff --git a/include/block/nbd.h b/include/block/nbd.h
index a96fb5fceb..e0c64e4981 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -133,9 +133,10 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
QCryptoTLSCreds *tlscreds, const char *hostname,
QIOChannel **outioc,
off_t *size, Error **errp);
-int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size);
+int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
+ Error **errp);
ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request);
-ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply);
+ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp);
int nbd_client(int fd);
int nbd_disconnect(int fd);
diff --git a/nbd/client.c b/nbd/client.c
index f102375504..f9e1d75be4 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -627,11 +627,13 @@ fail:
}
#ifdef __linux__
-int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
+ Error **errp)
{
unsigned long sectors = size / BDRV_SECTOR_SIZE;
if (size / BDRV_SECTOR_SIZE != sectors) {
- LOG("Export size %lld too large for 32-bit kernel", (long long) size);
+ error_setg(errp, "Export size %lld too large for 32-bit kernel",
+ (long long) size);
return -E2BIG;
}
@@ -639,7 +641,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, NBD_SET_SOCK, (unsigned long) sioc->fd) < 0) {
int serrno = errno;
- LOG("Failed to set NBD socket");
+ error_setg(errp, "Failed to set NBD socket");
return -serrno;
}
@@ -647,7 +649,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, NBD_SET_BLKSIZE, (unsigned long)BDRV_SECTOR_SIZE) < 0) {
int serrno = errno;
- LOG("Failed setting NBD block size");
+ error_setg(errp, "Failed setting NBD block size");
return -serrno;
}
@@ -659,7 +661,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, NBD_SET_SIZE_BLOCKS, sectors) < 0) {
int serrno = errno;
- LOG("Failed setting size (in blocks)");
+ error_setg(errp, "Failed setting size (in blocks)");
return -serrno;
}
@@ -670,12 +672,12 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, BLKROSET, (unsigned long) &read_only) < 0) {
int serrno = errno;
- LOG("Failed setting read-only attribute");
+ error_setg(errp, "Failed setting read-only attribute");
return -serrno;
}
} else {
int serrno = errno;
- LOG("Failed setting flags");
+ error_setg(errp, "Failed setting flags");
return -serrno;
}
}
@@ -723,8 +725,10 @@ int nbd_disconnect(int fd)
}
#else
-int nbd_init(int fd, QIOChannelSocket *ioc, uint16_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *ioc, uint16_t flags, off_t size,
+ Error **errp)
{
+ error_setg(errp, "nbd_init is not supported");
return -ENOTSUP;
}
@@ -758,19 +762,19 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
return write_sync(ioc, buf, sizeof(buf), NULL);
}
-ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
+ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
{
uint8_t buf[NBD_REPLY_SIZE];
uint32_t magic;
ssize_t ret;
- ret = read_sync_eof(ioc, buf, sizeof(buf), NULL);
+ ret = read_sync_eof(ioc, buf, sizeof(buf), errp);
if (ret <= 0) {
return ret;
}
if (ret != sizeof(buf)) {
- LOG("read failed");
+ error_setg(errp, "read failed");
return -EINVAL;
}
@@ -788,7 +792,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
if (reply->error == ESHUTDOWN) {
/* This works even on mingw which lacks a native ESHUTDOWN */
- LOG("server shutting down");
+ error_setg(errp, "server shutting down");
return -EINVAL;
}
TRACE("Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32
@@ -796,7 +800,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
magic, reply->error, reply->handle);
if (magic != NBD_REPLY_MAGIC) {
- LOG("invalid magic (got 0x%" PRIx32 ")", magic);
+ error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
return -EINVAL;
}
return sizeof(buf);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index e080fb7c75..83b8b3e1d9 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -288,8 +288,9 @@ static void *nbd_client_thread(void *arg)
goto out_socket;
}
- ret = nbd_init(fd, sioc, nbdflags, size);
+ ret = nbd_init(fd, sioc, nbdflags, size, &local_error);
if (ret < 0) {
+ error_report_err(local_error);
goto out_fd;
}
diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
index 0c13888ba1..a24c6bfece 100644
--- a/tests/qemu-iotests/083.out
+++ b/tests/qemu-iotests/083.out
@@ -69,10 +69,12 @@ read failed: Input/output error
=== Check disconnect 4 reply ===
+read failed
read failed: Input/output error
=== Check disconnect 8 reply ===
+read failed
read failed: Input/output error
=== Check disconnect before data ===
--
2.11.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v2] nbd/client.c: use errp instead of LOG
2017-05-26 11:09 ` [Qemu-devel] [PATCH v2] " Vladimir Sementsov-Ogievskiy
@ 2017-05-26 13:43 ` Eric Blake
0 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2017-05-26 13:43 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, pbonzini
[-- Attachment #1: Type: text/plain, Size: 897 bytes --]
On 05/26/2017 06:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move to modern errp scheme from just LOGging errors.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> Fixes:
>
> - local_err initialized to NULL
> - 083 iotest ajusted
>
> \I feel like an idiot...
>
We all have days like that ;)
> +++ b/tests/qemu-iotests/083.out
> @@ -69,10 +69,12 @@ read failed: Input/output error
>
> === Check disconnect 4 reply ===
>
> +read failed
> read failed: Input/output error
Having a double error report is awkward; can we figure out where the
duplication is coming from and trim it down to a single report?
But that can be done as a followup.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PULL 15/20] exec: simplify phys_page_find() params
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (13 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 14/20] nbd/client.c: use errp instead of LOG Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 16/20] virtio-scsi: Unset hotplug handler when unrealize Paolo Bonzini
` (6 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
From: Peter Xu <peterx@redhat.com>
It really only plays with the dispatchers, so the parameter list does
not need that complexity. This helps for readability at least.
Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <1494838260-30439-2-git-send-email-peterx@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
exec.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/exec.c b/exec.c
index 96e3ac9..29633cd 100644
--- a/exec.c
+++ b/exec.c
@@ -373,10 +373,11 @@ static inline bool section_covers_addr(const MemoryRegionSection *section,
int128_getlo(section->size), addr);
}
-static MemoryRegionSection *phys_page_find(PhysPageEntry lp, hwaddr addr,
- Node *nodes, MemoryRegionSection *sections)
+static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr addr)
{
- PhysPageEntry *p;
+ PhysPageEntry lp = d->phys_map, *p;
+ Node *nodes = d->map.nodes;
+ MemoryRegionSection *sections = d->map.sections;
hwaddr index = addr >> TARGET_PAGE_BITS;
int i;
@@ -414,8 +415,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
section_covers_addr(section, addr)) {
update = false;
} else {
- section = phys_page_find(d->phys_map, addr, d->map.nodes,
- d->map.sections);
+ section = phys_page_find(d, addr);
update = true;
}
if (resolve_subpage && section->mr->subpage) {
@@ -1283,8 +1283,7 @@ static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *secti
subpage_t *subpage;
hwaddr base = section->offset_within_address_space
& TARGET_PAGE_MASK;
- MemoryRegionSection *existing = phys_page_find(d->phys_map, base,
- d->map.nodes, d->map.sections);
+ MemoryRegionSection *existing = phys_page_find(d, base);
MemoryRegionSection subsection = {
.offset_within_address_space = base,
.size = int128_make64(TARGET_PAGE_SIZE),
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 16/20] virtio-scsi: Unset hotplug handler when unrealize
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (14 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 15/20] exec: simplify phys_page_find() params Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 17/20] vhost-user-scsi: Introduce vhost-user-scsi host device Paolo Bonzini
` (5 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Fam Zheng, qemu-stable
From: Fam Zheng <famz@redhat.com>
This matches the qbus_set_hotplug_handler in realize, and it releases
the final reference to the embedded VirtIODevice so that it is
properly finalized.
A use-after-free is fixed with this patch, indirectly:
virtio_device_instance_finalize wasn't called at hot-unplug, and the
vdev->listener would be a dangling pointer in the global and the per
address space listener list. See also RHBZ 1449031.
Cc: qemu-stable@nongnu.org
Signed-off-by: Fam Zheng <famz@redhat.com>
Message-Id: <20170518102808.30046-1-famz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/virtio-scsi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 46a3e3f..f46f06d 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -918,6 +918,9 @@ void virtio_scsi_common_unrealize(DeviceState *dev, Error **errp)
static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp)
{
+ VirtIOSCSI *s = VIRTIO_SCSI(dev);
+
+ qbus_set_hotplug_handler(BUS(&s->bus), NULL, &error_abort);
virtio_scsi_common_unrealize(dev, errp);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 17/20] vhost-user-scsi: Introduce vhost-user-scsi host device
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (15 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 16/20] virtio-scsi: Unset hotplug handler when unrealize Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 18/20] vhost-user-scsi: Introduce a vhost-user-scsi sample application Paolo Bonzini
` (4 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Felipe Franciosi
From: Felipe Franciosi <felipe@nutanix.com>
This commit introduces a vhost-user device for SCSI. This is based
on the existing vhost-scsi implementation, but done over vhost-user
instead. It also uses a chardev to connect to the backend. Unlike
vhost-scsi (today), VMs using vhost-user-scsi can be live migrated.
To use it, start Qemu with a command line equivalent to:
qemu-system-x86_64 \
-chardev socket,id=vus0,path=/tmp/vus.sock \
-device vhost-user-scsi-pci,chardev=vus0,bus=pci.0,addr=...
A separate commit presents a sample application linked with libiscsi to
provide a backend for vhost-user-scsi.
Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
Message-Id: <1488479153-21203-4-git-send-email-felipe@nutanix.com>
---
.gitignore | 1 +
default-configs/pci.mak | 1 +
default-configs/s390x-softmmu.mak | 1 +
hw/scsi/Makefile.objs | 1 +
hw/scsi/vhost-user-scsi.c | 215 ++++++++++++++++++++++++++++++++++++
hw/virtio/virtio-pci.c | 54 +++++++++
hw/virtio/virtio-pci.h | 11 ++
include/hw/virtio/vhost-user-scsi.h | 35 ++++++
include/hw/virtio/virtio-scsi.h | 3 +
9 files changed, 322 insertions(+)
create mode 100644 hw/scsi/vhost-user-scsi.c
create mode 100644 include/hw/virtio/vhost-user-scsi.h
diff --git a/.gitignore b/.gitignore
index 55a001e..fa96bd2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -50,6 +50,7 @@
/qemu-version.h.tmp
/module_block.h
/vscclient
+/vhost-user-scsi
/fsdev/virtfs-proxy-helper
*.[1-9]
*.a
diff --git a/default-configs/pci.mak b/default-configs/pci.mak
index 60dc651..ada9c6f 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -42,3 +42,4 @@ CONFIG_VGA=y
CONFIG_VGA_PCI=y
CONFIG_IVSHMEM=$(CONFIG_EVENTFD)
CONFIG_ROCKER=y
+CONFIG_VHOST_USER_SCSI=$(CONFIG_POSIX)
diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
index 9615a48..9a0b6d9 100644
--- a/default-configs/s390x-softmmu.mak
+++ b/default-configs/s390x-softmmu.mak
@@ -1,5 +1,6 @@
CONFIG_PCI=y
CONFIG_VIRTIO_PCI=y
+CONFIG_VHOST_USER_SCSI=y
CONFIG_VIRTIO=y
CONFIG_SCLPCONSOLE=y
CONFIG_TERMINAL3270=y
diff --git a/hw/scsi/Makefile.objs b/hw/scsi/Makefile.objs
index 54d8754..b188f72 100644
--- a/hw/scsi/Makefile.objs
+++ b/hw/scsi/Makefile.objs
@@ -11,4 +11,5 @@ obj-$(CONFIG_PSERIES) += spapr_vscsi.o
ifeq ($(CONFIG_VIRTIO),y)
obj-y += virtio-scsi.o virtio-scsi-dataplane.o
obj-$(CONFIG_VHOST_SCSI) += vhost-scsi-common.o vhost-scsi.o
+obj-$(CONFIG_VHOST_USER_SCSI) += vhost-scsi-common.o vhost-user-scsi.o
endif
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
new file mode 100644
index 0000000..694a637
--- /dev/null
+++ b/hw/scsi/vhost-user-scsi.c
@@ -0,0 +1,215 @@
+/*
+ * vhost-user-scsi host device
+ *
+ * Copyright (c) 2016 Nutanix Inc. All rights reserved.
+ *
+ * Author:
+ * Felipe Franciosi <felipe@nutanix.com>
+ *
+ * This work is largely based on the "vhost-scsi" implementation by:
+ * Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
+ * Nicholas Bellinger <nab@risingtidesystems.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/typedefs.h"
+#include "qom/object.h"
+#include "hw/fw-path-provider.h"
+#include "hw/qdev-core.h"
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-backend.h"
+#include "hw/virtio/vhost-user-scsi.h"
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-access.h"
+#include "sysemu/char.h"
+
+/* Features supported by the host application */
+static const int user_feature_bits[] = {
+ VIRTIO_F_NOTIFY_ON_EMPTY,
+ VIRTIO_RING_F_INDIRECT_DESC,
+ VIRTIO_RING_F_EVENT_IDX,
+ VIRTIO_SCSI_F_HOTPLUG,
+ VHOST_INVALID_FEATURE_BIT
+};
+
+static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
+{
+ VHostUserSCSI *s = (VHostUserSCSI *)vdev;
+ VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+ bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running;
+
+ if (vsc->dev.started == start) {
+ return;
+ }
+
+ if (start) {
+ int ret;
+
+ ret = vhost_scsi_common_start(vsc);
+ if (ret < 0) {
+ error_report("unable to start vhost-user-scsi: %s", strerror(-ret));
+ exit(1);
+ }
+ } else {
+ vhost_scsi_common_stop(vsc);
+ }
+}
+
+static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+}
+
+static void vhost_user_scsi_save(QEMUFile *f, void *opaque)
+{
+ VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
+ virtio_save(vdev, f);
+}
+
+static int vhost_user_scsi_load(QEMUFile *f, void *opaque, int version_id)
+{
+ VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
+ return virtio_load(vdev, f, version_id);
+}
+
+static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
+{
+ VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
+ VHostUserSCSI *s = VHOST_USER_SCSI(dev);
+ VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+ static int vhost_user_scsi_id;
+ Error *err = NULL;
+ int ret;
+
+ if (!vs->conf.chardev.chr) {
+ error_setg(errp, "vhost-user-scsi: missing chardev");
+ return;
+ }
+
+ virtio_scsi_common_realize(dev, vhost_dummy_handle_output,
+ vhost_dummy_handle_output,
+ vhost_dummy_handle_output, &err);
+ if (err != NULL) {
+ error_propagate(errp, err);
+ return;
+ }
+
+ vsc->dev.nvqs = 2 + vs->conf.num_queues;
+ vsc->dev.vqs = g_new(struct vhost_virtqueue, vsc->dev.nvqs);
+ vsc->dev.vq_index = 0;
+ vsc->dev.backend_features = 0;
+
+ ret = vhost_dev_init(&vsc->dev, (void *)&vs->conf.chardev,
+ VHOST_BACKEND_TYPE_USER, 0);
+ if (ret < 0) {
+ error_setg(errp, "vhost-user-scsi: vhost initialization failed: %s",
+ strerror(-ret));
+ return;
+ }
+
+ /* Channel and lun both are 0 for bootable vhost-user-scsi disk */
+ vsc->channel = 0;
+ vsc->lun = 0;
+ vsc->target = vs->conf.boot_tpgt;
+
+ register_savevm(dev, "vhost-user-scsi", vhost_user_scsi_id++, 1,
+ vhost_user_scsi_save, vhost_user_scsi_load, s);
+}
+
+static void vhost_user_scsi_unrealize(DeviceState *dev, Error **errp)
+{
+ VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+ VHostUserSCSI *s = VHOST_USER_SCSI(dev);
+ VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+
+ /* This will stop the vhost backend. */
+ vhost_user_scsi_set_status(vdev, 0);
+
+ vhost_dev_cleanup(&vsc->dev);
+ g_free(vsc->dev.vqs);
+
+ virtio_scsi_common_unrealize(dev, errp);
+}
+
+static uint64_t vhost_user_scsi_get_features(VirtIODevice *vdev,
+ uint64_t features, Error **errp)
+{
+ VHostUserSCSI *s = VHOST_USER_SCSI(vdev);
+
+ // Turn on predefined features supported by this device
+ features |= s->host_features;
+
+ return vhost_scsi_common_get_features(vdev, features, errp);
+}
+
+static Property vhost_user_scsi_properties[] = {
+ DEFINE_PROP_CHR("chardev", VirtIOSCSICommon, conf.chardev),
+ DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0),
+ DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1),
+ DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
+ 0xFFFF),
+ DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon, conf.cmd_per_lun, 128),
+ DEFINE_PROP_BIT64("hotplug", VHostUserSCSI, host_features,
+ VIRTIO_SCSI_F_HOTPLUG,
+ true),
+ DEFINE_PROP_BIT64("param_change", VHostUserSCSI, host_features,
+ VIRTIO_SCSI_F_CHANGE,
+ true),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_scsi_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+ FWPathProviderClass *fwc = FW_PATH_PROVIDER_CLASS(klass);
+
+ dc->props = vhost_user_scsi_properties;
+ set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+ vdc->realize = vhost_user_scsi_realize;
+ vdc->unrealize = vhost_user_scsi_unrealize;
+ vdc->get_features = vhost_user_scsi_get_features;
+ vdc->set_config = vhost_scsi_common_set_config;
+ vdc->set_status = vhost_user_scsi_set_status;
+ fwc->get_dev_path = vhost_scsi_common_get_fw_dev_path;
+}
+
+static void vhost_user_scsi_instance_init(Object *obj)
+{
+ VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(obj);
+ VHostSCSICommon *vsc = VHOST_SCSI_COMMON(obj);
+
+ vsc->feature_bits = user_feature_bits;
+
+ // Add the bootindex property for this object
+ device_add_bootindex_property(obj, &vsc->bootindex, "bootindex", NULL,
+ DEVICE(vsc), NULL);
+
+ // Set boot index according the the device config
+ object_property_set_int(obj, vs->conf.bootindex, "bootindex", NULL);
+}
+
+static const TypeInfo vhost_user_scsi_info = {
+ .name = TYPE_VHOST_USER_SCSI,
+ .parent = TYPE_VHOST_SCSI_COMMON,
+ .instance_size = sizeof(VHostUserSCSI),
+ .class_init = vhost_user_scsi_class_init,
+ .instance_init = vhost_user_scsi_instance_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_FW_PATH_PROVIDER },
+ { }
+ },
+};
+
+static void virtio_register_types(void)
+{
+ type_register_static(&vhost_user_scsi_info);
+}
+
+type_init(virtio_register_types)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f9b7244..d52faeb 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2135,6 +2135,59 @@ static const TypeInfo vhost_scsi_pci_info = {
};
#endif
+/* vhost-user-scsi-pci */
+static Property vhost_user_scsi_pci_properties[] = {
+ DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
+ DEV_NVECTORS_UNSPECIFIED),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+ VHostUserSCSIPCI *dev = VHOST_USER_SCSI_PCI(vpci_dev);
+ DeviceState *vdev = DEVICE(&dev->vdev);
+ VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
+
+ if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
+ vpci_dev->nvectors = vs->conf.num_queues + 3;
+ }
+
+ qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
+ object_property_set_bool(OBJECT(vdev), true, "realized", errp);
+}
+
+static void vhost_user_scsi_pci_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+ PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+ k->realize = vhost_user_scsi_pci_realize;
+ set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+ dc->props = vhost_user_scsi_pci_properties;
+ pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+ pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_SCSI;
+ pcidev_k->revision = 0x00;
+ pcidev_k->class_id = PCI_CLASS_STORAGE_SCSI;
+}
+
+static void vhost_user_scsi_pci_instance_init(Object *obj)
+{
+ VHostUserSCSIPCI *dev = VHOST_USER_SCSI_PCI(obj);
+
+ virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+ TYPE_VHOST_USER_SCSI);
+ object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
+ "bootindex", &error_abort);
+}
+
+static const TypeInfo vhost_user_scsi_pci_info = {
+ .name = TYPE_VHOST_USER_SCSI_PCI,
+ .parent = TYPE_VIRTIO_PCI,
+ .instance_size = sizeof(VHostUserSCSIPCI),
+ .instance_init = vhost_user_scsi_pci_instance_init,
+ .class_init = vhost_user_scsi_pci_class_init,
+};
+
/* vhost-vsock-pci */
#ifdef CONFIG_VHOST_VSOCK
@@ -2612,6 +2665,7 @@ static void virtio_pci_register_types(void)
#ifdef CONFIG_VHOST_SCSI
type_register_static(&vhost_scsi_pci_info);
#endif
+ type_register_static(&vhost_user_scsi_pci_info);
#ifdef CONFIG_VHOST_VSOCK
type_register_static(&vhost_vsock_pci_info);
#endif
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index b095dfc..69f5959 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -26,6 +26,7 @@
#include "hw/virtio/virtio-input.h"
#include "hw/virtio/virtio-gpu.h"
#include "hw/virtio/virtio-crypto.h"
+#include "hw/virtio/vhost-user-scsi.h"
#ifdef CONFIG_VIRTFS
#include "hw/9pfs/virtio-9p.h"
@@ -44,6 +45,7 @@ typedef struct VirtIOBalloonPCI VirtIOBalloonPCI;
typedef struct VirtIOSerialPCI VirtIOSerialPCI;
typedef struct VirtIONetPCI VirtIONetPCI;
typedef struct VHostSCSIPCI VHostSCSIPCI;
+typedef struct VHostUserSCSIPCI VHostUserSCSIPCI;
typedef struct VirtIORngPCI VirtIORngPCI;
typedef struct VirtIOInputPCI VirtIOInputPCI;
typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
@@ -230,6 +232,15 @@ struct VHostSCSIPCI {
};
#endif
+#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci"
+#define VHOST_USER_SCSI_PCI(obj) \
+ OBJECT_CHECK(VHostUserSCSIPCI, (obj), TYPE_VHOST_USER_SCSI_PCI)
+
+struct VHostUserSCSIPCI {
+ VirtIOPCIProxy parent_obj;
+ VHostUserSCSI vdev;
+};
+
/*
* virtio-blk-pci: This extends VirtioPCIProxy.
*/
diff --git a/include/hw/virtio/vhost-user-scsi.h b/include/hw/virtio/vhost-user-scsi.h
new file mode 100644
index 0000000..01861f7
--- /dev/null
+++ b/include/hw/virtio/vhost-user-scsi.h
@@ -0,0 +1,35 @@
+/*
+ * vhost-user-scsi host device
+ *
+ * Copyright (c) 2016 Nutanix Inc. All rights reserved.
+ *
+ * Author:
+ * Felipe Franciosi <felipe@nutanix.com>
+ *
+ * This file is largely based on "vhost-scsi.h" by:
+ * Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef VHOST_USER_SCSI_H
+#define VHOST_USER_SCSI_H
+
+#include "qemu-common.h"
+#include "hw/qdev.h"
+#include "hw/virtio/virtio-scsi.h"
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-scsi-common.h"
+
+#define TYPE_VHOST_USER_SCSI "vhost-user-scsi"
+#define VHOST_USER_SCSI(obj) \
+ OBJECT_CHECK(VHostUserSCSI, (obj), TYPE_VHOST_USER_SCSI)
+
+typedef struct VHostUserSCSI {
+ VHostSCSICommon parent_obj;
+ uint64_t host_features;
+} VHostUserSCSI;
+
+#endif /* VHOST_USER_SCSI_H */
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index eac2013..3827e95 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -21,6 +21,7 @@
#include "hw/virtio/virtio.h"
#include "hw/pci/pci.h"
#include "hw/scsi/scsi.h"
+#include "sysemu/char.h"
#include "sysemu/iothread.h"
#define TYPE_VIRTIO_SCSI_COMMON "virtio-scsi-common"
@@ -53,6 +54,8 @@ struct VirtIOSCSIConf {
char *vhostfd;
char *wwpn;
#endif
+ CharBackend chardev;
+ int32_t bootindex;
uint32_t boot_tpgt;
IOThread *iothread;
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 18/20] vhost-user-scsi: Introduce a vhost-user-scsi sample application
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (16 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 17/20] vhost-user-scsi: Introduce vhost-user-scsi host device Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 19/20] target/i386: enable A20 automatically in system management mode Paolo Bonzini
` (3 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Felipe Franciosi
From: Felipe Franciosi <felipe@nutanix.com>
This commit introduces a vhost-user-scsi backend sample application. It
must be linked with libiscsi and libvhost-user.
To use it, compile with:
$ make vhost-user-scsi
And run as follows:
$ ./vhost-user-scsi -u vus.sock -i iscsi://uri_to_target/
$ qemu-system-x86_64 --enable-kvm -m 512 \
-object memory-backend-file,id=mem,size=512m,share=on,mem-path=guestmem \
-numa node,memdev=mem \
-chardev socket,id=vhost-user-scsi,path=vus.sock \
-device vhost-user-scsi-pci,chardev=vhost-user-scsi \
The application is currently limited at one LUN only and it processes
requests synchronously (therefore only achieving QD1). The purpose of
the code is to show how a backend can be implemented and to test the
vhost-user-scsi Qemu implementation.
If a different instance of this vhost-user-scsi application is executed
at a remote host, a VM can be live migrated to such a host.
Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
Message-Id: <1488479153-21203-5-git-send-email-felipe@nutanix.com>
---
Makefile | 3 +
Makefile.objs | 4 +
contrib/vhost-user-scsi/Makefile.objs | 1 +
contrib/vhost-user-scsi/vhost-user-scsi.c | 886 ++++++++++++++++++++++++++++++
4 files changed, 894 insertions(+)
create mode 100644 contrib/vhost-user-scsi/Makefile.objs
create mode 100644 contrib/vhost-user-scsi/vhost-user-scsi.c
diff --git a/Makefile b/Makefile
index c830d7a..e14988d 100644
--- a/Makefile
+++ b/Makefile
@@ -269,6 +269,7 @@ dummy := $(call unnest-vars,, \
ivshmem-client-obj-y \
ivshmem-server-obj-y \
libvhost-user-obj-y \
+ vhost-user-scsi-obj-y \
qga-vss-dll-obj-y \
block-obj-y \
block-obj-m \
@@ -473,6 +474,8 @@ ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) $(COMMON_LDADDS)
$(call LINK, $^)
ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) $(COMMON_LDADDS)
$(call LINK, $^)
+vhost-user-scsi$(EXESUF): $(vhost-user-scsi-obj-y)
+ $(call LINK, $^)
module_block.h: $(SRC_PATH)/scripts/modules/module_block.py config-host.mak
$(call quiet-command,$(PYTHON) $< $@ \
diff --git a/Makefile.objs b/Makefile.objs
index 2100845..1fa9450 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -112,6 +112,10 @@ qga-vss-dll-obj-y = qga/
ivshmem-client-obj-y = contrib/ivshmem-client/
ivshmem-server-obj-y = contrib/ivshmem-server/
libvhost-user-obj-y = contrib/libvhost-user/
+vhost-user-scsi.o-cflags := $(LIBISCSI_CFLAGS)
+vhost-user-scsi.o-libs := $(LIBISCSI_LIBS)
+vhost-user-scsi-obj-y = contrib/vhost-user-scsi/
+vhost-user-scsi-obj-y += contrib/libvhost-user/libvhost-user.o
######################################################################
trace-events-subdirs =
diff --git a/contrib/vhost-user-scsi/Makefile.objs b/contrib/vhost-user-scsi/Makefile.objs
new file mode 100644
index 0000000..e83a38a
--- /dev/null
+++ b/contrib/vhost-user-scsi/Makefile.objs
@@ -0,0 +1 @@
+vhost-user-scsi-obj-y = vhost-user-scsi.o
diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c b/contrib/vhost-user-scsi/vhost-user-scsi.c
new file mode 100644
index 0000000..e41bad0
--- /dev/null
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -0,0 +1,886 @@
+/*
+ * vhost-user-scsi sample application
+ *
+ * Copyright (c) 2016 Nutanix Inc. All rights reserved.
+ *
+ * Author:
+ * Felipe Franciosi <felipe@nutanix.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 only.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "contrib/libvhost-user/libvhost-user.h"
+#include "hw/virtio/virtio-scsi.h"
+#include "iscsi/iscsi.h"
+
+#include <glib.h>
+
+/* Small compat shim from glib 2.32 */
+#ifndef G_SOURCE_CONTINUE
+#define G_SOURCE_CONTINUE TRUE
+#endif
+#ifndef G_SOURCE_REMOVE
+#define G_SOURCE_REMOVE FALSE
+#endif
+
+//#define VUS_DEBUG 1
+
+/** Log helpers **/
+
+#define PPRE \
+ struct timespec ts; \
+ char timebuf[64]; \
+ struct tm tm; \
+ (void)clock_gettime(CLOCK_REALTIME, &ts); \
+ (void)strftime(timebuf, 64, "%Y%m%d %T", gmtime_r(&ts.tv_sec, &tm))
+
+#define PEXT(lvl, msg, ...) do { \
+ PPRE; \
+ fprintf(stderr, "%s.%06ld " lvl ": %s:%s():%d: " msg "\n", \
+ timebuf, ts.tv_nsec/1000, \
+ __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
+} while(0)
+
+#define PNOR(lvl, msg, ...) do { \
+ PPRE; \
+ fprintf(stderr, "%s.%06ld " lvl ": " msg "\n", \
+ timebuf, ts.tv_nsec/1000, ## __VA_ARGS__); \
+} while(0);
+
+#ifdef VUS_DEBUG
+#define PDBG(msg, ...) PEXT("DBG", msg, ## __VA_ARGS__)
+#define PERR(msg, ...) PEXT("ERR", msg, ## __VA_ARGS__)
+#define PLOG(msg, ...) PEXT("LOG", msg, ## __VA_ARGS__)
+#else
+#define PDBG(msg, ...) { }
+#define PERR(msg, ...) PNOR("ERR", msg, ## __VA_ARGS__)
+#define PLOG(msg, ...) PNOR("LOG", msg, ## __VA_ARGS__)
+#endif
+
+/** vhost-user-scsi specific definitions **/
+
+#define VUS_MAX_LUNS 1 // Only 1 lun supported today
+#define VUS_MAX_DEVS 1 // Only 1 devices supported today
+
+#define VUS_ISCSI_INITIATOR "iqn.2016-11.com.nutanix:vhost-user-scsi"
+
+typedef struct iscsi_lun {
+ struct iscsi_context *iscsi_ctx;
+ int iscsi_lun;
+} iscsi_lun_t;
+
+typedef struct vhost_scsi_dev {
+ VuDev vu_dev;
+ int server_sock;
+ GMainLoop *loop;
+ GTree *fdmap; // maps fd to gsource context id
+ iscsi_lun_t luns[VUS_MAX_LUNS];
+} vhost_scsi_dev_t;
+
+static vhost_scsi_dev_t *vhost_scsi_devs[VUS_MAX_DEVS];
+
+/** glib event loop integration for libvhost-user and misc callbacks **/
+
+typedef struct vus_gsrc {
+ GSource parent;
+ vhost_scsi_dev_t *vdev_scsi;
+ GPollFD gfd;
+ vu_watch_cb vu_cb;
+} vus_gsrc_t;
+
+static gint vus_fdmap_compare(gconstpointer a, gconstpointer b)
+{
+ if (b > a) return 1;
+ if (b < a) return -1;
+ return 0;
+}
+
+static gboolean vus_gsrc_prepare(GSource *src, gint *timeout)
+{
+ assert(timeout);
+
+ *timeout = -1;
+ return FALSE;
+}
+
+static gboolean vus_gsrc_check(GSource *src)
+{
+ vus_gsrc_t *vus_src = (vus_gsrc_t *)src;
+
+ assert(vus_src);
+
+ return vus_src->gfd.revents & vus_src->gfd.events;
+}
+
+static gboolean vus_gsrc_dispatch(GSource *src, GSourceFunc cb, gpointer data)
+{
+ vhost_scsi_dev_t *vdev_scsi;
+ vus_gsrc_t *vus_src = (vus_gsrc_t *)src;
+
+ assert(vus_src);
+ assert(!(vus_src->vu_cb && cb));
+
+ vdev_scsi = vus_src->vdev_scsi;
+
+ assert(vdev_scsi);
+
+ if (vus_src->vu_cb) {
+ int vu_evt = 0;
+
+ if (vus_src->gfd.events & G_IO_IN) vu_evt |= VU_WATCH_IN;
+ if (vus_src->gfd.events & G_IO_OUT) vu_evt |= VU_WATCH_OUT;
+ if (vus_src->gfd.events & G_IO_PRI) vu_evt |= VU_WATCH_PRI;
+ if (vus_src->gfd.events & G_IO_ERR) vu_evt |= VU_WATCH_ERR;
+ if (vus_src->gfd.events & G_IO_HUP) vu_evt |= VU_WATCH_HUP;
+
+ vus_src->vu_cb(&vdev_scsi->vu_dev, vu_evt, data);
+
+ return G_SOURCE_CONTINUE;
+ }
+
+ if (cb) {
+ return cb(data);
+ }
+
+ return G_SOURCE_CONTINUE;
+}
+
+static GSourceFuncs vus_gsrc_funcs = {
+ vus_gsrc_prepare,
+ vus_gsrc_check,
+ vus_gsrc_dispatch,
+ NULL
+};
+
+static int vus_gsrc_new(vhost_scsi_dev_t *vdev_scsi, int fd, GIOCondition cond,
+ vu_watch_cb vu_cb, GSourceFunc gsrc_cb, gpointer data)
+{
+ GSource *vus_gsrc;
+ vus_gsrc_t *vus_src;
+ guint id;
+
+ assert(vdev_scsi);
+ assert(fd >= 0);
+ assert(vu_cb || gsrc_cb);
+ assert(!(vu_cb && gsrc_cb));
+
+ vus_gsrc = g_source_new(&vus_gsrc_funcs, sizeof(vus_gsrc_t));
+ if (!vus_gsrc) {
+ PERR("Error creating GSource for new watch");
+ return -1;
+ }
+ vus_src = (vus_gsrc_t *)vus_gsrc;
+
+ vus_src->vdev_scsi = vdev_scsi;
+ vus_src->gfd.fd = fd;
+ vus_src->gfd.events = cond;
+ vus_src->vu_cb = vu_cb;
+
+ g_source_add_poll(vus_gsrc, &vus_src->gfd);
+ g_source_set_callback(vus_gsrc, gsrc_cb, data, NULL);
+ id = g_source_attach(vus_gsrc, NULL);
+ assert(id);
+ g_source_unref(vus_gsrc);
+
+ g_tree_insert(vdev_scsi->fdmap, (gpointer)(uintptr_t)fd,
+ (gpointer)(uintptr_t)id);
+
+ return 0;
+}
+
+/* from libiscsi's scsi-lowlevel.h **
+ *
+ * nb. We can't directly include scsi-lowlevel.h due to a namespace conflict:
+ * Qemu's scsi.h also defines "SCSI_XFER_NONE".
+ */
+
+#define SCSI_CDB_MAX_SIZE 16
+
+struct scsi_iovector {
+ struct scsi_iovec *iov;
+ int niov;
+ int nalloc;
+ size_t offset;
+ int consumed;
+};
+
+struct scsi_allocated_memory {
+ struct scsi_allocated_memory *next;
+ char buf[0];
+};
+
+struct scsi_data {
+ int size;
+ unsigned char *data;
+};
+
+enum scsi_sense_key {
+ SCSI_SENSE_NO_SENSE = 0x00,
+ SCSI_SENSE_RECOVERED_ERROR = 0x01,
+ SCSI_SENSE_NOT_READY = 0x02,
+ SCSI_SENSE_MEDIUM_ERROR = 0x03,
+ SCSI_SENSE_HARDWARE_ERROR = 0x04,
+ SCSI_SENSE_ILLEGAL_REQUEST = 0x05,
+ SCSI_SENSE_UNIT_ATTENTION = 0x06,
+ SCSI_SENSE_DATA_PROTECTION = 0x07,
+ SCSI_SENSE_BLANK_CHECK = 0x08,
+ SCSI_SENSE_VENDOR_SPECIFIC = 0x09,
+ SCSI_SENSE_COPY_ABORTED = 0x0a,
+ SCSI_SENSE_COMMAND_ABORTED = 0x0b,
+ SCSI_SENSE_OBSOLETE_ERROR_CODE = 0x0c,
+ SCSI_SENSE_OVERFLOW_COMMAND = 0x0d,
+ SCSI_SENSE_MISCOMPARE = 0x0e
+};
+
+struct scsi_sense {
+ unsigned char error_type;
+ enum scsi_sense_key key;
+ int ascq;
+ unsigned sense_specific:1;
+ unsigned ill_param_in_cdb:1;
+ unsigned bit_pointer_valid:1;
+ unsigned char bit_pointer;
+ uint16_t field_pointer;
+};
+
+enum scsi_residual {
+ SCSI_RESIDUAL_NO_RESIDUAL = 0,
+ SCSI_RESIDUAL_UNDERFLOW,
+ SCSI_RESIDUAL_OVERFLOW
+};
+
+struct scsi_task {
+ int status;
+ int cdb_size;
+ int xfer_dir;
+ int expxferlen;
+ unsigned char cdb[SCSI_CDB_MAX_SIZE];
+ enum scsi_residual residual_status;
+ size_t residual;
+ struct scsi_sense sense;
+ struct scsi_data datain;
+ struct scsi_allocated_memory *mem;
+ void *ptr;
+
+ uint32_t itt;
+ uint32_t cmdsn;
+ uint32_t lun;
+
+ struct scsi_iovector iovector_in;
+ struct scsi_iovector iovector_out;
+};
+
+/** libiscsi integration **/
+
+static int iscsi_add_lun(iscsi_lun_t *lun, char *iscsi_uri) {
+ struct iscsi_url *iscsi_url;
+ struct iscsi_context *iscsi_ctx;
+ int ret = 0;
+
+ assert(lun);
+ assert(iscsi_uri);
+
+ iscsi_ctx = iscsi_create_context(VUS_ISCSI_INITIATOR);
+ if (!iscsi_ctx) {
+ PERR("Unable to create iSCSI context");
+ return -1;
+ }
+
+ iscsi_url = iscsi_parse_full_url(iscsi_ctx, iscsi_uri);
+ if (!iscsi_url) {
+ PERR("Unable to parse iSCSI URL: %s", iscsi_get_error(iscsi_ctx));
+ goto fail;
+ }
+
+ iscsi_set_session_type(iscsi_ctx, ISCSI_SESSION_NORMAL);
+ iscsi_set_header_digest(iscsi_ctx, ISCSI_HEADER_DIGEST_NONE_CRC32C);
+ if (iscsi_full_connect_sync(iscsi_ctx, iscsi_url->portal, iscsi_url->lun)) {
+ PERR("Unable to login to iSCSI portal: %s", iscsi_get_error(iscsi_ctx));
+ goto fail;
+ }
+
+ lun->iscsi_ctx = iscsi_ctx;
+ lun->iscsi_lun = iscsi_url->lun;
+
+ PDBG("Context %p created for lun 0: %s", iscsi_ctx, iscsi_uri);
+
+out:
+ if (iscsi_url) {
+ iscsi_destroy_url(iscsi_url);
+ }
+ return ret;
+
+fail:
+ (void)iscsi_destroy_context(iscsi_ctx);
+ ret = -1;
+ goto out;
+}
+
+static struct scsi_task *scsi_task_new(int cdb_len, uint8_t *cdb, int dir,
+ int xfer_len) {
+ struct scsi_task *task;
+
+ assert(cdb_len > 0);
+ assert(cdb);
+
+ task = calloc(1, sizeof(struct scsi_task));
+ if (!task) {
+ PERR("Error allocating task: %s", strerror(errno));
+ return NULL;
+ }
+
+ memcpy(task->cdb, cdb, cdb_len);
+ task->cdb_size = cdb_len;
+ task->xfer_dir = dir;
+ task->expxferlen = xfer_len;
+
+ return task;
+}
+
+static int get_cdb_len(uint8_t *cdb) {
+ assert(cdb);
+
+ switch (cdb[0] >> 5) {
+ case 0: return 6;
+ case 1: /* fall through */
+ case 2: return 10;
+ case 4: return 16;
+ case 5: return 12;
+ }
+ PERR("Unable to determine cdb len (0x%02hhX)", cdb[0]>>5);
+ return -1;
+}
+
+static int handle_cmd_sync(struct iscsi_context *ctx,
+ VirtIOSCSICmdReq *req,
+ struct iovec *out, unsigned int out_len,
+ VirtIOSCSICmdResp *rsp,
+ struct iovec *in, unsigned int in_len) {
+ struct scsi_task *task;
+ uint32_t dir;
+ uint32_t len;
+ int cdb_len;
+ int i;
+
+ assert(ctx);
+ assert(req);
+ assert(rsp);
+
+ if (!((!req->lun[1]) && (req->lun[2] == 0x40) && (!req->lun[3]))) {
+ // Ignore anything different than target=0, lun=0
+ PDBG("Ignoring unconnected lun (0x%hhX, 0x%hhX)",
+ req->lun[1], req->lun[3]);
+ rsp->status = SCSI_STATUS_CHECK_CONDITION;
+ memset(rsp->sense, 0, sizeof(rsp->sense));
+ rsp->sense_len = 18;
+ rsp->sense[0] = 0x70;
+ rsp->sense[2] = SCSI_SENSE_ILLEGAL_REQUEST;
+ rsp->sense[7] = 10;
+ rsp->sense[12] = 0x24;
+
+ return 0;
+ }
+
+ cdb_len = get_cdb_len(req->cdb);
+ if (cdb_len == -1) {
+ return -1;
+ }
+
+ len = 0;
+ if (!out_len && !in_len) {
+ dir = SCSI_XFER_NONE;
+ } else if (out_len) {
+ dir = SCSI_XFER_TO_DEV;
+ for (i=0; i<out_len; i++) {
+ len += out[i].iov_len;
+ }
+ } else {
+ dir = SCSI_XFER_FROM_DEV;
+ for (i=0; i<in_len; i++) {
+ len += in[i].iov_len;
+ }
+ }
+
+ task = scsi_task_new(cdb_len, req->cdb, dir, len);
+ if (!task) {
+ PERR("Unable to create iscsi task");
+ return -1;
+ }
+
+ if (dir == SCSI_XFER_TO_DEV) {
+ task->iovector_out.iov = (struct scsi_iovec *)out;
+ task->iovector_out.niov = out_len;
+ } else if (dir == SCSI_XFER_FROM_DEV) {
+ task->iovector_in.iov = (struct scsi_iovec *)in;
+ task->iovector_in.niov = in_len;
+ }
+
+ PDBG("Sending iscsi cmd (cdb_len=%d, dir=%d, task=%p)",
+ cdb_len, dir, task);
+ if (!iscsi_scsi_command_sync(ctx, 0, task, NULL)) {
+ PERR("Error serving SCSI command");
+ free(task);
+ return -1;
+ }
+
+ memset(rsp, 0, sizeof(*rsp));
+
+ rsp->status = task->status;
+ rsp->resid = task->residual;
+
+ if (task->status == SCSI_STATUS_CHECK_CONDITION) {
+ rsp->response = VIRTIO_SCSI_S_FAILURE;
+ rsp->sense_len = task->datain.size - 2;
+ memcpy(rsp->sense, &task->datain.data[2], rsp->sense_len);
+ }
+
+ free(task);
+
+ PDBG("Filled in rsp: status=%hhX, resid=%u, response=%hhX, sense_len=%u",
+ rsp->status, rsp->resid, rsp->response, rsp->sense_len);
+
+ return 0;
+}
+
+/** libvhost-user callbacks **/
+
+static vhost_scsi_dev_t *vdev_scsi_find_by_vu(VuDev *vu_dev);
+
+static void vus_panic_cb(VuDev *vu_dev, const char *buf) {
+ vhost_scsi_dev_t *vdev_scsi;
+
+ assert(vu_dev);
+
+ vdev_scsi = vdev_scsi_find_by_vu(vu_dev);
+
+ if (buf) {
+ PERR("vu_panic: %s", buf);
+ }
+
+ if (vdev_scsi) {
+ assert(vdev_scsi->loop);
+ g_main_loop_quit(vdev_scsi->loop);
+ }
+}
+
+static void vus_add_watch_cb(VuDev *vu_dev, int fd, int vu_evt, vu_watch_cb cb,
+ void *pvt) {
+ vhost_scsi_dev_t *vdev_scsi;
+ gushort conds = 0;
+ guint id;
+
+ assert(vu_dev);
+ assert(fd >= 0);
+ assert(cb);
+
+ vdev_scsi = vdev_scsi_find_by_vu(vu_dev);
+ if (!vdev_scsi) {
+ vus_panic_cb(vu_dev, NULL);
+ return;
+ }
+
+ id = (guint)(uintptr_t)g_tree_lookup(vdev_scsi->fdmap,
+ (gpointer)(uintptr_t)fd);
+ if (id) {
+ GSource *vus_src = g_main_context_find_source_by_id(NULL, id);
+ assert(vus_src);
+ g_source_destroy(vus_src);
+ (void)g_tree_remove(vdev_scsi->fdmap, (gpointer)(uintptr_t)fd);
+ }
+
+ if (vu_evt & VU_WATCH_IN) conds |= G_IO_IN;
+ if (vu_evt & VU_WATCH_OUT) conds |= G_IO_OUT;
+ if (vu_evt & VU_WATCH_PRI) conds |= G_IO_PRI;
+ if (vu_evt & VU_WATCH_ERR) conds |= G_IO_ERR;
+ if (vu_evt & VU_WATCH_HUP) conds |= G_IO_HUP;
+
+ if (vus_gsrc_new(vdev_scsi, fd, conds, cb, NULL, pvt)) {
+ vus_panic_cb(vu_dev, NULL);
+ }
+}
+
+static void vus_del_watch_cb(VuDev *vu_dev, int fd) {
+ vhost_scsi_dev_t *vdev_scsi;
+ guint id;
+
+ assert(vu_dev);
+ assert(fd >= 0);
+
+ vdev_scsi = vdev_scsi_find_by_vu(vu_dev);
+ if (!vdev_scsi) {
+ vus_panic_cb(vu_dev, NULL);
+ return;
+ }
+
+ id = (guint)(uintptr_t)g_tree_lookup(vdev_scsi->fdmap,
+ (gpointer)(uintptr_t)fd);
+ if (id) {
+ GSource *vus_src = g_main_context_find_source_by_id(NULL, id);
+ assert(vus_src);
+ g_source_destroy(vus_src);
+ (void)g_tree_remove(vdev_scsi->fdmap, (gpointer)(uintptr_t)fd);
+ }
+}
+
+static void vus_proc_ctl(VuDev *vu_dev, int idx) {
+ /* Control VQ not implemented */
+}
+
+static void vus_proc_evt(VuDev *vu_dev, int idx) {
+ /* Event VQ not implemented */
+}
+
+static void vus_proc_req(VuDev *vu_dev, int idx) {
+ vhost_scsi_dev_t *vdev_scsi;
+ VuVirtq *vq;
+
+ assert(vu_dev);
+
+ vdev_scsi = vdev_scsi_find_by_vu(vu_dev);
+ if (!vdev_scsi) {
+ vus_panic_cb(vu_dev, NULL);
+ return;
+ }
+
+ if ((idx < 0) || (idx >= VHOST_MAX_NR_VIRTQUEUE)) {
+ PERR("VQ Index out of range: %d", idx);
+ vus_panic_cb(vu_dev, NULL);
+ return;
+ }
+
+ vq = vu_get_queue(vu_dev, idx);
+ if (!vq) {
+ PERR("Error fetching VQ (dev=%p, idx=%d)", vu_dev, idx);
+ vus_panic_cb(vu_dev, NULL);
+ return;
+ }
+
+ PDBG("Got kicked on vq[%d]@%p", idx, vq);
+
+ while(1) {
+ VuVirtqElement *elem;
+ VirtIOSCSICmdReq *req;
+ VirtIOSCSICmdResp *rsp;
+
+ elem = vu_queue_pop(vu_dev, vq, sizeof(VuVirtqElement));
+ if (!elem) {
+ PDBG("No more elements pending on vq[%d]@%p", idx, vq);
+ break;
+ }
+ PDBG("Popped elem@%p", elem);
+
+ assert(!((elem->out_num > 1) && (elem->in_num > 1)));
+ assert((elem->out_num > 0) && (elem->in_num > 0));
+
+ if (elem->out_sg[0].iov_len < sizeof(VirtIOSCSICmdReq)) {
+ PERR("Invalid virtio-scsi req header");
+ vus_panic_cb(vu_dev, NULL);
+ break;
+ }
+ req = (VirtIOSCSICmdReq *)elem->out_sg[0].iov_base;
+
+ if (elem->in_sg[0].iov_len < sizeof(VirtIOSCSICmdResp)) {
+ PERR("Invalid virtio-scsi rsp header");
+ vus_panic_cb(vu_dev, NULL);
+ break;
+ }
+ rsp = (VirtIOSCSICmdResp *)elem->in_sg[0].iov_base;
+
+ if (handle_cmd_sync(vdev_scsi->luns[0].iscsi_ctx,
+ req, &elem->out_sg[1], elem->out_num-1,
+ rsp, &elem->in_sg[1], elem->in_num-1) != 0) {
+ vus_panic_cb(vu_dev, NULL);
+ break;
+ }
+
+ vu_queue_push(vu_dev, vq, elem, 0);
+ vu_queue_notify(vu_dev, vq);
+
+ free(elem);
+ }
+}
+
+static void vus_queue_set_started(VuDev *vu_dev, int idx, bool started) {
+ VuVirtq *vq;
+
+ assert(vu_dev);
+
+ if ((idx < 0) || (idx >= VHOST_MAX_NR_VIRTQUEUE)) {
+ PERR("VQ Index out of range: %d", idx);
+ vus_panic_cb(vu_dev, NULL);
+ return;
+ }
+
+ vq = vu_get_queue(vu_dev, idx);
+
+ switch(idx) {
+ case 0:
+ vu_set_queue_handler(vu_dev, vq, started?vus_proc_ctl:NULL);
+ break;
+ case 1:
+ vu_set_queue_handler(vu_dev, vq, started?vus_proc_evt:NULL);
+ break;
+ default:
+ vu_set_queue_handler(vu_dev, vq, started?vus_proc_req:NULL);
+ }
+}
+
+static const VuDevIface vus_iface = {
+ .queue_set_started = vus_queue_set_started,
+};
+
+static gboolean vus_vhost_cb(gpointer data) {
+ VuDev *vu_dev = (VuDev *)data;
+
+ assert(vu_dev);
+
+ if (!vu_dispatch(vu_dev) != 0) {
+ PERR("Error processing vhost message");
+ vus_panic_cb(vu_dev, NULL);
+ return G_SOURCE_REMOVE;
+ }
+
+ return G_SOURCE_CONTINUE;
+}
+
+/** misc helpers **/
+
+static int unix_sock_new(char *unix_fn) {
+ int sock;
+ struct sockaddr_un un;
+ size_t len;
+
+ assert(unix_fn);
+
+ sock = socket(AF_UNIX, SOCK_STREAM, 0);
+ if (sock <= 0) {
+ perror("socket");
+ return -1;
+ }
+
+ un.sun_family = AF_UNIX;
+ (void)snprintf(un.sun_path, sizeof(un.sun_path), "%s", unix_fn);
+ len = sizeof(un.sun_family) + strlen(un.sun_path);
+
+ (void)unlink(unix_fn);
+ if (bind(sock, (struct sockaddr *)&un, len) < 0) {
+ perror("bind");
+ goto fail;
+ }
+
+ if (listen(sock, 1) < 0) {
+ perror("listen");
+ goto fail;
+ }
+
+ return sock;
+
+fail:
+ (void)close(sock);
+
+ return -1;
+}
+
+/** vhost-user-scsi **/
+
+static vhost_scsi_dev_t *vdev_scsi_find_by_vu(VuDev *vu_dev) {
+ int i;
+
+ assert(vu_dev);
+
+ for (i=0; i<VUS_MAX_DEVS; i++) {
+ if (&vhost_scsi_devs[i]->vu_dev == vu_dev) {
+ return vhost_scsi_devs[i];
+ }
+ }
+
+ PERR("Unknown VuDev %p", vu_dev);
+ return NULL;
+}
+
+static void vdev_scsi_deinit(vhost_scsi_dev_t *vdev_scsi) {
+ if (!vdev_scsi) {
+ return;
+ }
+
+ if (vdev_scsi->server_sock >= 0) {
+ struct sockaddr_storage ss;
+ socklen_t sslen = sizeof(ss);
+
+ if (getsockname(vdev_scsi->server_sock, (struct sockaddr *)&ss,
+ &sslen) == 0) {
+ struct sockaddr_un *su = (struct sockaddr_un *)&ss;
+ (void)unlink(su->sun_path);
+ }
+
+ (void)close(vdev_scsi->server_sock);
+ vdev_scsi->server_sock = -1;
+ }
+
+ if (vdev_scsi->loop) {
+ g_main_loop_unref(vdev_scsi->loop);
+ vdev_scsi->loop = NULL;
+ }
+}
+
+static vhost_scsi_dev_t *vdev_scsi_new(char *unix_fn) {
+ vhost_scsi_dev_t *vdev_scsi = NULL;
+
+ assert(unix_fn);
+
+ vdev_scsi = calloc(1, sizeof(vhost_scsi_dev_t));
+ if (!vdev_scsi) {
+ PERR("calloc: %s", strerror(errno));
+ return NULL;
+ }
+
+ vdev_scsi->server_sock = unix_sock_new(unix_fn);
+ if (vdev_scsi->server_sock < 0) {
+ goto err;
+ }
+
+ vdev_scsi->loop = g_main_loop_new(NULL, FALSE);
+ if (!vdev_scsi->loop) {
+ PERR("Error creating glib event loop");
+ goto err;
+ }
+
+ vdev_scsi->fdmap = g_tree_new(vus_fdmap_compare);
+ if (!vdev_scsi->fdmap) {
+ PERR("Error creating glib tree for fdmap");
+ goto err;
+ }
+
+ return vdev_scsi;
+
+err:
+ vdev_scsi_deinit(vdev_scsi);
+ free(vdev_scsi);
+
+ return NULL;
+}
+
+static int vdev_scsi_add_iscsi_lun(vhost_scsi_dev_t *vdev_scsi,
+ char *iscsi_uri, uint32_t lun) {
+ assert(vdev_scsi);
+ assert(iscsi_uri);
+ assert(lun < VUS_MAX_LUNS);
+
+ if (vdev_scsi->luns[lun].iscsi_ctx) {
+ PERR("Lun %d already configured", lun);
+ return -1;
+ }
+
+ if (iscsi_add_lun(&vdev_scsi->luns[lun], iscsi_uri) != 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
+static int vdev_scsi_run(vhost_scsi_dev_t *vdev_scsi) {
+ int cli_sock;
+ int ret = 0;
+
+ assert(vdev_scsi);
+ assert(vdev_scsi->server_sock >= 0);
+ assert(vdev_scsi->loop);
+
+ cli_sock = accept(vdev_scsi->server_sock, (void *)0, (void *)0);
+ if (cli_sock < 0) {
+ perror("accept");
+ return -1;
+ }
+
+ vu_init(&vdev_scsi->vu_dev,
+ cli_sock,
+ vus_panic_cb,
+ vus_add_watch_cb,
+ vus_del_watch_cb,
+ &vus_iface);
+
+ if (vus_gsrc_new(vdev_scsi, cli_sock, G_IO_IN, NULL, vus_vhost_cb,
+ &vdev_scsi->vu_dev)) {
+ goto fail;
+ }
+
+ g_main_loop_run(vdev_scsi->loop);
+
+out:
+ vu_deinit(&vdev_scsi->vu_dev);
+
+ return ret;
+
+fail:
+ ret = -1;
+ goto out;
+}
+
+int main(int argc, char **argv)
+{
+ vhost_scsi_dev_t *vdev_scsi = NULL;
+ char *unix_fn = NULL;
+ char *iscsi_uri = NULL;
+ int opt, err = EXIT_SUCCESS;
+
+ while ((opt = getopt(argc, argv, "u:i:")) != -1) {
+ switch (opt) {
+ case 'h':
+ goto help;
+ case 'u':
+ unix_fn = strdup(optarg);
+ break;
+ case 'i':
+ iscsi_uri = strdup(optarg);
+ break;
+ default:
+ goto help;
+ }
+ }
+ if (!unix_fn || !iscsi_uri) {
+ goto help;
+ }
+
+ vdev_scsi = vdev_scsi_new(unix_fn);
+ if (!vdev_scsi) {
+ goto err;
+ }
+ vhost_scsi_devs[0] = vdev_scsi;
+
+ if (vdev_scsi_add_iscsi_lun(vdev_scsi, iscsi_uri, 0) != 0) {
+ goto err;
+ }
+
+ if (vdev_scsi_run(vdev_scsi) != 0) {
+ goto err;
+ }
+
+out:
+ if (vdev_scsi) {
+ vdev_scsi_deinit(vdev_scsi);
+ free(vdev_scsi);
+ }
+ if (unix_fn) {
+ free(unix_fn);
+ }
+ if (iscsi_uri) {
+ free(iscsi_uri);
+ }
+
+ return err;
+
+err:
+ err = EXIT_FAILURE;
+ goto out;
+
+help:
+ fprintf(stderr, "Usage: %s [ -u unix_sock_path -i iscsi_uri ] | [ -h ]\n",
+ argv[0]);
+ fprintf(stderr, " -u path to unix socket\n");
+ fprintf(stderr, " -i iscsi uri for lun 0\n");
+ fprintf(stderr, " -h print help and quit\n");
+
+ goto err;
+}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 19/20] target/i386: enable A20 automatically in system management mode
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (17 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 18/20] vhost-user-scsi: Introduce a vhost-user-scsi sample application Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 11:21 ` [Qemu-devel] [PULL 20/20] target/i386: use multiple CPU AddressSpaces Paolo Bonzini
` (2 subsequent siblings)
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel
Ignore env->a20_mask when running in system management mode.
Reported-by: Anthony Xu <anthony.xu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <1494502528-12670-1-git-send-email-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/arch_memory_mapping.c | 18 +++++++++--------
target/i386/cpu.h | 9 +++++++++
target/i386/helper.c | 42 +++++++++++++++++++++------------------
3 files changed, 42 insertions(+), 27 deletions(-)
diff --git a/target/i386/arch_memory_mapping.c b/target/i386/arch_memory_mapping.c
index 826aee5..647cff2 100644
--- a/target/i386/arch_memory_mapping.c
+++ b/target/i386/arch_memory_mapping.c
@@ -272,25 +272,27 @@ void x86_cpu_get_memory_mapping(CPUState *cs, MemoryMappingList *list,
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ int32_t a20_mask;
if (!cpu_paging_enabled(cs)) {
/* paging is disabled */
return;
}
+ a20_mask = x86_get_a20_mask(env);
if (env->cr[4] & CR4_PAE_MASK) {
#ifdef TARGET_X86_64
if (env->hflags & HF_LMA_MASK) {
if (env->cr[4] & CR4_LA57_MASK) {
hwaddr pml5e_addr;
- pml5e_addr = (env->cr[3] & PLM4_ADDR_MASK) & env->a20_mask;
- walk_pml5e(list, cs->as, pml5e_addr, env->a20_mask);
+ pml5e_addr = (env->cr[3] & PLM4_ADDR_MASK) & a20_mask;
+ walk_pml5e(list, cs->as, pml5e_addr, a20_mask);
} else {
hwaddr pml4e_addr;
- pml4e_addr = (env->cr[3] & PLM4_ADDR_MASK) & env->a20_mask;
- walk_pml4e(list, cs->as, pml4e_addr, env->a20_mask,
+ pml4e_addr = (env->cr[3] & PLM4_ADDR_MASK) & a20_mask;
+ walk_pml4e(list, cs->as, pml4e_addr, a20_mask,
0xffffULL << 48);
}
} else
@@ -298,16 +300,16 @@ void x86_cpu_get_memory_mapping(CPUState *cs, MemoryMappingList *list,
{
hwaddr pdpe_addr;
- pdpe_addr = (env->cr[3] & ~0x1f) & env->a20_mask;
- walk_pdpe2(list, cs->as, pdpe_addr, env->a20_mask);
+ pdpe_addr = (env->cr[3] & ~0x1f) & a20_mask;
+ walk_pdpe2(list, cs->as, pdpe_addr, a20_mask);
}
} else {
hwaddr pde_addr;
bool pse;
- pde_addr = (env->cr[3] & ~0xfff) & env->a20_mask;
+ pde_addr = (env->cr[3] & ~0xfff) & a20_mask;
pse = !!(env->cr[4] & CR4_PSE_MASK);
- walk_pde2(list, cs->as, pde_addr, env->a20_mask, pse);
+ walk_pde2(list, cs->as, pde_addr, a20_mask, pse);
}
}
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index c4602ca..32a3a0c 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1624,6 +1624,15 @@ static inline MemTxAttrs cpu_get_mem_attrs(CPUX86State *env)
return ((MemTxAttrs) { .secure = (env->hflags & HF_SMM_MASK) != 0 });
}
+static inline int32_t x86_get_a20_mask(CPUX86State *env)
+{
+ if (env->hflags & HF_SMM_MASK) {
+ return -1;
+ } else {
+ return env->a20_mask;
+ }
+}
+
/* fpu_helper.c */
void cpu_set_mxcsr(CPUX86State *env, uint32_t val);
void cpu_set_fpuc(CPUX86State *env, uint16_t val);
diff --git a/target/i386/helper.c b/target/i386/helper.c
index f11cac6..6c16e7c 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -724,6 +724,7 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
uint64_t ptep, pte;
+ int32_t a20_mask;
target_ulong pde_addr, pte_addr;
int error_code = 0;
int is_dirty, prot, page_size, is_write, is_user;
@@ -739,6 +740,7 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
#endif
is_write = is_write1 & 1;
+ a20_mask = x86_get_a20_mask(env);
if (!(env->cr[0] & CR0_PG_MASK)) {
pte = addr;
#ifdef TARGET_X86_64
@@ -777,7 +779,7 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
if (la57) {
pml5e_addr = ((env->cr[3] & ~0xfff) +
- (((addr >> 48) & 0x1ff) << 3)) & env->a20_mask;
+ (((addr >> 48) & 0x1ff) << 3)) & a20_mask;
pml5e = x86_ldq_phys(cs, pml5e_addr);
if (!(pml5e & PG_PRESENT_MASK)) {
goto do_fault;
@@ -796,7 +798,7 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
}
pml4e_addr = ((pml5e & PG_ADDRESS_MASK) +
- (((addr >> 39) & 0x1ff) << 3)) & env->a20_mask;
+ (((addr >> 39) & 0x1ff) << 3)) & a20_mask;
pml4e = x86_ldq_phys(cs, pml4e_addr);
if (!(pml4e & PG_PRESENT_MASK)) {
goto do_fault;
@@ -810,7 +812,7 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
}
ptep &= pml4e ^ PG_NX_MASK;
pdpe_addr = ((pml4e & PG_ADDRESS_MASK) + (((addr >> 30) & 0x1ff) << 3)) &
- env->a20_mask;
+ a20_mask;
pdpe = x86_ldq_phys(cs, pdpe_addr);
if (!(pdpe & PG_PRESENT_MASK)) {
goto do_fault;
@@ -835,7 +837,7 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
{
/* XXX: load them when cr3 is loaded ? */
pdpe_addr = ((env->cr[3] & ~0x1f) + ((addr >> 27) & 0x18)) &
- env->a20_mask;
+ a20_mask;
pdpe = x86_ldq_phys(cs, pdpe_addr);
if (!(pdpe & PG_PRESENT_MASK)) {
goto do_fault;
@@ -848,7 +850,7 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
}
pde_addr = ((pdpe & PG_ADDRESS_MASK) + (((addr >> 21) & 0x1ff) << 3)) &
- env->a20_mask;
+ a20_mask;
pde = x86_ldq_phys(cs, pde_addr);
if (!(pde & PG_PRESENT_MASK)) {
goto do_fault;
@@ -870,7 +872,7 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
x86_stl_phys_notdirty(cs, pde_addr, pde);
}
pte_addr = ((pde & PG_ADDRESS_MASK) + (((addr >> 12) & 0x1ff) << 3)) &
- env->a20_mask;
+ a20_mask;
pte = x86_ldq_phys(cs, pte_addr);
if (!(pte & PG_PRESENT_MASK)) {
goto do_fault;
@@ -886,7 +888,7 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
/* page directory entry */
pde_addr = ((env->cr[3] & ~0xfff) + ((addr >> 20) & 0xffc)) &
- env->a20_mask;
+ a20_mask;
pde = x86_ldl_phys(cs, pde_addr);
if (!(pde & PG_PRESENT_MASK)) {
goto do_fault;
@@ -913,7 +915,7 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
/* page directory entry */
pte_addr = ((pde & ~0xfff) + ((addr >> 10) & 0xffc)) &
- env->a20_mask;
+ a20_mask;
pte = x86_ldl_phys(cs, pte_addr);
if (!(pte & PG_PRESENT_MASK)) {
goto do_fault;
@@ -992,7 +994,7 @@ do_check_protect_pse36:
}
do_mapping:
- pte = pte & env->a20_mask;
+ pte = pte & a20_mask;
/* align to page_size */
pte &= PG_ADDRESS_MASK & ~(page_size - 1);
@@ -1039,11 +1041,13 @@ hwaddr x86_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
CPUX86State *env = &cpu->env;
target_ulong pde_addr, pte_addr;
uint64_t pte;
+ int32_t a20_mask;
uint32_t page_offset;
int page_size;
+ a20_mask = x86_get_a20_mask(env);
if (!(env->cr[0] & CR0_PG_MASK)) {
- pte = addr & env->a20_mask;
+ pte = addr & a20_mask;
page_size = 4096;
} else if (env->cr[4] & CR4_PAE_MASK) {
target_ulong pdpe_addr;
@@ -1064,7 +1068,7 @@ hwaddr x86_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
if (la57) {
pml5e_addr = ((env->cr[3] & ~0xfff) +
- (((addr >> 48) & 0x1ff) << 3)) & env->a20_mask;
+ (((addr >> 48) & 0x1ff) << 3)) & a20_mask;
pml5e = x86_ldq_phys(cs, pml5e_addr);
if (!(pml5e & PG_PRESENT_MASK)) {
return -1;
@@ -1074,13 +1078,13 @@ hwaddr x86_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
}
pml4e_addr = ((pml5e & PG_ADDRESS_MASK) +
- (((addr >> 39) & 0x1ff) << 3)) & env->a20_mask;
+ (((addr >> 39) & 0x1ff) << 3)) & a20_mask;
pml4e = x86_ldq_phys(cs, pml4e_addr);
if (!(pml4e & PG_PRESENT_MASK)) {
return -1;
}
pdpe_addr = ((pml4e & PG_ADDRESS_MASK) +
- (((addr >> 30) & 0x1ff) << 3)) & env->a20_mask;
+ (((addr >> 30) & 0x1ff) << 3)) & a20_mask;
pdpe = x86_ldq_phys(cs, pdpe_addr);
if (!(pdpe & PG_PRESENT_MASK)) {
return -1;
@@ -1095,14 +1099,14 @@ hwaddr x86_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
#endif
{
pdpe_addr = ((env->cr[3] & ~0x1f) + ((addr >> 27) & 0x18)) &
- env->a20_mask;
+ a20_mask;
pdpe = x86_ldq_phys(cs, pdpe_addr);
if (!(pdpe & PG_PRESENT_MASK))
return -1;
}
pde_addr = ((pdpe & PG_ADDRESS_MASK) +
- (((addr >> 21) & 0x1ff) << 3)) & env->a20_mask;
+ (((addr >> 21) & 0x1ff) << 3)) & a20_mask;
pde = x86_ldq_phys(cs, pde_addr);
if (!(pde & PG_PRESENT_MASK)) {
return -1;
@@ -1114,7 +1118,7 @@ hwaddr x86_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
} else {
/* 4 KB page */
pte_addr = ((pde & PG_ADDRESS_MASK) +
- (((addr >> 12) & 0x1ff) << 3)) & env->a20_mask;
+ (((addr >> 12) & 0x1ff) << 3)) & a20_mask;
page_size = 4096;
pte = x86_ldq_phys(cs, pte_addr);
}
@@ -1125,7 +1129,7 @@ hwaddr x86_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
uint32_t pde;
/* page directory entry */
- pde_addr = ((env->cr[3] & ~0xfff) + ((addr >> 20) & 0xffc)) & env->a20_mask;
+ pde_addr = ((env->cr[3] & ~0xfff) + ((addr >> 20) & 0xffc)) & a20_mask;
pde = x86_ldl_phys(cs, pde_addr);
if (!(pde & PG_PRESENT_MASK))
return -1;
@@ -1134,14 +1138,14 @@ hwaddr x86_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
page_size = 4096 * 1024;
} else {
/* page directory entry */
- pte_addr = ((pde & ~0xfff) + ((addr >> 10) & 0xffc)) & env->a20_mask;
+ pte_addr = ((pde & ~0xfff) + ((addr >> 10) & 0xffc)) & a20_mask;
pte = x86_ldl_phys(cs, pte_addr);
if (!(pte & PG_PRESENT_MASK)) {
return -1;
}
page_size = 4096;
}
- pte = pte & env->a20_mask;
+ pte = pte & a20_mask;
}
#ifdef TARGET_X86_64
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PULL 20/20] target/i386: use multiple CPU AddressSpaces
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (18 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 19/20] target/i386: enable A20 automatically in system management mode Paolo Bonzini
@ 2017-05-19 11:21 ` Paolo Bonzini
2017-05-19 12:41 ` [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 no-reply
2017-05-19 15:49 ` Stefan Hajnoczi
21 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-19 11:21 UTC (permalink / raw)
To: qemu-devel
This speeds up SMM switches. Later on it may remove the need to take
the BQL, and it may also allow to reuse code between TCG and KVM.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.c | 15 +++++++++-----
target/i386/cpu.h | 11 +++++++++-
target/i386/helper.c | 54 ++++++++++++++++++++++++------------------------
target/i386/machine.c | 4 ----
target/i386/smm_helper.c | 18 ----------------
5 files changed, 47 insertions(+), 55 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a41d595..a638832 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3239,7 +3239,7 @@ static void x86_cpu_machine_done(Notifier *n, void *unused)
cpu->smram = g_new(MemoryRegion, 1);
memory_region_init_alias(cpu->smram, OBJECT(cpu), "smram",
smram, 0, 1ull << 32);
- memory_region_set_enabled(cpu->smram, false);
+ memory_region_set_enabled(cpu->smram, true);
memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->smram, 1);
}
}
@@ -3619,7 +3619,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
#ifndef CONFIG_USER_ONLY
if (tcg_enabled()) {
- AddressSpace *newas = g_new(AddressSpace, 1);
+ AddressSpace *as_normal = address_space_init_shareable(cs->memory,
+ "cpu-memory");
+ AddressSpace *as_smm = g_new(AddressSpace, 1);
cpu->cpu_as_mem = g_new(MemoryRegion, 1);
cpu->cpu_as_root = g_new(MemoryRegion, 1);
@@ -3635,9 +3637,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
get_system_memory(), 0, ~0ull);
memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->cpu_as_mem, 0);
memory_region_set_enabled(cpu->cpu_as_mem, true);
- address_space_init(newas, cpu->cpu_as_root, "CPU");
- cs->num_ases = 1;
- cpu_address_space_init(cs, newas, 0);
+ address_space_init(as_smm, cpu->cpu_as_root, "CPU");
+
+ cs->num_ases = 2;
+ cpu_address_space_init(cs, as_normal, 0);
+ cpu_address_space_init(cs, as_smm, 1);
/* ... SMRAM with higher priority, linked from /machine/smram. */
cpu->machine_done.notify = x86_cpu_machine_done;
@@ -4053,6 +4057,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
#ifdef CONFIG_USER_ONLY
cc->handle_mmu_fault = x86_cpu_handle_mmu_fault;
#else
+ cc->asidx_from_attrs = x86_asidx_from_attrs;
cc->get_memory_mapping = x86_cpu_get_memory_mapping;
cc->get_phys_page_debug = x86_cpu_get_phys_page_debug;
cc->write_elf64_note = x86_cpu_write_elf64_note;
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 32a3a0c..c2e081c 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1450,6 +1450,16 @@ int x86_cpu_handle_mmu_fault(CPUState *cpu, vaddr addr,
void x86_cpu_set_a20(X86CPU *cpu, int a20_state);
#ifndef CONFIG_USER_ONLY
+static inline int x86_asidx_from_attrs(CPUState *cs, MemTxAttrs attrs)
+{
+ return !!attrs.secure;
+}
+
+static inline AddressSpace *cpu_addressspace(CPUState *cs, MemTxAttrs attrs)
+{
+ return cpu_get_address_space(cs, cpu_asidx_from_attrs(cs, attrs));
+}
+
uint8_t x86_ldub_phys(CPUState *cs, hwaddr addr);
uint32_t x86_lduw_phys(CPUState *cs, hwaddr addr);
uint32_t x86_ldl_phys(CPUState *cs, hwaddr addr);
@@ -1652,7 +1662,6 @@ void do_interrupt_x86_hardirq(CPUX86State *env, int intno, int is_hw);
/* smm_helper.c */
void do_smm_enter(X86CPU *cpu);
-void cpu_smm_update(X86CPU *cpu);
/* apic.c */
void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
diff --git a/target/i386/helper.c b/target/i386/helper.c
index 6c16e7c..d0daa1f 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -1403,89 +1403,89 @@ uint8_t x86_ldub_phys(CPUState *cs, hwaddr addr)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ MemTxAttrs attrs = cpu_get_mem_attrs(env);
+ AddressSpace *as = cpu_addressspace(cs, attrs);
- return address_space_ldub(cs->as, addr,
- cpu_get_mem_attrs(env),
- NULL);
+ return address_space_ldub(as, addr, attrs, NULL);
}
uint32_t x86_lduw_phys(CPUState *cs, hwaddr addr)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ MemTxAttrs attrs = cpu_get_mem_attrs(env);
+ AddressSpace *as = cpu_addressspace(cs, attrs);
- return address_space_lduw(cs->as, addr,
- cpu_get_mem_attrs(env),
- NULL);
+ return address_space_lduw(as, addr, attrs, NULL);
}
uint32_t x86_ldl_phys(CPUState *cs, hwaddr addr)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ MemTxAttrs attrs = cpu_get_mem_attrs(env);
+ AddressSpace *as = cpu_addressspace(cs, attrs);
- return address_space_ldl(cs->as, addr,
- cpu_get_mem_attrs(env),
- NULL);
+ return address_space_ldl(as, addr, attrs, NULL);
}
uint64_t x86_ldq_phys(CPUState *cs, hwaddr addr)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ MemTxAttrs attrs = cpu_get_mem_attrs(env);
+ AddressSpace *as = cpu_addressspace(cs, attrs);
- return address_space_ldq(cs->as, addr,
- cpu_get_mem_attrs(env),
- NULL);
+ return address_space_ldq(as, addr, attrs, NULL);
}
void x86_stb_phys(CPUState *cs, hwaddr addr, uint8_t val)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ MemTxAttrs attrs = cpu_get_mem_attrs(env);
+ AddressSpace *as = cpu_addressspace(cs, attrs);
- address_space_stb(cs->as, addr, val,
- cpu_get_mem_attrs(env),
- NULL);
+ address_space_stb(as, addr, val, attrs, NULL);
}
void x86_stl_phys_notdirty(CPUState *cs, hwaddr addr, uint32_t val)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ MemTxAttrs attrs = cpu_get_mem_attrs(env);
+ AddressSpace *as = cpu_addressspace(cs, attrs);
- address_space_stl_notdirty(cs->as, addr, val,
- cpu_get_mem_attrs(env),
- NULL);
+ address_space_stl_notdirty(as, addr, val, attrs, NULL);
}
void x86_stw_phys(CPUState *cs, hwaddr addr, uint32_t val)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ MemTxAttrs attrs = cpu_get_mem_attrs(env);
+ AddressSpace *as = cpu_addressspace(cs, attrs);
- address_space_stw(cs->as, addr, val,
- cpu_get_mem_attrs(env),
- NULL);
+ address_space_stw(as, addr, val, attrs, NULL);
}
void x86_stl_phys(CPUState *cs, hwaddr addr, uint32_t val)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ MemTxAttrs attrs = cpu_get_mem_attrs(env);
+ AddressSpace *as = cpu_addressspace(cs, attrs);
- address_space_stl(cs->as, addr, val,
- cpu_get_mem_attrs(env),
- NULL);
+ address_space_stl(as, addr, val, attrs, NULL);
}
void x86_stq_phys(CPUState *cs, hwaddr addr, uint64_t val)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ MemTxAttrs attrs = cpu_get_mem_attrs(env);
+ AddressSpace *as = cpu_addressspace(cs, attrs);
- address_space_stq(cs->as, addr, val,
- cpu_get_mem_attrs(env),
- NULL);
+ address_space_stq(as, addr, val, attrs, NULL);
}
#endif
diff --git a/target/i386/machine.c b/target/i386/machine.c
index 3cb2729..8c7a822 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -274,10 +274,6 @@ static int cpu_post_load(void *opaque, int version_id)
cpu_x86_update_dr7(env, dr7);
}
tlb_flush(cs);
-
- if (tcg_enabled()) {
- cpu_smm_update(cpu);
- }
return 0;
}
diff --git a/target/i386/smm_helper.c b/target/i386/smm_helper.c
index f051a77..90621e5 100644
--- a/target/i386/smm_helper.c
+++ b/target/i386/smm_helper.c
@@ -43,19 +43,6 @@ void helper_rsm(CPUX86State *env)
#define SMM_REVISION_ID 0x00020000
#endif
-/* Called with iothread lock taken */
-void cpu_smm_update(X86CPU *cpu)
-{
- CPUX86State *env = &cpu->env;
- bool smm_enabled = (env->hflags & HF_SMM_MASK);
-
- g_assert(qemu_mutex_iothread_locked());
-
- if (cpu->smram) {
- memory_region_set_enabled(cpu->smram, smm_enabled);
- }
-}
-
void do_smm_enter(X86CPU *cpu)
{
CPUX86State *env = &cpu->env;
@@ -73,7 +60,6 @@ void do_smm_enter(X86CPU *cpu)
} else {
env->hflags2 |= HF2_NMI_MASK;
}
- cpu_smm_update(cpu);
sm_state = env->smbase + 0x8000;
@@ -338,10 +324,6 @@ void helper_rsm(CPUX86State *env)
env->hflags2 &= ~HF2_SMM_INSIDE_NMI_MASK;
env->hflags &= ~HF_SMM_MASK;
- qemu_mutex_lock_iothread();
- cpu_smm_update(cpu);
- qemu_mutex_unlock_iothread();
-
qemu_log_mask(CPU_LOG_INT, "SMM: after RSM\n");
log_cpu_state_mask(CPU_LOG_INT, CPU(cpu), CPU_DUMP_CCOP);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (19 preceding siblings ...)
2017-05-19 11:21 ` [Qemu-devel] [PULL 20/20] target/i386: use multiple CPU AddressSpaces Paolo Bonzini
@ 2017-05-19 12:41 ` no-reply
2017-05-19 15:51 ` Stefan Hajnoczi
2017-05-19 15:49 ` Stefan Hajnoczi
21 siblings, 1 reply; 28+ messages in thread
From: no-reply @ 2017-05-19 12:41 UTC (permalink / raw)
To: pbonzini; +Cc: famz, qemu-devel
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 1495192872-27667-1-git-send-email-pbonzini@redhat.com
Type: series
Subject: [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
6fce4cd target/i386: use multiple CPU AddressSpaces
a04ba9d target/i386: enable A20 automatically in system management mode
87c38d5 vhost-user-scsi: Introduce a vhost-user-scsi sample application
a63728e vhost-user-scsi: Introduce vhost-user-scsi host device
bda4194 virtio-scsi: Unset hotplug handler when unrealize
ca14443 exec: simplify phys_page_find() params
7eee4fd nbd/client.c: use errp instead of LOG
388beda nbd: add errp to read_sync, write_sync and drop_sync
0032273 nbd: add errp parameter to nbd_wr_syncv()
bdf25c9 nbd: read_sync and friends: return 0 on success
b61d7d1 nbd: strict nbd_wr_syncv
cc100d3 Check the return value of fcntl in qemu_set_cloexec
94297c6 kvm: irqchip: skip update msi when disabled
f8f04f1 msix: trace control bit write op
11bfe30 kvm: irqchip: trace changes on msi add/remove
192c432 mc146818rtc: embrace all x86 specific code
6e1b003 mc146818rtc: drop unnecessary '#ifdef TARGET_I386'
cb9a45b mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386
98a508b mc146818rtc: precisely count the clock for periodic timer
b9744f3 mc146818rtc: update periodic timer only if it is needed
=== OUTPUT BEGIN ===
Checking PATCH 1/20: mc146818rtc: update periodic timer only if it is needed...
Checking PATCH 2/20: mc146818rtc: precisely count the clock for periodic timer...
ERROR: braces {} are necessary for all arms of this statement
#129: FILE: hw/timer/mc146818rtc.c:216:
+ if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
[...]
+ } else
[...]
total: 1 errors, 0 warnings, 181 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/20: mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386...
Checking PATCH 4/20: mc146818rtc: drop unnecessary '#ifdef TARGET_I386'...
Checking PATCH 5/20: mc146818rtc: embrace all x86 specific code...
Checking PATCH 6/20: kvm: irqchip: trace changes on msi add/remove...
Checking PATCH 7/20: msix: trace control bit write op...
Checking PATCH 8/20: kvm: irqchip: skip update msi when disabled...
Checking PATCH 9/20: Check the return value of fcntl in qemu_set_cloexec...
Checking PATCH 10/20: nbd: strict nbd_wr_syncv...
Checking PATCH 11/20: nbd: read_sync and friends: return 0 on success...
Checking PATCH 12/20: nbd: add errp parameter to nbd_wr_syncv()...
Checking PATCH 13/20: nbd: add errp to read_sync, write_sync and drop_sync...
Checking PATCH 14/20: nbd/client.c: use errp instead of LOG...
ERROR: code indent should never use tabs
#126: FILE: nbd/client.c:729:
+^I Error **errp)$
total: 1 errors, 0 warnings, 146 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 15/20: exec: simplify phys_page_find() params...
Checking PATCH 16/20: virtio-scsi: Unset hotplug handler when unrealize...
Checking PATCH 17/20: vhost-user-scsi: Introduce vhost-user-scsi host device...
ERROR: do not use C99 // comments
#216: FILE: hw/scsi/vhost-user-scsi.c:145:
+ // Turn on predefined features supported by this device
ERROR: do not use C99 // comments
#261: FILE: hw/scsi/vhost-user-scsi.c:190:
+ // Add the bootindex property for this object
ERROR: do not use C99 // comments
#265: FILE: hw/scsi/vhost-user-scsi.c:194:
+ // Set boot index according the the device config
total: 3 errors, 0 warnings, 382 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 18/20: vhost-user-scsi: Introduce a vhost-user-scsi sample application...
ERROR: do not use C99 // comments
#109: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:28:
+//#define VUS_DEBUG 1
ERROR: spaces required around that '/' (ctx:VxV)
#123: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:42:
+ timebuf, ts.tv_nsec/1000, \
^
ERROR: __func__ should be used instead of gcc specific __FUNCTION__
#124: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:43:
+ __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
ERROR: space required before the open parenthesis '('
#125: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:44:
+} while(0)
ERROR: spaces required around that '/' (ctx:VxV)
#130: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:49:
+ timebuf, ts.tv_nsec/1000, ## __VA_ARGS__); \
^
ERROR: space required before the open parenthesis '('
#131: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:50:
+} while(0);
ERROR: do not use C99 // comments
#145: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:64:
+#define VUS_MAX_LUNS 1 // Only 1 lun supported today
ERROR: do not use C99 // comments
#146: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:65:
+#define VUS_MAX_DEVS 1 // Only 1 devices supported today
ERROR: do not use C99 // comments
#159: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:78:
+ GTree *fdmap; // maps fd to gsource context id
ERROR: trailing statements should be on next line
#176: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:95:
+ if (b > a) return 1;
ERROR: braces {} are necessary for all arms of this statement
#176: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:95:
+ if (b > a) return 1;
[...]
ERROR: trailing statements should be on next line
#177: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:96:
+ if (b < a) return -1;
ERROR: braces {} are necessary for all arms of this statement
#177: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:96:
+ if (b < a) return -1;
[...]
ERROR: trailing statements should be on next line
#213: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:132:
+ if (vus_src->gfd.events & G_IO_IN) vu_evt |= VU_WATCH_IN;
ERROR: braces {} are necessary for all arms of this statement
#213: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:132:
+ if (vus_src->gfd.events & G_IO_IN) vu_evt |= VU_WATCH_IN;
[...]
ERROR: trailing statements should be on next line
#214: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:133:
+ if (vus_src->gfd.events & G_IO_OUT) vu_evt |= VU_WATCH_OUT;
ERROR: braces {} are necessary for all arms of this statement
#214: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:133:
+ if (vus_src->gfd.events & G_IO_OUT) vu_evt |= VU_WATCH_OUT;
[...]
ERROR: trailing statements should be on next line
#215: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:134:
+ if (vus_src->gfd.events & G_IO_PRI) vu_evt |= VU_WATCH_PRI;
ERROR: braces {} are necessary for all arms of this statement
#215: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:134:
+ if (vus_src->gfd.events & G_IO_PRI) vu_evt |= VU_WATCH_PRI;
[...]
ERROR: trailing statements should be on next line
#216: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:135:
+ if (vus_src->gfd.events & G_IO_ERR) vu_evt |= VU_WATCH_ERR;
ERROR: braces {} are necessary for all arms of this statement
#216: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:135:
+ if (vus_src->gfd.events & G_IO_ERR) vu_evt |= VU_WATCH_ERR;
[...]
ERROR: trailing statements should be on next line
#217: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:136:
+ if (vus_src->gfd.events & G_IO_HUP) vu_evt |= VU_WATCH_HUP;
ERROR: braces {} are necessary for all arms of this statement
#217: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:136:
+ if (vus_src->gfd.events & G_IO_HUP) vu_evt |= VU_WATCH_HUP;
[...]
ERROR: use QEMU instead of Qemu or QEmu
#277: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:196:
+ * Qemu's scsi.h also defines "SCSI_XFER_NONE".
ERROR: open brace '{' following function declarations go on the next line
#358: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:277:
+static int iscsi_add_lun(iscsi_lun_t *lun, char *iscsi_uri) {
ERROR: open brace '{' following function declarations go on the next line
#423: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:342:
+static int get_cdb_len(uint8_t *cdb) {
ERROR: spaces required around that '>>' (ctx:VxV)
#433: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:352:
+ PERR("Unable to determine cdb len (0x%02hhX)", cdb[0]>>5);
^
ERROR: do not use C99 // comments
#453: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:372:
+ // Ignore anything different than target=0, lun=0
ERROR: spaces required around that '=' (ctx:VxV)
#477: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:396:
+ for (i=0; i<out_len; i++) {
^
ERROR: spaces required around that '<' (ctx:VxV)
#477: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:396:
+ for (i=0; i<out_len; i++) {
^
ERROR: spaces required around that '=' (ctx:VxV)
#482: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:401:
+ for (i=0; i<in_len; i++) {
^
ERROR: spaces required around that '<' (ctx:VxV)
#482: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:401:
+ for (i=0; i<in_len; i++) {
^
ERROR: open brace '{' following function declarations go on the next line
#532: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:451:
+static void vus_panic_cb(VuDev *vu_dev, const char *buf) {
ERROR: trailing statements should be on next line
#574: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:493:
+ if (vu_evt & VU_WATCH_IN) conds |= G_IO_IN;
ERROR: braces {} are necessary for all arms of this statement
#574: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:493:
+ if (vu_evt & VU_WATCH_IN) conds |= G_IO_IN;
[...]
ERROR: trailing statements should be on next line
#575: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:494:
+ if (vu_evt & VU_WATCH_OUT) conds |= G_IO_OUT;
ERROR: braces {} are necessary for all arms of this statement
#575: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:494:
+ if (vu_evt & VU_WATCH_OUT) conds |= G_IO_OUT;
[...]
ERROR: trailing statements should be on next line
#576: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:495:
+ if (vu_evt & VU_WATCH_PRI) conds |= G_IO_PRI;
ERROR: braces {} are necessary for all arms of this statement
#576: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:495:
+ if (vu_evt & VU_WATCH_PRI) conds |= G_IO_PRI;
[...]
ERROR: trailing statements should be on next line
#577: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:496:
+ if (vu_evt & VU_WATCH_ERR) conds |= G_IO_ERR;
ERROR: braces {} are necessary for all arms of this statement
#577: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:496:
+ if (vu_evt & VU_WATCH_ERR) conds |= G_IO_ERR;
[...]
ERROR: trailing statements should be on next line
#578: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:497:
+ if (vu_evt & VU_WATCH_HUP) conds |= G_IO_HUP;
ERROR: braces {} are necessary for all arms of this statement
#578: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:497:
+ if (vu_evt & VU_WATCH_HUP) conds |= G_IO_HUP;
[...]
ERROR: open brace '{' following function declarations go on the next line
#585: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:504:
+static void vus_del_watch_cb(VuDev *vu_dev, int fd) {
ERROR: open brace '{' following function declarations go on the next line
#608: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:527:
+static void vus_proc_ctl(VuDev *vu_dev, int idx) {
ERROR: open brace '{' following function declarations go on the next line
#612: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:531:
+static void vus_proc_evt(VuDev *vu_dev, int idx) {
ERROR: open brace '{' following function declarations go on the next line
#616: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:535:
+static void vus_proc_req(VuDev *vu_dev, int idx) {
ERROR: space required before the open parenthesis '('
#643: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:562:
+ while(1) {
ERROR: spaces required around that '-' (ctx:VxV)
#673: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:592:
+ req, &elem->out_sg[1], elem->out_num-1,
^
ERROR: spaces required around that '-' (ctx:VxV)
#674: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:593:
+ rsp, &elem->in_sg[1], elem->in_num-1) != 0) {
^
ERROR: open brace '{' following function declarations go on the next line
#686: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:605:
+static void vus_queue_set_started(VuDev *vu_dev, int idx, bool started) {
ERROR: space required before the open parenthesis '('
#699: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:618:
+ switch(idx) {
ERROR: spaces required around that '?' (ctx:VxV)
#701: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:620:
+ vu_set_queue_handler(vu_dev, vq, started?vus_proc_ctl:NULL);
^
ERROR: spaces required around that ':' (ctx:VxV)
#701: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:620:
+ vu_set_queue_handler(vu_dev, vq, started?vus_proc_ctl:NULL);
^
ERROR: spaces required around that '?' (ctx:VxV)
#704: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:623:
+ vu_set_queue_handler(vu_dev, vq, started?vus_proc_evt:NULL);
^
ERROR: spaces required around that ':' (ctx:VxV)
#704: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:623:
+ vu_set_queue_handler(vu_dev, vq, started?vus_proc_evt:NULL);
^
ERROR: spaces required around that '?' (ctx:VxV)
#707: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:626:
+ vu_set_queue_handler(vu_dev, vq, started?vus_proc_req:NULL);
^
ERROR: spaces required around that ':' (ctx:VxV)
#707: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:626:
+ vu_set_queue_handler(vu_dev, vq, started?vus_proc_req:NULL);
^
ERROR: open brace '{' following function declarations go on the next line
#715: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:634:
+static gboolean vus_vhost_cb(gpointer data) {
ERROR: open brace '{' following function declarations go on the next line
#731: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:650:
+static int unix_sock_new(char *unix_fn) {
ERROR: open brace '{' following function declarations go on the next line
#769: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:688:
+static vhost_scsi_dev_t *vdev_scsi_find_by_vu(VuDev *vu_dev) {
ERROR: spaces required around that '=' (ctx:VxV)
#774: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:693:
+ for (i=0; i<VUS_MAX_DEVS; i++) {
^
ERROR: spaces required around that '<' (ctx:VxV)
#774: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:693:
+ for (i=0; i<VUS_MAX_DEVS; i++) {
^
ERROR: open brace '{' following function declarations go on the next line
#784: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:703:
+static void vdev_scsi_deinit(vhost_scsi_dev_t *vdev_scsi) {
ERROR: open brace '{' following function declarations go on the next line
#809: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:728:
+static vhost_scsi_dev_t *vdev_scsi_new(char *unix_fn) {
ERROR: open brace '{' following function declarations go on the next line
#864: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:783:
+static int vdev_scsi_run(vhost_scsi_dev_t *vdev_scsi) {
total: 66 errors, 0 warnings, 912 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 19/20: target/i386: enable A20 automatically in system management mode...
Checking PATCH 20/20: target/i386: use multiple CPU AddressSpaces...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19
2017-05-19 12:41 ` [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 no-reply
@ 2017-05-19 15:51 ` Stefan Hajnoczi
2017-05-19 16:09 ` Paolo Bonzini
0 siblings, 1 reply; 28+ messages in thread
From: Stefan Hajnoczi @ 2017-05-19 15:51 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, famz
[-- Attachment #1: Type: text/plain, Size: 18563 bytes --]
On Fri, May 19, 2017 at 05:41:28AM -0700, no-reply@patchew.org wrote:
> Hi,
>
> This series seems to have some coding style problems. See output below for
> more information:
Yikes, on second thought I've dropped the pull request for now.
Please look at these coding style violations.
Thanks,
Stefan
>
> Message-id: 1495192872-27667-1-git-send-email-pbonzini@redhat.com
> Type: series
> Subject: [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
>
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
>
> git config --local diff.renamelimit 0
> git config --local diff.renames True
>
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
> echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
> if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
> failed=1
> echo
> fi
> n=$((n+1))
> done
>
> exit $failed
> === TEST SCRIPT END ===
>
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> 6fce4cd target/i386: use multiple CPU AddressSpaces
> a04ba9d target/i386: enable A20 automatically in system management mode
> 87c38d5 vhost-user-scsi: Introduce a vhost-user-scsi sample application
> a63728e vhost-user-scsi: Introduce vhost-user-scsi host device
> bda4194 virtio-scsi: Unset hotplug handler when unrealize
> ca14443 exec: simplify phys_page_find() params
> 7eee4fd nbd/client.c: use errp instead of LOG
> 388beda nbd: add errp to read_sync, write_sync and drop_sync
> 0032273 nbd: add errp parameter to nbd_wr_syncv()
> bdf25c9 nbd: read_sync and friends: return 0 on success
> b61d7d1 nbd: strict nbd_wr_syncv
> cc100d3 Check the return value of fcntl in qemu_set_cloexec
> 94297c6 kvm: irqchip: skip update msi when disabled
> f8f04f1 msix: trace control bit write op
> 11bfe30 kvm: irqchip: trace changes on msi add/remove
> 192c432 mc146818rtc: embrace all x86 specific code
> 6e1b003 mc146818rtc: drop unnecessary '#ifdef TARGET_I386'
> cb9a45b mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386
> 98a508b mc146818rtc: precisely count the clock for periodic timer
> b9744f3 mc146818rtc: update periodic timer only if it is needed
>
> === OUTPUT BEGIN ===
> Checking PATCH 1/20: mc146818rtc: update periodic timer only if it is needed...
> Checking PATCH 2/20: mc146818rtc: precisely count the clock for periodic timer...
> ERROR: braces {} are necessary for all arms of this statement
> #129: FILE: hw/timer/mc146818rtc.c:216:
> + if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
> [...]
> + } else
> [...]
>
> total: 1 errors, 0 warnings, 181 lines checked
>
> Your patch has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> Checking PATCH 3/20: mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386...
> Checking PATCH 4/20: mc146818rtc: drop unnecessary '#ifdef TARGET_I386'...
> Checking PATCH 5/20: mc146818rtc: embrace all x86 specific code...
> Checking PATCH 6/20: kvm: irqchip: trace changes on msi add/remove...
> Checking PATCH 7/20: msix: trace control bit write op...
> Checking PATCH 8/20: kvm: irqchip: skip update msi when disabled...
> Checking PATCH 9/20: Check the return value of fcntl in qemu_set_cloexec...
> Checking PATCH 10/20: nbd: strict nbd_wr_syncv...
> Checking PATCH 11/20: nbd: read_sync and friends: return 0 on success...
> Checking PATCH 12/20: nbd: add errp parameter to nbd_wr_syncv()...
> Checking PATCH 13/20: nbd: add errp to read_sync, write_sync and drop_sync...
> Checking PATCH 14/20: nbd/client.c: use errp instead of LOG...
> ERROR: code indent should never use tabs
> #126: FILE: nbd/client.c:729:
> +^I Error **errp)$
>
> total: 1 errors, 0 warnings, 146 lines checked
>
> Your patch has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> Checking PATCH 15/20: exec: simplify phys_page_find() params...
> Checking PATCH 16/20: virtio-scsi: Unset hotplug handler when unrealize...
> Checking PATCH 17/20: vhost-user-scsi: Introduce vhost-user-scsi host device...
> ERROR: do not use C99 // comments
> #216: FILE: hw/scsi/vhost-user-scsi.c:145:
> + // Turn on predefined features supported by this device
>
> ERROR: do not use C99 // comments
> #261: FILE: hw/scsi/vhost-user-scsi.c:190:
> + // Add the bootindex property for this object
>
> ERROR: do not use C99 // comments
> #265: FILE: hw/scsi/vhost-user-scsi.c:194:
> + // Set boot index according the the device config
>
> total: 3 errors, 0 warnings, 382 lines checked
>
> Your patch has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> Checking PATCH 18/20: vhost-user-scsi: Introduce a vhost-user-scsi sample application...
> ERROR: do not use C99 // comments
> #109: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:28:
> +//#define VUS_DEBUG 1
>
> ERROR: spaces required around that '/' (ctx:VxV)
> #123: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:42:
> + timebuf, ts.tv_nsec/1000, \
> ^
>
> ERROR: __func__ should be used instead of gcc specific __FUNCTION__
> #124: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:43:
> + __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
>
> ERROR: space required before the open parenthesis '('
> #125: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:44:
> +} while(0)
>
> ERROR: spaces required around that '/' (ctx:VxV)
> #130: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:49:
> + timebuf, ts.tv_nsec/1000, ## __VA_ARGS__); \
> ^
>
> ERROR: space required before the open parenthesis '('
> #131: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:50:
> +} while(0);
>
> ERROR: do not use C99 // comments
> #145: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:64:
> +#define VUS_MAX_LUNS 1 // Only 1 lun supported today
>
> ERROR: do not use C99 // comments
> #146: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:65:
> +#define VUS_MAX_DEVS 1 // Only 1 devices supported today
>
> ERROR: do not use C99 // comments
> #159: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:78:
> + GTree *fdmap; // maps fd to gsource context id
>
> ERROR: trailing statements should be on next line
> #176: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:95:
> + if (b > a) return 1;
>
> ERROR: braces {} are necessary for all arms of this statement
> #176: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:95:
> + if (b > a) return 1;
> [...]
>
> ERROR: trailing statements should be on next line
> #177: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:96:
> + if (b < a) return -1;
>
> ERROR: braces {} are necessary for all arms of this statement
> #177: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:96:
> + if (b < a) return -1;
> [...]
>
> ERROR: trailing statements should be on next line
> #213: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:132:
> + if (vus_src->gfd.events & G_IO_IN) vu_evt |= VU_WATCH_IN;
>
> ERROR: braces {} are necessary for all arms of this statement
> #213: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:132:
> + if (vus_src->gfd.events & G_IO_IN) vu_evt |= VU_WATCH_IN;
> [...]
>
> ERROR: trailing statements should be on next line
> #214: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:133:
> + if (vus_src->gfd.events & G_IO_OUT) vu_evt |= VU_WATCH_OUT;
>
> ERROR: braces {} are necessary for all arms of this statement
> #214: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:133:
> + if (vus_src->gfd.events & G_IO_OUT) vu_evt |= VU_WATCH_OUT;
> [...]
>
> ERROR: trailing statements should be on next line
> #215: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:134:
> + if (vus_src->gfd.events & G_IO_PRI) vu_evt |= VU_WATCH_PRI;
>
> ERROR: braces {} are necessary for all arms of this statement
> #215: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:134:
> + if (vus_src->gfd.events & G_IO_PRI) vu_evt |= VU_WATCH_PRI;
> [...]
>
> ERROR: trailing statements should be on next line
> #216: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:135:
> + if (vus_src->gfd.events & G_IO_ERR) vu_evt |= VU_WATCH_ERR;
>
> ERROR: braces {} are necessary for all arms of this statement
> #216: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:135:
> + if (vus_src->gfd.events & G_IO_ERR) vu_evt |= VU_WATCH_ERR;
> [...]
>
> ERROR: trailing statements should be on next line
> #217: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:136:
> + if (vus_src->gfd.events & G_IO_HUP) vu_evt |= VU_WATCH_HUP;
>
> ERROR: braces {} are necessary for all arms of this statement
> #217: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:136:
> + if (vus_src->gfd.events & G_IO_HUP) vu_evt |= VU_WATCH_HUP;
> [...]
>
> ERROR: use QEMU instead of Qemu or QEmu
> #277: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:196:
> + * Qemu's scsi.h also defines "SCSI_XFER_NONE".
>
> ERROR: open brace '{' following function declarations go on the next line
> #358: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:277:
> +static int iscsi_add_lun(iscsi_lun_t *lun, char *iscsi_uri) {
>
> ERROR: open brace '{' following function declarations go on the next line
> #423: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:342:
> +static int get_cdb_len(uint8_t *cdb) {
>
> ERROR: spaces required around that '>>' (ctx:VxV)
> #433: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:352:
> + PERR("Unable to determine cdb len (0x%02hhX)", cdb[0]>>5);
> ^
>
> ERROR: do not use C99 // comments
> #453: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:372:
> + // Ignore anything different than target=0, lun=0
>
> ERROR: spaces required around that '=' (ctx:VxV)
> #477: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:396:
> + for (i=0; i<out_len; i++) {
> ^
>
> ERROR: spaces required around that '<' (ctx:VxV)
> #477: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:396:
> + for (i=0; i<out_len; i++) {
> ^
>
> ERROR: spaces required around that '=' (ctx:VxV)
> #482: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:401:
> + for (i=0; i<in_len; i++) {
> ^
>
> ERROR: spaces required around that '<' (ctx:VxV)
> #482: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:401:
> + for (i=0; i<in_len; i++) {
> ^
>
> ERROR: open brace '{' following function declarations go on the next line
> #532: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:451:
> +static void vus_panic_cb(VuDev *vu_dev, const char *buf) {
>
> ERROR: trailing statements should be on next line
> #574: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:493:
> + if (vu_evt & VU_WATCH_IN) conds |= G_IO_IN;
>
> ERROR: braces {} are necessary for all arms of this statement
> #574: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:493:
> + if (vu_evt & VU_WATCH_IN) conds |= G_IO_IN;
> [...]
>
> ERROR: trailing statements should be on next line
> #575: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:494:
> + if (vu_evt & VU_WATCH_OUT) conds |= G_IO_OUT;
>
> ERROR: braces {} are necessary for all arms of this statement
> #575: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:494:
> + if (vu_evt & VU_WATCH_OUT) conds |= G_IO_OUT;
> [...]
>
> ERROR: trailing statements should be on next line
> #576: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:495:
> + if (vu_evt & VU_WATCH_PRI) conds |= G_IO_PRI;
>
> ERROR: braces {} are necessary for all arms of this statement
> #576: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:495:
> + if (vu_evt & VU_WATCH_PRI) conds |= G_IO_PRI;
> [...]
>
> ERROR: trailing statements should be on next line
> #577: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:496:
> + if (vu_evt & VU_WATCH_ERR) conds |= G_IO_ERR;
>
> ERROR: braces {} are necessary for all arms of this statement
> #577: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:496:
> + if (vu_evt & VU_WATCH_ERR) conds |= G_IO_ERR;
> [...]
>
> ERROR: trailing statements should be on next line
> #578: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:497:
> + if (vu_evt & VU_WATCH_HUP) conds |= G_IO_HUP;
>
> ERROR: braces {} are necessary for all arms of this statement
> #578: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:497:
> + if (vu_evt & VU_WATCH_HUP) conds |= G_IO_HUP;
> [...]
>
> ERROR: open brace '{' following function declarations go on the next line
> #585: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:504:
> +static void vus_del_watch_cb(VuDev *vu_dev, int fd) {
>
> ERROR: open brace '{' following function declarations go on the next line
> #608: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:527:
> +static void vus_proc_ctl(VuDev *vu_dev, int idx) {
>
> ERROR: open brace '{' following function declarations go on the next line
> #612: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:531:
> +static void vus_proc_evt(VuDev *vu_dev, int idx) {
>
> ERROR: open brace '{' following function declarations go on the next line
> #616: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:535:
> +static void vus_proc_req(VuDev *vu_dev, int idx) {
>
> ERROR: space required before the open parenthesis '('
> #643: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:562:
> + while(1) {
>
> ERROR: spaces required around that '-' (ctx:VxV)
> #673: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:592:
> + req, &elem->out_sg[1], elem->out_num-1,
> ^
>
> ERROR: spaces required around that '-' (ctx:VxV)
> #674: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:593:
> + rsp, &elem->in_sg[1], elem->in_num-1) != 0) {
> ^
>
> ERROR: open brace '{' following function declarations go on the next line
> #686: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:605:
> +static void vus_queue_set_started(VuDev *vu_dev, int idx, bool started) {
>
> ERROR: space required before the open parenthesis '('
> #699: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:618:
> + switch(idx) {
>
> ERROR: spaces required around that '?' (ctx:VxV)
> #701: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:620:
> + vu_set_queue_handler(vu_dev, vq, started?vus_proc_ctl:NULL);
> ^
>
> ERROR: spaces required around that ':' (ctx:VxV)
> #701: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:620:
> + vu_set_queue_handler(vu_dev, vq, started?vus_proc_ctl:NULL);
> ^
>
> ERROR: spaces required around that '?' (ctx:VxV)
> #704: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:623:
> + vu_set_queue_handler(vu_dev, vq, started?vus_proc_evt:NULL);
> ^
>
> ERROR: spaces required around that ':' (ctx:VxV)
> #704: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:623:
> + vu_set_queue_handler(vu_dev, vq, started?vus_proc_evt:NULL);
> ^
>
> ERROR: spaces required around that '?' (ctx:VxV)
> #707: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:626:
> + vu_set_queue_handler(vu_dev, vq, started?vus_proc_req:NULL);
> ^
>
> ERROR: spaces required around that ':' (ctx:VxV)
> #707: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:626:
> + vu_set_queue_handler(vu_dev, vq, started?vus_proc_req:NULL);
> ^
>
> ERROR: open brace '{' following function declarations go on the next line
> #715: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:634:
> +static gboolean vus_vhost_cb(gpointer data) {
>
> ERROR: open brace '{' following function declarations go on the next line
> #731: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:650:
> +static int unix_sock_new(char *unix_fn) {
>
> ERROR: open brace '{' following function declarations go on the next line
> #769: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:688:
> +static vhost_scsi_dev_t *vdev_scsi_find_by_vu(VuDev *vu_dev) {
>
> ERROR: spaces required around that '=' (ctx:VxV)
> #774: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:693:
> + for (i=0; i<VUS_MAX_DEVS; i++) {
> ^
>
> ERROR: spaces required around that '<' (ctx:VxV)
> #774: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:693:
> + for (i=0; i<VUS_MAX_DEVS; i++) {
> ^
>
> ERROR: open brace '{' following function declarations go on the next line
> #784: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:703:
> +static void vdev_scsi_deinit(vhost_scsi_dev_t *vdev_scsi) {
>
> ERROR: open brace '{' following function declarations go on the next line
> #809: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:728:
> +static vhost_scsi_dev_t *vdev_scsi_new(char *unix_fn) {
>
> ERROR: open brace '{' following function declarations go on the next line
> #864: FILE: contrib/vhost-user-scsi/vhost-user-scsi.c:783:
> +static int vdev_scsi_run(vhost_scsi_dev_t *vdev_scsi) {
>
> total: 66 errors, 0 warnings, 912 lines checked
>
> Your patch has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> Checking PATCH 19/20: target/i386: enable A20 automatically in system management mode...
> Checking PATCH 20/20: target/i386: use multiple CPU AddressSpaces...
> === OUTPUT END ===
>
> Test command exited with code: 1
>
>
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19
2017-05-19 11:20 [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 Paolo Bonzini
` (20 preceding siblings ...)
2017-05-19 12:41 ` [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19 no-reply
@ 2017-05-19 15:49 ` Stefan Hajnoczi
21 siblings, 0 replies; 28+ messages in thread
From: Stefan Hajnoczi @ 2017-05-19 15:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4749 bytes --]
On Fri, May 19, 2017 at 01:20:52PM +0200, Paolo Bonzini wrote:
> The following changes since commit 56821559f0ba682fe6b367815572e6f974d329ab:
>
> Merge remote-tracking branch 'dgilbert/tags/pull-hmp-20170517' into staging (2017-05-18 13:36:15 +0100)
>
> are available in the git repository at:
>
>
> git://github.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to e10dc0ca6854c4f47cc5e9d47e20c62aa875f518:
>
> target/i386: use multiple CPU AddressSpaces (2017-05-19 13:01:32 +0200)
>
> ----------------------------------------------------------------
> * virtio-scsi use-after-free fix (Fam)
> * vhost-user-scsi support (Felipe)
> * SMM fixes and improvements for TCG (myself)
> * irqchip and AddressSpaceDispatch cleanups and fixes (Peter)
> * Coverity fix (Stefano)
> * NBD cleanups (Vladimir)
> * RTC accuracy improvements and code cleanups (Guangrong+Yunfang)
>
> ----------------------------------------------------------------
> Fam Zheng (1):
> virtio-scsi: Unset hotplug handler when unrealize
>
> Felipe Franciosi (2):
> vhost-user-scsi: Introduce vhost-user-scsi host device
> vhost-user-scsi: Introduce a vhost-user-scsi sample application
>
> Paolo Bonzini (2):
> target/i386: enable A20 automatically in system management mode
> target/i386: use multiple CPU AddressSpaces
>
> Peter Xu (4):
> kvm: irqchip: trace changes on msi add/remove
> msix: trace control bit write op
> kvm: irqchip: skip update msi when disabled
> exec: simplify phys_page_find() params
>
> Stefano Stabellini (1):
> Check the return value of fcntl in qemu_set_cloexec
>
> Tai Yunfang (1):
> mc146818rtc: precisely count the clock for periodic timer
>
> Vladimir Sementsov-Ogievskiy (5):
> nbd: strict nbd_wr_syncv
> nbd: read_sync and friends: return 0 on success
> nbd: add errp parameter to nbd_wr_syncv()
> nbd: add errp to read_sync, write_sync and drop_sync
> nbd/client.c: use errp instead of LOG
>
> Xiao Guangrong (4):
> mc146818rtc: update periodic timer only if it is needed
> mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386
> mc146818rtc: drop unnecessary '#ifdef TARGET_I386'
> mc146818rtc: embrace all x86 specific code
>
> .gitignore | 1 +
> Makefile | 3 +
> Makefile.objs | 4 +
> block/nbd-client.c | 11 +-
> contrib/vhost-user-scsi/Makefile.objs | 1 +
> contrib/vhost-user-scsi/vhost-user-scsi.c | 886 ++++++++++++++++++++++++++++++
> default-configs/pci.mak | 1 +
> default-configs/s390x-softmmu.mak | 1 +
> exec.c | 13 +-
> hw/pci/msix.c | 11 +-
> hw/pci/trace-events | 3 +
> hw/scsi/Makefile.objs | 1 +
> hw/scsi/vhost-user-scsi.c | 215 ++++++++
> hw/scsi/virtio-scsi.c | 3 +
> hw/timer/mc146818rtc.c | 206 ++++---
> hw/virtio/virtio-pci.c | 54 ++
> hw/virtio/virtio-pci.h | 11 +
> include/block/nbd.h | 8 +-
> include/hw/virtio/vhost-user-scsi.h | 35 ++
> include/hw/virtio/virtio-scsi.h | 3 +
> kvm-all.c | 4 +-
> nbd/client.c | 125 ++---
> nbd/common.c | 23 +-
> nbd/nbd-internal.h | 40 +-
> nbd/server.c | 92 ++--
> qemu-nbd.c | 3 +-
> target/i386/arch_memory_mapping.c | 18 +-
> target/i386/cpu.c | 15 +-
> target/i386/cpu.h | 20 +-
> target/i386/helper.c | 96 ++--
> target/i386/kvm.c | 12 +-
> target/i386/machine.c | 4 -
> target/i386/smm_helper.c | 18 -
> trace-events | 3 +-
> util/oslib-posix.c | 4 +-
> 35 files changed, 1642 insertions(+), 306 deletions(-)
> create mode 100644 contrib/vhost-user-scsi/Makefile.objs
> create mode 100644 contrib/vhost-user-scsi/vhost-user-scsi.c
> create mode 100644 hw/scsi/vhost-user-scsi.c
> create mode 100644 include/hw/virtio/vhost-user-scsi.h
> --
> 1.8.3.1
>
>
Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread