Re: [Qemu-devel] Virtual Machine Generation ID

[Michael S. Tsirkin]

On Tue, Jan 17, 2017 at 05:24:04PM +0100, Igor Mammedov wrote:
> 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.

+1 thanks, thanks is what I was trying to say.

> BTW in-place update is racy anyways as OS could be reading it
> at the same time,

That would be up to the applications to use correctly.
E.g.

        check id
        send transaction
        ---> racy

        send transaction
        check id
        ---> not racy

I can't of course answer for it that all applications use the id correctly.
Some of them might decide to accept the risk of a race condition.

> but we can't do anything about it as vmgenid
> spec didn't provide means for atomic update.

I agree it's not up to us to fix.

> 
> > --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
> > >> >>  
> > >> >