| Message ID | 20191113091809.31365-1-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ahci: zero-initialize port struct | expand |
On 11/13/19 10:18, Gerd Hoffmann wrote: > Specifically port->driver.lchs needs clearing, otherwise seabios will s/driver/drive/ > try interpret whatever random crap happens to be there as disk geometry, > which may or may not break boot depending on how lucky you are. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > src/hw/ahci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/hw/ahci.c b/src/hw/ahci.c > index 97a072a1ca81..d45b4307ec68 100644 > --- a/src/hw/ahci.c > +++ b/src/hw/ahci.c > @@ -345,6 +345,7 @@ ahci_port_alloc(struct ahci_ctrl_s *ctrl, u32 pnr) > warn_noalloc(); > return NULL; > } > + memset(port, 0, sizeof(*port)); > port->pnr = pnr; > port->ctrl = ctrl; > port->list = memalign_tmp(1024, 1024); > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
On Wed, Nov 13, 2019 at 10:35:03AM +0100, Laszlo Ersek wrote: > On 11/13/19 10:18, Gerd Hoffmann wrote: > > Specifically port->driver.lchs needs clearing, otherwise seabios will > > s/driver/drive/ > Reviewed-by: Laszlo Ersek <lersek@redhat.com> Typo fixed & pushed. thanks, Gerd
Hi,
Does this fix a bug that actually happened?
I just noticed that in my lchs patches I assumed that lchs struct is
zeroed out in all devices (not only ahci):
9caa19be0e53 (geometry: Apply LCHS values for boot devices)
Seems like this is not the case but why only ahci is affected?
The list of devices is at least:
* ata
* ahci
* scsi
* esp
* lsi
* megasas
* mpt
* pvscsi
* virtio
* virtio-blk
As specified in the commit message.
Also Gerd it seems that my lchs patches were not committed in the
latest submitted version (v4)!!!
The ABI of the fw config key is completely broken.
On Wed, Nov 13, 2019 at 4:02 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Nov 13, 2019 at 10:35:03AM +0100, Laszlo Ersek wrote:
> > On 11/13/19 10:18, Gerd Hoffmann wrote:
> > > Specifically port->driver.lchs needs clearing, otherwise seabios will
> >
> > s/driver/drive/
>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Typo fixed & pushed.
>
> thanks,
> Gerd
> _______________________________________________
> SeaBIOS mailing list -- seabios@seabios.org
> To unsubscribe send an email to seabios-leave@seabios.org
We should make sure drive.lchs is zeroed out for the following devices: git grep "drive_s drive" src/hw/ata.h: struct drive_s drive; src/hw/esp-scsi.c: struct drive_s drive; src/hw/lsi-scsi.c: struct drive_s drive; src/hw/megasas.c: struct drive_s drive; src/hw/mpt-scsi.c: struct drive_s drive; src/hw/nvme-int.h: struct drive_s drive; src/hw/pvscsi.c: struct drive_s drive; src/hw/sdcard.c: struct drive_s drive; src/hw/usb-msc.c: struct drive_s drive; src/hw/usb-uas.c: struct drive_s drive; src/hw/virtio-blk.c: struct drive_s drive; src/hw/virtio-scsi.c: struct drive_s drive; Sam On Wed, Nov 13, 2019 at 5:03 PM Sam Eiderman <sameid@google.com> wrote: > > Hi, > > Does this fix a bug that actually happened? > > I just noticed that in my lchs patches I assumed that lchs struct is > zeroed out in all devices (not only ahci): > > 9caa19be0e53 (geometry: Apply LCHS values for boot devices) > > Seems like this is not the case but why only ahci is affected? > > The list of devices is at least: > > * ata > * ahci > * scsi > * esp > * lsi > * megasas > * mpt > * pvscsi > * virtio > * virtio-blk > > As specified in the commit message. > > Also Gerd it seems that my lchs patches were not committed in the > latest submitted version (v4)!!! > The ABI of the fw config key is completely broken. > > > On Wed, Nov 13, 2019 at 4:02 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > On Wed, Nov 13, 2019 at 10:35:03AM +0100, Laszlo Ersek wrote: > > > On 11/13/19 10:18, Gerd Hoffmann wrote: > > > > Specifically port->driver.lchs needs clearing, otherwise seabios will > > > > > > s/driver/drive/ > > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > > > Typo fixed & pushed. > > > > thanks, > > Gerd > > _______________________________________________ > > SeaBIOS mailing list -- seabios@seabios.org > > To unsubscribe send an email to seabios-leave@seabios.org
Sorry the correct list (which includes ahci) is: git grep "drive_s * drive" src/hw/ahci.h: struct drive_s drive; src/hw/ata.h: struct drive_s drive; src/hw/esp-scsi.c: struct drive_s drive; src/hw/lsi-scsi.c: struct drive_s drive; src/hw/megasas.c: struct drive_s drive; src/hw/mpt-scsi.c: struct drive_s drive; src/hw/nvme-int.h: struct drive_s drive; src/hw/pvscsi.c: struct drive_s drive; src/hw/sdcard.c: struct drive_s drive; src/hw/usb-msc.c: struct drive_s drive; src/hw/usb-uas.c: struct drive_s drive; src/hw/virtio-blk.c: struct drive_s drive; src/hw/virtio-scsi.c: struct drive_s drive; Went over this list now, seems only ahci_port_alloc was missing memset(0) so after this patch all of the devices that contain lchs are covered. But we still need to revert my lchs patches and reapply them with the newest version (v4) Sam On Wed, Nov 13, 2019 at 5:18 PM Sam Eiderman <sameid@google.com> wrote: > > We should make sure drive.lchs is zeroed out for the following devices: > > git grep "drive_s drive" > > src/hw/ata.h: struct drive_s drive; > src/hw/esp-scsi.c: struct drive_s drive; > src/hw/lsi-scsi.c: struct drive_s drive; > src/hw/megasas.c: struct drive_s drive; > src/hw/mpt-scsi.c: struct drive_s drive; > src/hw/nvme-int.h: struct drive_s drive; > src/hw/pvscsi.c: struct drive_s drive; > src/hw/sdcard.c: struct drive_s drive; > src/hw/usb-msc.c: struct drive_s drive; > src/hw/usb-uas.c: struct drive_s drive; > src/hw/virtio-blk.c: struct drive_s drive; > src/hw/virtio-scsi.c: struct drive_s drive; > > Sam > > > On Wed, Nov 13, 2019 at 5:03 PM Sam Eiderman <sameid@google.com> wrote: > > > > Hi, > > > > Does this fix a bug that actually happened? > > > > I just noticed that in my lchs patches I assumed that lchs struct is > > zeroed out in all devices (not only ahci): > > > > 9caa19be0e53 (geometry: Apply LCHS values for boot devices) > > > > Seems like this is not the case but why only ahci is affected? > > > > The list of devices is at least: > > > > * ata > > * ahci > > * scsi > > * esp > > * lsi > > * megasas > > * mpt > > * pvscsi > > * virtio > > * virtio-blk > > > > As specified in the commit message. > > > > Also Gerd it seems that my lchs patches were not committed in the > > latest submitted version (v4)!!! > > The ABI of the fw config key is completely broken. > > > > > > On Wed, Nov 13, 2019 at 4:02 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > > On Wed, Nov 13, 2019 at 10:35:03AM +0100, Laszlo Ersek wrote: > > > > On 11/13/19 10:18, Gerd Hoffmann wrote: > > > > > Specifically port->driver.lchs needs clearing, otherwise seabios will > > > > > > > > s/driver/drive/ > > > > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > > > > > Typo fixed & pushed. > > > > > > thanks, > > > Gerd > > > _______________________________________________ > > > SeaBIOS mailing list -- seabios@seabios.org > > > To unsubscribe send an email to seabios-leave@seabios.org
Hi Sam, On 11/13/19 4:03 PM, Sam Eiderman wrote: > Hi, > > Does this fix a bug that actually happened? > > I just noticed that in my lchs patches I assumed that lchs struct is > zeroed out in all devices (not only ahci): > > 9caa19be0e53 (geometry: Apply LCHS values for boot devices) > > Seems like this is not the case but why only ahci is affected? > > The list of devices is at least: > > * ata > * ahci > * scsi > * esp > * lsi > * megasas > * mpt > * pvscsi > * virtio > * virtio-blk > > As specified in the commit message. > > Also Gerd it seems that my lchs patches were not committed in the > latest submitted version (v4)!!! > The ABI of the fw config key is completely broken. What do you mean? Can you be more specific?
Sure, There are two issues here. The first issue is that my commits which applied to seabios master: * 9caa19b - geometry: Apply LCHS values for boot devices * cb56f61 - config: Add toggle for bootdevice information * ad29109 - geometry: Add boot_lchs_find_*() utility functions * b3d2120 - boot: Reorder functions in boot.c * 7c66a43 - geometry: Read LCHS from fw_cfg Are not from the latest version which was submitted to the mailing list (v4) * fw_cfg key name has changed * The value and of the key has changed from binary (v1) to textual (v4) * Other fixes and variable name changes. So these commits need to be reverted and reapplied with the latest version (v4) The second issue is that my commits, (in v4 too) will require this fix that Gerd added ([PATCH] ahci: zero-initialize port struct) since they change how SeaBIOS uses lchs. Previously, before any of my commits, drive.lchs could contain "random crap" since it was always set before being used in setup_translation(). After my patches, get_translation() invokes overriden_lchs_supplied() which checks: "return drive->lchs.cylinder || drive->lchs.head || drive->lchs.sector;" So there is an assumption that "drive->lchs" is zeroed when lchs is not supplied for the host. This was true for all devices using "drive->lchs" (all were memset to 0) except ahci. (I used 'git grep "drive_s * drive"' to find them all). So Gerd fix is indeed needed and then all devices are covered (drive->lchs is memset to 0). Now only the first issue remains... Sam On Wed, Nov 13, 2019 at 6:12 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Hi Sam, > > On 11/13/19 4:03 PM, Sam Eiderman wrote: > > Hi, > > > > Does this fix a bug that actually happened? > > > > I just noticed that in my lchs patches I assumed that lchs struct is > > zeroed out in all devices (not only ahci): > > > > 9caa19be0e53 (geometry: Apply LCHS values for boot devices) > > > > Seems like this is not the case but why only ahci is affected? > > > > The list of devices is at least: > > > > * ata > > * ahci > > * scsi > > * esp > > * lsi > > * megasas > > * mpt > > * pvscsi > > * virtio > > * virtio-blk > > > > As specified in the commit message. > > > > Also Gerd it seems that my lchs patches were not committed in the > > latest submitted version (v4)!!! > > The ABI of the fw config key is completely broken. > > What do you mean? Can you be more specific? >
Links to latest commits from archive. You can see all changes in the cover letter. [SeaBIOS] [PATCH v4 0/5] Add Qemu to SeaBIOS LCHS interface https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/VLNFBEERTWLEUO6LM5BYLBNVIFCTP46M/ [SeaBIOS] [PATCH v4 1/5] geometry: Read LCHS from fw_cfg https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/B3IPD3HH4UPDYJWFE4KX3HXUCNW5GPEW/ [SeaBIOS] [PATCH v4 2/5] boot: Reorder functions in boot.c https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/YDVU3WIGOSKZ2RQSMR5UVQNZ66K4IG65/ [SeaBIOS] [PATCH v4 3/5] boot: Build ata and scsi paths in function https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/RY33DRZZ3UK3UMQ3Q6BY2KUCHRRW4MRK/ [SeaBIOS] [PATCH v4 4/5] geometry: Add boot_lchs_find_*() utility functions https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/DAJOULWFK24DX4DY3VS6WWOOQNWW3GSG/ [SeaBIOS] [PATCH v4 5/5] geometry: Apply LCHS values for boot devices https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/UUCTPPJ4PTS5CUTCFLOH3YOEXGC6HQ4T/ Sam On Wed, Nov 13, 2019 at 6:35 PM Sam Eiderman <sameid@google.com> wrote: > > Sure, > > There are two issues here. > > The first issue is that my commits which applied to seabios master: > > * 9caa19b - geometry: Apply LCHS values for boot devices > * cb56f61 - config: Add toggle for bootdevice information > * ad29109 - geometry: Add boot_lchs_find_*() utility functions > * b3d2120 - boot: Reorder functions in boot.c > * 7c66a43 - geometry: Read LCHS from fw_cfg > > Are not from the latest version which was submitted to the mailing list (v4) > * fw_cfg key name has changed > * The value and of the key has changed from binary (v1) to textual (v4) > * Other fixes and variable name changes. > > So these commits need to be reverted and reapplied with the latest version (v4) > > The second issue is that my commits, (in v4 too) will require this fix > that Gerd added ([PATCH] ahci: zero-initialize port struct) since they > change how SeaBIOS uses lchs. > > Previously, before any of my commits, drive.lchs could contain "random > crap" since it was always set before being used in > setup_translation(). > > After my patches, get_translation() invokes overriden_lchs_supplied() > which checks: "return drive->lchs.cylinder || drive->lchs.head || > drive->lchs.sector;" > So there is an assumption that "drive->lchs" is zeroed when lchs is > not supplied for the host. > > This was true for all devices using "drive->lchs" (all were memset to > 0) except ahci. > (I used 'git grep "drive_s * drive"' to find them all). > > So Gerd fix is indeed needed and then all devices are covered > (drive->lchs is memset to 0). > > Now only the first issue remains... > > Sam > > On Wed, Nov 13, 2019 at 6:12 PM Philippe Mathieu-Daudé > <philmd@redhat.com> wrote: > > > > Hi Sam, > > > > On 11/13/19 4:03 PM, Sam Eiderman wrote: > > > Hi, > > > > > > Does this fix a bug that actually happened? > > > > > > I just noticed that in my lchs patches I assumed that lchs struct is > > > zeroed out in all devices (not only ahci): > > > > > > 9caa19be0e53 (geometry: Apply LCHS values for boot devices) > > > > > > Seems like this is not the case but why only ahci is affected? > > > > > > The list of devices is at least: > > > > > > * ata > > > * ahci > > > * scsi > > > * esp > > > * lsi > > > * megasas > > > * mpt > > > * pvscsi > > > * virtio > > > * virtio-blk > > > > > > As specified in the commit message. > > > > > > Also Gerd it seems that my lchs patches were not committed in the > > > latest submitted version (v4)!!! > > > The ABI of the fw config key is completely broken. > > > > What do you mean? Can you be more specific? > >
On Wed, Nov 13, 2019 at 05:03:58PM +0200, Sam Eiderman wrote: > Hi, > > Does this fix a bug that actually happened? Yes, "make check-qtest" may fail. It's kind of random though. > I just noticed that in my lchs patches I assumed that lchs struct is > zeroed out in all devices (not only ahci): ahci was the only one not zeroing out the struct (yes, I've reviewed them all). > Also Gerd it seems that my lchs patches were not committed in the > latest submitted version (v4)!!! Whoops. Can you sent a patch seabios/master ... v4 please? IIRC there didn't change much, mostly the parser function, so that should be alot less churn than a full revert + v4 reapply. thanks, Gerd
Do you want a single commit with the changes? I am afraid it will be slightly unreadable when looking at file histories. The only commit that didn't change was: [SeaBIOS] [PATCH v4 2/5] boot: Reorder functions in boot.c Sam On Thu, Nov 14, 2019 at 9:20 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Wed, Nov 13, 2019 at 05:03:58PM +0200, Sam Eiderman wrote: > > Hi, > > > > Does this fix a bug that actually happened? > > Yes, "make check-qtest" may fail. It's kind of random though. > > > I just noticed that in my lchs patches I assumed that lchs struct is > > zeroed out in all devices (not only ahci): > > ahci was the only one not zeroing out the struct (yes, I've reviewed > them all). > > > Also Gerd it seems that my lchs patches were not committed in the > > latest submitted version (v4)!!! > > Whoops. Can you sent a patch seabios/master ... v4 please? > > IIRC there didn't change much, mostly the parser function, so that > should be alot less churn than a full revert + v4 reapply. > > thanks, > Gerd >
On Thu, Nov 14, 2019 at 11:08:59AM +0200, Sam Eiderman wrote: > Do you want a single commit with the changes? That is the idea, unless it is too messy. I don't have v4 any more, looks like I've deleted v4 instead of v3 while cleaning up my mail folders, so I can't easily check. Do you have v3 and v4 as git branches somewhere? > I am afraid it will be slightly unreadable when looking at file histories. > The only commit that didn't change was: > [SeaBIOS] [PATCH v4 2/5] boot: Reorder functions in boot.c Hmm, looks like there have been more changes than I remember. Maybe it's better to revert and re-apply the changed patches then. cheers, Gerd
Hi, > > I am afraid it will be slightly unreadable when looking at file histories. > > The only commit that didn't change was: > > [SeaBIOS] [PATCH v4 2/5] boot: Reorder functions in boot.c > > Hmm, looks like there have been more changes than I remember. Yep, v3..v4 diff is pretty big, so revert + apply v4 looks more reasonable. Can you have a look and double-check? https://git.kraxel.org/cgit/seabios/log/?h=lchs-fixup thanks, Gerd
Looks great Sam On Fri, Nov 15, 2019 at 1:35 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > > I am afraid it will be slightly unreadable when looking at file histories. > > > The only commit that didn't change was: > > > [SeaBIOS] [PATCH v4 2/5] boot: Reorder functions in boot.c > > > > Hmm, looks like there have been more changes than I remember. > > Yep, v3..v4 diff is pretty big, so revert + apply v4 looks more > reasonable. Can you have a look and double-check? > > https://git.kraxel.org/cgit/seabios/log/?h=lchs-fixup > > thanks, > Gerd >
diff --git a/src/hw/ahci.c b/src/hw/ahci.c index 97a072a1ca81..d45b4307ec68 100644 --- a/src/hw/ahci.c +++ b/src/hw/ahci.c @@ -345,6 +345,7 @@ ahci_port_alloc(struct ahci_ctrl_s *ctrl, u32 pnr) warn_noalloc(); return NULL; } + memset(port, 0, sizeof(*port)); port->pnr = pnr; port->ctrl = ctrl; port->list = memalign_tmp(1024, 1024);
Specifically port->driver.lchs needs clearing, otherwise seabios will try interpret whatever random crap happens to be there as disk geometry, which may or may not break boot depending on how lucky you are. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/ahci.c | 1 + 1 file changed, 1 insertion(+)