Re: [Qemu-devel] [PATCH v3 RESEND 3/3] nvram: at24c: use standard error reporting

[Peter Maydell]

On 20 May 2018 at 08:00, Wolfram Sang  wrote:
> Replace the ERR macro with error_report() because fprintf is deprecated.
> This also fixes the prefix printed out twice.
>
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Wolfram Sang 
> ---
>  hw/nvram/eeprom_at24c.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
> @@ -63,8 +61,7 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
>  if (ee->blk && ee->changed) {
>  int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
>  if (len != ee->rsize) {
> -ERR(TYPE_AT24C_EE
> -" : failed to write backing file\n");
> +error_report("failed to write backing file");
>  }
>  DPRINTK("Wrote to backing file\n");
>  }
> @@ -125,7 +122,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
>  EEPROMState *ee = AT24C_EE(i2c);
>
>  if (!ee->rsize) {
> -ERR("rom-size not allowed to be 0\n");
> +error_report("rom-size not allowed to be 0");
>  exit(1);
>  }

Hi; if we're going to overhaul the error handling for this
device, I think we might as well do it properly, by moving
the code in this init function which can fail into a new
device realize method. The realize method takes an Error**
and can report failures that way rather than by causing
QEMU to exit.

> @@ -166,8 +162,7 @@ void at24c_eeprom_reset(DeviceState *state)
>  int len = blk_pread(ee->blk, 0, ee->mem, ee->rsize);
>
>  if (len != ee->rsize) {
> -ERR(TYPE_AT24C_EE
> -" : Failed initial sync with backing file\n");
> +error_report("Failed initial sync with backing file");
>  }
>  DPRINTK("Reset read backing file\n");
>  }

Errors in reset and event methods are a bit more awkward;
error_report is an improvement in those cases.

thanks
-- PMM

Re: [PATCH qemu v3] device_tree: Increase FDT_MAX_SIZE to 1 MiB

[Peter Maydell]

On 12 April 2018 at 14:55, Geert Uytterhoeven <geert+rene...@glider.be> wrote:
> It is not uncommon for a contemporary FDT to be larger than 64 KiB,
> leading to failures loading the device tree from sysfs:
>
> qemu-system-aarch64: qemu_fdt_setprop: Couldn't set ...: FDT_ERR_NOSPACE
>
> Hence increase the limit to 1 MiB, like on PPC.
>
> For reference, the largest arm64 DTB created from the Linux sources is
> ca. 75 KiB large (100 KiB when built with symbols/fixup support).
>
> Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
> ---
> v3:
>   - Update example size figures,
>
> v2:
>   - Enlarge from 128 KiB to 1 MiB, as suggested by Peter Maydell.
> ---
>  device_tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks; applied to target-arm.next for 2.13. I'll add cc:qemu-stable too.

-- PMM

Re: [Qemu-devel] [PATCH] device_tree: Increase FDT_MAX_SIZE to 128 KiB

[Peter Maydell]

On 13 February 2018 at 16:41, Geert Uytterhoeven
 wrote:
> It is not uncommon for a contemporary FDT to be larger than 64 KiB,
> leading to failures loading the device tree from sysfs:
>
> qemu-system-aarch64: qemu_fdt_setprop: Couldn't set ...: FDT_ERR_NOSPACE
>
> For reference, the largest arm64 DTB created from the Linux sources is
> 70 KiB large (93 KiB when built with symbols/fixup support).

I think we should probably give ourselves a bit more headroom,
then -- at least 256K.

The ppc boards actually define their own version of this constant:

#define FDT_MAX_SIZE0x0010

so I think we might as well just go with that in device_tree.c for
consistency.

thanks
-- PMM

Re: [PATCH/RFC 3/5] hw/arm/virt: Allow dynamic sysbus devices again

[Peter Maydell]

On 9 February 2018 at 15:37, Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> Hi Peter,
>
> On Fri, Feb 9, 2018 at 4:27 PM, Peter Maydell <peter.mayd...@linaro.org> 
> wrote:
>> On 9 February 2018 at 15:17, Geert Uytterhoeven <geert+rene...@glider.be> 
>> wrote:
>>> Allow the instantation of generic dynamic sysbus devices again, without
>>> the need to create a new device-specific vfio type.
>>>
>>> This is a partial revert of commit  6f2062b9758ebc64 ("hw/arm/virt:
>>> Allow only supported dynamic sysbus devices").
>>>
>>> Not-Yet-Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
>>> ---
>>>  hw/arm/virt.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index b334c82edae9fb1f..fa83784fc08ed076 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1596,6 +1596,7 @@ static void virt_machine_class_init(ObjectClass *oc, 
>>> void *data)
>>>  mc->max_cpus = 255;
>>>  machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
>>>  machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
>>> +machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SYS_BUS_DEVICE);
>>>  mc->block_default_type = IF_VIRTIO;
>>>  mc->no_cdrom = 1;
>>>  mc->pci_allow_0_address = true;
>>
>> This needs a lot of justification. Dynamic sysbus is not supposed
>> to be for plugging any random sysbus device in, it's for vfio,
>> which needs special casing anyway to work right. (Overall it's
>
> Sure. Is there a way to limit this to vfio devices?

That would be the code here which implements the whitelist of
"only allow this for the vfio devices which we have
support for mangling the device tree for and know will work".

>> a terrible hack -- in an ideal world all vfio would use pci
>> or another probeable bus.)
>
> What about vfio-platform, which is my use case?
> On DT-based systems, platform devices are described very well in DT (even
> better than what's provided by probing PCI IDs and BARs ;-)

The TYPE_VFIO_* devices in the whitelist all use DT, yes:
the code to handle creating/mangling the dt nodes for the
passed-through device is in hw/arm/sysbus-fdt.c.

thanks
-- PMM

Re: [PATCH/RFC 3/5] hw/arm/virt: Allow dynamic sysbus devices again

[Peter Maydell]

On 9 February 2018 at 15:17, Geert Uytterhoeven  wrote:
> Allow the instantation of generic dynamic sysbus devices again, without
> the need to create a new device-specific vfio type.
>
> This is a partial revert of commit  6f2062b9758ebc64 ("hw/arm/virt:
> Allow only supported dynamic sysbus devices").
>
> Not-Yet-Signed-off-by: Geert Uytterhoeven 
> ---
>  hw/arm/virt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b334c82edae9fb1f..fa83784fc08ed076 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1596,6 +1596,7 @@ static void virt_machine_class_init(ObjectClass *oc, 
> void *data)
>  mc->max_cpus = 255;
>  machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
>  machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
> +machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SYS_BUS_DEVICE);
>  mc->block_default_type = IF_VIRTIO;
>  mc->no_cdrom = 1;
>  mc->pci_allow_0_address = true;

This needs a lot of justification. Dynamic sysbus is not supposed
to be for plugging any random sysbus device in, it's for vfio,
which needs special casing anyway to work right. (Overall it's
a terrible hack -- in an ideal world all vfio would use pci
or another probeable bus.)

thanks
-- PMM