From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=35428 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1P7nhe-0001gh-BJ
	for qemu-devel@nongnu.org; Mon, 18 Oct 2010 07:16:31 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <weil@mail.berlios.de>) id 1P7nhd-00011h-Bv
	for qemu-devel@nongnu.org; Mon, 18 Oct 2010 07:16:30 -0400
Received: from moutng.kundenserver.de ([212.227.126.171]:54228)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <weil@mail.berlios.de>) id 1P7nhd-00010s-1N
	for qemu-devel@nongnu.org; Mon, 18 Oct 2010 07:16:29 -0400
Message-ID: <4CBC2C7A.4000803@mail.berlios.de>
Date: Mon, 18 Oct 2010 13:16:10 +0200
From: Stefan Weil <weil@mail.berlios.de>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH 1/2] pci: Automatically patch PCI device
	id in	PCI ROM
References: <m3bp6y5zvu.fsf@blackfin.pond.sub.org>
	<1287175867-7757-1-git-send-email-weil@mail.berlios.de>
	<4CBC1BA8.7030604@redhat.com>
In-Reply-To: <4CBC1BA8.7030604@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>, QEMU Developers <qemu-devel@nongnu.org>, Markus Armbruster <armbru@redhat.com>

Am 18.10.2010 12:04, schrieb Gerd Hoffmann:
>   Hi,
>
>> +    /* Don't patch a rom with wrong vendor id (might be changed if 
>> needed). */
>> +    if (vendor_id != rom_vendor_id) {
>> +        return;
>> +    }
>
> Yes, please drop that one.  If this is accepted I'd like to use this for
> vga roms too, so we have to carry only two of them instead of four.
>
>> +    if (device_id != rom_device_id) {
>> +        /* Patch device id and checksum (at offset 6 for etherboot 
>> roms). */
>
> Does this offset work for all roms?


As far as I know there is no well-defined checksum offset.
The checksum is simply set by modifying any byte (which
normally should be unused).

Etherboot has some unused bytes at the beginning of rom data
and always uses the same offset 6.

For other roms which also don't use the byte at offset 6, this approach
will work, too. If they store code or vital data at that location,
we destroy that data, so it won't work.

The VGA bios roms have a sequence of several bytes of zero
starting at offset 6, so maybe this data is not important and
we may change the byte at offset 6, but that should be checked
before using this mechanism.

>
>>   /* Add an option rom for the device */
>>   static int pci_add_option_rom(PCIDevice *pdev)
>>   {
>> @@ -1849,6 +1900,8 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>       load_image(path, ptr);
>>       qemu_free(path);
>>
>> +    pci_patch_device_id(pdev, ptr, size);
>> +
>
> I'd prefer this being opt-in per driver instead of being applied 
> globally (and maybe also pass in a flag whenever a vendor mismatch is 
> fine or not).
>
> cheers,
>   Gerd

As long as the driver specifies the romfile name,
we get an implicitly defined behaviour: either the
rom matches and nothing special is done, or it doesn't
and the id(s) will be fixed.

So neither flag nor opt-in seems to be needed.