On Wed, Sep 4, 2024 at 9:25 PM Samuel Holland <samuel.holland@xxxxxxxxxx> wrote: > > On 2024-09-04 10:20 AM, Anup Patel wrote: > > On Wed, Sep 4, 2024 at 8:27 PM Samuel Holland <samuel.holland@xxxxxxxxxx> 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@xxxxxxxxxx> wrote: > >>>> On 2024-09-04 7:17 AM, Anup Patel wrote: > >>>>> On Thu, Aug 29, 2024 at 6:32 AM Samuel Holland > >>>>> <samuel.holland@xxxxxxxxxx> 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@xxxxxxxxxx> > >>>>>> --- > >>>>>> > >>>>>> (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. Regards, Anup