From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58818) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTXwu-0002HH-1C for qemu-devel@nongnu.org; Wed, 10 Feb 2016 11:49:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTXwq-00085h-QB for qemu-devel@nongnu.org; Wed, 10 Feb 2016 11:49:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46752) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTXwq-00085U-GW for qemu-devel@nongnu.org; Wed, 10 Feb 2016 11:49:00 -0500 Date: Wed, 10 Feb 2016 18:48:56 +0200 From: "Michael S. Tsirkin" Message-ID: <20160210184746-mutt-send-email-mst@redhat.com> References: <1454612376-7072-1-git-send-email-mst@redhat.com> <1454612376-7072-49-git-send-email-mst@redhat.com> <20160205192507.41fc6024@nial.brq.redhat.com> <20160208131443.GC6420@rkaganb.sw.ru> <56B8F89F.3090603@redhat.com> <20160209155249.GA13787@rkaganb.sw.ru> <56BA1229.8090809@redhat.com> <56BA319C.9010505@redhat.com> <20160209204743-mutt-send-email-mst@redhat.com> <56BB61E6.1090500@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56BB61E6.1090500@redhat.com> Subject: Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: Peter Maydell , Eduardo Habkost , qemu-devel@nongnu.org, Roman Kagan , Paolo Bonzini , Igor Mammedov , Laszlo Ersek , Richard Henderson On Wed, Feb 10, 2016 at 11:14:30AM -0500, John Snow wrote: > > > On 02/09/2016 01:48 PM, Michael S. Tsirkin wrote: > > On Tue, Feb 09, 2016 at 07:36:12PM +0100, Laszlo Ersek wrote: > >> On 02/09/16 17:22, John Snow wrote: > >>> > >>> > >>> On 02/09/2016 10:52 AM, Roman Kagan wrote: > >>>> On Mon, Feb 08, 2016 at 03:20:47PM -0500, John Snow wrote: > >>>>> On 02/08/2016 08:14 AM, Roman Kagan wrote: > >>>>>> On Fri, Feb 05, 2016 at 07:25:07PM +0100, Igor Mammedov wrote: > >>>>>>>> + aml_append(fdi, > >>>>>>>> + aml_int(cylinders - 1)); /* Maximum Cylinder Number */ > >>>>>>> this puts uint64_t(-1) in AML i.e. cylinders == 0 and overflow happens here > >>>>>>> > >>>>>>> CCing Jon > >>>>>> > >>>>>> I guess this is the effect of John's fdc rework. I used to think zero > >>>>>> geometry was impossible at the time this patch was developed. > >>>>>> > >>>>>> I wonder if it hasn't been fixed already by > >>>>>> > >>>>>> commit fd9bdbd3459e5b9d51534f0747049bc5b6145e07 > >>>>>> Author: John Snow > >>>>>> Date: Wed Feb 3 11:28:55 2016 -0500 > >>>>>> > >>>>>> fdc: fix detection under Linux > >>>>> > >>>>> Yes, hopefully solved on my end. The geometry values for an empty disk > >>>>> are not well defined (they certainly don't have any *meaning*) so if you > >>>>> are populating tables based on an empty drive, I just hope you also have > >>>>> the mechanisms needed to update said tables when the media changes. > >>>> > >>>> I don't. At the time the patch was developed there basically were no > >>>> mechanisms to update the geometry at all (and this was what you patchset > >>>> addressed, in particular, wasn't it?) so I didn't care. > >>>> > >>> > >>> That's not true. > >>> > >>> You could swap different 1.44MB-class diskettes for other geometries, > >>> check this out: > >>> > >>> static const FDFormat fd_formats[] = { > >>> /* First entry is default format */ > >>> /* 1.44 MB 3"1/2 floppy disks */ > >>> { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, }, > >>> { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, }, > >>> { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, }, > >>> { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, }, > >>> { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, }, > >>> { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, }, > >>> { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, }, > >>> { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, }, > >>> ... > >>> > >>> You absolutely could get different sector and track counts before my > >>> patchset. > >>> > >>> > >>>> Now if it actually has to be fully dynamic it's gonna be more > >>>> involved... > >>>> > >>>>> What do the guests use these values for? Are they fixed at boot? > >>>> > >>>> Only Windows guests use it so it's hard to tell. I can only claim that > >>>> if I stick bogus values into that ACPI object the guest fails to read > >>>> the floppy. > >> > >> We discussed this with John a bit on IRC. > >> > >> In my opinion, the real mess in this case is in the ACPI spec itself. If > >> you re-read the _FDI control method's description, the Package that it > >> returns contains *dynamic* geometry data, about the *disk* (not *drive*): > >> > >> - Maximum Cylinder Number // Integer (WORD) > >> - Maximum Sector Number // Integer (WORD) > >> - Maximum Head Number // Integer (WORD) > >> > >> What this seems to require is: the firmware developer should write ACPI > >> code that > >> - dynamically accesses the floppy drive / controller (using MMIO or IO > >> port registers), > >> - retrieves the geometry of the disk actually inserted, > >> - and returns the data nicely packaged. > >> > >> In effect, an ACPI-level driver for the disk. > >> > >> Now, John explained to me (and please keep in mind that this is my > >> personal account of his explanation, not a factual rendering thereof), > >> that there used to be no *standard* way to interrogate the current > >> disk's geometry, other than trial and error with seeking. > >> > > At the very least, the Intel 82078 does not appear to be capable of > replying to any query with a CHS triplet. It *can* report back the total > number of sectors and the size of each sector, but that still requires > geometry guesswork outside of the drive. > > >> Apparently in UEFI Windows, Microsoft didn't want to implement this > >> trial and error seeking, so -- given there was also no portable > >> *hardware spec* to retrieve the same data, directly from the disk drive > >> or controller -- they punted the entire question to ACPI. That is, to > >> the firmware implementor. > >> > >> This is entirely bogus. For one, it ties the platform firmware (the UEFI > >> binary in the flash chip on your motherboard) to your specific floppy > >> drive / controller. Say good-bye to any independently bought & installed > >> floppy drives. > >> > >> In theory, a floppy controller that comes with a separate PCI card could > >> offer an option ROM with a UEFI driver in it, and that UEFI driver could > >> install a separate SSDT with the hardware-matching _FDI in it. Still an > >> unreasonable requirement, given that the *only* way Windows can be > >> installed unattended (with external device drivers) is to provide those > >> drivers on a floppy. Because, end-to-end, > >> > >> do you want unattended UEFI installation with 3rd party drivers? > >> > >> translates to > >> > >> better have a PCI-based floppy controller such that its oprom > >> installs an _FDI with dynamic hardware access, > >> *or* have your platform firmware match your floppy hardware > >> > >> Implementing this in QEMU would require: > >> - inventing virt-only registers for the FDC that provide the current > >> disk geometry, > > We do secretly have these registers, but of course there's no spec to > interpreting what they mean. For instance, what do they read when the > drive is empty? I am not sure that information is codified in the ACPI spec. > > Could be wrong, you seemed to indicate that the ACPI spec said that the > info matches what you get from a certain legacy bios routine, but I > don't know the specifics of that routine, either. > > >> - and generating AML code that reads those registers > >> > >> *or* > >> > >> - implementing the trial and error seeking in AML > >> > >> Waste of time, don't do it. Microsoft have never documented their usage > >> of _FDI. (Their forums are full of confused users whose physical floppy > >> drives don't work under UEFI Windows!) > >> > >> I'm quite sure the _FDI addition in the ACPI spec is actually from > >> Microsoft as well, but of course the *reasoning* / background for _FDI > >> is also not public. Microsoft seem to push stuff into the ACPI spec that > >> serves them, while conveniently ignoring non-optional parts of the spec > >> that they don't feel like supporting (I'm looking at you, > >> DataTableRegion). And their level of support is not public. > >> > >> So, the last paragraph of Roman's email > >> remains > >> relevant -- do whatever ugly static hack is necessary in QEMU's AML > >> generator to restore the one use case to working state that matters: > >> unattended installation to a virtio disk. > >> > >> Noone in their right mind uses floppy in the guest interactively, and > >> even for the unattended installation, floppy is used only because > >> Windows is too stupid to work off a CD-ROM fully automatically. (Where > >> everything one needs would be interrogable from on hardware / ATAPI / SCSI.) > >> > >> IMHO, do the *absolute minimum* to adapt this AML generation patch to > >> John's FDC rework, and ignore all dynamic aspects (like media change). > >> > >> Thanks > >> Laszlo > > > > I'm fine with reporting some static values that make windows work fine. > > What I think is wrong is reporting geometry that happens to > > match current windows. > > > > Can you rephrase that last sentence? I don't follow, I'm sorry. > > > Roman, does the 0xFF "empty disk geometry" hack appear to work for > Windows 10? > > Maybe it's fine enough as-is, as per Laszlo's good synopsis here. > > --js Right. What I would like to avoid, if possible, is reporting the current data for whatever diskette happens to be used at machine_done time. This just makes things fragile if you switch to a different one. -- MST