Xen devel - Jun 2008 - [patch][vtd] Remove ASSERT in hvmloader.c when assigning disk controller to a guest

Hi, 
If boot a HVM guest configured with a physical disk, it will check the
device id of emulated device and then check physical device. But the
device id of physical device is not necessary 0x7010. So ASSERT may
fail, and the guest crashes at this point. This patch removes the ASSERT
of vendor_id and device_id, to assign SATA controller to guest.

Signed-off-by: Li Zhang <li.zhang@intel.com>

Thanks
-Li



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

If the device is not PIIX3 IDE then presumably we should not do the two
pci_writew() invocations either? Words 0x40 and 0x42 in the PCI config space
could have a completely different purpose in a different IDE controller.

 -- Keir

On 25/6/08 12:21, "Zhang, Li" <li.zhang@intel.com> wrote:
> Hi, 
> If boot a HVM guest configured with a physical disk, it will check the
> device id of emulated device and then check physical device. But the
> device id of physical device is not necessary 0x7010. So ASSERT may
> fail, and the guest crashes at this point. This patch removes the ASSERT
> of vendor_id and device_id, to assign SATA controller to guest.
> 
> Signed-off-by: Li Zhang <li.zhang@intel.com>
> 
> Thanks
> -Li
> 


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

Hi, Keir
If the device is not PIIX3 IDE, we also should do the two pci_writew().
The 0x40 and 0x42 are timing registers of IDE0 and IDE1, and they are
used to enable the IDE command decoding function.
And from the PIIX3, ICH to ICH10, the IDE timing registers addresses are
the same. So I think removing the ASSERT is OK. The original comment in
the file is a little puzzling. 
The attachment is a new version. 
>  -----Original Message-----
>  From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>  Sent: Wednesday, June 25, 2008 7:31 PM
>  To: Zhang, Li; xen-devel@lists.xensource.com
>  Subject: Re: [patch][vtd] Remove ASSERT in hvmloader.c when assigning
disk
>  controller to a guest
>  
>  If the device is not PIIX3 IDE then presumably we should not do the
two
>  pci_writew() invocations either? Words 0x40 and 0x42 in the PCI
config space
>  could have a completely different purpose in a different IDE
controller.
>  
>   -- Keir
>  
>  On 25/6/08 12:21, "Zhang, Li" <li.zhang@intel.com> wrote:
>  
>  > Hi,
>  > If boot a HVM guest configured with a physical disk, it will check
the
>  > device id of emulated device and then check physical device. But
the
>  > device id of physical device is not necessary 0x7010. So ASSERT may
>  > fail, and the guest crashes at this point. This patch removes the
ASSERT
>  > of vendor_id and device_id, to assign SATA controller to guest.
>  >
>  > Signed-off-by: Li Zhang <li.zhang@intel.com>
>  >
>  > Thanks
>  > -Li
>  >
>  

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

