Re: [Qemu-block] [Qemu-devel] [PATCH v3 01/16] fdc: move object structures to header file

2018-01-04 Thread Marcel Apfelbaum 

On 29/12/2017 16:29, Hervé Poussineau wrote:

We are now able to embed floppy controllers in another object.



Hi Hervé,

Are you sure we need to move all the struct definitions to the header file?

I looked at patch 11/16 and it seems only FDCtrlISABus definition is needed.
And also only the typedef is needed and not the fields.

It may worth a look also patches 2/16 and 3/16.

Thanks,
Marcel


Signed-off-by: Hervé Poussineau 
---
  hw/block/fdc.c | 102 
  include/hw/block/fdc.h | 103 +
  2 files changed, 103 insertions(+), 102 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 7b7dd41296..c81e0313c8 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,15 +60,8 @@
  #define TYPE_FLOPPY_BUS "floppy-bus"
  #define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
  
-typedef struct FDCtrl FDCtrl;

-typedef struct FDrive FDrive;
  static FDrive *get_drv(FDCtrl *fdctrl, int unit);
  
-typedef struct FloppyBus {

-BusState bus;
-FDCtrl *fdc;
-} FloppyBus;
-
  static const TypeInfo floppy_bus_info = {
  .name = TYPE_FLOPPY_BUS,
  .parent = TYPE_BUS,
@@ -178,36 +171,6 @@ static FDriveSize drive_size(FloppyDriveType drive)
  #define FD_SECTOR_SC   2   /* Sector size code */
  #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */
  
-/* Floppy disk drive emulation */

-typedef enum FDiskFlags {
-FDISK_DBL_SIDES  = 0x01,
-} FDiskFlags;
-
-struct FDrive {
-FDCtrl *fdctrl;
-BlockBackend *blk;
-BlockConf *conf;
-/* Drive status */
-FloppyDriveType drive;/* CMOS drive type*/
-uint8_t perpendicular;/* 2.88 MB access mode*/
-/* Position */
-uint8_t head;
-uint8_t track;
-uint8_t sect;
-/* Media */
-FloppyDriveType disk; /* Current disk type  */
-FDiskFlags flags;
-uint8_t last_sect;/* Nb sector per track*/
-uint8_t max_track;/* Nb of tracks   */
-uint16_t bps; /* Bytes per sector   */
-uint8_t ro;   /* Is read-only   */
-uint8_t media_changed;/* Is media changed   */
-uint8_t media_rate;   /* Data rate of medium*/
-
-bool media_validated; /* Have we validated the media? */
-};
-
-
  static FloppyDriveType get_fallback_drive_type(FDrive *drv);
  
  /* Hack: FD_SEEK is expected to work on empty drives. However, QEMU

@@ -819,60 +782,6 @@ enum {
  #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
  #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
  
-struct FDCtrl {

-MemoryRegion iomem;
-qemu_irq irq;
-/* Controller state */
-QEMUTimer *result_timer;
-int dma_chann;
-uint8_t phase;
-IsaDma *dma;
-/* Controller's identification */
-uint8_t version;
-/* HW */
-uint8_t sra;
-uint8_t srb;
-uint8_t dor;
-uint8_t dor_vmstate; /* only used as temp during vmstate */
-uint8_t tdr;
-uint8_t dsr;
-uint8_t msr;
-uint8_t cur_drv;
-uint8_t status0;
-uint8_t status1;
-uint8_t status2;
-/* Command FIFO */
-uint8_t *fifo;
-int32_t fifo_size;
-uint32_t data_pos;
-uint32_t data_len;
-uint8_t data_state;
-uint8_t data_dir;
-uint8_t eot; /* last wanted sector */
-/* States kept only to be returned back */
-/* precompensation */
-uint8_t precomp_trk;
-uint8_t config;
-uint8_t lock;
-/* Power down config (also with status regB access mode */
-uint8_t pwrd;
-/* Floppy drives */
-FloppyBus bus;
-uint8_t num_floppies;
-FDrive drives[MAX_FD];
-struct {
-BlockBackend *blk;
-FloppyDriveType type;
-} qdev_for_drives[MAX_FD];
-int reset_sensei;
-uint32_t check_media_rate;
-FloppyDriveType fallback; /* type=auto failure fallback */
-/* Timers state */
-uint8_t timer0;
-uint8_t timer1;
-PortioList portio_list;
-};
-
  static FloppyDriveType get_fallback_drive_type(FDrive *drv)
  {
  return drv->fdctrl->fallback;
@@ -891,17 +800,6 @@ typedef struct FDCtrlSysBus {
  
  #define ISA_FDC(obj) OBJECT_CHECK(FDCtrlISABus, (obj), TYPE_ISA_FDC)
  
-typedef struct FDCtrlISABus {

-ISADevice parent_obj;
-
-uint32_t iobase;
-uint32_t irq;
-uint32_t dma;
-struct FDCtrl state;
-int32_t bootindexA;
-int32_t bootindexB;
-} FDCtrlISABus;
-
  static uint32_t fdctrl_read (void *opaque, uint32_t reg)
  {
  FDCtrl *fdctrl = opaque;
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index 1749dabf25..d076b2fc1a 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -2,12 +2,115 @@
  #define HW_FDC_H
  
  #include "qemu-common.h"

+#include "hw/block/block.h"
+#include "hw/isa/isa.h"
  
  /* fdc.c */

  #define MAX_FD 2
  
+typedef struct FDCtrl FDCtrl;

+
+/* Floppy disk drive emulation */
+typedef

Re: [Qemu-block] [PATCH V5] pci: removed the is_express field since a uniform interface was inserted

2017-12-19 Thread Marcel Apfelbaum 
@@ -937,6 +937,13 @@ static Property xen_pci_passthrough_properties[] = {
  DEFINE_PROP_END_OF_LIST(),
  };
  
+static void xen_pci_passthrough_instance_init(Object *obj)

+{
+/* QEMU_PCI_CAP_EXPRESS initialization does not depend on QEMU command
+ * line, therefore, no need to wait to realize like other devices */
+PCI_DEVICE(obj)->cap_present |= QEMU_PCI_CAP_EXPRESS;
+}
+
  static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data)
  {
  DeviceClass *dc = DEVICE_CLASS(klass);
@@ -946,7 +953,6 @@ static void xen_pci_passthrough_class_init(ObjectClass 
*klass, void *data)
  k->exit = xen_pt_unregister_device;
  k->config_read = xen_pt_pci_read_config;
  k->config_write = xen_pt_pci_write_config;
-k->is_express = 1; /* We might be */
  set_bit(DEVICE_CATEGORY_MISC, dc->categories);
  dc->desc = "Assign an host PCI device with Xen";
  dc->props = xen_pci_passthrough_properties;
@@ -965,6 +971,7 @@ static const TypeInfo xen_pci_passthrough_info = {
  .instance_size = sizeof(XenPCIPassthroughState),
  .instance_finalize = xen_pci_passthrough_finalize,
  .class_init = xen_pci_passthrough_class_init,
+.instance_init = xen_pci_passthrough_instance_init,
  .interfaces = (InterfaceInfo[]) {
  { INTERFACE_CONVENTIONAL_PCI_DEVICE },
  { INTERFACE_PCIE_DEVICE },
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 8d02a0a383..a27be85111 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -236,9 +236,6 @@ typedef struct PCIDeviceClass {
   */
  int is_bridge;
  
-/* pcie stuff */

-int is_express;   /* is this device pci express? */
-
  /* rom bar */
  const char *romfile;
  } PCIDeviceClass;



Reviewed-by: Marcel Apfelbaum <mar...@redhat.com>

Thanks,
Marcel

Re: [Qemu-block] [PATCH v2 2/4] hw/pci-host/piix: QOM'ify the IGD Passthrough host bridge

2017-12-18 Thread Marcel Apfelbaum 

On 18/12/2017 17:12, Philippe Mathieu-Daudé wrote:

Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
---
v2: use g_strdup_printf(), use 'pci_dev' variable.

  hw/pci-host/piix.c | 45 -
  1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index a684a7cca9..bb6560f815 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -804,60 +804,55 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
  {0xa8, 4},  /* SNB: base of GTT stolen memory */
  };
  
-static int host_pci_config_read(int pos, int len, uint32_t *val)

+static void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp)
  {
-char path[PATH_MAX];
-int config_fd;
-ssize_t size = sizeof(path);
+int rc, config_fd;
  /* Access real host bridge. */
-int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
-  0, 0, 0, 0, "config");
-int ret = 0;
-
-if (rc >= size || rc < 0) {
-return -ENODEV;
-}
+char *path = g_strdup_printf("/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
+ 0, 0, 0, 0, "config");
  
  config_fd = open(path, O_RDWR);

  if (config_fd < 0) {
-return -ENODEV;
+error_setg_errno(errp, errno, "Failed to open: %s", path);
+goto out;
  }
  
  if (lseek(config_fd, pos, SEEK_SET) != pos) {

-ret = -errno;
-goto out;
+error_setg_errno(errp, errno, "Failed to seek: %s", path);
+goto out_close_fd;
  }
  
  do {

  rc = read(config_fd, (uint8_t *)val, len);
  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
  if (rc != len) {
-ret = -errno;
+error_setg_errno(errp, errno, "Failed to read: %s", path);
  }
  
-out:

+out_close_fd:
  close(config_fd);
-return ret;
+out:
+g_free(path);
  }
  
-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)

+static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
  {
  uint32_t val = 0;
-int rc, i, num;
+int i, num;
  int pos, len;
+Error *local_err = NULL;
  
  num = ARRAY_SIZE(igd_host_bridge_infos);

  for (i = 0; i < num; i++) {
  pos = igd_host_bridge_infos[i].offset;
  len = igd_host_bridge_infos[i].len;
-rc = host_pci_config_read(pos, len, );
-if (rc) {
-return -ENODEV;
+host_pci_config_read(pos, len, , _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
  }
  pci_default_write_config(pci_dev, pos, val, len);
  }
-
-return 0;
  }
  
  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)

@@ -865,7 +860,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass 
*klass, void *data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
  
-k->init = igd_pt_i440fx_initfn;

+k->realize = igd_pt_i440fx_realize;
  dc->desc = "IGD Passthrough Host bridge";
  }
   >


Reviewed-by: Marcel Apfelbaum <mar...@redhat.com>

Thanks,
Marcel

Re: [Qemu-block] [PATCH 2/4] hw/pci-host/piix: QOM'ify the IGD Passthrough host bridge

2017-12-17 Thread Marcel Apfelbaum 

Hi Philippe,

Thanks for the patch.

On 17/12/2017 22:49, Philippe Mathieu-Daudé wrote:

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/pci-host/piix.c | 31 +++
  1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index a684a7cca9..0153f32a32 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -804,7 +804,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
  {0xa8, 4},  /* SNB: base of GTT stolen memory */
  };
  
