Re: [edk2] [PATCH v4 00/10] OvmfPkg: support for Q35 SATA

[Jordan Justen]

Laszlo, thanks for both resurrecting and cleaning up this series. :)

And Reza, thanks for getting it working originally!

I had a minor note on 06/10, but series

Reviewed-by: Jordan Justen 

On 2015-09-15 19:57:20, Laszlo Ersek wrote:
> I've gone through Reza's v3 posting with extreme care, and also through
> the feedback that series elicited.
> 
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10545
> 
> Ultimately I've preserved the mostly uncontested patches (v3 1/5, v3
> 2/5, v3 5/5), although I placed them in a more logical / bisectable
> order, plus I updated the commit messages extensively, and squashed in a
> fix from Feng where it was appropriate.
> 
> The controversial patches (v3 3/5, v3 4/5) I've simply dropped, and
> instead of those I implemented Feng's recommendations the best I could
> (see the precise references broken out in the patches). This allowed me
> to avoid touching AhciModeInitialization().
> 
> I also sprinkled a few fixes (cosmetic or otherwise) over DuetPkg and
> PcAtChipsetPkg, plus extended QemuBootOrderLib so that it covers Q35
> SATA disks and CD-ROMs.
> 
> Reza: in
> 
> you mentioned an OVMF "lock up"; on 2014-10-29. I think that *might*
> have been an issue that Hannes fixed later in QEMU; see commit 702c8c8b
> ("ahci: Fix CD-ROM signature").
> 
> Hannes, Gabriel: I've determined that the patches you've been using
> correspond to Reza's v1 and v2 postings. (I forget which one of you has
> been using which one of v1 vs. v2; the point is, none of those were v3.
> In any case, grab v4.)
> 
> Public branch: .
> 
> NOTE: You will also need the following QEMU bugfix for this to work:
> .

What's the impact without the QEMU fix?

-Jordan

> Testing with various guest OSes is welcome. I tested Fedora (installer
> CD-ROM and installed disk). I also did some light-weight regression
> testing with i440fx IDE devices.
> 
> "Enjoy", I guess. :)
> 
> Cc: Alexander Graf 
> Cc: Reza Jelveh 
> Cc: Jordan Justen 
> Cc: Ruiyu Ni 
> Cc: Hannes Reinecke 
> Cc: Gabriel L. Somlo 
> Cc: Feng Tian 
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (7):
>   DuetPkg: SataControllerDxe: fix typo in "DisqulifiedModes"
>   DuetPkg: SataControllerDxe: fix private array subscripting
>   OvmfPkg: SataControllerDxe: add cascading error handling to Start()
>   OvmfPkg: SataControllerDxe: enable IO / mem access and DMA when
> binding
>   OvmfPkg: SataControllerDxe: enable AHCI mode if IS_PCI_SATADPA()
>   PcAtChipsetPkg: IdeControllerDxe: fix protocol usage hints in the INF
> file
>   OvmfPkg: QemuBootOrderLib: recognize Q35 SATA disks / CD-ROMs
> 
> Reza Jelveh (3):
>   OvmfPkg: copy SataControllerDxe from DuetPkg
>   MdeModulePkg: AtaAtapiPassThru: select master/slave around DIAG
> command
>   OvmfPkg: enable SATA controller
> 
>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataControllerDxe.inf |   2 +-
>  PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf |   4 +-
>  DuetPkg/SataControllerDxe/SataController.h   |   2 +-
>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.h  |   9 +-
>  DuetPkg/SataControllerDxe/SataController.c   |  74 +++--
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c  |   5 +
>  OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c  |  43 +
>  {DuetPkg => OvmfPkg}/SataControllerDxe/ComponentName.c   |   0
>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.c  | 172 
> +++-
>  OvmfPkg/OvmfPkgIa32.dsc  |   5 +-
>  OvmfPkg/OvmfPkgIa32.fdf  |   5 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc   |   5 +-
>  OvmfPkg/OvmfPkgIa32X64.fdf   |   5 +-
>  OvmfPkg/OvmfPkgX64.dsc   |   5 +-
>  OvmfPkg/OvmfPkgX64.fdf   |   5 +-
>  15 files changed, 267 insertions(+), 74 deletions(-)
>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/SataControllerDxe.inf (91%)
>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.h (96%)
>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/ComponentName.c (100%)
>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.c (85%)
> 
> -- 
> 1.8.3.1
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 00/10] OvmfPkg: support for Q35 SATA

