Re: [Qemu-devel] [PATCH 7/9] AHCI: Rework IRQ constants

[Philippe Mathieu-Daudé]

On 08/08/2017 03:33 PM, John Snow wrote:

Create a new enum so that we can name the IRQ bits, which will make
debugging
them a little nicer if we can print them out. Not handled in this
patch, but
this will make it possible to get a nice debug printf detailing
exactly which
status bits are set, as it can be multiple at any given time.

As a consequence of this patch, it is no longer possible to set
multiple IRQ
codes at once, but nothing was utilizing this ability anyway.

Signed-off-by: John Snow 
---
   hw/ide/ahci.c  | 49
++---
   hw/ide/ahci_internal.h | 44
+++-
   hw/ide/trace-events|  2 +-
   3 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d5acceb..c5a9b69 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -56,6 +56,27 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
   static void ahci_unmap_clb_address(AHCIDevice *ad);
   static void ahci_unmap_fis_address(AHCIDevice *ad);
   +static const char *AHCIPortIRQ_lookup[AHCI_PORT_IRQ__END] = {
+[AHCI_PORT_IRQ_BIT_DHRS] = "DHRS",
+[AHCI_PORT_IRQ_BIT_PSS]  = "PSS",
+[AHCI_PORT_IRQ_BIT_DSS]  = "DSS",
+[AHCI_PORT_IRQ_BIT_SDBS] = "SDBS",
+[AHCI_PORT_IRQ_BIT_UFS]  = "UFS",
+[AHCI_PORT_IRQ_BIT_DPS]  = "DPS",
+[AHCI_PORT_IRQ_BIT_PCS]  = "PCS",
+[AHCI_PORT_IRQ_BIT_DMPS] = "DMPS",
+[8 ... 21]   = "RESERVED",
+[AHCI_PORT_IRQ_BIT_PRCS] = "PRCS",
+[AHCI_PORT_IRQ_BIT_IPMS] = "IPMS",
+[AHCI_PORT_IRQ_BIT_OFS]  = "OFS",
+[25] = "RESERVED",
+[AHCI_PORT_IRQ_BIT_INFS] = "INFS",
+[AHCI_PORT_IRQ_BIT_IFS]  = "IFS",
+[AHCI_PORT_IRQ_BIT_HBDS] = "HBDS",
+[AHCI_PORT_IRQ_BIT_HBFS] = "HBFS",
+[AHCI_PORT_IRQ_BIT_TFES] = "TFES",
+[AHCI_PORT_IRQ_BIT_CPDS] = "CPDS"
+};
 static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
   {
@@ -170,12 +191,18 @@ static void ahci_check_irq(AHCIState *s)
   }
 static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
- int irq_type)
+ enum AHCIPortIRQ irqbit)
   {
-DPRINTF(d->port_no, "trigger irq %#x -> %x\n",
-irq_type, d->port_regs.irq_mask & irq_type);
+g_assert(irqbit >= 0 && irqbit < 32);


Since you now use an enum this check is no more necessary.



You can actually still pass in a wrong value if you wanted to. This is
to prevent old-style IRQ masks from getting passed in by accident.


No accident :) This is now an enum, also:

hw/ide$ git grep ahci_trigger_irq
ahci.c:764:ahci_trigger_irq(s, ad, AHCI_PORT_IRQ_BIT_SDBS);
ahci.c:807:ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
ahci.c:810:ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_PSS);
ahci.c:850:ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
ahci.c:853:ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
ahci.c:1128:ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_OFS);
ahci.c:1276:ahci_trigger_irq(s, >dev[port], 
AHCI_PORT_IRQ_BIT_HBFS);


You only use enum values, not any cast.




However ...


+uint32_t irq = 1U << irqbit;
+uint32_t irqstat = d->port_regs.irq_stat | irq;
   -d->port_regs.irq_stat |= irq_type;