Sorry, forget the attachment. :)
>  -----Original Message-----
>  From: xen-devel-bounces@lists.xensource.com
>  [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Zhang, Li
>  Sent: Wednesday, June 25, 2008 10:36 PM
>  To: Keir Fraser; xen-devel@lists.xensource.com
>  Subject: [Xen-devel] RE: [patch][vtd] Remove ASSERT in hvmloader.c
>  whenassigning disk controller to a guest
>  
>  Hi, Keir
>  If the device is not PIIX3 IDE, we also should do the two
pci_writew().
>  The 0x40 and 0x42 are timing registers of IDE0 and IDE1, and they are
>  used to enable the IDE command decoding function.
>  And from the PIIX3, ICH to ICH10, the IDE timing registers addresses
are
>  the same. So I think removing the ASSERT is OK. The original comment
in
>  the file is a little puzzling.
>  The attachment is a new version.
>  
>  >  -----Original Message-----
>  >  From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>  >  Sent: Wednesday, June 25, 2008 7:31 PM
>  >  To: Zhang, Li; xen-devel@lists.xensource.com
>  >  Subject: Re: [patch][vtd] Remove ASSERT in hvmloader.c when
assigning
>  disk
>  >  controller to a guest
>  >
>  >  If the device is not PIIX3 IDE then presumably we should not do
the
>  two
>  >  pci_writew() invocations either? Words 0x40 and 0x42 in the PCI
>  config space
>  >  could have a completely different purpose in a different IDE
>  controller.
>  >
>  >   -- Keir
>  >
>  >  On 25/6/08 12:21, "Zhang, Li" <li.zhang@intel.com>
wrote:
>  >
>  >  > Hi,
>  >  > If boot a HVM guest configured with a physical disk, it will
check
>  the
>  >  > device id of emulated device and then check physical device.
But
>  the
>  >  > device id of physical device is not necessary 0x7010. So ASSERT
may
>  >  > fail, and the guest crashes at this point. This patch removes
the
>  ASSERT
>  >  > of vendor_id and device_id, to assign SATA controller to guest.
>  >  >
>  >  > Signed-off-by: Li Zhang <li.zhang@intel.com>
>  >  >
>  >  > Thanks
>  >  > -Li
>  >  >
>  >
>  
>  
>  _______________________________________________
>  Xen-devel mailing list
>  Xen-devel@lists.xensource.com
>  http://lists.xensource.com/xen-devel

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

On 25/6/08 15:38, "Zhang, Li" <li.zhang@intel.com> wrote:
>>  If the device is not PIIX3 IDE, we also should do the two
> pci_writew().
>>  The 0x40 and 0x42 are timing registers of IDE0 and IDE1, and they are
>>  used to enable the IDE command decoding function.
>>  And from the PIIX3, ICH to ICH10, the IDE timing registers addresses
> are
>>  the same. So I think removing the ASSERT is OK. The original comment
> in
>>  the file is a little puzzling.
So the registers exist in all *Intel* chipsets that we care about. What
about other vendors? Should we make the pci_writew() invocations conditional
on vendor_id==0x8086?

 -- Keir



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

The registers exist in all *Intel* chipsets that we care about. And the
registers of some other vendors are different. Here we should add the
ASSERT vendor_id == 0x8086.
See the patch. Thanks.
>  -----Original Message-----
>  From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>  Sent: Wednesday, June 25, 2008 10:47 PM
>  To: Zhang, Li; xen-devel@lists.xensource.com
>  Subject: Re: [Xen-devel] RE: [patch][vtd] Remove ASSERT in
hvmloader.c
>  whenassigning disk controller to a guest
>  
>  On 25/6/08 15:38, "Zhang, Li" <li.zhang@intel.com> wrote:
>  
>  >>  If the device is not PIIX3 IDE, we also should do the two
>  > pci_writew().
>  >>  The 0x40 and 0x42 are timing registers of IDE0 and IDE1, and
they
are
>  >>  used to enable the IDE command decoding function.
>  >>  And from the PIIX3, ICH to ICH10, the IDE timing registers
addresses
>  > are
>  >>  the same. So I think removing the ASSERT is OK. The original
comment
>  > in
>  >>  the file is a little puzzling.
>  
>  So the registers exist in all *Intel* chipsets that we care about.
What
>  about other vendors? Should we make the pci_writew() invocations
conditional
>  on vendor_id==0x8086?
>  
>   -- Keir
>  


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

Add ''if (vendor == 0x8086)'' is better. If using IDE Controller
from
other vendors, add the corresponding condition. Here the guest should
not crash. So send the patch again. Sorry for troubling.
Thanks 
>  -----Original Message-----
>  From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>  Sent: Wednesday, June 25, 2008 10:47 PM
>  To: Zhang, Li; xen-devel@lists.xensource.com
>  Subject: Re: [Xen-devel] RE: [patch][vtd] Remove ASSERT in
hvmloader.c
>  whenassigning disk controller to a guest
>  
>  On 25/6/08 15:38, "Zhang, Li" <li.zhang@intel.com> wrote:
>  
>  >>  If the device is not PIIX3 IDE, we also should do the two
>  > pci_writew().
>  >>  The 0x40 and 0x42 are timing registers of IDE0 and IDE1, and
they
are
>  >>  used to enable the IDE command decoding function.
>  >>  And from the PIIX3, ICH to ICH10, the IDE timing registers
addresses
>  > are
>  >>  the same. So I think removing the ASSERT is OK. The original
comment
>  > in
>  >>  the file is a little puzzling.
>  
>  So the registers exist in all *Intel* chipsets that we care about.
What
>  about other vendors? Should we make the pci_writew() invocations
conditional
>  on vendor_id==0x8086?
>  
>   -- Keir
>  


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