[Laszlo Ersek]

On 09/18/15 20:47, Jordan Justen wrote:
> Laszlo, thanks for both resurrecting and cleaning up this series. :)
> 
> And Reza, thanks for getting it working originally!

Right, thanks for that!

> 
> I had a minor note on 06/10, but series
> 
> Reviewed-by: Jordan Justen 

Thanks a lot for the quick review!

I guess I'd like to do the following:
- if Ray reviews patches #1 and #2 soon, then I'll go ahead with your
R-b, and commit the thing. And I'll promise to address the note on 06/10
once Mike and Feng answer.

- Otherwise, if Mike and Feng answer earlier than Ray reviews patches #1
and #2, then I can submit a v2 just as well.

- In v2 I could actually replicate the fixes in patch #1 and #2
separately for DuetPkg and OvmfPkg (ie. copy first, then fix both copies
independently). This would add two more patches for OvmfPkg, but it
would also make the OvmfPkg subset self-contained, and I could commit it
without waiting for DuetPkg.

Thanks
Laszlo

> On 2015-09-15 19:57:20, Laszlo Ersek wrote:
>> I've gone through Reza's v3 posting with extreme care, and also through
>> the feedback that series elicited.
>>
>> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10545
>>
>> Ultimately I've preserved the mostly uncontested patches (v3 1/5, v3
>> 2/5, v3 5/5), although I placed them in a more logical / bisectable
>> order, plus I updated the commit messages extensively, and squashed in a
>> fix from Feng where it was appropriate.
>>
>> The controversial patches (v3 3/5, v3 4/5) I've simply dropped, and
>> instead of those I implemented Feng's recommendations the best I could
>> (see the precise references broken out in the patches). This allowed me
>> to avoid touching AhciModeInitialization().
>>
>> I also sprinkled a few fixes (cosmetic or otherwise) over DuetPkg and
>> PcAtChipsetPkg, plus extended QemuBootOrderLib so that it covers Q35
>> SATA disks and CD-ROMs.
>>
>> Reza: in
>> 
>> you mentioned an OVMF "lock up"; on 2014-10-29. I think that *might*
>> have been an issue that Hannes fixed later in QEMU; see commit 702c8c8b
>> ("ahci: Fix CD-ROM signature").
>>
>> Hannes, Gabriel: I've determined that the patches you've been using
>> correspond to Reza's v1 and v2 postings. (I forget which one of you has
>> been using which one of v1 vs. v2; the point is, none of those were v3.
>> In any case, grab v4.)
>>
>> Public branch: .
>>
>> NOTE: You will also need the following QEMU bugfix for this to work:
>> .
> 
> What's the impact without the QEMU fix?
> 
> -Jordan
> 
>> Testing with various guest OSes is welcome. I tested Fedora (installer
>> CD-ROM and installed disk). I also did some light-weight regression
>> testing with i440fx IDE devices.
>>
>> "Enjoy", I guess. :)
>>
>> Cc: Alexander Graf 
>> Cc: Reza Jelveh 
>> Cc: Jordan Justen 
>> Cc: Ruiyu Ni 
>> Cc: Hannes Reinecke 
>> Cc: Gabriel L. Somlo 
>> Cc: Feng Tian 
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (7):
>>   DuetPkg: SataControllerDxe: fix typo in "DisqulifiedModes"
>>   DuetPkg: SataControllerDxe: fix private array subscripting
>>   OvmfPkg: SataControllerDxe: add cascading error handling to Start()
>>   OvmfPkg: SataControllerDxe: enable IO / mem access and DMA when
>> binding
>>   OvmfPkg: SataControllerDxe: enable AHCI mode if IS_PCI_SATADPA()
>>   PcAtChipsetPkg: IdeControllerDxe: fix protocol usage hints in the INF
>> file
>>   OvmfPkg: QemuBootOrderLib: recognize Q35 SATA disks / CD-ROMs
>>
>> Reza Jelveh (3):
>>   OvmfPkg: copy SataControllerDxe from DuetPkg
>>   MdeModulePkg: AtaAtapiPassThru: select master/slave around DIAG
>> command
>>   OvmfPkg: enable SATA controller
>>
>>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataControllerDxe.inf |   2 +-
>>  PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf |   4 +-
>>  DuetPkg/SataControllerDxe/SataController.h   |   2 +-
>>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.h  |   9 +-
>>  DuetPkg/SataControllerDxe/SataController.c   |  74 +++--
>>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c  |   5 +
>>  OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c  |  43 +
>>  {DuetPkg => OvmfPkg}/SataControllerDxe/ComponentName.c   |   0
>>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.c  | 172 
>> +++-
>>  OvmfPkg/OvmfPkgIa32.dsc  |   5 +-
>>  OvmfPkg/OvmfPkgIa32.fdf  |   5 +-
>>  OvmfPkg/OvmfPkgIa32X64.dsc   |   5 +-
>>  OvmfPkg/OvmfPkgIa32X64.fdf   |   5 +-
>>  OvmfPkg/OvmfPkgX64.dsc 

