Re: [PATCH v1 3/4] dt-bindings: Add bindings for vmgenid

From: Landge, Sudan
Date: Wed Mar 20 2024 - 08:16:36 EST


Hi Krzysztof,


The recipient were removed by mistake. I have added them all back and fixed the email client to send mail in the right format. Sorry about that.
I'll revert after I have done more analysis and better data to explain.
Thank you for the quick feedback again.

Thanks and regards,
Sudan

On 20/03/2024 10:24, Krzysztof Kozlowski wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



On 20/03/2024 11:17, Landge, Sudan wrote:

On 19/03/2024 15:28, Krzysztof Kozlowski wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



Why did you remove all the people from CC list?
>>>
On 19/03/2024 15:32, Sudan Landge wrote:
Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
("virt: vmgenid: notify RNG of VM fork and supply generation ID"), as an
ACPI only device.
That's not a valid rationale. Second today... we do not add things to
bindings just because someone added some crazy or not crazy idea to Linux.

Bindings represent the hardware.

Please come with real rationale. Even if this is accepted, above reason
is just wrong and will be used as an excuse to promote more crap into
bindings.

Thank you for the quick review.

I will add more details to the problem we are trying to fix with an
updated cover letter

but to summarize the problem briefly:

Firecracker is a minimalist feature hypervisor and we do not have ACPI
support

for ARM yet. The vmgenid devicetree support looked a better option because

supporting ACPI on ARM means supporting UEFI which adds a lot of complexity.

That does not convince me. Amount of work for you is not making virtual
stuff real hardware. Come with some other discoverable protocol - you
have full control of both sides of this thing.


A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.
Got it, thanks.
Add a devicetree binding support for vmgenid so that hypervisors
can support vmgenid without the need to support ACPI.
Devicetree is not for virtual platforms. Virtual platform can define
whatever interface they want (virtio, ACPI, "VTree" (just invented now)).
Sorry for my lack of experience in this area. I took reference of virtio
devices when I

uploaded the patch. We would still like to support vmgenid via a
devicetree so I'll

revert with a new approach.

There are other solutions, I think. This was discussed already multiple
times.


Signed-off-by: Sudan Landge<sudanl@xxxxxxxxxx>
---
.../devicetree/bindings/vmgenid/vmgenid.yaml | 57 +++++++++++++++++++
No, you do not get your own hardware subsystem. Use existing ones.

Got it. The changes are related to the "rng" subsystem so I'll rethink
if that is the

right place for this and revert.

Your wrapping is odd. Please use some decent email client.

Anyway, I am not discussing topics semi-private. Keep all maintainers in
loop.

Best regards,
Krzysztof