Re: [Qemu-devel] [PATCH v7 3/3] i386: populate floppy drive information in DSDT

[Michael S. Tsirkin]

On Mon, Feb 08, 2016 at 04:00:37PM +0300, Roman Kagan wrote:
> On Sun, Feb 07, 2016 at 11:08:07AM +0200, Michael S. Tsirkin wrote:
> > On Tue, Jan 26, 2016 at 02:50:25PM +0100, Igor Mammedov wrote:
> > > From: Roman Kagan <address@hidden>
> > > -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)
> > 
> > acpi spec says these are WORD values. Are they really uint8_t?
> 
> Yes, see struct FDrive.
> 
> > > +    fdi = aml_package(0x10);
> > 
> > Why 0x10 and not 16?
> 
> I'm unaware of the difference...  If you have any preference I'll
> adjust.  (Originally I stuck with hex because so did iasl -d and the
> then hand-written ASL).

Decimal as other packages please.

> > > +    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 */
> > > +    aml_append(fdi, aml_int(sectors));  /* Maximum Sector Number */
> > > +    aml_append(fdi, aml_int(heads - 1));  /* Maximum Head Number */
> > 
> > Doesn't above change on media change?
> 
> I guess no, because this IMHO is supposed to describe the drive
> properties, not the diskette properties.  But I'll double-check.

Where you take this from seems to be diskette properties though ...

> I'm confused about the status of this patchset: I saw you post a pull
> request with this series last week, and now you review it.  What should
> I do now in response to your comments: rework and resubmit it or post
> incremental fixes on top of it?
> 
> Thanks,
> Roman.

I dropped and re-did review because Igor reported XP crashing.
You should address comments and resubmit.

Thanks!

-- 
MST