Re: [Qemu-devel] [RFC v3 31/56] ac97: convert to memory API

2011-07-12 Thread Avi Kivity 

On 07/12/2011 01:03 AM, malc wrote:


  Here's a new version:

This one looks acceptable[1], original submission said:
fixes BAR sizing as well. what was wrong with it?


The nabm BAR, for example, was registered as 64 bytes of byte ioports, 
128 bytes of word ioports, and 256 bytes of long ioports.  I expect this 
was an error.


The new patch preserves the error.


[..snip..]

P.S. Sans minor inconsistency with trailing commas.



Where I expect more fields, I leave a trailing comma.  It makes further 
patches nicer.


--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [RFC v3 31/56] ac97: convert to memory API

2011-07-11 Thread Avi Kivity 
 
 Shouldn't it be possible to do something like:
 
 typedef struct OldMemoryRegionOps {
 MemoryRegionOps parent_ops;
 CPUReadMemoryFunc *readfn[3];
 CPUWriteMemoryFunc *writefn[3];
 void *opaque;
 } OldMemoryRegionOps;
 
 That should allow old-style implementations to be converted without
 introducing trampoline functions everywhere.
 

I should, I'll give it a go.

Re: [Qemu-devel] [RFC v3 31/56] ac97: convert to memory API

2011-07-11 Thread Avi Kivity 

On 07/11/2011 04:42 AM, Anthony Liguori wrote:

On 07/10/2011 03:33 PM, malc wrote:

On Sun, 10 Jul 2011, Avi Kivity wrote:


fixes BAR sizing as well.


I find this patch disgusting, the read and write handlers in particular.


Shouldn't it be possible to do something like:

typedef struct OldMemoryRegionOps {
MemoryRegionOps parent_ops;
CPUReadMemoryFunc *readfn[3];
CPUWriteMemoryFunc *writefn[3];
void *opaque;
} OldMemoryRegionOps;

That should allow old-style implementations to be converted without 
introducing trampoline functions everywhere.


Here's a new version:


