Re: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of returned data

2016-01-25 Thread Laszlo Ersek 
On 01/25/16 03:45, Tian, Feng wrote:
> I am OK with the fix. 
> 
> Reviewed-by: Feng Tian <feng.t...@intel.com>
> 
> PS: The "should" words is my thought on the write case (not tested as 
> well:-)). I thought the first patch should also work on short write case.

Patch committed to SVN as r19737.

Thanks guys!
Laszlo

> 
> Thanks
> Feng
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Saturday, January 23, 2016 00:59
> To: Paolo Bonzini; Tian, Feng
> Cc: edk2-de...@ml01.01.org; js...@redhat.com
> Subject: Re: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length 
> of returned data
> 
> On 01/22/16 15:23, Paolo Bonzini wrote:
>>
>>
>> On 21/01/2016 02:27, Tian, Feng wrote:
>>> Paolo,
>>>
>>> I think for short write case it means the data length to be written 
>>> in AtaPacketReadWrite, that is ByteCount, is less than the one 
>>> shipped in ATA cmd, for example, CDB (READ10.byte7&8).
>>>
>>> For such case, it should jump out the while loop in 
>>> AtaPacketReadWrite and send EFI_DEVICE_ERROR as DRQ is not clear.
>>>
>>> IMHO, Laszo & your patch all looks ok to me. of course, your patch 
>>> did an enhancement on how to reflect the real transfer data length
>>> :)
>>
>> Thanks!  Laszlo, can you commit it?
> 
> Absolutely; I haven't done it already only because Feng didn't send a formal 
> R-b. That fact, combined with "it should ..." in the second paragraph, told 
> me that there were further details to iron out.
> 
> If that's not the case: Feng, can you please post your Reviewed-by as usual? 
> Then I'll add my own T-b as well (already posted) and commit Paolo's patch.
> 
> Thanks
> Laszlo
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

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

Re: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of returned data

2016-01-24 Thread Tian, Feng 
I am OK with the fix. 

Reviewed-by: Feng Tian <feng.t...@intel.com>

PS: The "should" words is my thought on the write case (not tested as well:-)). 
I thought the first patch should also work on short write case.

Thanks
Feng

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Saturday, January 23, 2016 00:59
To: Paolo Bonzini; Tian, Feng
Cc: edk2-de...@ml01.01.org; js...@redhat.com
Subject: Re: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length 
of returned data

On 01/22/16 15:23, Paolo Bonzini wrote:
> 
> 
> On 21/01/2016 02:27, Tian, Feng wrote:
>> Paolo,
>>
>> I think for short write case it means the data length to be written 
>> in AtaPacketReadWrite, that is ByteCount, is less than the one 
>> shipped in ATA cmd, for example, CDB (READ10.byte7&8).
>>
>> For such case, it should jump out the while loop in 
>> AtaPacketReadWrite and send EFI_DEVICE_ERROR as DRQ is not clear.
>>
>> IMHO, Laszo & your patch all looks ok to me. of course, your patch 
>> did an enhancement on how to reflect the real transfer data length
>> :)
> 
> Thanks!  Laszlo, can you commit it?

Absolutely; I haven't done it already only because Feng didn't send a formal 
R-b. That fact, combined with "it should ..." in the second paragraph, told me 
that there were further details to iron out.

If that's not the case: Feng, can you please post your Reviewed-by as usual? 
Then I'll add my own T-b as well (already posted) and commit Paolo's patch.

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

Re: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of returned data

2016-01-22 Thread Paolo Bonzini 


On 21/01/2016 02:27, Tian, Feng wrote:
> Paolo,
> 
> I think for short write case it means the data length to be written in 
> AtaPacketReadWrite, that is ByteCount, is less than the one shipped in ATA 
> cmd, for example, CDB (READ10.byte7&8).
> 
> For such case, it should jump out the while loop in AtaPacketReadWrite and 
> send EFI_DEVICE_ERROR as DRQ is not clear.
> 
> IMHO, Laszo & your patch all looks ok to me. of course, your patch did an 
> enhancement on how to reflect the real transfer data length :) 

Thanks!  Laszlo, can you commit it?

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

Re: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of returned data

