Re: [PATCH v8 1/5] i386/tcg: implement x2APIC registers MSR access

[Bui Quang Minh <minhquangbui99@gmail.com>]

On 10/22/23 20:59, Phil Dennis-Jordan wrote:
> I can confirm that this works. The build issue obviously needs fixing,
> but once that's fixed, this improves on the status quo.
> 
> I've tested this and patch 2/5 with x2apic CPUID bit enabled with the
> hvf backend on macOS. To make it work in hvf mode, I used the attached
> additional minimal patch to wire it up, but with that in place it
> noticeably improves guest OS performance. (This patch doesn't yet
> implement raising exceptions or checking for x2apic mode, more on that
> in my comments below.)
> 
> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
> 
> On Tue, 26 Sept 2023 at 18:08, Bui Quang Minh <minhquangbui99@gmail.com> wrote:
>> @@ -455,6 +469,19 @@ void helper_rdmsr(CPUX86State *env)
>>           val = (cs->nr_threads * cs->nr_cores) | (cs->nr_cores << 16);
>>           break;
>>       }
>> +    case MSR_APIC_START ... MSR_APIC_END: {
>> +        int index = (uint32_t)env->regs[R_ECX] - MSR_APIC_START;
>> +
>> +        if (!is_x2apic_mode(env_archcpu(env)->apic_state)) {
>> +            raise_exception_ra(env, EXCP0D_GPF, GETPC());
>> +        }
>> +
>> +        qemu_mutex_lock_iothread();
>> +        val = apic_register_read(index);
>> +        qemu_mutex_unlock_iothread();
> 
> Shouldn't the x2apic mode check technically be inside the lock?
> Furthermore, we need the mode check logic in each accelerator whose
> MSR read and write we wire up. Finally, there's the exception raising
> issue which Michael noted.
> 
> So my suggestion would be to wrap the x2apic mode check and the call
> to the lower level apic_register_read into a standalone
> apic_x2apic_msr_read() or similar, and the equivalent for writes.
> These functions should then also return success or failure, the latter
> indicating an exception should be raised. Raising the exception can
> then also be implemented for each accelerator at the relevant call
> site. That contains the raise_exception_ra call in the TCG specific
> code, and I can do the equivalent on the hvf side.

Thanks a lot for your suggestion, I've taken this approach and 
implemented an apic_msr_read/write wrapper as you suggested in version 9 
(https://lore.kernel.org/qemu-devel/20231024152105.35942-1-minhquangbui99@gmail.com/)

Thank you,
Quang Minh.