On 08/04/2025 2:15 am, dm...@proton.me wrote:
> From: Denis Mukhin <dmuk...@ford.com>
>
> Use `asm goto()` in vmread_safe() to simplify the error handling logic.
>
> Update __vmread() to return `unsigned long` as per suggestion in [1].
> Rename __vmread() to vmread_unsafe() to match the behavior.
> Update all call sites everywhere. Drop `UNLIKELY_*()`.
>
> Group all vmread*() calls close to each other in vmx.h
>
> Rename internal vmr*() to vmread*().
>
> [1]
> https://lore.kernel.org/xen-devel/c452a1d7-4a57-4c5f-8a83-36a74ff22...@citrix.com/
>
> Signed-off-by: Denis Mukhin <dmuk...@ford.com>
> ---
> Link to CI:
> https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1756781092
> ---
> xen/arch/x86/cpu/vpmu_intel.c | 3 +-
> xen/arch/x86/hvm/vmx/intr.c | 26 +--
> xen/arch/x86/hvm/vmx/realmode.c | 2 +-
> xen/arch/x86/hvm/vmx/vmcs.c | 160 ++++++++++---------
> xen/arch/x86/hvm/vmx/vmx.c | 209 +++++++++++--------------
> xen/arch/x86/hvm/vmx/vvmx.c | 43 +++--
> xen/arch/x86/include/asm/domain.h | 2 +-
> xen/arch/x86/include/asm/hvm/vmx/vmx.h | 69 ++++----
> 8 files changed, 235 insertions(+), 279 deletions(-)
This is why I suggested not to convert everything in one go. It's now a
patch doing multiple complicated things, and is proportionally harder to
review.
For everyone in public, it is especially daft that we have __vmread()
which is void and (if it doesn't BUG()) will pass it's return value by
pointer. It leads to very unergonomic logic.
Start by just implementing vmread(), and updating __vmread() and
vmwrite_safe() to use it. You cannot use asm goto() for vmread()
because of the no-outputs constraint that we still need to follow.
Then, in a separate patch, you can do simple conversion such as ...
>
> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> index 7ce98ee42e..9c93d1f28c 100644
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -796,8 +796,7 @@ static int cf_check core2_vpmu_do_interrupt(void)
> else
> {
> /* No PMC overflow but perhaps a Trace Message interrupt. */
> - __vmread(GUEST_IA32_DEBUGCTL, &msr_content);
> - if ( !(msr_content & IA32_DEBUGCTLMSR_TR) )
> + if ( !(vmread_unsafe(GUEST_IA32_DEBUGCTL) & IA32_DEBUGCTLMSR_TR) )
> return 0;
> }
>
... this to vmread().
Splitting the patch makes a substantial difference to review-ability,
because patch 1 is "is this new helper implemented correctly?", and
patch 2 is "is this boilerplate rearrangement no overall change?".
For vmr(), I'd start by just wrapping vmread(). It's debugging logic
where brevity is important.
> diff --git a/xen/arch/x86/include/asm/domain.h
> b/xen/arch/x86/include/asm/domain.h
> index 6b877e33a1..ffe9acd75d 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -595,7 +595,7 @@ struct arch_vcpu
>
> /* Debug registers. */
> unsigned long dr[4];
> - unsigned long dr7; /* Ideally int, but __vmread() needs long. */
> + unsigned long dr7; /* Ideally int, but vmread_unsafe() needs unsigned
> long. */
> unsigned int dr6;
This comment was left as a hint, and you've just addressed the problem
forcing it to stay unsigned long.
Changing dr7 should be in a separate patch too.
~Andrew