Re: [edk2] [PATCH v4 00/10] OvmfPkg: support for Q35 SATA

[Laszlo Ersek]

On 09/16/15 21:16, Gabriel L. Somlo wrote:
> On Wed, Sep 16, 2015 at 05:32:54PM +0200, Laszlo Ersek wrote:
>> On 09/16/15 16:47, Gabriel L. Somlo wrote:
>>> whole series:
>>> Tested-by: Gabriel Somlo 
>>
>> Thanks for the testing. Here's a small but important update re: QEMU:
>>
 NOTE: You will also need the following QEMU bugfix for this to work:
 .
>>
>> Turns out Hannes already reported this issue, and John Snow (IDE / AHCI
>> / Floppy maintainer in QEMU) already has a patch (more comprehensive
>> than mine) queued for it. So for testing this edk2 series, people should
>> rather pick John's patch (which he designated in a later message in the
>> thread linked above). I tested that QEMU patch and it works.
> 
> After replacing your qemu patch with jsnow's, everything still works
> fine for me.

Thank you very much for retesting!

The QEMU patch in question is upstream now, as commit
aaeda4a3c9e4d1d25c65ce8ca98e2de06daf1eec.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 00/10] OvmfPkg: support for Q35 SATA

[Gabriel L. Somlo]

whole series:
Tested-by: Gabriel Somlo 

Successfully ran F22 Live with this series applied, both before and
after applying the remaining apple-specific set of Reza's patches.

Successfully ran Yosemite (10.10.5) with the remaining out-of-tree
apple-specific patches applied on top of this set.

Thanks much to everyone involved!

Cheers,
--Gabriel

