Re: [Qemu-devel] [PATCH 09/15] hw/ide: Emulate SiI3112 SATA controller

[David Gibson]

On Tue, Aug 22, 2017 at 03:01:18PM -0400, John Snow wrote:
> 
> 
> On 08/22/2017 07:08 AM, BALATON Zoltan wrote:
> > Hello,
> > 
> > Thanks for the review.
> > 
> > On Mon, 21 Aug 2017, John Snow wrote:
> >> On 08/20/2017 01:23 PM, BALATON Zoltan wrote:
> >>> This is a common generic PCI SATA conroller that is also used in PCs
> >>> but more importantly guests running on the Sam460ex board prefer this
> >>> card and have a driver for it (unlike for other SATA controllers
> >>> already emulated).
> >>
> >> Oh, interesting. This is basically an alternative to the PCI BMDMA
> >> interface and not really an alternative to AHCI. It doesn't really seem
> > 
> > Yes, this is a simple PCI SATA interface similar to IDE and not like
> > AHCI. I think it's an updated version of the CMD640 IDE interface or
> > more closely related to that. The Linux driver references CMD680 as well
> > so it's probably a SATA version of that chip.
> > 
> >> to use any of the SATA-specific interfaces to SATA drives (cough, not
> >> that we currently emulate a difference in QEMU... ATA and SATA both are
> >> simply IDEState*) so this really seems like another PCI IDE interface
> >> akin to the PCI BMDMA adapter we already have.
> >>
> >> It's just that guests will think they're using SATA, except... not. Not
> >> a big deal, *I think*...
> >>
> >> ...It isn't a problem that our support for NCQ commands is tied to AHCI,
> >> is it? Some of our current "SATA" support is tied very directly to AHCI
> >> (see is_ncq() in ahci.c) -- is that relevant here?
> > 
> > I don't think any of the clients on Sam460ex tries to use NCQ so maybe
> > it's OK. Linux might have a better driver but I'm not sure. This could
> > be fixed later. Although I've seen a hang in Linux which I haven't
> > debugged yet and don't know what causes it or if it's related to SATA at
> > all.
> > 
> > I'm not sure I've implemented everything correctly but at least the
> > firmware of the board can find disks and load files to boot OSes so
> > basic functions should be working. There are some other bugs elsewhere
> > which prevent me from testing more OSes at the moment. One of them is
> > QEMU crashing sometimes while accessing this SATA controller but it's
> > triggered from TCG generated code and depends on some timing because it
> > goes away if I change code running in the client or add some debug logs.
> > This is described here:
> > 
> 
> "Elsewhere" as in "Not seemingly related to this controller," it seems.
> 
> > http://lists.nongnu.org/archive/html/qemu-ppc/2017-08/msg00249.html
> > 
> > You don't happen to have an idea or seen similar before, do you? (Likely
> > it's not related to IDE but more likely some io memory region
> > interaction with TCG.)
> > 
> 
> Sorry, the traces look foreign to me.
> 
> > I intend to improve this device model later when found necessary but I
> > don't have much free time for it now so I'd like to submit it now to get
> > some more testing and maybe contributions from others.
> > 
> 
> Sure, but be advised that if the device causes problems outside of this
> use case and there's nobody willing or able to review it, that it may
> get removed again.
> 
> I don't have a lot of free time to go through the register list point by
> point and make sure this is implemented correctly either, but if this
> helps your work I'm OK not holding it up.

That's been pretty much my view on most of this.  I have neither the
at-hand knowledge of the hardware, nor time to look it up to give a
detailed review.  But if it's replacing a complete lack of support and
there's nothing obviously bogus, seems reasonable to let it in.

Jon, do you want me to queue this driver in my tree (for 2.11,
obviously), or do you want to take it through another tree?

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature