[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID |
Date: |
Thu, 16 Feb 2017 13:08:00 +0100 |
On Wed, 15 Feb 2017 21:52:55 +0100
Laszlo Ersek <address@hidden> wrote:
> On 02/15/17 21:09, Michael S. Tsirkin wrote:
> > On Wed, Feb 15, 2017 at 08:47:48PM +0100, Laszlo Ersek wrote:
>
> [snip]
>
> >> For patches #1, #3, #4 and #5:
> >>
> >> Tested-by: Laszlo Ersek <address@hidden>
> >>
> >> I'll soon post the OVMF patches.
> >>
> >> Thanks!
> >> Laszlo
> >
> >
> > How do you feel about Igor's request to change WRITE_POINTER to add
> > offset in there, so guest can pass in the address of GUID and
> > not start of table? Would that be a lot of work to add?
>
> I think it's doable in practice: simply add a constant from the command
> itself, for passing the value back to QEMU, and also for saving the
> fw_cfg write commend for S3 resume time.
>
> But, I disagree with it from a design POV.
>
> Igor's point is:
>
> > Math complicates QEMU code though and not only QMEMU but AML code as
> > well.
>
> As I understand it, the goal is to push the addition to the firmware
> (which is "one place"), rather than having to implement it twice in
> QEMU, i.e., in two places ((a) native QEMU logic, (b) AML generator).
>
> Here's my counter-argument:
>
> (a) As I mentioned earlier, assume a complex communication structure
> between the guest OS and QEMU. Currently our shared structure consists
> of a single field (the GUID), but next time it might contain several fields.
>
> For such a multi-field shared structure, QEMU will have to do manual
> offsetting into the guest RAM anyway, for accessing fields F1, F2, and
> F3. We will not create three separate WRITE_POINTER commands and let the
> firmware calculate and return the absolute GPAs of the fields F1, F2 and
> F3. Instead, there will be one WRITE_POINTER command, and QEMU will do
> the offsetting manually, minimally for fields F2 and F3.
>
> "src_offset" looks tempting now only because we have a shared structure
> with only one field, the GUID at offset 40 decimal.
benefits of having src_offset from QEMU POV I see are:
a) (biggest one) firmware and device code are clearly separated where:
- VMGENID_GUID_OFFSET would be used only by firmware side, such as:
WRITE_POINTER and AML addition to help OVMF detect non ACPI blob
- device doesn't have to assume/or have a knowledge about
layout of GUID blob except of size of data it's needs
to access at location provided by WRITE_POINTER as v7 shows it.
b) wrt shared blob I've envisioned slightly different approach,
where multiple WRITE_POINTER commands are used instead of one
with following workflow to extend shared blob:
1) firmware part of QEMU (acpi-build.c):
if (device_foo_present) {
fw_cfg_add_file_callback('/etc/device_foo_addr',
device_foo->addr_storage)
shared_off = device_foo->align(next_free_shared_offset)
WRITE_POINTER('/etc/device_foo_addr', 0,
'/etc/shared_blob, shared_off)
next_free_shared_offset = shared_off + device_foo->data_size;
}
2) device_foo accesses data at device_foo->addr_storage directly
* there is no need to spread knowledge of shared_blob
layout to device code anymore.
* no need to care where in shared_blob data will be placed,
* shared space is used only when device is present
* since there is no shared_writeback_blob, there isn't
need in mechanism to propagate written data to device
or notify device about write
drawback in this approach is that a device would consume
a file slot if fw_cfg and space for WRITE_POINTER in
linker file when present.
> (b) Regarding the runtime addition in the AML code:
as you pointed out WRITE_POINTER has nothing to do with addition
on AML side which is influenced by ADD_POINTER and OVMF and could
be fixed with flags down the road, so there is nothing to argue
about on this bullet.
> As discussed before, the main reason *now*, for not pointing VGIA (and
> other named integer objects) with ADD_POINTER commands directly to
> "meaningful" fields, is that OVMF probes the targets of ADD_POINTER
> commands for patterns that look like ACPI table headers. And, for the
> time being, we want to suppress any mis-recognitions by prepending some
> padding.
>
> Igor was right to dislike this, and we agreed that *down the road* we
> should add allocation flags, or further allocation commands, to supplant
> this kind of heuristics in OVMF. But:
>
> - we don't have time to do it now, plus
>
> - please observe that the runtime addition in AML relates to the
> ADD_POINTER and the ALLOCATE commands. It does not relate to
> WRITE_POINTER at all.
>
> Whatever we change on WRITE_POINTER will do nothing for suppressing
> OVMF's table header probing -- because that is tied to ADD_POINTER
> --, therefore WRITE_POINTER tweaks cannot eliminate the "need to add"
> in AML.
>
>
> In summary, I think the proposed WRITE_POINTER modification is
> implementable, but I think it will not pay off, because:
>
> (a) for QEMU logic, it will not prove useful as soon as we have a
> multi-field shared structure (QEMU will have to add field offsets anyway),
>
> (b) and for eliminating the AML addition (which is a consequence of the
> current ADD_POINTER handling in OVMF), it does nothing.
>
> Thanks
> Laszlo
- Re: [Qemu-devel] [PATCH v6 6/7] tests: Move reusable ACPI macros into a new header file, (continued)
Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID, Laszlo Ersek, 2017/02/15