On Wed, Sep 16, 2015 at 04:57:20AM +0200, Laszlo Ersek wrote:
> I've gone through Reza's v3 posting with extreme care, and also through
> the feedback that series elicited.
> 
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10545
> 
> Ultimately I've preserved the mostly uncontested patches (v3 1/5, v3
> 2/5, v3 5/5), although I placed them in a more logical / bisectable
> order, plus I updated the commit messages extensively, and squashed in a
> fix from Feng where it was appropriate.
> 
> The controversial patches (v3 3/5, v3 4/5) I've simply dropped, and
> instead of those I implemented Feng's recommendations the best I could
> (see the precise references broken out in the patches). This allowed me
> to avoid touching AhciModeInitialization().
> 
> I also sprinkled a few fixes (cosmetic or otherwise) over DuetPkg and
> PcAtChipsetPkg, plus extended QemuBootOrderLib so that it covers Q35
> SATA disks and CD-ROMs.
> 
> Reza: in
> 
> you mentioned an OVMF "lock up"; on 2014-10-29. I think that *might*
> have been an issue that Hannes fixed later in QEMU; see commit 702c8c8b
> ("ahci: Fix CD-ROM signature").
> 
> Hannes, Gabriel: I've determined that the patches you've been using
> correspond to Reza's v1 and v2 postings. (I forget which one of you has
> been using which one of v1 vs. v2; the point is, none of those were v3.
> In any case, grab v4.)
> 
> Public branch: .
> 
> NOTE: You will also need the following QEMU bugfix for this to work:
> .
> 
> Testing with various guest OSes is welcome. I tested Fedora (installer
> CD-ROM and installed disk). I also did some light-weight regression
> testing with i440fx IDE devices.
> 
> "Enjoy", I guess. :)
> 
> Cc: Alexander Graf 
> Cc: Reza Jelveh 
> Cc: Jordan Justen 
> Cc: Ruiyu Ni 
> Cc: Hannes Reinecke 
> Cc: Gabriel L. Somlo 
> Cc: Feng Tian 
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (7):
>   DuetPkg: SataControllerDxe: fix typo in "DisqulifiedModes"
>   DuetPkg: SataControllerDxe: fix private array subscripting
>   OvmfPkg: SataControllerDxe: add cascading error handling to Start()
>   OvmfPkg: SataControllerDxe: enable IO / mem access and DMA when
> binding
>   OvmfPkg: SataControllerDxe: enable AHCI mode if IS_PCI_SATADPA()
>   PcAtChipsetPkg: IdeControllerDxe: fix protocol usage hints in the INF
> file
>   OvmfPkg: QemuBootOrderLib: recognize Q35 SATA disks / CD-ROMs
> 
> Reza Jelveh (3):
>   OvmfPkg: copy SataControllerDxe from DuetPkg
>   MdeModulePkg: AtaAtapiPassThru: select master/slave around DIAG
> command
>   OvmfPkg: enable SATA controller
> 
>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataControllerDxe.inf |   2 +-
>  PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf |   4 +-
>  DuetPkg/SataControllerDxe/SataController.h   |   2 +-
>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.h  |   9 +-
>  DuetPkg/SataControllerDxe/SataController.c   |  74 +++--
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c  |   5 +
>  OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c  |  43 +
>  {DuetPkg => OvmfPkg}/SataControllerDxe/ComponentName.c   |   0
>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.c  | 172 
> +++-
>  OvmfPkg/OvmfPkgIa32.dsc  |   5 +-
>  OvmfPkg/OvmfPkgIa32.fdf  |   5 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc   |   5 +-
>  OvmfPkg/OvmfPkgIa32X64.fdf   |   5 +-
>  OvmfPkg/OvmfPkgX64.dsc   |   5 +-
>  OvmfPkg/OvmfPkgX64.fdf   |   5 +-
>  15 files changed, 267 insertions(+), 74 deletions(-)
>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/SataControllerDxe.inf (91%)
>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.h (96%)
>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/ComponentName.c (100%)
>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.c (85%)
> 
> -- 
> 1.8.3.1
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 00/10] OvmfPkg: support for Q35 SATA

[Laszlo Ersek]

On 09/16/15 16:47, Gabriel L. Somlo wrote:
> whole series:
> Tested-by: Gabriel Somlo 

Thanks for the testing. Here's a small but important update re: QEMU:

>> NOTE: You will also need the following QEMU bugfix for this to work:
>> .

Turns out Hannes already reported this issue, and John Snow (IDE / AHCI
/ Floppy maintainer in QEMU) already has a patch (more comprehensive
than mine) queued for it. So for testing this edk2 series, people should
rather pick John's patch (which he designated in a later message in the
thread linked above). I tested that QEMU patch and it works.