+trace_ahci_trigger_irq(s, d->port_no,
+   AHCIPortIRQ_lookup[irqbit], irq,
+   d->port_regs.irq_stat, irqstat,
+   irqstat & d->port_regs.irq_mask);
+


Why not keep using masked irqs, and iterate over AHCI_PORT_IRQ__END
bits? Something like:

 if (TRACE_AHCI_IRQ_ENABLED) {
 int irqbit;
 for (irqbit = 0; irqbit < AHCI_PORT_IRQ__END; irqbit++) {
 if (irqmask & BIT(irqbit)) {
 trace_ahci_trigger_irq(s, d->port_no, ...



Would rather not iterate like that for the IRQ path. Generally no place
in the codebase needs to raise more than one IRQ type at a time, so why
bother allowing it?


Indeed.

Re: [Qemu-devel] [PATCH 7/9] AHCI: Rework IRQ constants

[John Snow]

On 08/25/2017 10:00 AM, Philippe Mathieu-Daudé wrote:
> Hi John,
> 
> On 08/08/2017 03:33 PM, John Snow wrote:
>> Create a new enum so that we can name the IRQ bits, which will make
>> debugging
>> them a little nicer if we can print them out. Not handled in this
>> patch, but
>> this will make it possible to get a nice debug printf detailing
>> exactly which
>> status bits are set, as it can be multiple at any given time.
>>
>> As a consequence of this patch, it is no longer possible to set
>> multiple IRQ
>> codes at once, but nothing was utilizing this ability anyway.
>>
>> Signed-off-by: John Snow 
>> ---
>>   hw/ide/ahci.c  | 49
>> ++---
>>   hw/ide/ahci_internal.h | 44
>> +++-
>>   hw/ide/trace-events|  2 +-
>>   3 files changed, 74 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index d5acceb..c5a9b69 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -56,6 +56,27 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
>>   static void ahci_unmap_clb_address(AHCIDevice *ad);
>>   static void ahci_unmap_fis_address(AHCIDevice *ad);
>>   +static const char *AHCIPortIRQ_lookup[AHCI_PORT_IRQ__END] = {
>> +[AHCI_PORT_IRQ_BIT_DHRS] = "DHRS",
>> +[AHCI_PORT_IRQ_BIT_PSS]  = "PSS",
>> +[AHCI_PORT_IRQ_BIT_DSS]  = "DSS",
>> +[AHCI_PORT_IRQ_BIT_SDBS] = "SDBS",
>> +[AHCI_PORT_IRQ_BIT_UFS]  = "UFS",
>> +[AHCI_PORT_IRQ_BIT_DPS]  = "DPS",
>> +[AHCI_PORT_IRQ_BIT_PCS]  = "PCS",
>> +[AHCI_PORT_IRQ_BIT_DMPS] = "DMPS",
>> +[8 ... 21]   = "RESERVED",
>> +[AHCI_PORT_IRQ_BIT_PRCS] = "PRCS",
>> +[AHCI_PORT_IRQ_BIT_IPMS] = "IPMS",
>> +[AHCI_PORT_IRQ_BIT_OFS]  = "OFS",
>> +[25] = "RESERVED",
>> +[AHCI_PORT_IRQ_BIT_INFS] = "INFS",
>> +[AHCI_PORT_IRQ_BIT_IFS]  = "IFS",
>> +[AHCI_PORT_IRQ_BIT_HBDS] = "HBDS",
>> +[AHCI_PORT_IRQ_BIT_HBFS] = "HBFS",
>> +[AHCI_PORT_IRQ_BIT_TFES] = "TFES",
>> +[AHCI_PORT_IRQ_BIT_CPDS] = "CPDS"
>> +};
>> static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
>>   {
>> @@ -170,12 +191,18 @@ static void ahci_check_irq(AHCIState *s)
>>   }
>> static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
>> - int irq_type)
>> + enum AHCIPortIRQ irqbit)
>>   {
>> -DPRINTF(d->port_no, "trigger irq %#x -> %x\n",
>> -irq_type, d->port_regs.irq_mask & irq_type);
>> +g_assert(irqbit >= 0 && irqbit < 32);
> 
> Since you now use an enum this check is no more necessary.
> 

You can actually still pass in a wrong value if you wanted to. This is
to prevent old-style IRQ masks from getting passed in by accident.

> However ...
> 
>> +uint32_t irq = 1U << irqbit;
>> +uint32_t irqstat = d->port_regs.irq_stat | irq;
>>   -d->port_regs.irq_stat |= irq_type;
>> +trace_ahci_trigger_irq(s, d->port_no,
>> +   AHCIPortIRQ_lookup[irqbit], irq,
>> +   d->port_regs.irq_stat, irqstat,
>> +   irqstat & d->port_regs.irq_mask);
>> +
> 
> Why not keep using masked irqs, and iterate over AHCI_PORT_IRQ__END
> bits? Something like:
> 
> if (TRACE_AHCI_IRQ_ENABLED) {
> int irqbit;
> for (irqbit = 0; irqbit < AHCI_PORT_IRQ__END; irqbit++) {
> if (irqmask & BIT(irqbit)) {
> trace_ahci_trigger_irq(s, d->port_no, ...
> 

Would rather not iterate like that for the IRQ path. Generally no place
in the codebase needs to raise more than one IRQ type at a time, so why
bother allowing it?

Re: [Qemu-devel] [PATCH 7/9] AHCI: Rework IRQ constants

[Philippe Mathieu-Daudé]

Hi John,

On 08/08/2017 03:33 PM, John Snow wrote:

Create a new enum so that we can name the IRQ bits, which will make debugging
them a little nicer if we can print them out. Not handled in this patch, but
this will make it possible to get a nice debug printf detailing exactly which
status bits are set, as it can be multiple at any given time.

As a consequence of this patch, it is no longer possible to set multiple IRQ
codes at once, but nothing was utilizing this ability anyway.

Signed-off-by: John Snow 
---
  hw/ide/ahci.c  | 49 ++---
  hw/ide/ahci_internal.h | 44 +++-
  hw/ide/trace-events|  2 +-
  3 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d5acceb..c5a9b69 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -56,6 +56,27 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
  static void ahci_unmap_clb_address(AHCIDevice *ad);
  static void ahci_unmap_fis_address(AHCIDevice *ad);
  
+static const char *AHCIPortIRQ_lookup[AHCI_PORT_IRQ__END] = {

+[AHCI_PORT_IRQ_BIT_DHRS] = "DHRS",
+[AHCI_PORT_IRQ_BIT_PSS]  = "PSS",
+[AHCI_PORT_IRQ_BIT_DSS]  = "DSS",
+[AHCI_PORT_IRQ_BIT_SDBS] = "SDBS",
+[AHCI_PORT_IRQ_BIT_UFS]  = "UFS",
+[AHCI_PORT_IRQ_BIT_DPS]  = "DPS",
+[AHCI_PORT_IRQ_BIT_PCS]  = "PCS",
+[AHCI_PORT_IRQ_BIT_DMPS] = "DMPS",
+[8 ... 21]   = "RESERVED",
+[AHCI_PORT_IRQ_BIT_PRCS] = "PRCS",
+[AHCI_PORT_IRQ_BIT_IPMS] = "IPMS",
+[AHCI_PORT_IRQ_BIT_OFS]  = "OFS",
+[25] = "RESERVED",
+[AHCI_PORT_IRQ_BIT_INFS] = "INFS",
+[AHCI_PORT_IRQ_BIT_IFS]  = "IFS",
+[AHCI_PORT_IRQ_BIT_HBDS] = "HBDS",
+[AHCI_PORT_IRQ_BIT_HBFS] = "HBFS",
+[AHCI_PORT_IRQ_BIT_TFES] = "TFES",
+[AHCI_PORT_IRQ_BIT_CPDS] = "CPDS"
+};
  
  static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)

  {
@@ -170,12 +191,18 @@ static void ahci_check_irq(AHCIState *s)
  }
  
  static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,

- int irq_type)
+ enum AHCIPortIRQ irqbit)
  {
-DPRINTF(d->port_no, "trigger irq %#x -> %x\n",
-irq_type, d->port_regs.irq_mask & irq_type);
+g_assert(irqbit >= 0 && irqbit < 32);


Since you now use an enum this check is no more necessary.

However ...


+uint32_t irq = 1U << irqbit;
+uint32_t irqstat = d->port_regs.irq_stat | irq;
  
-d->port_regs.irq_stat |= irq_type;

+trace_ahci_trigger_irq(s, d->port_no,
+   AHCIPortIRQ_lookup[irqbit], irq,
+   d->port_regs.irq_stat, irqstat,
+   irqstat & d->port_regs.irq_mask);
+


Why not keep using masked irqs, and iterate over AHCI_PORT_IRQ__END 
bits? Something like:


if (TRACE_AHCI_IRQ_ENABLED) {
int irqbit;
for (irqbit = 0; irqbit < AHCI_PORT_IRQ__END; irqbit++) {
if (irqmask & BIT(irqbit)) {
trace_ahci_trigger_irq(s, d->port_no, ...


+d->port_regs.irq_stat = irqstat;
  ahci_check_irq(s);
  }
  
@@ -718,7 +745,7 @@ static void ahci_write_fis_sdb(AHCIState *s, NCQTransferState *ncq_tfs)
  
  /* Trigger IRQ if interrupt bit is set (which currently, it always is) */

  if (sdb_fis->flags & 0x40) {
-ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
+ahci_trigger_irq(s, ad, AHCI_PORT_IRQ_BIT_SDBS);
  }
  }
  
@@ -761,10 +788,10 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)

  ad->port.ifs[0].status;
  
  if (pio_fis[2] & ERR_STAT) {

-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
  }
  
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS);

+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_PSS);
  }
  
  static bool ahci_write_fis_d2h(AHCIDevice *ad)

@@ -804,10 +831,10 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad)
  ad->port.ifs[0].status;
  
  if (d2h_fis[2] & ERR_STAT) {

-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
  }
  
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS);

+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
  return true;
  }
  
