Re: [PATCH v2 4/4] bus: Make remove callback return void

2021-07-06 Thread Cornelia Huck
On Tue, Jul 06 2021, Uwe Kleine-König  wrote:

> The driver core ignores the return value of this callback because there
> is only little it can do when a device disappears.
>
> This is the final bit of a long lasting cleanup quest where several
> buses were converted to also return void from their remove callback.
> Additionally some resource leaks were fixed that were caused by drivers
> returning an error code in the expectation that the driver won't go
> away.
>
> With struct bus_type::remove returning void it's prevented that newly
> implemented buses return an ignored error code and so don't anticipate
> wrong expectations for driver authors.
>
> Acked-by: Russell King (Oracle)  (For ARM, Amba 
> and related parts)
> Acked-by: Mark Brown 
> Acked-by: Chen-Yu Tsai  (for drivers/bus/sunxi-rsb.c)
> Acked-by: Pali Rohár 
> Acked-by: Mauro Carvalho Chehab  (for drivers/media)
> Acked-by: Hans de Goede  (For drivers/platform)
> Acked-by: Alexandre Belloni 
> Acked-By: Vinod Koul 
> Acked-by: Juergen Gross  (For Xen)
> Acked-by: Lee Jones  (For drivers/mfd)
> Acked-by: Johannes Thumshirn  (For drivers/mcb)
> Acked-by: Johan Hovold 
> Acked-by: Srinivas Kandagatla  (For 
> drivers/slimbus)
> Acked-by: Kirti Wankhede  (For drivers/vfio)
> Acked-by: Maximilian Luz 
> Acked-by: Heikki Krogerus  (For ulpi and 
> typec)
> Acked-by: Samuel Iglesias Gonsálvez  (For ipack)
> Reviewed-by: Tom Rix  (For fpga)
> Acked-by: Geoff Levand  (For ps3)
> Signed-off-by: Uwe Kleine-König 
> ---
>
>  drivers/s390/cio/ccwgroup.c   | 4 +---
>  drivers/s390/cio/css.c| 4 +---
>  drivers/s390/cio/device.c | 4 +---
>  drivers/s390/cio/scm.c| 4 +---
>  drivers/s390/crypto/ap_bus.c      | 4 +---
>  drivers/vfio/mdev/mdev_driver.c   | 4 +---

For drivers/s390 and drivers/vfio:

Reviewed-by: Cornelia Huck 




Re: [PATCH] bus: Make remove callback return void

2021-07-06 Thread Cornelia Huck
On Tue, Jul 06 2021, Cornelia Huck  wrote:

> On Tue, Jul 06 2021, Uwe Kleine-König  wrote:
>
>> The driver core ignores the return value of this callback because there
>> is only little it can do when a device disappears.
>>
>> This is the final bit of a long lasting cleanup quest where several
>> buses were converted to also return void from their remove callback.
>> Additionally some resource leaks were fixed that were caused by drivers
>> returning an error code in the expectation that the driver won't go
>> away.
>>
>> With struct bus_type::remove returning void it's prevented that newly
>> implemented buses return an ignored error code and so don't anticipate
>> wrong expectations for driver authors.
>
> Yay!
>
>>
>> Signed-off-by: Uwe Kleine-König 
>> ---
>> Hello,
>>
>> this patch depends on "PCI: endpoint: Make struct pci_epf_driver::remove
>> return void" that is not yet applied, see
>> https://lore.kernel.org/r/20210223090757.57604-1-u.kleine-koe...@pengutronix.de.
>>
>> I tested it using allmodconfig on amd64 and arm, but I wouldn't be
>> surprised if I still missed to convert a driver. So it would be great to
>> get this into next early after the merge window closes.
>
> I'm afraid you missed the s390-specific busses in drivers/s390/cio/
> (css/ccw/ccwgroup).

The change for vfio/mdev looks good.

The following should do the trick for s390; not sure if other
architectures have easy-to-miss busses as well.

diff --git a/drivers/s390/cio/ccwgroup.c b/drivers/s390/cio/ccwgroup.c
index 9748165e08e9..a66f416138ab 100644
--- a/drivers/s390/cio/ccwgroup.c
+++ b/drivers/s390/cio/ccwgroup.c
@@ -439,17 +439,15 @@ module_exit(cleanup_ccwgroup);
 
 /** driver stuff **/
 
-static int ccwgroup_remove(struct device *dev)
+static void ccwgroup_remove(struct device *dev)
 {
struct ccwgroup_device *gdev = to_ccwgroupdev(dev);
struct ccwgroup_driver *gdrv = to_ccwgroupdrv(dev->driver);
 
if (!dev->driver)
-   return 0;
+   return;
if (gdrv->remove)
gdrv->remove(gdev);
-
-   return 0;
 }
 
 static void ccwgroup_shutdown(struct device *dev)
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
index a974943c27da..ebc321edba51 100644
--- a/drivers/s390/cio/css.c
+++ b/drivers/s390/cio/css.c
@@ -1371,15 +1371,14 @@ static int css_probe(struct device *dev)
return ret;
 }
 
-static int css_remove(struct device *dev)
+static void css_remove(struct device *dev)
 {
struct subchannel *sch;
-   int ret;
 
sch = to_subchannel(dev);
-   ret = sch->driver->remove ? sch->driver->remove(sch) : 0;
+   if (sch->driver->remove)
+   sch->driver->remove(sch);
sch->driver = NULL;
-   return ret;
 }
 
 static void css_shutdown(struct device *dev)
diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
index 84f659cafe76..61d5d55bd9c8 100644
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -1742,7 +1742,7 @@ ccw_device_probe (struct device *dev)
return 0;
 }
 
-static int ccw_device_remove(struct device *dev)
+static void ccw_device_remove(struct device *dev)
 {
struct ccw_device *cdev = to_ccwdev(dev);
struct ccw_driver *cdrv = cdev->drv;
@@ -1776,8 +1776,6 @@ static int ccw_device_remove(struct device *dev)
spin_unlock_irq(cdev->ccwlock);
io_subchannel_quiesce(sch);
__disable_cmf(cdev);
-
-   return 0;
 }
 
 static void ccw_device_shutdown(struct device *dev)
diff --git a/drivers/s390/cio/scm.c b/drivers/s390/cio/scm.c
index 9f26d4310bb3..b6b4589c70bd 100644
--- a/drivers/s390/cio/scm.c
+++ b/drivers/s390/cio/scm.c
@@ -28,12 +28,13 @@ static int scmdev_probe(struct device *dev)
return scmdrv->probe ? scmdrv->probe(scmdev) : -ENODEV;
 }
 
-static int scmdev_remove(struct device *dev)
+static void scmdev_remove(struct device *dev)
 {
struct scm_device *scmdev = to_scm_dev(dev);
struct scm_driver *scmdrv = to_scm_drv(dev->driver);
 
-   return scmdrv->remove ? scmdrv->remove(scmdev) : -ENODEV;
+   if (scmdrv->remove)
+   scmdrv->remove(scmdev);
 }
 
 static int scmdev_uevent(struct device *dev, struct kobj_uevent_env *env)
diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index d2560186d771..8a0d37c0e2a5 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -884,7 +884,7 @@ static int ap_device_probe(struct device *dev)
return rc;
 }
 
-static int ap_device_remove(struct device *dev)
+static void ap_device_remove(struct device *dev)
 {
struct ap_device *ap_dev = to_ap_dev(dev);
struct ap_driver *ap_drv = ap_dev->drv;
@@ -909,8 +909,6 @@ static int ap_device_remove(struct device *dev)
ap_dev->drv = NULL;
 
put_device(dev);
-
-   return 0;
 }
 
 struct ap_queue *ap_get_qdev(ap_qid_t qid)




Re: [PATCH] bus: Make remove callback return void

2021-07-06 Thread Cornelia Huck
On Tue, Jul 06 2021, Uwe Kleine-König  wrote:

> The driver core ignores the return value of this callback because there
> is only little it can do when a device disappears.
>
> This is the final bit of a long lasting cleanup quest where several
> buses were converted to also return void from their remove callback.
> Additionally some resource leaks were fixed that were caused by drivers
> returning an error code in the expectation that the driver won't go
> away.
>
> With struct bus_type::remove returning void it's prevented that newly
> implemented buses return an ignored error code and so don't anticipate
> wrong expectations for driver authors.

Yay!

