From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46284) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ce4QO-0004mO-53 for qemu-devel@nongnu.org; Wed, 15 Feb 2017 13:35:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ce4QK-0004Yq-R6 for qemu-devel@nongnu.org; Wed, 15 Feb 2017 13:35:32 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60478) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ce4QK-0004Yc-FO for qemu-devel@nongnu.org; Wed, 15 Feb 2017 13:35:28 -0500 Date: Wed, 15 Feb 2017 19:35:20 +0100 From: Igor Mammedov Message-ID: <20170215193520.1d436ea3@nial.brq.redhat.com> In-Reply-To: <5E496DF7-107A-49E2-95AE-66DC32F6225B@skyportsystems.com> References: <9dffe31a245cf6a717eef8227fe80ca666b168b5.1487139038.git.ben@skyportsystems.com> <20170215115723.77e4d9d7@nial.brq.redhat.com> <1f4a6eb2-0838-2218-88e3-4f65498a41d6@redhat.com> <20170215162225.74d48058@nial.brq.redhat.com> <20170215172515-mutt-send-email-mst@kernel.org> <20170215165602.72e96c28@nial.brq.redhat.com> <20170215183736-mutt-send-email-mst@kernel.org> <20170215184309.521aa90f@nial.brq.redhat.com> <5216BB96-2CB8-4E83-8950-40E44EE5264D@skyportsystems.com> <20170215200506-mutt-send-email-mst@kernel.org> <5E496DF7-107A-49E2-95AE-66DC32F6225B@skyportsystems.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 1/7] linker-loader: Add new 'write pointer' command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ben Warren Cc: "Michael S. Tsirkin" , Laszlo Ersek , qemu-devel@nongnu.org On Wed, 15 Feb 2017 10:14:55 -0800 Ben Warren wrote: > > On Feb 15, 2017, at 10:06 AM, Michael S. Tsirkin wrote: > >=20 > > On Wed, Feb 15, 2017 at 09:54:08AM -0800, Ben Warren wrote: =20 > >>=20 > >> On Feb 15, 2017, at 9:43 AM, Igor Mammedov wr= ote: > >>=20 > >> On Wed, 15 Feb 2017 18:39:06 +0200 > >> "Michael S. Tsirkin" wrote: > >>=20 > >>=20 > >> On Wed, Feb 15, 2017 at 04:56:02PM +0100, Igor Mammedov wrote: > >>=20 > >> On Wed, 15 Feb 2017 17:30:00 +0200 > >> "Michael S. Tsirkin" wrote: > >>=20 > >>=20 > >> On Wed, Feb 15, 2017 at 04:22:25PM +0100, Igor Mammedov= wrote: > >>=20 > >>=20 > >> On Wed, 15 Feb 2017 15:13:20 +0100 > >> Laszlo Ersek wrote: > >>=20 > >>=20 > >> Commenting under Igor's reply for simplicity > >>=20 > >> On 02/15/17 11:57, Igor Mammedov wrote: =20 > >>=20 > >> On Tue, 14 Feb 2017 22:15:43 -0800 > >> ben@skyportsystems.com wrote: > >>=20 > >>=20 > >> From: Ben Warren > >>=20 > >> This is similar to the existing 'add po= inter' > >> functionality, but instead > >> of instructing the guest (BIOS or UEFI)= to > >> patch memory, it instructs > >> the guest to write the pointer back to = QEMU via > >> a writeable fw_cfg file. > >>=20 > >> Signed-off-by: Ben Warren < =20 > >> ben@skyportsystems.com> =20 > >> --- > >> hw/acpi/bios-linker-loader.c | = 58 > >> ++++++++++++++++++++++++++++++++++-- > >> include/hw/acpi/bios-linker-loader.h | = 6 ++++ > >> 2 files changed, 61 insertions(+), 3 de= letions > >> (-) > >>=20 > >> diff --git a/hw/acpi/bios-linker-loader= .c b/hw/ > >> acpi/bios-linker-loader.c > >> index d963ebe..5030cf1 100644 > >> --- a/hw/acpi/bios-linker-loader.c > >> +++ b/hw/acpi/bios-linker-loader.c > >> @@ -78,6 +78,19 @@ struct BiosLinkerLoa= derEntry > >> { > >> uint32_t length; > >> } cksum; > >>=20 > >> + /* > >> + * COMMAND_WRITE_POINTER - wri= te the > >> fw_cfg file (originating from > >> + * @dest_file) at @wr_pointer.= offset, > >> by adding a pointer to the table > >> + * originating from @src_file.= 1,2,4 > >> or 8 byte unsigned > >> + * addition is used depending = on > >> @wr_pointer.size. > >> + */ =20 > >>=20 > >>=20 > >> The words "adding" and "addition" are causing c= onfusion > >> here. > >>=20 > >> In all of the previous discussion, *addition* w= as out > >> of scope from > >> WRITE_POINTER. Again, the firmware is specifica= lly not > >> required to > >> *read* any part of the fw_cfg blob identified by > >> "dest_file". > >>=20 > >> WRITE_POINTER instructs the firmware to return = the > >> allocation address of > >> the downloaded "src_file" to QEMU. Any necessary > >> runtime subscripting > >> within "src_file" is to be handled by QEMU code > >> dynamically. > >>=20 > >> For example, consider that "src_file" has *seve= ral* > >> fields that QEMU > >> wants to massage; in that case, indexing within= QEMU > >> code with field > >> offsets is simply unavoidable. =20 > >>=20 > >> what I don't like here is that this indexing would = be > >> rather fragile > >> and has to be done in different parts of QEMU /devi= ce, AML > >> /. > >>=20 > >> I'd prefer this helper function to have the same > >> @src_offset > >> behavior as ADD_POINTER where patched address could= point > >> to > >> any part of src_file i.e. not just beginning. =20 > >>=20 > >>=20 > >>=20 > >>=20 > >> /* > >> * COMMAND_ADD_POINTER - patch the table (origin= ating > >> from > >> * @dest_file) at @pointer.offset, by adding a p= ointer > >> to the table > >> * originating from @src_file. 1,2,4 or 8 byte u= nsigned > >> * addition is used depending on @pointer.size. > >> */ > >>=20 > >> so the way ADD works is > >> read at offset > >> add table address > >> write result at offset > >>=20 > >> in other words it is always beginning of table that is = added. =20 > >>=20 > >> more exactly it's, read at=20 > >> src_offset =3D *(dst_blob_ptr+dst_offset) > >> *(dst_blob+dst_offset) =3D src_blob_ptr + src_offset > >>=20 > >>=20 > >> Would the following be acceptable? > >>=20 > >>=20 > >> * COMMAND_WRITE_POINTER - update the fw_cfg file > >> (originating from > >> * @dest_file) at @wr_pointer.offset, by writing= a > >> pointer to the table > >> * originating from @src_file. 1,2,4 or 8 byte u= nsigned > >> value > >> * is written depending on @wr_pointer.size. =20 > >>=20 > >> it looses 'adding' part of ADD_POINTER command which handles > >> src_offset, > >> however implementing adding part looks a bit complicated > >> as patched blob (dst) is not in guest memory but in QEMU and > >> on reset *(dst_blob+dst_offset) should be reset to src_offs= et. > >> Considering dst file could be device specific memory (field= /blob/ > >> whatever) > >> it could be hard to track/notice proper reset behavior. > >>=20 > >> So now I'm not sure if src_offset is worth adding. =20 > >>=20 > >>=20 > >> Right. Let's just do this math in QEMU if we have to. > >>=20 > >> Math complicates QEMU code though and not only QMEMU but AML code a= s well. > >> Considering that we are adding a new command and don't have to keep > >> any sort of compatibility we can pass src_offset as part > >> of command instead of hiding it inside of dst_file. > >> Something like this: > >>=20 > >> /* > >> * COMMAND_WRITE_POINTER - write the fw_cfg file (originatin= g from > >> * @dest_file) at @wr_pointer.offset, by writing a pointer to > >> @src_offset > >> * within the table originating from @src_file. 1,2,4 or 8 b= yte > >> unsigned > >> * addition is used depending on @wr_pointer.size. > >> */ > >> struct { > >> char dest_file[BIOS_LINKER_LOADER_FILESZ]; > >> char src_file[BIOS_LINKER_LOADER_FILESZ]; > >> - uint32_t offset; > >> + uint32_t dst_offset; > >> + uint32_t src_offset; > >> uint8_t size; > >> } wr_pointer; > >>=20 > >>=20 > >> OK, this is easy enough to do and maybe we=E2=80=99ll have a use case = in the future. > >> I=E2=80=99ll make this change in v7 =20 > >=20 > >=20 > > So if you do, you want to set it to VMGENID_GUID_OFFSET. > > =20 > Oh, I was going to set it to 0 since that doesn=E2=80=99t require any oth= er changes (other than to SeaBIOS) it should be changed in following places: bios_linker_loader_write_pointer(linker, VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint64_t), - VMGENID_GUID_FW_CFG_FILE); + VMGENID_GUID_FW_CFG_FILE, VMGENID_GUID_OFFSET); =20 ... - cpu_physical_memory_write(vmgenid_addr + VMGENID_GUID_OFFSET, + cpu_physical_memory_write(vmgenid_addr, guid_le.data, sizeof(guid_le.data)); > >>=20 > >>=20 > >>=20 > >>=20 > >>=20 > >>=20 > >>=20 > >>=20 > >>=20 > >> (1) So, the above looks correct, but please rep= lace > >> "adding" with > >> "storing", and "unsigned addition" with "store". > >>=20 > >> Side point: the case for ADD_POINTER is differe= nt; > >> there we patch > >> several individual ACPI objects. The fact that I > >> requested explicit > >> addition within the ADDR method, as opposed to > >> pre-setting VGIA to a > >> nonzero offset, is an *incidental* limitation (= coming > >> from the OVMF ACPI > >> SDT header probe suppressor), and we'll likely = fix that > >> up later, with > >> ALLOCATE command hints or something like that. = However, > >> in > >> WRITE_POINTER, asking for the exact allocation = address > >> of "src_file" is > >> an *inherent* characteristic. > >>=20 > >> For reference, this is the command's descriptio= n from > >> the (not as yet > >> posted) OVMF series: > >>=20 > >> // QemuLoaderCmdWritePointer: the bytes at > >> // [PointerOffset..PointerOffset+PointerSize) i= n the > >> writeable fw_cfg > >> // file PointerFile are to receive the absolute= address > >> of PointeeFile, > >> // as allocated and downloaded by the firmware.= Store > >> the base address > >> // of where PointeeFile's contents have been pl= aced > >> (when > >> // QemuLoaderCmdAllocate has been executed for > >> PointeeFile) to this > >> // portion of PointerFile. > >> // > >> // This command is similar to QemuLoaderCmdAddP= ointer; > >> the difference is > >> // that the "pointer to patch" does not exist in > >> guest-physical address > >> // space, only in "fw_cfg file space". In addit= ion, the > >> "pointer to > >> // patch" is not initialized by QEMU with a pos= sibly > >> nonzero offset > >> // value: the base address of the memory alloca= ted for > >> downloading > >> // PointeeFile shall not increment the pointer,= but > >> overwrite it. > >>=20 > >> In the last SeaBIOS patch series, namely > >>=20 > >> [SeaBIOS] [PATCH v3 2/2] QEMU fw_cfg: Add comma= nd to > >> write back address > >> of file > >>=20 > >> function romfile_loader_write_pointer() impleme= nted > >> just that plain > >> store (not an addition), and that was exactly r= ight. > >>=20 > >> Continuing: > >>=20 > >>=20 > >> + struct { > >> + char dest_file > >> [BIOS_LINKER_LOADER_FILESZ]; > >> + char src_file > >> [BIOS_LINKER_LOADER_FILESZ]; > >> + uint32_t offset; > >> + uint8_t size; > >> + } wr_pointer; > >> + > >> /* padding */ > >> char pad[124]; > >> }; > >> @@ -85,9 +98,10 @@ struct BiosLinkerLoa= derEntry > >> { > >> typedef struct BiosLinkerLoaderEntry > >> BiosLinkerLoaderEntry; > >>=20 > >> enum { > >> - BIOS_LINKER_LOADER_COMMAND_ALLOCAT= E =3D > >> 0x1, > >> - BIOS_LINKER_LOADER_COMMAND_ADD_POI= NTER =3D > >> 0x2, > >> - BIOS_LINKER_LOADER_COMMAND_ADD_CHE= CKSUM =3D > >> 0x3, > >> + BIOS_LINKER_LOADER_COMMAND_ALLOCATE > >> =3D 0x1, > >> + BIOS_LINKER_LOADER_COMMAND_ADD_POI= NTER > >> =3D 0x2, > >> + BIOS_LINKER_LOADER_COMMAND_ADD_CHE= CKSUM > >> =3D 0x3, > >> + BIOS_LINKER_LOADER_COMMAND_WRITE_P= OINTER > >> =3D 0x4, > >> }; > >>=20 > >> enum { > >> @@ -278,3 +292,41 @@ void > >> bios_linker_loader_add_pointer(BIOSLink= er > >> *linker, > >>=20 > >> g_array_append_vals(linker->cmd_blo= b, & > >> entry, sizeof entry); > >> } > >> + > >> +/* > >> + * bios_linker_loader_write_pointer: a= sk guest > >> to write a pointer to the > >> + * source file into the destination fi= le, and > >> write it back to QEMU via > >> + * fw_cfg DMA. > >> + * > >> + * @linker: linker object instance > >> + * @dest_file: destination file that m= ust be > >> written > >> + * @dst_patched_offset: location within > >> destination file blob to be patched > >> + * with the point= er to > >> @src_file, in bytes > >> + * @dst_patched_offset_size: size of t= he > >> pointer to be patched > >> + * at @dst_patche= d_offset > >> in @dest_file blob, in bytes > >> + * @src_file: source file who's addres= s must > >> be taken > >> + */ > >> +void bios_linker_loader_write_pointer > >> (BIOSLinker *linker, > >> + co= nst char > >> *dest_file, > >> + ui= nt32_t > >> dst_patched_offset, > >> + ui= nt8_t > >> dst_patched_size, > >> + co= nst char > >> *src_file) =20 > >>=20 > >> API is missing "src_offset" even though it'= s not > >> used in this series, > >> a patch on top to fix it up is ok for me as= far as > >> Seabios/OVMF > >> counterpart can handle src_offset correctly= from > >> starters. =20 > >>=20 > >>=20 > >> According to the above, it is the right thing n= ot to > >> add "src_offset" > >> here. The documentation on the command is sligh= tly > >> incorrect (and causes > >> confusion), but the helper function's signature= and > >> comments are okay. > >>=20 > >>=20 > >>=20 > >>=20 > >> +{ > >> + BiosLinkerLoaderEntry entry; > >> + const BiosLinkerFileEntry *source_= file =3D > >> + bios_linker_find_file(linker, > >> src_file); > >> + > >> + assert(source_file); =20 > >>=20 > >>=20 > >> I wish we kept the following asserts from > >> bios_linker_loader_add_pointer(): > >>=20 > >> assert(dst_patched_offset < dst_file->blob->= len); > >> assert(dst_patched_offset + dst_patched_size= <=3D > >> dst_file->blob->len); > >>=20 > >> Namely, just because the dst_file is never supp= osed to > >> be downloaded by > >> the firmware, it still remains a requirement th= at the > >> "dst file offset > >> range" that is to be rewritten *do fall* within= the dst > >> file. > >>=20 > >> Nonetheless, this is not critical. (OVMF at lea= st > >> verifies it anyway.) > >>=20 > >> Summary (from my side anyway): I feel that the > >> documentation of the new > >> command is very important. Please fix it up as > >> suggested under (1), in > >> v7. Regarding the asserts, I'll let you decide. > >>=20 > >> With the documentation fixed up: > >>=20 > >> Reviewed-by: Laszlo Ersek > >>=20 > >> (If you don't wish to post a v7, I'm also compl= etely > >> fine if Michael or > >> someone else fixes up the docs as proposed in (= 1), > >> before committing the > >> patch.) > >>=20 > >> Thanks! > >> Laszlo > >>=20 > >>=20 > >> + memset(&entry, 0, sizeof entry); > >> + strncpy(entry.wr_pointer.dest_file, > >> dest_file, > >> + sizeof entry.wr_pointer.de= st_file > >> - 1); > >> + strncpy(entry.wr_pointer.src_file, > >> src_file, > >> + sizeof entry.wr_pointer.sr= c_file - > >> 1); > >> + entry.command =3D cpu_to_le32 > >> (BIOS_LINKER_LOADER_COMMAND_WRITE_POINT= ER); > >> + entry.wr_pointer.offset =3D cpu_to= _le32 > >> (dst_patched_offset); > >> + entry.wr_pointer.size =3D dst_patc= hed_size; > >> + assert(dst_patched_size =3D=3D 1 || > >> dst_patched_size =3D=3D 2 || > >> + dst_patched_size =3D=3D 4 || > >> dst_patched_size =3D=3D 8); > >> + > >> + g_array_append_vals(linker->cmd_bl= ob, & > >> entry, sizeof entry); > >> +} > >> diff --git a/include/hw/acpi/ > >> bios-linker-loader.h b/include/hw/acpi/ > >> bios-linker-loader.h > >> index fa1e5d1..f9ba5d6 100644 > >> --- a/include/hw/acpi/bios-linker-loade= r.h > >> +++ b/include/hw/acpi/bios-linker-loade= r.h > >> @@ -26,5 +26,11 @@ void > >> bios_linker_loader_add_pointer(BIOSLink= er > >> *linker, > >> con= st char > >> *src_file, > >> uin= t32_t > >> src_offset); > >>=20 > >> +void bios_linker_loader_write_pointer > >> (BIOSLinker *linker, > >> + = const > >> char *dest_file, > >> + = uint32_t > >> dst_patched_offset, > >> + = uint8_t > >> dst_patched_size, > >> + = const > >> char *src_file); > >> + > >> void bios_linker_loader_cleanup(BIOSLin= ker > >> *linker); > >> #endif =20 >=20