@@ -1082,7 +1109,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,

   "is smaller than the requested size (0x%zx)",
   ncq_tfs->sglist.size, size);
  ncq_err(ncq_tfs);
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_OFS);
  return;
  } else if (ncq_tfs->sglist.size != size) {
  trace_process_ncq_command_large(s, port, tag,
@@ -1225,7 +1252,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t 
slot)
  

[Qemu-devel] [PATCH 7/9] AHCI: Rework IRQ constants

[John Snow]

Create a new enum so that we can name the IRQ bits, which will make debugging
them a little nicer if we can print them out. Not handled in this patch, but
this will make it possible to get a nice debug printf detailing exactly which
status bits are set, as it can be multiple at any given time.

As a consequence of this patch, it is no longer possible to set multiple IRQ
codes at once, but nothing was utilizing this ability anyway.

Signed-off-by: John Snow 
---
 hw/ide/ahci.c  | 49 ++---
 hw/ide/ahci_internal.h | 44 +++-
 hw/ide/trace-events|  2 +-
 3 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d5acceb..c5a9b69 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -56,6 +56,27 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
 static void ahci_unmap_clb_address(AHCIDevice *ad);
 static void ahci_unmap_fis_address(AHCIDevice *ad);
 
+static const char *AHCIPortIRQ_lookup[AHCI_PORT_IRQ__END] = {
+[AHCI_PORT_IRQ_BIT_DHRS] = "DHRS",
+[AHCI_PORT_IRQ_BIT_PSS]  = "PSS",
+[AHCI_PORT_IRQ_BIT_DSS]  = "DSS",
+[AHCI_PORT_IRQ_BIT_SDBS] = "SDBS",
+[AHCI_PORT_IRQ_BIT_UFS]  = "UFS",
+[AHCI_PORT_IRQ_BIT_DPS]  = "DPS",
+[AHCI_PORT_IRQ_BIT_PCS]  = "PCS",
+[AHCI_PORT_IRQ_BIT_DMPS] = "DMPS",
+[8 ... 21]   = "RESERVED",
+[AHCI_PORT_IRQ_BIT_PRCS] = "PRCS",
+[AHCI_PORT_IRQ_BIT_IPMS] = "IPMS",
+[AHCI_PORT_IRQ_BIT_OFS]  = "OFS",
+[25] = "RESERVED",
+[AHCI_PORT_IRQ_BIT_INFS] = "INFS",
+[AHCI_PORT_IRQ_BIT_IFS]  = "IFS",
+[AHCI_PORT_IRQ_BIT_HBDS] = "HBDS",
+[AHCI_PORT_IRQ_BIT_HBFS] = "HBFS",
+[AHCI_PORT_IRQ_BIT_TFES] = "TFES",
+[AHCI_PORT_IRQ_BIT_CPDS] = "CPDS"
+};
 
 static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
 {
@@ -170,12 +191,18 @@ static void ahci_check_irq(AHCIState *s)
 }
 
 static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
- int irq_type)
+ enum AHCIPortIRQ irqbit)
 {
-DPRINTF(d->port_no, "trigger irq %#x -> %x\n",
-irq_type, d->port_regs.irq_mask & irq_type);
+g_assert(irqbit >= 0 && irqbit < 32);
+uint32_t irq = 1U << irqbit;
+uint32_t irqstat = d->port_regs.irq_stat | irq;
 
-d->port_regs.irq_stat |= irq_type;
+trace_ahci_trigger_irq(s, d->port_no,
+   AHCIPortIRQ_lookup[irqbit], irq,
+   d->port_regs.irq_stat, irqstat,
+   irqstat & d->port_regs.irq_mask);
+
+d->port_regs.irq_stat = irqstat;
 ahci_check_irq(s);
 }
 
