Re: [Qemu-devel] [PATCH v8 3/4] fdc: add function to determine drive chs limits

[John Snow]

On 02/18/2016 05:01 AM, Michael S. Tsirkin wrote:
> On Thu, Feb 18, 2016 at 12:50:51PM +0300, Roman Kagan wrote:
>> On Wed, Feb 17, 2016 at 10:15:32PM +0200, Michael S. Tsirkin wrote:
>>> On Wed, Feb 17, 2016 at 09:25:32PM +0300, Roman Kagan wrote:
>>>> When populating ACPI objects for floppy drives one needs to provide the
>>>> maximum values for cylinder, sector, and head number the drive supports.
>>>>
>>>> This patch adds a function that iterates through the array of predefined
>>>> floppy drive formats and returns the maximum values of c, h, s, out of
>>>> those matching the given floppy drive type.
>>>>
>>>> Signed-off-by: Roman Kagan <address@hidden>
>>>> Cc: Igor Mammedov <address@hidden>
>>>> Cc: "Michael S. Tsirkin" <address@hidden>
>>>> Cc: Marcel Apfelbaum <address@hidden>
>>>> Cc: John Snow <address@hidden>
>>>> Cc: Laszlo Ersek <address@hidden>
>>>> Cc: Kevin O'Connor <address@hidden>
>>>> ---
>>>> changes since v7:
>>>>  - use drive max c,h,s rather than the current diskette geometry
>>>>
>>>>  hw/block/fdc.c         | 23 +++++++++++++++++++++++
>>>>  include/hw/block/fdc.h |  2 ++
>>>>  2 files changed, 25 insertions(+)
>>>>
>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>> index 9838d21..fc3aef9 100644
>>>> --- a/hw/block/fdc.c
>>>> +++ b/hw/block/fdc.c
>>>> @@ -2557,6 +2557,29 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice 
>>>> *fdc, int i)
>>>>      return isa->state.drives[i].drive;
>>>>  }
>>>>  
>>>> +void isa_fdc_get_drive_max_chs(FloppyDriveType type,
>>>> +                               uint8_t *maxc, uint8_t *maxh, uint8_t 
>>>> *maxs)
>>>> +{
>>>> +    const FDFormat *fdf;
>>>> +
>>>> +    *maxc = *maxh = *maxs = 0;
>>>> +    for (fdf = fd_formats; fdf->drive != FLOPPY_DRIVE_TYPE_NONE; fdf++) {
>>>> +        if (fdf->drive != type) {
>>>> +            continue;
>>>> +        }
>>>
>>> Hmm. How does this interact with the fallback/autodetect thing?
>>
>> AFAICS the drive type is chosen at realize time once and for good, based
>> on the fdtype property, the size of the inserted image (autodetect), and
>> the fallback property.
>>
>> So basically there's no interaction: this function only determines the
>> max c,h,s among all the formats of that type.
>>
>>> I wonder whether we can just ignore the type and take
>>> global maximum in all cases.
>>
>> There's non-zero chance that we can; however it's up to the
>> closed-source windows driver and can only be proven by testing relevant
>> image sizes across relevant windows versions.  OTOH the resulting code
>> wouldn't differ too much from this patch.  The patch also appears closer
>> to the definition in the ACPI spec, and I tested it already, so I'd be
>> reluctant to invest more effort into it.
> 
> Whatever jsnow says is right is fine by me.
> 

What is right was not always clear :)

However, the interpretation that this should be the maximum supported
value seems correct, and the function looks mechanically correct.

Style shedding aside;

Reviewed-by: John Snow <address@hidden>

>>>> +        if (*maxc < fdf->max_track) {
>>>> +            *maxc = fdf->max_track;
>>>> +        }
>>>> +        if (*maxh < fdf->max_head) {
>>>> +            *maxh = fdf->max_head;
>>>> +        }
>>>> +        if (*maxs < fdf->last_sect) {
>>>> +            *maxs = fdf->last_sect;
>>>> +        }
>>>> +    }
>>>> +    (*maxc)--;
>>>
>>> Why not just *maxc = fdf->max_track - 1 above?
>>
>> No particular reason, I just thought it was more readable to not mess
>> with - 1 when searching for the maximum.  I don't care either way, can
>> rewrite if you want me to.
>>
>> Roman.
> 
> It's confusing IMHO - also, if there's an error and you find nothing,
> you get 0xFF instead of 0x0.
> 

This function SHOULD always find something. If it doesn't, it means the
fdformat table has no entries for our given FDrive type, which should be
an assert during the boot process over in pick_drive_type and children,
so we should be safe.

--js