On Wed, 21 May 2025 at 14:45, David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 21.05.25 15:32, Fuad Tabba wrote:
> > Hi David,
> >
> > On Wed, 21 May 2025 at 14:22, David Hildenbrand <david@xxxxxxxxxx> wrote:
> >>
> >> On 21.05.25 15:15, Fuad Tabba wrote:
> >>> Hi David,
> >>>
> >>> On Wed, 21 May 2025 at 13:44, David Hildenbrand <david@xxxxxxxxxx> wrote:
> >>>>
> >>>> On 21.05.25 12:29, Fuad Tabba wrote:
> >>>>> On Wed, 21 May 2025 at 11:26, David Hildenbrand <david@xxxxxxxxxx> wrote:
> >>>>>>
> >>>>>> On 21.05.25 12:12, Fuad Tabba wrote:
> >>>>>>> Hi David,
> >>>>>>>
> >>>>>>> On Wed, 21 May 2025 at 09:05, David Hildenbrand <david@xxxxxxxxxx> wrote:
> >>>>>>>>
> >>>>>>>> On 13.05.25 18:34, Fuad Tabba wrote:
> >>>>>>>>> Enable mapping guest_memfd in arm64. For now, it applies to all
> >>>>>>>>> VMs in arm64 that use guest_memfd. In the future, new VM types
> >>>>>>>>> can restrict this via kvm_arch_gmem_supports_shared_mem().
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Fuad Tabba <tabba@xxxxxxxxxx>
> >>>>>>>>> ---
> >>>>>>>>> arch/arm64/include/asm/kvm_host.h | 10 ++++++++++
> >>>>>>>>> arch/arm64/kvm/Kconfig | 1 +
> >>>>>>>>> 2 files changed, 11 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >>>>>>>>> index 08ba91e6fb03..2514779f5131 100644
> >>>>>>>>> --- a/arch/arm64/include/asm/kvm_host.h
> >>>>>>>>> +++ b/arch/arm64/include/asm/kvm_host.h
> >>>>>>>>> @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void)
> >>>>>>>>> return true;
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> +static inline bool kvm_arch_supports_gmem(struct kvm *kvm)
> >>>>>>>>> +{
> >>>>>>>>> + return IS_ENABLED(CONFIG_KVM_GMEM);
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>> +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm)
> >>>>>>>>> +{
> >>>>>>>>> + return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM);
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>> #endif /* __ARM64_KVM_HOST_H__ */
> >>>>>>>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> >>>>>>>>> index 096e45acadb2..8c1e1964b46a 100644
> >>>>>>>>> --- a/arch/arm64/kvm/Kconfig
> >>>>>>>>> +++ b/arch/arm64/kvm/Kconfig
> >>>>>>>>> @@ -38,6 +38,7 @@ menuconfig KVM
> >>>>>>>>> select HAVE_KVM_VCPU_RUN_PID_CHANGE
> >>>>>>>>> select SCHED_INFO
> >>>>>>>>> select GUEST_PERF_EVENTS if PERF_EVENTS
> >>>>>>>>> + select KVM_GMEM_SHARED_MEM
> >>>>>>>>> help
> >>>>>>>>> Support hosting virtualized guest machines.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Do we have to reject somewhere if we are given a guest_memfd that was
> >>>>>>>> *not* created using the SHARED flag? Or will existing checks already
> >>>>>>>> reject that?
> >>>>>>>
> >>>>>>> We don't reject, but I don't think we need to. A user can create a
> >>>>>>> guest_memfd that's private in arm64, it would just be useless.
> >>>>>>
> >>>>>> But the arm64 fault routine would not be able to handle that properly, no?
> >>>>>
> >>>>> Actually it would. The function user_mem_abort() doesn't care whether
> >>>>> it's private or shared. It would fault it into the guest correctly
> >>>>> regardless.
> >>>>
> >>>>
> >>>> I think what I meant is that: if it's !shared (private only), shared
> >>>> accesses (IOW all access without CoCo) should be taken from the user
> >>>> space mapping.
> >>>>
> >>>> But user_mem_abort() would blindly go to kvm_gmem_get_pfn() because
> >>>> "is_gmem = kvm_slot_has_gmem(memslot) = true".
> >>>
> >>> Yes, since it is a gmem-backed slot.
> >>>
> >>>> In other words, arm64 would have to *ignore* guest_memfd that does not
> >>>> support shared?
> >>>>
> >>>> That's why I was wondering whether we should just immediately refuse
> >>>> such guest_memfds.
> >>>
> >>> My thinking is that if a user deliberately creates a
> >>> guest_memfd-backed slot without designating it as being sharable, then
> >>> either they would find out when they try to map that memory to the
> >>> host userspace (mapping it would fail), or it could be that they
> >>> deliberately want to set up a VM with memslots that not mappable at
> >>> all by the host.
> >>
> >> Hm. But that would meant that we interpret "private" memory as a concept
> >> that is not understood by the VM. Because the VM does not know what
> >> "private" memory is ...
> >>
> >>> Perhaps to add some layer of security (although a
> >>> very flimsy one, since it's not a confidential guest).
> >>
> >> Exactly my point. If you don't want to mmap it then ... don't mmap it :)
> >>
> >>>
> >>> I'm happy to a check to prevent this. The question is, how to do it
> >>> exactly (I assume it would be in kvm_gmem_create())? Would it be
> >>> arch-specific, i.e., prevent arm64 from creating non-shared
> >>> guest_memfd backed memslots? Or do it by VM type? Even if we do it by
> >>> VM-type it would need to be arch-specific, since we allow private
> >>> guest_memfd slots for the default VM in x86, but we wouldn't for
> >>> arm64.
> >>>
> >>> We could add another function, along the lines of
> >>> kvm_arch_supports_gmem_only_shared_mem(), but considering that it
> >>> actually works, and (arguably) would behave as intended, I'm not sure
> >>> if it's worth the complexity.
> >>>
> >>> What do you think?
> >>
> >> My thinking was to either block this at slot creation time or at
> >> guest_memfd creation time. And we should probably block that for other
> >> VM types as well that do not support private memory?
> >>
> >> I mean, creating guest_memfd for private memory when there is no concept
> >> of private memory for the VM is ... weird, no? :)
> >
> > Actually, I could add this as an arch-specific check in
> > arch/arm64/kvm/mmu.c:kvm_arch_prepare_memory_region(). That way, core
> > KVM/guest_memfd code doesn't need to handle this arm64-specific behavior.
> >
> > Does that sound good?
>
> Yes, but only do so if you agree.
Ack :)
/fuad
> --
> Cheers,
>
> David / dhildenb
>
