[PATCH] x86/hvm: Allow supplying a dynamic start ASID

[PATCH] x86/hvm: Allow supplying a dynamic start ASID

[Vaishali Thakkar]

Currently, Xen always starts the ASID allocation at 1. But
for SEV technologies the ASID space is divided. This is
because it's a security issue if a guest is started as
ES/SNP and is migrated to SEV-only. So, the types are
tracked explicitly.

Thus, in preparation of SEV support in Xen, add min_asid
to allow supplying the dynamic start ASID during the
allocation process.

Signed-off-by: Vaishali Thakkar <vaishali.thakkar@vates.tech>
---
 xen/arch/x86/hvm/asid.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/asid.c b/xen/arch/x86/hvm/asid.c
index 8d27b7dba1..e14b64f2c8 100644
--- a/xen/arch/x86/hvm/asid.c
+++ b/xen/arch/x86/hvm/asid.c
@@ -41,6 +41,7 @@ boolean_param("asid", opt_asid_enabled);
 /* Per-CPU ASID management. */
 struct hvm_asid_data {
    uint64_t core_asid_generation;
+   uint32_t min_asid;
    uint32_t next_asid;
    uint32_t max_asid;
    bool disabled;
@@ -53,7 +54,8 @@ void hvm_asid_init(int nasids)
     static int8_t g_disabled = -1;
     struct hvm_asid_data *data = &this_cpu(hvm_asid_data);
 
-    data->max_asid = nasids - 1;
+    data->min_asid = 1;
+    data->max_asid = nasids - data->min_asid;
     data->disabled = !opt_asid_enabled || (nasids <= 1);
 
     if ( g_disabled != data->disabled )
@@ -66,8 +68,8 @@ void hvm_asid_init(int nasids)
     /* Zero indicates 'invalid generation', so we start the count at one. */
     data->core_asid_generation = 1;
 
-    /* Zero indicates 'ASIDs disabled', so we start the count at one. */
-    data->next_asid = 1;
+    /* Zero indicates 'ASIDs disabled', so we start the count at min_asid. */
+    data->next_asid = data->min_asid;
 }
 
 void hvm_asid_flush_vcpu_asid(struct hvm_vcpu_asid *asid)
@@ -117,7 +119,7 @@ bool hvm_asid_handle_vmenter(struct hvm_vcpu_asid *asid)
     if ( unlikely(data->next_asid > data->max_asid) )
     {
         hvm_asid_flush_core();
-        data->next_asid = 1;
+        data->next_asid = data->min_asid;
         if ( data->disabled )
             goto disabled;
     }
-- 
2.44.0

Re: [PATCH] x86/hvm: Allow supplying a dynamic start ASID

[Andrew Cooper]

On 16/04/2024 9:54 am, Vaishali Thakkar wrote:
> Currently, Xen always starts the ASID allocation at 1. But
> for SEV technologies the ASID space is divided. This is
> because it's a security issue if a guest is started as
> ES/SNP and is migrated to SEV-only. So, the types are
> tracked explicitly.
>
> Thus, in preparation of SEV support in Xen, add min_asid
> to allow supplying the dynamic start ASID during the
> allocation process.
>
> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@vates.tech>

Mechanically, this is fine, but is it going to be useful in the long term?

For SEV and SEV-ES/SNP, we must run the VM in the single fixed ASID
negotiated with the ASP.  This is not not optional.

For non-encrypted VMs, we are free to choose between using the remaining
ASID space as we used to (i.e. run it per-pCPU and tick it over to flush
the TLBs), or to run it in a fixed ASID per guest too.

The latter lets us use INVLPGB, and would avoid maintaining two
different TLB-tagging schemes.


I'd say that we absolutely do want INVLPGB support for managing
non-encrypted VMs, and we cannot mix both fixed and floating ASIDs at
the same time.

We should seriously consider whether it's worth maintaining two schemes,
or just switching wholesale to a fixed ASID scheme.

I don't have a good answer here...  If it where anything but TLB
flushing, it would be an obvious choice, but TLB handling bugs are some
of the nastiest to show up.

~Andrew

Re: [PATCH] x86/hvm: Allow supplying a dynamic start ASID

[Vaishali Thakkar]

