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

[Phil Dennis-Jordan]

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.

It may also be cleaner to only implement the shared xAPIC and x2APIC
functionality in the apic_register_{read|write} functions, and put the
code that's specific to MMIO and MSR paths in the
apic_mem_{read|write} and apic_x2apic_msr_{read|write} wrappers,
respectively? Not sure.

0001-i386-hvf-Wires-up-MSRs-and-enables-x2apic-mode-when-.patch
Description: Binary data