Re: [PATCH] kvm: optimize ISR lookups

Avi Kivity
2012-05-30T15:05:41Z

On 05/24/2012 01:00 AM, Thomas Gleixner wrote:

> Thought more about that.
>
> We have a clear distinction between HW accessed data and software
> accessed data.
>
> If I look at TPR then it is special cased already and it does:
>
> case APIC_TASKPRI:
> report_tpr_access(apic, false|true);
> /* fall thru */
>
> And the fall through is using the general accessor for all not special
> cased registers.
>
> So all you have to do is
>
> case APIC_TASKPRI:
> report_tpr_access(apic, false|true);
> + return access_mapped_reg(...);
>
> Instead of the fall through.
>
> So there is no synchronizing back and forth problem simply because you
> already have a special case for that register.
>
> I know you'll argue that the tpr reporting is a special hack for
> windows guests, at least that's what the changelog tells.
>
> But even if we have a few more registers accessed by hardware and if
> they do not require a special casing, I really doubt that the overhead
> of special casing those few regs will be worse than not having the
> obvious optimization in place.
>
> And looking deeper it's a total non issue. The apic mapping is 4k. The
> register stride is strictly 0x10. That makes a total of 256 possible
> registers.
>
> So now you have two possibilites:
>
> 1) Create a 256 bit == 64byte bitfield to select the one or the other
> representation.
>
> The overhead of checking the bit is not going to be observable.
>
> 2) Create a 256 function pointer array == 2k resp. 1k (64 / 32bit)
>
> That's not a lot of memory even if you have to maintain two
> separate variants for read and write, but it allows you to get rid
> of the already horribly compiled switch case in apic_read/write and
> you'll get the optional stuff like report_tpr_access() w/o extra
> conditionals just for free.
>
> An extra goodie is that you can catch any access to a non existing
> register which you now just silently ignore. And that allows you
> to react on any future hardware oddities without adding a single
> runtime conditional.
>
> This is stricly x86 and x86 is way better at dealing with indirect
> calls than with the mess gcc creates compiling those switch case
> constructs.
>
> So I'd go for that and rather spend the memory and the time in
> setting up the function pointers on init/ioctl than dealing with
> the inconsistency of HW/SW representation with magic hacks.
>

I like the bitmap version, it seems very lightweight. But by itself it
doesn't allow us to use bitmap_weight (or the other bitmap accessors),
unless you assume beforehand that those registers will never be in the
hardware-layout region.

(you also need extra code for copying the APIC state to and from
userspace; right now we just memcpy the APIC page)

--
error compiling committee.c: too many arguments to function