On 4/16/24 4:12 PM, Andrew Cooper wrote:
> On 16/04/2024 9:54 am, Vaishali Thakkar wrote:
>> Currently, Xen always starts the ASID allocation at 1. But
>> for SEV technologies the ASID space is divided. This is
>> because it's a security issue if a guest is started as
>> ES/SNP and is migrated to SEV-only. So, the types are
>> tracked explicitly.
>>
>> Thus, in preparation of SEV support in Xen, add min_asid
>> to allow supplying the dynamic start ASID during the
>> allocation process.
>>
>> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@vates.tech>
>
> Mechanically, this is fine, but is it going to be useful in the long term?
>
> For SEV and SEV-ES/SNP, we must run the VM in the single fixed ASID
> negotiated with the ASP.  This is not not optional.
>
> For non-encrypted VMs, we are free to choose between using the remaining
> ASID space as we used to (i.e. run it per-pCPU and tick it over to flush
> the TLBs), or to run it in a fixed ASID per guest too.
>
> The latter lets us use INVLPGB, and would avoid maintaining two
> different TLB-tagging schemes.
>
>
> I'd say that we absolutely do want INVLPGB support for managing
> non-encrypted VMs, and we cannot mix both fixed and floating ASIDs at
> the same time.

I agree that having a both schemes at the time is not practical. And I've
some RFC patches in work to propose fixed ASID scheme for all domains
(encrypted or not) to start a discussion regarding that.

IMO, this patch is still useful because my idea was to then use min_asid
as a holder to separate out the non-encrypted, encrypted space and SEV ES/
SNP ASID space. If it's more easier to see the usefulness of this patch
along with other asid fixes, I'm fine with putting it in that RFC patchset.
But I thought that this change was independent enough to be sent by
itself.

> We should seriously consider whether it's worth maintaining two schemes,
> or just switching wholesale to a fixed ASID scheme.
>
> I don't have a good answer here...  If it where anything but TLB
> flushing, it would be an obvious choice, but TLB handling bugs are some
> of the nastiest to show up.
>
> ~Andrew

Re: [PATCH] x86/hvm: Allow supplying a dynamic start ASID

[Vaishali Thakkar]

On 4/16/24 4:25 PM, Vaishali Thakkar wrote:
> On 4/16/24 4:12 PM, Andrew Cooper wrote:
>> On 16/04/2024 9:54 am, Vaishali Thakkar wrote:
>>> Currently, Xen always starts the ASID allocation at 1. But
>>> for SEV technologies the ASID space is divided. This is
>>> because it's a security issue if a guest is started as
>>> ES/SNP and is migrated to SEV-only. So, the types are
>>> tracked explicitly.
>>>
>>> Thus, in preparation of SEV support in Xen, add min_asid
>>> to allow supplying the dynamic start ASID during the
>>> allocation process.
>>>
>>> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@vates.tech>
>>
>> Mechanically, this is fine, but is it going to be useful in the long term?
>>
>> For SEV and SEV-ES/SNP, we must run the VM in the single fixed ASID
>> negotiated with the ASP.  This is not not optional.
>>
>> For non-encrypted VMs, we are free to choose between using the remaining
>> ASID space as we used to (i.e. run it per-pCPU and tick it over to flush
>> the TLBs), or to run it in a fixed ASID per guest too.
>>
>> The latter lets us use INVLPGB, and would avoid maintaining two
>> different TLB-tagging schemes.
>>
>>
>> I'd say that we absolutely do want INVLPGB support for managing
>> non-encrypted VMs, and we cannot mix both fixed and floating ASIDs at
>> the same time.
>
> I agree that having a both schemes at the time is not practical. And I've
> some RFC patches in work to propose fixed ASID scheme for all domains
> (encrypted or not) to start a discussion regarding that.
>
> IMO, this patch is still useful because my idea was to then use min_asid
> as a holder to separate out the non-encrypted, encrypted space and SEV ES/
> SNP ASID space. If it's more easier to see the usefulness of this patch
> along with other asid fixes, I'm fine with putting it in that RFC patchset.
> But I thought that this change was independent enough to be sent by
> itself.

s/encrypted/SEV

>> We should seriously consider whether it's worth maintaining two schemes,
>> or just switching wholesale to a fixed ASID scheme.
>>
>> I don't have a good answer here...  If it where anything but TLB
>> flushing, it would be an obvious choice, but TLB handling bugs are some
>> of the nastiest to show up.
>>
>> ~Andrew
>
>