Re: [PATCH v3 3/7] virt: geniezone: Introduce GenieZone hypervisor support

Re: [PATCH v3 3/7] virt: geniezone: Introduce GenieZone hypervisor support

[Marc Zyngier]

On Fri, 12 May 2023 09:04:01 +0100,
Yi-De Wu <yi-de.wu@mediatek.com> wrote:
> 
> From: "Yingshiuan Pan" <yingshiuan.pan@mediatek.com>
> 
> GenieZone is MediaTek hypervisor solution, and it is running in EL2
> stand alone as a type-I hypervisor. This patch exports a set of ioctl
> interfaces for userspace VMM (e.g., crosvm) to operate guest VMs
> lifecycle (creation and destroy) on GenieZone.
> 
> Signed-off-by: Yingshiuan Pan <yingshiuan.pan@mediatek.com>
> Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>

[...]

> +/**
> + * gzvm_gfn_to_pfn_memslot() - Translate gfn (guest ipa) to pfn (host pa),
> + *			       result is in @pfn
> + *
> + * Leverage KVM's gfn_to_pfn_memslot(). Because gfn_to_pfn_memslot() needs
> + * kvm_memory_slot as parameter, this function populates necessary fileds
> + * for calling gfn_to_pfn_memslot().
> + *
> + * Return:
> + * * 0			- Succeed
> + * * -EFAULT		- Failed to convert
> + */
> +static int gzvm_gfn_to_pfn_memslot(struct gzvm_memslot *memslot, u64 gfn, u64 *pfn)
> +{
> +	hfn_t __pfn;
> +	struct kvm_memory_slot kvm_slot = {0};
> +
> +	kvm_slot.base_gfn = memslot->base_gfn;
> +	kvm_slot.npages = memslot->npages;
> +	kvm_slot.dirty_bitmap = NULL;
> +	kvm_slot.userspace_addr = memslot->userspace_addr;
> +	kvm_slot.flags = memslot->flags;
> +	kvm_slot.id = memslot->slot_id;
> +	kvm_slot.as_id = 0;
> +
> +	__pfn = gfn_to_pfn_memslot(&kvm_slot, gfn);
> +	if (is_error_noslot_pfn(__pfn)) {
> +		*pfn = 0;
> +		return -EFAULT;
> +	}

I have commented on this before: there is absolutely *no way* that you
can use KVM as the unwilling helper for your stuff. You are passing
uninitialised data to the core KVM, completely ignoring the semantics
of all the other fields.

More importantly, you are now holding us responsible for any breakage
that would be caused to your code if we change the internals of this
*PRIVATE FUNCTION*.

Do you see Xen or Hyper-V using KVM's internals as some sort of
backend to make their life easier? No, because they understand that
this is off-limits, and creates an unhealthy dependency for both
hypervisors.

So this is a strong NAK. And you can trust me to keep voicing my
opposition to this sort of horror, wherever I will see these patches.

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] virt: geniezone: Introduce GenieZone hypervisor support

[Yi-De Wu (吳一德)]

On Thu, 2023-05-18 at 09:27 +0100, Marc Zyngier wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, 12 May 2023 09:04:01 +0100,
> Yi-De Wu <yi-de.wu@mediatek.com> wrote:
> > 
> > From: "Yingshiuan Pan" <yingshiuan.pan@mediatek.com>
> > 
> > GenieZone is MediaTek hypervisor solution, and it is running in EL2
> > stand alone as a type-I hypervisor. This patch exports a set of
> > ioctl
> > interfaces for userspace VMM (e.g., crosvm) to operate guest VMs
> > lifecycle (creation and destroy) on GenieZone.
> > 
> > Signed-off-by: Yingshiuan Pan <yingshiuan.pan@mediatek.com>
> > Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>
> 
> [...]
> 
> > +/**
> > + * gzvm_gfn_to_pfn_memslot() - Translate gfn (guest ipa) to pfn
> > (host pa),
> > + *                          result is in @pfn
> > + *
> > + * Leverage KVM's gfn_to_pfn_memslot(). Because
> > gfn_to_pfn_memslot() needs
> > + * kvm_memory_slot as parameter, this function populates necessary
> > fileds
> > + * for calling gfn_to_pfn_memslot().
> > + *
> > + * Return:
> > + * * 0                       - Succeed
> > + * * -EFAULT         - Failed to convert
> > + */
> > +static int gzvm_gfn_to_pfn_memslot(struct gzvm_memslot *memslot,
> > u64 gfn, u64 *pfn)
> > +{
> > +     hfn_t __pfn;
> > +     struct kvm_memory_slot kvm_slot = {0};
> > +
> > +     kvm_slot.base_gfn = memslot->base_gfn;
> > +     kvm_slot.npages = memslot->npages;
> > +     kvm_slot.dirty_bitmap = NULL;
> > +     kvm_slot.userspace_addr = memslot->userspace_addr;
> > +     kvm_slot.flags = memslot->flags;
> > +     kvm_slot.id = memslot->slot_id;
> > +     kvm_slot.as_id = 0;
> > +
> > +     __pfn = gfn_to_pfn_memslot(&kvm_slot, gfn);
> > +     if (is_error_noslot_pfn(__pfn)) {
> > +             *pfn = 0;
> > +             return -EFAULT;
> > +     }
> 
> I have commented on this before: there is absolutely *no way* that
> you
> can use KVM as the unwilling helper for your stuff. You are passing
> uninitialised data to the core KVM, completely ignoring the semantics
> of all the other fields.
> 
> More importantly, you are now holding us responsible for any breakage
> that would be caused to your code if we change the internals of this
> *PRIVATE FUNCTION*.
> 
> Do you see Xen or Hyper-V using KVM's internals as some sort of
> backend to make their life easier? No, because they understand that
> this is off-limits, and creates an unhealthy dependency for both
> hypervisors.
> 
> So this is a strong NAK. And you can trust me to keep voicing my
> opposition to this sort of horror, wherever I will see these patches.
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

Noted and fully understood. The patch for this bug fix using our own
implementation would be submitted soon.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/7] dt-bindings: hypervisor: Add MediaTek GenieZone hypervisor

[Yi-De Wu (吳一德)]

On Fri, 2023-05-12 at 12:09 +0100, Conor Dooley wrote:
> On Fri, May 12, 2023 at 04:04:00PM +0800, Yi-De Wu wrote:
> > From: "Yingshiuan Pan" <yingshiuan.pan@mediatek.com>
> > 
> > Add documentation for GenieZone(gzvm) node. This node informs gzvm
> > driver to start probing if geniezone hypervisor is available and
> > able to do virtual machine operations.
> 
> Propagated from v2:
> > > Why can't the driver just try and do virtual machine operations
> > > to
> > > see
> > > if the hypervisor is there? IOW, make your software interfaces
> > > discoverable. DT is for non-discoverable hardware.
> > 
> > Can do, our hypervisor is discoverable through invoking probing
> > hypercall, and we use the device tree to prevent unnecessary module
> > loading on all systems.
> 
> Rob is out of office at the moment, but that appears to be a request
> to
> drop the use of devicetree entirely. Mainly re-posting so that that
> conversation appears on the latest version of the patchset, given you
> only replied to Rob today.
> 
> Thanks,
> Conor.

We will remove our dt here and use the discoverable way to initialize
our devices. V4 patches which contain the changes mentioned would be
submitted soon in recent days.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel