Hans-Jörg Höxer <hshoexer@genua.de> writes:

> Hi,
>
> On Sat, Apr 26, 2025 at 08:51:42PM +0200, Alexander Bluhm wrote:
>> ...
>> Can we please put extern declarations in a header file?
>> +extern int amd64_min_noes_asid;
>> In a C file, other than where it is defined, can easily lead to
>> mistakes when something in the code changes.
>
> ack.
>
>> Function name _vmm_alloc_vpid() with an underscore looks strange.
>> Can we call it vmm_alloc_vpid_vcpu() ?
>
> ack.
>
> Update diff below.
>

I like this one with one style nit. ok dv@.

> Take care,
> HJ.
>
> ------------------------------------------------------------------
> commit e13ab6cb2dd5ce625202e3ffb94a7f933db28dfe
> Author: Hans-Joerg Hoexer <hshoexer@genua.de>
> Date:   Wed Jul 10 13:25:04 2024 +0200
>
>     vmm: minimum ASID for non-SEVES guests
>
>     Introduce vmm_alloc_asid() to be used by SVM base VMs.  No change
>     for VMX based VMs.
>
> diff --git a/sys/arch/amd64/amd64/identcpu.c b/sys/arch/amd64/amd64/identcpu.c
> index 18ecbc8f4c8..1a9a99e9bbb 100644
> --- a/sys/arch/amd64/amd64/identcpu.c
> +++ b/sys/arch/amd64/amd64/identcpu.c
> @@ -67,6 +67,7 @@ int cpuspeed;
>
>  int amd64_has_xcrypt;
>  int amd64_pos_cbit;	/* C bit position for SEV */
> +int amd64_min_noes_asid;
>  int has_rdrand;
>  int has_rdseed;
>
> @@ -710,6 +711,7 @@ identifycpu(struct cpu_info *ci)
>  		    'd', CPUID_MEMBER(ci_feature_amdsev_edx),
>  		    CPUID_AMDSEV_EDX_BITS);
>  		amd64_pos_cbit = (ci->ci_feature_amdsev_ebx & 0x3f);
> +		amd64_min_noes_asid = (ci->ci_feature_amdsev_edx);

The parens look strange here because they're not guarding
anything. Maybe it's just the context of the diff hunk, but given no
bitwise ops can we drop them?

>  	}
>
>  	printf("\n");
> diff --git a/sys/arch/amd64/amd64/vmm_machdep.c b/sys/arch/amd64/amd64/vmm_machdep.c
> index e1a17583fe3..9b5d56da27b 100644
> --- a/sys/arch/amd64/amd64/vmm_machdep.c
> +++ b/sys/arch/amd64/amd64/vmm_machdep.c
> @@ -36,6 +36,7 @@
>  #include <machine/pmap.h>
>  #include <machine/biosvar.h>
>  #include <machine/segments.h>
> +#include <machine/cpu.h>
>  #include <machine/cpufunc.h>
>  #include <machine/vmmvar.h>
>
> @@ -124,7 +125,9 @@ int svm_fault_page(struct vcpu *, paddr_t);
>  int vmx_fault_page(struct vcpu *, paddr_t);
>  int vmx_handle_np_fault(struct vcpu *);
>  int svm_handle_np_fault(struct vcpu *);
> +int vmm_alloc_vpid_vcpu(uint16_t *, struct vcpu *);
>  int vmm_alloc_vpid(uint16_t *);
> +int vmm_alloc_asid(uint16_t *, struct vcpu *);
>  void vmm_free_vpid(uint16_t);
>  const char *vcpu_state_decode(u_int);
>  const char *vmx_exit_reason_decode(uint32_t);
> @@ -2766,7 +2769,7 @@ vcpu_init_svm(struct vcpu *vcpu, struct vm_create_params *vcp)
>  	int ret = 0;
>
>  	/* Allocate an ASID early to avoid km_alloc if we're out of ASIDs. */
> -	if (vmm_alloc_vpid(&vcpu->vc_vpid))
> +	if (vmm_alloc_asid(&vcpu->vc_vpid, vcpu))
>  		return (ENOMEM);
>
>  	/* Allocate VMCB VA */
> @@ -6354,27 +6357,32 @@ vcpu_run_svm(struct vcpu *vcpu, struct vm_run_params *vrp)
>  }
>
>  /*
> - * vmm_alloc_vpid
> + * vmm_alloc_vpid_vcpu
>   *
>   * Sets the memory location pointed to by "vpid" to the next available VPID
> - * or ASID.
> + * or ASID. For SEV-ES consider minimum ASID value for non-ES enabled guests.
>   *
>   * Parameters:
>   *  vpid: Pointer to location to receive the next VPID/ASID
> + *  vcpu: Pointer to VCPU data structure
>   *
>   * Return Values:
>   *  0: The operation completed successfully
>   *  ENOMEM: No VPIDs/ASIDs were available. Content of 'vpid' is unchanged.
>   */
>  int
> -vmm_alloc_vpid(uint16_t *vpid)
> +vmm_alloc_vpid_vcpu(uint16_t *vpid, struct vcpu *vcpu)
>  {
> -	uint16_t i;
> +	uint16_t i, minasid;
>  	uint8_t idx, bit;
>  	struct vmm_softc *sc = vmm_softc;
>
>  	rw_enter_write(&vmm_softc->vpid_lock);
> -	for (i = 1; i <= sc->max_vpid; i++) {
> +	if (vcpu == NULL || vcpu->vc_seves)
> +		minasid = 1;
> +	else
> +		minasid = amd64_min_noes_asid;
> +	for (i = minasid; i <= sc->max_vpid; i++) {
>  		idx = i / 8;
>  		bit = i - (idx * 8);
>
> @@ -6396,6 +6404,18 @@ vmm_alloc_vpid(uint16_t *vpid)
>  	return ENOMEM;
>  }
>
> +int
> +vmm_alloc_vpid(uint16_t *vpid)
> +{
> +	return vmm_alloc_vpid_vcpu(vpid, NULL);
> +}
> +
> +int
> +vmm_alloc_asid(uint16_t *asid, struct vcpu *vcpu)
> +{
> +	return vmm_alloc_vpid_vcpu(asid, vcpu);
> +}
> +
>  /*
>   * vmm_free_vpid
>   *
> diff --git a/sys/arch/amd64/include/cpu.h b/sys/arch/amd64/include/cpu.h
> index fc560c0ffce..77ad7bf0f34 100644
> --- a/sys/arch/amd64/include/cpu.h
> +++ b/sys/arch/amd64/include/cpu.h
> @@ -416,6 +416,7 @@ void	identifycpu(struct cpu_info *);
>  int	cpu_amd64speed(int *);
>  extern int cpuspeed;
>  extern int amd64_pos_cbit;
> +extern int amd64_min_noes_asid;
>
>  /* machdep.c */
>  void	dumpconf(void);