| Message ID | 20180419220914.959-2-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19 April 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > 4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent > use of Chardev->be. Also, this CharBackend member is private and is > not supposed to be accessible. > > Fix it by removing the inconsistent check. > > Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/isa/isa-superio.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c > index b95608a003..08afe44731 100644 > --- a/hw/isa/isa-superio.c > +++ b/hw/isa/isa-superio.c > @@ -43,7 +43,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) > if (!k->parallel.is_enabled || k->parallel.is_enabled(sio, i)) { > /* FIXME use a qdev chardev prop instead of parallel_hds[] */ > chr = parallel_hds[i]; > - if (chr == NULL || chr->be) { > + if (chr == NULL) { > name = g_strdup_printf("discarding-parallel%d", i); > chr = qemu_chr_new(name, "null"); > } else { > @@ -83,7 +83,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) > if (!k->serial.is_enabled || k->serial.is_enabled(sio, i)) { > /* FIXME use a qdev chardev prop instead of serial_hds[] */ > chr = serial_hds[i]; > - if (chr == NULL || chr->be) { > + if (chr == NULL) { > name = g_strdup_printf("discarding-serial%d", i); > chr = qemu_chr_new(name, "null"); > } else { You should not need to create fake null devices like this. The device using the chardev should just cope with having a NULL pointer now we have commit 12051d82f004024. Also consider having cc:stable on this patchset? thanks -- PMM
Hi Peter, On 04/20/2018 05:43 AM, Peter Maydell wrote: > On 19 April 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> 4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent >> use of Chardev->be. Also, this CharBackend member is private and is >> not supposed to be accessible. >> >> Fix it by removing the inconsistent check. >> >> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/isa/isa-superio.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c >> index b95608a003..08afe44731 100644 >> --- a/hw/isa/isa-superio.c >> +++ b/hw/isa/isa-superio.c >> @@ -43,7 +43,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) >> if (!k->parallel.is_enabled || k->parallel.is_enabled(sio, i)) { >> /* FIXME use a qdev chardev prop instead of parallel_hds[] */ >> chr = parallel_hds[i]; >> - if (chr == NULL || chr->be) { >> + if (chr == NULL) { >> name = g_strdup_printf("discarding-parallel%d", i); >> chr = qemu_chr_new(name, "null"); >> } else { >> @@ -83,7 +83,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) >> if (!k->serial.is_enabled || k->serial.is_enabled(sio, i)) { >> /* FIXME use a qdev chardev prop instead of serial_hds[] */ >> chr = serial_hds[i]; >> - if (chr == NULL || chr->be) { >> + if (chr == NULL) { >> name = g_strdup_printf("discarding-serial%d", i); >> chr = qemu_chr_new(name, "null"); >> } else { > > You should not need to create fake null devices like this. The > device using the chardev should just cope with having a NULL > pointer now we have commit 12051d82f004024. I am trying to model a SoC with 4 uarts, and started using the current pattern: for (i = 0; i < MAX_SERIAL_PORTS; i+++) { if (serial_hds[i]) { serial_mm_init(..., serial_hds[i], ...); Then the test firmware failed due to unassigned access after rebasing, due to: commit ed860129acd3fcd0b1e47884e810212aaca4d21b Author: Peter Maydell <peter.maydell@linaro.org> Date: Thu Sep 7 13:54:54 2017 +0100 boards.h: Define new flag ignore_memory_transaction_failures Define a new MachineClass field ignore_memory_transaction_failures. If this is flag is true then the CPU will ignore memory transaction failures which should cause the CPU to take an exception due to an access to an unassigned physical address; the transaction will instead return zero (for a read) or be ignored (for a write). This should be set only by legacy board models which rely on the old RAZ/WI behaviour for handling devices that QEMU does not yet model. New board models 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. We need this for ARM boards, where we're about to implement support for generating external aborts on memory transaction failures. Too many of our legacy board models rely on the RAZ/WI behaviour and we would break currently working guests when their "probe for device" code provoked an external abort rather than a RAZ. Which is fine, I understand new boards shouldn't use the ignore_memory_transaction_failures flag. The chardev subsystem is still confuse to me. I think than checking if a chardev backend is connected to instantiate and mmap a device and his irqs is incorrect, since boards/SoCs always come with the same hardware. Indeed previous to 12051d82f004024, qemu_chr_fe_init(s=NULL) was doing if (CHARDEV_IS_MUX(s)) { ... } else { s->be = b; /* NULL deref... */ } Which seems the original reason of my bogus "if (chr == NULL || chr->be) ..." check. Since we can now safely call serial_mm_init(..., chr=NULL, ...), I'll work on simplifying and fixing this. > Also consider having cc:stable on this patchset? Once we get a consensus on the series, OK. Thanks, Phil.
On 20 April 2018 at 13:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi Peter, > > On 04/20/2018 05:43 AM, Peter Maydell wrote: >> On 19 April 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>> 4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent >>> use of Chardev->be. Also, this CharBackend member is private and is >>> not supposed to be accessible. >> You should not need to create fake null devices like this. The >> device using the chardev should just cope with having a NULL >> pointer now we have commit 12051d82f004024. > > I am trying to model a SoC with 4 uarts, and started using the current > pattern: > > for (i = 0; i < MAX_SERIAL_PORTS; i+++) { > if (serial_hds[i]) { > serial_mm_init(..., serial_hds[i], ...); This is wrong, because it means that the UART device will only be visible to the guest if the user connected it to something (with -serial whatever). You should always create the same number of devices, which correspond to what hard wired UARTs the board has. (The exception would I guess be if the devices were pluggable, so each serial device was a pluggable PCI UART or something weird; in that case there's an argument that -serial means "create and plug in one of these devices". x86 is probably also different for legacy reasons.) > Since we can now safely call serial_mm_init(..., chr=NULL, ...), I'll > work on simplifying and fixing this. Correct. There are a bunch of dubious workarounds in the codebase which try to handle the problem at different levels, but we've decided that the right thing is for the NULL pointer to be dealt with in one place at the bottom (in the qemu_chr_fe_* functions). Incidentally I was wondering if we could sensibly get rid of the compile time MAX_SERIAL_PORTS. Right now there's no way to model a device with five UARTs such that they can all be sent to interesting places. I think there's no reason for this except that the PC traditionally has 4 UARTs and nobody's ever changed the code... thanks -- PMM
On 04/20/2018 09:59 AM, Peter Maydell wrote: > On 20 April 2018 at 13:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> On 04/20/2018 05:43 AM, Peter Maydell wrote: >>> On 19 April 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>> 4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent >>>> use of Chardev->be. Also, this CharBackend member is private and is >>>> not supposed to be accessible. > >>> You should not need to create fake null devices like this. The >>> device using the chardev should just cope with having a NULL >>> pointer now we have commit 12051d82f004024. >> >> I am trying to model a SoC with 4 uarts, and started using the current >> pattern: >> >> for (i = 0; i < MAX_SERIAL_PORTS; i+++) { >> if (serial_hds[i]) { >> serial_mm_init(..., serial_hds[i], ...); > > This is wrong, because it means that the UART device will > only be visible to the guest if the user connected it to > something (with -serial whatever). You should always create > the same number of devices, which correspond to what > hard wired UARTs the board has. Agreed! > (The exception would I guess be if the devices were pluggable, > so each serial device was a pluggable PCI UART or something > weird; in that case there's an argument that -serial means > "create and plug in one of these devices". x86 is probably > also different for legacy reasons.) Good example, also USB uarts. >> Since we can now safely call serial_mm_init(..., chr=NULL, ...), I'll >> work on simplifying and fixing this. > > Correct. There are a bunch of dubious workarounds in the > codebase which try to handle the problem at different levels, > but we've decided that the right thing is for the NULL pointer > to be dealt with in one place at the bottom (in the qemu_chr_fe_* > functions). OK, I'll happily clean that :) > Incidentally I was wondering if we could sensibly get rid of > the compile time MAX_SERIAL_PORTS. Right now there's no way > to model a device with five UARTs such that they can all > be sent to interesting places. I think there's no reason > for this except that the PC traditionally has 4 UARTs and > nobody's ever changed the code... Yes, I thought the same and planned to relax this limit (and few others) in my "remove i386/pc dependency from non-PC world" series...
On 20 April 2018 at 14:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 04/20/2018 09:59 AM, Peter Maydell wrote: >> Incidentally I was wondering if we could sensibly get rid of >> the compile time MAX_SERIAL_PORTS. Right now there's no way >> to model a device with five UARTs such that they can all >> be sent to interesting places. I think there's no reason >> for this except that the PC traditionally has 4 UARTs and >> nobody's ever changed the code... > > Yes, I thought the same and planned to relax this limit (and few others) > in my "remove i386/pc dependency from non-PC world" series... It seems fairly straightforward so I've put some patches together, might be able to get them onto the list this afternoon. thanks -- PMM
>>> On 04/20/2018 05:43 AM, Peter Maydell wrote: >>> Since we can now safely call serial_mm_init(..., chr=NULL, ...), I'll >>> work on simplifying and fixing this. >> >> Correct. There are a bunch of dubious workarounds in the >> codebase which try to handle the problem at different levels, >> but we've decided that the right thing is for the NULL pointer >> to be dealt with in one place at the bottom (in the qemu_chr_fe_* >> functions). > > OK, I'll happily clean that :) Hmmm removing the "if (serial_hds[0])" check I reach: void serial_realize_core(SerialState *s, Error **errp) { if (!qemu_chr_fe_backend_connected(&s->chr)) { error_setg(errp, "Can't create serial device, empty char device"); return; }
On 20 April 2018 at 15:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>> On 04/20/2018 05:43 AM, Peter Maydell wrote: >>>> Since we can now safely call serial_mm_init(..., chr=NULL, ...), I'll >>>> work on simplifying and fixing this. >>> >>> Correct. There are a bunch of dubious workarounds in the >>> codebase which try to handle the problem at different levels, >>> but we've decided that the right thing is for the NULL pointer >>> to be dealt with in one place at the bottom (in the qemu_chr_fe_* >>> functions). >> >> OK, I'll happily clean that :) > > Hmmm removing the "if (serial_hds[0])" check I reach: > > void serial_realize_core(SerialState *s, Error **errp) > { > if (!qemu_chr_fe_backend_connected(&s->chr)) { > error_setg(errp, "Can't create serial device, empty char device"); > return; > } Yeah. That check is unnecessary; nothing fails to work if it is removed. I've got a patch deleting it in my "drop MAX_SERIAL_PORTS" series. thanks -- PMM
diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c index b95608a003..08afe44731 100644 --- a/hw/isa/isa-superio.c +++ b/hw/isa/isa-superio.c @@ -43,7 +43,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) if (!k->parallel.is_enabled || k->parallel.is_enabled(sio, i)) { /* FIXME use a qdev chardev prop instead of parallel_hds[] */ chr = parallel_hds[i]; - if (chr == NULL || chr->be) { + if (chr == NULL) { name = g_strdup_printf("discarding-parallel%d", i); chr = qemu_chr_new(name, "null"); } else { @@ -83,7 +83,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) if (!k->serial.is_enabled || k->serial.is_enabled(sio, i)) { /* FIXME use a qdev chardev prop instead of serial_hds[] */ chr = serial_hds[i]; - if (chr == NULL || chr->be) { + if (chr == NULL) { name = g_strdup_printf("discarding-serial%d", i); chr = qemu_chr_new(name, "null"); } else {
4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent use of Chardev->be. Also, this CharBackend member is private and is not supposed to be accessible. Fix it by removing the inconsistent check. Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/isa/isa-superio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)