Re: [PATCH v4 09/10] RISC-V: KVM: Allow Smnpm and Ssnpm extensions for guests

[Samuel Holland <samuel.holland@sifive.com>]

Hi Anup,

On 2024-09-05 12:18 AM, Anup Patel wrote:
> On Wed, Sep 4, 2024 at 9:25 PM Samuel Holland <samuel.holland@sifive.com> wrote:
>>
>> On 2024-09-04 10:20 AM, Anup Patel wrote:
>>> On Wed, Sep 4, 2024 at 8:27 PM Samuel Holland <samuel.holland@sifive.com> wrote:
>>>>
>>>> Hi Anup,
>>>>
>>>> On 2024-09-04 9:45 AM, Anup Patel wrote:
>>>>> On Wed, Sep 4, 2024 at 8:01 PM Samuel Holland <samuel.holland@sifive.com> wrote:
>>>>>> On 2024-09-04 7:17 AM, Anup Patel wrote:
>>>>>>> On Thu, Aug 29, 2024 at 6:32 AM Samuel Holland
>>>>>>> <samuel.holland@sifive.com> wrote:
>>>>>>>>
>>>>>>>> The interface for controlling pointer masking in VS-mode is henvcfg.PMM,
>>>>>>>> which is part of the Ssnpm extension, even though pointer masking in
>>>>>>>> HS-mode is provided by the Smnpm extension. As a result, emulating Smnpm
>>>>>>>> in the guest requires (only) Ssnpm on the host.
>>>>>>>>
>>>>>>>> Since the guest configures Smnpm through the SBI Firmware Features
>>>>>>>> interface, the extension can be disabled by failing the SBI call. Ssnpm
>>>>>>>> cannot be disabled without intercepting writes to the senvcfg CSR.
>>>>>>>>
>>>>>>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> (no changes since v2)
>>>>>>>>
>>>>>>>> Changes in v2:
>>>>>>>>  - New patch for v2
>>>>>>>>
>>>>>>>>  arch/riscv/include/uapi/asm/kvm.h | 2 ++
>>>>>>>>  arch/riscv/kvm/vcpu_onereg.c      | 3 +++
>>>>>>>>  2 files changed, 5 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
>>>>>>>> index e97db3296456..4f24201376b1 100644
>>>>>>>> --- a/arch/riscv/include/uapi/asm/kvm.h
>>>>>>>> +++ b/arch/riscv/include/uapi/asm/kvm.h
>>>>>>>> @@ -175,6 +175,8 @@ enum KVM_RISCV_ISA_EXT_ID {
>>>>>>>>         KVM_RISCV_ISA_EXT_ZCF,
>>>>>>>>         KVM_RISCV_ISA_EXT_ZCMOP,
>>>>>>>>         KVM_RISCV_ISA_EXT_ZAWRS,
>>>>>>>> +       KVM_RISCV_ISA_EXT_SMNPM,
>>>>>>>> +       KVM_RISCV_ISA_EXT_SSNPM,
>>>>>>>>         KVM_RISCV_ISA_EXT_MAX,
>>>>>>>>  };
>>>>>>>>
>>>>>>>> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
>>>>>>>> index b319c4c13c54..6f833ec2344a 100644
>>>>>>>> --- a/arch/riscv/kvm/vcpu_onereg.c
>>>>>>>> +++ b/arch/riscv/kvm/vcpu_onereg.c
>>>>>>>> @@ -34,9 +34,11 @@ static const unsigned long kvm_isa_ext_arr[] = {
>>>>>>>>         [KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m,
>>>>>>>>         [KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v,
>>>>>>>>         /* Multi letter extensions (alphabetically sorted) */
>>>>>>>> +       [KVM_RISCV_ISA_EXT_SMNPM] = RISCV_ISA_EXT_SSNPM,
>>>>>>>
>>>>>>> Why not use KVM_ISA_EXT_ARR() macro here ?
>>>>>>
>>>>>> Because the extension name in the host does not match the extension name in the
>>>>>> guest. Pointer masking for HS mode is provided by Smnpm. Pointer masking for VS
>>>>>> mode is provided by Ssnpm at the hardware level, but this needs to appear to the
>>>>>> guest as if Smnpm was implemented, since the guest thinks it is running on bare
>>>>>> metal.
>>>>>
>>>>> Okay, makes sense.
>>>>>
>>>>>>
>>>>>>>>         KVM_ISA_EXT_ARR(SMSTATEEN),
>>>>>>>>         KVM_ISA_EXT_ARR(SSAIA),
>>>>>>>>         KVM_ISA_EXT_ARR(SSCOFPMF),
>>>>>>>> +       KVM_ISA_EXT_ARR(SSNPM),
>>>>>>>>         KVM_ISA_EXT_ARR(SSTC),
>>>>>>>>         KVM_ISA_EXT_ARR(SVINVAL),
>>>>>>>>         KVM_ISA_EXT_ARR(SVNAPOT),
>>>>>>>> @@ -129,6 +131,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext)
>>>>>>>>         case KVM_RISCV_ISA_EXT_M:
>>>>>>>>         /* There is not architectural config bit to disable sscofpmf completely */
>>>>>>>>         case KVM_RISCV_ISA_EXT_SSCOFPMF:
>>>>>>>> +       case KVM_RISCV_ISA_EXT_SSNPM:
>>>>>>>
>>>>>>> Why not add KVM_RISCV_ISA_EXT_SMNPM here ?
>>>>>>>
>>>>>>> Disabling Smnpm from KVM user space is very different from
>>>>>>> disabling Smnpm from Guest using SBI FWFT extension.
>>>>>>
>>>>>> Until a successful SBI FWFT call to KVM to enable pointer masking for VS mode,
>>>>>> the existence of Smnpm has no visible effect on the guest. So failing the SBI
>>>>>> call is sufficient to pretend that the hardware does not support Smnpm.
>>>>>>
>>>>>>> The KVM user space should always add Smnpm in the
>>>>>>> Guest ISA string whenever the Host ISA string has it.
>>>>>>
>>>>>> I disagree. Allowing userspace to disable extensions is useful for testing and
>>>>>> to support migration to hosts which do not support those extensions. So I would
>>>>>> only add extensions to this list if there is no possible way to disable them.
>>>>>
>>>>> I am not saying to disallow KVM user space disabling Smnpm.
>>>>
>>>> Then I'm confused. This is the "return false;" switch case inside
>>>> kvm_riscv_vcpu_isa_disable_allowed(). If I add KVM_RISCV_ISA_EXT_SMNPM here,
>>>> then (unless I am misreading the code) I am disallowing KVM userspace from
>>>> disabling Smnpm in the guest (i.e. preventing KVM userspace from removing Smnpm
>>>> from the guest ISA string). If that is not desired, then why do you suggest I
>>>> add KVM_RISCV_ISA_EXT_SMNPM here?
>>>
>>> Yes, adding KVM_RISCV_ISA_EXT_SMNPM here means KVM
>>> user space can't disable it using ONE_REG interface but KVM user
>>> space can certainly not add it in the Guest ISA string.
>>
>> Is there a problem with allowing KVM userspace to disable the ISA extension with
>> the ONE_REG interface?
>>
>> If KVM userspace removes Smnpm from the ISA string without the host kernel's
>> knowledge, that doesn't actually prevent the guest from successfully calling
>> sbi_fwft_set(POINTER_MASKING_PMLEN, ...), so it doesn't guarantee that the VM
>> can be migrated to a host without pointer masking support. So the ONE_REG
>> interface still has value. (And that's my answer to your original question "Why
>> not add KVM_RISCV_ISA_EXT_SMNPM here ?")
> 
> Currently, disabling KVM_RISCV_ISA_EXT_SMNPM via ONE_REG
> will only clear the corresponding bit in VCPU isa bitmap. Basically, the
> KVM user space disabling KVM_RISCV_ISA_EXT_SMNPM for Guest
> changes nothing for the Guest/VM.
> 
> On other hand, disabling KVM_RISCV_ISA_EXT_SVPBMT via
> ONE_REG will not only clear it from VCPU isa bitmap but also
> disable Svpmbt from henvcfg CSR for the Guest/VM.
> 
> In other words, if disabling an ISA extension is allowed by the
> kvm_riscv_vcpu_isa_disable_allowed() then the Guest/VM must
> see a different behaviour when the ISA extension is disabled by
> KVM user space.
> 
>>
>>>>> The presence of Smnpm in ISA only means that it is present in HW
>>>>> but it needs to be explicitly configured/enabled using SBI FWFT.
>>>>>
>>>>> KVM user space can certainly disable extensions by not adding it to
>>>>> ISA string based on the KVMTOOL/QEMU-KVM command line option.
>>>>> Additionally, when SBI FWFT is added to KVM RISC-V. It will have its
>>>>> own way to explicitly disable firmware features from KVM user space.
>>>>
>>>> I think we agree on this, but your explanation here appears to conflict with
>>>> your suggested code change. Apologies if I'm missing something.
>>>
>>> I think the confusion is about what does it mean when Smnpm is present
>>> in the ISA string. We have two approaches:
>>>
>>> 1) Presence of Smnpm in ISA string only means it is present in HW but
>>>     says nothing about its enable/disable state. To configure/enable
>>>     Smnpm, the supervisor must use SBI FWFT.
>>>
>>> 2) Presence of Smnpm in ISA string means it is present in HW and
>>>     enabled at boot-time. To re-configure/disable Smnpm, the supervisor
>>>     must use SBI FWFT.
>>>
>>> I am suggesting approach #1 but I am guessing you are leaning towards
>>> approach #2 ?
>>>
>>> For approach #2, additional hencfg.PMM configuration is required in
>>> this patch based on the state of KVM_RISCV_ISA_EXT_SMNPM.
>>
>> No, I am definitely suggesting only approach #1. My proposal for adding pointer
>> masking to the SBI FWFT extension[1] specifies the feature as disabled by
>> default, and this would apply both inside and ouside a VM.
>>
>> But I am also suggesting that the ONE_REG interface is a useful way to
>> completely hide the extension from the guest, like we do for other extensions
>> such as Svpbmt. The only difference between something like Svpbmt and Smnpm is
>> that instead of clearing a bit in henvcfg to hide the extension from the guest,
>> we reject calls to sbi_fwft_set(POINTER_MASKING_PMLEN, ...) when the ISA
>> extension is hidden from the guest.
> 
> I think we are converging towards the same thing.
> 
> How about this ?
> 
> For this series, lets add KVM_RISCV_ISA_EXT_SMNPM to
> kvm_riscv_vcpu_isa_disable_allowed() so that for the time
> being KVM user space can't disable Smnpm.
> 
> In the future, a separate series which adds SBI FWFT to
> KVM RISC-V will remove KVM_RISCV_ISA_EXT_SMNPM
> from the kvm_riscv_vcpu_isa_disable_allowed() because
> disabling Smnpm from KVM user space would mean that
> the POINTER_MASKING_PMLEN firmware feature is
> not available to the Guest/VM.
> 
> This means in the future (after SBI FWFT is implemented in
> KVM RISC-V), Guest with Smnpm disabled can be migrated
> to a host without pointer masking.

OK, that is a reasonable compromise. I'll do that for v5.

Regards,
Samuel