2016-01-22 Thread Laszlo Ersek 
On 01/22/16 15:23, Paolo Bonzini wrote:
> 
> 
> On 21/01/2016 02:27, Tian, Feng wrote:
>> Paolo,
>>
>> I think for short write case it means the data length to be written
>> in AtaPacketReadWrite, that is ByteCount, is less than the one
>> shipped in ATA cmd, for example, CDB (READ10.byte7&8).
>>
>> For such case, it should jump out the while loop in
>> AtaPacketReadWrite and send EFI_DEVICE_ERROR as DRQ is not clear.
>>
>> IMHO, Laszo & your patch all looks ok to me. of course, your patch
>> did an enhancement on how to reflect the real transfer data length
>> :)
> 
> Thanks!  Laszlo, can you commit it?

Absolutely; I haven't done it already only because Feng didn't send a
formal R-b. That fact, combined with "it should ..." in the second
paragraph, told me that there were further details to iron out.

If that's not the case: Feng, can you please post your Reviewed-by as
usual? Then I'll add my own T-b as well (already posted) and commit
Paolo's patch.

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

Re: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of returned data

2016-01-20 Thread Tian, Feng 
Paolo,

I think for short write case it means the data length to be written in 
AtaPacketReadWrite, that is ByteCount, is less than the one shipped in ATA cmd, 
for example, CDB (READ10.byte7&8).

For such case, it should jump out the while loop in AtaPacketReadWrite and send 
EFI_DEVICE_ERROR as DRQ is not clear.

IMHO, Laszo & your patch all looks ok to me. of course, your patch did an 
enhancement on how to reflect the real transfer data length :) 

Thanks
Feng

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paolo 
Bonzini
Sent: Wednesday, January 20, 2016 23:58
To: edk2-de...@ml01.01.org
Cc: js...@redhat.com; ler...@redhat.com
Subject: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of 
returned data

For some SCSI commands, notably INQUIRY, it's relatively common for the device 
to provide less data than we intended to read, and for this reason 
EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET makes InTransferLength and 
OutTransferLength read-write.  Make ATAPI aware of this.

This makes it possible to handle EFI_NOT_READY always, not just for read as 
done in r19685.

I've chosen to use a break statement instead of calling CheckStatusRegister 
directly; the break statement reaches a pre-existing call the 
CheckStatusRegister function.  This ensures that the assignment to *ByteCount 
is not missed, and adds a further sanity check to DRQClear.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
***UNTESTED***

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 32 ++---
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index 320ee90..6478f7b 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -1936,7 +1936,7 @@ AtaPacketReadWrite (
   IN EFI_PCI_IO_PROTOCOL   *PciIo,
   IN EFI_IDE_REGISTERS *IdeRegisters,
   IN OUT VOID  *Buffer,
-  IN UINT64ByteCount,
+  IN OUT UINT32*ByteCount,
   IN BOOLEAN   Read,
   IN UINT64Timeout
   )
