Re: [Qemu-devel] [PATCH v2 10/23] target-arm: implement CPACR register logic
14.05.2014 10:06, Sergey Fedorov пишет:
> On 13.05.2014 20:15, Fabian Aggeler wrote:
>> From: Sergey Fedorov
>>
>> CPACR register allows to control access rights to coprocessor 0-13
>> interfaces. Bits corresponding to unimplemented coprocessors should be
>> RAZ/WI. QEMU implements only VFP coprocessor on ARMv6+ targets. So only
>> cp10 & cp11 bits are writable.
>>
>> Signed-off-by: Sergey Fedorov
>> Signed-off-by: Fabian Aggeler
>> ---
>> target-arm/helper.c| 6 ++
>> target-arm/translate.c | 26 +++---
>> 2 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index cf1f88c..4e82259 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -477,6 +477,12 @@ static const ARMCPRegInfo not_v7_cp_reginfo[] = {
>> static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> uint64_t value)
>> {
>> +uint32_t mask = 0;
>> +
>> +if (arm_feature(env, ARM_FEATURE_VFP)) {
>> +mask |= 0x00f0; /* VFP coprocessor: cp10 & cp11 */
>> +}
>> +value &= mask;
>> if (env->cp15.c1_coproc != value) {
>> env->cp15.c1_coproc = value;
>> /* ??? Is this safe when called from within a TB? */
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 87d0918..c815fb3 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -6866,9 +6866,29 @@ static int disas_coproc_insn(CPUARMState * env,
>> DisasContext *s, uint32_t insn)
>> const ARMCPRegInfo *ri;
>>
>> cpnum = (insn >> 8) & 0xf;
>> -if (arm_feature(env, ARM_FEATURE_XSCALE)
>> -&& ((env->cp15.c15_cpar ^ 0x3fff) & (1 << cpnum)))
>> -return 1;
>> +if (cpnum < 14) {
>> +if (arm_feature(env, ARM_FEATURE_XSCALE)) {
>> +if (~env->cp15.c15_cpar & (1 << cpnum)) {
>> +return 1;
>> +}
>> +} else {
>> +/* Bits [20:21] of CPACR control access to cp10
>> + * Bits [23:22] of CPACR control access to cp11 */
>> +switch ((env->cp15.c1_coproc >> (cpnum * 2)) & 3) {
>> +case 0: /* access denied */
>> +return 1;
>> +case 1: /* privileged mode access only */
>> +if (IS_USER(s)) {
>> +return 1;
>> +}
>> +break;
>> +case 2: /* reserved */
>> +return 1;
>> +case 3: /* privileged and user mode access */
>> +break;
>> +}
>> +}
>> +}
>>
>> /* First check for coprocessor space used for actual instructions */
>> switch (cpnum) {
> Please, look at disas_vfp_insn() and disas_neon_*_insn() functions.
> Looks like them should be updated. In that case do not forget to adjust
> arm_cpu_reset() so user emulation would be able to execute VFP/NEON
> instructions.
See ARM ARM v7-AR B1.11.1
>
> Thanks,
> Sergey.
Re: [Qemu-devel] [PATCH v2 06/23] target-arm: add arm_is_secure() function
14.05.2014 18:42, Greg Bellows пишет:
> On 14 May 2014 00:53, Sergey Fedorov wrote:
>
>> On 13.05.2014 20:15, Fabian Aggeler wrote:
>>> arm_is_secure() function allows to determine CPU security state
>>> if the CPU implements Security Extensions.
>>>
>>> Signed-off-by: Sergey Fedorov
>>> Signed-off-by: Fabian Aggeler
>>> ---
>>> target-arm/cpu.h | 15 +++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index a56d3d6..6ea0432 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -640,6 +640,21 @@ static inline int arm_feature(CPUARMState *env, int
>> feature)
>>> return (env->features & (1ULL << feature)) != 0;
>>> }
>>>
>>> +/* Return true if the processor is in secure state */
>>> +static inline bool arm_is_secure(CPUARMState *env)
>>> +{
>>> +#if !defined(CONFIG_USER_ONLY)
>>> +if (arm_feature(env, ARM_FEATURE_SECURITY_EXTENSIONS)) {
>> I think feature test can be safely avoided here. Without this feature
>> that should be no way to switch to monitor mode and to access SCR register.
>>
> I agree with the feature check here. For correctness, we should only be
> examining c1_scr if the security extension is enabled. This is consistent
> with only registering the SCR register if the feature is enabled.
So this check will be done every time arm_is_secure() is called, e.g. on
each MMU table walk.
Moreover I've noticed that this function deviates from ARM ARM v7-AR
description in section B1.5.1 which states: "The IsSecure() function
returns TRUE if the processor is in Secure state, or if the
implementation does not include
the Security Extensions, and FALSE otherwise." Then there is a pseudo
code for that function.
>
>>> +return ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) ||
>>> +!(env->cp15.c1_scr & 1);
>>> +} else {
>>> +return false;
>>> +}
>>> +#else
>>> +return false;
>> That is a good question how to treat user emulation: secure or
>> non-secure. Perhaps assuming user emulation in secure state may simplify
>> code in the following patches.
>
>>> +#endif
>>> +}
>>> +
>>> /* Return true if the specified exception level is running in AArch64
>> state. */
>>> static inline bool arm_el_is_aa64(CPUARMState *env, int el)
>>> {
>> Thanks,
>> Sergey.
>>
>>
Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers
On 12/20/2013 06:33 PM, Peter Maydell wrote: > This sounds like it could work, though there are some wrinkles for > registers with readfns/writefns -- do we have extra s vs ns read/write > functions, or just one set of functions which has to look in env->ns to > figure out whether to use the S or NS version? What about defining a separate ARMCPRegInfo for each banked AArch32 system register? I think it would be close to AArch64 concept. It would allow to use separate read/write handlers if necessary or reuse the same handlers otherwise. When the handlers is not used, the translation code would simply lookup the ARMCPRegInfo for corresponding secure state and use the field offset. Best regards, Sergey Fedorov
Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers
On 12/22/2013 05:08 AM, Peter Crosthwaite wrote: > On Sat, Dec 21, 2013 at 12:33 AM, Peter Maydell > wrote: >> On 20 December 2013 14:12, Fedorov Sergey wrote: >>> I've briefly looked at the v8 ARM ARM. As I can see there is no banked >>> system control registers in AArch64. Seems the concept is changed to provide >>> separate registers for each meaningful execution level. Please, correct me >>> if I am wrong. > Isn't that just another definition of banking? > >> Yes, I think this is generally correct. >> >>> So I think there shouldn't be "active" and "banked" fields for banked >>> AArch32 CP15 registers as in my patch. > I don't see how this extra scheme warrants the > active-set+mass-hotswapping impl. Why can the accessors just be aware > of the two different banking schemes at access time? > > Seems it is worth to use AArch64 view >>> of system control registers as a basis. That means there would be separate S >>> and NS register fields in CPU state structure that will me mapped to >>> separate AArch64 registers. ARMCPRegInfo structure would have additional >>> field holding NS register state filed offset for AArch32 banked registers. >> This sounds like it could work, > I largely agree, except for the need for an active set. > >> though there are some wrinkles for >> registers with readfns/writefns -- do we have extra s vs ns read/write >> functions, or just one set of functions which has to look in env->ns to >> figure out whether to use the S or NS version? > One set sounds better to me - if your resorting to open coding your > situation is probably complicated enough such that there is little you > can do in a data driven way. That said, it could be useful to define > both r.w handlers and fieldoffsets, and then the custom handlers > access do actual register read/write through a generic helper fn > (passed the CPRegInfo) that uses ->fieldoffset and is banking aware. > This handlers the common cases where helper functions are doing: > > 1: Normal access + some side effects > 2: Manipulation of the read/written value on the way in/out. > > without the need for all handlers having to open code banking functionality. > > Regards, > Peter > >>> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git >>> repository holds the most actual A64 support? >> It's still a work in progress so it depends what you want. >> a64-third-fourth-set is the last set of patches that went out for >> review, and should generally work for integer instructions. >> a64-working is my work-in-progress branch so it will have the >> most recent versions of everything, but it rebases frequently >> and is liable to occasionally be broken... >> >> thanks >> -- PMM >> Seems I agree with you. Best regards, Sergey Fedorov
Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers
On 12/20/2013 06:38 PM, Fedorov Sergey wrote: > On 12/20/2013 06:33 PM, Peter Maydell wrote: >> On 20 December 2013 14:12, Fedorov Sergey wrote: >>> I've briefly looked at the v8 ARM ARM. As I can see there is no banked >>> system control registers in AArch64. Seems the concept is changed to provide >>> separate registers for each meaningful execution level. Please, correct me >>> if I am wrong. >> Yes, I think this is generally correct. >> >>> So I think there shouldn't be "active" and "banked" fields for banked >>> AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 view >>> of system control registers as a basis. That means there would be separate S >>> and NS register fields in CPU state structure that will me mapped to >>> separate AArch64 registers. ARMCPRegInfo structure would have additional >>> field holding NS register state filed offset for AArch32 banked registers. >> This sounds like it could work, though there are some wrinkles for >> registers with readfns/writefns -- do we have extra s vs ns read/write >> functions, or just one set of functions which has to look in env->ns to >> figure out whether to use the S or NS version? > I think if most read/write functions do the same work for both S/NS > versions then this code should not be duplicated. But on the other hand, separate S/NS read/write functions could be reused for AArch64 register descriptions that is separate for each EL... > >>> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git >>> repository holds the most actual A64 support? >> It's still a work in progress so it depends what you want. >> a64-third-fourth-set is the last set of patches that went out for >> review, and should generally work for integer instructions. >> a64-working is my work-in-progress branch so it will have the >> most recent versions of everything, but it rebases frequently >> and is liable to occasionally be broken... > Thanks. > >> thanks >> -- PMM >> -- Best regards, Sergey Fedorov
Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers
On 12/20/2013 06:33 PM, Peter Maydell wrote: > On 20 December 2013 14:12, Fedorov Sergey wrote: >> I've briefly looked at the v8 ARM ARM. As I can see there is no banked >> system control registers in AArch64. Seems the concept is changed to provide >> separate registers for each meaningful execution level. Please, correct me >> if I am wrong. > Yes, I think this is generally correct. > >> So I think there shouldn't be "active" and "banked" fields for banked >> AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 view >> of system control registers as a basis. That means there would be separate S >> and NS register fields in CPU state structure that will me mapped to >> separate AArch64 registers. ARMCPRegInfo structure would have additional >> field holding NS register state filed offset for AArch32 banked registers. > This sounds like it could work, though there are some wrinkles for > registers with readfns/writefns -- do we have extra s vs ns read/write > functions, or just one set of functions which has to look in env->ns to > figure out whether to use the S or NS version? I think if most read/write functions do the same work for both S/NS versions then this code should not be duplicated. > >> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git >> repository holds the most actual A64 support? > It's still a work in progress so it depends what you want. > a64-third-fourth-set is the last set of patches that went out for > review, and should generally work for integer instructions. > a64-working is my work-in-progress branch so it will have the > most recent versions of everything, but it rebases frequently > and is liable to occasionally be broken... Thanks. > > thanks > -- PMM > -- Best regards, Sergey Fedorov
Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers
On 12/19/2013 03:38 PM, Peter Maydell wrote: On 19 December 2013 07:27, Fedorov Sergey wrote: Yes, this banking scheme makes state changing events quite heavy. But maintaining the active copies allows to keep translation table walking code untouched. I think there is a trade-off between state changing and translation table walking overheads. We shouldn't be doing tlb walks that often that it makes a difference whether we do env->ttbr0 or env->ttbr0[env->ns] ... I think the CP banking is the most fragile thing in this patch series and this should become much better after review :) It would probably be a good idea to look at the v8 ARM ARM and figure out how banked-for-NS/S registers should fit in with the AArch64 vs AArch32 split. [if you don't have a copy, it's on the ARM website: http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html you'll need to register an account on the website if you don't already have one but it's a fairly simple "fill in the form" automated process] Note in particular that: * many of the current uint32_t fields in our CPU state struct are likely to widen to uint64_t, so the AArch64 representation is canonical, and the AArch32 register accessors access a part of that state (typically the lower 32 bits) * registers which are banked S/NS in AArch32 are not necessarily banked in AArch64 AArch64 support is likely to land before your TrustZone stuff does so we need to make the two features work together cleanly. thanks -- PMM I've briefly looked at the v8 ARM ARM. As I can see there is no banked system control registers in AArch64. Seems the concept is changed to provide separate registers for each meaningful execution level. Please, correct me if I am wrong. So I think there shouldn't be "active" and "banked" fields for banked AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 view of system control registers as a basis. That means there would be separate S and NS register fields in CPU state structure that will me mapped to separate AArch64 registers. ARMCPRegInfo structure would have additional field holding NS register state filed offset for AArch32 banked registers. Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git repository holds the most actual A64 support? Thanks! Best regards, Sergey Fedorov
Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers
On 12/19/2013 04:44 PM, Peter Crosthwaite wrote: On Thu, Dec 19, 2013 at 9:38 PM, Peter Maydell wrote: On 19 December 2013 07:27, Fedorov Sergey wrote: Yes, this banking scheme makes state changing events quite heavy. But maintaining the active copies allows to keep translation table walking code untouched. I think there is a trade-off between state changing and translation table walking overheads. We shouldn't be doing tlb walks that often that it makes a difference whether we do env->ttbr0 or env->ttbr0[env->ns] ... I think the CP banking is the most fragile thing in this patch series and this should become much better after review :) It would probably be a good idea to look at the v8 ARM ARM and figure out how banked-for-NS/S registers should fit in with the AArch64 vs AArch32 split. [if you don't have a copy, it's on the ARM website: http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html you'll need to register an account on the website if you don't already have one but it's a fairly simple "fill in the form" automated process] Note in particular that: * many of the current uint32_t fields in our CPU state struct are likely to widen to uint64_t, so the AArch64 representation is canonical, and the AArch32 register accessors access a part of that state (typically the lower 32 bits) * registers which are banked S/NS in AArch32 are not necessarily banked in AArch64 Adding to that, are there any other reasons to bank a register other than sec-extensions? It seems like what you have implemented here is too sec specific for simply calling it "banked" (without further clarification of what you are banking for). Regards, Peter I'm not sure that I understand your question correctly but I try to answer. From ARMv7 ARM document section "B3.15.3 Classification of system control registers": "Banked system control registers have two copies, one Secure and one Non-secure." I don't know any use of term "CP15 banked registers" other that related to Security Extensions. Best regards, Sergey Fedorov AArch64 support is likely to land before your TrustZone stuff does so we need to make the two features work together cleanly. thanks -- PMM
Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers
On 12/19/2013 08:37 AM, Peter Crosthwaite wrote:
On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov wrote:
Banked coprocessor registers are switched on two cases:
1) Entering or leaving CPU monitor mode with SCR.NS bit set;
2) Setting SCR.NS bit not from CPU monitor mode
Coprocessor register banking is done similar to CPU core register
banking. Some of SCTRL bits are common for secure and non-secure state.
Translation table base masks are updated on register switch instead
of TTBCR write.
So I was rather confused as to your banking scheme until I got to this
patch. Now I see the implementation. You are mass-hot-swapping in the
active state on critical CPU state changing events.
Signed-off-by: Sergey Fedorov
---
target-arm/helper.c | 77 ++-
1 file changed, 76 insertions(+), 1 deletion(-)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index e1e9762..7bfadb0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -11,6 +11,7 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t
address,
int access_type, int is_user,
hwaddr *phys_ptr, int *prot,
target_ulong *page_size);
+static void switch_cp15_regs(CPUARMState *env, int secure);
#endif
static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
@@ -1553,6 +1554,17 @@ static int sctlr_write(CPUARMState *env, const
ARMCPRegInfo *ri, uint64_t value)
}
#ifndef CONFIG_USER_ONLY
+static int scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
+{
+if ((value & 1/*NS*/) && (env->uncached_cpsr & CPSR_M) !=
ARM_CPU_MODE_MON) {
+/* We are going to Non-secure state; switch banked system control
registers */
+switch_cp15_regs(env, 0);
+}
+
+env->cp15.c1_scr = value;
+return 0;
+}
+
static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t value)
{
@@ -1572,7 +1584,7 @@ static const ARMCPRegInfo tz_cp_reginfo[] = {
#ifndef CONFIG_USER_ONLY
{ .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
.access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
- .resetvalue = 0 },
+ .writefn = scr_write, .resetvalue = 0 },
{ .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
.access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vbar_write,
.fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
@@ -2284,6 +2296,69 @@ void switch_mode(CPUARMState *env, int mode)
env->regs[13] = env->banked_r13[i];
env->regs[14] = env->banked_r14[i];
env->spsr = env->banked_spsr[i];
+
+if ((mode == ARM_CPU_MODE_MON || old_mode == ARM_CPU_MODE_MON) &&
+(env->cp15.c1_scr & 1/*NS*/)) {
+/* We are going to change Security state; switch banked system control
registers */
+switch_cp15_regs(env, (mode == ARM_CPU_MODE_MON));
+}
+}
+
+void switch_cp15_regs(CPUARMState *env, int secure)
+{
+int i;
+
+/* Save current Security state registers */
+i = arm_is_secure(env) ? 0 : 1;
+env->cp15.banked_c0_cssel[i] = env->cp15.c0_cssel;
+env->cp15.banked_c1_sys[i] = env->cp15.c1_sys;
+env->cp15.banked_c2_base0[i] = env->cp15.c2_base0;
+env->cp15.banked_c2_base0_hi[i] = env->cp15.c2_base0_hi;
+env->cp15.banked_c2_base1[i] = env->cp15.c2_base1;
+env->cp15.banked_c2_base1_hi[i] = env->cp15.c2_base1_hi;
+env->cp15.banked_c2_control[i] = env->cp15.c2_control;
+env->cp15.banked_c3[i] = env->cp15.c3;
+env->cp15.banked_c5_data[i] = env->cp15.c5_data;
+env->cp15.banked_c5_insn[i] = env->cp15.c5_insn;
+env->cp15.banked_c6_data[i] = env->cp15.c6_data;
+env->cp15.banked_c6_insn[i] = env->cp15.c6_insn;
+env->cp15.banked_c7_par[i] = env->cp15.c7_par;
+env->cp15.banked_c7_par_hi[i] = env->cp15.c7_par_hi;
+env->cp15.banked_c13_context[i] = env->cp15.c13_context;
+env->cp15.banked_c13_fcse[i] = env->cp15.c13_fcse;
+env->cp15.banked_c13_tls1[i] = env->cp15.c13_tls1;
+env->cp15.banked_c13_tls2[i] = env->cp15.c13_tls2;
+env->cp15.banked_c13_tls3[i] = env->cp15.c13_tls3;
+
+/* Restore new Security state registers */
+i = secure ? 0 : 1;
+env->cp15.c0_cssel = env->cp15.banked_c0_cssel[i];
+/* Maintain the value of common bits */
+env->cp15.c1_sys &= 0x8204000;
+env->cp15.c1_sys |= env->cp15.banked_c1_sys[i] & ~0x8204000;
+env->cp15.c2_base0 = env->cp15.banked_c2_base0[i];
+env->cp15.c2_base0_hi = env->cp15.banked_c2_base0_hi[i];
+env->cp15.c2_base1 = env->cp15.banked_c2_base1[i];
+env->cp15.c2_base1_hi = env->cp15.banked_c2_base1_hi[i];
+{
+int maskshift;
+env->cp15.c2_control = env->cp15.banked_c2_control[i];
+maskshift = extract32(env->cp15.c2_control, 0, 3);
+env->cp15.c2_mask = ~(((uint32_t)0xu) >> maskshift);
+env->cp15.c2_b
Re: [Qemu-devel] [RFC PATCH 17/21] target-arm: use c13_context field for CONTEXTIDR
On 12/19/2013 08:31 AM, Peter Crosthwaite wrote:
On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov wrote:
Use c13_context field instead of c13_fcse for CONTEXTIDR register
definition.
This a standalone (I.E. not TZ related) bug?
Regards,
peter
Yes, I think so. Then I will submit this patch separately soon.
Best regards,
Sergey Fedorov
Signed-off-by: Sergey Fedorov
---
target-arm/helper.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 9442e08..e1e9762 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -359,7 +359,7 @@ static const ARMCPRegInfo cp_reginfo[] = {
.resetvalue = 0, .writefn = fcse_write, .raw_writefn = raw_write, },
{ .name = "CONTEXTIDR", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 =
1,
.access = PL1_RW, .type = ARM_CP_BANKED,
- .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
+ .fieldoffset = offsetof(CPUARMState, cp15.c13_context),
.resetvalue = 0, .writefn = contextidr_write, .raw_writefn = raw_write,
},
/* ??? This covers not just the impdef TLB lockdown registers but also
* some v7VMSA registers relating to TEX remap, so it is overly broad.
--
1.7.9.5
Re: [Qemu-devel] [RFC PATCH 02/21] target-arm: move SCR & VBAR into TrustZone register list
On 12/19/2013 07:12 AM, Peter Crosthwaite wrote:
On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov wrote:
Define a new ARM CP register info list for TrustZone Security Extension
feature. Register that list only for ARM cores with TrustZone support.
SCR and VBAR are security extension registers. So move them into
TrustZone feature register list.
Signed-off-by: Sergey Fedorov
---
target-arm/helper.c | 39 +--
1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3445813..a247ca0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -543,13 +543,6 @@ static int pmintenclr_write(CPUARMState *env, const
ARMCPRegInfo *ri,
return 0;
}
-static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
- uint64_t value)
-{
-env->cp15.c12_vbar = value & ~0x1Ful;
-return 0;
-}
-
static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t *value)
{
@@ -635,13 +628,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
.access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
.fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
.resetvalue = 0, .writefn = pmintenclr_write, },
-{ .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
- .access = PL1_RW, .writefn = vbar_write,
- .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
- .resetvalue = 0 },
-{ .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
- .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
- .resetvalue = 0, },
{ .name = "CCSIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
.access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE },
{ .name = "CSSELR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0,
@@ -1526,6 +1512,28 @@ static int sctlr_write(CPUARMState *env, const
ARMCPRegInfo *ri, uint64_t value)
return 0;
}
+#ifndef CONFIG_USER_ONLY
+static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+env->cp15.c12_vbar = value & ~0x1Ful;
+return 0;
+}
+#endif
+
+static const ARMCPRegInfo tz_cp_reginfo[] = {
+#ifndef CONFIG_USER_ONLY
+{ .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
+ .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
+ .resetvalue = 0 },
+{ .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
+ .access = PL1_RW, .writefn = vbar_write,
+ .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
+ .resetvalue = 0 },
+#endif
+REGINFO_SENTINEL
+};
+
void register_cp_regs_for_features(ARMCPU *cpu)
{
/* Register all the coprocessor registers based on feature bits */
@@ -1663,6 +1671,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
if (arm_feature(env, ARM_FEATURE_LPAE)) {
define_arm_cp_regs(cpu, lpae_cp_reginfo);
}
+if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
+define_arm_cp_regs(cpu, tz_cp_reginfo);
So ARM docmentation refers to these features as being conditional on
the "security extensions" option, not "trustzone". To match
documentation i think it may actually be
ARM_FEATURE_SECURITY_EXTENSIONS (or some truntaction thereof for
brevity). On what level of ARM documentation is the "trustzone" term
defined?
Regards,
Peter
The "TrustZone" term is not mentioned in ARM architecture manual. That
is a name for technology of system-wide approach to security. So
strictly speaking there should be used term "Security Extensions". I
cannot find any official truncation of this term.
Best regards,
Sergey Fedorov
+}
/* Slightly awkwardly, the OMAP and StrongARM cores need all of
* cp15 crn=0 to be writes-ignored, whereas for other cores they should
* be read-only (ie write causes UNDEF exception).
--
1.7.9.5
Re: [Qemu-devel] [PATCH] qom: fix cast results caching
On 12/17/2013 01:40 PM, Peter Crosthwaite wrote:
On Tue, Dec 17, 2013 at 7:20 PM, Sergey Fedorov wrote:
A single cast cache is used for both an object casting and a class
casting. In case of interface presence a class cast result may be not
the same pointer as opposite to an object casting. So do not cache cast
results for an object casting in a presence of interfaces.
I think this is fixed by my cast cache splitter patch which is
currently enqueued.
http://patchwork.ozlabs.org/patch/294766/
Regards,
Peter
Yes, your cache splitter looks much better. Thank you.
Best regards,
Sergey Fedorov
Signed-off-by: Sergey Fedorov
---
qom/object.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qom/object.c b/qom/object.c
index fc19cf6..f7384de 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -473,7 +473,7 @@ Object *object_dynamic_cast_assert(Object *obj, const char
*typename,
assert(obj == inst);
-if (obj && obj == inst) {
+if (obj && obj == inst && !obj->class->interfaces) {
for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
obj->class->cast_cache[i - 1] = obj->class->cast_cache[i];
}
--
1.7.9.5
Re: [Qemu-devel] [PATCH] target-arm: fix TTBCR write masking
This patch is a prerequisite for the following up TrustZone support patches. Thanks. Best regards, Sergey Fedorov On 12/10/2013 12:57 PM, Peter Maydell wrote: On 10 December 2013 06:41, Sergey Fedorov wrote: Current implementation is not accurate according to ARMv7-AR reference manual. See "B4.1.153 TTBCR, Translation Table Base Control Register, VMSA | TTBCR format when using the Long-descriptor translation table format". When LPAE feature is supported, EAE, bit[31] selects translation descriptor format and, therefore, TTBCR format. Signed-off-by: Sergey Fedorov Yeah, when I wrote that I was relying on the fact that all the valid bits in a short-format descriptor are also valid in the long-format mask, so correct guest software won't care (the other bits are UNK/SBZP). However the implementation side of UNK/SBZP requires us to treat the bits as RAZ/WI, so this patch is correct. Reviewed-by: Peter Maydell thanks -- PMM
Re: [Qemu-devel] [RFC PATCH 00/21] target-arm: add CPU core TrustZone support
On 12/04/2013 03:13 PM, Peter Maydell wrote: On 4 December 2013 10:08, Fedorov Sergey wrote: On 12/03/2013 12:48 PM, Sergey Fedorov wrote: This patch set implements a basic support of CPU core TrustZone feature. We'd like this patch series finally to be merged into mainstream. What should be done to achieve this goal? I'd like to see TZ support in mainline too. That is the most important for me now :-) So I will try these patches to get shape suitable for mainline. Thanks! The high level answer is that it needs to get code reviewed, and you need to fix issues that are raised in code review. Unfortunately my review queue is currently pretty full (it has Allwinner board support, DIGIC board support, a bunch of Cadence fixes, ARMv8 32 bit new instructions and the A64 64 bit instruction support in it, all of which are fairly big patchsets), so it may take me a little while to get to this patchset. It is on my todo list though, so it won't get forgotten :-) thanks -- PMM Best regards, Sergey Fedorov
Re: [Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode
On 12/04/2013 03:18 PM, Peter Maydell wrote: On 4 December 2013 10:58, Peter Crosthwaite wrote: So what im proposing is just a slightly more general patch. Is it really any more complicated than just applying your change pattern for the hyp mode? I think it would be, because of the wrinkle that hyp mode doesn't have a banked LR, so the existing "assume we can just translate the mode into a single index good for both LR and SP" logic would break. The minimal change if we wanted to keep VMSD bumps to a minimum would be to increase the size of the banked_spsr[] and banked_r13[] arrays to allow for Hyp mode but do nothing else (except add a comment about it I guess). If we want to bump VMSD just once for monitor + hypervisor mode then we need to add ELR_hyp register definition too. I think then it would be better to implement hypervisor mode and its special banking scheme, too. But I doubt it worth to combine these two things into one patch. The motiviation is less VMSD version bumps in ARM CPU (a place where I expect assume such version bumps to be considerable annoyance). Well, they're only a problem at the point where we start trying to support cross-version migration; we're not at that point yet... thanks -- PMM Best regards, Sergey Fedorov
Re: [Qemu-devel] [RFC PATCH 00/21] target-arm: add CPU core TrustZone support
On 12/03/2013 12:48 PM, Sergey Fedorov wrote: This patch set implements a basic support of CPU core TrustZone feature. The following major functionalities are implemented: * CPU monitor mode * Separate code translation for each secure state * CPACR & NSACR co-processor access control * Separate TLB for each secure state * Co-processor register banking * SMC instruction * FIQ/IRQ routing to monitor mode There is no support for banked co-processor register migration, save/load its VM state yet. That is an open question how to implement this functionality. Any suggestions is greatly appreciated. This patch set is a request for comments for the proof of concept. Sergey Fedorov (18): target-arm: move SCR & VBAR into TrustZone register list target-arm: adjust TTBCR for TrustZone feature target-arm: add arm_is_secure() helper target-arm: reject switching to monitor mode from non-secure state target-arm: adjust arm_current_pl() for TrustZone target-arm: adjust SCR CP15 register access rights target-arm: add non-secure Translation Block flag target-arm: implement CPACR register logic target-arm: add NSACR support target-arm: add SDER definition target-arm: split TLB for secure state target-arm: add banked coprocessor register type target-arm: convert appropriate coprocessor registers to banked type target-arm: use c13_context field for CONTEXTIDR target-arm: switch banked CP registers target-arm: add MVBAR support target-arm: implement SMC instruction target-arm: implement IRQ/FIQ routing to Monitor mode Svetlana Fedoseeva (3): target-arm: add TrustZone CPU feature target-arm: preserve RAO/WI bits of ARMv7 SCTLR target-arm: add CPU Monitor mode target-arm/cpu.c |6 +- target-arm/cpu.h | 126 target-arm/helper.c| 308 +- target-arm/machine.c | 12 +- target-arm/translate.c | 388 ++-- target-arm/translate.h |2 + 6 files changed, 585 insertions(+), 257 deletions(-) We'd like this patch series finally to be merged into mainstream. What should be done to achieve this goal? Best regards, Sergey Fedorov
Re: [Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode
On 12/03/2013 04:51 PM, Peter Maydell wrote:
On 3 December 2013 12:20, Peter Crosthwaite
wrote:
On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov wrote:
From: Svetlana Fedoseeva
Define CPU monitor mode. Adjust core registers banking. Adjust CPU VM
state info. Provide CPU mode name for monitor mode.
Signed-off-by: Svetlana Fedoseeva
Signed-off-by: Sergey Fedorov
---
target-arm/cpu.h |7 ---
target-arm/helper.c|3 +++
target-arm/machine.c | 12 ++--
target-arm/translate.c |2 +-
4 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 0b93e39..94d8bd1 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -124,9 +124,9 @@ typedef struct CPUARMState {
uint32_t spsr;
/* Banked registers. */
-uint32_t banked_spsr[6];
-uint32_t banked_r13[6];
-uint32_t banked_r14[6];
+uint32_t banked_spsr[7];
+uint32_t banked_r13[7];
+uint32_t banked_r14[7];
Are there any more modes yet to be implemented? It might save on
future VMSD version bumps if we just pad this out to its ultimate
value now.
The remaining mode defined for AArch32 which we don't
implement yet is Hyp mode, which has a banked R13 and SPSR,
but not a banked LR.
-- PMM
So should a number of banked core registers be increased more?
Personally, I'd like to keep this patch only TZ-related.
Best regards,
Sergey Fedorov
Re: [Qemu-devel] [RFC PATCH 04/21] target-arm: preserve RAO/WI bits of ARMv7 SCTLR
On 12/03/2013 04:17 PM, Peter Crosthwaite wrote:
On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov wrote:
From: Svetlana Fedoseeva
Signed-off-by: Svetlana Fedoseeva
Signed-off-by: Sergey Fedorov
---
target-arm/helper.c |4
1 file changed, 4 insertions(+)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 6642e53..d7922ad 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1507,6 +1507,10 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t
value)
{
+if (arm_feature(env, ARM_FEATURE_V7)) {
+value = value | 0x00c50078; /* This bits are RAO/WI */
Magic number. "these bits ".
Would be acceptable to substitute this magic number with "bitshifted
constants combined with bitwise or", e.g. as in vmsa_ttbcr_raw_write()?
+}
+
env->cp15.c1_sys = value;
/* ??? Lots of these bits are not implemented. */
/* This may enable/disable the MMU, so do a TLB flush. */
--
1.7.9.5
--
Best regards,
Sergey Fedorov, Junior Software Engineer,
Samsung R&D Institute Rus.
E-mail: s.fedo...@samsung.com
Re: [Qemu-devel] [RFC PATCH 03/21] target-arm: adjust TTBCR for TrustZone feature
On 12/03/2013 04:15 PM, Peter Crosthwaite wrote:
On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov wrote:
TTBCR has additional fields PD0 and PD1 when using Short-descriptor
translation table format on a CPU with TrustZone feature support.
Signed-off-by: Sergey Fedorov
---
target-arm/helper.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index a247ca0..6642e53 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1159,8 +1159,10 @@ static int vmsa_ttbcr_raw_write(CPUARMState *env, const
ARMCPRegInfo *ri,
{
int maskshift = extract32(value, 0, 3);
-if (arm_feature(env, ARM_FEATURE_LPAE)) {
+if (arm_feature(env, ARM_FEATURE_LPAE) && (value & (1 << 31))) {
This appears to be changing more than just trustzone dependent
behavior. That is, if we take just this hunk and ignore the one below
you see a change in the non-tz behaviour. Is the hunk legitimate
irrespective of trustzone support?
Yes, current implementation is not accurate according to ARMv7-AR
reference manual. See "B4.1.153 TTBCR, Translation Table Base Control
Register, VMSA | TTBCR format when using the Long-descriptor translation
table format". When LPAE feature is supported, EAE, bit[31] selects
translation descriptor format and, therefore, TTBCR format.
value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
+} else if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
+value &= 0x37;
} else {
value &= 7;
}
There are a few magic numbers in the patch probably worth macrofiying.
As I can see, magic numbers are widely used through all of this file to
represent CP register fields and other things. Maybe the macrofying
should be done separately from this patch series?
Regards,
Peter
--
1.7.9.5
Best regards,
Sergey Fedorov
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 10/29/2013 06:55 PM, Stefan Hajnoczi wrote:
On Mon, Oct 21, 2013 at 03:44:46PM +0400, Fedorov Sergey wrote:
After our discussion about this patch I decided to keep my patch in
our branch until rebase onto a new release. Recently I have rebased
our branch onto v1.5.3 and reverted my patch. Then I face an issue
when using user-mode networking with USB network device for mounting
root file system through NFS. Fragmented UDP packets from host to
guest does not handled properly. Seems that some fragments is lost
or somehow stalled. See guest tcpdump log below.
03:16:52.259690 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF],
proto UDP (17), length 164)
10.0.2.15.3369105030 > 10.0.2.2.nfs: 136 readdirplus fh
Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C
512 bytes @ 0 max 4096 verf
03:16:52.262323 IP (tos 0x0, ttl 64, id 16, offset 0, flags [+],
proto UDP (17), length 1500)
10.0.2.2.nfs > 10.0.2.15.3369105030: reply ok 1472 readdirplus
POST: DIR 40777 ids 0/0 sz 4096 verf
03:16:52.264592 IP (tos 0x0, ttl 64, id 16, offset 1480, flags [+],
proto UDP (17), length 1500)
10.0.2.2 > 10.0.2.15: udp
03:16:54.462961 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF],
proto UDP (17), length 164)
10.0.2.15.3369105030 > 10.0.2.2.nfs: 136 readdirplus fh
Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C
512 bytes @ 0 max 4096 verf
03:16:54.466300 IP (tos 0x0, ttl 64, id 17, offset 0, flags [+],
proto UDP (17), length 1500)
10.0.2.2.nfs > 10.0.2.15.3369105030: reply ok 1472 readdirplus
POST: DIR 40777 ids 0/0 sz 4096 verf
03:16:54.467084 IP (tos 0x0, ttl 64, id 17, offset 1480, flags [+],
proto UDP (17), length 1500)
10.0.2.2 > 10.0.2.15: udp
...
I didn't investigate the cause of the problem in detail. I just reverted
commit 199ee608f0d08510b5c6c37f31a7fbff211d63c4
Author: Luigi Rizzo
Date: Tue Feb 5 17:53:31 2013 +0100
net: fix qemu_flush_queued_packets() in presence of a hub
And then applied my patch. After that everything works fine for me.
See guest tcpdump log below.
04:45:15.897245 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF],
proto UDP (17), length 164)
10.0.2.15.3642011847 > 10.0.2.2.nfs: 136 readdirplus fh
Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C
512 bytes @ 0 max 4096 verf
04:45:15.899686 IP (tos 0x0, ttl 64, id 15, offset 0, flags [+],
proto UDP (17), length 1500)
10.0.2.2.nfs > 10.0.2.15.3642011847: reply ok 1472 readdirplus
POST: DIR 40777 ids 0/0 sz 4096 verf
04:45:15.906253 IP (tos 0x0, ttl 64, id 15, offset 1480, flags [+],
proto UDP (17), length 1500)
10.0.2.2 > 10.0.2.15: udp
04:45:15.906687 IP (tos 0x0, ttl 64, id 15, offset 2960, flags
[none], proto UDP (17), length 240)
10.0.2.2 > 10.0.2.15: udp
So there must be something wrong with already applied patch. What
could you suggest?
The next step is to investigate the cause.
Perhaps hw/usb/dev-network.c:usb_net_handle_datain() is not calling
qemu_flush_queued_packets() every time in_buf[] is read completely.
This if statement looks strange to me:
if (s->in_ptr >= s->in_len &&
(is_rndis(s) || (s->in_len & (64 - 1)) || !len)) {
/* no short packet necessary */
usb_net_reset_in_buf(s);
}
Try placing printfs to find out whether qemu_flush_queued_packets() is
getting called when you see packet loss.
Stefan
Seems that I have figured out the problem. net_hub_flush() does not
flush source port. And qemu_flush_queued_packets() also returns after
calling net_hub_flush(). So I think the problem is that neither
qemu_flush_queued_packets() nor net_hub_flush() call
qemu_net_queue_flush() for the source port. So I think it sould be fixed
in qemu_flush_queued_packets() by removing the return statement after
calling net_hub_flush(). That fix does work for me. So I could submit
that patch after getting permission for that.
--
Best regards,
Sergey Fedorov, Junior Software Engineer,
Samsung R&D Institute Rus.
E-mail: s.fedo...@samsung.com
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 10/21/2013 03:52 PM, Fedorov Sergey wrote: On 10/21/2013 03:44 PM, Fedorov Sergey wrote: On 04/23/2013 04:00 PM, Stefan Hajnoczi wrote: On Tue, Apr 23, 2013 at 11:41:42AM +0400, Fedorov Sergey wrote: Beyond that, we also want to avoid growing net queues indefinitely. If the hub does not implement .can_receive() then it relies on growing queues (keeping packets buffered in memory). No, net_hub_receive() calls qemu_send_packet(). If the destination queue cannot receive the packet qemu_net_queue_append() will take care of queue->nq_maxlen. You are right, sorry. We do discard packets at nq_maxlen. The problem with ignoring .can_receive() on the hub is that it breaks flow control. For example, net/tap.c is designed to avoid reading more packets if its peer cannot receive (see tap_can_send()). If the hub claims it can always receive we waste cycles reading packets from the tap device only to discard them. Since qemu.git already has a fix which preserves flow control, I am not going to merge your patch. Stefan Dear, Stefan Hajnoczi, After our discussion about this patch I decided to keep my patch in our branch until rebase onto a new release. Recently I have rebased our branch onto v1.5.3 and reverted my patch. Then I face an issue when using user-mode networking with USB network device for mounting root file system through NFS. Fragmented UDP packets from host to guest does not handled properly. Seems that some fragments is lost or somehow stalled. See guest tcpdump log below. 03:16:52.259690 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 164) 10.0.2.15.3369105030 > 10.0.2.2.nfs: 136 readdirplus fh Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C 512 bytes @ 0 max 4096 verf 03:16:52.262323 IP (tos 0x0, ttl 64, id 16, offset 0, flags [+], proto UDP (17), length 1500) 10.0.2.2.nfs > 10.0.2.15.3369105030: reply ok 1472 readdirplus POST: DIR 40777 ids 0/0 sz 4096 verf 03:16:52.264592 IP (tos 0x0, ttl 64, id 16, offset 1480, flags [+], proto UDP (17), length 1500) 10.0.2.2 > 10.0.2.15: udp 03:16:54.462961 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 164) 10.0.2.15.3369105030 > 10.0.2.2.nfs: 136 readdirplus fh Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C 512 bytes @ 0 max 4096 verf 03:16:54.466300 IP (tos 0x0, ttl 64, id 17, offset 0, flags [+], proto UDP (17), length 1500) 10.0.2.2.nfs > 10.0.2.15.3369105030: reply ok 1472 readdirplus POST: DIR 40777 ids 0/0 sz 4096 verf 03:16:54.467084 IP (tos 0x0, ttl 64, id 17, offset 1480, flags [+], proto UDP (17), length 1500) 10.0.2.2 > 10.0.2.15: udp ... I didn't investigate the cause of the problem in detail. I just reverted commit 199ee608f0d08510b5c6c37f31a7fbff211d63c4 Author: Luigi Rizzo Date: Tue Feb 5 17:53:31 2013 +0100 net: fix qemu_flush_queued_packets() in presence of a hub And then applied my patch. After that everything works fine for me. See guest tcpdump log below. 04:45:15.897245 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 164) 10.0.2.15.3642011847 > 10.0.2.2.nfs: 136 readdirplus fh Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C 512 bytes @ 0 max 4096 verf 04:45:15.899686 IP (tos 0x0, ttl 64, id 15, offset 0, flags [+], proto UDP (17), length 1500) 10.0.2.2.nfs > 10.0.2.15.3642011847: reply ok 1472 readdirplus POST: DIR 40777 ids 0/0 sz 4096 verf 04:45:15.906253 IP (tos 0x0, ttl 64, id 15, offset 1480, flags [+], proto UDP (17), length 1500) 10.0.2.2 > 10.0.2.15: udp 04:45:15.906687 IP (tos 0x0, ttl 64, id 15, offset 2960, flags [none], proto UDP (17), length 240) 10.0.2.2 > 10.0.2.15: udp So there must be something wrong with already applied patch. What could you suggest? Sorry, I missed that Anthony Liguori email address has been changed. So I resend the email. Ping. -- Best regards, Sergey Fedorov, Junior Software Engineer, Samsung R&D Institute Rus. E-mail: s.fedo...@samsung.com
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 10/21/2013 03:44 PM, Fedorov Sergey wrote: On 04/23/2013 04:00 PM, Stefan Hajnoczi wrote: On Tue, Apr 23, 2013 at 11:41:42AM +0400, Fedorov Sergey wrote: Beyond that, we also want to avoid growing net queues indefinitely. If the hub does not implement .can_receive() then it relies on growing queues (keeping packets buffered in memory). No, net_hub_receive() calls qemu_send_packet(). If the destination queue cannot receive the packet qemu_net_queue_append() will take care of queue->nq_maxlen. You are right, sorry. We do discard packets at nq_maxlen. The problem with ignoring .can_receive() on the hub is that it breaks flow control. For example, net/tap.c is designed to avoid reading more packets if its peer cannot receive (see tap_can_send()). If the hub claims it can always receive we waste cycles reading packets from the tap device only to discard them. Since qemu.git already has a fix which preserves flow control, I am not going to merge your patch. Stefan Dear, Stefan Hajnoczi, After our discussion about this patch I decided to keep my patch in our branch until rebase onto a new release. Recently I have rebased our branch onto v1.5.3 and reverted my patch. Then I face an issue when using user-mode networking with USB network device for mounting root file system through NFS. Fragmented UDP packets from host to guest does not handled properly. Seems that some fragments is lost or somehow stalled. See guest tcpdump log below. 03:16:52.259690 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 164) 10.0.2.15.3369105030 > 10.0.2.2.nfs: 136 readdirplus fh Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C 512 bytes @ 0 max 4096 verf 03:16:52.262323 IP (tos 0x0, ttl 64, id 16, offset 0, flags [+], proto UDP (17), length 1500) 10.0.2.2.nfs > 10.0.2.15.3369105030: reply ok 1472 readdirplus POST: DIR 40777 ids 0/0 sz 4096 verf 03:16:52.264592 IP (tos 0x0, ttl 64, id 16, offset 1480, flags [+], proto UDP (17), length 1500) 10.0.2.2 > 10.0.2.15: udp 03:16:54.462961 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 164) 10.0.2.15.3369105030 > 10.0.2.2.nfs: 136 readdirplus fh Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C 512 bytes @ 0 max 4096 verf 03:16:54.466300 IP (tos 0x0, ttl 64, id 17, offset 0, flags [+], proto UDP (17), length 1500) 10.0.2.2.nfs > 10.0.2.15.3369105030: reply ok 1472 readdirplus POST: DIR 40777 ids 0/0 sz 4096 verf 03:16:54.467084 IP (tos 0x0, ttl 64, id 17, offset 1480, flags [+], proto UDP (17), length 1500) 10.0.2.2 > 10.0.2.15: udp ... I didn't investigate the cause of the problem in detail. I just reverted commit 199ee608f0d08510b5c6c37f31a7fbff211d63c4 Author: Luigi Rizzo Date: Tue Feb 5 17:53:31 2013 +0100 net: fix qemu_flush_queued_packets() in presence of a hub And then applied my patch. After that everything works fine for me. See guest tcpdump log below. 04:45:15.897245 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 164) 10.0.2.15.3642011847 > 10.0.2.2.nfs: 136 readdirplus fh Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C 512 bytes @ 0 max 4096 verf 04:45:15.899686 IP (tos 0x0, ttl 64, id 15, offset 0, flags [+], proto UDP (17), length 1500) 10.0.2.2.nfs > 10.0.2.15.3642011847: reply ok 1472 readdirplus POST: DIR 40777 ids 0/0 sz 4096 verf 04:45:15.906253 IP (tos 0x0, ttl 64, id 15, offset 1480, flags [+], proto UDP (17), length 1500) 10.0.2.2 > 10.0.2.15: udp 04:45:15.906687 IP (tos 0x0, ttl 64, id 15, offset 2960, flags [none], proto UDP (17), length 240) 10.0.2.2 > 10.0.2.15: udp So there must be something wrong with already applied patch. What could you suggest? Sorry, I missed that Anthony Liguori email address has been changed. So I resend the email. -- Best regards, Sergey Fedorov, Junior Software Engineer, Samsung R&D Institute Rus. E-mail: s.fedo...@samsung.com
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 04/23/2013 04:00 PM, Stefan Hajnoczi wrote: On Tue, Apr 23, 2013 at 11:41:42AM +0400, Fedorov Sergey wrote: Beyond that, we also want to avoid growing net queues indefinitely. If the hub does not implement .can_receive() then it relies on growing queues (keeping packets buffered in memory). No, net_hub_receive() calls qemu_send_packet(). If the destination queue cannot receive the packet qemu_net_queue_append() will take care of queue->nq_maxlen. You are right, sorry. We do discard packets at nq_maxlen. The problem with ignoring .can_receive() on the hub is that it breaks flow control. For example, net/tap.c is designed to avoid reading more packets if its peer cannot receive (see tap_can_send()). If the hub claims it can always receive we waste cycles reading packets from the tap device only to discard them. Since qemu.git already has a fix which preserves flow control, I am not going to merge your patch. Stefan Dear, Stefan Hajnoczi, After our discussion about this patch I decided to keep my patch in our branch until rebase onto a new release. Recently I have rebased our branch onto v1.5.3 and reverted my patch. Then I face an issue when using user-mode networking with USB network device for mounting root file system through NFS. Fragmented UDP packets from host to guest does not handled properly. Seems that some fragments is lost or somehow stalled. See guest tcpdump log below. 03:16:52.259690 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 164) 10.0.2.15.3369105030 > 10.0.2.2.nfs: 136 readdirplus fh Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C 512 bytes @ 0 max 4096 verf 03:16:52.262323 IP (tos 0x0, ttl 64, id 16, offset 0, flags [+], proto UDP (17), length 1500) 10.0.2.2.nfs > 10.0.2.15.3369105030: reply ok 1472 readdirplus POST: DIR 40777 ids 0/0 sz 4096 verf 03:16:52.264592 IP (tos 0x0, ttl 64, id 16, offset 1480, flags [+], proto UDP (17), length 1500) 10.0.2.2 > 10.0.2.15: udp 03:16:54.462961 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 164) 10.0.2.15.3369105030 > 10.0.2.2.nfs: 136 readdirplus fh Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C 512 bytes @ 0 max 4096 verf 03:16:54.466300 IP (tos 0x0, ttl 64, id 17, offset 0, flags [+], proto UDP (17), length 1500) 10.0.2.2.nfs > 10.0.2.15.3369105030: reply ok 1472 readdirplus POST: DIR 40777 ids 0/0 sz 4096 verf 03:16:54.467084 IP (tos 0x0, ttl 64, id 17, offset 1480, flags [+], proto UDP (17), length 1500) 10.0.2.2 > 10.0.2.15: udp ... I didn't investigate the cause of the problem in detail. I just reverted commit 199ee608f0d08510b5c6c37f31a7fbff211d63c4 Author: Luigi Rizzo Date: Tue Feb 5 17:53:31 2013 +0100 net: fix qemu_flush_queued_packets() in presence of a hub And then applied my patch. After that everything works fine for me. See guest tcpdump log below. 04:45:15.897245 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 164) 10.0.2.15.3642011847 > 10.0.2.2.nfs: 136 readdirplus fh Unknown/01000700040012002873593C9B3C43388E23748B0BAD870C 512 bytes @ 0 max 4096 verf 04:45:15.899686 IP (tos 0x0, ttl 64, id 15, offset 0, flags [+], proto UDP (17), length 1500) 10.0.2.2.nfs > 10.0.2.15.3642011847: reply ok 1472 readdirplus POST: DIR 40777 ids 0/0 sz 4096 verf 04:45:15.906253 IP (tos 0x0, ttl 64, id 15, offset 1480, flags [+], proto UDP (17), length 1500) 10.0.2.2 > 10.0.2.15: udp 04:45:15.906687 IP (tos 0x0, ttl 64, id 15, offset 2960, flags [none], proto UDP (17), length 240) 10.0.2.2 > 10.0.2.15: udp So there must be something wrong with already applied patch. What could you suggest? -- Best regards, Sergey Fedorov, Junior Software Engineer, Samsung R&D Institute Rus. E-mail: s.fedo...@samsung.com
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 04/23/2013 04:00 PM, Stefan Hajnoczi wrote: On Tue, Apr 23, 2013 at 11:41:42AM +0400, Fedorov Sergey wrote: Beyond that, we also want to avoid growing net queues indefinitely. If the hub does not implement .can_receive() then it relies on growing queues (keeping packets buffered in memory). No, net_hub_receive() calls qemu_send_packet(). If the destination queue cannot receive the packet qemu_net_queue_append() will take care of queue->nq_maxlen. You are right, sorry. We do discard packets at nq_maxlen. The problem with ignoring .can_receive() on the hub is that it breaks flow control. For example, net/tap.c is designed to avoid reading more packets if its peer cannot receive (see tap_can_send()). If the hub claims it can always receive we waste cycles reading packets from the tap device only to discard them. Since qemu.git already has a fix which preserves flow control, I am not going to merge your patch. Stefan OK, I see. Thanks for attention. -- Best regards, Sergey Fedorov, Junior Software Engineer, Samsung R&D Institute Rus. E-mail: s.fedo...@samsung.com
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 04/23/2013 03:48 PM, Stefan Hajnoczi wrote: On Tue, Apr 23, 2013 at 01:32:11PM +0400, Fedorov Sergey wrote: On 04/23/2013 10:58 AM, Stefan Hajnoczi wrote: On Mon, Apr 22, 2013 at 07:27:21PM +0400, Fedorov Sergey wrote: On 04/22/2013 06:57 PM, Stefan Hajnoczi wrote: On Mon, Apr 22, 2013 at 04:26:16PM +0400, Fedorov Sergey wrote: On 04/22/2013 03:47 PM, Stefan Hajnoczi wrote: On Thu, Apr 18, 2013 at 03:31:55PM +0400, Sergey Fedorov wrote: Network hub should always receive incoming packets. Then forward them to the appropriate port queue and let the qemu_send_packet() do the right things. If the destination queue cannot receive the packet it will be appended to the queue. When the receiver call qemu_flush_queued_packets() later the queue will be really flushed and no packets will be stalled in the sender network queue. Signed-off-by: Sergey Fedorov --- net/hub.c | 20 1 file changed, 20 deletions(-) What is the point of this change? There is no semantic difference for well-behaved net clients. Does it fix a bug, if so, please include details? Stefan Yes, this fixes a bug. There were packet stalls when using user-mode networking with USB network device. slirp_output() calls qemu_send_packet() which eventually calls qemu_net_queue_send(). qemu_net_queue_send() calls qemu_can_send_packet(), which calls can_receive() callback of network hub. Then net_hub_port_can_receive() also calls qemu_can_send_packet() for each port except packet source port. Sometimes USB network device is not able to receive packet and qemu_can_send_packet() returns false. In my case there is no more ports and net_hub_port_can_receive() returns false. So qemu_net_queue_send() call qemu_net_queue_append() instead of qemu_net_queue_deliver(). qemu_net_queue_append() appends the packet to the receiving port of the network hub which is not flushed when USB netork device calls qemu_flush_queued_packets(). It is flushed only when slirp resend the packet by timeout. Actually there is no need in net_hub_port_can_receive() as the network hub can always receive packets and pass it to its port network clients with qemu_send_packet(). And if the destination port network client cannot receive the packet it will be queued in the *destination* port network client queue. Queued packets from that queue will be delivered as soon as the network client call qemu_flush_queued_packets(). Please confirm the bug is still present in qemu.git/master. It should have been fixed by the following commit: commit 199ee608f0d08510b5c6c37f31a7fbff211d63c4 Author: Luigi Rizzo Date: Tue Feb 5 17:53:31 2013 +0100 net: fix qemu_flush_queued_packets() in presence of a hub Stefan Yes, this commit fixes a bug but from other side. I think it's better to just let the qemu_send_packet() do the right things. E.g. network hub has 3 ports. Suppose when iterating through port list in net_hub_port_can_receive() a packet is successfully delivered to the first port, and then is queued in the source port queue because the second port cannot receive packets. Later net_hub_flush() will flush the packet from the source port queue and it will be delivered in every port. But it had been already delivered to one of them. So it will be delivered twice to some ports. Moreover there is less chance to dequeue the packet if several clients can't receive periodically. Did you mean "second port" instead of "source port" in the beginning? I don't see a scenario where the packet is delivered to the same port twice. If one port can receive then the packet will be sent and other ports will queue the packet. Whether dump network client can always receive? If so, then the packet will always be sent to the dump client regardless of whether the other clients can receive. Is there actually any synchronization? Yes, the dump net client can always receive. Other net clients may not. There is no explicit synchronization between send queues on the hub. Stefan So why don't we apply this patch? -- Best regards, Sergey Fedorov, Junior Software Engineer, Samsung R&D Institute Rus. E-mail: s.fedo...@samsung.com
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 04/23/2013 10:58 AM, Stefan Hajnoczi wrote: On Mon, Apr 22, 2013 at 07:27:21PM +0400, Fedorov Sergey wrote: On 04/22/2013 06:57 PM, Stefan Hajnoczi wrote: On Mon, Apr 22, 2013 at 04:26:16PM +0400, Fedorov Sergey wrote: On 04/22/2013 03:47 PM, Stefan Hajnoczi wrote: On Thu, Apr 18, 2013 at 03:31:55PM +0400, Sergey Fedorov wrote: Network hub should always receive incoming packets. Then forward them to the appropriate port queue and let the qemu_send_packet() do the right things. If the destination queue cannot receive the packet it will be appended to the queue. When the receiver call qemu_flush_queued_packets() later the queue will be really flushed and no packets will be stalled in the sender network queue. Signed-off-by: Sergey Fedorov --- net/hub.c | 20 1 file changed, 20 deletions(-) What is the point of this change? There is no semantic difference for well-behaved net clients. Does it fix a bug, if so, please include details? Stefan Yes, this fixes a bug. There were packet stalls when using user-mode networking with USB network device. slirp_output() calls qemu_send_packet() which eventually calls qemu_net_queue_send(). qemu_net_queue_send() calls qemu_can_send_packet(), which calls can_receive() callback of network hub. Then net_hub_port_can_receive() also calls qemu_can_send_packet() for each port except packet source port. Sometimes USB network device is not able to receive packet and qemu_can_send_packet() returns false. In my case there is no more ports and net_hub_port_can_receive() returns false. So qemu_net_queue_send() call qemu_net_queue_append() instead of qemu_net_queue_deliver(). qemu_net_queue_append() appends the packet to the receiving port of the network hub which is not flushed when USB netork device calls qemu_flush_queued_packets(). It is flushed only when slirp resend the packet by timeout. Actually there is no need in net_hub_port_can_receive() as the network hub can always receive packets and pass it to its port network clients with qemu_send_packet(). And if the destination port network client cannot receive the packet it will be queued in the *destination* port network client queue. Queued packets from that queue will be delivered as soon as the network client call qemu_flush_queued_packets(). Please confirm the bug is still present in qemu.git/master. It should have been fixed by the following commit: commit 199ee608f0d08510b5c6c37f31a7fbff211d63c4 Author: Luigi Rizzo Date: Tue Feb 5 17:53:31 2013 +0100 net: fix qemu_flush_queued_packets() in presence of a hub Stefan Yes, this commit fixes a bug but from other side. I think it's better to just let the qemu_send_packet() do the right things. E.g. network hub has 3 ports. Suppose when iterating through port list in net_hub_port_can_receive() a packet is successfully delivered to the first port, and then is queued in the source port queue because the second port cannot receive packets. Later net_hub_flush() will flush the packet from the source port queue and it will be delivered in every port. But it had been already delivered to one of them. So it will be delivered twice to some ports. Moreover there is less chance to dequeue the packet if several clients can't receive periodically. Did you mean "second port" instead of "source port" in the beginning? I don't see a scenario where the packet is delivered to the same port twice. If one port can receive then the packet will be sent and other ports will queue the packet. Whether dump network client can always receive? If so, then the packet will always be sent to the dump client regardless of whether the other clients can receive. Is there actually any synchronization? Note that the source port will not queue the packet. When the blocked ports can receive again their send queues will be flushed. Paolo mentioned that we want to provide a consistent view of packets, especially when -net dump is used. Beyond that, we also want to avoid growing net queues indefinitely. If the hub does not implement .can_receive() then it relies on growing queues (keeping packets buffered in memory). Stefan -- Best regards, Sergey Fedorov, Junior Software Engineer, Samsung R&D Institute Rus. E-mail: s.fedo...@samsung.com
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 04/23/2013 10:58 AM, Stefan Hajnoczi wrote: On Mon, Apr 22, 2013 at 07:27:21PM +0400, Fedorov Sergey wrote: On 04/22/2013 06:57 PM, Stefan Hajnoczi wrote: On Mon, Apr 22, 2013 at 04:26:16PM +0400, Fedorov Sergey wrote: On 04/22/2013 03:47 PM, Stefan Hajnoczi wrote: On Thu, Apr 18, 2013 at 03:31:55PM +0400, Sergey Fedorov wrote: Network hub should always receive incoming packets. Then forward them to the appropriate port queue and let the qemu_send_packet() do the right things. If the destination queue cannot receive the packet it will be appended to the queue. When the receiver call qemu_flush_queued_packets() later the queue will be really flushed and no packets will be stalled in the sender network queue. Signed-off-by: Sergey Fedorov --- net/hub.c | 20 1 file changed, 20 deletions(-) What is the point of this change? There is no semantic difference for well-behaved net clients. Does it fix a bug, if so, please include details? Stefan Yes, this fixes a bug. There were packet stalls when using user-mode networking with USB network device. slirp_output() calls qemu_send_packet() which eventually calls qemu_net_queue_send(). qemu_net_queue_send() calls qemu_can_send_packet(), which calls can_receive() callback of network hub. Then net_hub_port_can_receive() also calls qemu_can_send_packet() for each port except packet source port. Sometimes USB network device is not able to receive packet and qemu_can_send_packet() returns false. In my case there is no more ports and net_hub_port_can_receive() returns false. So qemu_net_queue_send() call qemu_net_queue_append() instead of qemu_net_queue_deliver(). qemu_net_queue_append() appends the packet to the receiving port of the network hub which is not flushed when USB netork device calls qemu_flush_queued_packets(). It is flushed only when slirp resend the packet by timeout. Actually there is no need in net_hub_port_can_receive() as the network hub can always receive packets and pass it to its port network clients with qemu_send_packet(). And if the destination port network client cannot receive the packet it will be queued in the *destination* port network client queue. Queued packets from that queue will be delivered as soon as the network client call qemu_flush_queued_packets(). Please confirm the bug is still present in qemu.git/master. It should have been fixed by the following commit: commit 199ee608f0d08510b5c6c37f31a7fbff211d63c4 Author: Luigi Rizzo Date: Tue Feb 5 17:53:31 2013 +0100 net: fix qemu_flush_queued_packets() in presence of a hub Stefan Yes, this commit fixes a bug but from other side. I think it's better to just let the qemu_send_packet() do the right things. E.g. network hub has 3 ports. Suppose when iterating through port list in net_hub_port_can_receive() a packet is successfully delivered to the first port, and then is queued in the source port queue because the second port cannot receive packets. Later net_hub_flush() will flush the packet from the source port queue and it will be delivered in every port. But it had been already delivered to one of them. So it will be delivered twice to some ports. Moreover there is less chance to dequeue the packet if several clients can't receive periodically. Did you mean "second port" instead of "source port" in the beginning? I don't see a scenario where the packet is delivered to the same port twice. If one port can receive then the packet will be sent and other ports will queue the packet. I agree. I was wrong with that. Note that the source port will not queue the packet. The source port will queue the packet if no port, except the source one, can receive. When the blocked ports can receive again their send queues will be flushed. Paolo mentioned that we want to provide a consistent view of packets, especially when -net dump is used. Beyond that, we also want to avoid growing net queues indefinitely. If the hub does not implement .can_receive() then it relies on growing queues (keeping packets buffered in memory). No, net_hub_receive() calls qemu_send_packet(). If the destination queue cannot receive the packet qemu_net_queue_append() will take care of queue->nq_maxlen. Stefan -- Best regards, Sergey Fedorov, Junior Software Engineer, Samsung R&D Institute Rus. E-mail: s.fedo...@samsung.com
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 04/22/2013 08:09 PM, Paolo Bonzini wrote: Il 22/04/2013 17:27, Fedorov Sergey ha scritto: E.g. network hub has 3 ports. Suppose when iterating through port list in net_hub_port_can_receive() a packet is successfully delivered to the first port, and then is queued in the source port queue because the second port cannot receive packets. Later net_hub_flush() will flush the packet from the source port queue and it will be delivered in every port. But it had been already delivered to one of them. So it will be delivered twice to some ports. Moreover there is less chance to dequeue the packet if several clients can't receive periodically. Perhaps it is indeed wrong to do this blocking in can_receive()... You're right that a hubport can always receive, but the hub itself should have a queue. If one port cannot receive, the packet should be appended to the hub's queue. And net_hub_flush will just go through the hub's queue. Anyway, actually there is no need in net_hub_port_can_receive() as the network hub can always receive packets and pass it to its port network clients with qemu_send_packet(). I think it's more natural solution. I think the point was to keep dumps in sync with what actually happened on the other ports. Otherwise a "-net dump" port will show the packet immediately, even though it hasn't been delivered yet. Paolo The user documentation says that `-net dump' dumps network traffic on VLAN. There is nothing said about interfaces connected to VALN nor synchronization with them. In my view, VLAN hub is a device that forwards an incoming packet to all other ports as soon as possible, just like real hardware hub. I think of `-net dump' as a special network interface that dumps every packet received by VLAN hub itself, not delivered to its clients. If we want to dump packets actually delivered to network interfaces we need to do it in per-interface manner. Maybe I misunderstand? -- Best regards, Sergey Fedorov, Junior Software Engineer, Samsung R&D Institute Rus. E-mail: s.fedo...@samsung.com
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 04/22/2013 06:57 PM, Stefan Hajnoczi wrote: On Mon, Apr 22, 2013 at 04:26:16PM +0400, Fedorov Sergey wrote: On 04/22/2013 03:47 PM, Stefan Hajnoczi wrote: On Thu, Apr 18, 2013 at 03:31:55PM +0400, Sergey Fedorov wrote: Network hub should always receive incoming packets. Then forward them to the appropriate port queue and let the qemu_send_packet() do the right things. If the destination queue cannot receive the packet it will be appended to the queue. When the receiver call qemu_flush_queued_packets() later the queue will be really flushed and no packets will be stalled in the sender network queue. Signed-off-by: Sergey Fedorov --- net/hub.c | 20 1 file changed, 20 deletions(-) What is the point of this change? There is no semantic difference for well-behaved net clients. Does it fix a bug, if so, please include details? Stefan Yes, this fixes a bug. There were packet stalls when using user-mode networking with USB network device. slirp_output() calls qemu_send_packet() which eventually calls qemu_net_queue_send(). qemu_net_queue_send() calls qemu_can_send_packet(), which calls can_receive() callback of network hub. Then net_hub_port_can_receive() also calls qemu_can_send_packet() for each port except packet source port. Sometimes USB network device is not able to receive packet and qemu_can_send_packet() returns false. In my case there is no more ports and net_hub_port_can_receive() returns false. So qemu_net_queue_send() call qemu_net_queue_append() instead of qemu_net_queue_deliver(). qemu_net_queue_append() appends the packet to the receiving port of the network hub which is not flushed when USB netork device calls qemu_flush_queued_packets(). It is flushed only when slirp resend the packet by timeout. Actually there is no need in net_hub_port_can_receive() as the network hub can always receive packets and pass it to its port network clients with qemu_send_packet(). And if the destination port network client cannot receive the packet it will be queued in the *destination* port network client queue. Queued packets from that queue will be delivered as soon as the network client call qemu_flush_queued_packets(). Please confirm the bug is still present in qemu.git/master. It should have been fixed by the following commit: commit 199ee608f0d08510b5c6c37f31a7fbff211d63c4 Author: Luigi Rizzo Date: Tue Feb 5 17:53:31 2013 +0100 net: fix qemu_flush_queued_packets() in presence of a hub Stefan Yes, this commit fixes a bug but from other side. I think it's better to just let the qemu_send_packet() do the right things. E.g. network hub has 3 ports. Suppose when iterating through port list in net_hub_port_can_receive() a packet is successfully delivered to the first port, and then is queued in the source port queue because the second port cannot receive packets. Later net_hub_flush() will flush the packet from the source port queue and it will be delivered in every port. But it had been already delivered to one of them. So it will be delivered twice to some ports. Moreover there is less chance to dequeue the packet if several clients can't receive periodically. Anyway, actually there is no need in net_hub_port_can_receive() as the network hub can always receive packets and pass it to its port network clients with qemu_send_packet(). I think it's more natural solution. Sergey
Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler
On 04/22/2013 03:47 PM, Stefan Hajnoczi wrote: On Thu, Apr 18, 2013 at 03:31:55PM +0400, Sergey Fedorov wrote: Network hub should always receive incoming packets. Then forward them to the appropriate port queue and let the qemu_send_packet() do the right things. If the destination queue cannot receive the packet it will be appended to the queue. When the receiver call qemu_flush_queued_packets() later the queue will be really flushed and no packets will be stalled in the sender network queue. Signed-off-by: Sergey Fedorov --- net/hub.c | 20 1 file changed, 20 deletions(-) What is the point of this change? There is no semantic difference for well-behaved net clients. Does it fix a bug, if so, please include details? Stefan Yes, this fixes a bug. There were packet stalls when using user-mode networking with USB network device. slirp_output() calls qemu_send_packet() which eventually calls qemu_net_queue_send(). qemu_net_queue_send() calls qemu_can_send_packet(), which calls can_receive() callback of network hub. Then net_hub_port_can_receive() also calls qemu_can_send_packet() for each port except packet source port. Sometimes USB network device is not able to receive packet and qemu_can_send_packet() returns false. In my case there is no more ports and net_hub_port_can_receive() returns false. So qemu_net_queue_send() call qemu_net_queue_append() instead of qemu_net_queue_deliver(). qemu_net_queue_append() appends the packet to the receiving port of the network hub which is not flushed when USB netork device calls qemu_flush_queued_packets(). It is flushed only when slirp resend the packet by timeout. Actually there is no need in net_hub_port_can_receive() as the network hub can always receive packets and pass it to its port network clients with qemu_send_packet(). And if the destination port network client cannot receive the packet it will be queued in the *destination* port network client queue. Queued packets from that queue will be delivered as soon as the network client call qemu_flush_queued_packets(). -- Best regards, Sergey Fedorov, Junior Software Engineer, Samsung R&D Institute Rus. E-mail: s.fedo...@samsung.com
