Re: [Qemu-devel] Re: sparc esp NetBSD-guest "sd3: mode sense (4) returned nonsense"

[Blue Swirl <blauwirbel@gmail.com>]

On Thu, Oct 8, 2009 at 7:14 PM, Artyom Tarasenko
<atar4qemu@googlemail.com> wrote:
> 2009/9/23 Artyom Tarasenko <atar4qemu@googlemail.com>:
>> 2009/9/19 Blue Swirl <blauwirbel@gmail.com>:
>>> On Fri, Sep 18, 2009 at 8:26 PM, Artyom Tarasenko
>>> <atar4qemu@googlemail.com> wrote:
>>>> 2009/9/14 Blue Swirl <blauwirbel@gmail.com>:
>>>>> On Mon, Sep 14, 2009 at 7:47 PM, Artyom Tarasenko
>>>>> <atar4qemu@googlemail.com> wrote:
>>>>>> 2009/9/14 Blue Swirl <blauwirbel@gmail.com>:
>>>>>>> On Mon, Sep 14, 2009 at 7:29 PM, Artyom Tarasenko
>>>>>>> <atar4qemu@googlemail.com> wrote:
>>>>>>>> 2009/9/14 Blue Swirl <blauwirbel@gmail.com>:
>>>>>>>>> On Mon, Sep 14, 2009 at 12:32 AM, Artyom Tarasenko
>>>>>>>>> <atar4qemu@googlemail.com> wrote:
>>>>>>>>>> From NetBSD source, it looks like HDD geometry detection should work
>>>>>>>>>> under qemu: they call "mode sense" and "read capacity", and both
>>>>>>>>>> commands are implemented in qemu's hw/scsi-disk.h. It doesn't work
>>>>>>>>>> though, so NetBSD has to fabricate a disk geometry.
>>>>>>>>>>
>>>>>>>>>> To make debugging easier I tried to boot an older version - NetBSD
>>>>>>>>>> 1.3.3. And put some extra debugging in esp.c:
>>>>>>>>>>
>>>>>>>>>> static uint32_t get_cmd(ESPState *s, uint8_t *buf)
>>>>>>>>>> {
>>>>>>>>>>    uint32_t dmalen;
>>>>>>>>>>    int target;
>>>>>>>>>>
>>>>>>>>>>    target = s->wregs[ESP_WBUSID] & BUSID_DID;
>>>>>>>>>>    if (s->dma) {
>>>>>>>>>>        dmalen = s->rregs[ESP_TCLO] | (s->rregs[ESP_TCMID] << 8);
>>>>>>>>>>        s->dma_memory_read(s->dma_opaque, buf, dmalen);
>>>>>>>>>>    } else {
>>>>>>>>>>        dmalen = s->ti_size;
>>>>>>>>>>        memcpy(buf, s->ti_buf, dmalen);
>>>>>>>>>> printf("NON-DMA rptr %d, wptr %d %2x (0) %2x %2x %2x %2x\n",
>>>>>>>>>> s->ti_rptr, s-> ti_wptr, buf[0],buf[1], buf[2],buf[3], buf[4]);
>>>>>>>>>>        buf[0] = 0;
>>>>>>>>>>    }
>>>>>>>>>>
>>>>>>>>>> qemu-system-sparc -M SS-20 -nographic  -hda ~/sparc/miniroot-133.fs -m 64
>>>>>>>>>> ...
>>>>>>>>>> NON-DMA rptr 0, wptr 1 c0 (0)  0  0 1a  0
>>>>>>>>>> Set ATN & Stop: cmdlen 3
>>>>>>>>>> scsi-disk: Command: lun=0 tag=0x0 data=0x00 0x00 0x1a 0x00 0x04 0x00
>>>>>>>>>> scsi-disk: Test Unit Ready
>>>>>>>>>> scsi-disk: Command complete tag=0x0 status=0 sense=0
>>>>>>>>>> sd3: mode sense (4) returned nonsense; using fictitious geometry
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> NetBSD sent command "0x1a" via Set ATN & Stop, but it for some reason
>>>>>>>>>> the command got padded and disk got "0x0 0x0 0x1a", no wonder that its
>>>>>>>>>> output looks like a non-sense to NetBSD.
>>>>>>>>>>
>>>>>>>>>> Any ideas why does it happen?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The problem could be in the DMA (sparc32_dma.c), or incorrect
>>>>>>>>> programming of DMA or IOMMU DVMA by NetBSD, (or bug in iommu.c).
>>>>>>>>
>>>>>>>> Why DMA? It hits the else branch of "if (s->dma)". Does the command
>>>>>>>> still get in via DMA?
>>>>>>>
>>>>>>> Sorry, I missed that. But is the response also read without DMA?
>>>>>>>
>>>>>>
>>>>>> You mean the disk's response? It doesn't matter, because the disk just
>>>>>> doesn't get the command.
>>>>>
>>>>> Ah, I see. What about FIFO state then, perhaps there are some leftover
>>>>> bytes (0, 0 could be status + sense?) from the previous command in the
>>>>> buffer before the command is written there?
>>>>
>>>> You were right, it was FIFO, but I ran the tests in a wrong qemu
>>>> branch. It's sort of funny, because the bug was fixed in the HEAD by
>>>> my own patch (the "Message accepted" patch).
>>>>
>>>> Now the disk gets commands properly, but NetBSD still complains about
>>>> getting nonsense.
>>>>
>>>> One of the reasons is, the disk's geometry has to be explicitly
>>>> specified via -hdachs , but
>>>>
>>>>> But is the response also read without DMA?
>>>>
>>>> you are right about this one too. It is read via DMA, and it seems
>>>> that the response gets shifted by -8 bytes:
>>>> the follofing hack in hw/sparc32_dma.c makes NetBSD to recognize the geometry:
>>>
>>> Could be a bug in the DMA controller. For example, the feature for
>>> automatic load of next address is not implemented. IIRC it's not
>>> available in all versions, so downgrading the controller version may
>>> help.
>>
>> Downgrading the controller version didn't change anything. I also
>> tried to boot with -M LX , to downgrade other components as well, the
>> result was still the same.
>>
>> But this brings me to another question: Is there a reason for silent
>> catching of errors produced by unimplemented features?
>>
>> I like the way it is implenented in hw/scsi-disk.c: along with DPRINTF
>> for debugging there is a BADF for reporting unimplemented/unexpected
>> cases. DPRINTFs may be turned on by a #define, and BADFs are always
>> on. Shouldn't similar constructs were used for mmu, iommu and other
>> units with partially implemented funcionality?
>
>
> Actually, scsi-disk.c doesn't implement block descriptor for mode
> pages. The SCSI-2 documentation suggests, that although the block
> descriptor is optional for an arbitrary SCSI-2 device (chapter 8.2.10,
> http://ldkelley.com/SCSI2/SCSI2/SCSI2/SCSI2/SCSI2-08.html ) it is
> mandatory for a disk: chapters 9.1.2, 9.3.3 (
> http://ldkelley.com/SCSI2/SCSI2/SCSI2/SCSI2-09.html ) don't say
> "optional" any more, just "The block descriptor in the MODE SENSE data
> describes the block lengths that are used on the medium."

I agree.

> NetBSD expects that the block descriptor is always there:
> sd.c:
>
> struct scsi_mode_sense_data {
>        struct scsi_mode_header header;
>        struct scsi_blk_desc blk_desc;
>        union scsi_disk_pages pages;
> };
>
> Shall we implement the block descriptor? We can start with the
> following, which fixes NetBSD geometry detection. Shall I post it as a
> patch?

Yes, please. I did not see any difference with NetBSD (2.1, 3.0 or
4.0) Sparc32 guest, though.

> And there is one more problem regarding the disk geometry. The
> "-hdachs" command line switch's sanity check seems to be IDE-specific:
> for instance it doesn't accept "-hdachs 6,64,32". Is there an
> alternative way to specify the SCSI disk geometry?

I haven't tried, but does -drive handle cyls= etc?

> +                if (nb_sectors > UINT32_MAX)
> +                   nb_sectors = UINT32_MAX;

Here the indentation was off by one.

> +                nb_sectors--;
> +                outbuf[3] = 8; /* Block descriptor length.  */
> +                p[0] = 0;

This is density code (add comment?), but it looks like only some weird
optical devices could have nonzero values.

> +                p[1] = (nb_sectors >> 16) & 0xff;
> +                p[2] = (nb_sectors >> 8) & 0xff;
> +                p[3] = nb_sectors & 0xff;
> +                p[4] = 0;  /* reserved */
> +                p[5] = 0; /* bytes 5-7 are the sector size in bytes */
> +                p[6] = s->cluster_size * 2;
> +                p[7] = 0;
> +                p += 8;
> +            }
> +
> +            if (page == 4 ) {

Extra space after 4.