| Message ID | dabc5ffbb5b9d29e88087dcbe03b1ba0232c342c.1485308342.git.ben@skyportsystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ben, sorry about being late to reviewing this series. I hope I can now spend more time on it. - Please do not try to address my comments immediately. It's very possible (even likely) that Igor, MST and myself could have different opinions on things, so first please await agreement between your reviewers. - I think you should have CC'd Igor and Michael directly. I'm adding them to this reply; hopefully that will be enough for them to monitor this series. - I'll likely be unable to review everything with 100% coverage; so addressing (or sufficiently refuting) my comments might not guarantee that the next version will be merged at once. With all that said: On 01/25/17 02:43, ben@skyportsystems.com wrote: > From: Ben Warren <ben@skyportsystems.com> > > This is initially used to patch a 64-bit address into the VM Generation ID SSDT (1) I think this commit message line is overlong; I think we wrap at 74 chars or so. Not critical, but worth mentioning. > > Signed-off-by: Ben Warren <ben@skyportsystems.com> > --- > hw/acpi/aml-build.c | 28 ++++++++++++++++++++++++++++ > include/hw/acpi/aml-build.h | 4 ++++ > 2 files changed, 32 insertions(+) > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index b2a1e40..dc4edc2 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char *name_format, ...) > return offset; > } > > +/* > + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a qword, > + * and return the offset to 0x00000000 for runtime patching. > + * > + * Warning: runtime patching is best avoided. Only use this as > + * a replacement for DataTableRegion (for guests that don't > + * support it). > + */ (2) Since we're adding a qword (8-byte integer), the hexadecimal constant should have 16 nibbles: 0x0000000000000000. (After copying the comment from build_append_named_dword(), it should be actualized.) (3) Normally the functions in this file that create AML operators carry a note about the ACPI spec version and exact location that defines the operator. I see that commit f20354910893 ("acpi: add build_append_named_dword, returning an offset in buffer", 2016-03-01) missed that too. Unless this tradition has been willfully abandoned, I suggest that you add the right reference here, and also (in retrospect) to build_append_named_dword(). > +int > +build_append_named_qword(GArray *array, const char *name_format, ...) > +{ > + int offset; > + va_list ap; > + > + build_append_byte(array, 0x08); /* NameOp */ > + va_start(ap, name_format); > + build_append_namestringv(array, name_format, ap); > + va_end(ap); > + > + build_append_byte(array, 0x0E); /* QWordPrefix */ > + > + offset = array->len; > + build_append_int_noprefix(array, 0x00000000, 8); (4) I guess the constant should be updated here too, for consistency with the comment. The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically, because an error there would break the functionality immediately, and you'd notice.) Thanks! Laszlo > + assert(array->len == offset + 8); > + > + return offset; > +} > + > static GPtrArray *alloc_list; > > static Aml *aml_alloc(void) > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > index 559326c..dbf63cf 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -385,6 +385,10 @@ int > build_append_named_dword(GArray *array, const char *name_format, ...) > GCC_FMT_ATTR(2, 3); > > +int > +build_append_named_qword(GArray *array, const char *name_format, ...) > +GCC_FMT_ATTR(2, 3); > + > void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base, > uint64_t len, int node, MemoryAffinityFlags flags); > >
Hi Laszlo, > On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com> wrote: > > Hi Ben, > > sorry about being late to reviewing this series. I hope I can now spend > more time on it. > > - Please do not try to address my comments immediately. It's very > possible (even likely) that Igor, MST and myself could have different > opinions on things, so first please await agreement between your reviewers. > Thanks for the very detailed review. I’ll give it a couple of days and then start work on the suggested changes. > - I think you should have CC'd Igor and Michael directly. I'm adding > them to this reply; hopefully that will be enough for them to monitor > this series. > > - I'll likely be unable to review everything with 100% coverage; so > addressing (or sufficiently refuting) my comments might not guarantee > that the next version will be merged at once. > > With all that said: > > On 01/25/17 02:43, ben@skyportsystems.com <mailto:ben@skyportsystems.com> wrote: >> From: Ben Warren <ben@skyportsystems.com <mailto:ben@skyportsystems.com>> >> >> This is initially used to patch a 64-bit address into the VM Generation ID SSDT > > (1) I think this commit message line is overlong; I think we wrap at 74 > chars or so. Not critical, but worth mentioning. > >> >> Signed-off-by: Ben Warren <ben@skyportsystems.com> >> --- >> hw/acpi/aml-build.c | 28 ++++++++++++++++++++++++++++ >> include/hw/acpi/aml-build.h | 4 ++++ >> 2 files changed, 32 insertions(+) >> >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >> index b2a1e40..dc4edc2 100644 >> --- a/hw/acpi/aml-build.c >> +++ b/hw/acpi/aml-build.c >> @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char *name_format, ...) >> return offset; >> } >> >> +/* >> + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a qword, >> + * and return the offset to 0x00000000 for runtime patching. >> + * >> + * Warning: runtime patching is best avoided. Only use this as >> + * a replacement for DataTableRegion (for guests that don't >> + * support it). >> + */ > > (2) Since we're adding a qword (8-byte integer), the hexadecimal > constant should have 16 nibbles: 0x0000000000000000. (After copying the > comment from build_append_named_dword(), it should be actualized.) > > (3) Normally the functions in this file that create AML operators carry > a note about the ACPI spec version and exact location that defines the > operator. I see that commit f20354910893 ("acpi: add > build_append_named_dword, returning an offset in buffer", 2016-03-01) > missed that too. > > Unless this tradition has been willfully abandoned, I suggest that you > add the right reference here, and also (in retrospect) to > build_append_named_dword(). > >> +int >> +build_append_named_qword(GArray *array, const char *name_format, ...) >> +{ >> + int offset; >> + va_list ap; >> + >> + build_append_byte(array, 0x08); /* NameOp */ >> + va_start(ap, name_format); >> + build_append_namestringv(array, name_format, ap); >> + va_end(ap); >> + >> + build_append_byte(array, 0x0E); /* QWordPrefix */ >> + >> + offset = array->len; >> + build_append_int_noprefix(array, 0x00000000, 8); > > (4) I guess the constant should be updated here too, for consistency > with the comment. > > The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically, > because an error there would break the functionality immediately, and > you'd notice.) > > Thanks! > Laszlo > >> + assert(array->len == offset + 8); >> + >> + return offset; >> +} >> + >> static GPtrArray *alloc_list; >> >> static Aml *aml_alloc(void) >> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h >> index 559326c..dbf63cf 100644 >> --- a/include/hw/acpi/aml-build.h >> +++ b/include/hw/acpi/aml-build.h >> @@ -385,6 +385,10 @@ int >> build_append_named_dword(GArray *array, const char *name_format, ...) >> GCC_FMT_ATTR(2, 3); >> >> +int >> +build_append_named_qword(GArray *array, const char *name_format, ...) >> +GCC_FMT_ATTR(2, 3); >> + >> void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base, >> uint64_t len, int node, MemoryAffinityFlags flags); —Ben
On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote: > Hi Laszlo, > > > On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com> wrote: > > Hi Ben, > > sorry about being late to reviewing this series. I hope I can now spend > more time on it. > > - Please do not try to address my comments immediately. It's very > possible (even likely) that Igor, MST and myself could have different > opinions on things, so first please await agreement between your reviewers. > > > Thanks for the very detailed review. I’ll give it a couple of days and then > start work on the suggested changes. > > > - I think you should have CC'd Igor and Michael directly. I'm adding > them to this reply; hopefully that will be enough for them to monitor > this series. > > - I'll likely be unable to review everything with 100% coverage; so > addressing (or sufficiently refuting) my comments might not guarantee > that the next version will be merged at once. > > With all that said: > > On 01/25/17 02:43, ben@skyportsystems.com wrote: > > From: Ben Warren <ben@skyportsystems.com> > > This is initially used to patch a 64-bit address into the VM Generation > ID SSDT > > > (1) I think this commit message line is overlong; I think we wrap at 74 > chars or so. Not critical, but worth mentioning. > > > > Signed-off-by: Ben Warren <ben@skyportsystems.com> > --- > hw/acpi/aml-build.c | 28 ++++++++++++++++++++++++++++ > include/hw/acpi/aml-build.h | 4 ++++ > 2 files changed, 32 insertions(+) > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index b2a1e40..dc4edc2 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char > *name_format, ...) > return offset; > } > > +/* > + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a > qword, > + * and return the offset to 0x00000000 for runtime patching. > + * > + * Warning: runtime patching is best avoided. Only use this as > + * a replacement for DataTableRegion (for guests that don't > + * support it). > + */ only one comment: QWords first appeared in ACPI 2.0 and XP does not like them. Not strictly a blocker as people can avoid using the feature, but nice to have. Will either UEFI or seabios allocate memory outside 4G range? If not you do not need a qword. > > (2) Since we're adding a qword (8-byte integer), the hexadecimal > constant should have 16 nibbles: 0x0000000000000000. (After copying the > comment from build_append_named_dword(), it should be actualized.) > > (3) Normally the functions in this file that create AML operators carry > a note about the ACPI spec version and exact location that defines the > operator. I see that commit f20354910893 ("acpi: add > build_append_named_dword, returning an offset in buffer", 2016-03-01) > missed that too. > > Unless this tradition has been willfully abandoned, I suggest that you > add the right reference here, and also (in retrospect) to > build_append_named_dword(). > > > +int > +build_append_named_qword(GArray *array, const char *name_format, ...) > +{ > + int offset; > + va_list ap; > + > + build_append_byte(array, 0x08); /* NameOp */ > + va_start(ap, name_format); > + build_append_namestringv(array, name_format, ap); > + va_end(ap); > + > + build_append_byte(array, 0x0E); /* QWordPrefix */ > + > + offset = array->len; > + build_append_int_noprefix(array, 0x00000000, 8); > > > (4) I guess the constant should be updated here too, for consistency > with the comment. > > The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically, > because an error there would break the functionality immediately, and > you'd notice.) > > Thanks! > Laszlo > > > + assert(array->len == offset + 8); > + > + return offset; > +} > + > static GPtrArray *alloc_list; > > static Aml *aml_alloc(void) > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > index 559326c..dbf63cf 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -385,6 +385,10 @@ int > build_append_named_dword(GArray *array, const char *name_format, ...) > GCC_FMT_ATTR(2, 3); > > +int > +build_append_named_qword(GArray *array, const char *name_format, ...) > +GCC_FMT_ATTR(2, 3); > + > void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base, > uint64_t len, int node, MemoryAffinityFlags > flags); > > > —Ben >
On 01/25/17 19:35, Michael S. Tsirkin wrote: > On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote: >> Hi Laszlo, >> >> >> On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com> wrote: >> >> Hi Ben, >> >> sorry about being late to reviewing this series. I hope I can now spend >> more time on it. >> >> - Please do not try to address my comments immediately. It's very >> possible (even likely) that Igor, MST and myself could have different >> opinions on things, so first please await agreement between your reviewers. >> >> >> Thanks for the very detailed review. I’ll give it a couple of days and then >> start work on the suggested changes. >> >> >> - I think you should have CC'd Igor and Michael directly. I'm adding >> them to this reply; hopefully that will be enough for them to monitor >> this series. >> >> - I'll likely be unable to review everything with 100% coverage; so >> addressing (or sufficiently refuting) my comments might not guarantee >> that the next version will be merged at once. >> >> With all that said: >> >> On 01/25/17 02:43, ben@skyportsystems.com wrote: >> >> From: Ben Warren <ben@skyportsystems.com> >> >> This is initially used to patch a 64-bit address into the VM Generation >> ID SSDT >> >> >> (1) I think this commit message line is overlong; I think we wrap at 74 >> chars or so. Not critical, but worth mentioning. >> >> >> >> Signed-off-by: Ben Warren <ben@skyportsystems.com> >> --- >> hw/acpi/aml-build.c | 28 ++++++++++++++++++++++++++++ >> include/hw/acpi/aml-build.h | 4 ++++ >> 2 files changed, 32 insertions(+) >> >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >> index b2a1e40..dc4edc2 100644 >> --- a/hw/acpi/aml-build.c >> +++ b/hw/acpi/aml-build.c >> @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char >> *name_format, ...) >> return offset; >> } >> >> +/* >> + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a >> qword, >> + * and return the offset to 0x00000000 for runtime patching. >> + * >> + * Warning: runtime patching is best avoided. Only use this as >> + * a replacement for DataTableRegion (for guests that don't >> + * support it). >> + */ > > only one comment: QWords first appeared in ACPI 2.0 and > XP does not like them. Not strictly a blocker as people can > avoid using the feature, but nice to have. Does XP have a driver for VMGENID? If not, then I'd prefer to stick with the qword VGIA. > Will either UEFI or seabios allocate > memory outside 4G range? If not you do not need a qword. Good point (assuming XP has a driver for VMGENID). OVMF keeps all such allocations (i.e., for COMMAND_ALLOCATE and the upcoming COMMAND_ALLOCATE_RETURN_ADDR) under 4GB, so as far as OVMF is concerned, using a dword for the VGIA named object should be fine. Accordingly, a 4-byte wide ADD_POINTER command should be used for patching VGIA. Considering the fw_cfg file that receives the address, and COMMAND_ALLOCATE_RETURN_ADDR more generally, I'd still prefer if those stayed 8-byte wide, regardless of XP's support for VMGENID. Hm... It looks like VMGENID *can* be consumed on Windows XP SP3, as long as "Hyper-V integration services" are installed: https://msdn.microsoft.com/en-us/library/jj643357(v=vs.85).aspx The virtual machine must be running a guest operating system that has support for the virtual machine generation identifier. Currently, these are the following. The following operating systems have native support for the virtual machine generation identifier. [...] The following operating can be used as the guest operating system if the Hyper-V integration services from Windows 8 or Windows Server 2012 are installed. [...] * Windows XP with Service Pack 3 (SP3) Additionally, under <https://technet.microsoft.com/en-us/library/dn792027(v=ws.11).aspx>: Supported Windows client guest operating systems Windows XP with [...] Install the integration [...] Service Pack 3 (SP3) services after you set up the operating system in the virtual machine. This seems to be consistent with the VMGENID spec requirement that the ADDR method return a package of two 32-bit integers, not a single 64-bit one. So, I agree with using a dword for VGIA (and a matching 4-byte wide ADD_POINTER command). But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. In the future we might introduce more allocation hints (for the "zone" field) that would enable the firmware to allocate from the full 64-bit address space. NB, I didn't check SeaBIOS (should be easy for Ben); AFAIR it only uses 16-bit and 32-bit modes. Thanks! Laszlo > > > > >> >> (2) Since we're adding a qword (8-byte integer), the hexadecimal >> constant should have 16 nibbles: 0x0000000000000000. (After copying the >> comment from build_append_named_dword(), it should be actualized.) >> >> (3) Normally the functions in this file that create AML operators carry >> a note about the ACPI spec version and exact location that defines the >> operator. I see that commit f20354910893 ("acpi: add >> build_append_named_dword, returning an offset in buffer", 2016-03-01) >> missed that too. >> >> Unless this tradition has been willfully abandoned, I suggest that you >> add the right reference here, and also (in retrospect) to >> build_append_named_dword(). >> >> >> +int >> +build_append_named_qword(GArray *array, const char *name_format, ...) >> +{ >> + int offset; >> + va_list ap; >> + >> + build_append_byte(array, 0x08); /* NameOp */ >> + va_start(ap, name_format); >> + build_append_namestringv(array, name_format, ap); >> + va_end(ap); >> + >> + build_append_byte(array, 0x0E); /* QWordPrefix */ >> + >> + offset = array->len; >> + build_append_int_noprefix(array, 0x00000000, 8); >> >> >> (4) I guess the constant should be updated here too, for consistency >> with the comment. >> >> The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically, >> because an error there would break the functionality immediately, and >> you'd notice.) >> >> Thanks! >> Laszlo >> >> >> + assert(array->len == offset + 8); >> + >> + return offset; >> +} >> + >> static GPtrArray *alloc_list; >> >> static Aml *aml_alloc(void) >> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h >> index 559326c..dbf63cf 100644 >> --- a/include/hw/acpi/aml-build.h >> +++ b/include/hw/acpi/aml-build.h >> @@ -385,6 +385,10 @@ int >> build_append_named_dword(GArray *array, const char *name_format, ...) >> GCC_FMT_ATTR(2, 3); >> >> +int >> +build_append_named_qword(GArray *array, const char *name_format, ...) >> +GCC_FMT_ATTR(2, 3); >> + >> void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base, >> uint64_t len, int node, MemoryAffinityFlags >> flags); >> >> >> —Ben >> > >
> On Jan 25, 2017, at 4:48 PM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 01/25/17 19:35, Michael S. Tsirkin wrote: >> On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote: >>> Hi Laszlo, >>> >>> >>> On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>> >>> Hi Ben, >>> >>> sorry about being late to reviewing this series. I hope I can now spend >>> more time on it. >>> >>> - Please do not try to address my comments immediately. It's very >>> possible (even likely) that Igor, MST and myself could have different >>> opinions on things, so first please await agreement between your reviewers. >>> >>> >>> Thanks for the very detailed review. I’ll give it a couple of days and then >>> start work on the suggested changes. >>> >>> >>> - I think you should have CC'd Igor and Michael directly. I'm adding >>> them to this reply; hopefully that will be enough for them to monitor >>> this series. >>> >>> - I'll likely be unable to review everything with 100% coverage; so >>> addressing (or sufficiently refuting) my comments might not guarantee >>> that the next version will be merged at once. >>> >>> With all that said: >>> >>> On 01/25/17 02:43, ben@skyportsystems.com wrote: >>> >>> From: Ben Warren <ben@skyportsystems.com> >>> >>> This is initially used to patch a 64-bit address into the VM Generation >>> ID SSDT >>> >>> >>> (1) I think this commit message line is overlong; I think we wrap at 74 >>> chars or so. Not critical, but worth mentioning. >>> >>> >>> >>> Signed-off-by: Ben Warren <ben@skyportsystems.com> >>> --- >>> hw/acpi/aml-build.c | 28 ++++++++++++++++++++++++++++ >>> include/hw/acpi/aml-build.h | 4 ++++ >>> 2 files changed, 32 insertions(+) >>> >>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >>> index b2a1e40..dc4edc2 100644 >>> --- a/hw/acpi/aml-build.c >>> +++ b/hw/acpi/aml-build.c >>> @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char >>> *name_format, ...) >>> return offset; >>> } >>> >>> +/* >>> + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a >>> qword, >>> + * and return the offset to 0x00000000 for runtime patching. >>> + * >>> + * Warning: runtime patching is best avoided. Only use this as >>> + * a replacement for DataTableRegion (for guests that don't >>> + * support it). >>> + */ >> >> only one comment: QWords first appeared in ACPI 2.0 and >> XP does not like them. Not strictly a blocker as people can >> avoid using the feature, but nice to have. > > Does XP have a driver for VMGENID? > > If not, then I'd prefer to stick with the qword VGIA. > >> Will either UEFI or seabios allocate >> memory outside 4G range? If not you do not need a qword. > > Good point (assuming XP has a driver for VMGENID). > > OVMF keeps all such allocations (i.e., for COMMAND_ALLOCATE and the > upcoming COMMAND_ALLOCATE_RETURN_ADDR) under 4GB, so as far as OVMF is > concerned, using a dword for the VGIA named object should be fine. > Accordingly, a 4-byte wide ADD_POINTER command should be used for > patching VGIA. > > Considering the fw_cfg file that receives the address, and > COMMAND_ALLOCATE_RETURN_ADDR more generally, I'd still prefer if those > stayed 8-byte wide, regardless of XP's support for VMGENID. > > > Hm... It looks like VMGENID *can* be consumed on Windows XP SP3, as long > as "Hyper-V integration services" are installed: > > https://msdn.microsoft.com/en-us/library/jj643357(v=vs.85).aspx <https://msdn.microsoft.com/en-us/library/jj643357(v=vs.85).aspx> > > The virtual machine must be running a guest operating system that > has support for the virtual machine generation identifier. > Currently, these are the following. > The following operating systems have native support for the virtual > machine generation identifier. > [...] > > The following operating can be used as the guest operating system > if the Hyper-V integration services from Windows 8 or Windows > Server 2012 are installed. > > [...] > * Windows XP with Service Pack 3 (SP3) > > Additionally, under > <https://technet.microsoft.com/en-us/library/dn792027(v=ws.11).aspx <https://technet.microsoft.com/en-us/library/dn792027(v=ws.11).aspx>>: > > Supported Windows client guest operating systems > > Windows XP with [...] Install the integration [...] > Service Pack 3 (SP3) services after you set > up the operating system > in the virtual machine. > > This seems to be consistent with the VMGENID spec requirement that the > ADDR method return a package of two 32-bit integers, not a single 64-bit > one. > > So, I agree with using a dword for VGIA (and a matching 4-byte wide > ADD_POINTER command). > > But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. > In the future we might introduce more allocation hints (for the "zone" > field) that would enable the firmware to allocate from the full 64-bit > address space. > > NB, I didn't check SeaBIOS (should be easy for Ben); AFAIR it only uses > 16-bit and 32-bit modes. > Right, it appears that the allocated address will always fit in 32 bits. As mentioned, XP should be OK since the ADDR method returns a package of two 32-bit numbers. I propose to still include this patch but touch up the comments as requested by Laszlo. This way it will be in the toolbox for future users and has been tested. I will also change VGIA to dword and hard code the AML to return ADDR[1] = 0. FYI: here’s an iasl dump from Windows 2012 Hyper-V in case you haven’t seen it. Note that Microsoft uses E00 and violates the HID name length spec: Scope (\_SB) { Device (GENC) { Name (_CID, "VM_Gen_Counter") // _CID: Compatible ID Name (_HID, "Hyper_V_Gen_Counter_V1") // _HID: Hardware ID Name (_UID, 0x00) // _UID: Unique ID Name (_DDN, "VM_Gen_Counter") // _DDN: DOS Device Name Method (ADDR, 0, NotSerialized) { Name (LPKG, Package (0x02) { 0x00, 0x00 }) LPKG [0x00] = GCAL /* \GCAL */ LPKG [0x01] = GCAH /* \GCAH */ Return (LPKG) /* \_SB_.GENC.ADDR.LPKG */ } } } Scope (\_GPE) { Method (_E00, 0, NotSerialized) // _Exx: Edge-Triggered GPE { Notify (\_SB.GENC, 0x80) // Status Change } } > Thanks! > Laszlo > >> >> >> >> >>> >>> (2) Since we're adding a qword (8-byte integer), the hexadecimal >>> constant should have 16 nibbles: 0x0000000000000000. (After copying the >>> comment from build_append_named_dword(), it should be actualized.) >>> >>> (3) Normally the functions in this file that create AML operators carry >>> a note about the ACPI spec version and exact location that defines the >>> operator. I see that commit f20354910893 ("acpi: add >>> build_append_named_dword, returning an offset in buffer", 2016-03-01) >>> missed that too. >>> >>> Unless this tradition has been willfully abandoned, I suggest that you >>> add the right reference here, and also (in retrospect) to >>> build_append_named_dword(). >>> >>> >>> +int >>> +build_append_named_qword(GArray *array, const char *name_format, ...) >>> +{ >>> + int offset; >>> + va_list ap; >>> + >>> + build_append_byte(array, 0x08); /* NameOp */ >>> + va_start(ap, name_format); >>> + build_append_namestringv(array, name_format, ap); >>> + va_end(ap); >>> + >>> + build_append_byte(array, 0x0E); /* QWordPrefix */ >>> + >>> + offset = array->len; >>> + build_append_int_noprefix(array, 0x00000000, 8); >>> >>> >>> (4) I guess the constant should be updated here too, for consistency >>> with the comment. >>> >>> The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically, >>> because an error there would break the functionality immediately, and >>> you'd notice.) >>> >>> Thanks! >>> Laszlo >>> >>> >>> + assert(array->len == offset + 8); >>> + >>> + return offset; >>> +} >>> + >>> static GPtrArray *alloc_list; >>> >>> static Aml *aml_alloc(void) >>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h >>> index 559326c..dbf63cf 100644 >>> --- a/include/hw/acpi/aml-build.h >>> +++ b/include/hw/acpi/aml-build.h >>> @@ -385,6 +385,10 @@ int >>> build_append_named_dword(GArray *array, const char *name_format, ...) >>> GCC_FMT_ATTR(2, 3); >>> >>> +int >>> +build_append_named_qword(GArray *array, const char *name_format, ...) >>> +GCC_FMT_ATTR(2, 3); >>> + >>> void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base, >>> uint64_t len, int node, MemoryAffinityFlags >>> flags); >>> >>> >>> —Ben
On 01/26/17 06:35, Ben Warren wrote: > >> On Jan 25, 2017, at 4:48 PM, Laszlo Ersek <lersek@redhat.com >> <mailto:lersek@redhat.com>> wrote: >> >> On 01/25/17 19:35, Michael S. Tsirkin wrote: >>> On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote: >>>> Hi Laszlo, >>>> >>>> >>>> On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com >>>> <mailto:lersek@redhat.com>> wrote: >>>> >>>> Hi Ben, >>>> >>>> sorry about being late to reviewing this series. I hope I can now >>>> spend >>>> more time on it. >>>> >>>> - Please do not try to address my comments immediately. It's very >>>> possible (even likely) that Igor, MST and myself could have different >>>> opinions on things, so first please await agreement between your >>>> reviewers. >>>> >>>> >>>> Thanks for the very detailed review. I’ll give it a couple of days >>>> and then >>>> start work on the suggested changes. >>>> >>>> >>>> - I think you should have CC'd Igor and Michael directly. I'm adding >>>> them to this reply; hopefully that will be enough for them to monitor >>>> this series. >>>> >>>> - I'll likely be unable to review everything with 100% coverage; so >>>> addressing (or sufficiently refuting) my comments might not guarantee >>>> that the next version will be merged at once. >>>> >>>> With all that said: >>>> >>>> On 01/25/17 02:43, ben@skyportsystems.com >>>> <mailto:ben@skyportsystems.com> wrote: >>>> >>>> From: Ben Warren <ben@skyportsystems.com >>>> <mailto:ben@skyportsystems.com>> >>>> >>>> This is initially used to patch a 64-bit address into the VM >>>> Generation >>>> ID SSDT >>>> >>>> >>>> (1) I think this commit message line is overlong; I think we wrap >>>> at 74 >>>> chars or so. Not critical, but worth mentioning. >>>> >>>> >>>> >>>> Signed-off-by: Ben Warren <ben@skyportsystems.com >>>> <mailto:ben@skyportsystems.com>> >>>> --- >>>> hw/acpi/aml-build.c | 28 ++++++++++++++++++++++++++++ >>>> include/hw/acpi/aml-build.h | 4 ++++ >>>> 2 files changed, 32 insertions(+) >>>> >>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >>>> index b2a1e40..dc4edc2 100644 >>>> --- a/hw/acpi/aml-build.c >>>> +++ b/hw/acpi/aml-build.c >>>> @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, >>>> const char >>>> *name_format, ...) >>>> return offset; >>>> } >>>> >>>> +/* >>>> + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a >>>> qword, >>>> + * and return the offset to 0x00000000 for runtime patching. >>>> + * >>>> + * Warning: runtime patching is best avoided. Only use this as >>>> + * a replacement for DataTableRegion (for guests that don't >>>> + * support it). >>>> + */ >>> >>> only one comment: QWords first appeared in ACPI 2.0 and >>> XP does not like them. Not strictly a blocker as people can >>> avoid using the feature, but nice to have. >> >> Does XP have a driver for VMGENID? >> >> If not, then I'd prefer to stick with the qword VGIA. >> >>> Will either UEFI or seabios allocate >>> memory outside 4G range? If not you do not need a qword. >> >> Good point (assuming XP has a driver for VMGENID). >> >> OVMF keeps all such allocations (i.e., for COMMAND_ALLOCATE and the >> upcoming COMMAND_ALLOCATE_RETURN_ADDR) under 4GB, so as far as OVMF is >> concerned, using a dword for the VGIA named object should be fine. >> Accordingly, a 4-byte wide ADD_POINTER command should be used for >> patching VGIA. >> >> Considering the fw_cfg file that receives the address, and >> COMMAND_ALLOCATE_RETURN_ADDR more generally, I'd still prefer if those >> stayed 8-byte wide, regardless of XP's support for VMGENID. >> >> >> Hm... It looks like VMGENID *can* be consumed on Windows XP SP3, as long >> as "Hyper-V integration services" are installed: >> >> https://msdn.microsoft.com/en-us/library/jj643357(v=vs.85).aspx >> >> The virtual machine must be running a guest operating system that >> has support for the virtual machine generation identifier. >> Currently, these are the following. >> The following operating systems have native support for the virtual >> machine generation identifier. >> [...] >> >> The following operating can be used as the guest operating system >> if the Hyper-V integration services from Windows 8 or Windows >> Server 2012 are installed. >> >> [...] >> * Windows XP with Service Pack 3 (SP3) >> >> Additionally, under >> <https://technet.microsoft.com/en-us/library/dn792027(v=ws.11).aspx>: >> >> Supported Windows client guest operating systems >> >> Windows XP with [...] Install the integration [...] >> Service Pack 3 (SP3) services after you set >> up the operating system >> in the virtual machine. >> >> This seems to be consistent with the VMGENID spec requirement that the >> ADDR method return a package of two 32-bit integers, not a single 64-bit >> one. >> >> So, I agree with using a dword for VGIA (and a matching 4-byte wide >> ADD_POINTER command). >> >> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. >> In the future we might introduce more allocation hints (for the "zone" >> field) that would enable the firmware to allocate from the full 64-bit >> address space. >> >> NB, I didn't check SeaBIOS (should be easy for Ben); AFAIR it only uses >> 16-bit and 32-bit modes. >> > Right, it appears that the allocated address will always fit in 32 bits. > As mentioned, XP should be OK since the ADDR method returns a package > of two 32-bit numbers. > > I propose to still include this patch but touch up the comments as > requested by Laszlo. This way it will be in the toolbox for future > users and has been tested. I will also change VGIA to dword and hard > code the AML to return ADDR[1] = 0. Sounds good! > > FYI: here’s an iasl dump from Windows 2012 Hyper-V in case you haven’t > seen it. Note that Microsoft uses E00 and violates the HID name length > spec: :) Thanks! Laszlo > Scope (\_SB) > { > Device (GENC) > { > Name (_CID, "VM_Gen_Counter") // _CID: Compatible ID > Name (_HID, "Hyper_V_Gen_Counter_V1") // _HID: Hardware ID > Name (_UID, 0x00) // _UID: Unique ID > Name (_DDN, "VM_Gen_Counter") // _DDN: DOS Device Name > Method (ADDR, 0, NotSerialized) > { > Name (LPKG, Package (0x02) > { > 0x00, > 0x00 > }) > LPKG [0x00] = GCAL /* \GCAL */ > LPKG [0x01] = GCAH /* \GCAH */ > Return (LPKG) /* \_SB_.GENC.ADDR.LPKG */ > } > } > } > > Scope (\_GPE) > { > Method (_E00, 0, NotSerialized) // _Exx: Edge-Triggered GPE > { > Notify (\_SB.GENC, 0x80) // Status Change > } > } > >> Thanks! >> Laszlo >> >>> >>> >>> >>> >>>> >>>> (2) Since we're adding a qword (8-byte integer), the hexadecimal >>>> constant should have 16 nibbles: 0x0000000000000000. (After >>>> copying the >>>> comment from build_append_named_dword(), it should be actualized.) >>>> >>>> (3) Normally the functions in this file that create AML operators >>>> carry >>>> a note about the ACPI spec version and exact location that >>>> defines the >>>> operator. I see that commit f20354910893 ("acpi: add >>>> build_append_named_dword, returning an offset in buffer", 2016-03-01) >>>> missed that too. >>>> >>>> Unless this tradition has been willfully abandoned, I suggest >>>> that you >>>> add the right reference here, and also (in retrospect) to >>>> build_append_named_dword(). >>>> >>>> >>>> +int >>>> +build_append_named_qword(GArray *array, const char >>>> *name_format, ...) >>>> +{ >>>> + int offset; >>>> + va_list ap; >>>> + >>>> + build_append_byte(array, 0x08); /* NameOp */ >>>> + va_start(ap, name_format); >>>> + build_append_namestringv(array, name_format, ap); >>>> + va_end(ap); >>>> + >>>> + build_append_byte(array, 0x0E); /* QWordPrefix */ >>>> + >>>> + offset = array->len; >>>> + build_append_int_noprefix(array, 0x00000000, 8); >>>> >>>> >>>> (4) I guess the constant should be updated here too, for consistency >>>> with the comment. >>>> >>>> The rest looks okay. (I didn't verify 0x0E == QWordPrefix >>>> specifically, >>>> because an error there would break the functionality immediately, and >>>> you'd notice.) >>>> >>>> Thanks! >>>> Laszlo >>>> >>>> >>>> + assert(array->len == offset + 8); >>>> + >>>> + return offset; >>>> +} >>>> + >>>> static GPtrArray *alloc_list; >>>> >>>> static Aml *aml_alloc(void) >>>> diff --git a/include/hw/acpi/aml-build.h >>>> b/include/hw/acpi/aml-build.h >>>> index 559326c..dbf63cf 100644 >>>> --- a/include/hw/acpi/aml-build.h >>>> +++ b/include/hw/acpi/aml-build.h >>>> @@ -385,6 +385,10 @@ int >>>> build_append_named_dword(GArray *array, const char >>>> *name_format, ...) >>>> GCC_FMT_ATTR(2, 3); >>>> >>>> +int >>>> +build_append_named_qword(GArray *array, const char >>>> *name_format, ...) >>>> +GCC_FMT_ATTR(2, 3); >>>> + >>>> void build_srat_memory(AcpiSratMemoryAffinity *numamem, >>>> uint64_t base, >>>> uint64_t len, int node, >>>> MemoryAffinityFlags >>>> flags); >>>> >>>> >>>> —Ben >
On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote: > On 01/25/17 19:35, Michael S. Tsirkin wrote: > > On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote: > >> Hi Laszlo, > >> > >> > >> On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com> wrote: > >> > >> Hi Ben, > >> > >> sorry about being late to reviewing this series. I hope I can now spend > >> more time on it. > >> > >> - Please do not try to address my comments immediately. It's very > >> possible (even likely) that Igor, MST and myself could have different > >> opinions on things, so first please await agreement between your reviewers. > >> > >> > >> Thanks for the very detailed review. I’ll give it a couple of days and then > >> start work on the suggested changes. > >> > >> > >> - I think you should have CC'd Igor and Michael directly. I'm adding > >> them to this reply; hopefully that will be enough for them to monitor > >> this series. > >> > >> - I'll likely be unable to review everything with 100% coverage; so > >> addressing (or sufficiently refuting) my comments might not guarantee > >> that the next version will be merged at once. > >> > >> With all that said: > >> > >> On 01/25/17 02:43, ben@skyportsystems.com wrote: > >> > >> From: Ben Warren <ben@skyportsystems.com> > >> > >> This is initially used to patch a 64-bit address into the VM Generation > >> ID SSDT > >> > >> > >> (1) I think this commit message line is overlong; I think we wrap at 74 > >> chars or so. Not critical, but worth mentioning. > >> > >> > >> > >> Signed-off-by: Ben Warren <ben@skyportsystems.com> > >> --- > >> hw/acpi/aml-build.c | 28 ++++++++++++++++++++++++++++ > >> include/hw/acpi/aml-build.h | 4 ++++ > >> 2 files changed, 32 insertions(+) > >> > >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > >> index b2a1e40..dc4edc2 100644 > >> --- a/hw/acpi/aml-build.c > >> +++ b/hw/acpi/aml-build.c > >> @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char > >> *name_format, ...) > >> return offset; > >> } > >> > >> +/* > >> + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a > >> qword, > >> + * and return the offset to 0x00000000 for runtime patching. > >> + * > >> + * Warning: runtime patching is best avoided. Only use this as > >> + * a replacement for DataTableRegion (for guests that don't > >> + * support it). > >> + */ > > > > only one comment: QWords first appeared in ACPI 2.0 and > > XP does not like them. Not strictly a blocker as people can > > avoid using the feature, but nice to have. > > Does XP have a driver for VMGENID? > > If not, then I'd prefer to stick with the qword VGIA. Not sure but in any case even if it won't be able to use it we also don't want it to crash. > > Will either UEFI or seabios allocate > > memory outside 4G range? If not you do not need a qword. > > Good point (assuming XP has a driver for VMGENID). > > OVMF keeps all such allocations (i.e., for COMMAND_ALLOCATE and the > upcoming COMMAND_ALLOCATE_RETURN_ADDR) under 4GB, so as far as OVMF is > concerned, using a dword for the VGIA named object should be fine. > Accordingly, a 4-byte wide ADD_POINTER command should be used for > patching VGIA. > > Considering the fw_cfg file that receives the address, and > COMMAND_ALLOCATE_RETURN_ADDR more generally, I'd still prefer if those > stayed 8-byte wide, regardless of XP's support for VMGENID. > > > Hm... It looks like VMGENID *can* be consumed on Windows XP SP3, as long > as "Hyper-V integration services" are installed: > > https://msdn.microsoft.com/en-us/library/jj643357(v=vs.85).aspx > > The virtual machine must be running a guest operating system that > has support for the virtual machine generation identifier. > Currently, these are the following. > The following operating systems have native support for the virtual > machine generation identifier. > [...] > > The following operating can be used as the guest operating system > if the Hyper-V integration services from Windows 8 or Windows > Server 2012 are installed. > > [...] > * Windows XP with Service Pack 3 (SP3) > > Additionally, under > <https://technet.microsoft.com/en-us/library/dn792027(v=ws.11).aspx>: > > Supported Windows client guest operating systems > > Windows XP with [...] Install the integration [...] > Service Pack 3 (SP3) services after you set > up the operating system > in the virtual machine. > > This seems to be consistent with the VMGENID spec requirement that the > ADDR method return a package of two 32-bit integers, not a single 64-bit > one. > > So, I agree with using a dword for VGIA (and a matching 4-byte wide > ADD_POINTER command). > > But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with COMMAND_ALLOCATE. If we want to allow this stuff in high 64 bit, as you correctly say we will need a new zone to allocate 64 bit memory. As for XP support - might it be reasonable to require that these machines have less than 4G RAM at boot? > In the future we might introduce more allocation hints (for the "zone" > field) that would enable the firmware to allocate from the full 64-bit > address space. The difficulty with new commands always was compatibility with old firmware. I guess now that we have writeable fw cfg we will be able to support negotiation cleanly. Should we start now? > NB, I didn't check SeaBIOS (should be easy for Ben); AFAIR it only uses > 16-bit and 32-bit modes. > > Thanks! > Laszlo Right. > > > > > > > > > >> > >> (2) Since we're adding a qword (8-byte integer), the hexadecimal > >> constant should have 16 nibbles: 0x0000000000000000. (After copying the > >> comment from build_append_named_dword(), it should be actualized.) > >> > >> (3) Normally the functions in this file that create AML operators carry > >> a note about the ACPI spec version and exact location that defines the > >> operator. I see that commit f20354910893 ("acpi: add > >> build_append_named_dword, returning an offset in buffer", 2016-03-01) > >> missed that too. > >> > >> Unless this tradition has been willfully abandoned, I suggest that you > >> add the right reference here, and also (in retrospect) to > >> build_append_named_dword(). > >> > >> > >> +int > >> +build_append_named_qword(GArray *array, const char *name_format, ...) > >> +{ > >> + int offset; > >> + va_list ap; > >> + > >> + build_append_byte(array, 0x08); /* NameOp */ > >> + va_start(ap, name_format); > >> + build_append_namestringv(array, name_format, ap); > >> + va_end(ap); > >> + > >> + build_append_byte(array, 0x0E); /* QWordPrefix */ > >> + > >> + offset = array->len; > >> + build_append_int_noprefix(array, 0x00000000, 8); > >> > >> > >> (4) I guess the constant should be updated here too, for consistency > >> with the comment. > >> > >> The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically, > >> because an error there would break the functionality immediately, and > >> you'd notice.) > >> > >> Thanks! > >> Laszlo > >> > >> > >> + assert(array->len == offset + 8); > >> + > >> + return offset; > >> +} > >> + > >> static GPtrArray *alloc_list; > >> > >> static Aml *aml_alloc(void) > >> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > >> index 559326c..dbf63cf 100644 > >> --- a/include/hw/acpi/aml-build.h > >> +++ b/include/hw/acpi/aml-build.h > >> @@ -385,6 +385,10 @@ int > >> build_append_named_dword(GArray *array, const char *name_format, ...) > >> GCC_FMT_ATTR(2, 3); > >> > >> +int > >> +build_append_named_qword(GArray *array, const char *name_format, ...) > >> +GCC_FMT_ATTR(2, 3); > >> + > >> void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base, > >> uint64_t len, int node, MemoryAffinityFlags > >> flags); > >> > >> > >> —Ben > >> > > > >
On 01/26/17 16:20, Michael S. Tsirkin wrote: > On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote: >> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. > > > What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with > COMMAND_ALLOCATE. It's a new command being introduced in this series, at my suggestion. It does the exact same thing as COMMAND_ALLOCATE, except once the allocation / download is carried out by the firmware, the firmware writes back the allocation address to the fw_cfg file that is named in an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This is how QEMU learns where the blob in GPA space was placed by the firmware.) The format for this address-receiving fw_cfg file is supposed to be 8-byte, little endian. My request above is simply that we stick with the 8-byte size for this fw_cfg file, for receiving a guest allocation address. Regardless of the fact that currently all such allocation addresses fit in 4 bytes. > If we want to allow this stuff in high 64 bit, as you > correctly say we will need a new zone to allocate 64 bit memory. > As for XP support - might it be reasonable to require that > these machines have less than 4G RAM at boot? Perhaps; I'm not sure. At the moment I have zero concrete use cases in mind. I just want COMMAND_ALLOCATE_RETURN_ADDR to promise the firmware that the firmware will be able to return 8 bytes / LE as the allocation address. How this will interact with any new zones and RAM sizes vs. guest OSes is TBD in the future. >> In the future we might introduce more allocation hints (for the "zone" >> field) that would enable the firmware to allocate from the full 64-bit >> address space. > > The difficulty with new commands always was compatibility with old > firmware. I guess now that we have writeable fw cfg we will be > able to support negotiation cleanly. Specifically for the zone field of COMMAND_ALLOCATE (and identically, COMMAND_ALLOCATE_RETURN_ADDR), I think we might not need full-blown negotiation; there aren't that many firmwares to check compatibility with -- OVMF and SeaBIOS. If old versions of those happen to handle a new zone value gracefully (such as "not fseg", simply), i.e. they'd behave the same as now, then we shouldn't need negotiation. Otherwise, we'll need it (once we have a particular use case). > Should we start now? No, I don't think so. I don't have any use case for 64-bit allocation; what we have now works perfectly. I just wanted to emphasize that permitting an 8-byte width for the alloc address to be returned is more "future proof" than a 4-byte size, for COMMAND_ALLOCATE_RETURN_ADDR; independently of what size we choose right here for VGIA. Thanks, Laszlo
On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote: > On 01/26/17 16:20, Michael S. Tsirkin wrote: > > On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote: > > >> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. > > > > > > What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with > > COMMAND_ALLOCATE. > > It's a new command being introduced in this series, at my suggestion. It > does the exact same thing as COMMAND_ALLOCATE, except once the > allocation / download is carried out by the firmware, the firmware > writes back the allocation address to the fw_cfg file that is named in > an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This > is how QEMU learns where the blob in GPA space was placed by the > firmware.) The format for this address-receiving fw_cfg file is supposed > to be 8-byte, little endian. I see. I really think it's better as a separate command though. E.g. COMMAND_WRITE_PTR? > My request above is simply that we stick with the 8-byte size for this > fw_cfg file, for receiving a guest allocation address. Regardless of the > fact that currently all such allocation addresses fit in 4 bytes. > > > If we want to allow this stuff in high 64 bit, as you > > correctly say we will need a new zone to allocate 64 bit memory. > > As for XP support - might it be reasonable to require that > > these machines have less than 4G RAM at boot? > > Perhaps; I'm not sure. At the moment I have zero concrete use cases in > mind. I just want COMMAND_ALLOCATE_RETURN_ADDR to promise the firmware > that the firmware will be able to return 8 bytes / LE as the allocation > address. How this will interact with any new zones and RAM sizes vs. > guest OSes is TBD in the future. > > >> In the future we might introduce more allocation hints (for the "zone" > >> field) that would enable the firmware to allocate from the full 64-bit > >> address space. > > > > The difficulty with new commands always was compatibility with old > > firmware. I guess now that we have writeable fw cfg we will be > > able to support negotiation cleanly. > > Specifically for the zone field of COMMAND_ALLOCATE (and identically, > COMMAND_ALLOCATE_RETURN_ADDR), I think we might not need full-blown > negotiation; there aren't that many firmwares to check compatibility > with -- OVMF and SeaBIOS. If old versions of those happen to handle a > new zone value gracefully (such as "not fseg", simply), i.e. they'd > behave the same as now, then we shouldn't need negotiation. Otherwise, > we'll need it (once we have a particular use case). > > > Should we start now? > > No, I don't think so. I don't have any use case for 64-bit allocation; > what we have now works perfectly. I just wanted to emphasize that > permitting an 8-byte width for the alloc address to be returned is more > "future proof" than a 4-byte size, for COMMAND_ALLOCATE_RETURN_ADDR; > independently of what size we choose right here for VGIA. > > Thanks, > Laszlo I agree here.
On 01/26/17 19:15, Michael S. Tsirkin wrote: > On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote: >> On 01/26/17 16:20, Michael S. Tsirkin wrote: >>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote: >> >>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. >>> >>> >>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with >>> COMMAND_ALLOCATE. >> >> It's a new command being introduced in this series, at my suggestion. It >> does the exact same thing as COMMAND_ALLOCATE, except once the >> allocation / download is carried out by the firmware, the firmware >> writes back the allocation address to the fw_cfg file that is named in >> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This >> is how QEMU learns where the blob in GPA space was placed by the >> firmware.) The format for this address-receiving fw_cfg file is supposed >> to be 8-byte, little endian. > > I see. I really think it's better as a separate command though. > E.g. COMMAND_WRITE_PTR? Sure, but please provide specifics, otherwise Ben & myself will have a hard time divining & implementing your intent :) Thanks, Laszlo
On Thu, Jan 26, 2017 at 07:25:22PM +0100, Laszlo Ersek wrote: > On 01/26/17 19:15, Michael S. Tsirkin wrote: > > On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote: > >> On 01/26/17 16:20, Michael S. Tsirkin wrote: > >>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote: > >> > >>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. > >>> > >>> > >>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with > >>> COMMAND_ALLOCATE. > >> > >> It's a new command being introduced in this series, at my suggestion. It > >> does the exact same thing as COMMAND_ALLOCATE, except once the > >> allocation / download is carried out by the firmware, the firmware > >> writes back the allocation address to the fw_cfg file that is named in > >> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This > >> is how QEMU learns where the blob in GPA space was placed by the > >> firmware.) The format for this address-receiving fw_cfg file is supposed > >> to be 8-byte, little endian. > > > > I see. I really think it's better as a separate command though. > > E.g. COMMAND_WRITE_PTR? > > Sure, but please provide specifics, otherwise Ben & myself will have a > hard time divining & implementing your intent :) > > Thanks, > Laszlo I would say a variant of ADD_POINTER: /* * COMMAND_WRITE_POINTER - update a writeable file named * @pointer.dest_file at @pointer.offset, by writing pointer to * the table originating from @src_file. 1,2,4 or 8 byte * unsigned write is used depending on @pointer.size. */ struct { char dest_file[BIOS_LINKER_LOADER_FILESZ]; char src_file[BIOS_LINKER_LOADER_FILESZ]; uint32_t offset; uint8_t size; } pointer;
On 01/26/17 19:59, Michael S. Tsirkin wrote: > On Thu, Jan 26, 2017 at 07:25:22PM +0100, Laszlo Ersek wrote: >> On 01/26/17 19:15, Michael S. Tsirkin wrote: >>> On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote: >>>> On 01/26/17 16:20, Michael S. Tsirkin wrote: >>>>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote: >>>> >>>>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. >>>>> >>>>> >>>>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with >>>>> COMMAND_ALLOCATE. >>>> >>>> It's a new command being introduced in this series, at my suggestion. It >>>> does the exact same thing as COMMAND_ALLOCATE, except once the >>>> allocation / download is carried out by the firmware, the firmware >>>> writes back the allocation address to the fw_cfg file that is named in >>>> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This >>>> is how QEMU learns where the blob in GPA space was placed by the >>>> firmware.) The format for this address-receiving fw_cfg file is supposed >>>> to be 8-byte, little endian. >>> >>> I see. I really think it's better as a separate command though. >>> E.g. COMMAND_WRITE_PTR? >> >> Sure, but please provide specifics, otherwise Ben & myself will have a >> hard time divining & implementing your intent :) >> >> Thanks, >> Laszlo > > I would say a variant of ADD_POINTER: > > /* > * COMMAND_WRITE_POINTER - update a writeable file named > * @pointer.dest_file at @pointer.offset, by writing pointer to > * the table originating from @src_file. 1,2,4 or 8 byte > * unsigned write is used depending on @pointer.size. > */ > struct { > char dest_file[BIOS_LINKER_LOADER_FILESZ]; > char src_file[BIOS_LINKER_LOADER_FILESZ]; > uint32_t offset; > uint8_t size; > } pointer; This will require more work in OVMF, but it does seem feasible, and I agree this command is significantly more flexible than what I proposed. In particular, it allows us to receive multiple guest-side allocation addresses into the same writeable fw_cfg file, just at different offsets of the fw_cfg file. For the comment block above, I have one suggestion: replace pointer to the table originating from @src_file with pointer to the blob originating from @src_file "table" is quite overloaded in this context. As we normally pack several ACPI tables in a single fw_cfg blob, I like to be pedantic about "blob" vs. "table" (restricting the latter to "ACPI table"). E.g., structured buffer coming from "etc/vmgenid" is not an ACPI table, but "blob" definitely covers it. If Ben is okay with it, I think this command is a good fit. I'll make an effort to write the OVMF patches in time to test them with one of Ben's next patch set versions (so we can determine if the QEMU stuff works with both SeaBIOS and OVMF before committing the QEMU patches). Thanks! Laszlo
On Thu, Jan 26, 2017 at 08:59:04PM +0200, Michael S. Tsirkin wrote: > On Thu, Jan 26, 2017 at 07:25:22PM +0100, Laszlo Ersek wrote: > > On 01/26/17 19:15, Michael S. Tsirkin wrote: > > > On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote: > > >> On 01/26/17 16:20, Michael S. Tsirkin wrote: > > >>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote: > > >> > > >>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. > > >>> > > >>> > > >>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with > > >>> COMMAND_ALLOCATE. > > >> > > >> It's a new command being introduced in this series, at my suggestion. It > > >> does the exact same thing as COMMAND_ALLOCATE, except once the > > >> allocation / download is carried out by the firmware, the firmware > > >> writes back the allocation address to the fw_cfg file that is named in > > >> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This > > >> is how QEMU learns where the blob in GPA space was placed by the > > >> firmware.) The format for this address-receiving fw_cfg file is supposed > > >> to be 8-byte, little endian. > > > > > > I see. I really think it's better as a separate command though. > > > E.g. COMMAND_WRITE_PTR? > > > > Sure, but please provide specifics, otherwise Ben & myself will have a > > hard time divining & implementing your intent :) > > > > Thanks, > > Laszlo > > I would say a variant of ADD_POINTER: > > /* > * COMMAND_WRITE_POINTER - update a writeable file named > * @pointer.dest_file at @pointer.offset, by writing pointer to > * the table originating from @src_file. 1,2,4 or 8 byte > * unsigned write is used depending on @pointer.size. > */ > struct { > char dest_file[BIOS_LINKER_LOADER_FILESZ]; > char src_file[BIOS_LINKER_LOADER_FILESZ]; > uint32_t offset; > uint8_t size; > } pointer; > I'm okay with this approach. If an offset is going to be added, shouldn't both a source offset and destination offset be used? /* * COMMAND_WRITE_POINTER - update a writeable file named * @pointer.dest_file at @pointer.dest_offset, by writing pointer * plus @pointer.src_offset to the blob originating from * @src_file. 1,2,4 or 8 byte unsigned write is used depending * on @pointer.size. */ struct { char dest_file[BIOS_LINKER_LOADER_FILESZ]; char src_file[BIOS_LINKER_LOADER_FILESZ]; uint32_t src_offset, dest_offset; uint8_t size; } pointer; I doubt the offsets or size is really all that important though. -Kevin
On 01/27/17 15:18, Kevin O'Connor wrote: > On Thu, Jan 26, 2017 at 08:59:04PM +0200, Michael S. Tsirkin wrote: >> On Thu, Jan 26, 2017 at 07:25:22PM +0100, Laszlo Ersek wrote: >>> On 01/26/17 19:15, Michael S. Tsirkin wrote: >>>> On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote: >>>>> On 01/26/17 16:20, Michael S. Tsirkin wrote: >>>>>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote: >>>>> >>>>>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide. >>>>>> >>>>>> >>>>>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with >>>>>> COMMAND_ALLOCATE. >>>>> >>>>> It's a new command being introduced in this series, at my suggestion. It >>>>> does the exact same thing as COMMAND_ALLOCATE, except once the >>>>> allocation / download is carried out by the firmware, the firmware >>>>> writes back the allocation address to the fw_cfg file that is named in >>>>> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This >>>>> is how QEMU learns where the blob in GPA space was placed by the >>>>> firmware.) The format for this address-receiving fw_cfg file is supposed >>>>> to be 8-byte, little endian. >>>> >>>> I see. I really think it's better as a separate command though. >>>> E.g. COMMAND_WRITE_PTR? >>> >>> Sure, but please provide specifics, otherwise Ben & myself will have a >>> hard time divining & implementing your intent :) >>> >>> Thanks, >>> Laszlo >> >> I would say a variant of ADD_POINTER: >> >> /* >> * COMMAND_WRITE_POINTER - update a writeable file named >> * @pointer.dest_file at @pointer.offset, by writing pointer to >> * the table originating from @src_file. 1,2,4 or 8 byte >> * unsigned write is used depending on @pointer.size. >> */ >> struct { >> char dest_file[BIOS_LINKER_LOADER_FILESZ]; >> char src_file[BIOS_LINKER_LOADER_FILESZ]; >> uint32_t offset; >> uint8_t size; >> } pointer; >> > > I'm okay with this approach. > > If an offset is going to be added, shouldn't both a source offset and > destination offset be used? > > /* > * COMMAND_WRITE_POINTER - update a writeable file named > * @pointer.dest_file at @pointer.dest_offset, by writing pointer > * plus @pointer.src_offset to the blob originating from > * @src_file. 1,2,4 or 8 byte unsigned write is used depending > * on @pointer.size. > */ > struct { > char dest_file[BIOS_LINKER_LOADER_FILESZ]; > char src_file[BIOS_LINKER_LOADER_FILESZ]; > uint32_t src_offset, dest_offset; > uint8_t size; > } pointer; > > I doubt the offsets or size is really all that important though. The offset into the fw_cfg file that receives the allocation address is important, that allows the same file to receive several different addresses (for different downloaded blobs), at different offsets. OTOH, asking the firmware to add a constant to the address value before writing it to the fw_cfg file is not necessary, in my opinion. The blob that the firmware allocated and downloaded originates from QEMU to begin with, so QEMU knows its internal structure. Here's a diagram for ADD_POINTER: blob-to-patch +---------------+ | | | pointer = N |-----------+ | | | pointed-to-blob +---------------+ | +----------------+ | | | +-----> | offset N: ... | | | +----------------+ In this case, QEMU pre-populates the pointer field in "blob-to-patch" with the value N (a small relative offset, from the beginning of "pointed-to-blob"). The firmware then increases the pointer field in "blob-to-patch" with the absolute allocation address of "pointed-to-blob". Thus "pointer" will point to the absolute address of offset N into "pointed-to-blob". This is useful primarily when the "pointer" field in "blob-to-patch" is part of an ACPI table in "blob-to-patch". ACPI tables are consumed by the guest OS. The information necessary for the above is: - name of "blob-to-patch" - name of "pointed-to-blob" - relative address of pointer field to patch within "blob-to-patch" - size of the same Compare WRITE_POINTER: fw-cfg-file-to-rewrite +---------------+ | | | pointer = XXX |-----------+ | | | pointed-to-blob +---------------+ +-----> +----------------+ | | | offset N: ... | | offset K: ... | | | +----------------+ In this case, the "pointer field" that the firmware should update lives inside QEMU only. The only consumer for it is QEMU itself, from which "pointed-to-blob" originates too. The original value XXX is irrelevant. The firmware writes the base address of "pointed-to-blob" over XXX, and then QEMU can add N whenever it wants to access the field at offset N in "pointed-to-blob". It can add K too, whenever it wants to access another field in "pointed-to-blob". The information necessary for this command is: - name of "fw-cfg-file-to-rewrite" - name of "pointed-to-blob" - offset of "pointer field" to overwrite within "fw-cfg-file-to-rewrite" - size of the same Thanks Laszlo
On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote: > On 01/27/17 15:18, Kevin O'Connor wrote: > > If an offset is going to be added, shouldn't both a source offset and > > destination offset be used? > > > > /* > > * COMMAND_WRITE_POINTER - update a writeable file named > > * @pointer.dest_file at @pointer.dest_offset, by writing pointer > > * plus @pointer.src_offset to the blob originating from > > * @src_file. 1,2,4 or 8 byte unsigned write is used depending > > * on @pointer.size. > > */ > > struct { > > char dest_file[BIOS_LINKER_LOADER_FILESZ]; > > char src_file[BIOS_LINKER_LOADER_FILESZ]; > > uint32_t src_offset, dest_offset; > > uint8_t size; > > } pointer; > > > > I doubt the offsets or size is really all that important though. > > The offset into the fw_cfg file that receives the allocation address is > important, that allows the same file to receive several different > addresses (for different downloaded blobs), at different offsets. > > OTOH, asking the firmware to add a constant to the address value before > writing it to the fw_cfg file is not necessary, in my opinion. The blob > that the firmware allocated and downloaded originates from QEMU to begin > with, so QEMU knows its internal structure. I guess I'm missing why QEMU would want to use the same writable file for multiple pointers as well as why it would want support for pointers smaller than 8 bytes in size. If it's because it may be easier to support an internal QEMU blob of a particular format, then adding a src_offset would facilitate that. However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then that's fine too. I'm okay with either format. -Kevin
On 01/27/17 16:43, Kevin O'Connor wrote: > On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote: >> On 01/27/17 15:18, Kevin O'Connor wrote: >>> If an offset is going to be added, shouldn't both a source offset and >>> destination offset be used? >>> >>> /* >>> * COMMAND_WRITE_POINTER - update a writeable file named >>> * @pointer.dest_file at @pointer.dest_offset, by writing pointer >>> * plus @pointer.src_offset to the blob originating from >>> * @src_file. 1,2,4 or 8 byte unsigned write is used depending >>> * on @pointer.size. >>> */ >>> struct { >>> char dest_file[BIOS_LINKER_LOADER_FILESZ]; >>> char src_file[BIOS_LINKER_LOADER_FILESZ]; >>> uint32_t src_offset, dest_offset; >>> uint8_t size; >>> } pointer; >>> >>> I doubt the offsets or size is really all that important though. >> >> The offset into the fw_cfg file that receives the allocation address is >> important, that allows the same file to receive several different >> addresses (for different downloaded blobs), at different offsets. >> >> OTOH, asking the firmware to add a constant to the address value before >> writing it to the fw_cfg file is not necessary, in my opinion. The blob >> that the firmware allocated and downloaded originates from QEMU to begin >> with, so QEMU knows its internal structure. > > I guess I'm missing why QEMU would want to use the same writable file > for multiple pointers I know no specific reason; I just thought this possible generalization was one of the advantages in Michael's suggestion. > as well as why it would want support for > pointers smaller than 8 bytes in size. Hm, right, good point. > If it's because it may be > easier to support an internal QEMU blob of a particular format, then > adding a src_offset would facilitate that. > > However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then > that's fine too. That might be the main reason I guess; reading back a bit, Michael wrote "... a variant of ADD_POINTER". > I'm okay with either format. I'd say let's go ahead with Michael's proposal then. Ben, are you okay with that? Thanks! Laszlo
> On Jan 27, 2017, at 8:12 AM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 01/27/17 16:43, Kevin O'Connor wrote: >> On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote: >>> On 01/27/17 15:18, Kevin O'Connor wrote: >>>> If an offset is going to be added, shouldn't both a source offset and >>>> destination offset be used? >>>> >>>> /* >>>> * COMMAND_WRITE_POINTER - update a writeable file named >>>> * @pointer.dest_file at @pointer.dest_offset, by writing pointer >>>> * plus @pointer.src_offset to the blob originating from >>>> * @src_file. 1,2,4 or 8 byte unsigned write is used depending >>>> * on @pointer.size. >>>> */ >>>> struct { >>>> char dest_file[BIOS_LINKER_LOADER_FILESZ]; >>>> char src_file[BIOS_LINKER_LOADER_FILESZ]; >>>> uint32_t src_offset, dest_offset; >>>> uint8_t size; >>>> } pointer; >>>> >>>> I doubt the offsets or size is really all that important though. >>> >>> The offset into the fw_cfg file that receives the allocation address is >>> important, that allows the same file to receive several different >>> addresses (for different downloaded blobs), at different offsets. >>> >>> OTOH, asking the firmware to add a constant to the address value before >>> writing it to the fw_cfg file is not necessary, in my opinion. The blob >>> that the firmware allocated and downloaded originates from QEMU to begin >>> with, so QEMU knows its internal structure. >> >> I guess I'm missing why QEMU would want to use the same writable file >> for multiple pointers > > I know no specific reason; I just thought this possible generalization > was one of the advantages in Michael's suggestion. > >> as well as why it would want support for >> pointers smaller than 8 bytes in size. > > Hm, right, good point. > >> If it's because it may be >> easier to support an internal QEMU blob of a particular format, then >> adding a src_offset would facilitate that. >> >> However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then >> that's fine too. > > That might be the main reason I guess; reading back a bit, Michael wrote > "... a variant of ADD_POINTER". > >> I'm okay with either format. > > I'd say let's go ahead with Michael's proposal then. Ben, are you okay > with that? > Yes, this seems reasonable. If I’m interpreting this properly, it’s like ADD_POINTER, except that we extend the write path back to QEMU over DMA. Is that about right? > Thanks! > Laszlo —Ben
On 01/27/17 19:19, Ben Warren wrote: > >> On Jan 27, 2017, at 8:12 AM, Laszlo Ersek <lersek@redhat.com >> <mailto:lersek@redhat.com>> wrote: >> >> On 01/27/17 16:43, Kevin O'Connor wrote: >>> On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote: >>>> On 01/27/17 15:18, Kevin O'Connor wrote: >>>>> If an offset is going to be added, shouldn't both a source offset and >>>>> destination offset be used? >>>>> >>>>> /* >>>>> * COMMAND_WRITE_POINTER - update a writeable file named >>>>> * @pointer.dest_file at @pointer.dest_offset, by writing >>>>> pointer >>>>> * plus @pointer.src_offset to the blob originating from >>>>> * @src_file. 1,2,4 or 8 byte unsigned write is used depending >>>>> * on @pointer.size. >>>>> */ >>>>> struct { >>>>> char dest_file[BIOS_LINKER_LOADER_FILESZ]; >>>>> char src_file[BIOS_LINKER_LOADER_FILESZ]; >>>>> uint32_t src_offset, dest_offset; >>>>> uint8_t size; >>>>> } pointer; >>>>> >>>>> I doubt the offsets or size is really all that important though. >>>> >>>> The offset into the fw_cfg file that receives the allocation address is >>>> important, that allows the same file to receive several different >>>> addresses (for different downloaded blobs), at different offsets. >>>> >>>> OTOH, asking the firmware to add a constant to the address value before >>>> writing it to the fw_cfg file is not necessary, in my opinion. The blob >>>> that the firmware allocated and downloaded originates from QEMU to begin >>>> with, so QEMU knows its internal structure. >>> >>> I guess I'm missing why QEMU would want to use the same writable file >>> for multiple pointers >> >> I know no specific reason; I just thought this possible generalization >> was one of the advantages in Michael's suggestion. >> >>> as well as why it would want support for >>> pointers smaller than 8 bytes in size. >> >> Hm, right, good point. >> >>> If it's because it may be >>> easier to support an internal QEMU blob of a particular format, then >>> adding a src_offset would facilitate that. >>> >>> However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then >>> that's fine too. >> >> That might be the main reason I guess; reading back a bit, Michael wrote >> "... a variant of ADD_POINTER". >> >>> I'm okay with either format. >> >> I'd say let's go ahead with Michael's proposal then. Ben, are you okay >> with that? >> > Yes, this seems reasonable. If I’m interpreting this properly, it’s > like ADD_POINTER, except that we extend the write path back to QEMU over > DMA. Is that about right? Yes. Basically the command differs from ADD_POINTER in that (a) the pointer field to patch lives in "fw_cfg space", not in guest RAM, so rather than updating guest RAM, the firmware has to select a writeable fw_cfg file, seek ("skip") to the specified offset, and rewrite the field, (b) the nature of the patching is not "increment", just a plain "overwrite". Thanks! Laszlo
On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote: > On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote: > > On 01/27/17 15:18, Kevin O'Connor wrote: > > > If an offset is going to be added, shouldn't both a source offset and > > > destination offset be used? > > > > > > /* > > > * COMMAND_WRITE_POINTER - update a writeable file named > > > * @pointer.dest_file at @pointer.dest_offset, by writing pointer > > > * plus @pointer.src_offset to the blob originating from > > > * @src_file. 1,2,4 or 8 byte unsigned write is used depending > > > * on @pointer.size. > > > */ > > > struct { > > > char dest_file[BIOS_LINKER_LOADER_FILESZ]; > > > char src_file[BIOS_LINKER_LOADER_FILESZ]; > > > uint32_t src_offset, dest_offset; > > > uint8_t size; > > > } pointer; > > > > > > I doubt the offsets or size is really all that important though. > > > > The offset into the fw_cfg file that receives the allocation address is > > important, that allows the same file to receive several different > > addresses (for different downloaded blobs), at different offsets. > > > > OTOH, asking the firmware to add a constant to the address value before > > writing it to the fw_cfg file is not necessary, in my opinion. The blob > > that the firmware allocated and downloaded originates from QEMU to begin > > with, so QEMU knows its internal structure. > > I guess I'm missing why QEMU would want to use the same writable file > for multiple pointers as well as why it would want support for > pointers smaller than 8 bytes in size. If it's because it may be > easier to support an internal QEMU blob of a particular format, then > adding a src_offset would facilitate that. > > However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then > that's fine too. I'm okay with either format. > > -Kevin Both reasons :) offset is because it's easier for QEMU not to have to add more files (e.g. it simplifies cross-version migration if we don't). size is to mimick ADD_POINTER.
On Mon, 30 Jan 2017 22:28:41 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote: > > On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote: > > > On 01/27/17 15:18, Kevin O'Connor wrote: > > > > If an offset is going to be added, shouldn't both a source offset and > > > > destination offset be used? > > > > > > > > /* > > > > * COMMAND_WRITE_POINTER - update a writeable file named > > > > * @pointer.dest_file at @pointer.dest_offset, by writing pointer > > > > * plus @pointer.src_offset to the blob originating from > > > > * @src_file. 1,2,4 or 8 byte unsigned write is used depending > > > > * on @pointer.size. > > > > */ > > > > struct { > > > > char dest_file[BIOS_LINKER_LOADER_FILESZ]; > > > > char src_file[BIOS_LINKER_LOADER_FILESZ]; > > > > uint32_t src_offset, dest_offset; > > > > uint8_t size; > > > > } pointer; > > > > > > > > I doubt the offsets or size is really all that important though. > > > > > > The offset into the fw_cfg file that receives the allocation address is > > > important, that allows the same file to receive several different > > > addresses (for different downloaded blobs), at different offsets. > > > > > > OTOH, asking the firmware to add a constant to the address value before > > > writing it to the fw_cfg file is not necessary, in my opinion. The blob > > > that the firmware allocated and downloaded originates from QEMU to begin > > > with, so QEMU knows its internal structure. > > > > I guess I'm missing why QEMU would want to use the same writable file > > for multiple pointers as well as why it would want support for > > pointers smaller than 8 bytes in size. If it's because it may be > > easier to support an internal QEMU blob of a particular format, then > > adding a src_offset would facilitate that. > > > > However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then > > that's fine too. I'm okay with either format. > > > > -Kevin > > Both reasons :) offset is because it's easier for QEMU not to have to add > more files (e.g. it simplifies cross-version migration if we don't). On one hand offset simplifies since one file could be re-used for several pointers, on the other hand it doesn't make difference wrt migration since offset becomes ABI and has to be maintained in cross-version migration scenario (size of file shouldn't be issue as they are re-sizable now). So we just end-up with offset vs new file versioning. However considering that number of files is limited, offset scales up better. > size is to mimick ADD_POINTER. >
On Tue, Jan 31, 2017 at 10:51:02AM +0100, Igor Mammedov wrote: > On Mon, 30 Jan 2017 22:28:41 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote: > > > On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote: > > > > On 01/27/17 15:18, Kevin O'Connor wrote: > > > > > If an offset is going to be added, shouldn't both a source offset and > > > > > destination offset be used? > > > > > > > > > > /* > > > > > * COMMAND_WRITE_POINTER - update a writeable file named > > > > > * @pointer.dest_file at @pointer.dest_offset, by writing pointer > > > > > * plus @pointer.src_offset to the blob originating from > > > > > * @src_file. 1,2,4 or 8 byte unsigned write is used depending > > > > > * on @pointer.size. > > > > > */ > > > > > struct { > > > > > char dest_file[BIOS_LINKER_LOADER_FILESZ]; > > > > > char src_file[BIOS_LINKER_LOADER_FILESZ]; > > > > > uint32_t src_offset, dest_offset; > > > > > uint8_t size; > > > > > } pointer; > > > > > > > > > > I doubt the offsets or size is really all that important though. > > > > > > > > The offset into the fw_cfg file that receives the allocation address is > > > > important, that allows the same file to receive several different > > > > addresses (for different downloaded blobs), at different offsets. > > > > > > > > OTOH, asking the firmware to add a constant to the address value before > > > > writing it to the fw_cfg file is not necessary, in my opinion. The blob > > > > that the firmware allocated and downloaded originates from QEMU to begin > > > > with, so QEMU knows its internal structure. > > > > > > I guess I'm missing why QEMU would want to use the same writable file > > > for multiple pointers as well as why it would want support for > > > pointers smaller than 8 bytes in size. If it's because it may be > > > easier to support an internal QEMU blob of a particular format, then > > > adding a src_offset would facilitate that. > > > > > > However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then > > > that's fine too. I'm okay with either format. > > > > > > -Kevin > > > > Both reasons :) offset is because it's easier for QEMU not to have to add > > more files (e.g. it simplifies cross-version migration if we don't). > On one hand offset simplifies since one file could be re-used for > several pointers, on the other hand it doesn't make difference wrt > migration since offset becomes ABI and has to be maintained in > cross-version migration scenario (size of file shouldn't be issue > as they are re-sizable now). So we just end-up with offset vs new file > versioning. Not really - offset is migrated automatically since it's in RAM. No need to version it. > However considering that number of files is limited, > offset scales up better. > > > size is to mimick ADD_POINTER. > >
On Tue, 31 Jan 2017 23:39:44 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Jan 31, 2017 at 10:51:02AM +0100, Igor Mammedov wrote: > > On Mon, 30 Jan 2017 22:28:41 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote: > > > > On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote: > > > > > On 01/27/17 15:18, Kevin O'Connor wrote: > > > > > > If an offset is going to be added, shouldn't both a source offset and > > > > > > destination offset be used? > > > > > > > > > > > > /* > > > > > > * COMMAND_WRITE_POINTER - update a writeable file named > > > > > > * @pointer.dest_file at @pointer.dest_offset, by writing pointer > > > > > > * plus @pointer.src_offset to the blob originating from > > > > > > * @src_file. 1,2,4 or 8 byte unsigned write is used depending > > > > > > * on @pointer.size. > > > > > > */ > > > > > > struct { > > > > > > char dest_file[BIOS_LINKER_LOADER_FILESZ]; > > > > > > char src_file[BIOS_LINKER_LOADER_FILESZ]; > > > > > > uint32_t src_offset, dest_offset; > > > > > > uint8_t size; > > > > > > } pointer; > > > > > > > > > > > > I doubt the offsets or size is really all that important though. > > > > > > > > > > The offset into the fw_cfg file that receives the allocation address is > > > > > important, that allows the same file to receive several different > > > > > addresses (for different downloaded blobs), at different offsets. > > > > > > > > > > OTOH, asking the firmware to add a constant to the address value before > > > > > writing it to the fw_cfg file is not necessary, in my opinion. The blob > > > > > that the firmware allocated and downloaded originates from QEMU to begin > > > > > with, so QEMU knows its internal structure. > > > > > > > > I guess I'm missing why QEMU would want to use the same writable file > > > > for multiple pointers as well as why it would want support for > > > > pointers smaller than 8 bytes in size. If it's because it may be > > > > easier to support an internal QEMU blob of a particular format, then > > > > adding a src_offset would facilitate that. > > > > > > > > However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then > > > > that's fine too. I'm okay with either format. > > > > > > > > -Kevin > > > > > > Both reasons :) offset is because it's easier for QEMU not to have to add > > > more files (e.g. it simplifies cross-version migration if we don't). > > On one hand offset simplifies since one file could be re-used for > > several pointers, on the other hand it doesn't make difference wrt > > migration since offset becomes ABI and has to be maintained in > > cross-version migration scenario (size of file shouldn't be issue > > as they are re-sizable now). So we just end-up with offset vs new file > > versioning. > > Not really - offset is migrated automatically since it's in RAM. > No need to version it. I've meant offset inside of writebale blob dest_offset, it has to stay the same within a machine type across builds, so that if migration happens during linking time, the old already shadowed and migrated liker file would match offsets in writable fwcfg file on new qemu. In other words format of writable fw_cfg file becomes ABI. > > > However considering that number of files is limited, > > offset scales up better. > > > > > size is to mimick ADD_POINTER. > > >
On Wed, Feb 01, 2017 at 12:46:47PM +0100, Igor Mammedov wrote: > On Tue, 31 Jan 2017 23:39:44 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Jan 31, 2017 at 10:51:02AM +0100, Igor Mammedov wrote: > > > On Mon, 30 Jan 2017 22:28:41 +0200 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote: > > > > > On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote: > > > > > > On 01/27/17 15:18, Kevin O'Connor wrote: > > > > > > > If an offset is going to be added, shouldn't both a source offset and > > > > > > > destination offset be used? > > > > > > > > > > > > > > /* > > > > > > > * COMMAND_WRITE_POINTER - update a writeable file named > > > > > > > * @pointer.dest_file at @pointer.dest_offset, by writing pointer > > > > > > > * plus @pointer.src_offset to the blob originating from > > > > > > > * @src_file. 1,2,4 or 8 byte unsigned write is used depending > > > > > > > * on @pointer.size. > > > > > > > */ > > > > > > > struct { > > > > > > > char dest_file[BIOS_LINKER_LOADER_FILESZ]; > > > > > > > char src_file[BIOS_LINKER_LOADER_FILESZ]; > > > > > > > uint32_t src_offset, dest_offset; > > > > > > > uint8_t size; > > > > > > > } pointer; > > > > > > > > > > > > > > I doubt the offsets or size is really all that important though. > > > > > > > > > > > > The offset into the fw_cfg file that receives the allocation address is > > > > > > important, that allows the same file to receive several different > > > > > > addresses (for different downloaded blobs), at different offsets. > > > > > > > > > > > > OTOH, asking the firmware to add a constant to the address value before > > > > > > writing it to the fw_cfg file is not necessary, in my opinion. The blob > > > > > > that the firmware allocated and downloaded originates from QEMU to begin > > > > > > with, so QEMU knows its internal structure. > > > > > > > > > > I guess I'm missing why QEMU would want to use the same writable file > > > > > for multiple pointers as well as why it would want support for > > > > > pointers smaller than 8 bytes in size. If it's because it may be > > > > > easier to support an internal QEMU blob of a particular format, then > > > > > adding a src_offset would facilitate that. > > > > > > > > > > However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then > > > > > that's fine too. I'm okay with either format. > > > > > > > > > > -Kevin > > > > > > > > Both reasons :) offset is because it's easier for QEMU not to have to add > > > > more files (e.g. it simplifies cross-version migration if we don't). > > > On one hand offset simplifies since one file could be re-used for > > > several pointers, on the other hand it doesn't make difference wrt > > > migration since offset becomes ABI and has to be maintained in > > > cross-version migration scenario (size of file shouldn't be issue > > > as they are re-sizable now). So we just end-up with offset vs new file > > > versioning. > > > > Not really - offset is migrated automatically since it's in RAM. > > No need to version it. > I've meant offset inside of writebale blob dest_offset, > it has to stay the same within a machine type across builds, > so that if migration happens during linking time, the old > already shadowed and migrated liker file would match offsets in > writable fwcfg file on new qemu. > In other words format of writable fw_cfg file becomes ABI. We can always add new offsets without versioning though. So yes an ABI but easier to extend. > > > > > However considering that number of files is limited, > > > offset scales up better. > > > > > > > size is to mimick ADD_POINTER. > > > >
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index b2a1e40..dc4edc2 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char *name_format, ...) return offset; } +/* + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a qword, + * and return the offset to 0x00000000 for runtime patching. + * + * Warning: runtime patching is best avoided. Only use this as + * a replacement for DataTableRegion (for guests that don't + * support it). + */ +int +build_append_named_qword(GArray *array, const char *name_format, ...) +{ + int offset; + va_list ap; + + build_append_byte(array, 0x08); /* NameOp */ + va_start(ap, name_format); + build_append_namestringv(array, name_format, ap); + va_end(ap); + + build_append_byte(array, 0x0E); /* QWordPrefix */ + + offset = array->len; + build_append_int_noprefix(array, 0x00000000, 8); + assert(array->len == offset + 8); + + return offset; +} + static GPtrArray *alloc_list; static Aml *aml_alloc(void) diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h index 559326c..dbf63cf 100644 --- a/include/hw/acpi/aml-build.h +++ b/include/hw/acpi/aml-build.h @@ -385,6 +385,10 @@ int build_append_named_dword(GArray *array, const char *name_format, ...) GCC_FMT_ATTR(2, 3); +int +build_append_named_qword(GArray *array, const char *name_format, ...) +GCC_FMT_ATTR(2, 3); + void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base, uint64_t len, int node, MemoryAffinityFlags flags);