@@ -718,7 +745,7 @@ static void ahci_write_fis_sdb(AHCIState *s, 
NCQTransferState *ncq_tfs)
 
 /* Trigger IRQ if interrupt bit is set (which currently, it always is) */
 if (sdb_fis->flags & 0x40) {
-ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
+ahci_trigger_irq(s, ad, AHCI_PORT_IRQ_BIT_SDBS);
 }
 }
 
@@ -761,10 +788,10 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t 
len)
 ad->port.ifs[0].status;
 
 if (pio_fis[2] & ERR_STAT) {
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
 }
 
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_PSS);
 }
 
 static bool ahci_write_fis_d2h(AHCIDevice *ad)
@@ -804,10 +831,10 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad)
 ad->port.ifs[0].status;
 
 if (d2h_fis[2] & ERR_STAT) {
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
 }
 
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
 return true;
 }
 
@@ -1082,7 +1109,7 @@ static void process_ncq_command(AHCIState *s, int port, 
uint8_t *cmd_fis,
  "is smaller than the requested size (0x%zx)",
  ncq_tfs->sglist.size, size);
 ncq_err(ncq_tfs);
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_OFS);
 return;
 } else if (ncq_tfs->sglist.size != size) {
 trace_process_ncq_command_large(s, port, tag,
@@ -1225,7 +1252,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t 
slot)
 trace_handle_cmd_badfis(s, port);
 return -1;
 } else if (cmd_len != 0x80) {
-ahci_trigger_irq(s, >dev[port], PORT_IRQ_HBUS_ERR);
+ahci_trigger_irq(s, >dev[port], AHCI_PORT_IRQ_BIT_HBFS);
 trace_handle_cmd_badmap(s, port, cmd_len);
 goto out;
 }
diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index 1e21169..7e67add 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -91,6 +91,31 @@
 #define PORT_CMD_ISSUE0x38 /* command