[Qemu-devel] Re: [PATCH v9 3/8] pci: clean up of pci status register

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Nov 16, 2010 at 05:26:07PM +0900, Isaku Yamahata wrote:
> This patch refine the initialization/reset of
> pci status registers.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

This one seems good. Applied with some tweaks: I cut down the comment:
we don't really need to repeat what code does IMO, rather why it does
it, and clarified the comment text.  Also split init_w1c out to a
function so that each function does one thing.

> ---
>  hw/pci.c |   41 +++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 52fe655..fba765b 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -145,6 +145,9 @@ static void pci_device_reset(PCIDevice *dev)
>      pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
>                                   pci_get_word(dev->wmask + PCI_COMMAND) |
>                                   pci_get_word(dev->w1cmask + PCI_COMMAND));
> +    pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
> +                                 pci_get_word(dev->wmask + PCI_STATUS) |
> +                                 pci_get_word(dev->w1cmask + PCI_STATUS));
>      dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
>      dev->config[PCI_INTERRUPT_LINE] = 0x0;
>      for (r = 0; r < PCI_NUM_REGIONS; ++r) {
> @@ -540,7 +543,7 @@ static void pci_init_cmask(PCIDevice *dev)
>      dev->cmask[PCI_CAPABILITY_LIST] = 0xff;
>  }
>  
> -static void pci_init_wmask(PCIDevice *dev)
> +static void pci_init_wmask_w1cmask(PCIDevice *dev)
>  {
>      int config_size = pci_config_size(dev);
>  
> @@ -595,6 +598,40 @@ static void pci_init_wmask(PCIDevice *dev)
>                   PCI_COMMAND_MASTER | PCI_COMMAND_PARITY | PCI_COMMAND_SERR |
>                   PCI_COMMAND_INTX_DISABLE);
>  
> +    /*
> +     * bit 0-2: reserved
> +     * bit 3: PCI_STATUS_INTERRUPT: RO
> +     * bit 4: PCI_STATUS_CAP_LIST: RO
> +     * bit 5: PCI_STATUS_66MHZ: RO
> +     * bit 6: PCI_STATUS_UDF: reserved (PCI 2.2-)
> +     * bit 7: PCI_STATUS_FAST_BACK: RO
> +     * bit 8: PCI_STATUS_PARITY
> +     *        type 0: RW for bus master
> +     *        type 1: RW1C
> +     * bit 9-10: PCI_STATUS_DEVSEL: RO
> +     * bit 11: PCI_STATUS_SIG_TARGET_ABORT
> +     *         type 0: RW1C for targets that is capable of terminating
> +     *                 a transaction.
> +     *         type 1: RW1C
> +     * bit 12: PCI_STATUS_REC_TARGET_ABORT
> +     *         type 0: RW1C for masters
> +     *         type 1: RW1C
> +     * bit 13: PCI_STATUS_REC_MASTER_ABORT
> +     *         type 0: RW1C for masters
> +     *         type 1: RW1C
> +     * bit 14: PCI_STATUS_SIG_SYSTEM_ERROR
> +     *         type 0: RW1C with execptions
> +     *         type 1: RW1C
> +     * bit : PCI_STATUS_DETECTED_PARITY: RW1C
> +     *
> +     * It's okay to set w1mask even for RO=0(i.e. reserved) because
> +     * writing value 1 to w1c bit whose value is 0 has no effect.
> +     */
> +    pci_set_word(dev->w1cmask + PCI_STATUS,
> +                 PCI_STATUS_PARITY | PCI_STATUS_SIG_TARGET_ABORT |
> +                 PCI_STATUS_REC_TARGET_ABORT | PCI_STATUS_REC_MASTER_ABORT |
> +                 PCI_STATUS_SIG_SYSTEM_ERROR | PCI_STATUS_DETECTED_PARITY);
> +
>      memset(dev->wmask + PCI_CONFIG_HEADER_SIZE, 0xff,
>             config_size - PCI_CONFIG_HEADER_SIZE);
>  }
> @@ -725,7 +762,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>          pci_set_default_subsystem_id(pci_dev);
>      }
>      pci_init_cmask(pci_dev);
> -    pci_init_wmask(pci_dev);
> +    pci_init_wmask_w1cmask(pci_dev);
>      if (is_bridge) {
>          pci_init_wmask_bridge(pci_dev);
>      }
> -- 
> 1.7.1.1