Re: [PATCH v1 01/10] xen/pt: Use xen_host_pci_get_[byte|word] instead of dev.config

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> During init time we treat the dev.config area as a cache
> of the host view. However during execution time we treat it
> as guest view (by the generic PCI API). We need to sync Xen's
> code to the generic PCI API view. This is the first step
> by replacing all of the code that uses dev.config or
> pci_get_[byte|word] to get host value to actually use the
> xen_host_pci_get_[byte|word] functions.
> 
> Interestingly in 'xen_pt_ptr_reg_init' we also needed to swap
> reg_field from uint32_t to uint8_t - since the access is only
> for one byte not four bytes. We can split this as a seperate
> patch however we would have to use a cast to thwart compiler
> warnings in the meantime.
> 
> We also truncated 'flags' to 'flag' to make the code fit within
> the 80 characters.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  hw/xen/xen_pt.c             | 24 +++++++++++---
>  hw/xen/xen_pt_config_init.c | 77 +++++++++++++++++++++++++++++++--------------
>  2 files changed, 73 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index ea1ceda..7575311 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -689,7 +689,7 @@ static int xen_pt_initfn(PCIDevice *d)
>  {
>      XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
>      int rc = 0;
> -    uint8_t machine_irq = 0;
> +    uint8_t machine_irq = 0, scratch;
>      uint16_t cmd = 0;
>      int pirq = XEN_PT_UNASSIGNED_PIRQ;
>  
> @@ -735,7 +735,12 @@ static int xen_pt_initfn(PCIDevice *d)
>      }
>  
>      /* Bind interrupt */
> -    if (!s->dev.config[PCI_INTERRUPT_PIN]) {
> +    rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
> +    if (rc) {
> +        XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
> +        scratch = 0;
> +    }
> +    if (!scratch) {
>          XEN_PT_LOG(d, "no pin interrupt\n");
>          goto out;
>      }
> @@ -785,8 +790,19 @@ static int xen_pt_initfn(PCIDevice *d)
>  
>  out:
>      if (cmd) {
> -        xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
> -                              pci_get_word(d->config + PCI_COMMAND) | cmd);
> +        uint16_t val;
> +
> +        rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
> +        if (rc) {
> +            XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
> +        } else {
> +            val |= cmd;
> +            rc = xen_host_pci_set_word(&s->real_device, PCI_COMMAND, val);
> +            if (rc) {
> +                XEN_PT_ERR(d, "Failed to write PCI_COMMAND val=0x%x!(rc: %d)\n",
> +                           val, rc);
> +            }
> +        }
>      }
>  
>      memory_listener_register(&s->memory_listener, &s->dev.bus_master_as);
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 21d4938..e34f9f8 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -800,15 +800,21 @@ static XenPTRegInfo xen_pt_emu_reg_vendor[] = {
>  static inline uint8_t get_capability_version(XenPCIPassthroughState *s,
>                                               uint32_t offset)
>  {
> -    uint8_t flags = pci_get_byte(s->dev.config + offset + PCI_EXP_FLAGS);
> -    return flags & PCI_EXP_FLAGS_VERS;
> +    uint8_t flag;
> +    if (xen_host_pci_get_byte(&s->real_device, offset + PCI_EXP_FLAGS, &flag)) {
> +        return 0;
> +    }
> +    return flag & PCI_EXP_FLAGS_VERS;
>  }
>  
>  static inline uint8_t get_device_type(XenPCIPassthroughState *s,
>                                        uint32_t offset)
>  {
> -    uint8_t flags = pci_get_byte(s->dev.config + offset + PCI_EXP_FLAGS);
> -    return (flags & PCI_EXP_FLAGS_TYPE) >> 4;
> +    uint8_t flag;
> +    if (xen_host_pci_get_byte(&s->real_device, offset + PCI_EXP_FLAGS, &flag)) {
> +        return 0;
> +    }
> +    return (flag & PCI_EXP_FLAGS_TYPE) >> 4;
>  }
>  
>  /* initialize Link Control register */
> @@ -857,8 +863,14 @@ static int xen_pt_linkctrl2_reg_init(XenPCIPassthroughState *s,
>          reg_field = XEN_PT_INVALID_REG;
>      } else {
>          /* set Supported Link Speed */
> -        uint8_t lnkcap = pci_get_byte(s->dev.config + real_offset - reg->offset
> -                                      + PCI_EXP_LNKCAP);
> +        uint8_t lnkcap;
> +        int rc;
> +        rc = xen_host_pci_get_byte(&s->real_device,
> +                                   real_offset - reg->offset + PCI_EXP_LNKCAP,
> +                                   &lnkcap);
> +        if (rc) {
> +            return rc;
> +        }
>          reg_field |= PCI_EXP_LNKCAP_SLS & lnkcap;
>      }
>  
> @@ -1039,13 +1051,15 @@ static int xen_pt_msgctrl_reg_init(XenPCIPassthroughState *s,
>                                     XenPTRegInfo *reg, uint32_t real_offset,
>                                     uint32_t *data)
>  {
> -    PCIDevice *d = &s->dev;
>      XenPTMSI *msi = s->msi;
> -    uint16_t reg_field = 0;
> +    uint16_t reg_field;
> +    int rc;
>  
>      /* use I/O device register's value as initial value */
> -    reg_field = pci_get_word(d->config + real_offset);
> -
> +    rc = xen_host_pci_get_word(&s->real_device, real_offset, &reg_field);
> +    if (rc) {
> +        return rc;
> +    }
>      if (reg_field & PCI_MSI_FLAGS_ENABLE) {
>          XEN_PT_LOG(&s->dev, "MSI already enabled, disabling it first\n");
>          xen_host_pci_set_word(&s->real_device, real_offset,
> @@ -1411,12 +1425,14 @@ static int xen_pt_msixctrl_reg_init(XenPCIPassthroughState *s,
>                                      XenPTRegInfo *reg, uint32_t real_offset,
>                                      uint32_t *data)
>  {
> -    PCIDevice *d = &s->dev;
> -    uint16_t reg_field = 0;
> +    uint16_t reg_field;
> +    int rc;
>  
>      /* use I/O device register's value as initial value */
> -    reg_field = pci_get_word(d->config + real_offset);
> -
> +    rc = xen_host_pci_get_word(&s->real_device, real_offset, &reg_field);
> +    if (rc) {
> +        return rc;
> +    }
>      if (reg_field & PCI_MSIX_FLAGS_ENABLE) {
>          XEN_PT_LOG(&s->dev, "MSIX already enabled, disabling it first\n");
>          xen_host_pci_set_word(&s->real_device, real_offset,
> @@ -1511,8 +1527,7 @@ static int xen_pt_vendor_size_init(XenPCIPassthroughState *s,
>                                     const XenPTRegGroupInfo *grp_reg,
>                                     uint32_t base_offset, uint8_t *size)
>  {
> -    *size = pci_get_byte(s->dev.config + base_offset + 0x02);
> -    return 0;
> +    return xen_host_pci_get_byte(&s->real_device, base_offset + 0x02, size);
>  }
>  /* get PCI Express Capability Structure register group size */
>  static int xen_pt_pcie_size_init(XenPCIPassthroughState *s,
> @@ -1591,12 +1606,15 @@ static int xen_pt_msi_size_init(XenPCIPassthroughState *s,
>                                  const XenPTRegGroupInfo *grp_reg,
>                                  uint32_t base_offset, uint8_t *size)
>  {
> -    PCIDevice *d = &s->dev;
>      uint16_t msg_ctrl = 0;
>      uint8_t msi_size = 0xa;
> +    int rc;
>  
> -    msg_ctrl = pci_get_word(d->config + (base_offset + PCI_MSI_FLAGS));
> -
> +    rc = xen_host_pci_get_word(&s->real_device, base_offset + PCI_MSI_FLAGS,
> +                               &msg_ctrl);
> +    if (rc) {
> +        return rc;
> +    }
>      /* check if 64-bit address is capable of per-vector masking */
>      if (msg_ctrl & PCI_MSI_FLAGS_64BIT) {
>          msi_size += 4;
> @@ -1739,11 +1757,14 @@ static int xen_pt_ptr_reg_init(XenPCIPassthroughState *s,
>                                 XenPTRegInfo *reg, uint32_t real_offset,
>                                 uint32_t *data)
>  {
> -    int i;
> -    uint8_t *config = s->dev.config;
> -    uint32_t reg_field = pci_get_byte(config + real_offset);
> +    int i, rc;
> +    uint8_t reg_field;
>      uint8_t cap_id = 0;
>  
> +    rc = xen_host_pci_get_byte(&s->real_device, real_offset, &reg_field);
> +    if (rc) {
> +        return rc;
> +    }
>      /* find capability offset */
>      while (reg_field) {
>          for (i = 0; xen_pt_emu_reg_grps[i].grp_size != 0; i++) {
> @@ -1752,7 +1773,11 @@ static int xen_pt_ptr_reg_init(XenPCIPassthroughState *s,
>                  continue;
>              }
>  
> -            cap_id = pci_get_byte(config + reg_field + PCI_CAP_LIST_ID);
> +            rc = xen_host_pci_get_byte(&s->real_device,
> +                                       reg_field + PCI_CAP_LIST_ID, &cap_id);
> +            if (rc) {
> +                return rc;
> +            }
>              if (xen_pt_emu_reg_grps[i].grp_id == cap_id) {
>                  if (xen_pt_emu_reg_grps[i].grp_type == XEN_PT_GRP_TYPE_EMU) {
>                      goto out;
> @@ -1763,7 +1788,11 @@ static int xen_pt_ptr_reg_init(XenPCIPassthroughState *s,
>          }
>  
>          /* next capability */
> -        reg_field = pci_get_byte(config + reg_field + PCI_CAP_LIST_NEXT);
> +        rc = xen_host_pci_get_byte(&s->real_device,
> +                                   reg_field + PCI_CAP_LIST_NEXT, &reg_field);
> +        if (rc) {
> +            return rc;
> +        }
>      }
>  
>  out:
> -- 
> 2.1.0
>