-static int host_pci_config_read(int pos, int len, uint32_t *val)

+static void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp)
  {
  char path[PATH_MAX];
  int config_fd;
@@ -812,19 +812,19 @@ static int host_pci_config_read(int pos, int len, 
uint32_t *val)
  /* Access real host bridge. */
  int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
0, 0, 0, 0, "config");
-int ret = 0;
  
  if (rc >= size || rc < 0) {

-return -ENODEV;
+error_setg(_abort, "Invalid PCI device");


I think a return statement is missing here.


  }
  
  config_fd = open(path, O_RDWR);

  if (config_fd < 0) {
-return -ENODEV;
+error_setg_errno(errp, errno, "Failed to open: %s", path);
+return;
  }
  
  if (lseek(config_fd, pos, SEEK_SET) != pos) {

-ret = -errno;
+error_setg_errno(errp, errno, "Failed to seek: %s", path);
  goto out;
  }
  
@@ -832,32 +832,31 @@ static int host_pci_config_read(int pos, int len, uint32_t *val)

  rc = read(config_fd, (uint8_t *)val, len);
  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
  if (rc != len) {
-ret = -errno;
+error_setg_errno(errp, errno, "Failed to read: %s", path);
  }
  
  out:

  close(config_fd);
-return ret;
  }
  