>
> Signed-off-by: Uwe Kleine-König 
> ---
> Hello,
>
> this patch depends on "PCI: endpoint: Make struct pci_epf_driver::remove
> return void" that is not yet applied, see
> https://lore.kernel.org/r/20210223090757.57604-1-u.kleine-koe...@pengutronix.de.
>
> I tested it using allmodconfig on amd64 and arm, but I wouldn't be
> surprised if I still missed to convert a driver. So it would be great to
> get this into next early after the merge window closes.

I'm afraid you missed the s390-specific busses in drivers/s390/cio/
(css/ccw/ccwgroup).




Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

2021-02-23 Thread Cornelia Huck
On Tue, 23 Feb 2021 10:37:01 +1100
David Gibson  wrote:

> On Tue, Feb 23, 2021 at 10:33:55AM +1100, David Gibson wrote:
> > On Mon, Feb 22, 2021 at 06:50:44PM +0100, Cornelia Huck wrote:  
> > > On Mon, 22 Feb 2021 18:41:07 +0100
> > > Philippe Mathieu-Daudé  wrote:
> > >   
> > > > On 2/22/21 6:24 PM, Cornelia Huck wrote:  
> > > > > On Fri, 19 Feb 2021 18:38:37 +0100
> > > > > Philippe Mathieu-Daudé  wrote:
> > > > > 
> > > > >> MachineClass::kvm_type() can return -1 on failure.
> > > > >> Document it, and add a check in kvm_init().
> > > > >>
> > > > >> Signed-off-by: Philippe Mathieu-Daudé 
> > > > >> ---
> > > > >>  include/hw/boards.h | 3 ++-
> > > > >>  accel/kvm/kvm-all.c | 6 ++
> > > > >>  2 files changed, 8 insertions(+), 1 deletion(-)
> > > > >>
> > > > >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > >> index a46dfe5d1a6..68d3d10f6b0 100644
> > > > >> --- a/include/hw/boards.h
> > > > >> +++ b/include/hw/boards.h
> > > > >> @@ -127,7 +127,8 @@ typedef struct {
> > > > >>   *implement and a stub device is required.
> > > > >>   * @kvm_type:
> > > > >>   *Return the type of KVM corresponding to the kvm-type string 
> > > > >> option or
> > > > >> - *computed based on other criteria such as the host kernel 
> > > > >> capabilities.
> > > > >> + *computed based on other criteria such as the host kernel 
> > > > >> capabilities
> > > > >> + *(which can't be negative), or -1 on error.
> > > > >>   * @numa_mem_supported:
> > > > >>   *true if '--numa node.mem' option is supported and false 
> > > > >> otherwise
> > > > >>   * @smp_parse:
> > > > >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > > > >> index 84c943fcdb2..b069938d881 100644
> > > > >> --- a/accel/kvm/kvm-all.c
> > > > >> +++ b/accel/kvm/kvm-all.c
> > > > >> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
> > > > >>  
> > > > >> "kvm-type",
> > > > >>  
> > > > >> _abort);
> > > > >>  type = mc->kvm_type(ms, kvm_type);
> > > > >> +if (type < 0) {
> > > > >> +ret = -EINVAL;
> > > > >> +fprintf(stderr, "Failed to detect kvm-type for machine 
> > > > >> '%s'\n",
> > > > >> +mc->name);
> > > > >> +goto err;
> > > > >> +}
> > > > >>  }
> > > > >>  
> > > > >>  do {
> > > > > 
> > > > > No objection to this patch; but I'm wondering why some non-pseries
> > > > > machines implement the kvm_type callback, when I see the kvm-type
> > > > > property only for pseries? Am I holding my git grep wrong?
> > > > 
> > > > Can it be what David commented here?
> > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html
> > > >   
> > > 
> > > Ok, I might be confused about the other ppc machines; but I'm wondering
> > > about the kvm_type callback for mips and arm/virt. Maybe I'm just
> > > confused by the whole mechanism?  
> > 
> > For ppc at least, not sure about in general, pseries is the only
> > machine type that can possibly work under more than one KVM flavour
> > (HV or PR).  So, it's the only one where it's actually useful to be
> > able to configure this.  
> 
> Wait... I'm not sure that's true.  At least theoretically, some of the
> Book3E platforms could work with either PR or the Book3E specific
> KVM.  Not sure if KVM PR supports all the BookE instructions it would
> need to in practice.
> 
> Possibly pseries is just the platform where there's been enough people
> interested in setting the KVM flavour so far.

If I'm not utterly confused by the code, it seems the pseries machines
are the only ones where you can actually get to an invocation of
->kvm_type(): You need to have a 'kvm-type' machine property, and
AFAICS only the pseries machine has that.

(Or is something hiding behind some macro magic?)


pgpc1rJ17FYph.pgp
Description: OpenPGP digital signature


Re: [PATCH v2 02/11] hw/boards: Introduce machine_class_valid_for_accelerator()

2021-02-22 Thread Cornelia Huck
On Mon, 22 Feb 2021 18:46:15 +0100
Philippe Mathieu-Daudé  wrote:

> On 2/22/21 6:34 PM, Cornelia Huck wrote:
> > On Fri, 19 Feb 2021 18:38:38 +0100
> > Philippe Mathieu-Daudé  wrote:
> >   
> >> Introduce the valid_accelerators[] field to express the list
> >> of valid accelators a machine can use, and add the
> >> machine_class_valid_for_current_accelerator() and
> >> machine_class_valid_for_accelerator() methods.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé 
> >> ---
> >>  include/hw/boards.h | 24 
> >>  hw/core/machine.c   | 26 ++
> >>  2 files changed, 50 insertions(+)
> >>
> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >> index 68d3d10f6b0..4d08bc12093 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -36,6 +36,24 @@ void machine_set_cpu_numa_node(MachineState *machine,
> >> const CpuInstanceProperties *props,
> >> Error **errp);
> >>  
> >> +/**
> >> + * machine_class_valid_for_accelerator:
> >> + * @mc: the machine class
> >> + * @acc_name: accelerator name
> >> + *
> >> + * Returns %true if the accelerator is valid for the machine, %false
> >> + * otherwise. See #MachineClass.valid_accelerators.  
> > 
> > Naming confusion: is the machine class valid for the accelerator, or
> > the accelerator valid for the machine class? Or either? :)  
> 
> "the accelerator valid for the machine class".
> 
> Is this clearer?
> 
> "Returns %true if the current accelerator is valid for the
>  selected machine, %false otherwise.
> 
> Or...
> 
> "Returns %true if the selected accelerator is valid for the
>  current machine, %false otherwise.

Maybe that one, given how it ends up being called? Or "specified
machine"?
> 
> How would look "either"?
> 
> The machine is already selected, and the accelerator too...

Yes, so this is basically testing the (machine,accelerator) tuple,
which is what I meant with 'either'.

> 
> >   
> >> + */
> >> +bool machine_class_valid_for_accelerator(MachineClass *mc, const char 
> >> *acc_name);
> >> +/**
> >> + * machine_class_valid_for_current_accelerator:
> >> + * @mc: the machine class
> >> + *
> >> + * Returns %true if the accelerator is valid for the current machine,
> >> + * %false otherwise. See #MachineClass.valid_accelerators.  
> > 
> > Same here: current accelerator vs. current machine.

So maybe

"Returns %true if the current accelerator is valid for the specified
machine class..." ?

> >   
> >> + */
> >> +bool machine_class_valid_for_current_accelerator(MachineClass *mc);




Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

2021-02-22 Thread Cornelia Huck
On Mon, 22 Feb 2021 18:41:07 +0100
Philippe Mathieu-Daudé  wrote:

> On 2/22/21 6:24 PM, Cornelia Huck wrote:
> > On Fri, 19 Feb 2021 18:38:37 +0100
> > Philippe Mathieu-Daudé  wrote:
> >   
> >> MachineClass::kvm_type() can return -1 on failure.
> >> Document it, and add a check in kvm_init().
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé 
> >> ---
> >>  include/hw/boards.h | 3 ++-
> >>  accel/kvm/kvm-all.c | 6 ++
> >>  2 files changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >> index a46dfe5d1a6..68d3d10f6b0 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -127,7 +127,8 @@ typedef struct {
> >>   *implement and a stub device is required.
> >>   * @kvm_type:
> >>   *Return the type of KVM corresponding to the kvm-type string option 
> >> or
> >> - *computed based on other criteria such as the host kernel 
> >> capabilities.
> >> + *computed based on other criteria such as the host kernel 
> >> capabilities
> >> + *(which can't be negative), or -1 on error.
> >>   * @numa_mem_supported:
> >>   *true if '--numa node.mem' option is supported and false otherwise
> >>   * @smp_parse:
> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> >> index 84c943fcdb2..b069938d881 100644
> >> --- a/accel/kvm/kvm-all.c
> >> +++ b/accel/kvm/kvm-all.c
> >> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
> >>  "kvm-type",
> >>  _abort);
> >>  type = mc->kvm_type(ms, kvm_type);
> >> +if (type < 0) {
> >> +ret = -EINVAL;
> >> +fprintf(stderr, "Failed to detect kvm-type for machine 
> >> '%s'\n",
> >> +mc->name);
> >> +goto err;
> >> +}
> >>  }
> >>  
> >>  do {  
> > 
> > No objection to this patch; but I'm wondering why some non-pseries
> > machines implement the kvm_type callback, when I see the kvm-type
> > property only for pseries? Am I holding my git grep wrong?  
> 
> Can it be what David commented here?
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html
> 

Ok, I might be confused about the other ppc machines; but I'm wondering
about the kvm_type callback for mips and arm/virt. Maybe I'm just
confused by the whole mechanism?




Re: [PATCH v2 11/11] softmmu/vl: Exit gracefully when accelerator is not supported

2021-02-22 Thread Cornelia Huck
On Fri, 19 Feb 2021 18:38:47 +0100
Philippe Mathieu-Daudé  wrote:

> Before configuring an accelerator, check it is valid for
> the current machine. Doing so we can return a simple error
> message instead of criptic one.

s/criptic/cryptic/

> 
> Before:
> 
>   $ qemu-system-arm -M raspi2b -enable-kvm
>   qemu-system-arm: /build/qemu-ETIdrs/qemu-4.2/exec.c:865: 
> cpu_address_space_init: Assertion `asidx == 0 || !kvm_enabled()' failed.
>   Aborted
> 
>   $ qemu-system-aarch64 -M xlnx-zcu102 -enable-kvm -smp 6
>   qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid 
> argument
> 
> After:
> 
>   $ qemu-system-arm -M raspi2b -enable-kvm
>   qemu-system-aarch64: invalid accelerator 'kvm' for machine raspi2b
> 
>   $ qemu-system-aarch64 -M xlnx-zcu102 -enable-kvm -smp 6
>   qemu-system-aarch64: -accel kvm: invalid accelerator 'kvm' for machine 
> xlnx-zcu102
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  softmmu/vl.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index b219ce1f357..f2c4074310b 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2133,6 +2133,7 @@ static int do_configure_accelerator(void *opaque, 
> QemuOpts *opts, Error **errp)
>  const char *acc = qemu_opt_get(opts, "accel");
>  AccelClass *ac = accel_find(acc);
>  AccelState *accel;
> +MachineClass *mc;
>  int ret;
>  bool qtest_with_kvm;
>  
> @@ -2145,6 +2146,12 @@ static int do_configure_accelerator(void *opaque, 
> QemuOpts *opts, Error **errp)
>  }
>  return 0;
>  }
> +mc = MACHINE_GET_CLASS(current_machine);
> +if (!qtest_chrdev && !machine_class_valid_for_accelerator(mc, ac->name)) 
> {

Shouldn't qtest be already allowed in any case in the checking function?

> +*p_init_failed = true;
> +error_report("invalid accelerator '%s' for machine %s", acc, 
> mc->name);
> +return 0;
> +}
>  accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
>  object_apply_compat_props(OBJECT(accel));
>  qemu_opt_foreach(opts, accelerator_set_property,




Re: [PATCH v2 03/11] hw/core: Restrict 'query-machines' to those supported by current accel

2021-02-22 Thread Cornelia Huck
On Fri, 19 Feb 2021 18:38:39 +0100
Philippe Mathieu-Daudé  wrote:

> Do not let 'query-machines' return machines not valid with
> the current accelerator.
> 
> Suggested-by: Daniel Berrangé 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/core/machine-qmp-cmds.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 44e979e503b..c8630bc2ddc 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -204,6 +204,10 @@ MachineInfoList *qmp_query_machines(Error **errp)
>  MachineClass *mc = el->data;
>  MachineInfo *info;
>  
> +if (!machine_class_valid_for_current_accelerator(mc)) {
> +continue;
> +}
> +
>  info = g_malloc0(sizeof(*info));
>  if (mc->is_default) {
>  info->has_is_default = true;

Reviewed-by: Cornelia Huck 




Re: [PATCH v2 07/11] hw/s390x: Explicit the s390-ccw-virtio machines support TCG and KVM

2021-02-22 Thread Cornelia Huck
On Fri, 19 Feb 2021 18:38:43 +0100
Philippe Mathieu-Daudé  wrote:

I'd lose the 'Explicit' in $SUBJECT.


> All s390-ccw-virtio machines support TCG and KVM.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/s390x/s390-virtio-ccw.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 2972b607f36..1f168485066 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -586,6 +586,10 @@ static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
>  return newsz;
>  }
>  
> +static const char *const valid_accels[] = {
> +"tcg", "kvm", NULL
> +};
> +
>  static void ccw_machine_class_init(ObjectClass *oc, void *data)
>  {
>  MachineClass *mc = MACHINE_CLASS(oc);
> @@ -612,6 +616,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void 
> *data)
>  mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
>  /* it is overridden with 'host' cpu *in kvm_arch_init* */
>  mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu");
> +mc->valid_accelerators = valid_accels;
>  hc->plug = s390_machine_device_plug;
>  hc->unplug_request = s390_machine_device_unplug_request;
>  nc->nmi_monitor_handler = s390_nmi;

Reviewed-by: Cornelia Huck 




Re: [PATCH v2 02/11] hw/boards: Introduce machine_class_valid_for_accelerator()

2021-02-22 Thread Cornelia Huck
On Fri, 19 Feb 2021 18:38:38 +0100
Philippe Mathieu-Daudé  wrote:

> Introduce the valid_accelerators[] field to express the list
> of valid accelators a machine can use, and add the
> machine_class_valid_for_current_accelerator() and
> machine_class_valid_for_accelerator() methods.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/boards.h | 24 
>  hw/core/machine.c   | 26 ++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 68d3d10f6b0..4d08bc12093 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -36,6 +36,24 @@ void machine_set_cpu_numa_node(MachineState *machine,
> const CpuInstanceProperties *props,
> Error **errp);
>  
> +/**
> + * machine_class_valid_for_accelerator:
> + * @mc: the machine class
> + * @acc_name: accelerator name
> + *
> + * Returns %true if the accelerator is valid for the machine, %false
> + * otherwise. See #MachineClass.valid_accelerators.

Naming confusion: is the machine class valid for the accelerator, or
the accelerator valid for the machine class? Or either? :)

> + */
> +bool machine_class_valid_for_accelerator(MachineClass *mc, const char 
> *acc_name);
> +/**
> + * machine_class_valid_for_current_accelerator:
> + * @mc: the machine class
> + *
> + * Returns %true if the accelerator is valid for the current machine,
> + * %false otherwise. See #MachineClass.valid_accelerators.

Same here: current accelerator vs. current machine.

> + */
> +bool machine_class_valid_for_current_accelerator(MachineClass *mc);
> +
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char 
> *type);
>  /*
>   * Checks that backend isn't used, preps it for exclusive usage and
> @@ -125,6 +143,11 @@ typedef struct {
>   *should instead use "unimplemented-device" for all memory ranges where
>   *the guest will attempt to probe for a device that QEMU doesn't
>   *implement and a stub device is required.
> + * @valid_accelerators:
> + *If this machine supports a specific set of virtualization accelerators,
> + *this contains a NULL-terminated list of the accelerators that can be
> + *used. If this field is not set, any accelerator is valid. The QTest
> + *accelerator is always valid.
>   * @kvm_type:
>   *Return the type of KVM corresponding to the kvm-type string option or
>   *computed based on other criteria such as the host kernel capabilities
> @@ -166,6 +189,7 @@ struct MachineClass {
>  const char *alias;
>  const char *desc;
>  const char *deprecation_reason;
> +const char *const *valid_accelerators;
>  
>  void (*init)(MachineState *state);
>  void (*reset)(MachineState *state);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 970046f4388..c42d8e382b1 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -518,6 +518,32 @@ static void machine_set_nvdimm_persistence(Object *obj, 
> const char *value,
>  nvdimms_state->persistence_string = g_strdup(value);
>  }
>  
> +bool machine_class_valid_for_accelerator(MachineClass *mc, const char 
> *acc_name)
> +{
> +const char *const *name = mc->valid_accelerators;
> +
> +if (!name) {
> +return true;
> +}
> +if (strcmp(acc_name, "qtest") == 0) {
> +return true;
> +}
> +
> +for (unsigned i = 0; name[i]; i++) {
> +if (strcasecmp(acc_name, name[i]) == 0) {
> +return true;
> +}
> +}
> +return false;
> +}
> +
> +bool machine_class_valid_for_current_accelerator(MachineClass *mc)
> +{
> +AccelClass *ac = ACCEL_GET_CLASS(current_accel());
> +
> +return machine_class_valid_for_accelerator(mc, ac->name);
> +}

The implementation of the function tests for the current accelerator,
so I think you need to tweak the description above?

> +
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char 
> *type)
>  {
>  QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));




Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

2021-02-22 Thread Cornelia Huck
On Fri, 19 Feb 2021 18:38:37 +0100
Philippe Mathieu-Daudé  wrote:

> MachineClass::kvm_type() can return -1 on failure.
> Document it, and add a check in kvm_init().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/boards.h | 3 ++-
>  accel/kvm/kvm-all.c | 6 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index a46dfe5d1a6..68d3d10f6b0 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -127,7 +127,8 @@ typedef struct {
>   *implement and a stub device is required.
>   * @kvm_type:
>   *Return the type of KVM corresponding to the kvm-type string option or
> - *computed based on other criteria such as the host kernel capabilities.
> + *computed based on other criteria such as the host kernel capabilities
> + *(which can't be negative), or -1 on error.
>   * @numa_mem_supported:
>   *true if '--numa node.mem' option is supported and false otherwise
>   * @smp_parse:
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 84c943fcdb2..b069938d881 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
>  "kvm-type",
>  _abort);
>  type = mc->kvm_type(ms, kvm_type);
> +if (type < 0) {
> +ret = -EINVAL;
> +fprintf(stderr, "Failed to detect kvm-type for machine '%s'\n",
> +mc->name);
> +goto err;
> +}
>  }
>  
>  do {

No objection to this patch; but I'm wondering why some non-pseries
machines implement the kvm_type callback, when I see the kvm-type
property only for pseries? Am I holding my git grep wrong?




Re: [PATCH v4 30/32] qdev: Rename qdev_get_prop_ptr() to object_field_prop_ptr()

2020-12-14 Thread Cornelia Huck
On Fri, 11 Dec 2020 17:05:27 -0500
Eduardo Habkost  wrote:

> The function will be moved to common QOM code, as it is not
> specific to TYPE_DEVICE anymore.
> 
> Reviewed-by: Stefan Berger 
> Signed-off-by: Eduardo Habkost 
> ---
> Changes v1 -> v2:
> * Rename to object_field_prop_ptr() instead of object_static_prop_ptr()
> ---
> Cc: Stefan Berger 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Cornelia Huck 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Richard Henderson 
> Cc: David Hildenbrand 
> Cc: Thomas Huth 
> Cc: Matthew Rosato 
> Cc: Alex Williamson 
> Cc: qemu-de...@nongnu.org
> Cc: xen-devel@lists.xenproject.org
> Cc: qemu-bl...@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
>  include/hw/qdev-properties.h |  2 +-
>  backends/tpm/tpm_util.c  |  6 ++--
>  hw/block/xen-block.c |  4 +--
>  hw/core/qdev-properties-system.c | 50 +-
>  hw/core/qdev-properties.c| 60 
>  hw/s390x/css.c   |  4 +--
>  hw/s390x/s390-pci-bus.c  |  4 +--
>  hw/vfio/pci-quirks.c |  4 +--
>  8 files changed, 67 insertions(+), 67 deletions(-)

Reviewed-by: Cornelia Huck 




Re: [PATCH v4 23/32] qdev: Move dev->realized check to qdev_property_set()

2020-12-14 Thread Cornelia Huck
On Fri, 11 Dec 2020 17:05:20 -0500
Eduardo Habkost  wrote:

> Every single qdev property setter function manually checks
> dev->realized.  We can just check dev->realized inside
> qdev_property_set() instead.
> 
> The check is being added as a separate function
> (qdev_prop_allow_set()) because it will become a callback later.
> 
> Reviewed-by: Stefan Berger 
> Signed-off-by: Eduardo Habkost 
> ---
> Changes v1 -> v2:
> * Removed unused variable at xen_block_set_vdev()
> * Redone patch after changes in the previous patches in the
>   series
> ---
> Cc: Stefan Berger 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Cornelia Huck 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Richard Henderson 
> Cc: David Hildenbrand 
> Cc: Thomas Huth 
> Cc: Matthew Rosato 
> Cc: Alex Williamson 
> Cc: Mark Cave-Ayland 
> Cc: Artyom Tarasenko 
> Cc: qemu-de...@nongnu.org
> Cc: xen-devel@lists.xenproject.org
> Cc: qemu-bl...@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
>  backends/tpm/tpm_util.c  |   6 --
>  hw/block/xen-block.c |   6 --
>  hw/core/qdev-properties-system.c |  70 --
>  hw/core/qdev-properties.c| 100 ++-
>  hw/s390x/css.c   |   6 --
>  hw/s390x/s390-pci-bus.c  |   6 --
>  hw/vfio/pci-quirks.c |   6 --
>  target/sparc/cpu.c   |   6 --
>  8 files changed, 18 insertions(+), 188 deletions(-)

Reviewed-by: Cornelia Huck 




Re: [PATCH v2 4/6] xen: Delete xen_available() function

2020-11-27 Thread Cornelia Huck
On Wed, 25 Nov 2020 15:56:34 -0500
Eduardo Habkost  wrote:

> The function can be replaced with accel_available("xen").
> 
> Signed-off-by: Eduardo Habkost 
> ---
> Cc: Paolo Bonzini 
> Cc: qemu-de...@nongnu.org
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: xen-devel@lists.xenproject.org
> Cc: Richard Henderson 
> Cc: Claudio Fontana 
> Cc: Roman Bolshakov 
> ---
>  include/sysemu/arch_init.h | 2 --
>  softmmu/arch_init.c| 9 -
>  softmmu/vl.c   | 6 +++---
>  3 files changed, 3 insertions(+), 14 deletions(-)

Reviewed-by: Cornelia Huck 




Re: [PATCH v3 09/53] qdev: Make qdev_get_prop_ptr() get Object* arg

2020-11-13 Thread Cornelia Huck
On Thu, 12 Nov 2020 16:43:06 -0500
Eduardo Habkost  wrote:

> Make the code more generic and not specific to TYPE_DEVICE.
> 
> Reviewed-by: Marc-André Lureau 
> Signed-off-by: Eduardo Habkost 
> ---
> Changes v1 -> v2:
> - Fix build error with CONFIG_XEN
>   I took the liberty of keeping the Reviewed-by line from
>   Marc-André as the build fix is a trivial one line change
> ---
> Cc: Stefan Berger 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Cornelia Huck 
> Cc: Thomas Huth 
> Cc: Richard Henderson 
> Cc: David Hildenbrand 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Matthew Rosato 
> Cc: Alex Williamson 
> Cc: qemu-de...@nongnu.org
> Cc: xen-devel@lists.xenproject.org
> Cc: qemu-bl...@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
>  include/hw/qdev-properties.h |  2 +-
>  backends/tpm/tpm_util.c  |  8 ++--
>  hw/block/xen-block.c |  5 +-
>  hw/core/qdev-properties-system.c | 57 +-
>  hw/core/qdev-properties.c| 82 +---
>  hw/s390x/css.c   |  5 +-
>  hw/s390x/s390-pci-bus.c  |  4 +-
>  hw/vfio/pci-quirks.c |  5 +-
>  8 files changed, 68 insertions(+), 100 deletions(-)

Reviewed-by: Cornelia Huck  #s390 parts




Re: [PATCH 6/7] qom: Add FIELD_PTR, a type-safe wrapper for object_field_prop_ptr()

2020-11-09 Thread Cornelia Huck
On Wed,  4 Nov 2020 12:25:11 -0500
Eduardo Habkost  wrote:

> Introduce a FIELD_PTR macro that will ensure the size of the area
> we are accessing has the correct size, and will return a pointer
> of the correct type.
> 
> Signed-off-by: Eduardo Habkost 
> ---
> Cc: Stefan Berger 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Cornelia Huck 
> Cc: Thomas Huth 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Richard Henderson 
> Cc: David Hildenbrand 
> Cc: Matthew Rosato 
> Cc: Alex Williamson 
> Cc: qemu-de...@nongnu.org
> Cc: xen-devel@lists.xenproject.org
> Cc: qemu-bl...@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
>  include/qom/field-property.h | 21 ++-
>  backends/tpm/tpm_util.c  |  6 ++--
>  hw/block/xen-block.c |  4 +--
>  hw/core/qdev-properties-system.c | 50 +-
>  hw/s390x/css.c   |  4 +--
>  hw/s390x/s390-pci-bus.c  |  4 +--
>  hw/vfio/pci-quirks.c |  4 +--
>  qom/field-property.c |  3 +-
>  qom/property-types.c     | 60 +---
>  9 files changed, 89 insertions(+), 67 deletions(-)

Acked-by: Cornelia Huck 




Re: [PATCH 4/7] qom: Replace void* parameter with Property* on field getters/setters

2020-11-09 Thread Cornelia Huck
On Wed,  4 Nov 2020 12:25:09 -0500
Eduardo Habkost  wrote:

> All field property getters and setters must interpret the fourth
> argument as Property*.  Change the function signature of field
> property getters and setters to indicate that.
> 
> Signed-off-by: Eduardo Habkost 
> ---
> Cc: Stefan Berger 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Richard Henderson 
> Cc: David Hildenbrand 
> Cc: Cornelia Huck 
> Cc: Thomas Huth 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Matthew Rosato 
> Cc: Alex Williamson 
> Cc: Mark Cave-Ayland 
> Cc: Artyom Tarasenko 
> Cc: qemu-de...@nongnu.org
> Cc: xen-devel@lists.xenproject.org
> Cc: qemu-bl...@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
>  include/qom/field-property-internal.h |   8 +-
>  include/qom/field-property.h  |  26 ---
>  backends/tpm/tpm_util.c   |  11 ++-
>  hw/block/xen-block.c  |   6 +-
>  hw/core/qdev-properties-system.c  |  86 +-
>  hw/s390x/css.c|   6 +-
>  hw/s390x/s390-pci-bus.c   |   6 +-
>  hw/vfio/pci-quirks.c  |  10 +--
>  qom/property-types.c  | 102 +-
>  target/sparc/cpu.c|   4 +-
>  10 files changed, 105 insertions(+), 160 deletions(-)

Acked-by: Cornelia Huck 




Re: [PATCH 3/5] qom: Remove module_obj_name parameter from OBJECT_DECLARE* macros

2020-09-17 Thread Cornelia Huck
On Wed, 16 Sep 2020 14:25:17 -0400
Eduardo Habkost  wrote:

> One of the goals of having less boilerplate on QOM declarations
> is to avoid human error.  Requiring an extra argument that is
> never used is an opportunity for mistakes.
> 
> Remove the unused argument from OBJECT_DECLARE_TYPE and
> OBJECT_DECLARE_SIMPLE_TYPE.
> 
> Coccinelle patch used to convert all users of the macros:
> 
>   @@
>   declarer name OBJECT_DECLARE_TYPE;
>   identifier InstanceType, ClassType, lowercase, UPPERCASE;
>   @@
>OBJECT_DECLARE_TYPE(InstanceType, ClassType,
>   -lowercase,
>UPPERCASE);
> 
>   @@
>   declarer name OBJECT_DECLARE_SIMPLE_TYPE;
>   identifier InstanceType, lowercase, UPPERCASE;
>   @@
>OBJECT_DECLARE_SIMPLE_TYPE(InstanceType,
>   -lowercase,
>    UPPERCASE);
> 
> Signed-off-by: Eduardo Habkost 

Acked-by: Cornelia Huck 




Re: [PATCH-for-5.1 2/3] various: Remove unnecessary OBJECT() cast

2020-04-14 Thread Cornelia Huck
On Sun, 12 Apr 2020 23:09:53 +0200
Philippe Mathieu-Daudé  wrote:

> The OBJECT() macro is defined as:
> 
>   #define OBJECT(obj) ((Object *)(obj))
> 
> Remove unnecessary OBJECT() casts.
> 
> Patch created mechanically using spatch with this script:
> 
>   @@
>   typedef Object;
>   Object *o;
>   @@
>   -   OBJECT(o)
>   +   o
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/core/bus.c   | 2 +-
>  hw/ide/ahci-allwinner.c | 2 +-
>  hw/ipmi/smbus_ipmi.c| 2 +-
>  hw/microblaze/petalogix_ml605_mmu.c | 8 
>  hw/s390x/sclp.c | 2 +-
>  monitor/misc.c  | 3 +--
>  qom/object.c| 4 ++--
>  7 files changed, 11 insertions(+), 12 deletions(-)
> 

s390x part:

Acked-by: Cornelia Huck 




Re: [Xen-devel] [PATCH v3 19/20] Let cpu_[physical]_memory() calls pass a boolean 'is_write' argument

2020-02-21 Thread Cornelia Huck
On Thu, 20 Feb 2020 14:05:47 +0100
Philippe Mathieu-Daudé  wrote:

> Use an explicit boolean type.
> 
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/exec_rw_const.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  scripts/coccinelle/exec_rw_const.cocci | 14 ++
>  include/exec/cpu-common.h  |  4 ++--
>  hw/display/exynos4210_fimd.c   |  3 ++-
>  hw/display/milkymist-tmu2.c|  8 
>  hw/display/omap_dss.c  |  2 +-
>  hw/display/ramfb.c |  2 +-
>  hw/misc/pc-testdev.c   |  2 +-
>  hw/nvram/spapr_nvram.c |  4 ++--
>  hw/ppc/ppc440_uc.c |  6 --
>  hw/ppc/spapr_hcall.c   |  4 ++--
>  hw/s390x/ipl.c |  2 +-
>  hw/s390x/s390-pci-bus.c|  2 +-
>  hw/s390x/virtio-ccw.c  |  2 +-
>  hw/xen/xen_pt_graphics.c   |  2 +-
>  target/i386/hax-all.c  |  4 ++--
>  target/s390x/excp_helper.c |  2 +-
>  target/s390x/helper.c  |  6 +++---
>  17 files changed, 43 insertions(+), 26 deletions(-)
> 

> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 7773499d7f..0817874b48 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -626,7 +626,7 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
>  uint8_t *addr;
>  uint64_t len = 4096;
>  
> -addr = cpu_physical_memory_map(cpu->env.psa, , 1);
> +addr = cpu_physical_memory_map(cpu->env.psa, , true);
>  if (!addr || len < QIPL_ADDRESS + sizeof(QemuIplParameters)) {
>  error_report("Cannot set QEMU IPL parameters");
>  return;
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 7c6a2b3c63..ed8be124da 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -641,7 +641,7 @@ static uint8_t set_ind_atomic(uint64_t ind_loc, uint8_t 
> to_be_set)
>  hwaddr len = 1;
>  uint8_t *ind_addr;
>  
> -ind_addr = cpu_physical_memory_map(ind_loc, , 1);
> +ind_addr = cpu_physical_memory_map(ind_loc, , true);
>  if (!ind_addr) {
>  s390_pci_generate_error_event(ERR_EVENT_AIRERR, 0, 0, 0, 0);
>  return -1;
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 13f57e7b67..50cf95b781 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -790,7 +790,7 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, 
> uint64_t ind_loc,
>  hwaddr len = 1;
>  uint8_t *ind_addr;
>  
> -ind_addr = cpu_physical_memory_map(ind_loc, , 1);
> +ind_addr = cpu_physical_memory_map(ind_loc, , true);
>  if (!ind_addr) {
>  error_report("%s(%x.%x.%04x): unable to access indicator",
>   __func__, sch->cssid, sch->ssid, sch->schid);

> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index 1e9d6f20c1..3b58d10df3 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -393,7 +393,7 @@ static int mchk_store_vregs(CPUS390XState *env, uint64_t 
> mcesao)
>  MchkExtSaveArea *sa;
>  int i;
>  
> -sa = cpu_physical_memory_map(mcesao, , 1);
> +sa = cpu_physical_memory_map(mcesao, , true);
>  if (!sa) {
>  return -EFAULT;
>  }
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index a3a49164e4..b810ad431e 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -151,7 +151,7 @@ LowCore *cpu_map_lowcore(CPUS390XState *env)
>  LowCore *lowcore;
>  hwaddr len = sizeof(LowCore);
>  
> -lowcore = cpu_physical_memory_map(env->psa, , 1);
> +lowcore = cpu_physical_memory_map(env->psa, , true);
>  
>  if (len < sizeof(LowCore)) {
>  cpu_abort(env_cpu(env), "Could not map lowcore\n");
> @@ -246,7 +246,7 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool 
> store_arch)
>  hwaddr len = sizeof(*sa);
>  int i;
>  
> -sa = cpu_physical_memory_map(addr, , 1);
> +sa = cpu_physical_memory_map(addr, , true);
>  if (!sa) {
>  return -EFAULT;
>  }
> @@ -298,7 +298,7 @@ int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, 
> hwaddr len)
>  hwaddr save = len;
>  int i;
>  
> -sa = cpu_physical_memory_map(addr, , 1);
> +sa = cpu_physical_memory_map(addr, , true);
>  if (!sa) {
>  return -EFAULT;
>  }

s390 parts
Acked-by: Cornelia Huck 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 08/20] Remove unnecessary cast when using the address_space API

2020-02-21 Thread Cornelia Huck
On Thu, 20 Feb 2020 14:05:36 +0100
Philippe Mathieu-Daudé  wrote:

> This commit was produced with the included Coccinelle script
> scripts/coccinelle/exec_rw_const.
> 
> Two lines in hw/net/dp8393x.c that Coccinelle produced that
> were over 80 characters were re-wrapped by hand.
> 
> Suggested-by: Stefan Weil 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  scripts/coccinelle/exec_rw_const.cocci | 15 +-
>  target/i386/hvf/vmx.h  |  2 +-
>  hw/arm/boot.c  |  6 ++
>  hw/dma/rc4030.c|  4 ++--
>  hw/dma/xlnx-zdma.c |  2 +-
>  hw/net/cadence_gem.c   | 21 +--
>  hw/net/dp8393x.c   | 28 +-
>  hw/s390x/css.c |  4 ++--
>  qtest.c| 12 +--
>  target/i386/hvf/x86_mmu.c  |  2 +-
>  target/i386/whpx-all.c |  2 +-
>  target/s390x/mmu_helper.c  |  2 +-
>  12 files changed, 54 insertions(+), 46 deletions(-)
> 

> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 844caab408..f27f8c45a5 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -875,7 +875,7 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
>  return -EINVAL; /* channel program check */
>  }
>  ret = address_space_rw(_space_memory, idaw_addr,
> -   MEMTXATTRS_UNSPECIFIED, (void *) ,
> +   MEMTXATTRS_UNSPECIFIED, ,
> sizeof(idaw.fmt2), false);
>  cds->cda = be64_to_cpu(idaw.fmt2);
>  } else {
> @@ -884,7 +884,7 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
>  return -EINVAL; /* channel program check */
>  }
>  ret = address_space_rw(_space_memory, idaw_addr,
> -   MEMTXATTRS_UNSPECIFIED, (void *) ,
> +   MEMTXATTRS_UNSPECIFIED, ,
> sizeof(idaw.fmt1), false);
>  cds->cda = be64_to_cpu(idaw.fmt1);
>  if (cds->cda & 0x8000) {

> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index c9f3f34750..0be2f300bb 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -106,7 +106,7 @@ static inline bool read_table_entry(CPUS390XState *env, 
> hwaddr gaddr,
>   * We treat them as absolute addresses and don't wrap them.
>   */
>  if (unlikely(address_space_read(cs->as, gaddr, MEMTXATTRS_UNSPECIFIED,
> -(uint8_t *)entry, sizeof(*entry)) !=
> +entry, sizeof(*entry)) !=
>   MEMTX_OK)) {
>  return false;
>  }

s390 parts
Acked-by: Cornelia Huck 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC v5 026/126] python: add commit-per-subsystem.py

2019-11-12 Thread Cornelia Huck
On Fri, 11 Oct 2019 19:04:12 +0300
Vladimir Sementsov-Ogievskiy  wrote:

> Add script to automatically commit tree-wide changes per-subsystem.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---

I think this still needs some notes as to the supposed usage.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC v5 000/126] error: auto propagated local_err

2019-11-12 Thread Cornelia Huck
On Fri, 8 Nov 2019 22:57:25 +0400
Marc-André Lureau  wrote:

> Hi
> 
> On Fri, Nov 8, 2019 at 7:31 PM Vladimir Sementsov-Ogievskiy
>  wrote:
> >
> > Finally, what is the plan?
> >
> > Markus what do you think?
> >
> > Now a lot of patches are reviewed, but a lot of are not.
> >
> > Is there any hope that all patches will be reviewed? Should I resend the
> > whole series, or may be reduce it to reviewed subsystems only?  
> 
> I don't think we have well established rules for whole-tree cleanups
> like this. In the past, several cleanup series got lost.

Yes, it is always problematic if a series touches a lot of different
subsystems.

> 
> It will take ages to get every subsystem maintainer to review the
> patches. Most likely, since they are quite systematic, there isn't
> much to say and it is easy to miss something that has some hidden
> ramifications. Perhaps whole-tree cleanups should require at least 2
> reviewers to bypass the subsytem maintainer review? But my past
> experience with this kind of exercice doesn't encourage me, and
> probably I am not the only one.

It's not just the reviews; it's easy to miss compile problems on less
mainstream architectures (and even easier to miss functional problems
there, although they are probably less likely with automated rework.)
CI can probably help, but that's something for the future.

Anyway, I've now gotten around to that series; spotted one problem in
s390x code, I think.

One thing that's helpful for such a large series is a git branch that
makes it easy to give the series a quick go. (You can use patchew, but
it takes time before it gets all mails, so just pushing it somewhere
and letting people know is a good idea anyway.)


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v6 12/26] hw/s390x: Hard code size with MO_{8|16|32|64}

2019-08-08 Thread Cornelia Huck
On Wed, 7 Aug 2019 08:30:04 +
 wrote:

> Temporarily no-op size_memop was introduced to aid the conversion of
> memory_region_dispatch_{read|write} operand "unsigned size" into
> "MemOp op".
> 
> Now size_memop is implemented, again hard coded size but with

"Now that size_memop has been implemented properly, again hard code the
size but with..."

> MO_{8|16|32|64}. This is more expressive and avoid size_memop calls.

s/avoid/avoids/

> 
> Signed-off-by: Tony Nguyen 
> ---
>  hw/s390x/s390-pci-inst.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v6 08/26] hw/vfio: Access MemoryRegion with MemOp

2019-08-08 Thread Cornelia Huck
On Wed, 7 Aug 2019 08:28:40 +
 wrote:

> The memory_region_dispatch_{read|write} operand "unsigned size" is
> being converted into a "MemOp op".
> 
> Convert interfaces by using no-op size_memop.
> 
> After all interfaces are converted, size_memop will be implemented
> and the memory_region_dispatch_{read|write} operand "unsigned size"
> will be converted into a "MemOp op".
> 
> As size_memop is a no-op, this patch does not change any behaviour.
> 
> Signed-off-by: Tony Nguyen 
> Reviewed-by: Richard Henderson 
> ---
>  hw/vfio/pci-quirks.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v6 07/26] hw/virtio: Access MemoryRegion with MemOp

2019-08-08 Thread Cornelia Huck
On Wed, 7 Aug 2019 08:28:16 +
 wrote:

> The memory_region_dispatch_{read|write} operand "unsigned size" is
> being converted into a "MemOp op".
> 
> Convert interfaces by using no-op size_memop.
> 
> After all interfaces are converted, size_memop will be implemented
> and the memory_region_dispatch_{read|write} operand "unsigned size"
> will be converted into a "MemOp op".
> 
> As size_memop is a no-op, this patch does not change any behaviour.
> 
> Signed-off-by: Tony Nguyen 
> Reviewed-by: Richard Henderson 
> ---
>  hw/virtio/virtio-pci.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v6 05/26] hw/s390x: Access MemoryRegion with MemOp

2019-08-08 Thread Cornelia Huck
On Wed, 7 Aug 2019 08:27:35 +
 wrote:

> The memory_region_dispatch_{read|write} operand "unsigned size" is
> being converted into a "MemOp op".
> 
> Convert interfaces by using no-op size_memop.
> 
> After all interfaces are converted, size_memop will be implemented
> and the memory_region_dispatch_{read|write} operand "unsigned size"
> will be converted into a "MemOp op".
> 
> As size_memop is a no-op, this patch does not change any behaviour.
> 
> Signed-off-by: Tony Nguyen 
> Reviewed-by: Richard Henderson 
> ---
>  hw/s390x/s390-pci-inst.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v6 02/26] tcg: TCGMemOp is now accelerator independent MemOp

2019-08-08 Thread Cornelia Huck
On Wed, 7 Aug 2019 08:26:23 +
 wrote:

> Preparation for collapsing the two byte swaps, adjust_endianness and
> handle_bswap, along the I/O path.
> 
> Target dependant attributes are conditionalize upon NEED_CPU_H.

s/conditionalize/conditionalized/ ?

> 
> Signed-off-by: Tony Nguyen 
> Acked-by: David Gibson 
> Reviewed-by: Richard Henderson 
> ---
>  MAINTAINERS |   1 +
>  accel/tcg/cputlb.c  |   2 +-
>  include/exec/memop.h| 110 ++
>  target/alpha/translate.c|   2 +-
>  target/arm/translate-a64.c  |  48 ++--
>  target/arm/translate-a64.h  |   2 +-
>  target/arm/translate-sve.c  |   2 +-
>  target/arm/translate.c  |  32 
>  target/arm/translate.h  |   2 +-
>  target/hppa/translate.c |  14 ++--
>  target/i386/translate.c | 132 
> 
>  target/m68k/translate.c |   2 +-
>  target/microblaze/translate.c   |   4 +-
>  target/mips/translate.c |   8 +-
>  target/openrisc/translate.c |   4 +-
>  target/ppc/translate.c  |  12 +--
>  target/riscv/insn_trans/trans_rva.inc.c |   8 +-
>  target/riscv/insn_trans/trans_rvi.inc.c |   4 +-
>  target/s390x/translate.c|   6 +-
>  target/s390x/translate_vx.inc.c |  10 +--
>  target/sparc/translate.c|  14 ++--
>  target/tilegx/translate.c   |  10 +--
>  target/tricore/translate.c  |   8 +-
>  tcg/README  |   2 +-
>  tcg/aarch64/tcg-target.inc.c|  26 +++
>  tcg/arm/tcg-target.inc.c|  26 +++
>  tcg/i386/tcg-target.inc.c   |  24 +++---
>  tcg/mips/tcg-target.inc.c   |  16 ++--
>  tcg/optimize.c  |   2 +-
>  tcg/ppc/tcg-target.inc.c|  12 +--
>  tcg/riscv/tcg-target.inc.c  |  20 ++---
>  tcg/s390/tcg-target.inc.c   |  14 ++--
>  tcg/sparc/tcg-target.inc.c  |   6 +-
>  tcg/tcg-op.c|  38 -
>  tcg/tcg-op.h|  86 ++---
>  tcg/tcg.c   |   2 +-
>  tcg/tcg.h   | 101 ++--
>  trace/mem-internal.h|   4 +-
>  trace/mem.h     |   4 +-
>  39 files changed, 421 insertions(+), 399 deletions(-)
>  create mode 100644 include/exec/memop.h

Acked-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v6 01/26] configure: Define TARGET_ALIGNED_ONLY

2019-08-08 Thread Cornelia Huck
On Wed, 7 Aug 2019 08:25:37 +
 wrote:

> Rename ALIGNED_ONLY to TARGET_ALIGNED_ONLY for clarity and move
> defines out of target/foo/cpu.h into configure, as we do with
> TARGET_WORDS_BIGENDIAN, so that it is always defined early.
> 
> Poisoned TARGET_ALIGNED_ONLY to prevent use in common code.
> 
> Signed-off-by: Tony Nguyen 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Richard Henderson 
> Reviewed-by: Aleksandar Markovic 
> ---
>  configure | 10 +-
>  include/exec/poison.h |  1 +
>  include/qom/cpu.h |  2 +-
>  target/alpha/cpu.h|  2 --
>  target/hppa/cpu.h |  1 -
>  target/mips/cpu.h |  2 --
>  target/sh4/cpu.h  |  2 --
>  target/sparc/cpu.h|  2 --
>  target/xtensa/cpu.h   |  2 --
>  tcg/tcg.c |  2 +-
>  tcg/tcg.h |  8 +---
>  11 files changed, 17 insertions(+), 17 deletions(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument

2019-02-20 Thread Cornelia Huck
On Wed, 20 Feb 2019 11:53:42 +0100
Marc-André Lureau  wrote:

> Hi
> 
> On Wed, Feb 20, 2019 at 2:02 AM Philippe Mathieu-Daudé
>  wrote:
> >
> > Hi,
> >
> > This series convert the chardev::qemu_chr_write() to take unsigned
> > length argument. To do so I went through all caller and checked if
> > there are no negative value possible.  
> 
> 
> Changing signedness is problematic and can easily introduce bugs that
> are easy to miss during review.
> 
> I agree with Cornelia about idiomatic use of int. Changing "int" for
> "size_t" isn't systematically a clear win.
> 
> Even Google C++ style recommends to avoid unsigned types "(except for
> representing bitfields or modular arithmetic). Do not use an unsigned
> type merely to assert that a variable is non-negative."
> https://google.github.io/styleguide/cppguide.html#Integer_Types - see 
> rationale
> 
> Since Paolo you suggested the change, could you give some convincing
> arguments that it's worth taking the plunge?

FWIW, using an explicitly unsigned type for a length sounds fine; but
not all conversions are really convincing (albeit not wrong).

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 21/25] s390x/sclp: Use size_t in process_mdb()

2019-02-20 Thread Cornelia Huck
On Wed, 20 Feb 2019 02:02:28 +0100
Philippe Mathieu-Daudé  wrote:

> Since it is unlikely we have sizeof(mdbo->mto.message) < 0,
> we can convert this variable to an unsigned type.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/char/sclpconsole-lm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 22/25] s390x/sclp: Let write_console_data() take a size_t

2019-02-20 Thread Cornelia Huck
On Wed, 20 Feb 2019 02:02:29 +0100
Philippe Mathieu-Daudé  wrote:

> Since all callers provide an unsigned value, we can safely
> use a size_t argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/char/sclpconsole-lm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 20/25] s390x/sclp: Use a const variable to improve readability

2019-02-20 Thread Cornelia Huck
On Wed, 20 Feb 2019 02:02:27 +0100
Philippe Mathieu-Daudé  wrote:

> We will reuse this variable in the next patch.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/char/sclpconsole-lm.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> index dbc91a1e5b..49543e2c83 100644
> --- a/hw/char/sclpconsole-lm.c
> +++ b/hw/char/sclpconsole-lm.c
> @@ -210,13 +210,14 @@ static int process_mdb(SCLPEvent *event, MDBO *mdbo)
>  int rc;
>  int len;
>  uint8_t buffer[SIZE_BUFFER];
> -
> -len = be16_to_cpu(mdbo->length);
> -len -= sizeof(mdbo->length) + sizeof(mdbo->type)
> +const size_t hlen = sizeof(mdbo->length)
> ++ sizeof(mdbo->type)
>  + sizeof(mdbo->mto.line_type_flags)
>  + sizeof(mdbo->mto.alarm_control)
>  + sizeof(mdbo->mto._reserved);
>  
> +len = be16_to_cpu(mdbo->length);
> +len -= hlen;
>  assert(len <= SIZE_BUFFER);
>  
>  /* convert EBCDIC SCLP contents to ASCII console message */

I'd probably merge this with the next patch, though.

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 19/25] s390/ebcdic: Use size_t to iterate over arrays

2019-02-20 Thread Cornelia Huck
On Wed, 20 Feb 2019 02:02:26 +0100
Philippe Mathieu-Daudé  wrote:

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/s390x/ebcdic.h | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/s390x/ebcdic.h b/include/hw/s390x/ebcdic.h
> index 69a04cab62..d89174e113 100644
> --- a/include/hw/s390x/ebcdic.h
> +++ b/include/hw/s390x/ebcdic.h
> @@ -83,18 +83,18 @@ static const uint8_t ascii2ebcdic[] = {
>  0x90, 0x3F, 0x3F, 0x3F, 0x3F, 0xEA, 0x3F, 0xFF
>  };
>  
> -static inline void ebcdic_put(uint8_t *p, const char *ascii, int len)
> +static inline void ebcdic_put(uint8_t *p, const char *ascii, size_t len)
>  {
> -int i;
> +size_t i;
>  
>  for (i = 0; i < len; i++) {
>  p[i] = ascii2ebcdic[(uint8_t)ascii[i]];
>  }
>  }
>  
> -static inline void ascii_put(uint8_t *p, const char *ebcdic, int len)
> +static inline void ascii_put(uint8_t *p, const char *ebcdic, size_t len)
>  {
> -int i;
> +size_t i;
>  
>  for (i = 0; i < len; i++) {
>  p[i] = ebcdic2ascii[(uint8_t)ebcdic[i]];

Making the passed len parameter a size_t makes sense; but using a
size_t as an array iterator looks a bit unidiomatic... it's not wrong,
though.

Acked-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 18/25] s390x/3270: Let insert_IAC_escape_char() use size_t

2019-02-20 Thread Cornelia Huck
On Wed, 20 Feb 2019 02:02:25 +0100
Philippe Mathieu-Daudé  wrote:

> This function takes size_t argument and return a size_t.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/char/terminal3270.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] machine: Use shorter format for GlobalProperty arrays

2019-01-08 Thread Cornelia Huck
On Mon,  7 Jan 2019 17:30:20 -0200
Eduardo Habkost  wrote:

> Instead of verbose arrays with 4 lines for each entry, make each
> entry take only one line.  This makes long arrays that couldn't
> fit in the screen become short and readable.
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  include/hw/i386/pc.h   |  18 +-
>  hw/core/machine.c  | 338 -
>  hw/i386/pc.c   | 720 +++--
>  hw/i386/pc_piix.c  | 192 ++
>  hw/ppc/spapr.c |  72 +---
>  hw/s390x/s390-virtio-ccw.c |  75 +---
>  hw/xen/xen-common.c|  18 +-
>  7 files changed, 265 insertions(+), 1168 deletions(-)
> 

(...)

> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index c737507053..811fdf913d 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c

(...)

> @@ -810,15 +798,8 @@ static void ccw_machine_2_6_class_options(MachineClass 
> *mc)
>  {
>  S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
>  static GlobalProperty compat[] = {
> -{
> -.driver   = TYPE_S390_IPL,
> -.property = "iplbext_migration",
> -.value= "off",
> -}, {
> -.driver   = TYPE_VIRTUAL_CSS_BRIDGE,
> -.property = "css_dev_path",
> -.value= "off",
> -},
> +{ TYPE_S390_IPL, "iplbext_migration", "off", },
> + { TYPE_VIRTUAL_CSS_BRIDGE, "css_dev_path", "off", },

The indentation looks off here.

>  };
>  
>  s390mc->ri_allowed = false;

(...)

With or without alignment:

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH 3/3] machine: Use shorter format for GlobalProperty arrays

2019-01-08 Thread Cornelia Huck
On Tue, 8 Jan 2019 07:45:43 +0100
Gerd Hoffmann  wrote:

>   Hi,
> 
> > +{ "migration", "decompress-error-check", "off" },
> > +{ "hda-audio", "use-timer", "false" },
> > +{ "cirrus-vga", "global-vmstate", "true" },
> > +{ "VGA", "global-vmstate", "true" },
> > +{ "vmware-svga", "global-vmstate", "true" },
> > +{ "qxl-vga", "global-vmstate", "true" },  
> 
> I'd like to have the fields aligned.  Especially in cases like this one
> where multiple devices get the same value assigned it makes things more
> readable:
> 
> { "migration",   "decompress-error-check", "off"   },
> { "hda-audio",   "use-timer",  "false" },
> { "cirrus-vga",  "global-vmstate", "true"  },
> { "VGA", "global-vmstate", "true"  },
> { "vmware-svga", "global-vmstate", "true"  },
> { "qxl-vga", "global-vmstate", "true"  },
> 
> thanks,
>   Gerd
> 

I'm a bit on the fence here. It does make things more readable (at
least in your example), but I find editing aligned tables a bit
annoying. OTOH, that won't happen often, anyway.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] machine: Eliminate unnecessary stringify() usage

2019-01-08 Thread Cornelia Huck
On Mon,  7 Jan 2019 17:30:19 -0200
Eduardo Habkost  wrote:

> stringify() is useful when we need to use macros in compat_props
> (like when we set virtio-baloon-pci.class=PCI_CLASS_MEMORY_RAM at
> pc_i440fx_1_0_machine_options()), but it is pointless when we are
> already providing a number literal.
> 
> Replace stringify() with string literals when appropriate.
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  hw/core/machine.c |  8 ++--
>  hw/i386/pc.c  | 94 +++
>  hw/i386/pc_piix.c | 30 +++
>  hw/ppc/spapr.c|  2 +-
>  4 files changed, 67 insertions(+), 67 deletions(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] spapr: Eliminate SPAPR_PCI_2_7_MMIO_WIN_SIZE macro

2019-01-08 Thread Cornelia Huck
On Mon,  7 Jan 2019 17:30:18 -0200
Eduardo Habkost  wrote:

> The macro is only used in one place, where the purpose of the
> value is obvious.  Eliminate the macro so we don't need to rely
> on stringify().
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  include/hw/pci-host/spapr.h | 1 -
>  hw/ppc/spapr.c  | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 00/22] QOM'ify SysBusDeviceClass->init

2018-11-19 Thread Cornelia Huck
On Mon, 19 Nov 2018 20:07:58 +0800
Mao Zhongyi  wrote:

> The SysBusDeviceClass::init() interface is considered
> as a legacy interface and there are currently some
> efforts going on to get rid of it. Thus convert 
> SysBusDeviceClass::init to DeviceClass::realize.

In case my comment to the s390 change comes off as negative: I like
getting rid of the legacy interface :)

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 12/21] hw: Do not include "sysemu/block-backend.h" if it is not necessary

2018-05-29 Thread Cornelia Huck
On Mon, 28 May 2018 20:27:10 -0300
Philippe Mathieu-Daudé  wrote:

> Remove those unneeded includes to speed up the compilation
> process a little bit. (Continue 7eceff5b5a1fa cleanup)
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/arm/collie.c  | 1 -
>  hw/arm/gumstix.c | 1 -
>  hw/arm/mainstone.c   | 1 -
>  hw/arm/nseries.c | 1 -
>  hw/arm/omap1.c   | 1 -
>  hw/arm/omap2.c   | 1 -
>  hw/arm/omap_sx1.c| 1 -
>  hw/arm/pxa2xx.c  | 1 -
>  hw/arm/spitz.c   | 1 -
>  hw/arm/versatilepb.c | 1 -
>  hw/arm/vexpress.c| 1 -
>  hw/arm/virt.c| 1 -
>  hw/arm/xilinx_zynq.c | 1 -
>  hw/arm/z2.c  | 1 -
>  hw/block/dataplane/virtio-blk.c  | 1 -
>  hw/block/virtio-blk.c| 1 -
>  hw/core/qdev-properties.c| 1 -
>  hw/cris/axis_dev88.c | 1 -
>  hw/display/tc6393xb.c| 1 -
>  hw/ide/pci.c | 1 -
>  hw/ide/via.c | 1 -
>  hw/isa/isa-superio.c | 1 -
>  hw/lm32/lm32_boards.c| 1 -
>  hw/lm32/milkymist.c  | 1 -
>  hw/microblaze/petalogix_ml605_mmu.c  | 1 -
>  hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 -
>  hw/mips/mips_r4k.c   | 1 -
>  hw/ppc/spapr.c   | 1 -
>  hw/ppc/virtex_ml507.c| 2 --
>  hw/s390x/virtio-ccw.c| 1 -
>  hw/scsi/mptsas.c | 1 -
>  hw/sd/pl181.c| 1 -
>  hw/sd/sdhci.c| 1 -
>  hw/sd/ssi-sd.c   | 1 -
>  hw/sh4/r2d.c | 1 -
>  hw/virtio/virtio-pci.c   | 1 -
>  hw/xen/xen_devconfig.c   | 1 -
>  hw/xtensa/xtfpga.c   | 1 -
>  38 files changed, 39 deletions(-)

Acked-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 14/21] hw: Do not include "sysemu/blockdev.h" if it is not necessary

2018-05-29 Thread Cornelia Huck
On Mon, 28 May 2018 20:27:12 -0300
Philippe Mathieu-Daudé  wrote:

> Remove those unneeded includes to speed up the compilation
> process a little bit.
> 
> Code change produced with:
> 
> $ git grep '#include "sysemu/blockdev.h"' | \
>   cut -d: -f-1 | \
>   xargs egrep -L 
> "(BlockInterfaceType|DriveInfo|drive_get|blk_legacy_dinfo|blockdev_mark_auto_del)"
>  | \
>   xargs sed -i.bak '/#include "sysemu\/blockdev.h"/d'
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/block/m25p80.c  | 1 -
>  hw/block/onenand.c | 1 -
>  hw/i386/xen/xen-mapcache.c | 1 -
>  hw/s390x/virtio-ccw.c  | 1 -
>  hw/scsi/scsi-generic.c | 1 -
>  hw/sd/sdhci.c  | 1 -
>  hw/usb/dev-storage.c   | 1 -
>  monitor.c  | 1 -
>  8 files changed, 8 deletions(-)

Acked-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel