Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID
On Thu, 16 Feb 2017 14:29:28 +0100
Laszlo Ersek wrote:
> On 02/16/17 13:08, Igor Mammedov wrote:
> > On Wed, 15 Feb 2017 21:52:55 +0100
> > Laszlo Ersek 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
>
> 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.
>
> This is where I disagree, I think. Above you mention
> device_foo->data_size. If "data_size" covers multiple fields, then the
> device code itself will have to add relative offsets, for accessing
> those different fields in guest RAM.
>
> With the current command, the only difference is that the device code
> has to receive a "base offset" from the outside, pointing into the
> shared blob, and then add the field offsets to that. Thus the addition
> cannot be avoided anyway.
instead of explicit offsets for base offset, device will probably use
struct cast to access fields.
> You do have a point about sharing the same area between different
> devices though. The above pseudo-code looks like a good pattern. This
> way "acpi-build.c" won't have to hand out the shared blob offsets to
> existing device instances directly; instead, the blob offsets are handed
> down to the firmware, and the devices will get their struct base
> addresses from the firmware. Using one WRITE_POINTER command for that,
> per device, seems fine.
>
> I'll update the OVMF patches.
Thanks!
>
> Thanks
> Laszlo
>
> > * 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
Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID
On 02/16/17 13:08, Igor Mammedov wrote:
> On Wed, 15 Feb 2017 21:52:55 +0100
> Laszlo Ersek 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
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.
This is where I disagree, I think. Above you mention
device_foo->data_size. If "data_size" covers multiple fields, then the
device code itself will have to add relative offsets, for accessing
those different fields in guest RAM.
With the current command, the only difference is that the device code
has to receive a "base offset" from the outside, pointing into the
shared blob, and then add the field offsets to that. Thus the addition
cannot be avoided anyway.
You do have a point about sharing the same area between different
devices though. The above pseudo-code looks like a good pattern. This
way "acpi-build.c" won't have to hand out the shared blob offsets to
existing device instances directly; instead, the blob offsets are handed
down to the firmware, and the devices will get their struct base
addresses from the firmware. Using one WRITE_POINTER command for that,
per device, seems fine.
I'll update the OVMF patches.
Thanks
Laszlo
> * 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
Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID
On Wed, 15 Feb 2017 21:52:55 +0100
Laszlo Ersek 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
> >>
> >> 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,
Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID
On 02/16/17 07:10, Ben Warren wrote: > >> On Feb 15, 2017, at 12:52 PM, Laszlo Ersek 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 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. >> >> (b) Regarding the runtime addition in the AML code: >> >> 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. >> > OK Laszlo, in v7 (imminent) I went ahead and implemented this > src_offset. If you are truly dead-set against it, it’s not very hard > to remove. To me it seems pretty harmless. I'd like to hear Igor's opinion about my counter-argument above. It would be a fair bit of work in OVMF: - it introduces a new addition which has to be range checked (the sum cannot overflow either the pointer object or the size of the pointed-to blob), - if the range check fails, that's a new error condition that needs handling, - in edk2 we document error conditions carefully, so it affects documentation too, for the function that processes WRITE_POINTER, - I'd have to redo all the testing. After I post the OVMF patches, we can count on about one week of review time (assuming I don't have to post a v2, that is!). That comes pretty close to the end of February, which is when I want to do a downstream OVMF rebase. (I can't do it in March due to a conference, and then it's too late.) The downstream rebase will take a few days too. I would have preferred to post the OVMF patches near the beginning of this week. So, as I said, it is implementable, I'm not "dead-set" against it, but the price for me to pay is definitely not zero. And, as it stands, I disagree with the design -- there don't seem to be any general benefits. I'd like to hear back from Igor. If he convincingly refutes my argument, I'm game. Thanks, Laszlo
Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID
> On Feb 15, 2017, at 12:52 PM, Laszlo Ersek 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 >>> >>> 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. > > (b) Regarding the runtime addition in the AML code: > > 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. > OK Laszlo, in v7 (imminent) I went ahead and implemented this src_offset. If you are truly dead-set against it, it’s not very hard to remove. To me it seems pretty harmless. > Thanks > Laszlo smime.p7s Description: S/MIME cryptographic signature
Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID
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 >> >> 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. (b) Regarding the runtime addition in the AML code: 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 0/7] Add support for VM Generation ID
> On Feb 15, 2017, at 12:09 PM, Michael S. Tsirkin wrote: > > On Wed, Feb 15, 2017 at 08:47:48PM +0100, Laszlo Ersek wrote: >> On 02/15/17 07:15, b...@skyportsystems.com wrote: >>> From: Ben Warren >>> >>> This patch set adds support for passing a GUID to Windows guests. It >>> is a re-implementation of previous patch sets written by Igor Mammedov >>> et al, but this time passing the GUID data as a fw_cfg blob. >>> >>> This patch set has dependencies on new guest functionality, in >>> particular the support for a new linker-loader command and the ability >>> to write back data to QEMU over a DMA link. Work is in flight in both >>> SeaBIOS and OVMF to support this. >>> >>> v5->v6: >>>- Rebased to top of tree. >>>- Changed device from sysbus to a simple device. This removed the need >>> for >>> adding dynamic sysbus support to pc_piix boards. >>>- Removed patch that introduced QWORD patching of AML. >>>- Removed ability to set GUID via QMP/HMP. >>>- Improved comments/documentation in code. >> >> So here's my testing with a RHEL-7 guest: >> >> (1) The command line option passed to QEMU is >> >> -device vmgenid,guid=00112233-4455-6677-8899-AABBCCDDEEFF >> >> This is the example GUID provided in the SMBIOS spec v3.0.0 (DSP0134), >> section 7.2.1 "System -- UUID". (SMBIOS is only relevant here because it >> codifies the fact that Microsoft consumes UUID in little-endian order.) >> The expected representation, according to the SMBIOS spec, is >> >> 33 22 11 00 55 44 77 66 88 99 AA BB CC DD EE FF >> >> (2) Here's an excerpt from the OVMF log: >> >>> ProcessCmdAllocate: File="etc/vmgenid_guid" Alignment=0x1000 Zone=1 >>> Size=0x1000 Address=0x7FE5C000 >> >> This is where "etc/vmgenid_guid" is allocated and downloaded, the >> allocation address is 0x7FE5C000. >> >>> Select Item: 0x19 >>> Select Item: 0x22 >>> ProcessCmdAllocate: File="etc/acpi/tables" Alignment=0x40 Zone=1 >>> Size=0x2 Address=0x7E7AB000 >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49 Start=0x40 >>> Length=0x1403 >>> ProcessCmdAddPointer: PointerFile="etc/acpi/tables" >>> PointeeFile="etc/acpi/tables" PointerOffset=0x1467 PointerSize=4 >>> ProcessCmdAddPointer: PointerFile="etc/acpi/tables" >>> PointeeFile="etc/acpi/tables" PointerOffset=0x146B PointerSize=4 >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x144C >>> Start=0x1443 Length=0x74 >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x14C0 >>> Start=0x14B7 Length=0x80 >>> Select Item: 0x19 >>> SaveCondensedWritePointerToS3Context: 0x002B/[0x+8] := 0x7FE5C000 >>> (0) >> >> This is where OVMF stashes the WRITE_POINTER command in "condensed" >> form, for S3. The fw_cfg selector value is 0x2B (for the fw_cfg file to >> be rewritten), the pointer is located at offset 0, has size 0, and the >> value to assign is 0x7FE5C000. And, this is #0 of the saved / condensed >> WRITE_POINTER commands. >> >>> Select Item: 0x2B >>> ProcessCmdWritePointer: PointerFile="etc/vmgenid_addr" >>> PointeeFile="etc/vmgenid_guid" PointerOffset=0x0 PointerSize=8 >> >> This is where the WRITE_POINTER command is actually executed, during >> normal boot. >> >>> ProcessCmdAddPointer: PointerFile="etc/acpi/tables" >>> PointeeFile="etc/vmgenid_guid" PointerOffset=0x1561 PointerSize=4 >> >> This is where we link "etc/vmgenid_guid" into VGIA. >> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x1540 >>> Start=0x1537 Length=0xCA >>> ProcessCmdAddPointer: PointerFile="etc/acpi/tables" >>> PointeeFile="etc/acpi/tables" PointerOffset=0x1625 PointerSize=4 >>> ProcessCmdAddPointer: PointerFile="etc/acpi/tables" >>> PointeeFile="etc/acpi/tables" PointerOffset=0x1629 PointerSize=4 >>> ProcessCmdAddPointer: PointerFile="etc/acpi/tables" >>> PointeeFile="etc/acpi/tables" PointerOffset=0x162D PointerSize=4 >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x160A >>> Start=0x1601 Length=0x30 >>> ProcessCmdAddPointer: PointerFile="etc/acpi/rsdp" >>> PointeeFile="etc/acpi/tables" PointerOffset=0x10 PointerSize=4 >>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 >>> Length=0x24 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwCfgTables: unknown loader command: 0x0 >>> InstallQemuFwC
Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID
On Wed, Feb 15, 2017 at 08:47:48PM +0100, Laszlo Ersek wrote: > On 02/15/17 07:15, b...@skyportsystems.com wrote: > > From: Ben Warren > > > > This patch set adds support for passing a GUID to Windows guests. It > > is a re-implementation of previous patch sets written by Igor Mammedov > > et al, but this time passing the GUID data as a fw_cfg blob. > > > > This patch set has dependencies on new guest functionality, in > > particular the support for a new linker-loader command and the ability > > to write back data to QEMU over a DMA link. Work is in flight in both > > SeaBIOS and OVMF to support this. > > > > v5->v6: > > - Rebased to top of tree. > > - Changed device from sysbus to a simple device. This removed the need > > for > > adding dynamic sysbus support to pc_piix boards. > > - Removed patch that introduced QWORD patching of AML. > > - Removed ability to set GUID via QMP/HMP. > > - Improved comments/documentation in code. > > So here's my testing with a RHEL-7 guest: > > (1) The command line option passed to QEMU is > > -device vmgenid,guid=00112233-4455-6677-8899-AABBCCDDEEFF > > This is the example GUID provided in the SMBIOS spec v3.0.0 (DSP0134), > section 7.2.1 "System -- UUID". (SMBIOS is only relevant here because it > codifies the fact that Microsoft consumes UUID in little-endian order.) > The expected representation, according to the SMBIOS spec, is > > 33 22 11 00 55 44 77 66 88 99 AA BB CC DD EE FF > > (2) Here's an excerpt from the OVMF log: > > > ProcessCmdAllocate: File="etc/vmgenid_guid" Alignment=0x1000 Zone=1 > > Size=0x1000 Address=0x7FE5C000 > > This is where "etc/vmgenid_guid" is allocated and downloaded, the > allocation address is 0x7FE5C000. > > > Select Item: 0x19 > > Select Item: 0x22 > > ProcessCmdAllocate: File="etc/acpi/tables" Alignment=0x40 Zone=1 > > Size=0x2 Address=0x7E7AB000 > > ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49 Start=0x40 > > Length=0x1403 > > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > > PointeeFile="etc/acpi/tables" PointerOffset=0x1467 PointerSize=4 > > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > > PointeeFile="etc/acpi/tables" PointerOffset=0x146B PointerSize=4 > > ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x144C > > Start=0x1443 Length=0x74 > > ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x14C0 > > Start=0x14B7 Length=0x80 > > Select Item: 0x19 > > SaveCondensedWritePointerToS3Context: 0x002B/[0x+8] := 0x7FE5C000 > > (0) > > This is where OVMF stashes the WRITE_POINTER command in "condensed" > form, for S3. The fw_cfg selector value is 0x2B (for the fw_cfg file to > be rewritten), the pointer is located at offset 0, has size 0, and the > value to assign is 0x7FE5C000. And, this is #0 of the saved / condensed > WRITE_POINTER commands. > > > Select Item: 0x2B > > ProcessCmdWritePointer: PointerFile="etc/vmgenid_addr" > > PointeeFile="etc/vmgenid_guid" PointerOffset=0x0 PointerSize=8 > > This is where the WRITE_POINTER command is actually executed, during > normal boot. > > > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > > PointeeFile="etc/vmgenid_guid" PointerOffset=0x1561 PointerSize=4 > > This is where we link "etc/vmgenid_guid" into VGIA. > > > ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x1540 > > Start=0x1537 Length=0xCA > > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > > PointeeFile="etc/acpi/tables" PointerOffset=0x1625 PointerSize=4 > > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > > PointeeFile="etc/acpi/tables" PointerOffset=0x1629 PointerSize=4 > > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > > PointeeFile="etc/acpi/tables" PointerOffset=0x162D PointerSize=4 > > ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x160A > > Start=0x1601 Length=0x30 > > ProcessCmdAddPointer: PointerFile="etc/acpi/rsdp" > > PointeeFile="etc/acpi/tables" PointerOffset=0x10 PointerSize=4 > > ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 > > Length=0x24 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > InstallQemuFwCfgTables: unknown loader command: 0x0 > > Process2ndPassCmdAddPointer: checking for ACPI header
Re: [Qemu-devel] [PATCH v6 0/7] Add support for VM Generation ID
On 02/15/17 07:15, b...@skyportsystems.com wrote: > From: Ben Warren > > This patch set adds support for passing a GUID to Windows guests. It > is a re-implementation of previous patch sets written by Igor Mammedov > et al, but this time passing the GUID data as a fw_cfg blob. > > This patch set has dependencies on new guest functionality, in > particular the support for a new linker-loader command and the ability > to write back data to QEMU over a DMA link. Work is in flight in both > SeaBIOS and OVMF to support this. > > v5->v6: > - Rebased to top of tree. > - Changed device from sysbus to a simple device. This removed the need > for > adding dynamic sysbus support to pc_piix boards. > - Removed patch that introduced QWORD patching of AML. > - Removed ability to set GUID via QMP/HMP. > - Improved comments/documentation in code. So here's my testing with a RHEL-7 guest: (1) The command line option passed to QEMU is -device vmgenid,guid=00112233-4455-6677-8899-AABBCCDDEEFF This is the example GUID provided in the SMBIOS spec v3.0.0 (DSP0134), section 7.2.1 "System -- UUID". (SMBIOS is only relevant here because it codifies the fact that Microsoft consumes UUID in little-endian order.) The expected representation, according to the SMBIOS spec, is 33 22 11 00 55 44 77 66 88 99 AA BB CC DD EE FF (2) Here's an excerpt from the OVMF log: > ProcessCmdAllocate: File="etc/vmgenid_guid" Alignment=0x1000 Zone=1 > Size=0x1000 Address=0x7FE5C000 This is where "etc/vmgenid_guid" is allocated and downloaded, the allocation address is 0x7FE5C000. > Select Item: 0x19 > Select Item: 0x22 > ProcessCmdAllocate: File="etc/acpi/tables" Alignment=0x40 Zone=1 Size=0x2 > Address=0x7E7AB000 > ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49 Start=0x40 > Length=0x1403 > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > PointeeFile="etc/acpi/tables" PointerOffset=0x1467 PointerSize=4 > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > PointeeFile="etc/acpi/tables" PointerOffset=0x146B PointerSize=4 > ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x144C > Start=0x1443 Length=0x74 > ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x14C0 > Start=0x14B7 Length=0x80 > Select Item: 0x19 > SaveCondensedWritePointerToS3Context: 0x002B/[0x+8] := 0x7FE5C000 (0) This is where OVMF stashes the WRITE_POINTER command in "condensed" form, for S3. The fw_cfg selector value is 0x2B (for the fw_cfg file to be rewritten), the pointer is located at offset 0, has size 0, and the value to assign is 0x7FE5C000. And, this is #0 of the saved / condensed WRITE_POINTER commands. > Select Item: 0x2B > ProcessCmdWritePointer: PointerFile="etc/vmgenid_addr" > PointeeFile="etc/vmgenid_guid" PointerOffset=0x0 PointerSize=8 This is where the WRITE_POINTER command is actually executed, during normal boot. > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > PointeeFile="etc/vmgenid_guid" PointerOffset=0x1561 PointerSize=4 This is where we link "etc/vmgenid_guid" into VGIA. > ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x1540 > Start=0x1537 Length=0xCA > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > PointeeFile="etc/acpi/tables" PointerOffset=0x1625 PointerSize=4 > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > PointeeFile="etc/acpi/tables" PointerOffset=0x1629 PointerSize=4 > ProcessCmdAddPointer: PointerFile="etc/acpi/tables" > PointeeFile="etc/acpi/tables" PointerOffset=0x162D PointerSize=4 > ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x160A > Start=0x1601 Length=0x30 > ProcessCmdAddPointer: PointerFile="etc/acpi/rsdp" > PointeeFile="etc/acpi/tables" PointerOffset=0x10 PointerSize=4 > ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 > Length=0x24 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > InstallQemuFwCfgTables: unknown loader command: 0x0 > Process2ndPassCmdAddPointer: checking for ACPI header in "etc/acpi/tables" at > 0x7E7AB000 (remaining: 0x2): found "FACS" size 0x40 > Process2ndPassCmdAddPointer: checking for ACPI header in "etc/acpi/tables" at > 0x7E7AB040 (remaining: 0x1FFC0): found "DSDT" size 0x1403 > Process2ndPassCmdAddPointer: checking for ACPI header