-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)

+static void igd_pt_i440fx_realize(PCIDevice *pci, Error **errp)


I would keep pci_dev, pci is too "broad".
Also is kind of convention, try to grep the source code.


Other than that the patch looks good to me.
Thanks,
Marcel


  {
  uint32_t val = 0;
-int rc, i, num;
+int i, num;
  int pos, len;
+Error *local_err = NULL;
  
  num = ARRAY_SIZE(igd_host_bridge_infos);

  for (i = 0; i < num; i++) {
  pos = igd_host_bridge_infos[i].offset;
  len = igd_host_bridge_infos[i].len;
-rc = host_pci_config_read(pos, len, );
-if (rc) {
-return -ENODEV;
+host_pci_config_read(pos, len, , _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
  }
-pci_default_write_config(pci_dev, pos, val, len);
+pci_default_write_config(pci, pos, val, len);
  }
-
-return 0;
  }
  
  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)

@@ -865,7 +864,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass 
*klass, void *data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
  
-k->init = igd_pt_i440fx_initfn;

+k->realize = igd_pt_i440fx_realize;
  dc->desc = "IGD Passthrough Host bridge";
  }

Re: [Qemu-block] [PATCH V4] pci: removed the is_express field since a uniform interface was inserted

2017-12-12 Thread Marcel Apfelbaum 
..a27be85111 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -236,9 +236,6 @@ typedef struct PCIDeviceClass {
   */
  int is_bridge;
  
-/* pcie stuff */

-int is_express;   /* is this device pci express? */
-
  /* rom bar */
  const char *romfile;
  } PCIDeviceClass;



No changes in the code, my review stands.

Reviewed-by: Marcel Apfelbaum <mar...@redhat.com>

Thanks,
Marcel

Re: [Qemu-block] [PATCH V3] pci: removed the is_express field since a uniform interface was inserted

2017-12-06 Thread Marcel Apfelbaum 
git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
index 7659253090..a4ca3ba30f 100644
--- a/hw/pci-host/xilinx-pcie.c
+++ b/hw/pci-host/xilinx-pcie.c
@@ -298,7 +298,6 @@ static void xilinx_pcie_root_class_init(ObjectClass *klass, 
void *data)
  k->device_id = 0x7021;
  k->revision = 0;
  k->class_id = PCI_CLASS_BRIDGE_HOST;
-k->is_express = true;
  k->is_bridge = true;
  k->init = xilinx_pcie_root_init;
  k->exit = pci_bridge_exitfn;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b2d139bd9a..6b5676b0f4 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2014,12 +2014,14 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
