| Message ID | 20170607211330.13266-1-cdall@linaro.org |
|---|---|
| State | Accepted |
| Commit | 70c9b44270f75bfb7a5701d81aa49380d139e8f0 |
| Headers | show |
On Wed, Jun 7, 2017 at 11:13 PM, Christoffer Dall <cdall@linaro.org> wrote: > The function to check if -chardev is supported by QEMU was written a > long time ago, where adding chardevs did not make sense on the fixed ARM > platforms. Since then, we now have a general purpose virt platform, > which should support plugging in any device over PCIe which is supported > in a similar fashion on x86. > > Signed-off-by: Christoffer Dall <cdall@linaro.org> > --- > src/qemu/qemu_capabilities.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 7f22492..1348af7 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -5507,6 +5507,11 @@ virQEMUCapsSupportsChardev(const virDomainDef *def, > if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != VIR_ARCH_AARCH64)) > return true; > > + /* The virt machine has a PCIe bus and allows plugging in the same type of > + * devices as x86 systems do on a PCIe bus. */ > + if (qemuDomainIsVirt(def)) > + return true; > + > /* This may not be true for all ARM machine types, but at least > * the only supported non-virtio serial devices of vexpress and versatile > * don't have the -chardev property wired up. */ > -- > 2.9.0 > ping? Thanks, -Christoffer -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2017-06-07 at 23:13 +0200, Christoffer Dall wrote: > The function to check if -chardev is supported by QEMU was written a > long time ago, where adding chardevs did not make sense on the fixed ARM > platforms. Since then, we now have a general purpose virt platform, > which should support plugging in any device over PCIe which is supported > in a similar fashion on x86. > > Signed-off-by: Christoffer Dall <cdall@linaro.org> > --- > src/qemu/qemu_capabilities.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 7f22492..1348af7 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -5507,6 +5507,11 @@ virQEMUCapsSupportsChardev(const virDomainDef *def, > if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != VIR_ARCH_AARCH64)) > return true; > > + /* The virt machine has a PCIe bus and allows plugging in the same type of > + * devices as x86 systems do on a PCIe bus. */ > + if (qemuDomainIsVirt(def)) > + return true; > + > /* This may not be true for all ARM machine types, but at least > * the only supported non-virtio serial devices of vexpress and versatile > * don't have the -chardev property wired up. */ We have two bugs tracking this issue: https://bugs.linaro.org/show_bug.cgi?id=2777 https://bugzilla.redhat.com/show_bug.cgi?id=1435681 You mention in [1] that applying this patch and using a recent QEMU fixes the problem for you, however I can't say the same: I still get -device isa-serial,chardev=charserial0,id=serial0: No 'ISA' bus found for device 'isa-serial' Would you mind sharing your guest XML and the resulting QEMU command line? [1] https://bugs.linaro.org/show_bug.cgi?id=2777#c36 -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jun 22, 2017 at 8:30 AM, Andrea Bolognani <abologna@redhat.com> wrote: > On Wed, 2017-06-07 at 23:13 +0200, Christoffer Dall wrote: >> The function to check if -chardev is supported by QEMU was written a >> long time ago, where adding chardevs did not make sense on the fixed ARM >> platforms. Since then, we now have a general purpose virt platform, >> which should support plugging in any device over PCIe which is supported >> in a similar fashion on x86. >> >> Signed-off-by: Christoffer Dall <cdall@linaro.org> >> --- >> src/qemu/qemu_capabilities.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c >> index 7f22492..1348af7 100644 >> --- a/src/qemu/qemu_capabilities.c >> +++ b/src/qemu/qemu_capabilities.c >> @@ -5507,6 +5507,11 @@ virQEMUCapsSupportsChardev(const virDomainDef *def, >> if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != VIR_ARCH_AARCH64)) >> return true; >> >> + /* The virt machine has a PCIe bus and allows plugging in the same type of >> + * devices as x86 systems do on a PCIe bus. */ >> + if (qemuDomainIsVirt(def)) >> + return true; >> + >> /* This may not be true for all ARM machine types, but at least >> * the only supported non-virtio serial devices of vexpress and versatile >> * don't have the -chardev property wired up. */ > > We have two bugs tracking this issue: > > https://bugs.linaro.org/show_bug.cgi?id=2777 > https://bugzilla.redhat.com/show_bug.cgi?id=1435681 > > You mention in [1] that applying this patch and using a > recent QEMU fixes the problem for you, however I can't > say the same: I still get > > -device isa-serial,chardev=charserial0,id=serial0: > No 'ISA' bus found for device 'isa-serial' > Well that's not the bug reported in 2777. If you try to create an ISA bus or configure your domain with an ISA bus on AArch64 your are bound to fail, because we never had, and we never will have, support for an ISA bus on AArch64. To verify what this patch changes, you can use the test xml file listed in [1] as well: <domain type='kvm'> <name>testlogfile</name> <memory unit='KiB'>524288</memory> <os> <type arch='aarch64' machine='virt-2.7'>hvm</type> </os> <devices> <serial type='pty'> <log file='/tmp/testlogfile.log' append='off'/> <target port='0'/> </serial> </devices> </domain> Or any working domain configuration where you add <log file='...' /> to the domain definition. It may be that we have an additional bug in libvirt that it under some circumstances tries to create an ISA bus with an AArch64 VM, but I don't see that being related to the patch above? Note that the submitted patch fixes virQEMUCapsSupportsChardev, which should be independent from any ISA bus fixes in libvirt, but given my very limited experience with libvirt, I may be wrong here. In summary, if your test setup goes from "error: unsupported configuration: logfile not supported in this QEMU binary" to "-device isa-serial,chardev=charserial0,id=serial0: No 'ISA' bus found for device 'isa-serial'" then I'd argue that my patch solves the first issue. Thanks, -Christoffer [1] https://bugs.linaro.org/show_bug.cgi?id=2777#c36 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, 2017-06-23 at 18:32 +0200, Christoffer Dall wrote: > > You mention in [1] that applying this patch and using a > > recent QEMU fixes the problem for you, however I can't > > say the same: I still get > > > > -device isa-serial,chardev=charserial0,id=serial0: > > No 'ISA' bus found for device 'isa-serial' > > Well that's not the bug reported in 2777. If you try to create an ISA > bus or configure your domain with an ISA bus on AArch64 your are bound > to fail, because we never had, and we never will have, support for an > ISA bus on AArch64. Sure, I'm aware of that. > To verify what this patch changes, you can use the test xml file > listed in [1] as well: > > <domain type='kvm'> > <name>testlogfile</name> > <memory unit='KiB'>524288</memory> > <os> > <type arch='aarch64' machine='virt-2.7'>hvm</type> > </os> > <devices> > <serial type='pty'> > <log file='/tmp/testlogfile.log' append='off'/> > <target port='0'/> > </serial> > </devices> > </domain> > > Or any working domain configuration where you add <log file='...' /> > to the domain definition. In both cases, I get the error reported in my previous message. You didn't really answer my question, though: can *you* start such a guest succesfully using a patched libvirt? And if so, what is the corresponding QEMU command line? > It may be that we have an additional bug in libvirt that it under some > circumstances tries to create an ISA bus with an AArch64 VM, but I > don't see that being related to the patch above? It's not. > Note that the submitted patch fixes virQEMUCapsSupportsChardev, which > should be independent from any ISA bus fixes in libvirt, but given my > very limited experience with libvirt, I may be wrong here. No, you're correct. > In summary, if your test setup goes from "error: unsupported > configuration: logfile not supported in this QEMU binary" to "-device > isa-serial,chardev=charserial0,id=serial0: No 'ISA' bus found for > device 'isa-serial'" then I'd argue that my patch solves the first > issue. The way I see it, the bug is about libvirt being unable to launch guests which use the <console><log> feature, and with that in mind your patch is correct but doesn't solve the issue, because even thought that specific error is gone you immediately run into a different one and your guest is still unable to start. Just to be clear: I'm not against this patch, we definitely want to fix virQEMUCapsSupportsChardev(). What gave me pause is simply the fact that you seemed to claim it made the <console><log> feature usable, which I'm still unconvinced is actually the case. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, Jun 24, 2017 at 3:37 PM, Andrea Bolognani <abologna@redhat.com> wrote: > On Fri, 2017-06-23 at 18:32 +0200, Christoffer Dall wrote: >> > You mention in [1] that applying this patch and using a >> > recent QEMU fixes the problem for you, however I can't >> > say the same: I still get >> > >> > -device isa-serial,chardev=charserial0,id=serial0: >> > No 'ISA' bus found for device 'isa-serial' >> >> Well that's not the bug reported in 2777. If you try to create an ISA >> bus or configure your domain with an ISA bus on AArch64 your are bound >> to fail, because we never had, and we never will have, support for an >> ISA bus on AArch64. > > Sure, I'm aware of that. > >> To verify what this patch changes, you can use the test xml file >> listed in [1] as well: >> >> <domain type='kvm'> >> <name>testlogfile</name> >> <memory unit='KiB'>524288</memory> >> <os> >> <type arch='aarch64' machine='virt-2.7'>hvm</type> >> </os> >> <devices> >> <serial type='pty'> >> <log file='/tmp/testlogfile.log' append='off'/> >> <target port='0'/> >> </serial> >> </devices> >> </domain> >> >> Or any working domain configuration where you add <log file='...' /> >> to the domain definition. > > In both cases, I get the error reported in my previous > message. > > You didn't really answer my question, though: can *you* > start such a guest succesfully using a patched libvirt? Yes, I can. But since I now left for holiday and I turned off my desktop at home, I cannot immediately dig out the details of a config domain xml file etc. Riku, did you have a chance to reproduce the issue with my patch as well? > And if so, what is the corresponding QEMU command line? > I'll be happy to dig this out for you when I return though. >> It may be that we have an additional bug in libvirt that it under some >> circumstances tries to create an ISA bus with an AArch64 VM, but I >> don't see that being related to the patch above? > > It's not. > >> Note that the submitted patch fixes virQEMUCapsSupportsChardev, which >> should be independent from any ISA bus fixes in libvirt, but given my >> very limited experience with libvirt, I may be wrong here. > > No, you're correct. > >> In summary, if your test setup goes from "error: unsupported >> configuration: logfile not supported in this QEMU binary" to "-device >> isa-serial,chardev=charserial0,id=serial0: No 'ISA' bus found for >> device 'isa-serial'" then I'd argue that my patch solves the first >> issue. > > The way I see it, the bug is about libvirt being unable to > launch guests which use the <console><log> feature, and with > that in mind your patch is correct but doesn't solve the > issue, because even thought that specific error is gone you > immediately run into a different one and your guest is still > unable to start. I didn't experience this, it was actually working on my end. I wonder if it's related to the QEMU version, where I seem to remember we changed what some serial options turned into. But I for sure did not see "-device isa-serial..." on the command line, so maybe not. In any case, I'll reproduce again when I'm back and send you the details. > > Just to be clear: I'm not against this patch, we definitely > want to fix virQEMUCapsSupportsChardev(). What gave me pause > is simply the fact that you seemed to claim it made the > <console><log> feature usable, which I'm still unconvinced > is actually the case. > Oh, I didn't intend to claim that. I intended to claim that <serial type='pty'> <log file='/tmp/testlogfile.log' append='off'/> <target port='0'/> now works. I'm not sure where I claimed more beyond that, can you point me to specifics (this patch or the bug report, etc.) and I'll be happy to correct that? At this point I'm a little confused about how to proceed here. Would you like further evidence of an environment that reproduces the issue with console and the isa bus, with additional logic added to this patch to fix that, or should we get this patch merged and fix the other issue separately? I'll try to look at reproducing the isa bus thing and seeing if I can come up with a fix when I'm back, unless someone beats me to it, which is not unlikely given the time it takes me to dig through libvirt abstraction layers. Thanks, -Christoffer -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, 2017-06-24 at 16:07 +0200, Christoffer Dall wrote: > > The way I see it, the bug is about libvirt being unable to > > launch guests which use the <console><log> feature, and with > > that in mind your patch is correct but doesn't solve the > > issue, because even thought that specific error is gone you > > immediately run into a different one and your guest is still > > unable to start. > > I didn't experience this, it was actually working on my end. I wonder > if it's related to the QEMU version, where I seem to remember we > changed what some serial options turned into. But I for sure did not > see "-device isa-serial..." on the command line, so maybe not. That's very different from the behavior I'm seeing, and I can't figure out why that would be the case. That's why having your QEMU command line would be very useful. As for differences in QEMU binaries, there might be some capability that I haven't considered and influences the generated command line. I'll look into that. > In any case, I'll reproduce again when I'm back and send you the details. Sounds good to me. > > Just to be clear: I'm not against this patch, we definitely > > want to fix virQEMUCapsSupportsChardev(). What gave me pause > > is simply the fact that you seemed to claim it made the > > <console><log> feature usable, which I'm still unconvinced > > is actually the case. > > Oh, I didn't intend to claim that. I intended to claim that > > <serial type='pty'> > <log file='/tmp/testlogfile.log' append='off'/> > <target port='0'/> > > now works. Well, that's the same thing, really :) Adding <serial type='pty'/> will automatically add <console type='pty'/>, so if one works the other should work as well, since they translate to a single QEMU option at the end of the day. > I'm not sure where I claimed more beyond that, can you > point me to specifics (this patch or the bug report, etc.) and I'll be > happy to correct that? https://bugs.linaro.org/show_bug.cgi?id=2777#c36 Also, twice in the message I'm replying to ;) > At this point I'm a little confused about how to proceed here. Would > you like further evidence of an environment that reproduces the issue > with console and the isa bus, with additional logic added to this > patch to fix that, or should we get this patch merged and fix the > other issue separately? We can merge the patch without further changes to it, as it fixes part of the issues that prevent the feature to work. Actually, I just added Reviewed-by: Andrea Bolognani <abologna@redhat.com> and pushed it :) > I'll try to look at reproducing the isa bus thing and seeing if I can > come up with a fix when I'm back, unless someone beats me to it, which > is not unlikely given the time it takes me to dig through libvirt > abstraction layers. I thought that fixing this would require QEMU changes, but Drew recently pointed out[1] that it might be possible to make it work using existing QEMU features only. I'll look into that in the next few days. Enjoy your vacation ^^ [1] https://bugzilla.redhat.com/show_bug.cgi?id=1456882#c4 -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Jun 25, 2017 at 4:07 AM, Andrea Bolognani <abologna@redhat.com> wrote: > On Sat, 2017-06-24 at 16:07 +0200, Christoffer Dall wrote: >> > The way I see it, the bug is about libvirt being unable to >> > launch guests which use the <console><log> feature, and with >> > that in mind your patch is correct but doesn't solve the >> > issue, because even thought that specific error is gone you >> > immediately run into a different one and your guest is still >> > unable to start. >> >> I didn't experience this, it was actually working on my end. I wonder >> if it's related to the QEMU version, where I seem to remember we >> changed what some serial options turned into. But I for sure did not >> see "-device isa-serial..." on the command line, so maybe not. > > That's very different from the behavior I'm seeing, and I > can't figure out why that would be the case. That's why > having your QEMU command line would be very useful. > > As for differences in QEMU binaries, there might be some > capability that I haven't considered and influences the > generated command line. I'll look into that. Cool, I'll have a look as well and will document my complete environment, then hopefully we can diff with yours and see where this ISA thing shows up. > >> In any case, I'll reproduce again when I'm back and send you the details. > > Sounds good to me. > >> > Just to be clear: I'm not against this patch, we definitely >> > want to fix virQEMUCapsSupportsChardev(). What gave me pause >> > is simply the fact that you seemed to claim it made the >> > <console><log> feature usable, which I'm still unconvinced >> > is actually the case. >> >> Oh, I didn't intend to claim that. I intended to claim that >> >> <serial type='pty'> >> <log file='/tmp/testlogfile.log' append='off'/> >> <target port='0'/> >> >> now works. > > Well, that's the same thing, really :) I didn't know that. > > Adding <serial type='pty'/> will automatically add > <console type='pty'/>, so if one works the other should > work as well, since they translate to a single QEMU option > at the end of the day. > Thanks for the explanation. >> I'm not sure where I claimed more beyond that, can you >> point me to specifics (this patch or the bug report, etc.) and I'll be >> happy to correct that? > > https://bugs.linaro.org/show_bug.cgi?id=2777#c36 > > Also, twice in the message I'm replying to ;) > Please forgive my libvirt ignorance, I didn't know that <serial> and <console> would end up doing the same thing. >> At this point I'm a little confused about how to proceed here. Would >> you like further evidence of an environment that reproduces the issue >> with console and the isa bus, with additional logic added to this >> patch to fix that, or should we get this patch merged and fix the >> other issue separately? > > We can merge the patch without further changes to it, as > it fixes part of the issues that prevent the feature to work. > > Actually, I just added > > Reviewed-by: Andrea Bolognani <abologna@redhat.com> > > and pushed it :) > >> I'll try to look at reproducing the isa bus thing and seeing if I can >> come up with a fix when I'm back, unless someone beats me to it, which >> is not unlikely given the time it takes me to dig through libvirt >> abstraction layers. > > I thought that fixing this would require QEMU changes, but > Drew recently pointed out[1] that it might be possible to > make it work using existing QEMU features only. I'll look > into that in the next few days. Sounds good, and let us know if we can help on the QEMU side. > > Enjoy your vacation ^^ Thanks, -Christoffer -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/24/2017 10:07 PM, Andrea Bolognani wrote: > On Sat, 2017-06-24 at 16:07 +0200, Christoffer Dall wrote: >>> The way I see it, the bug is about libvirt being unable to >>> launch guests which use the <console><log> feature, and with >>> that in mind your patch is correct but doesn't solve the >>> issue, because even thought that specific error is gone you >>> immediately run into a different one and your guest is still >>> unable to start. >> >> I didn't experience this, it was actually working on my end. I wonder >> if it's related to the QEMU version, where I seem to remember we >> changed what some serial options turned into. But I for sure did not >> see "-device isa-serial..." on the command line, so maybe not. > > That's very different from the behavior I'm seeing, and I > can't figure out why that would be the case. That's why > having your QEMU command line would be very useful. > > As for differences in QEMU binaries, there might be some > capability that I haven't considered and influences the > generated command line. I'll look into that. > >> In any case, I'll reproduce again when I'm back and send you the details. > > Sounds good to me. > >>> Just to be clear: I'm not against this patch, we definitely >>> want to fix virQEMUCapsSupportsChardev(). What gave me pause >>> is simply the fact that you seemed to claim it made the >>> <console><log> feature usable, which I'm still unconvinced >>> is actually the case. >> >> Oh, I didn't intend to claim that. I intended to claim that >> >> <serial type='pty'> >> <log file='/tmp/testlogfile.log' append='off'/> >> <target port='0'/> >> >> now works. > > Well, that's the same thing, really :) > > Adding <serial type='pty'/> will automatically add > <console type='pty'/>, so if one works the other should > work as well, since they translate to a single QEMU option > at the end of the day. > >> I'm not sure where I claimed more beyond that, can you >> point me to specifics (this patch or the bug report, etc.) and I'll be >> happy to correct that? > > https://bugs.linaro.org/show_bug.cgi?id=2777#c36 > > Also, twice in the message I'm replying to ;) > >> At this point I'm a little confused about how to proceed here. Would >> you like further evidence of an environment that reproduces the issue >> with console and the isa bus, with additional logic added to this >> patch to fix that, or should we get this patch merged and fix the >> other issue separately? > > We can merge the patch without further changes to it, as > it fixes part of the issues that prevent the feature to work. > > Actually, I just added > > Reviewed-by: Andrea Bolognani <abologna@redhat.com> > > and pushed it :) It may fix part of the issue but it likely breaks all existing aarch64 -M virt libvirt VMs. My VM created by virt-manager has: <serial type='pty'> <target port='0'/> </serial> <console type='pty'> <target type='serial' port='0'/> </console> And after this patch fails with: error: Failed to start domain fedora25-aarch64 error: internal error: process exited while connecting to monitor: 2017-06-26T13:55:34.726293Z qemu-system-aarch64: -chardev pty,id=charserial0: char device redirected to /dev/pts/5 (label charserial0) 2017-06-26T13:55:34.782121Z qemu-system-aarch64: -device isa-serial,chardev=charserial0,id=serial0: No 'ISA' bus found for device 'isa-serial' There's a few things going on here: - Internally libvirt thinks that by default <serial> is isa-serial - For arm qemu though it's actually a pl011. This is a platform device and as such there isn't any current way to use -chardev with it AFAIK. - However -chardev will work for pci-serial device, which is <serial><target type='serial'/> So for one, we should change SupportsChardev to also check the devices target type. The test suite likely needs to be extended to add QEMU_CAPS_CHARDEV for all arm/aarch64 test cases, to demonstrate the change. And describe the existing logic by adding a TARGET_TYPE_PL011 which is filled in as the default for arm/aarch64, and then we can key off that in the code. After that I think the options are either: 1) Openstack/other tools specifies target type='pci' for PCIe machvirt, to make <log> work 2) Change the libvirt serial default to target type=pci for PCIe machvirt. Not sure if they are operationally equivalent for all cases we care about though. Maybe we could make some kind of adaptive default like 'if PCIe machvirt && serial device has <log> specified (or other features) then default target type=pci', but maybe that's too magic Would also be nice to have the libvirt output XML always list the serial target type which would clear up some of this confusion, but might cause migration XML compat issues for other archs In the interim though I think this patch should be reverted. Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jun 26, 2017 at 10:10:55AM -0400, Cole Robinson wrote: > On 06/24/2017 10:07 PM, Andrea Bolognani wrote: > > On Sat, 2017-06-24 at 16:07 +0200, Christoffer Dall wrote: > >> At this point I'm a little confused about how to proceed here. Would > >> you like further evidence of an environment that reproduces the issue > >> with console and the isa bus, with additional logic added to this > >> patch to fix that, or should we get this patch merged and fix the > >> other issue separately? > > > > We can merge the patch without further changes to it, as > > it fixes part of the issues that prevent the feature to work. > > > > Actually, I just added > > > > Reviewed-by: Andrea Bolognani <abologna@redhat.com> > > > > and pushed it :) > > It may fix part of the issue but it likely breaks all existing aarch64 -M virt > libvirt VMs. My VM created by virt-manager has: > > <serial type='pty'> > <target port='0'/> > </serial> > <console type='pty'> > <target type='serial' port='0'/> > </console> > > And after this patch fails with: > > error: Failed to start domain fedora25-aarch64 > error: internal error: process exited while connecting to monitor: > 2017-06-26T13:55:34.726293Z qemu-system-aarch64: -chardev pty,id=charserial0: > char device redirected to /dev/pts/5 (label charserial0) > 2017-06-26T13:55:34.782121Z qemu-system-aarch64: -device > isa-serial,chardev=charserial0,id=serial0: No 'ISA' bus found for device > 'isa-serial' > > There's a few things going on here: > > - Internally libvirt thinks that by default <serial> is isa-serial > - For arm qemu though it's actually a pl011. This is a platform device and as > such there isn't any current way to use -chardev with it AFAIK. There is, -chardev pty,id=chardev0,logfile=/my/log/file \ -serial chardev:chardev0 > - However -chardev will work for pci-serial device, which is <serial><target > type='serial'/> This works, but would require the guest kernel to have console=ttyS0 on its command line, and adds an unnecessary extra serial device to the guest, as it already has the PL011. > > So for one, we should change SupportsChardev to also check the devices target > type. The test suite likely needs to be extended to add QEMU_CAPS_CHARDEV for > all arm/aarch64 test cases, to demonstrate the change. And describe the > existing logic by adding a TARGET_TYPE_PL011 which is filled in as the default > for arm/aarch64, and then we can key off that in the code. > > After that I think the options are either: > > 1) Openstack/other tools specifies target type='pci' for PCIe machvirt, to > make <log> work > > 2) Change the libvirt serial default to target type=pci for PCIe machvirt. Not > sure if they are operationally equivalent for all cases we care about though. > Maybe we could make some kind of adaptive default like 'if PCIe machvirt && > serial device has <log> specified (or other features) then default target > type=pci', but maybe that's too magic 3) magic, but magic that enables the PL011 to be the one and only default serial console device for mach-virt. The magic I advocate using is the -serial 'chardev:<chardev>' shown above. > > Would also be nice to have the libvirt output XML always list the serial > target type which would clear up some of this confusion, but might cause > migration XML compat issues for other archs > > In the interim though I think this patch should be reverted. Reverted, or quickly completed. It's not wrong, but apparently it needs to also ensure pci-serial is chosen for ARM guest targets. Thanks, drew -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/26/2017 11:19 AM, Andrew Jones wrote: > On Mon, Jun 26, 2017 at 10:10:55AM -0400, Cole Robinson wrote: >> On 06/24/2017 10:07 PM, Andrea Bolognani wrote: >>> On Sat, 2017-06-24 at 16:07 +0200, Christoffer Dall wrote: >>>> At this point I'm a little confused about how to proceed here. Would >>>> you like further evidence of an environment that reproduces the issue >>>> with console and the isa bus, with additional logic added to this >>>> patch to fix that, or should we get this patch merged and fix the >>>> other issue separately? >>> >>> We can merge the patch without further changes to it, as >>> it fixes part of the issues that prevent the feature to work. >>> >>> Actually, I just added >>> >>> Reviewed-by: Andrea Bolognani <abologna@redhat.com> >>> >>> and pushed it :) >> >> It may fix part of the issue but it likely breaks all existing aarch64 -M virt >> libvirt VMs. My VM created by virt-manager has: >> >> <serial type='pty'> >> <target port='0'/> >> </serial> >> <console type='pty'> >> <target type='serial' port='0'/> >> </console> >> >> And after this patch fails with: >> >> error: Failed to start domain fedora25-aarch64 >> error: internal error: process exited while connecting to monitor: >> 2017-06-26T13:55:34.726293Z qemu-system-aarch64: -chardev pty,id=charserial0: >> char device redirected to /dev/pts/5 (label charserial0) >> 2017-06-26T13:55:34.782121Z qemu-system-aarch64: -device >> isa-serial,chardev=charserial0,id=serial0: No 'ISA' bus found for device >> 'isa-serial' >> >> There's a few things going on here: >> >> - Internally libvirt thinks that by default <serial> is isa-serial >> - For arm qemu though it's actually a pl011. This is a platform device and as >> such there isn't any current way to use -chardev with it AFAIK. > > There is, > > -chardev pty,id=chardev0,logfile=/my/log/file \ > -serial chardev:chardev0 > Ah interesting I didn't know about that, thanks. I sent some patches that convert to use that method for platform serial devices Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7f22492..1348af7 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5507,6 +5507,11 @@ virQEMUCapsSupportsChardev(const virDomainDef *def, if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != VIR_ARCH_AARCH64)) return true; + /* The virt machine has a PCIe bus and allows plugging in the same type of + * devices as x86 systems do on a PCIe bus. */ + if (qemuDomainIsVirt(def)) + return true; + /* This may not be true for all ARM machine types, but at least * the only supported non-virtio serial devices of vexpress and versatile * don't have the -chardev property wired up. */
The function to check if -chardev is supported by QEMU was written a long time ago, where adding chardevs did not make sense on the fixed ARM platforms. Since then, we now have a general purpose virt platform, which should support plugging in any device over PCIe which is supported in a similar fashion on x86. Signed-off-by: Christoffer Dall <cdall@linaro.org> --- src/qemu/qemu_capabilities.c | 5 +++++ 1 file changed, 5 insertions(+) -- 2.9.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list