diff --git a/hw/ac97.c b/hw/ac97.c
index 0b59896..b4f377d 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -160,8 +160,9 @@ typedef struct AC97LinkState {
 SWVoiceIn *voice_mc;
 int invalid_freq[3];
 uint8_t silence[128];
-uint32_t base[2];
 int bup_flag;
+MemoryRegion io_nam;
+MemoryRegion io_nabm;
 } AC97LinkState;

 enum {
@@ -583,7 +584,7 @@ static uint32_t nam_readw (void *opaque, uint32_t addr)
 {
 AC97LinkState *s = opaque;
 uint32_t val = ~0U;
-uint32_t index = addr - s-base[0];
+uint32_t index = addr;
 s-cas = 0;
 val = mixer_load (s, index);
 return val;
@@ -611,7 +612,7 @@ static void nam_writeb (void *opaque, uint32_t addr, 
uint32_t val)

 static void nam_writew (void *opaque, uint32_t addr, uint32_t val)
 {
 AC97LinkState *s = opaque;
-uint32_t index = addr - s-base[0];
+uint32_t index = addr;
 s-cas = 0;
 switch (index) {
 case AC97_Reset:
@@ -714,7 +715,7 @@ static uint32_t nabm_readb (void *opaque, uint32_t addr)
 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 uint32_t val = ~0U;

 switch (index) {
@@ -769,7 +770,7 @@ static uint32_t nabm_readw (void *opaque, uint32_t addr)
 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 uint32_t val = ~0U;

 switch (index) {
@@ -798,7 +799,7 @@ static uint32_t nabm_readl (void *opaque, uint32_t addr)
 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 uint32_t val = ~0U;

 switch (index) {
@@ -848,7 +849,7 @@ static void nabm_writeb (void *opaque, uint32_t 
addr, uint32_t val)

 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 switch (index) {
 case PI_LVI:
 case PO_LVI:
@@ -904,7 +905,7 @@ static void nabm_writew (void *opaque, uint32_t 
addr, uint32_t val)

 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 switch (index) {
 case PI_SR:
 case PO_SR:
@@ -924,7 +925,7 @@ static void nabm_writel (void *opaque, uint32_t 
addr, uint32_t val)

 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 switch (index) {
 case PI_BDBAR:
 case PO_BDBAR:
@@ -1230,31 +1231,33 @@ static const VMStateDescription vmstate_ac97 = {
 }
 };

-static void ac97_map (PCIDevice *pci_dev, int region_num,
-  pcibus_t addr, pcibus_t size, int type)
-{
-AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, pci_dev);
-PCIDevice *d = s-dev;
-
-if (!region_num) {
-s-base[0] = addr;
-register_ioport_read (addr, 256 * 1, 1, nam_readb, d);
-register_ioport_read (addr, 256 * 2, 2, nam_readw, d);
-register_ioport_read (addr, 256 * 4, 4, nam_readl, d);
-register_ioport_write (addr, 256 * 1, 1, nam_writeb, d);
-register_ioport_write (addr, 256 * 2, 2, nam_writew, d);
-register_ioport_write (addr, 256 * 4, 4, nam_writel, d);
-}
-else {
-s-base[1] = addr;
-register_ioport_read (addr, 64 * 1, 1, nabm_readb, d);
-register_ioport_read (addr, 64 * 2, 2, nabm_readw, d);
-register_ioport_read (addr, 64 * 4, 4, nabm_readl, d);
-register_ioport_write (addr, 64 * 1, 1, nabm_writeb, d);
-register_ioport_write (addr, 64 * 2, 2, nabm_writew, d);
-register_ioport_write (addr, 64 * 4, 4, nabm_writel, d);
-}
-}
+static const MemoryRegionPortio nam_portio[] = {
+{ 0, 256 * 1, 1, .read = nam_readb, },
+{ 0, 256 * 2, 2, .read = nam_readw, },
+{ 0, 256 * 4, 4, .read = nam_readl, },
+{ 0, 256 * 1, 1, .write = nam_writeb, },
+{ 0, 256 * 2, 2, .write = nam_writew, },
+{ 0, 256 * 4, 4, .write = nam_writel, },
+PORTIO_END,
+};
+
+static MemoryRegionOps ac97_io_nam_ops = {
+.old_portio = nam_portio,
+};
+
+static const MemoryRegionPortio nabm_portio[] = {
+{ 0, 64 * 1, 1, .read = nabm_readb, },
+{ 0, 64 * 2, 2, .read = nabm_readw, },
+{ 0, 64 * 4, 4, .read = nabm_readl, },
+{ 0, 64 * 1, 1,

Re: [Qemu-devel] [RFC v3 31/56] ac97: convert to memory API

2011-07-11 Thread malc 
On Mon, 11 Jul 2011, Avi Kivity wrote:

 On 07/11/2011 04:42 AM, Anthony Liguori wrote:
  On 07/10/2011 03:33 PM, malc wrote:
   On Sun, 10 Jul 2011, Avi Kivity wrote:
   
fixes BAR sizing as well.
   
   I find this patch disgusting, the read and write handlers in particular.
  
  Shouldn't it be possible to do something like:
  
  typedef struct OldMemoryRegionOps {
  MemoryRegionOps parent_ops;
  CPUReadMemoryFunc *readfn[3];
  CPUWriteMemoryFunc *writefn[3];
  void *opaque;
  } OldMemoryRegionOps;
  
  That should allow old-style implementations to be converted without
  introducing trampoline functions everywhere.
 
 Here's a new version:

This one looks acceptable[1], original submission said:
fixes BAR sizing as well. what was wrong with it?

[..snip..] 

P.S. Sans minor inconsistency with trailing commas.

-- 
mailto:av1...@comtv.ru

[Qemu-devel] [RFC v3 31/56] ac97: convert to memory API

2011-07-10 Thread Avi Kivity 
fixes BAR sizing as well.

Signed-off-by: Avi Kivity a...@redhat.com
---
 hw/ac97.c |  126 ++--
 1 files changed, 88 insertions(+), 38 deletions(-)

diff --git a/hw/ac97.c b/hw/ac97.c
index 0b59896..72a0667 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -160,8 +160,9 @@ typedef struct AC97LinkState {
 SWVoiceIn *voice_mc;
 int invalid_freq[3];
 uint8_t silence[128];
-uint32_t base[2];
 int bup_flag;
+MemoryRegion io_nam;
+MemoryRegion io_nabm;
 } AC97LinkState;
 
 enum {
@@ -583,7 +584,7 @@ static uint32_t nam_readw (void *opaque, uint32_t addr)
 {
 AC97LinkState *s = opaque;
 uint32_t val = ~0U;
-uint32_t index = addr - s-base[0];
+uint32_t index = addr;
 s-cas = 0;
 val = mixer_load (s, index);
 return val;
@@ -611,7 +612,7 @@ static void nam_writeb (void *opaque, uint32_t addr, 
uint32_t val)
 static void nam_writew (void *opaque, uint32_t addr, uint32_t val)
 {
 AC97LinkState *s = opaque;
-uint32_t index = addr - s-base[0];
+uint32_t index = addr;
 s-cas = 0;
 switch (index) {
 case AC97_Reset:
@@ -706,6 +707,37 @@ static void nam_writel (void *opaque, uint32_t addr, 
uint32_t val)
 s-cas = 0;
 }
 
+static uint64_t nam_read(void *opaque, target_phys_addr_t addr, unsigned size)
+{
+AC97LinkState *s = opaque;
+
+switch (size) {
+case 1: return nam_readb(s, addr);
+case 2: return nam_readw(s, addr);
+case 4: return nam_readl(s, addr);
+default: abort();
+}
+}
+
+static void nam_write(void *opaque, target_phys_addr_t addr,
+  uint64_t data, unsigned size)
+{
+AC97LinkState *s = opaque;
+
+switch (size) {
+case 1: return nam_writeb(s, addr, data);
+case 2: return nam_writew(s, addr, data);
+case 4: return nam_writel(s, addr, data);
+default: abort();
+}
+}
+
+static MemoryRegionOps ac97_io_nam_ops = {
+.read = nam_read,
+.write = nam_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
 /**
  * Native audio bus master
  * I/O Reads
@@ -714,7 +746,7 @@ static uint32_t nabm_readb (void *opaque, uint32_t addr)
 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 uint32_t val = ~0U;
 
 switch (index) {
@@ -769,7 +801,7 @@ static uint32_t nabm_readw (void *opaque, uint32_t addr)
 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 uint32_t val = ~0U;
 
 switch (index) {
@@ -798,7 +830,7 @@ static uint32_t nabm_readl (void *opaque, uint32_t addr)
 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 uint32_t val = ~0U;
 
 switch (index) {
@@ -848,7 +880,7 @@ static void nabm_writeb (void *opaque, uint32_t addr, 
uint32_t val)
 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 switch (index) {
 case PI_LVI:
 case PO_LVI:
@@ -904,7 +936,7 @@ static void nabm_writew (void *opaque, uint32_t addr, 
uint32_t val)
 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 switch (index) {
 case PI_SR:
 case PO_SR:
@@ -924,7 +956,7 @@ static void nabm_writel (void *opaque, uint32_t addr, 
uint32_t val)
 {
 AC97LinkState *s = opaque;
 AC97BusMasterRegs *r = NULL;
-uint32_t index = addr - s-base[1];
+uint32_t index = addr;
 switch (index) {
 case PI_BDBAR:
 case PO_BDBAR:
@@ -954,6 +986,38 @@ static void nabm_writel (void *opaque, uint32_t addr, 
uint32_t val)
 }
 }
 
+static uint64_t nabm_read(void *opaque, target_phys_addr_t addr,
+ unsigned size)
+{
+AC97LinkState *s = opaque;
+
+switch (size) {
+case 1: return nabm_readb(s, addr);
+case 2: return nabm_readw(s, addr);
+case 4: return nabm_readl(s, addr);
+default: abort();
+}
+}
+
+static void nabm_write(void *opaque, target_phys_addr_t addr,
+   uint64_t data, unsigned size)
+{
+AC97LinkState *s = opaque;
+
+switch (size) {
+case 1: return nabm_writeb(s, addr, data);
+case 2: return nabm_writew(s, addr, data);
+case 4: return nabm_writel(s, addr, data);
+default: abort();
+}
+}
+
+static MemoryRegionOps ac97_io_nabm_ops = {
+.read = nabm_read,
+.write = nabm_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
 static int write_audio (AC97LinkState *s, AC97BusMasterRegs *r,
 int max, int *stop)
 {
@@ -1230,32 +1294,6 @@ static const VMStateDescription vmstate_ac97 = {
 }
 };
 
-static void ac97_map (PCIDevice *pci_dev, int region_num,
-  pcibus_t addr, pcibus_t size, int type)
-{
-AC97LinkState *s =

Re: [Qemu-devel] [RFC v3 31/56] ac97: convert to memory API

2011-07-10 Thread malc 
On Sun, 10 Jul 2011, Avi Kivity wrote:

 fixes BAR sizing as well.

I find this patch disgusting, the read and write handlers in particular.

[..snip..]

-- 
mailto:av1...@comtv.ru

Re: [Qemu-devel] [RFC v3 31/56] ac97: convert to memory API

2011-07-10 Thread Anthony Liguori 

On 07/10/2011 03:33 PM, malc wrote:

On Sun, 10 Jul 2011, Avi Kivity wrote:


fixes BAR sizing as well.


I find this patch disgusting, the read and write handlers in particular.


Shouldn't it be possible to do something like:

typedef struct OldMemoryRegionOps {
MemoryRegionOps parent_ops;
CPUReadMemoryFunc *readfn[3];
CPUWriteMemoryFunc *writefn[3];
void *opaque;
} OldMemoryRegionOps;

That should allow old-style implementations to be converted without 
introducing trampoline functions everywhere.


Regards,

Anthony Liguori



[..snip..]