| Message ID | 1314436348-28837-1-git-send-email-daniel@drv.nu |
|---|---|
| State | New |
| Headers | show |
On 27.08.2011, at 04:12, Daniel Verkamp wrote: > Implement an I/O space index-data register pair as defined by the AHCI > spec, including the corresponding SATA PCI capability and BAR. > > This allows real-mode code to access the AHCI registers; real-mode > code cannot address the memory-mapped register space because it is > beyond the first megabyte. Very nice patch! I'll check and compare with a real ICH-9 when I get back to .de, but I'd assume you also did that already ;). Once I checked that the IO region is set up similarly, I'll give you my ack. Alex
On Sun, Aug 28, 2011 at 11:48 AM, Alexander Graf <agraf@suse.de> wrote: > > On 27.08.2011, at 04:12, Daniel Verkamp wrote: > >> Implement an I/O space index-data register pair as defined by the AHCI >> spec, including the corresponding SATA PCI capability and BAR. >> >> This allows real-mode code to access the AHCI registers; real-mode >> code cannot address the memory-mapped register space because it is >> beyond the first megabyte. > > Very nice patch! I'll check and compare with a real ICH-9 when I get > back to .de, but I'd assume you also did that already ;). Once I checked > that the IO region is set up similarly, I'll give you my ack. Please do double check against real hardware if you get the chance - I don't have a real ICH-9 handy to test against. This is all written based on my reading of the spec and testing with an internal DOS developer tool from work. I am mainly curious how the real thing handles writes to the index register that aren't divisible by 4 or are beyond the end of the register set (and how big that really is on ICH-9). Judging by the bits marked "RO" in the spec, I would guess writing 0x13 to the index and then reading it back should give 0x10, but I haven't tested it on real hw. Thanks, -- Daniel
On 08/30/2011 05:07 AM, Daniel Verkamp wrote: > On Sun, Aug 28, 2011 at 11:48 AM, Alexander Graf<agraf@suse.de> wrote: >> On 27.08.2011, at 04:12, Daniel Verkamp wrote: >> >>> Implement an I/O space index-data register pair as defined by the AHCI >>> spec, including the corresponding SATA PCI capability and BAR. >>> >>> This allows real-mode code to access the AHCI registers; real-mode >>> code cannot address the memory-mapped register space because it is >>> beyond the first megabyte. >> Very nice patch! I'll check and compare with a real ICH-9 when I get >> back to .de, but I'd assume you also did that already ;). Once I checked >> that the IO region is set up similarly, I'll give you my ack. > Please do double check against real hardware if you get the chance - I > don't have a real ICH-9 handy to test against. This is all written > based on my reading of the spec and testing with an internal DOS > developer tool from work. > > I am mainly curious how the real thing handles writes to the index > register that aren't divisible by 4 or are beyond the end of the > register set (and how big that really is on ICH-9). Judging by the > bits marked "RO" in the spec, I would guess writing 0x13 to the index > and then reading it back should give 0x10, but I haven't tested it on > real hw. Phew. So I finally got at least an ICH-9 system booting. This is what lspci -vvv tells me: 00:1f.2 SATA controller: Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port SATA AHCI Controller (rev 02) (prog-if 01 [AHCI 1.0]) Subsystem: Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port SATA AHCI Controller Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Interrupt: pin B routed to IRQ 26 Region 0: I/O ports at d000 [size=8] Region 1: I/O ports at cc00 [size=4] Region 2: I/O ports at c880 [size=8] Region 3: I/O ports at c800 [size=4] Region 4: I/O ports at c480 [size=32] Region 5: Memory at ffaf9000 (32-bit, non-prefetchable) [size=2K] Capabilities: [80] MSI: Enable+ Count=1/16 Maskable- 64bit- Address: fee0f00c Data: 4169 Capabilities: [70] Power Management version 3 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot+,D3cold-) Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME- Capabilities: [a8] SATA HBA v1.0 BAR4 Offset=00000004 Capabilities: [b0] Vendor Specific Information: Len=06 <?> Kernel driver in use: ahci So BAR4 is where the IDP info should be. Offset is 4 into that IO space and the space is 32 bytes long. Do you have the ICH-9 implementation spec? I can try to dig something up if you don't have it around. Please send me a small test program I can run on the machine to find out what happens for unaligned I/O accesses. That would be very helpful! Thanks, Alex
(Sorry for the slow response, was on vacation) On Thu, Sep 1, 2011 at 7:58 AM, Alexander Graf <agraf@suse.de> wrote: > On 08/30/2011 05:07 AM, Daniel Verkamp wrote: >> >> On Sun, Aug 28, 2011 at 11:48 AM, Alexander Graf<agraf@suse.de> wrote: >>> >>> On 27.08.2011, at 04:12, Daniel Verkamp wrote: >>> >>>> Implement an I/O space index-data register pair as defined by the AHCI >>>> spec, including the corresponding SATA PCI capability and BAR. >>>> >>>> This allows real-mode code to access the AHCI registers; real-mode >>>> code cannot address the memory-mapped register space because it is >>>> beyond the first megabyte. >>> >>> Very nice patch! I'll check and compare with a real ICH-9 when I get >>> back to .de, but I'd assume you also did that already ;). Once I checked >>> that the IO region is set up similarly, I'll give you my ack. >> >> Please do double check against real hardware if you get the chance - I >> don't have a real ICH-9 handy to test against. This is all written >> based on my reading of the spec and testing with an internal DOS >> developer tool from work. >> >> I am mainly curious how the real thing handles writes to the index >> register that aren't divisible by 4 or are beyond the end of the >> register set (and how big that really is on ICH-9). Judging by the >> bits marked "RO" in the spec, I would guess writing 0x13 to the index >> and then reading it back should give 0x10, but I haven't tested it on >> real hw. > > Phew. So I finally got at least an ICH-9 system booting. This is what lspci > -vvv tells me: > > 00:1f.2 SATA controller: Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 > port SATA AHCI Controller (rev 02) (prog-if 01 [AHCI 1.0]) > Subsystem: Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port SATA AHCI > Controller > Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- > Stepping- SERR- FastB2B- DisINTx+ > Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- > <MAbort- >SERR- <PERR- INTx- > Latency: 0 > Interrupt: pin B routed to IRQ 26 > Region 0: I/O ports at d000 [size=8] > Region 1: I/O ports at cc00 [size=4] > Region 2: I/O ports at c880 [size=8] > Region 3: I/O ports at c800 [size=4] > Region 4: I/O ports at c480 [size=32] > Region 5: Memory at ffaf9000 (32-bit, non-prefetchable) [size=2K] > Capabilities: [80] MSI: Enable+ Count=1/16 Maskable- 64bit- > Address: fee0f00c Data: 4169 > Capabilities: [70] Power Management version 3 > Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA > PME(D0-,D1-,D2-,D3hot+,D3cold-) > Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME- > Capabilities: [a8] SATA HBA v1.0 BAR4 Offset=00000004 > Capabilities: [b0] Vendor Specific Information: Len=06 <?> > Kernel driver in use: ahci > > So BAR4 is where the IDP info should be. Offset is 4 into that IO space and > the space is 32 bytes long. Do you have the ICH-9 implementation spec? I can > try to dig something up if you don't have it around. > I'm not sure I understand what you mean, but I think everything is in the right spot - compare with the real ICH-9 dump you provide (relevant parts quoted below; full lspci dump from QEMU device at end): Real: Region 4: I/O ports at c480 [size=32] QEMU: Region 4: I/O ports at c040 [size=32] (I/O address is different, but that is ok) Real: Capabilities: [a8] SATA HBA v1.0 BAR4 Offset=00000004 QEMU: Capabilities: [a8] SATA HBA v1.0 BAR4 Offset=00000004 (identical) > Please send me a small test program I can run on the machine to find out > what happens for unaligned I/O accesses. That would be very helpful! > I will try to put something together in the next few days and send it along; is a DOS test app suitable? Thanks, -- Daniel Verkamp Here is the lspci -vvv -nn -x dump of the QEMU-emulated AHCI controller with the patch applied: 00:04.0 SATA controller [0106]: Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port SATA AHCI Controller [8086:2922] (rev 02) (prog-if 01 [AHCI 1.0]) Subsystem: Red Hat, Inc Device [1af4:1100] Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Interrupt: pin A routed to IRQ 24 Region 4: I/O ports at c040 [size=32] Region 5: Memory at febf1000 (32-bit, non-prefetchable) [size=4K] Capabilities: [a8] SATA HBA v1.0 BAR4 Offset=00000004 Capabilities: [50] MSI: Enable+ Count=1/1 Maskable- 64bit+ Address: 00000000fee0100c Data: 4149 Kernel driver in use: ahci 00: 86 80 22 29 07 04 10 00 02 01 06 01 00 00 00 00 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20: 41 c0 00 00 00 10 bf fe 00 00 00 00 f4 1a 00 11 30: 00 00 00 00 a8 00 00 00 00 00 00 00 0b 01 00 00 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 50: 05 00 81 00 0c 10 e0 fe 00 00 00 00 49 41 00 00 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 90: 40 00 3f 00 00 00 00 00 00 00 00 00 00 00 00 00 a0: 00 00 00 00 00 00 00 00 12 50 10 00 48 00 00 00 b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Am 28.08.2011 20:48, schrieb Alexander Graf: > > On 27.08.2011, at 04:12, Daniel Verkamp wrote: > >> Implement an I/O space index-data register pair as defined by the AHCI >> spec, including the corresponding SATA PCI capability and BAR. >> >> This allows real-mode code to access the AHCI registers; real-mode >> code cannot address the memory-mapped register space because it is >> beyond the first megabyte. > > Very nice patch! I'll check and compare with a real ICH-9 when I get back to .de, but I'd assume you also did that already ;). Once I checked that the IO region is set up similarly, I'll give you my ack. What's the status with review/testing of this patch, Alex? Is it ready to be merged, should I drop it or do you just need some more time? Kevin
On 09/20/2011 03:39 PM, Kevin Wolf wrote: > Am 28.08.2011 20:48, schrieb Alexander Graf: >> On 27.08.2011, at 04:12, Daniel Verkamp wrote: >> >>> Implement an I/O space index-data register pair as defined by the AHCI >>> spec, including the corresponding SATA PCI capability and BAR. >>> >>> This allows real-mode code to access the AHCI registers; real-mode >>> code cannot address the memory-mapped register space because it is >>> beyond the first megabyte. >> Very nice patch! I'll check and compare with a real ICH-9 when I get back to .de, but I'd assume you also did that already ;). Once I checked that the IO region is set up similarly, I'll give you my ack. > What's the status with review/testing of this patch, Alex? Is it ready > to be merged, should I drop it or do you just need some more time? If you don't see regressions with it, I'd say we're good. I still don't have a validation program, but considering that it's a reasonably simple change and makes us more conforming to real hardware, I don't see why we shouldn't have it. Alex
Am 27.08.2011 11:12, schrieb Daniel Verkamp: > Implement an I/O space index-data register pair as defined by the AHCI > spec, including the corresponding SATA PCI capability and BAR. > > This allows real-mode code to access the AHCI registers; real-mode > code cannot address the memory-mapped register space because it is > beyond the first megabyte. > > Signed-off-by: Daniel Verkamp <daniel@drv.nu> Thanks, applied to the block branch. > --- a/hw/ide/ich.c > +++ b/hw/ide/ich.c > @@ -72,6 +72,14 @@ > #include <hw/ide/pci.h> > #include <hw/ide/ahci.h> > > +#define ICH9_SATA_CAP_OFFSET 0xA8 > + > +#define ICH9_IDP_BAR 4 > +#define ICH9_MEM_BAR 5 > + > +#define ICH9_IDP_INDEX 0x10 > +#define ICH9_IDP_INDEX_LOG2 0x04 Just wondering, why did you choose 0x10 and not 0? The spec reads as if the implementation could freely choose this, and I can't see what the first 16 Bytes are used for. Doesn't make it less correct, of course. Kevin
Am 21.09.2011 14:34, schrieb Kevin Wolf: > Am 27.08.2011 11:12, schrieb Daniel Verkamp: >> Implement an I/O space index-data register pair as defined by the AHCI >> spec, including the corresponding SATA PCI capability and BAR. >> >> This allows real-mode code to access the AHCI registers; real-mode >> code cannot address the memory-mapped register space because it is >> beyond the first megabyte. >> >> Signed-off-by: Daniel Verkamp <daniel@drv.nu> > > Thanks, applied to the block branch. > >> --- a/hw/ide/ich.c >> +++ b/hw/ide/ich.c >> @@ -72,6 +72,14 @@ >> #include <hw/ide/pci.h> >> #include <hw/ide/ahci.h> >> >> +#define ICH9_SATA_CAP_OFFSET 0xA8 >> + >> +#define ICH9_IDP_BAR 4 >> +#define ICH9_MEM_BAR 5 >> + >> +#define ICH9_IDP_INDEX 0x10 >> +#define ICH9_IDP_INDEX_LOG2 0x04 > > Just wondering, why did you choose 0x10 and not 0? The spec reads as if > the implementation could freely choose this, and I can't see what the > first 16 Bytes are used for. I forgot that we're not implementing a generic AHCI controller here but an ICH9 one, which does define the offsets used. Now I'm convinced. :-) Kevin
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 29521ba..431e58e 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -370,6 +370,43 @@ static MemoryRegionOps ahci_mem_ops = { .endianness = DEVICE_LITTLE_ENDIAN, }; +static uint64_t ahci_idp_read(void *opaque, target_phys_addr_t addr, + unsigned size) +{ + AHCIState *s = opaque; + + if (addr == s->idp_offset) { + /* index register */ + return s->idp_index; + } else if (addr == s->idp_offset + 4) { + /* data register - do memory read at location selected by index */ + return ahci_mem_read(opaque, s->idp_index, size); + } else { + return 0; + } +} + +static void ahci_idp_write(void *opaque, target_phys_addr_t addr, + uint64_t val, unsigned size) +{ + AHCIState *s = opaque; + + if (addr == s->idp_offset) { + /* index register - mask off reserved bits */ + s->idp_index = (uint32_t)val & ((AHCI_MEM_BAR_SIZE - 1) & ~3); + } else if (addr == s->idp_offset + 4) { + /* data register - do memory write at location selected by index */ + ahci_mem_write(opaque, s->idp_index, val, size); + } +} + +static MemoryRegionOps ahci_idp_ops = { + .read = ahci_idp_read, + .write = ahci_idp_write, + .endianness = DEVICE_LITTLE_ENDIAN, +}; + + static void ahci_reg_init(AHCIState *s) { int i; @@ -1126,7 +1163,9 @@ void ahci_init(AHCIState *s, DeviceState *qdev, int ports) s->dev = g_malloc0(sizeof(AHCIDevice) * ports); ahci_reg_init(s); /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */ - memory_region_init_io(&s->mem, &ahci_mem_ops, s, "ahci", 0x1000); + memory_region_init_io(&s->mem, &ahci_mem_ops, s, "ahci", AHCI_MEM_BAR_SIZE); + memory_region_init_io(&s->idp, &ahci_idp_ops, s, "ahci-idp", 32); + irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports); for (i = 0; i < s->ports; i++) { @@ -1146,6 +1185,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev, int ports) void ahci_uninit(AHCIState *s) { memory_region_destroy(&s->mem); + memory_region_destroy(&s->idp); g_free(s->dev); } diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h index e456193..a2a9400 100644 --- a/hw/ide/ahci.h +++ b/hw/ide/ahci.h @@ -24,7 +24,7 @@ #ifndef HW_IDE_AHCI_H #define HW_IDE_AHCI_H -#define AHCI_PCI_BAR 5 +#define AHCI_MEM_BAR_SIZE 0x1000 #define AHCI_MAX_PORTS 32 #define AHCI_MAX_SG 168 /* hardware max is 64K */ #define AHCI_DMA_BOUNDARY 0xffffffff @@ -212,6 +212,10 @@ #define RES_FIS_SDBFIS 0x58 #define RES_FIS_UFIS 0x60 +#define SATA_CAP_SIZE 0x8 +#define SATA_CAP_REV 0x2 +#define SATA_CAP_BAR 0x4 + typedef struct AHCIControlRegs { uint32_t cap; uint32_t ghc; @@ -290,6 +294,9 @@ typedef struct AHCIState { AHCIDevice *dev; AHCIControlRegs control_regs; MemoryRegion mem; + MemoryRegion idp; /* Index-Data Pair I/O port space */ + unsigned idp_offset; /* Offset of index in I/O port space */ + uint32_t idp_index; /* Current IDP index */ int ports; qemu_irq irq; } AHCIState; diff --git a/hw/ide/ich.c b/hw/ide/ich.c index 5278bc4..4ff68a8 100644 --- a/hw/ide/ich.c +++ b/hw/ide/ich.c @@ -72,6 +72,14 @@ #include <hw/ide/pci.h> #include <hw/ide/ahci.h> +#define ICH9_SATA_CAP_OFFSET 0xA8 + +#define ICH9_IDP_BAR 4 +#define ICH9_MEM_BAR 5 + +#define ICH9_IDP_INDEX 0x10 +#define ICH9_IDP_INDEX_LOG2 0x04 + static const VMStateDescription vmstate_ahci = { .name = "ahci", .unmigratable = 1, @@ -80,6 +88,8 @@ static const VMStateDescription vmstate_ahci = { static int pci_ich9_ahci_init(PCIDevice *dev) { struct AHCIPCIState *d; + int sata_cap_offset; + uint8_t *sata_cap; d = DO_UPCAST(struct AHCIPCIState, card, dev); ahci_init(&d->ahci, &dev->qdev, 6); @@ -98,7 +108,22 @@ static int pci_ich9_ahci_init(PCIDevice *dev) msi_init(dev, 0x50, 1, true, false); d->ahci.irq = d->card.irq[0]; - pci_register_bar(&d->card, 5, 0, &d->ahci.mem); + pci_register_bar(&d->card, ICH9_IDP_BAR, PCI_BASE_ADDRESS_SPACE_IO, + &d->ahci.idp); + pci_register_bar(&d->card, ICH9_MEM_BAR, PCI_BASE_ADDRESS_SPACE_MEMORY, + &d->ahci.mem); + + sata_cap_offset = pci_add_capability(&d->card, PCI_CAP_ID_SATA, + ICH9_SATA_CAP_OFFSET, SATA_CAP_SIZE); + if (sata_cap_offset < 0) { + return sata_cap_offset; + } + + sata_cap = d->card.config + sata_cap_offset; + pci_set_word(sata_cap + SATA_CAP_REV, 0x10); + pci_set_long(sata_cap + SATA_CAP_BAR, + (ICH9_IDP_BAR + 0x4) | (ICH9_IDP_INDEX_LOG2 << 4)); + d->ahci.idp_offset = ICH9_IDP_INDEX; return 0; } diff --git a/hw/pci_regs.h b/hw/pci_regs.h index e884096..e8357c3 100644 --- a/hw/pci_regs.h +++ b/hw/pci_regs.h @@ -211,6 +211,7 @@ #define PCI_CAP_ID_AGP3 0x0E /* AGP Target PCI-PCI bridge */ #define PCI_CAP_ID_EXP 0x10 /* PCI Express */ #define PCI_CAP_ID_MSIX 0x11 /* MSI-X */ +#define PCI_CAP_ID_SATA 0x12 /* Serial ATA */ #define PCI_CAP_ID_AF 0x13 /* PCI Advanced Features */ #define PCI_CAP_LIST_NEXT 1 /* Next capability in the list */ #define PCI_CAP_FLAGS 2 /* Capability defined flags (16 bits) */
Implement an I/O space index-data register pair as defined by the AHCI spec, including the corresponding SATA PCI capability and BAR. This allows real-mode code to access the AHCI registers; real-mode code cannot address the memory-mapped register space because it is beyond the first megabyte. Signed-off-by: Daniel Verkamp <daniel@drv.nu> --- hw/ide/ahci.c | 42 +++++++++++++++++++++++++++++++++++++++++- hw/ide/ahci.h | 9 ++++++++- hw/ide/ich.c | 27 ++++++++++++++++++++++++++- hw/pci_regs.h | 1 + 4 files changed, 76 insertions(+), 3 deletions(-)