@@ -1947,17 +1947,18 @@ AtaPacketReadWrite (
   EFI_STATUS  Status;
   UINT16  *PtrBuffer;
 
+  PtrBuffer = Buffer;
+  RequiredWordCount = *ByteCount >> 1;
+
   //
   // No data transfer is premitted.
   //
-  if (ByteCount == 0) {
+  if (RequiredWordCount == 0) {
 return EFI_SUCCESS;
   }
 
-  PtrBuffer = Buffer;
-  RequiredWordCount = (UINT32)RShiftU64(ByteCount, 1);
   //
-  // ActuralWordCount means the word count of data really transferred.
+  // ActualWordCount means the word count of data really transferred.
   //
   ActualWordCount = 0;
 
@@ -1967,14 +1968,16 @@ AtaPacketReadWrite (
 // to see whether indicates device is ready to transfer data.
 //
 Status = DRQReady2 (PciIo, IdeRegisters, Timeout);
-if ((Status == EFI_NOT_READY) && Read) {
-  //
-  // Device provided less data than we intended to read -- exit early.
-  //
-  return CheckStatusRegister (PciIo, IdeRegisters);
-}
 if (EFI_ERROR (Status)) {
-  return Status;
+  if (Status == EFI_NOT_READY) {
+//
+// Device provided less data than we intended to read, or wanted less
+// data than we intended to write, but it may still be successful.
+//
+break;
+  } else {
+return Status;
+  }
 }
 
 //
@@ -2040,6 +2043,7 @@ AtaPacketReadWrite (
 return EFI_DEVICE_ERROR;
   }
 
+  *ByteCount = ActualWordCount << 1;
   return Status;
 }
 
@@ -2138,7 +2142,7 @@ AtaPacketCommandExecute (
PciIo,
IdeRegisters,
Packet->InDataBuffer,
-   Packet->InTransferLength,
+   >InTransferLength,
TRUE,
Packet->Timeout
);
@@ -2147,7 +2151,7 @@ AtaPacketCommandExecute (
PciIo,
IdeRegisters,
Packet->OutDataBuffer,
-   Packet->OutTransferLength,
+   >OutTransferLength,
FALSE,
Packet->Timeout
);
--
2.5.0
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of returned data

2016-01-20 Thread Paolo Bonzini 
For some SCSI commands, notably INQUIRY, it's relatively common for
the device to provide less data than we intended to read, and for
this reason EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET makes
InTransferLength and OutTransferLength read-write.  Make ATAPI
aware of this.

This makes it possible to handle EFI_NOT_READY always, not just
for read as done in r19685.

I've chosen to use a break statement instead of calling
CheckStatusRegister directly; the break statement reaches a
pre-existing call the CheckStatusRegister function.  This
ensures that the assignment to *ByteCount is not missed, and
adds a further sanity check to DRQClear.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Paolo Bonzini 
---
***UNTESTED***

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 32 ++---
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index 320ee90..6478f7b 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -1936,7 +1936,7 @@ AtaPacketReadWrite (
   IN EFI_PCI_IO_PROTOCOL   *PciIo,
   IN EFI_IDE_REGISTERS *IdeRegisters,
   IN OUT VOID  *Buffer,
-  IN UINT64ByteCount,
+  IN OUT UINT32*ByteCount,
   IN BOOLEAN   Read,
   IN UINT64Timeout
   )
@@ -1947,17 +1947,18 @@ AtaPacketReadWrite (
   EFI_STATUS  Status;
   UINT16  *PtrBuffer;
 
+  PtrBuffer = Buffer;
+  RequiredWordCount = *ByteCount >> 1;
+
   //
   // No data transfer is premitted.
   //
-  if (ByteCount == 0) {
+  if (RequiredWordCount == 0) {
 return EFI_SUCCESS;
   }
 
-  PtrBuffer = Buffer;
-  RequiredWordCount = (UINT32)RShiftU64(ByteCount, 1);
   //
-  // ActuralWordCount means the word count of data really transferred.
+  // ActualWordCount means the word count of data really transferred.
   //
   ActualWordCount = 0;
 
@@ -1967,14 +1968,16 @@ AtaPacketReadWrite (
 // to see whether indicates device is ready to transfer data.
 //
 Status = DRQReady2 (PciIo, IdeRegisters, Timeout);
-if ((Status == EFI_NOT_READY) && Read) {
-  //
-  // Device provided less data than we intended to read -- exit early.
-  //
-  return CheckStatusRegister (PciIo, IdeRegisters);
-}
 if (EFI_ERROR (Status)) {
-  return Status;
+  if (Status == EFI_NOT_READY) {
+//
+// Device provided less data than we intended to read, or wanted less
+// data than we intended to write, but it may still be successful.
+//
+break;
+  } else {
+return Status;
+  }
 }
 
 //
@@ -2040,6 +2043,7 @@ AtaPacketReadWrite (
 return EFI_DEVICE_ERROR;
   }
 
+  *ByteCount = ActualWordCount << 1;
   return Status;
 }
 
@@ -2138,7 +2142,7 @@ AtaPacketCommandExecute (
PciIo,
IdeRegisters,
Packet->InDataBuffer,
-   Packet->InTransferLength,
+   >InTransferLength,
TRUE,
Packet->Timeout
);
@@ -2147,7 +2151,7 @@ AtaPacketCommandExecute (
PciIo,
IdeRegisters,
Packet->OutDataBuffer,
-   Packet->OutTransferLength,
+   >OutTransferLength,
FALSE,
Packet->Timeout
);
-- 
2.5.0
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of returned data

2016-01-20 Thread Laszlo Ersek 
Hi Paolo,

On 01/20/16 16:58, Paolo Bonzini wrote:
> For some SCSI commands, notably INQUIRY, it's relatively common for
> the device to provide less data than we intended to read, and for
> this reason EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET makes
> InTransferLength and OutTransferLength read-write.  Make ATAPI
> aware of this.
> 
> This makes it possible to handle EFI_NOT_READY always, not just
> for read as done in r19685.
> 
> I've chosen to use a break statement instead of calling
> CheckStatusRegister directly; the break statement reaches a
> pre-existing call the CheckStatusRegister function.  This
> ensures that the assignment to *ByteCount is not missed, and
> adds a further sanity check to DRQClear.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Paolo Bonzini 
> ---
>   ***UNTESTED***
> 
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 32 
> ++---
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> index 320ee90..6478f7b 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> @@ -1936,7 +1936,7 @@ AtaPacketReadWrite (
>IN EFI_PCI_IO_PROTOCOL   *PciIo,
>IN EFI_IDE_REGISTERS *IdeRegisters,
>IN OUT VOID  *Buffer,
> -  IN UINT64ByteCount,
> +  IN OUT UINT32*ByteCount,
>IN BOOLEAN   Read,
>IN UINT64Timeout
>)
> @@ -1947,17 +1947,18 @@ AtaPacketReadWrite (
>EFI_STATUS  Status;
>UINT16  *PtrBuffer;
>  
> +  PtrBuffer = Buffer;
> +  RequiredWordCount = *ByteCount >> 1;
> +
>//
>// No data transfer is premitted.
>//
> -  if (ByteCount == 0) {
> +  if (RequiredWordCount == 0) {
>  return EFI_SUCCESS;
>}
>  
> -  PtrBuffer = Buffer;
> -  RequiredWordCount = (UINT32)RShiftU64(ByteCount, 1);
>//
> -  // ActuralWordCount means the word count of data really transferred.
> +  // ActualWordCount means the word count of data really transferred.
>//
>ActualWordCount = 0;
>  
> @@ -1967,14 +1968,16 @@ AtaPacketReadWrite (
>  // to see whether indicates device is ready to transfer data.
>  //
>  Status = DRQReady2 (PciIo, IdeRegisters, Timeout);
> -if ((Status == EFI_NOT_READY) && Read) {
> -  //
> -  // Device provided less data than we intended to read -- exit early.
> -  //
> -  return CheckStatusRegister (PciIo, IdeRegisters);
> -}
>  if (EFI_ERROR (Status)) {
> -  return Status;
> +  if (Status == EFI_NOT_READY) {
> +//
> +// Device provided less data than we intended to read, or wanted less
> +// data than we intended to write, but it may still be successful.
> +//
> +break;
> +  } else {
> +return Status;
> +  }
>  }
>  
>  //
> @@ -2040,6 +2043,7 @@ AtaPacketReadWrite (
>  return EFI_DEVICE_ERROR;
>}
>  
> +  *ByteCount = ActualWordCount << 1;
>return Status;
>  }
>  
> @@ -2138,7 +2142,7 @@ AtaPacketCommandExecute (
> PciIo,
> IdeRegisters,
> Packet->InDataBuffer,
> -   Packet->InTransferLength,
> +   >InTransferLength,
> TRUE,
> Packet->Timeout
> );
> @@ -2147,7 +2151,7 @@ AtaPacketCommandExecute (
> PciIo,
> IdeRegisters,
> Packet->OutDataBuffer,
> -   Packet->OutTransferLength,
> +   >OutTransferLength,
> FALSE,
> Packet->Timeout
> );
> 

I tested this the same way I had reproduced the original issue
 and had tested my patches,
with the following command line:

(
  set -e

  cp /.../build-output/OVMF.fd .

  qemu-system-x86_64 \
  -m 2048 \
  -M pc \
  -debugcon file:ovmf.log \
  -global isa-debugcon.iobase=0x402 \
  -net none \
  -enable-kvm \
  -pflash OVMF.fd \
  -drive id=cd0,if=none,readonly,format=raw,file=Fedora22.iso \
  -device ide-cd,drive=cd0,bootindex=0
)

I first reverted my patch (SVN r19685) to see if the bug would still
reproduce. It did (the Fedora 22 installer ISO / the CD-ROM device
doesn't appear, I'm dropped to the UEFI shell).

Then I reapplied r19685 (well, dropped its revert), and then applied
your patch.

With your patch, Grub2 boots fine, the CD-ROM testing menu entry works
fine, and the Live system is launched.

Since your patch seems to modify write paths as well, do you want me to
test some writes as well? I guess I could add an ide-hd device,
formatted as FAT32, and use the CP command to copy files to it.

What would be a good test for a short write? I suspect that the FAT
driver would catch any

Re: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of returned data

2016-01-20 Thread Laszlo Ersek 
On 01/20/16 22:57, Paolo Bonzini wrote:
> 
> 
> On 20/01/2016 19:15, Laszlo Ersek wrote:
>> If you think it's unnecessary to test the *short* write case
>> specifically, then I'm willing to give my T-b. Thanks for the patch!
> 
> Your testing is enough.  The only way to test the short write case would
> probably be by writing a custom application.

Thanks!

Tested-by: Laszlo Ersek 

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