On 9/19/19 3:43 PM, John Snow wrote:
>
>
> On 9/19/19 3:48 PM, Tony Asleson wrote:
>> Initial attempt at returning a media error for ahci. This is certainly
>> wrong and needs serious improvement.
>>
>
> Hi; I have the unfortunate distinction of being the AHCI maintainer.
> Please CC me on future revisions and discussion if you are interacting
> with my problem child.
Will do and thank you for taking a look at this!
> Also remember to CC qemu-block.
>
>> Signed-off-by: Tony Asleson
>> ---
>> hw/ide/ahci.c | 27 +++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index d45393c019..f487764106 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -36,6 +36,7 @@
>> #include "hw/ide/internal.h"
>> #include "hw/ide/pci.h"
>> #include "ahci_internal.h"
>> +#include "block/error_inject.h"
>>
>> #include "trace.h"
>>
>> @@ -999,6 +1000,22 @@ static void ncq_err(NCQTransferState *ncq_tfs)
>> ncq_tfs->used = 0;
>> }
>>
>> +/*
>> + * Figure out correct way to report media error, this is at best a guess
>> + * and based on the output of linux kernel, not even remotely close.
>> + */
>
> Depends on what kind of error, exactly, you're trying to report, and at
> what level. (AHCI, NCQ, SATA, ATA?)
I was trying to return a media error, like a 3/1101 for SCSI device. I
think that is at the ATA level?
> Keep in mind that you've inserted an error path for SATA drives using
> NCQ, but seemingly haven't touched SATA drives not using NCQ, or ATAPI
> devices using either PATA/SATA, or ATA drives on PATA.
Correct, but for trying out a simple read on a SATA drive for Linux I
end up here first :-) Well, until the kernel retries a number of times
and finally gives up and returns an error while also disabling NCQ for
the device.
>> +static void ncq_media_err(NCQTransferState *ncq_tfs, uint64_t err_sector)
>> +{
>> +IDEState *ide_state = _tfs->drive->port.ifs[0];
>> +
>> +ide_state->error = ECC_ERR;
>> +ide_state->status = READY_STAT | ERR_STAT;
>> +ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
>> +ncq_tfs->lba = err_sector;
>> +qemu_sglist_destroy(_tfs->sglist);
>> +ncq_tfs->used = 0;
>> +}
>> +
>
> If you are definitely very sure you only want an ide_state error
> difference, you could just as well call ncq_err and then patch
> ide_state->error.
>
> (I am not sure that's what you want, but... maybe it is?)
As I mentioned above, return an unrecoverable media error.
SCSI sense data can report the first sector in error and I thought I
could do the same for SATA too?
> I'd have to check -- because I can't say the AHCI emulator was designed
> so much as congealed -- but you might need calls to ncq_finish.
>
> usually, ncq_cb handles the return from any NCQ command and will call
> ncq_err and ncq_finish as appropriate to tidy up the command.
>
> It might be a mistake that execute_ncq_command issues ncq_err in the
> `default` arm of the switch statement without a call to finish.
>
> If we do call ncq_finish from this context I'm not sure if we want
> block_acct_done here unconditionally. We may not have started a block
> accounting operation if we never started a backend operation. Everything
> else looks about right to me.
>
>
>> static void ncq_finish(NCQTransferState *ncq_tfs)
>> {
>> /* If we didn't error out, set our finished bit. Errored commands
>> @@ -1065,6 +1082,8 @@ static void execute_ncq_command(NCQTransferState
>> *ncq_tfs)
>> {
>> AHCIDevice *ad = ncq_tfs->drive;
>> IDEState *ide_state = >port.ifs[0];
>> +uint64_t error_sector = 0;
>> +char device_id[32];
>> int port = ad->port_no;
>>
>> g_assert(is_ncq(ncq_tfs->cmd));
>> @@ -1072,6 +1091,14 @@ static void execute_ncq_command(NCQTransferState
>> *ncq_tfs)
>>
>> switch (ncq_tfs->cmd) {
>> case READ_FPDMA_QUEUED:
>> +sprintf(device_id, "%lu", ide_state->wwn);
>
> This seems suspicious for your design in general, but I'd like to not
> run sprintf to a buffer in the hotpath for NCQ.
I totally agree.
I started out using integers in the call for error_in_read, as that is
what SCSI uses internally for wwn. Then I did NVMe and it's using a
string that doesn't apparently need to be an integer for the wwn? so I
changed it to being a string to accommodate.
I also find it interesting that