Thanks!
Laszlo


>>
>> Testing with various guest OSes is welcome. I tested Fedora (installer
>> CD-ROM and installed disk). I also did some light-weight regression
>> testing with i440fx IDE devices.
>>
>> "Enjoy", I guess. :)
>>
>> Cc: Alexander Graf 
>> Cc: Reza Jelveh 
>> Cc: Jordan Justen 
>> Cc: Ruiyu Ni 
>> Cc: Hannes Reinecke 
>> Cc: Gabriel L. Somlo 
>> Cc: Feng Tian 
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (7):
>>   DuetPkg: SataControllerDxe: fix typo in "DisqulifiedModes"
>>   DuetPkg: SataControllerDxe: fix private array subscripting
>>   OvmfPkg: SataControllerDxe: add cascading error handling to Start()
>>   OvmfPkg: SataControllerDxe: enable IO / mem access and DMA when
>> binding
>>   OvmfPkg: SataControllerDxe: enable AHCI mode if IS_PCI_SATADPA()
>>   PcAtChipsetPkg: IdeControllerDxe: fix protocol usage hints in the INF
>> file
>>   OvmfPkg: QemuBootOrderLib: recognize Q35 SATA disks / CD-ROMs
>>
>> Reza Jelveh (3):
>>   OvmfPkg: copy SataControllerDxe from DuetPkg
>>   MdeModulePkg: AtaAtapiPassThru: select master/slave around DIAG
>> command
>>   OvmfPkg: enable SATA controller
>>
>>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataControllerDxe.inf |   2 +-
>>  PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf |   4 +-
>>  DuetPkg/SataControllerDxe/SataController.h   |   2 +-
>>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.h  |   9 +-
>>  DuetPkg/SataControllerDxe/SataController.c   |  74 +++--
>>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c  |   5 +
>>  OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c  |  43 +
>>  {DuetPkg => OvmfPkg}/SataControllerDxe/ComponentName.c   |   0
>>  {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.c  | 172 
>> +++-
>>  OvmfPkg/OvmfPkgIa32.dsc  |   5 +-
>>  OvmfPkg/OvmfPkgIa32.fdf  |   5 +-
>>  OvmfPkg/OvmfPkgIa32X64.dsc   |   5 +-
>>  OvmfPkg/OvmfPkgIa32X64.fdf   |   5 +-
>>  OvmfPkg/OvmfPkgX64.dsc   |   5 +-
>>  OvmfPkg/OvmfPkgX64.fdf   |   5 +-
>>  15 files changed, 267 insertions(+), 74 deletions(-)
>>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/SataControllerDxe.inf (91%)
>>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.h (96%)
>>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/ComponentName.c (100%)
>>  copy {DuetPkg => OvmfPkg}/SataControllerDxe/SataController.c (85%)
>>
>> -- 
>> 1.8.3.1
>>

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 00/10] OvmfPkg: support for Q35 SATA

[Gabriel L. Somlo]

On Wed, Sep 16, 2015 at 05:32:54PM +0200, Laszlo Ersek wrote:
> On 09/16/15 16:47, Gabriel L. Somlo wrote:
> > whole series:
> > Tested-by: Gabriel Somlo 
> 
> Thanks for the testing. Here's a small but important update re: QEMU:
> 
> >> NOTE: You will also need the following QEMU bugfix for this to work:
> >> .
> 
> Turns out Hannes already reported this issue, and John Snow (IDE / AHCI
> / Floppy maintainer in QEMU) already has a patch (more comprehensive
> than mine) queued for it. So for testing this edk2 series, people should
> rather pick John's patch (which he designated in a later message in the
> thread linked above). I tested that QEMU patch and it works.

After replacing your qemu patch with jsnow's, everything still works
fine for me.

Thanks,
--Gabriel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel