Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Sun, Feb 14, 2016 at 05:02:14PM +0200, Michael S. Tsirkin wrote: > On Sat, Feb 13, 2016 at 12:26:53PM -0500, Kevin O'Connor wrote: > > On Tue, Feb 09, 2016 at 07:36:12PM +0100, Laszlo Ersek wrote: > > > 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) > > > > FWIW, that's not how I read the ACPI specification. I read it as > > saying that the information should be filled with the maximum number > > of CHS that the drive can support. So, even if a smaller disk happens > > to be in the drive the maximum the drive supports would not change. > > I agree. It says: > This object returns information about a floppy disk drive. This > information is the same as > that returned by the INT 13 Function 08H on Intel Architecture PCs. > > So disk drive, not disk information. > > And FWIW I have an old "the undocumented PC" book which says > this about int 13h function 8 (read diskette drive parameters): > > "Remember this data is *not* the limits of the media in the drive, > but the maximum capability of the specified drive". Thanks for the info, I've re-implemented the patchset according to it and Windows guests (w2k12, w10) seem to be happy with it, including dynamically (re-)inserting the "diskette" of compatible size. I'll submit the patches shortly. Thanks, Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Sat, Feb 13, 2016 at 12:26:53PM -0500, Kevin O'Connor 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)
>
> FWIW, that's not how I read the ACPI specification. I read it as
> saying that the information should be filled with the maximum number
> of CHS that the drive can support. So, even if a smaller disk happens
> to be in the drive the maximum the drive supports would not change.
I agree. It says:
This object returns information about a floppy disk drive. This
information is the same as
that returned by the INT 13 Function 08H on Intel Architecture PCs.
So disk drive, not disk information.
And FWIW I have an old "the undocumented PC" book which says
this about int 13h function 8 (read diskette drive parameters):
"Remember this data is *not* the limits of the media in the drive,
but the maximum capability of the specified drive".
> Also, FWIW, SeaBIOS uses the standard 1.44MB floppy controller timing
> information even if a 5.25 drive is found - as far as I know this
> information is only ever used on PIO to the floppy controller and the
> QEMU floppy controller doesn't care what timing parameters it is
> programmed with.
>
> -Kevin
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On 02/13/16 18:26, Kevin O'Connor 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)
>
> FWIW, that's not how I read the ACPI specification. I read it as
> saying that the information should be filled with the maximum number
> of CHS that the drive can support. So, even if a smaller disk happens
> to be in the drive the maximum the drive supports would not change.
If that works with Windows, I think it would be optimal for QEMU.
Thanks!
Laszlo
>
> Also, FWIW, SeaBIOS uses the standard 1.44MB floppy controller timing
> information even if a 5.25 drive is found - as far as I know this
> information is only ever used on PIO to the floppy controller and the
> QEMU floppy controller doesn't care what timing parameters it is
> programmed with.
>
> -Kevin
>
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
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)
FWIW, that's not how I read the ACPI specification. I read it as
saying that the information should be filled with the maximum number
of CHS that the drive can support. So, even if a smaller disk happens
to be in the drive the maximum the drive supports would not change.
Also, FWIW, SeaBIOS uses the standard 1.44MB floppy controller timing
information even if a 5.25 drive is found - as far as I know this
information is only ever used on PIO to the floppy controller and the
QEMU floppy controller doesn't care what timing parameters it is
programmed with.
-Kevin
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On 02/10/2016 12:33 PM, Roman Kagan wrote:
> On Wed, Feb 10, 2016 at 12:16:32PM -0500, John Snow wrote:
>> On 02/10/2016 12:10 PM, Roman Kagan wrote:
>>> Well, as I wrote in another mail, SeaBIOS, which is supposed to provide
>>> the same information to int 0x13/0x08, populates it with static data
>>> based only on the drive type as encoded in CMOS at initialization time,
>>> and everyone seem to have been fine with that so far. I'll need to
>>> re-test it with real Windows guests, though.
>>>
>>
>> OK, but what we appear to be doing currently is polling the current
>> geometry values for a drive instead of some pre-chosen ones based on the
>> drive type.
>>
>> What values does SeaBIOS use? (Can you point me to the table it uses?)
>
> src/hw/floppy.c:
>
> struct floppyinfo_s FloppyInfo[] VARFSEG = {
> // Unknown
> { {0, 0, 0}, 0x00, 0x00},
> // 1 - 360KB, 5.25" - 2 heads, 40 tracks, 9 sectors
> { {2, 40, 9}, FLOPPY_SIZE_525, FLOPPY_RATE_300K},
> // 2 - 1.2MB, 5.25" - 2 heads, 80 tracks, 15 sectors
> { {2, 80, 15}, FLOPPY_SIZE_525, FLOPPY_RATE_500K},
> // 3 - 720KB, 3.5" - 2 heads, 80 tracks, 9 sectors
> { {2, 80, 9}, FLOPPY_SIZE_350, FLOPPY_RATE_250K},
> // 4 - 1.44MB, 3.5" - 2 heads, 80 tracks, 18 sectors
> { {2, 80, 18}, FLOPPY_SIZE_350, FLOPPY_RATE_500K},
> // 5 - 2.88MB, 3.5" - 2 heads, 80 tracks, 36 sectors
> { {2, 80, 36}, FLOPPY_SIZE_350, FLOPPY_RATE_1M},
> // 6 - 160k, 5.25" - 1 heads, 40 tracks, 8 sectors
> { {1, 40, 8}, FLOPPY_SIZE_525, FLOPPY_RATE_250K},
> // 7 - 180k, 5.25" - 1 heads, 40 tracks, 9 sectors
> { {1, 40, 9}, FLOPPY_SIZE_525, FLOPPY_RATE_300K},
> // 8 - 320k, 5.25" - 2 heads, 40 tracks, 8 sectors
> { {2, 40, 8}, FLOPPY_SIZE_525, FLOPPY_RATE_250K},
> };
>
> The array is indexed by the floppy drive type from CMOS
>
> Roman.
>
Hm, thanks. These seem like sane geometries, but looking at the QEMU
geometry table ... I'm not sure what these even mean anymore, or what it
might mean when they don't align with the SeaBIOS values. We probably
don't even try very often. Maybe nobody ever has.
{ FLOPPY_DRIVE_TYPE_288, 36, 80, 1, FDRIVE_RATE_1M, }, // 5760
{ FLOPPY_DRIVE_TYPE_288, 39, 80, 1, FDRIVE_RATE_1M, }, // 6240
{ FLOPPY_DRIVE_TYPE_288, 40, 80, 1, FDRIVE_RATE_1M, }, // 6400
{ FLOPPY_DRIVE_TYPE_288, 44, 80, 1, FDRIVE_RATE_1M, }, // 7040
{ FLOPPY_DRIVE_TYPE_288, 48, 80, 1, FDRIVE_RATE_1M, }, // 7680
If sectors are 512 bytes, the byte size of these diskettes range from
2.813 MiB (or 2.99 Mb; I just today discovered the only way to get an
even 2.88 is to count this as ((36 * 80 * 2) * 512 / 1000 / 1024), which
mixes x^2 and x^10 designations) up through a whopping 3.74MiB/"3.84MB"
QEMU seems to try to support some real funky formats. Not even really
sure where Fabrice got these numbers from.
Sigh, as long as things sort of seem to work. I'm not going to invest
any further effort into figuring out exactly what we should be doing. If
the UEFI guests are working I'm happy if you are.
--js
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Wed, Feb 10, 2016 at 12:16:32PM -0500, John Snow wrote:
> On 02/10/2016 12:10 PM, Roman Kagan wrote:
> > Well, as I wrote in another mail, SeaBIOS, which is supposed to provide
> > the same information to int 0x13/0x08, populates it with static data
> > based only on the drive type as encoded in CMOS at initialization time,
> > and everyone seem to have been fine with that so far. I'll need to
> > re-test it with real Windows guests, though.
> >
>
> OK, but what we appear to be doing currently is polling the current
> geometry values for a drive instead of some pre-chosen ones based on the
> drive type.
>
> What values does SeaBIOS use? (Can you point me to the table it uses?)
src/hw/floppy.c:
struct floppyinfo_s FloppyInfo[] VARFSEG = {
// Unknown
{ {0, 0, 0}, 0x00, 0x00},
// 1 - 360KB, 5.25" - 2 heads, 40 tracks, 9 sectors
{ {2, 40, 9}, FLOPPY_SIZE_525, FLOPPY_RATE_300K},
// 2 - 1.2MB, 5.25" - 2 heads, 80 tracks, 15 sectors
{ {2, 80, 15}, FLOPPY_SIZE_525, FLOPPY_RATE_500K},
// 3 - 720KB, 3.5" - 2 heads, 80 tracks, 9 sectors
{ {2, 80, 9}, FLOPPY_SIZE_350, FLOPPY_RATE_250K},
// 4 - 1.44MB, 3.5" - 2 heads, 80 tracks, 18 sectors
{ {2, 80, 18}, FLOPPY_SIZE_350, FLOPPY_RATE_500K},
// 5 - 2.88MB, 3.5" - 2 heads, 80 tracks, 36 sectors
{ {2, 80, 36}, FLOPPY_SIZE_350, FLOPPY_RATE_1M},
// 6 - 160k, 5.25" - 1 heads, 40 tracks, 8 sectors
{ {1, 40, 8}, FLOPPY_SIZE_525, FLOPPY_RATE_250K},
// 7 - 180k, 5.25" - 1 heads, 40 tracks, 9 sectors
{ {1, 40, 9}, FLOPPY_SIZE_525, FLOPPY_RATE_300K},
// 8 - 320k, 5.25" - 2 heads, 40 tracks, 8 sectors
{ {2, 40, 8}, FLOPPY_SIZE_525, FLOPPY_RATE_250K},
};
The array is indexed by the floppy drive type from CMOS
Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
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: > >> 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. There are various possibilities, like returning False for the corresponding drive in _FDE (Floppy Disk Enumerate) object, or returning 0 aka unknown as drive type in _FDI. Anyway I'd hate needing to expose all of that in an ACPI-accessible form, this is going to become too fat. > 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. Indeed, it's supposed to do the same as https://en.wikipedia.org/wiki/INT_13H#INT_13h_AH.3D08h:_Read_Drive_Parameters and, as I wrote in another mail, the SeaBIOS implementation here is rather simplistic. > 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. I'll test and let you know. Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On 02/10/2016 12:10 PM, Roman Kagan wrote: > On Tue, Feb 09, 2016 at 07:36:12PM +0100, Laszlo Ersek wrote: >> 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. >> >> 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. > > Well, as I wrote in another mail, SeaBIOS, which is supposed to provide > the same information to int 0x13/0x08, populates it with static data > based only on the drive type as encoded in CMOS at initialization time, > and everyone seem to have been fine with that so far. I'll need to > re-test it with real Windows guests, though. > OK, but what we appear to be doing currently is polling the current geometry values for a drive instead of some pre-chosen ones based on the drive type. What values does SeaBIOS use? (Can you point me to the table it uses?) We could just duplicate those, probably. >> IMHO, do the *absolute minimum* to adapt this AML generation patch to >> John's FDC rework, and ignore all dynamic aspects (like media change). > > Hopefully that'll suffice. > > Roman. >
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Tue, Feb 09, 2016 at 07:36:12PM +0100, Laszlo Ersek wrote: > 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. > > 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. Well, as I wrote in another mail, SeaBIOS, which is supposed to provide the same information to int 0x13/0x08, populates it with static data based only on the drive type as encoded in CMOS at initialization time, and everyone seem to have been fine with that so far. I'll need to re-test it with real Windows guests, though. > IMHO, do the *absolute minimum* to adapt this AML generation patch to > John's FDC rework, and ignore all dynamic aspects (like media change). Hopefully that'll suffice. Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Tue, Feb 09, 2016 at 11:22:01AM -0500, John Snow wrote:
> > 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.
Indeed (sorry the patch was developed a couple of months ago so I had to
look at the code to refresh my memory).
However, I tried to implement the part of ACPI spec that read
> 9.9.2 _FDI (Floppy Disk Information)
>
> This object returns information about a floppy disk drive. This
> information is the same as that returned by the INT 13 Function 08H on
> IA-PCs.
so I went ahead and looked into what SeaBIOS did for int 0x13/0x08. And
what it did was read the CMOS at 0x10 and obtain the drive type, and
then return a hardcoded set of parameters (including geometry)
associated to that drive type. So this was what I basically did here,
too. (As a matter of fact the first patch I submitted was just pure ASL
which mimicked exactly the SeaBIOS behavior: read the CMOS and return
the corresponding Package with parameters.)
And IIRC the drive type couldn't change at runtime so I thought I wasn't
doing worse than it was.
As for what to do now, I'll try to check how tolerant the guests are of
changing the floppy geometry under them without updating _FDI, and then
decide.
Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
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
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
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,
>>
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
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.
>
> 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 vi
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
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.
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,
- 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 u
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
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.
>
> Roman.
>
--
—js
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
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. 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. Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On 02/08/2016 08:14 AM, Roman Kagan wrote:
> On Fri, Feb 05, 2016 at 07:25:07PM +0100, Igor Mammedov wrote:
>> On Thu, 4 Feb 2016 23:54:13 +0200
>> "Michael S. Tsirkin" wrote:
>>> -static Aml *build_fdc_device_aml(void)
>>> +static Aml *build_fdinfo_aml(int idx, uint8_t type, uint8_t cylinders,
>>> + uint8_t heads, uint8_t sectors)
>>> +{
>>> +Aml *dev, *fdi;
>>> +
>>> +dev = aml_device("FLP%c", 'A' + idx);
>>> +
>>> +aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
>>> +
>>> +fdi = aml_package(0x10);
>>> +aml_append(fdi, aml_int(idx)); /* Drive Number */
>>> +aml_append(fdi,
>>> +aml_int(cmos_get_fd_drive_type(type))); /* Device Type */
>>> +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
>
> Accidentally, I removed a "feature" where empty drives had geometry
> values applied to them, which allows seek on empty drives to work
> "by accident," as QEMU actually tries to disallow that.
>
> Seeks on empty drives should work, though, but the easiest thing is to
> restore the misfeature where empty drives have non-zero geometries
> applied.
>
> Document the hack accordingly.
>
> [Maintainer edit]
>
> This fix corrects a regression introduced in d5d47efc, where
> pick_geometry was modified such that it would not operate on empty
> drives, and as a result if there is no diskette inserted, QEMU
> no longer populates it with geometry bounds. As a result, seek fails
> when QEMU denies to move the current track, but reports success anyway.
> This can confuse the guest, leading to kernel panics in the guest.
>
>
> Signed-off-by: John Snow
> Reviewed-by: Eric Blake
> Message-id: 1454106932-17236-1-git-send-email-js...@redhat.com
>
> Roman.
>
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.
What do the guests use these values for? Are they fixed at boot?
--js
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Fri, Feb 05, 2016 at 07:25:07PM +0100, Igor Mammedov wrote:
> On Thu, 4 Feb 2016 23:54:13 +0200
> "Michael S. Tsirkin" wrote:
> > -static Aml *build_fdc_device_aml(void)
> > +static Aml *build_fdinfo_aml(int idx, uint8_t type, uint8_t cylinders,
> > + uint8_t heads, uint8_t sectors)
> > +{
> > +Aml *dev, *fdi;
> > +
> > +dev = aml_device("FLP%c", 'A' + idx);
> > +
> > +aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
> > +
> > +fdi = aml_package(0x10);
> > +aml_append(fdi, aml_int(idx)); /* Drive Number */
> > +aml_append(fdi,
> > +aml_int(cmos_get_fd_drive_type(type))); /* Device Type */
> > +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
Accidentally, I removed a "feature" where empty drives had geometry
values applied to them, which allows seek on empty drives to work
"by accident," as QEMU actually tries to disallow that.
Seeks on empty drives should work, though, but the easiest thing is to
restore the misfeature where empty drives have non-zero geometries
applied.
Document the hack accordingly.
[Maintainer edit]
This fix corrects a regression introduced in d5d47efc, where
pick_geometry was modified such that it would not operate on empty
drives, and as a result if there is no diskette inserted, QEMU
no longer populates it with geometry bounds. As a result, seek fails
when QEMU denies to move the current track, but reports success anyway.
This can confuse the guest, leading to kernel panics in the guest.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Message-id: 1454106932-17236-1-git-send-email-js...@redhat.com
Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Thu, 4 Feb 2016 23:54:13 +0200
"Michael S. Tsirkin" wrote:
> From: Roman Kagan
>
> On x86-based systems Linux determines the presence
> and the type of floppy drives via a query of a CMOS field.
> So does SeaBIOS when populating the return data for
> int 0x13 function 0x08.
>
> However Windows doesn't do it. Instead, it requests
> this information from BIOS via int 0x13/0x08 or through
> ACPI objects _FDE (Floppy Drive Enumerate) and _FDI
> (Floppy Drive Information) of the floppy controller object.
> On UEFI systems only ACPI-based detection is supported.
>
> QEMU doesn't provide those objects in its ACPI tables
> and as a result floppy drives aren't invisible to Windows
> on UEFI/OVMF.
>
> This patch adds those objects to the floppy controller
> in DSDT, populating them with the information from
> respective QEMU objects.
>
> Signed-off-by: Roman Kagan
> Signed-off-by: Igor Mammedov
> Reviewed-by: Michael S. Tsirkin
> Signed-off-by: Michael S. Tsirkin
> ---
> hw/i386/acpi-build.c | 61
> +---
> 1 file changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 842c731..caa2e87 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -37,6 +37,7 @@
> #include "hw/acpi/bios-linker-loader.h"
> #include "hw/loader.h"
> #include "hw/isa/isa.h"
> +#include "hw/block/fdc.h"
> #include "hw/acpi/memory_hotplug.h"
> #include "hw/mem/nvdimm.h"
> #include "sysemu/tpm.h"
> @@ -1227,10 +1228,47 @@ static void build_hpet_aml(Aml *table)
> aml_append(table, scope);
> }
>
> -static Aml *build_fdc_device_aml(void)
> +static Aml *build_fdinfo_aml(int idx, uint8_t type, uint8_t cylinders,
> + uint8_t heads, uint8_t sectors)
> +{
> +Aml *dev, *fdi;
> +
> +dev = aml_device("FLP%c", 'A' + idx);
> +
> +aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
> +
> +fdi = aml_package(0x10);
> +aml_append(fdi, aml_int(idx)); /* Drive Number */
> +aml_append(fdi,
> +aml_int(cmos_get_fd_drive_type(type))); /* Device Type */
> +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
> +aml_append(fdi, aml_int(sectors)); /* Maximum Sector Number */
> +aml_append(fdi, aml_int(heads - 1)); /* Maximum Head Number */
> +/* SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
> + * the drive type, so shall we */
> +aml_append(fdi, aml_int(0xAF)); /* disk_specify_1 */
> +aml_append(fdi, aml_int(0x02)); /* disk_specify_2 */
> +aml_append(fdi, aml_int(0x25)); /* disk_motor_wait */
> +aml_append(fdi, aml_int(0x02)); /* disk_sector_siz */
> +aml_append(fdi, aml_int(0x12)); /* disk_eot */
> +aml_append(fdi, aml_int(0x1B)); /* disk_rw_gap */
> +aml_append(fdi, aml_int(0xFF)); /* disk_dtl */
> +aml_append(fdi, aml_int(0x6C)); /* disk_formt_gap */
> +aml_append(fdi, aml_int(0xF6)); /* disk_fill */
> +aml_append(fdi, aml_int(0x0F)); /* disk_head_sttl */
> +aml_append(fdi, aml_int(0x08)); /* disk_motor_strt */
> +
> +aml_append(dev, aml_name_decl("_FDI", fdi));
> +return dev;
> +}
> +
> +static Aml *build_fdc_device_aml(ISADevice *fdc)
> {
> +int i;
> Aml *dev;
> Aml *crs;
> +uint32_t fde_buf[5] = {0, 0, 0, 0, cpu_to_le32(0x2)};
>
> dev = aml_device("FDC0");
> aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
> @@ -1243,6 +1281,21 @@ static Aml *build_fdc_device_aml(void)
> aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
> aml_append(dev, aml_name_decl("_CRS", crs));
>
> +for (i = 0; i < MAX_FD; i++) {
> +uint8_t type = isa_fdc_get_drive_type(fdc, i);
> +
> +if (type < FLOPPY_DRIVE_TYPE_NONE) {
> +uint8_t cylinders, heads, sectors;
> +
> +fde_buf[i] = cpu_to_le32(0x1);
> +isa_fdc_get_drive_geometry(fdc, i, &cylinders, &heads, §ors);
> +aml_append(dev,
> +build_fdinfo_aml(i, type, cylinders, heads, sectors));
> +}
> +}
> +aml_append(dev, aml_name_decl("_FDE",
> + aml_buffer(sizeof(fde_buf), (uint8_t *) fde_buf)));
> +
> return dev;
> }
>
> @@ -1387,13 +1440,15 @@ static Aml *build_com_device_aml(uint8_t uid)
>
> static void build_isa_devices_aml(Aml *table)
> {
> +ISADevice *fdc = pc_find_fdc0();
> +
> Aml *scope = aml_scope("_SB.PCI0.ISA");
>
> aml_append(scope, build_rtc_device_aml());
> aml_append(scope, build_kbd_device_aml());
> aml_append(scope, build_mouse_device_aml());
> -if (!!pc_find_fdc0()) {
> -aml_append(scope, build_fdc_device_aml());
> +if (!!fdc) {
> +aml_append(scope, build_fdc_device_aml(fdc));
> }
> aml_append(scope, build_lpt_device_aml());
> aml_append(
