Re: [Qemu-devel] Virtual Machine Generation ID

[Igor Mammedov]

On Tue, 17 Jan 2017 07:01:27 -0800
Ed Swierk <address@hidden> wrote:

> You mean what causes the guest to re-read the vmgenid guid? The
> vmgenid ACPI table defines a notify method, and when the guest
> receives the corresponding event, it re-reads the guid. (Also it
> appears that with Windows Server 2012 at least, if no notify method is
> defined, as is the case with Xen, the guest just re-reads it on
> demand.)
> 
> Wouldn't it be sufficient for the qmp set-vmgenid command to call
> acpi_ram_update() with the new guid, and acpi_send_event() to notify
> the guest?
pls note that acpi_ram_update() updates only memory on qemu side and
it's not mapped into guest. Updated memory will be re-read by firmware
when guest reboots.

But with vmgenid, QEMU might want to update guest RAM which
contains GUID without reboot, and for this it needs to know GPA
of GUID buffer and only after updating it send ACPI event.

BTW in-place update is racy anyways as OS could be reading it
at the same time, but we can't do anything about it as vmgenid
spec didn't provide means for atomic update.

> --Ed
> 
> 
> On Tue, Jan 17, 2017 at 6:33 AM, Michael S. Tsirkin <address@hidden> wrote:
> > Because this relies on guest to run code to read out the new fw cfg.
> > Thus guest can not reliably detect that host didn't update the gen id -
> > new one might be there in fw cfg but not yet read.
> >
> > RSDP never changes during guest lifetime so the issue does
> > not occur.
> >
> > On Tue, Jan 17, 2017 at 06:26:57AM -0800, Ed Swierk wrote:  
> >> Why is using fw_cfg for vmgenid preferable to the approach used for
> >> RSDP: call acpi_add_rom_blob() to allocate a MemoryRegion with the
> >> initial vmgenid guid, and call acpi_ram_update() with the new guid
> >> whenever the host needs to change it?
> >>
> >> --Ed
> >>
> >>
> >> On Tue, Jan 17, 2017 at 5:26 AM, Igor Mammedov <address@hidden> wrote:  
> >> > On Mon, 16 Jan 2017 10:57:42 -0800
> >> > Ben Warren <address@hidden> wrote:
> >> >  
> >> >> > On Jan 16, 2017, at 6:21 AM, Michael S. Tsirkin <address@hidden> 
> >> >> > wrote:
> >> >> >
> >> >> > On Sat, Jan 14, 2017 at 10:17:53PM -0800, Ben Warren wrote:  
> >> >> >> Hi Michael,
> >> >> >>
> >> >> >>
> >> >> >>    On Dec 10, 2016, at 7:28 PM, Michael S. Tsirkin <address@hidden> 
> >> >> >> wrote:
> >> >> >>
> >> >> >>    On Tue, Dec 06, 2016 at 06:15:34PM -0800, Ben Warren wrote:
> >> >> >>
> >> >> >>        Hi Michael,
> >> >> >>
> >> >> >>        I’m well on my way to implementing this, but I am really new 
> >> >> >> to the
> >> >> >>        QEMU code base and am struggling with some concepts.  Please 
> >> >> >> see below:
> >> >> >>
> >> >> >>            On Oct 5, 2016, at 6:29 PM, Michael S. Tsirkin 
> >> >> >> <address@hidden>
> >> >> >>            wrote:
> >> >> >>
> >> >> >>            On Tue, Oct 04, 2016 at 03:51:40PM -0700, Ed Swierk wrote:
> >> >> >>
> >> >> >>                On Thu, Sep 15, 2016 at 5:36 PM, Michael S. Tsirkin < 
> >> >> >>  
> >> >> >>                address@hidden> wrote:  
> >> >> >>
> >> >> >>                    On Thu, Sep 15, 2016 at 05:23:28PM -0700, Ed 
> >> >> >> Swierk wrote:
> >> >> >>
> >> >> >>                        I'm wondering what it will take to finish up 
> >> >> >> work on
> >> >> >>                        vmgenid.
> >> >> >>
> >> >> >>                        
> >> >> >> https://lists.gnu.org/archive/html/qemu-devel/2016-01/
> >> >> >>                        msg05599.html
> >> >> >>
> >> >> >>
> >> >> >>                    We have ACPI_BUILD_TPMLOG_FILE in tree now and I 
> >> >> >> think it
> >> >> >>                    could be
> >> >> >>                    allocated in a similar way.
> >> >> >>                    Integrate patch "fw-cfg: support writeable blobs" 
> >> >> >> to
> >> >> >>                    communicate the
> >> >> >>                    allocated address back to QEMU.
> >> >> >>
> >> >> >>
> >> >> >>                Starting with Igor's last version at
> >> >> >>                https://github.com/imammedo/qemu/commits/vmgen_wip , 
> >> >> >> it's not
> >> >> >>                clear to
> >> >> >>                me which changes need to be ported, which changes are 
> >> >> >> obsoleted
> >> >> >>                by
> >> >> >>                your new fw-cfg stuff and/or upstream churn in ACPI, 
> >> >> >> device
> >> >> >>                properties, etc. In particular ACPI is still a total 
> >> >> >> mystery to
> >> >> >>                me,
> >> >> >>                though passing a single address from guest to host 
> >> >> >> can't be
> >> >> >>                that hard,
> >> >> >>                can it?
> >> >> >>
> >> >> >>                Any clues would be appreciated.
> >> >> >>
> >> >> >>                --Ed
> >> >> >>
> >> >> >>
> >> >> >>            It might be best to just re-start from the beginning.
> >> >> >>            So the idea is that ACPI should be about supplying the 
> >> >> >> address
> >> >> >>            to guest. To supply address to host we'll use fw cfg.
> >> >> >>            This would be new I think:
> >> >> >>
> >> >> >>            - add support for writeable fw cfg blobs
> >> >> >>
> >> >> >>        patch applied
> >> >> >>
> >> >> >>            - add linker/loader command to write address of a blob 
> >> >> >> into
> >> >> >>            such a fw cfg file
> >> >> >>            - add a new file used for vm gen id, use loader command 
> >> >> >> above
> >> >> >>            to pass the address of a blob allocated for it to host
> >> >> >>
> >> >> >>        I don’t really understand the meaning of “file” in this 
> >> >> >> context.  It
> >> >> >>        seems to be a way of specifying individual fw_cfg entries 
> >> >> >> without
> >> >> >>        explicitly giving an index, but is not something that is 
> >> >> >> visible in
> >> >> >>        either the host or guest file system.  Is this about right?  
> >> >> >> In my code
> >> >> >>        I’m using “/etc/vmgenid”
> >> >> >>
> >> >> >>
> >> >> >>    yes
> >> >> >>
> >> >> >>
> >> >> >>        As for the blob, I’m thinking this is where my main problem 
> >> >> >> is.  The
> >> >> >>        ‘fw_cfg_add_*()’ functions take a data pointer but doesn’t 
> >> >> >> seem to copy
> >> >> >>        the data anywhere.  We pass essentially a pointer via ACPI to 
> >> >> >> the
> >> >> >>        guest, so what it points to needs to be in an accessible 
> >> >> >> region.  I
> >> >> >>        don’t get how to define the blob contents.  There are 
> >> >> >> command-line
> >> >> >>        ‘fw-cfg’ options where you can specify a file, but it’s not 
> >> >> >> clear to me
> >> >> >>        how to use them.  Maybe I reserve some IO memory or something?
> >> >> >>
> >> >> >>
> >> >> >>    Not sure I understand the question. fw cfg device will make
> >> >> >>    memory accessible to guest. Put the guest physical address there.
> >> >> >>    the address needs to be calculated by linker.
> >> >> >>
> >> >> >>
> >> >> >> I’m almost ready to submit a V2 of the patch set, but there’s still 
> >> >> >> one issue
> >> >> >> that I can’t figure out.  From the guest, I can read the contents of 
> >> >> >> the blob.
> >> >> >> If I make a change to the contents of the blob (via QMP) the guest 
> >> >> >> does not
> >> >> >> see the changes.  Is there something I need to do on the QEMU side 
> >> >> >> to “push”
> >> >> >> the updated fw_cfg contents to the guest?  I’ve noticed this both 
> >> >> >> when writing
> >> >> >> a qtest for the feature, and also in a Linux kernel module I wrote 
> >> >> >> that reads
> >> >> >> the ACPI contents in a guest.
> >> >> >>
> >> >> >> thanks,
> >> >> >> Ben  
> >> >> >
> >> >> > fw cfg entities are assumed to be immutable. This week
> >> >> > I'll merge support for writeable fw cfg entries.
> >> >> > I don't see why you want to change fw cfg transparently
> >> >> > though - I think it should be like this
> >> >> > - guest writes GPA into fw cfg
> >> >> > - qemu writes gen id at this GPA
> >> >> >  
> >> >> I’ve tried with your patch "fw-cfg-support-writeable-blobs”, setting 
> >> >> the ‘read-only’ flag on my blob to false:
> >> >>
> >> >> fw_cfg_add_file_callback(s, VMGENID_FW_CFG_FILE, NULL, NULL, 
> >> >> vms->guid.data, sizeof(vms->guid.data), false);
> >> >>
> >> >> and it doesn’t seem to make a difference.
> >> >>
> >> >> I think we have a misunderstanding here.  I’m storing the VM Generation 
> >> >> ID __data__ (a GUID) in a fw_cfg blob, not the address.  I’ll submit 
> >> >> the patch set ASAP so you will understand.  
> >> > there  should be another fw_cfg file for address so
> >> > that guest would be able to write it back into QEMU
> >> >  
> >> >>  
> >> >> > --
> >> >> > MST  
> >> >>
> >> >> regards,
> >> >> Ben
> >> >>  
> >> >