Zhang, Li
2008-Jun-25 11:21 UTC
[Xen-devel] [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
Keir Fraser
2008-Jun-25 11:31 UTC
[Xen-devel] 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
Zhang, Li
2008-Jun-25 14:36 UTC
[Xen-devel] RE: [patch][vtd] Remove ASSERT in hvmloader.c when assigning 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 assigningdisk> controller to a guest > > If the device is not PIIX3 IDE then presumably we should not do thetwo> pci_writew() invocations either? Words 0x40 and 0x42 in the PCIconfig space> could have a completely different purpose in a different IDEcontroller.> > -- 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 checkthe> > device id of emulated device and then check physical device. Butthe> > device id of physical device is not necessary 0x7010. So ASSERT may > > fail, and the guest crashes at this point. This patch removes theASSERT> > 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
Zhang, Li
2008-Jun-25 14:38 UTC
RE: [Xen-devel] RE: [patch][vtd] Remove ASSERT in hvmloader.c whenassigning disk controller to a guest
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 twopci_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 addressesare> the same. So I think removing the ASSERT is OK. The original commentin> 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 whenassigning> disk > > controller to a guest > > > > If the device is not PIIX3 IDE then presumably we should not dothe> 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 willcheck> the > > > device id of emulated device and then check physical device. But > the > > > device id of physical device is not necessary 0x7010. So ASSERTmay> > > fail, and the guest crashes at this point. This patch removesthe> 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
Keir Fraser
2008-Jun-25 14:46 UTC
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
Zhang, Li
2008-Jun-26 04:51 UTC
RE: [Xen-devel] RE: [patch][vtd] Remove ASSERT in hvmloader.c whenassigning disk controller to a guest
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 inhvmloader.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 theyare> >> used to enable the IDE command decoding function. > >> And from the PIIX3, ICH to ICH10, the IDE timing registersaddresses> > are > >> the same. So I think removing the ASSERT is OK. The originalcomment> > 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() invocationsconditional> on vendor_id==0x8086? > > -- Keir >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhang, Li
2008-Jun-27 01:34 UTC
RE: [Xen-devel] RE: [patch][vtd] Remove ASSERT in hvmloader.c whenassigning disk controller to a guest
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 inhvmloader.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 theyare> >> used to enable the IDE command decoding function. > >> And from the PIIX3, ICH to ICH10, the IDE timing registersaddresses> > are > >> the same. So I think removing the ASSERT is OK. The originalcomment> > 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() invocationsconditional> on vendor_id==0x8086? > > -- Keir >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel