On Tue, May 20, 2014 at 11:39:12PM +0800, Haojian Zhuang wrote:

On 20 May 2014 23:05, Christoffer Dall [off-list ref] wrote:

quoted

On Tue, May 20, 2014 at 10:16:22PM +0800, Haojian Zhuang wrote:

quoted

On 20 May 2014 22:01, Christoffer Dall [off-list ref] wrote:
It's the implementation of gich_apr in arm32.

We needn't add or change anything in struct vgic_cpu. And both the
assembly code and the code could be much easier.

But we do end up with an extra memory access from EL2 in the critical
path, and I believe Marc's concern here is that if we cross a cache
line, this might really hurt performance.

Sorry. Do we may cross a cache line or a TLB entry?

I think that you're concerning to cross TLB entries. The reason is in
 below.

1. If the problem is on crossing cache line, it's caused by too much
instructions. Either the packing nr_lr or the gich_apr adds some
instructions. The packing nr_lr needs a little more instructions.

I don't see why this argument is valid.  If you have a separate
instruction and data cache, you may be loading from a different cache
line when placing the static value close to your instructions.  If you
add a variable to the vcpu struct, all of the fields may no longer fit
in a single data cache line and you may cause the memory subsystem to
have to fetch another cache line.  I believe the latter is Marc's
concern, and I suspect he would be equally concerned about the former.

I'm not too concerned about a TLB entry here, that works at a 4K
granularity and with the proper alignment of the struct and hyp code,
that shouldn't be a concern.  Without it, of course, there's a risk of
requiring another TLB entry as well.

2. ldr instruction is a pseudo instruction. So it's parsed into operation
on PC register.

Eh, it just means that it does a load relative from the PC address, and
if the offset is too far to be encoded in the immediate field, then it
does an indirect load through a literal pool, if I understand what you
are referring to.  In any case, there will be at least one actual ldr
instruction issued on the PE.

Now I put gich_apr in interrupts_head.S, it results
in gich_apr variable before __kvm_hyp_code_start. It may cross the
TLB entries.
How about to declare gich_apr after __kvm_cpu_return in interrupts.S?
Since save_vgic_state & restore_vgic_state is only used once, declaring
gich_apr just after the code could avoid crossing TLB entry.

Again, all the fields in the vcpu struct are quite likely to be aligned
within a single data cache line, I don't believe that's the case if you
stick some data in between the the hyp code.

-Christoffer