Re: [PATCH v13 04/25] virt: geniezone: Add GenieZone hypervisor driver

["Liju-clr Chen (陳麗如)" <Liju-clr.Chen@mediatek.com>]

On Wed, 2024-12-11 at 09:44 +0100, Krzysztof Kozlowski wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 14/11/2024 11:07, Liju-clr Chen wrote:
> > +
> > +static int gzvm_dev_open(struct inode *inode, struct file *file)
> > +{
> > +     /*
> > +      * Reference count to prevent this module is unload without
> > destroying
> > +      * VM
> 
> So you re-implemented suppress-bind attrs... no, drop.
> 

Thanks, will fix in next version.

> > +      */
> > +     try_module_get(THIS_MODULE);
> > +     return 0;
> > +}
> > +
> > +static int gzvm_dev_release(struct inode *inode, struct file
> > *file)
> > +{
> > +     module_put(THIS_MODULE);
> > +     return 0;
> > +}
> > +
> > +static const struct file_operations gzvm_chardev_ops = {
> > +     .llseek         = noop_llseek,
> > +     .open           = gzvm_dev_open,
> > +     .release        = gzvm_dev_release,
> > +};
> > +
> > +static struct miscdevice gzvm_dev = {
> > +     .minor = MISC_DYNAMIC_MINOR,
> > +     .name = KBUILD_MODNAME,
> > +     .fops = &gzvm_chardev_ops,
> > +};
> > +
> > +static int gzvm_drv_probe(struct platform_device *pdev)
> > +{
> > +     if (gzvm_arch_probe(gzvm_drv.drv_version,
> > &gzvm_drv.hyp_version) != 0) {
> > +             dev_err(&pdev->dev, "Not found available conduit\n");
> 
> So you can autodetect your hypervisor? Why your soc info drivers
> cannot
> instantiate this device thus removing any need for fake DT node (fake
> because no resources and used only to satisfy Linux driver
> instantiation)?
> 
> 
Hi Krzysztof,

I'm following up regarding your recent feedback on the MTK SoC driver
instantiation, as well as your earlier concerns about probing for the
hypervisor on all systems.

To recap your previous comment [1]:

> So for every system and architecture you want to: probe, run some SMC
> and then print error that it is not othe system you wanted.
>
> I don't think this is what we want. You basically pollute all of
other
> users just to have your hypervisor guest additions...

We understand the concern about unnecessary probing and potential
impact on platforms that do not support the GenieZone hypervisor.
However, using a generic SoC info driver is not practical in our case,
as not all products based on the same SoC support the hypervisor or
require the gzvm driver.

Previously, we attempted a device tree solution, but as Rob and Conor
pointed out, introducing a DT node without hardware resources is not
acceptable for upstreaming.

Given these constraints, we are considering reverting to the original
approach, where the driver probes for the hypervisor's presence
directly via HVC. To address your concern about system-wide pollution,
we have guarded the gzvm driver with the CONFIG_MTK_GZVM Kconfig
option. This ensures the code is only compiled and active on selected
platforms, and will not affect other users or systems.

If you have any further suggestions or see a better
solution for this scenario, we would appreciate your advice.

Thanks for your time and feedback.

[1]
https://lore.kernel.org/all/2fe0c7f9-55fc-ae63-3631-8526a0212ccd@linaro.org/

> > +             return -ENODEV;
> > +     }
> > +
> > +     pr_debug("Found GenieZone hypervisor version %u.%u.%llu\n",
> > +              gzvm_drv.hyp_version.major,
> > gzvm_drv.hyp_version.minor,
> > +              gzvm_drv.hyp_version.sub);
> > +
> > +     return misc_register(&gzvm_dev);
> > +}
> > +
> > +static void gzvm_drv_remove(struct platform_device *pdev)
> > +{
> > +     misc_deregister(&gzvm_dev);
> > +}
> > +
> > +static const struct of_device_id gzvm_of_match[] = {
> > +     { .compatible = "mediatek,geniezone" },
> > +     {/* sentinel */},
> > +};
> > +
> > +static struct platform_driver gzvm_driver = {
> > +     .probe = gzvm_drv_probe,
> > +     .remove = gzvm_drv_remove,
> > +     .driver = {
> > +             .name = KBUILD_MODNAME,
> > +             .of_match_table = gzvm_of_match,
> > +     },
> > +};
> > +
> > +module_platform_driver(gzvm_driver);
> > +
> > +MODULE_DEVICE_TABLE(of, gzvm_of_match);
> 
> This is immediately after next to ID table. Never in different place,
> so
> I wonder from which obscure code did you copy it and what other
> issues
> like that we can find...
> 

Thanks, will fix in next version.

> > +MODULE_AUTHOR("MediaTek");
> > +MODULE_DESCRIPTION("GenieZone interface for VMM");
> > +MODULE_LICENSE("GPL");
> Best regards,
> Krzysztof