Re: Re: [PATCH][RFC] gfx_passthru: warning when vgabios rom has invalid checksum

[Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>]

[-- Attachment #1: Type: text/plain, Size: 3062 bytes --]

Hi,

Generally, we should validate the checksum right.

Weidong, have you seen the checksum adjusting?

I only saw it in Q35+IGD.
You know my Q35 is buggy. If this is the only issue of my Q35,
2nd patch (validate and disable) is the best way, I think.

Or, is this a IGD specific issue?
If so, checksum adjusting should be a workaround for IGD pass-through.
I assume IGD's VGABIOS is included in the system BIOS and might be
expanded to the RAM in a different way from the PCI gfx card.

I attach 3rd patch:
When invalid checksum is detected, adjust it if igd_passthru is enabled. 
In other case, stop loading.

I tried this on my Q35+IGD, the checksum has been adjusted and worked.

Do you have any idea?

Regards,
Noboru.

Signed-off-by: Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>

> Noboru Iwamatsu wrote:
>> Hi,
>>
>>> Weidong Han writes ("Re: [Xen-devel] Re: [PATCH][RFC] gfx_passthru:
>>> warning when vgabios rom has invalid checksum"):
>>>> Now I understand. Because your Q35 works with recalculated checksum, I
>>>> prefer to only add a warning message, and continue to load rom for gfx
>>>> passthru.
>>> Having read this thread I'm still a bit confused. The problem is that
>>> the VGA BIOS on the graphics card is broken and has a broken checksum,
>>> and the proposed workaround is to recalculate the checksum for the
>>> benefit of the guest ?
>>
>> In the native environment, the VGABIOS, the expansion ROM on the
>> graphics card, is placed into the 0C0000h address space, and then
>> executed. Of course, the checksum of the ROM must be valid.
>>
>> After this initialization, the system BIOS, the actual BIOS of the M/B,
>> can resize the expansion ROM code to reduce the amount of occupied
>> space. If the system BIOS resizes it, a new checksum must be calculated
>> and stored in the ROM image that is on the RAM.
>>
>> So, normally, shadowed VGABIOS, that is placed in 0C0000h, is already
>> modified and its checksum must be recalculated.
>>
>> Qemu-dm copies 0C0000h's contents of the dom0 to guest's 0C0000h.
>> Guest re-uses dom0's used-up VGABIOS.
>>
>> The problem that I mentioned is about this recalculated checksum.
>>
>> System BIOS must guarantee the checksum after the resizing, but,
>> some M/B does not.
>> However, after adjusting the checksum, guest seems to work, and
>> current qemu-dm does so. The buggy system BIOS might just forgets
>> to recalculate.
>>
>> Should we check strictly here?
>>
>>> Does this incorrectly checksummed BIOS work natively (ie without
>>> passthrough) and if so why is passthrough different ? Alternatively
>>> if it doesn't work native why are we trying to make it work with
>>> passthrough ?
>>>
>>> On another level, Weidong, are you suggesting you'd like to see Noboru
>>> produce a different patch which just produces a warning ?
>>
>> I sent "just warning" patch on the first of this thread.
>> I resend it.
> Yes, I ack this one. Acked-by: Weidong Han <weidong.han@intel.com>
>
> Regards,
> Weidong
>> Noboru.
>>
>> Signed-off-by: Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>
>>
>>
>



[-- Attachment #2: adjust-checksum-for-igd.patch --]
[-- Type: text/plain, Size: 806 bytes --]

diff --git a/hw/pass-through.c b/hw/pass-through.c
index ecb3d6f..3381782 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -4258,11 +4258,23 @@ static int setup_vga_pt(void)
         goto out;
     }
 
-    /* Adjust the bios checksum */
+    /* Validate the bios checksum */
     for ( c = (char*)bios; c < ((char*)bios + bios_size); c++ )
         checksum += *c;
     if ( checksum )
-        bios[bios_size - 1] -= checksum;
+    {
+        if ( igd_passthru )
+        {
+            bios[bios_size - 1] -= checksum;
+            PT_LOG("vga bios checksum is adjusted!\n");
+        }
+        else
+        {
+            PT_LOG("vga bios checksum is invalid!\n");
+            rc = -1;
+            goto out;
+        }
+    }
 
     cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
 


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel