Re:Re: [PATCH v2 3/9] backends/hostmem-file: Add "rom" property to support VM templating with R/O files

[ThinerLogoer]

At 2023-08-23 20:43:48, "David Hildenbrand" <david@redhat.com> wrote:
>>> +        The ``rom`` option specifies whether to create Read Only Memory 
>>> (ROM)
>>> +        that cannot be modified by the VM. If set to ``on``, the VM cannot
>>> +        modify the memory. If set to ``off``, the VM can modify the memory.
>>> +        If set to ``auto`` (default), the value of the ``readonly`` 
>>> property
>>> +        is used. This option is primarily helpful for VM templating, where 
>>> we
>>> +        want to open a file readonly (``readonly=on``) and allow private
>>> +        modifications of the memory by the VM (``share=off``, ``rom=off``).
>>> +
>>>      ``-object 
>>> memory-backend-ram,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave``
>>>          Creates a memory backend object, which can be used to back the
>>>          guest RAM. Memory backend objects offer more control than the
>> 
>> In one word, I'd suggest advertising the existence of "rom" option more 
>> invasively, whenever
>> private file mapping is used.
>> 
>> IMHO you should probably be more invasive here to warn unconditionally when
>> the memory backend file is going to be opened readwrite but is mapped non 
>> shared.
>
>As Daniel said, we should not add new warnings for sane use cases. But we can 
>indeed give a hint when opening the file failed, see below.
>

Well I don't think it's completely sane use cases though we can keep it for 
backward
compatibility.

A nonshared memory map backed by an fd opened readwrite is probably
problematic and not what one usually expect. Either it should be shared
or it should opened readonly. Current behavior sticks to
the file being opened readwrite by default but warning can be raised when
readonly holds (file path does not prefix with /dev, and is a nonempty regular 
file,
and mapping is private), indicate that the user may probably want a
"readonly=on,rom=off" setup.

>> 
>> I as a user find the patch series indeed work functionally when I am aware 
>> of the "rom"
>> option - but what if I am not aware, the outcome is still that qemu tried
>> to open the file readwrite even when it is going to be mapped private.
>
>Yes, the implicit "readonly=off" is active in any case, and we cannot change 
>that due to existing users unfortunately.
>

Yeah I am ok with that now, and I found a way for my setup to work with your 
patches,
so I am happy with it.

>> 
>> When the file is readonly, the error message is:
>> ```
>> qemu-system-x86_64: can't open backing store pc.ram for guest RAM: 
>> Permission denied
>> ```
>> 
>> This should be probably helpful if qemu found that the file exists as a 
>> regular file and
>> is mapped private, to say something like
>> 
>> ```
>> qemu-system-x86_64: can't open backing store pc.ram for guest RAM: 
>> Permission denied
>> tip: mapping is private and ram file is probably readonly, maybe you should 
>> append "readonly=on,rom=off"
>> to "-object memory-backend-file,..." option list. see documentation xxx for 
>> details
>> ```
>
>What about the following, if we can indeed open the file R/O and we're dealing 
>witha  private mapping:
>
>qemu-system-x86_64: can't open backing store tmp-file for guest RAM: 
>Permission denied
>Consider opening the backing store read-only using '-object 
>memory-backend-file,readonly=on,rom=off,...' (see "VM templating" 
>documentation)
>
>?

This is good. Wording might need improvement. (Are qemu error messages always 
this cold?)

--

Regards,

logoerthiner