**errp)
  {
  PCIDevice *pci_dev = (PCIDevice *)qdev;
  PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
+ObjectClass *klass = OBJECT_CLASS(pc);
  Error *local_err = NULL;
  PCIBus *bus;
  bool is_default_rom;
  
  /* initialize cap_present for pci_is_express() and pci_config_size() */

-if (pc->is_express) {
+if (object_class_dynamic_cast(klass, INTERFACE_PCIE_DEVICE) &&
+   !object_class_dynamic_cast(klass, INTERFACE_CONVENTIONAL_PCI_DEVICE)) {
  pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
  }
  
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c

index d5eae6239a..ee51feda59 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2447,7 +2447,6 @@ typedef struct MegasasInfo {
  uint16_t subsystem_id;
  int ioport_bar;
  int mmio_bar;
-bool is_express;
  int osts;
  const VMStateDescription *vmsd;
  Property *props;
@@ -2465,7 +2464,6 @@ static struct MegasasInfo megasas_devices[] = {
  .ioport_bar = 2,
  .mmio_bar = 0,
  .osts = MFI_1078_RM | 1,
-.is_express = false,
  .vmsd = _megasas_gen1,
  .props = megasas_properties_gen1,
  .interfaces = (InterfaceInfo[]) {
@@ -2482,7 +2480,6 @@ static struct MegasasInfo megasas_devices[] = {
  .ioport_bar = 0,
  .mmio_bar = 1,
  .osts = MFI_GEN2_RM,
-.is_express = true,
  .vmsd = _megasas_gen2,
  .props = megasas_properties_gen2,
  .interfaces = (InterfaceInfo[]) {
@@ -2506,7 +2503,6 @@ static void megasas_class_init(ObjectClass *oc, void 
*data)
  pc->subsystem_vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
  pc->subsystem_id = info->subsystem_id;
  pc->class_id = PCI_CLASS_STORAGE_RAID;
-pc->is_express = info->is_express;
  e->mmio_bar = info->mmio_bar;
  e->ioport_bar = info->ioport_bar;
  e->osts = info->osts;
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index af3a9d88de..2e4dd71248 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3649,6 +3649,11 @@ static Property xhci_properties[] = {
  DEFINE_PROP_END_OF_LIST(),
  };
  
+static void xhci_instance_init(Object *obj)

+{
+PCI_DEVICE(obj)->cap_present |= QEMU_PCI_CAP_EXPRESS;
+}
+
  static void xhci_class_init(ObjectClass *klass, void *data)
  {
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
@@ -3661,7 +3666,6 @@ static void xhci_class_init(ObjectClass *klass, void 
*data)
  k->realize  = usb_xhci_realize;
  k->exit = usb_xhci_exit;
  k->class_id = PCI_CLASS_SERIAL_USB;
-k->is_express   = 1;
  }
  
  static const TypeInfo xhci_info = {

@@ -3669,6 +3673,7 @@ static const TypeInfo xhci_info = {
  .parent= TYPE_PCI_DEVICE,
  .instance_size = sizeof(XHCIState),
  .class_init= xhci_class_init,
+.instance_init = xhci_instance_init,
  .abstract  = true,
  .interfaces = (InterfaceInfo[]) {
  { INTERFACE_PCIE_DEVICE },
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c977ee327f..afad0c002f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2972,6 +2972,8 @@ static void vfio_instance_init(Object *obj)
  vdev->host.function = ~0U;
  
  vdev->nv_gpudirect_clique = 0xFF;

+
+pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
  }
  
  static Property vfio_pci_dev_properties[] = {

@@ -3026,7 +3028,6 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, 
void *data)
  pdc->exit = vfio_exitfn;
  pdc->config_read = vfio_pci_read_config;
  pdc->config_write = vfio_pci_write_config;
-pdc->is_express = 1; /* We might be */
  }
  
  static const TypeInfo vfio_pci_dev_info = {

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 8d02a0a383..a27be85111 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -236,9 +236,6 @@ typedef struct PCIDeviceClass {
   */
  int is_bridge;
  
-/* pcie stuff */

-int is_express;   /* is this device pci express? */
-
  /* rom bar */
  const char *romfile;
  } PCIDeviceClass;



Reviewed-by: Marcel Apfelbaum <mar...@redhat.com>

Thanks,
Marcel

Re: [Qemu-block] [Qemu-devel] [PATCH v2] pci: removed the is_express field since a uniform interface was inserted

2017-12-05 Thread Marcel Apfelbaum 

On 04/12/2017 21:46, Philippe Mathieu-Daudé wrote:

Hi Yoni, Eduardo, Markus,

On 12/04/2017 07:18 AM, Yoni Bettan wrote:

 * according to Eduardo Habkost's commit
   fd3b02c8896d597dd8b9e053dec579cf0386aee1

 * since all PCIEs now implement INTERFACE_PCIE_DEVICE we
   don't need this field anymore

 * Devices that where only INTERFACE_PCIE_DEVICE (is_express == 1)
   or
   devices that where only INTERFACE_CONVENTIONAL_PCI_DEVICE 
(is_express == 0)
   where not affected by the change

   The only devices that were affected are those that are hybrid and 
also
   had (is_express == 1) - therefor only:
 - hw/vfio/pci.c
 - hw/usb/hcd-xhci.c

   For both i made sure that pci_dev->cap_present |= 
QEMU_PCI_CAP_EXPRESS

Signed-off-by: Yoni Bettan 
---
  docs/pcie_pci_bridge.txt   |  2 +-
  hw/block/nvme.c|  1 -
  hw/net/e1000e.c|  1 -
  hw/pci-bridge/pcie_pci_bridge.c|  1 -
  hw/pci-bridge/pcie_root_port.c |  1 -
  hw/pci-bridge/xio3130_downstream.c |  1 -
  hw/pci-bridge/xio3130_upstream.c   |  1 -
  hw/pci-host/xilinx-pcie.c  |  1 -
  hw/pci/pci.c   |  4 +++-
  hw/scsi/megasas.c  |  4 
  hw/usb/hcd-xhci.c  | 28 ++--
  hw/vfio/pci.c  | 37 +++--
  include/hw/pci/pci.h   |  3 ---
  include/hw/usb.h   |  1 +
  14 files changed, 62 insertions(+), 24 deletions(-)

diff --git a/docs/pcie_pci_bridge.txt b/docs/pcie_pci_bridge.txt
index 5a4203f97c..55f833ec10 100644
--- a/docs/pcie_pci_bridge.txt
+++ b/docs/pcie_pci_bridge.txt
@@ -110,5 +110,5 @@ To enable device hot-plug into the bridge on Linux there're 
3 ways:
  Implementation
  ==
  The PCIE-PCI bridge is based on PCI-PCI bridge, but also accumulates PCI 
Express
-features as a PCI Express device (is_express=1).
+features as a PCI Express device (pci_dev->cap_present & QEMU_PCI_CAP_EXPRESS 
!= 0).
  
diff --git a/hw/block/nvme.c b/hw/block/nvme.c

index 441e21ed1f..9325bc0911 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1087,7 +1087,6 @@ static void nvme_class_init(ObjectClass *oc, void *data)
  pc->vendor_id = PCI_VENDOR_ID_INTEL;
  pc->device_id = 0x5845;
  pc->revision = 2;
-pc->is_express = 1;
  
  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);

  dc->desc = "Non-Volatile Memory Express";
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index f1af279e8d..c360f0d8c9 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -675,7 +675,6 @@ static void e1000e_class_init(ObjectClass *class, void 
*data)
  c->revision = 0;
  c->romfile = "efi-e1000e.rom";
  c->class_id = PCI_CLASS_NETWORK_ETHERNET;
-c->is_express = 1;
  
  dc->desc = "Intel 82574L GbE Controller";

  dc->reset = e1000e_qdev_reset;
diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
index a4d827c99d..b7d9ebbec2 100644
--- a/hw/pci-bridge/pcie_pci_bridge.c
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -169,7 +169,6 @@ static void pcie_pci_bridge_class_init(ObjectClass *klass, 
void *data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
  
-k->is_express = 1;

  k->is_bridge = 1;
  k->vendor_id = PCI_VENDOR_ID_REDHAT;
  k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE;
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 9b6e4ce512..45f9e8cd4a 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -145,7 +145,6 @@ static void rp_class_init(ObjectClass *klass, void *data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
  
-k->is_express = 1;

  k->is_bridge = 1;
  k->config_write = rp_write_config;
  k->realize = rp_realize;
diff --git a/hw/pci-bridge/xio3130_downstream.c 
b/hw/pci-bridge/xio3130_downstream.c
index 1e09d2afb7..613a0d6bb7 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -177,7 +177,6 @@ static void xio3130_downstream_class_init(ObjectClass 
*klass, void *data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
  
-k->is_express = 1;

  k->is_bridge = 1;
  k->config_write = xio3130_downstream_write_config;
  k->realize = xio3130_downstream_realize;
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 227997ce46..d4645bddee 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -148,7 +148,6 @@ static void xio3130_upstream_class_init(ObjectClass *klass, 
void *data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
  
-k->is_express = 1;

Re: [Qemu-block] [PATCH v2] pci: removed the is_express field since a uniform interface was inserted

2017-12-05 Thread Marcel Apfelbaum 

Hi Yoni,

Thanks for the patch.

On 04/12/2017 12:18, Yoni Bettan wrote:

 * according to Eduardo Habkost's commit
   fd3b02c8896d597dd8b9e053dec579cf0386aee1

 * since all PCIEs now implement INTERFACE_PCIE_DEVICE we
   don't need this field anymore

 * Devices that where only INTERFACE_PCIE_DEVICE (is_express == 1)
   or
   devices that where only INTERFACE_CONVENTIONAL_PCI_DEVICE 
(is_express == 0)
   where not affected by the change

   The only devices that were affected are those that are hybrid and 
also
   had (is_express == 1) - therefor only:
 - hw/vfio/pci.c
 - hw/usb/hcd-xhci.c



Did you test the above devices? The other ones you don't need to check
because the new condition does not affect the flow.



   For both i made sure that pci_dev->cap_present |= 
QEMU_PCI_CAP_EXPRESS



Please avoid "code like" sentences in commit description,
try to describe what you rather then how.
(and i -> I)



Signed-off-by: Yoni Bettan 
---
  docs/pcie_pci_bridge.txt   |  2 +-
  hw/block/nvme.c|  1 -
  hw/net/e1000e.c|  1 -
  hw/pci-bridge/pcie_pci_bridge.c|  1 -
  hw/pci-bridge/pcie_root_port.c |  1 -
  hw/pci-bridge/xio3130_downstream.c |  1 -
  hw/pci-bridge/xio3130_upstream.c   |  1 -
  hw/pci-host/xilinx-pcie.c  |  1 -
  hw/pci/pci.c   |  4 +++-
  hw/scsi/megasas.c  |  4 
  hw/usb/hcd-xhci.c  | 28 ++--
  hw/vfio/pci.c  | 37 +++--
  include/hw/pci/pci.h   |  3 ---
  include/hw/usb.h   |  1 +
  14 files changed, 62 insertions(+), 24 deletions(-)

diff --git a/docs/pcie_pci_bridge.txt b/docs/pcie_pci_bridge.txt
index 5a4203f97c..55f833ec10 100644
--- a/docs/pcie_pci_bridge.txt
+++ b/docs/pcie_pci_bridge.txt
@@ -110,5 +110,5 @@ To enable device hot-plug into the bridge on Linux there're 
3 ways:
  Implementation
  ==
  The PCIE-PCI bridge is based on PCI-PCI bridge, but also accumulates PCI 
Express
-features as a PCI Express device (is_express=1).
+features as a PCI Express device (pci_dev->cap_present & QEMU_PCI_CAP_EXPRESS 
!= 0).
  
diff --git a/hw/block/nvme.c b/hw/block/nvme.c

index 441e21ed1f..9325bc0911 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1087,7 +1087,6 @@ static void nvme_class_init(ObjectClass *oc, void *data)
  pc->vendor_id = PCI_VENDOR_ID_INTEL;
  pc->device_id = 0x5845;
  pc->revision = 2;
-pc->is_express = 1;
  
  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);

  dc->desc = "Non-Volatile Memory Express";
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index f1af279e8d..c360f0d8c9 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -675,7 +675,6 @@ static void e1000e_class_init(ObjectClass *class, void 
*data)
  c->revision = 0;
  c->romfile = "efi-e1000e.rom";
  c->class_id = PCI_CLASS_NETWORK_ETHERNET;
-c->is_express = 1;
  
  dc->desc = "Intel 82574L GbE Controller";

  dc->reset = e1000e_qdev_reset;
diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
index a4d827c99d..b7d9ebbec2 100644
--- a/hw/pci-bridge/pcie_pci_bridge.c
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -169,7 +169,6 @@ static void pcie_pci_bridge_class_init(ObjectClass *klass, 
void *data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
  
-k->is_express = 1;

  k->is_bridge = 1;
  k->vendor_id = PCI_VENDOR_ID_REDHAT;
  k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE;
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 9b6e4ce512..45f9e8cd4a 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -145,7 +145,6 @@ static void rp_class_init(ObjectClass *klass, void *data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
  
-k->is_express = 1;

  k->is_bridge = 1;
  k->config_write = rp_write_config;
  k->realize = rp_realize;
diff --git a/hw/pci-bridge/xio3130_downstream.c 
b/hw/pci-bridge/xio3130_downstream.c
index 1e09d2afb7..613a0d6bb7 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -177,7 +177,6 @@ static void xio3130_downstream_class_init(ObjectClass 
*klass, void *data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
  
-k->is_express = 1;

  k->is_bridge = 1;
  k->config_write = xio3130_downstream_write_config;
  k->realize = xio3130_downstream_realize;
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 227997ce46..d4645bddee 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -148,7 +148,6 @@

Re: [Qemu-block] [PATCH v2 4/5] pci: Add INTERFACE_CONVENTIONAL_PCI_DEVICE to Conventional PCI devices

2017-09-28 Thread Marcel Apfelbaum 

On 27/09/2017 22:56, Eduardo Habkost wrote:

Add INTERFACE_CONVENTIONAL_PCI_DEVICE to all direct subtypes of
TYPE_PCI_DEVICE, except:

1) The ones that already have INTERFACE_PCIE_DEVICE set:

* base-xhci
* e1000e
* nvme
* pvscsi
* vfio-pci
* virtio-pci
* vmxnet3

2) base-pci-bridge

Not all PCI bridges are Conventional PCI devices, so
INTERFACE_CONVENTIONAL_PCI_DEVICE is added only to the subtypes
that are actually Conventional PCI:

* dec-21154-p2p-bridge
* i82801b11-bridge
* pbm-bridge
* pci-bridge

The direct subtypes of base-pci-bridge not touched by this patch
are:

* xilinx-pcie-root: Already marked as PCIe-only.
* pcie-pci-bridge: Already marked as PCIe-only.
* pcie-port: all non-abstract subtypes of pcie-port are already
   marked as PCIe-only devices.

3) megasas-base

Not all megasas devices are Conventional PCI devices, so the
interface names are added to the subclasses registered by
megasas_register_types(), according to information in the
megasas_devices[] array.

"megasas-gen2" already implements INTERFACE_PCIE_DEVICE, so add
INTERFACE_CONVENTIONAL_PCI_DEVICE only to "megasas".

Acked-by: Alberto Garcia <be...@igalia.com>
Acked-by: John Snow <js...@redhat.com>
Acked-by: Anthony PERARD <anthony.per...@citrix.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* s/legacy/conventional/
   * Suggested-by: Alex Williamson <alex.william...@redhat.com>
* Note about pcie-pci-bridge on commit message.
* New devices: sungem, sunhme

Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Igor Mammedov <imamm...@redhat.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Richard Henderson <r...@twiddle.net>
Cc: Eduardo Habkost <ehabk...@redhat.com>
Cc: Stefano Stabellini <sstabell...@kernel.org>
Cc: Anthony Perard <anthony.per...@citrix.com>
Cc: John Snow <js...@redhat.com>
Cc: Alberto Garcia <be...@igalia.com>
Cc: Aurelien Jarno <aurel...@aurel32.net>
Cc: Yongbok Kim <yongbok@imgtec.com>
Cc: Jiri Slaby <jsl...@suse.cz>
Cc: Alexander Graf <ag...@suse.de>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Cc: Jason Wang <jasow...@redhat.com>
Cc: Jiri Pirko <j...@resnulli.us>
Cc: "Hervé Poussineau" <hpous...@reactos.org>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: David Gibson <da...@gibson.dropbear.id.au>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
Cc: Artyom Tarasenko <atar4q...@gmail.com>
Cc: Alex Williamson <alex.william...@redhat.com>
Cc: qemu-de...@nongnu.org
Cc: xen-de...@lists.xenproject.org
Cc: qemu-block@nongnu.org
Cc: qemu-...@nongnu.org
Cc: qemu-...@nongnu.org
---
  hw/acpi/piix4.c |  1 +
  hw/audio/ac97.c |  4 
  hw/audio/es1370.c   |  4 
  hw/audio/intel-hda.c|  4 
  hw/char/serial-pci.c| 12 
  hw/display/cirrus_vga.c |  4 
  hw/display/qxl.c|  4 
  hw/display/sm501.c  |  4 
  hw/display/vga-pci.c|  4 
  hw/display/vmware_vga.c |  4 
  hw/i2c/smbus_ich9.c |  4 
  hw/i386/amd_iommu.c |  4 
  hw/i386/kvm/pci-assign.c|  4 
  hw/i386/pc_piix.c   |  4 
  hw/i386/xen/xen_platform.c  |  4 
  hw/i386/xen/xen_pvdevice.c  |  4 
  hw/ide/ich.c|  4 
  hw/ide/pci.c|  4 
  hw/ipack/tpci200.c  |  4 
  hw/isa/i82378.c |  4 
  hw/isa/lpc_ich9.c   |  1 +
  hw/isa/piix4.c  |  4 
  hw/isa/vt82c686.c   | 16 
  hw/mips/gt64xxx_pci.c   |  4 
  hw/misc/edu.c   |  5 +
  hw/misc/ivshmem.c   |  4 
  hw/misc/macio/macio.c   |  4 
  hw/misc/pci-testdev.c   |  4 
  hw/net/e1000.c  |  4 
  hw/net/eepro100.c   |  4 
  hw/net/ne2000.c |  4 
  hw/net/pcnet-pci.c  |  4 
  hw/net/rocker/rocker.c  |  4 
  hw/net/rtl8139.c|  4 
  hw/net/sungem.c |  4 
  hw/net/sunhme.c |  4 
  hw/pci-bridge/dec.c |  8 
  hw/pci-bridge/i82801b11.c   |  4 
  hw/pci-bridge/pci_bridge_dev.c  |  1 +
  hw/pci-bridge/pci_expander_bridge.c |  8 
  hw/pci-host/apb.c   |  8 
  hw/pci-host/bonito.c|  4 
  hw/pci-host/gpex.c  |  4 
  hw/pci-host/grackle.c   |  4 
  hw/pci-host/piix.c

Re: [Qemu-block] [PATCH v2 3/5] pci: Add INTERFACE_PCIE_DEVICE to all PCIe devices

2017-09-28 Thread Marcel Apfelbaum 

On 27/09/2017 22:56, Eduardo Habkost wrote:

Change all devices that set is_express=1 to implement
INTERFACE_PCIE_DEVICE.

Cc: Keith Busch <keith.bu...@intel.com>
Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: Dmitry Fleytman <dmi...@daynix.com>
Cc: Jason Wang <jasow...@redhat.com>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Cc: Paul Burton <paul.bur...@imgtec.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: qemu-block@nongnu.org
Reviewed-by: Alistair Francis <alistair.fran...@xilinx.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* base-xhci is marked as hybrid, now (in another patch)
* Included pcie-pci-bridge
---
  hw/block/nvme.c| 4 
  hw/net/e1000e.c| 4 
  hw/pci-bridge/pcie_pci_bridge.c| 1 +
  hw/pci-bridge/pcie_root_port.c | 4 
  hw/pci-bridge/xio3130_downstream.c | 4 
  hw/pci-bridge/xio3130_upstream.c   | 4 
  hw/pci-host/xilinx-pcie.c  | 4 
  hw/scsi/megasas.c  | 6 ++
  8 files changed, 31 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9aa32692a3..441e21ed1f 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1110,6 +1110,10 @@ static const TypeInfo nvme_info = {
  .instance_size = sizeof(NvmeCtrl),
  .class_init= nvme_class_init,
  .instance_init = nvme_instance_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
  };
  
  static void nvme_register_types(void)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 6c42b4478c..81f7934a59 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -708,6 +708,10 @@ static const TypeInfo e1000e_info = {
  .instance_size = sizeof(E1000EState),
  .class_init = e1000e_class_init,
  .instance_init = e1000e_instance_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
  };
  
  static void e1000e_register_types(void)

diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
index 9aa5cc3e45..88db143633 100644
--- a/hw/pci-bridge/pcie_pci_bridge.c
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -180,6 +180,7 @@ static const TypeInfo pcie_pci_bridge_info = {
  .class_init = pcie_pci_bridge_class_init,
  .interfaces = (InterfaceInfo[]) {
  { TYPE_HOTPLUG_HANDLER },
+{ INTERFACE_PCIE_DEVICE },
  { },
  }
  };
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 4d588cb22e..9b6e4ce512 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -161,6 +161,10 @@ static const TypeInfo rp_info = {
  .class_init= rp_class_init,
  .abstract  = true,
  .class_size = sizeof(PCIERootPortClass),
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
  };
  
  static void rp_register_types(void)

diff --git a/hw/pci-bridge/xio3130_downstream.c 
b/hw/pci-bridge/xio3130_downstream.c
index e706f36cb7..7d2f7629c1 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -195,6 +195,10 @@ static const TypeInfo xio3130_downstream_info = {
  .name  = "xio3130-downstream",
  .parent= TYPE_PCIE_SLOT,
  .class_init= xio3130_downstream_class_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
  };
  
  static void xio3130_downstream_register_types(void)

diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index a052224bbf..227997ce46 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -166,6 +166,10 @@ static const TypeInfo xio3130_upstream_info = {
  .name  = "x3130-upstream",
  .parent= TYPE_PCIE_PORT,
  .class_init= xio3130_upstream_class_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
  };
  
  static void xio3130_upstream_register_types(void)

diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
index 4613dda1d2..7659253090 100644
--- a/hw/pci-host/xilinx-pcie.c
+++ b/hw/pci-host/xilinx-pcie.c
@@ -317,6 +317,10 @@ static const TypeInfo xilinx_pcie_root_info = {
  .parent = TYPE_PCI_BRIDGE,
  .instance_size = sizeof(XilinxPCIERoot),
  .class_init = xilinx_pcie_root_class_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
  };
  
  static void xilinx_pcie_register(void)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 0db68aacee..535ee267c3 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2451,6 +2451,7 @@ typedef struct MegasasInfo {
  int osts;

Re: [Qemu-block] [RFC PATCH 13/56] pci: Make PCI addresses and sizes unsigned in QAPI/QMP

2017-08-22 Thread Marcel Apfelbaum 

Hi Markus,

On 07/08/2017 17:45, Markus Armbruster wrote:

Sizes and addresses should use QAPI type 'size' (uint64_t).
PciMemoryRegion members @address and @size are 'int' (int64_t).
qmp_query_pci_regions() implicitly converts from pcibus_t,
i.e. uint64_t.

Change these PciMemoryRegion members to 'size'.

query-pci now reports sizes and addresses above 2^63-1 correctly
instead of their (negative) two's complement.

HMP's "info pci" already reported them correctly, because it
implicitly converted back to uint64_t.

Signed-off-by: Markus Armbruster <arm...@redhat.com>
---
  qapi-schema.json | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 6aa6be9..c8cceb9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2062,7 +2062,7 @@
  # Since: 0.14.0
  ##
  { 'struct': 'PciMemoryRegion',
-  'data': {'bar': 'int', 'type': 'str', 'address': 'int', 'size': 'int',
+  'data': {'bar': 'int', 'type': 'str', 'address': 'size', 'size': 'size',
 '*prefetch': 'bool', '*mem_type_64': 'bool' } }
  
  ##




Reviewed-by: Marcel Apfelbaum <mar...@redhat.com>

Thanks,
Marcel

Re: [Qemu-block] [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

2016-03-03 Thread Marcel Apfelbaum 

On 03/03/2016 12:45 PM, Michael S. Tsirkin wrote:

On Thu, Mar 03, 2016 at 12:12:27PM +0200, Marcel Apfelbaum wrote:

+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
+ bool msi64bit, bool msi_per_vector_mask, Error **errp)
  {
  unsigned int vectors_order;
-uint16_t flags;
+uint16_t flags; /* Message Control register value */
  uint8_t cap_size;
  int config_offset;

  if (!msi_supported) {
+error_setg(errp, "MSI is not supported by interrupt controller");
  return -ENOTSUP;


First failure mode: board does not support MSI (-ENOTSUP).

Question to the PCI guys: why is this even an error?  A device with
capability MSI should work just fine in such a board.


Hi Markus,

Adding Jan Kiszka, maybe he can help.

That's a fair question. Is there any history for this decision?
The board not supporting MSI has nothing to do with the capability being there.
The HW should not change because the board doe not support it.

The capability should be present but not active.


Digging in git log will tell you why we have the msi_supported flag:

commit 02eb84d0ec97f183ac23ee939403a139e8849b1d ("qemu/pci: MSI-X support 
functions")

This is a safety measure to avoid breaking platforms which should 
support
MSI-X but currently lack this in the interrupt controller emulation.

in other words, on some platforms we must hide msi support from devices
because otherwise guests will try to use it, and our emulation is
incomplete.



OK, thanks. So the flag should be "msi_broken" or 
"msi_present_but_not_implemented" and not
"msi_supported" that leads (at least me) to the assumption that some platform 
*does not support msi*
rather than it supports it, but we don't emulate it.




And the conclusion from that is that for msi_init to fail silently is
at the moment the right thing to do.


But this is not the only thing we do, we are modifying the PCI devices. We 
could fail starting the VM
if a device supporting MSI is added on a platform with broken msi, but this 
will prevent us to use
assigned devices. Emulated devices should be created with a specific "msi=off" 
flag.

Thanks,
Marcel



The only other reason for it to fail is pci config space corruption,
this probably never happens in practice.

Re: [Qemu-block] [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

2016-03-03 Thread Marcel Apfelbaum 

On 03/02/2016 11:13 AM, Markus Armbruster wrote:

This got lost over the Christmas break, sorry.

Cc'ing Marcel for additional PCI expertise.

Cao jin  writes:


msi_init() is a supporting function in PCI device initialization,
in order to convert .init() to .realize(), it should be modified first.


"Supporting function" doesn't imply "should use Error to report
errors".  HACKING explains:

 Use the simplest suitable method to communicate success / failure to
 callers.  Stick to common methods: non-negative on success / -1 on
 error, non-negative / -errno, non-null / null, or Error objects.

 Example: when a function returns a non-null pointer on success, and it
 can fail only in one way (as far as the caller is concerned), returning
 null on failure is just fine, and certainly simpler and a lot easier on
 the eyes than propagating an Error object through an Error ** parameter.

 Example: when a function's callers need to report details on failure
 only the function really knows, use Error **, and set suitable errors.

 Do not report an error to the user when you're also returning an error
 for somebody else to handle.  Leave the reporting to the place that
 consumes the error returned.

As we'll see in your patch of msi.c, msi_init() can fail in multiple
ways, and uses -errno to comunicate them.  That can be okay even in
realize().  It also reports to the user.  That's what makes it
unsuitable for realize().


Also modify the callers


Actually, you're *fixing* callers!  But the bugs aren't 100% clear, yet,
see below for details.  Once we know what the bugs are, we'll want to
reword the commit message to describe the bugs and their impact.

I recommend to skip ahead to msi.c, then come back to the device models.


Bonus: add more comment for msi_init().
Signed-off-by: Cao jin 
---
  hw/audio/intel-hda.c   | 10 -
  hw/ide/ich.c   |  2 +-
  hw/net/vmxnet3.c   | 13 +++---
  hw/pci-bridge/ioh3420.c|  7 +++-
  hw/pci-bridge/pci_bridge_dev.c |  8 +++-
  hw/pci-bridge/xio3130_downstream.c |  8 +++-
  hw/pci-bridge/xio3130_upstream.c   |  8 +++-
  hw/pci/msi.c   | 18 +++--
  hw/scsi/megasas.c  | 15 +--
  hw/scsi/vmw_pvscsi.c   | 13 --
  hw/usb/hcd-xhci.c  | 81 +-
  hw/vfio/pci.c  | 20 +-
  include/hw/pci/msi.h   |  4 +-
  13 files changed, 135 insertions(+), 72 deletions(-)



[...]


Except I'm not sure the function should fail in the first place!  See
below.


+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
+ bool msi64bit, bool msi_per_vector_mask, Error **errp)
  {
  unsigned int vectors_order;
-uint16_t flags;
+uint16_t flags; /* Message Control register value */
  uint8_t cap_size;
  int config_offset;

  if (!msi_supported) {
+error_setg(errp, "MSI is not supported by interrupt controller");
  return -ENOTSUP;


First failure mode: board does not support MSI (-ENOTSUP).

Question to the PCI guys: why is this even an error?  A device with
capability MSI should work just fine in such a board.


Hi Markus,

Adding Jan Kiszka, maybe he can help.

That's a fair question. Is there any history for this decision?
The board not supporting MSI has nothing to do with the capability being there.
The HW should not change because the board doe not support it.

The capability should be present but not active.




  }

@@ -182,7 +190,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
  }

  cap_size = msi_cap_sizeof(flags);
-config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size);
+config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset,
+cap_size, errp);


pci_add_capability() is a wrapper around pci_add_capability2() that
additionally reports errors with error_report_err().  This is what makes
it unsuitable for realize().


  if (config_offset < 0) {
  return config_offset;


Inherits failing modes from pci_add_capability2().  Two: out of PCI
config space (-ENOSPC), and capability overlap (-EINVAL).

Question for the PCI guys: how can these happen?  When they happen, is
it a programming error?


out of PCI config space: a device emulation error, not enough room
for all its capabilities - it seems to be a programming error.

capability overlap: is for device assignment. This checks for a real HW
that is broke. - not a programming error.




  }
@@ -205,6 +214,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
  pci_set_long(dev->wmask + msi_mask_off(dev, msi64bit),
   0x >> (PCI_MSI_VECTORS_MAX - nr_vectors));
  }
+
  return config_offset;
  }




Thanks,
Marcel