Re: [PATCH QEMU v5] hw/arm/sysbus-fdt: Add support for instantiating generic devices
So I should start out upfront by saying that I'm aware that the reality is that people do want to do passthrough with this kind of hardware, and that's not an unreasonable thing to do. I just don't really like the way that pushes the software into having to do ugly things... Overall I'll let Eric call the shots about how well this feature fits into our sysbus-fdt support; he knows this code much better than I do. (I would have preferred us not to have sysbus-fdt passthrough at all, in an ideal world :-)) On Wed, 9 Jan 2019 at 16:30, Geert Uytterhoeven wrote: > On Wed, Jan 9, 2019 at 5:03 PM Peter Maydell wrote: > > whitelists for the devices we want to allow passthrough of and > > that we've tested to work. > > In the end, this will become a loong list (SoC x devices)... Well, yes, but it deters people from trying to do passthrough with hardware that's not designed in a way that makes passthrough reasonably supportable at a software level. > Reality is that on embedded, on-SoC devices are usually not on a PCI bus. > But IOMMUs are present, and virtualization is wanted. I don't insist on PCI. Probeable, and consistent in terms of what they provide and how you interact with them, is what we want. Embedded on-SoC devices are generally neither. (The host kernel could in theory provide an API that exposed only devices that were safely pass-through-able in a consistent way, I suppose, but it doesn't, or we wouldn't be having to fish through the device tree nodes making guesses about what's safe to allow and what isn't and having heuristics about not providing clocks info being ok if we think the clock might only be used for power management...) thanks -- PMM
Re: [PATCH QEMU v5] hw/arm/sysbus-fdt: Add support for instantiating generic devices
On Wed, 9 Jan 2019 at 15:55, Auger Eric wrote:
>
> Hi Geert,
>
> On 1/3/19 10:42 AM, Geert Uytterhoeven wrote:
> > Add a fallback for instantiating generic devices without a type-specific
> > or compatible-specific instantiation method. This will be used when no
> > other match is found.
> >
> > Generic device instantiation avoids having to write device-specific
> > instantiation methods for each and every "simple" device using only a
> > set of generic properties. Devices that need more specialized handling
> > can still provide their own instantiation methods.
> > +/* Ignoring the following may or may not work, hence the warning */
> > +{ "gpio-ranges", PROP_WARN }, /* no support for pinctrl yet */
> > +{ "dmas",PROP_WARN }, /* no support for external DMACs
> > yet */
> I would be tempted to simply reject things that may not work.
More generally, this whole feature seems to be "allow things that
might not work", isn't it? Otherwise we could just have explicit
whitelists for the devices we want to allow passthrough of and
that we've tested to work.
I have to say I'm not really very enthusiastic about
enhancing this to allow random device passthrough,
because it encourages further use of this. If people
want hardware that can be passed through they should
put it behind a bus that can be probed and can go
behind an IOMMU, ie pci or some equivalent. That
is a supportable hardware mechanism. All this
machinery feels very heath-robinson...
thanks
-- PMM
Re: [Qemu-devel] [PATCH v3 RESEND 3/3] nvram: at24c: use standard error reporting
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
On 12 April 2018 at 14:55, 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 > > 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 > --- > 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
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
On 9 February 2018 at 15:37, Geert Uytterhoeven wrote:
> Hi Peter,
>
> On Fri, Feb 9, 2018 at 4:27 PM, Peter Maydell
> wrote:
>> 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
>
> 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
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
