Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume
On Sat, Aug 25, 2018 at 03:57:19PM +0200, Marc-André Lureau wrote: > There is no need for per-command need_resume granularity, it should > resume after running an non-oob command on oob-disabled monitor. > > Signed-off-by: Marc-André Lureau > Reviewed-by: Markus Armbruster Note that this series/patch still conflict with the "enable out-of-band by default" series. [PATCH v6 00/13] monitor: enable OOB by default I'm not against this patch to be merged since it has its r-b, but I feel like we'd better judge on whether we still like the response queue first, in case one day we'll need to add these things back. When there could be functional changes around the code path I would think we'd better keep the cleanup patches postponed a bit until those functional changes are settled. For now the functional part is decide how to fix up the rest of out-of-band issues (my proposal is in the series above which should solve everything that is related to out-of-band to be fixed; if there is more, I'll continue to work on it), whether we should enable it by default for 3.1 (my answer is... yes...), and what to do with it. If we found that it's too hard to enable it by default, I'm thinking whether we can make it a persistent flag for monitor (maybe turning the "x-oob" into a real "oob" and keep it, then we don't turn it on by default), then we can let libvirt start working with out-of-band with the flag. After all it's actually working mostly (the pending issues are only things like flow control for malicious/buggy clients, but libvirt never had such an issue with it). Regards, -- Peter Xu
Re: [Qemu-devel] [PATCH v2] gtk: add zoom-to-fit to gtk options.
Gerd Hoffmann writes:
> This allows to set the option on the command line, i.e. "-display
> gtk,zoom-to-fit={on,off}", overriding the default choosen by qemu.
chosen
>
> Signed-off-by: Gerd Hoffmann
> ---
> ui/gtk.c | 8
> qapi/ui.json | 6 +-
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 5cce6ed42d..3ddb5fe162 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2136,6 +2136,8 @@ static GSList *gd_vc_gfx_init(GtkDisplayState *s,
> VirtualConsole *vc,
>QemuConsole *con, int idx,
>GSList *group, GtkWidget *view_menu)
> {
> +bool zoom_to_fit;
> +
> vc->label = qemu_console_get_label(con);
> vc->s = s;
> vc->gfx.scale_x = 1.0;
> @@ -2199,6 +2201,12 @@ static GSList *gd_vc_gfx_init(GtkDisplayState *s,
> VirtualConsole *vc,
> group = gd_vc_menu_init(s, vc, idx, group, view_menu);
>
> if (dpy_ui_info_supported(vc->gfx.dcl.con)) {
> +zoom_to_fit = true;
> +}
> +if (s->opts->u.gtk.has_zoom_to_fit) {
> +zoom_to_fit = s->opts->u.gtk.zoom_to_fit;
> +}
> +if (zoom_to_fit) {
> gtk_menu_item_activate(GTK_MENU_ITEM(s->zoom_fit_item));
> s->free_scale = true;
> }
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 4ca91bb45a..4d2387ce7b 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1022,12 +1022,16 @@
> # GTK display options.
> #
> # @grab-on-hover: Grab keyboard input on mouse hover.
> +# @zoom-to-fit: Zoom guest display to fit into the host window. When
> +# turned off the host window will be resized instead
> +# (since 3.1).
What's the default?
The cover letter says its chosen by QEMU. Matches my reading of the
patch. Please document it.
> #
> # Since: 2.12
> #
> ##
> { 'struct' : 'DisplayGTK',
> - 'data': { '*grab-on-hover' : 'bool' } }
> + 'data': { '*grab-on-hover' : 'bool',
> +'*zoom-to-fit' : 'bool' } }
>
> ##
> # @DisplayGLMode:
With a suitable doc fix
Reviewed-by: Markus Armbruster
[Qemu-devel] [Bug 1129571] Re: libreoffice armhf FTBFS
[Expired for qemu (Ubuntu) because there has been no activity for 60 days.] ** Changed in: qemu (Ubuntu) Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1129571 Title: libreoffice armhf FTBFS Status in QEMU: Won't Fix Status in qemu package in Ubuntu: Expired Bug description: We have been experiencing FTBFS of LibreOffice 3.5.7, 12.04, armhf in the launchpad buildds. We believe this is likely due to an error in qemu. While we do not have a small test case yet, we do have a build log (attaching here). The relevant snippet from the build log is: 3.5.7/solver/unxlngr.pro/bin/jaxp.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/juh.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/parser.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/xt.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/unoil.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/ridl.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/jurt.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/xmlsearch.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/LuceneHelpWrapper.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/HelpIndexerTool.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/lucene-core-2.3.jar:/build/buildd/libreoffice-3.5.7/solver/unxlngr.pro/bin/lucene-analyzers-2.3.jar" com.sun.star.help.HelpIndexerTool -lang cs -mod swriter -zipdir ../../unxlngr.pro/misc/ziptmpswriter_cs -o ../../unxlngr.pro/bin/swriter_cs.zip.unxlngr.pro dmake: Error code 132, while making '../../unxlngr.pro/bin/swriter_cs.zip' We believe this is from bash error code 128 + 4, where 4 is illegal instruction, thus leading us to suspect qemu. Any help in tracking this down would be appreciated. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1129571/+subscriptions
[Qemu-devel] [Bug 474968] Re: kvm smb server hogs cpu guest freeze
[Expired for qemu-kvm (Ubuntu) because there has been no activity for 60 days.] ** Changed in: qemu-kvm (Ubuntu) Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/474968 Title: kvm smb server hogs cpu guest freeze Status in QEMU: Won't Fix Status in qemu-kvm package in Ubuntu: Expired Bug description: Binary package hint: qemu-kvm kvm hogs the CPU reproducibly. I installed an Ubuntu using KVM. I run the machine with -net nic,model=rtl8139,macaddr=f0:00:BA:12:34:56 -net user,hostfwd=tcp::2223-:22,smb=/tmp/share, sshed into the machine and typed "telnet 10.0.2.4 139" to try whether the SMB server works. KVM then hogs the CPU. ProblemType: Bug Architecture: amd64 Date: Thu Nov 5 01:23:09 2009 DistroRelease: Ubuntu 9.10 KvmCmdLine: Error: command ['ps', '-C', 'kvm', '-F'] failed with exit code 1: UIDPID PPID CSZ RSS PSR STIME TTY TIME CMD MachineType: LENOVO 766636G Package: kvm 1:84+dfsg-0ubuntu16+0.11.0+0ubuntu6.3 PccardctlIdent: Socket 0: no product info available PccardctlStatus: Socket 0: no card ProcCmdLine: root=/dev/mapper/cryptroot source=UUID=9c3d5596-27c6-4fd5-bfcd-fa8eef6f1230 ro quiet splash crashkernel=384M-2G:64M,2G-:128M SourcePackage: qemu-kvm Uname: Linux 2.6.32-999-generic x86_64 dmi.bios.date: 07/01/2008 dmi.bios.vendor: LENOVO dmi.bios.version: 7NETB6WW (2.16 ) dmi.board.name: 766636G dmi.board.vendor: LENOVO dmi.board.version: Not Available dmi.chassis.asset.tag: No Asset Information dmi.chassis.type: 10 dmi.chassis.vendor: LENOVO dmi.chassis.version: Not Available dmi.modalias: dmi:bvnLENOVO:bvr7NETB6WW(2.16):bd07/01/2008:svnLENOVO:pn766636G:pvrThinkPadX61s:rvnLENOVO:rn766636G:rvrNotAvailable:cvnLENOVO:ct10:cvrNotAvailable: dmi.product.name: 766636G dmi.product.version: ThinkPad X61s dmi.sys.vendor: LENOVO To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/474968/+subscriptions
Re: [Qemu-devel] [PULL 12/25] tests: Clean up string interpolation into QMP input (simple cases)
Eric Blake writes:
> On 08/16/2018 03:36 AM, Markus Armbruster wrote:
>> When you build QMP input manually like this
>>
>> cmd = g_strdup_printf("{ 'execute': 'migrate',"
>>"'arguments': { 'uri': '%s' } }",
>>uri);
>> rsp = qmp(cmd);
>> g_free(cmd);
>>
>> you're responsible for escaping the interpolated values for JSON. Not
>> done here, and therefore works only for sufficiently nice @uri. For
>> instance, if @uri contained a single "'", qobject_from_vjsonf_nofail()
>> would abort. A sufficiently nasty @uri could even inject unwanted
>> members into the arguments object.
>>
>> Leaving interpolation into JSON to qmp() is more robust:
>>
>> rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
>>
>> It's also more concise.
>>
>> Clean up the simple cases where we interpolate exactly a JSON value.
>>
>> Bonus: gets rid of non-literal format strings. A step towards
>> compile-time format string checking without triggering
>> -Wformat-nonliteral.
>>
>
>> +++ b/tests/test-qga.c
>
>> @@ -446,10 +443,10 @@ static void test_qga_file_ops(gconstpointer fix)
>> enc = g_base64_encode(helloworld, sizeof(helloworld));
>> /* write */
>> -cmd = g_strdup_printf("{'execute': 'guest-file-write',"
>> - " 'arguments': { 'handle': %" PRId64 ","
>> - " 'buf-b64': '%s' } }", id, enc);
>> -ret = qmp_fd(fixture->fd, cmd);
>> +ret = qmp_fd(fixture->fd,
>> + "{'execute': 'guest-file-write',"
>> + " 'arguments': { 'handle': %" PRId64 ", 'buf-b64': %s } }",
>> + id, enc);
>
> This is a temporary regression of commit 1792d7d0, which affects test
> execution on Mac (with its weird %qd expansion for
> PRId64). Fortunately, you later fix it in commit 41, so I won't be
> upset if the pull request goes through without a v2 that avoids 'git
> bisect' breaking on Mac.
Sorry about the accidental revert of your patch. I rely on Peter's
testing to cover Mac.
This patch became commit 015715f554f, merged in commit c542a9f9794 on
August 16. The "later fix" is commit 53a0d616fec, merged in commit
cc9821fa9ac on August 25.
Re: [Qemu-devel] [PATCH v3 2/2] hw/pci: add PCI resource reserve capability to legacy PCI bridge
Hi Marcel, On 8/25/2018 12:51 AM, Marcel Apfelbaum wrote: On 08/21/2018 06:18 AM, Jing Liu wrote: Add hint to firmware (e.g. SeaBIOS) to reserve addtional BUS/IO/MEM/PREF resource for legacy pci-pci bridge. Add the resource reserve capability deleting in pci_bridge_dev_exitfn. Signed-off-by: Jing Liu --- hw/pci-bridge/pci_bridge_dev.c | 24 1 file changed, 24 insertions(+) [...] Reviewed-by: Marcel Apfelbaum Thanks for the quick reviewing and feedback. So could I ask what I should do now, update a new version with your rb or just waiting for pushing, or else? You just need to wait until Michael adds the series to his next pull request. Nothing more to do. OK, got it! Thanks! Please ping Michael if your code is not merged in a week or so. Got it, I will check by "git pull" to see if master branch has that. BTW, do you have some suggestion on the seabios counterpart patches? https://patchew.org/Seabios/1534386737-8131-1-git-send-email-jing2@linux.intel.com/ I plan to have a look this weekend. Glad to see your comments! Thanks very much, Jing Thanks, Marcel Jing Thanks, Marcel Thanks, Jing Thanks, Marcel
Re: [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts
On Fri, Aug 24, 2018 at 03:46:03PM +0200, Marc-André Lureau wrote: > Hi, > > On Fri, Aug 24, 2018 at 11:10 AM Peter Xu wrote: > > > > This is a RFC series. It majorly did these things: > > > > (1) move the monitor iothread management from monitor code to chardev > > code somehow, > > > > (2) decide which context/iothread to use for the chardev before > > chardev creates, by parsing monitor options earlier (not init, but > > only parsing) since monitor is the only exception now, > > > > (2) disallow chardev context to change for the whole lifecycle. > > > > Basically this work moves the only chardev iothread (the monitor > > iothread) into chardev's management, then it's easy to setup the > > iothread even before the chardev creates, hence no context switch for > > chardev is needed any more. In the future if we want to let any > > chardev to run on some other threads, we just define a new > > ChardevContext, then do qemu_opt_set_number(...) for the chardev. For > > now, there is only a monitor context. > > > > It does not mean that this will let chardev depend on monitor code, > > instead it'll totally remove the reverse dependency - before this > > work, the chardev backend strangely depends on the frontend to setup > > the context (which brought us many trouble). Now this should be gone. > > > > This series should solve the potential issue raised here: > > > > https://patchwork.kernel.org/patch/10122713/#22187395 > > > > And also should let Marc-André's vhost-user reconnect series work > > without breaking context switch of chardev (since it never switches > > now hence no way to break): > > > > http://patchwork.ozlabs.org/cover/961442/ > > > > Only smoke test carried out with out-of-band, and make check. > > > > Please have a look on whether this is a workable solution, > > Clever hack! > > The code could be simplified somewhat: > - it probably doesn't need ChardevContextMap Is it because we only have monitor to use it? If so, I suspect we might still want it since I'm just thinking maybe we can add a second one for COLO... [2] > - I would not typedef ChardevContext (took me a while to realize it was an > enum) Maybe, ChardevContextType? > - we should avoid the "context" option, perhaps during chardev > parsing, check -mon usage. [2] > - more :) > > Other issues: > - blend monitor code in chardev As I mentioned in the cover letter, my intention is not to blend monitor code into chardev. For example, we can define the name as CHR_CONTEXT_1 rather than CHR_CONTEXT_MONITOR and comment with /* CONTEXT_1 should only be used by monitor */ but it's just not that direct. I'd say it's not "blending monitor codes in chardev", but a pure naming. Instead, actually I see [1] tries to blend monitor code in chardev, which I would prefer not. That's why I used the QemuOpts to pass and decide what context to use. > - chardev cleanup is done after monitor cleanup, this may race iothreads Will it? My plan is that we only let frontend (monitor) depends on the frontend (chardev), then cleaning monitor then chardev seems the correct order for me without race, no? > - breaks chardev context switching (colo) I just had a glance at COLO, I'm not sure but... I think it's not really using the "dynamic context switching". IMHO it only needs a separate context. If so, it should be able to live with this series, we might just need to define one more for COLO at [1]. > - not a very dynamic solution (doesn't help to create oob monitor dynamically) I never created monitors dynamically, but if we want that we might need to expose this "context" property to user, then we can dynamically create out-of-band monitors (as long as when we create the chardev for the out-of-band monitor we pass in correct context property). > > I'd continue to look for options, we might come back to this one eventually :) > > thanks! Thanks for the very positive feedback! :) Yes, let's see whether we have better alternatives. Regards, -- Peter Xu
[Qemu-devel] [PATCH v2] slirp: Implement RFC2132 TFTP server name
This new usernet option can be used to add data for option 66 (tftp
server name) in the BOOTP reply, which is useful in PXE based automatic
OS install such as OpenBSD.
Signed-off-by: Fam Zheng
---
v2: - Adjust parameter order and field placement to be closer to other
tftp options. [Samuel]
- Drop 'optional'. [Eric]
---
net/slirp.c | 5 -
qapi/net.json| 5 -
slirp/bootp.c| 8
slirp/bootp.h| 1 +
slirp/libslirp.h | 4 +++-
slirp/slirp.c| 5 -
slirp/slirp.h| 1 +
7 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/net/slirp.c b/net/slirp.c
index 1e14318b4d..90ef09b40a 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -158,6 +158,7 @@ static int net_slirp_init(NetClientState *peer, const char
*model,
const char *vnameserver, const char *vnameserver6,
const char *smb_export, const char *vsmbserver,
const char **dnssearch, const char *vdomainname,
+ const char *tftp_server_name,
Error **errp)
{
/* default settings according to historic slirp */
@@ -376,6 +377,7 @@ static int net_slirp_init(NetClientState *peer, const char
*model,
s->slirp = slirp_init(restricted, ipv4, net, mask, host,
ipv6, ip6_prefix, vprefix6_len, ip6_host,
+ tftp_server_name,
vhostname, tftp_export, bootfile, dhcp,
dns, ip6_dns, dnssearch, vdomainname, s);
QTAILQ_INSERT_TAIL(_stacks, s, entry);
@@ -966,7 +968,8 @@ int net_init_slirp(const Netdev *netdev, const char *name,
user->ipv6_host, user->hostname, user->tftp,
user->bootfile, user->dhcpstart,
user->dns, user->ipv6_dns, user->smb,
- user->smbserver, dnssearch, user->domainname, errp);
+ user->smbserver, dnssearch, user->domainname,
+ user->tftp_server_name, errp);
while (slirp_configs) {
config = slirp_configs;
diff --git a/qapi/net.json b/qapi/net.json
index c86f351161..8f99fd911d 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -174,6 +174,8 @@
#
# @guestfwd: forward guest TCP connections
#
+# @tftp-server-name: RFC2132 "TFTP server name" string (Since 3.1)
+#
# Since: 1.2
##
{ 'struct': 'NetdevUserOptions',
@@ -198,7 +200,8 @@
'*smb': 'str',
'*smbserver': 'str',
'*hostfwd': ['String'],
-'*guestfwd': ['String'] } }
+'*guestfwd': ['String'],
+'*tftp-server-name': 'str' } }
##
# @NetdevTapOptions:
diff --git a/slirp/bootp.c b/slirp/bootp.c
index 9e7b53ba94..ed2cf3b96e 100644
--- a/slirp/bootp.c
+++ b/slirp/bootp.c
@@ -306,6 +306,14 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t
*bp)
q += val;
}
+if (slirp->tftp_server_name) {
+val = strlen(slirp->tftp_server_name);
+*q++ = RFC2132_TFTP_SERVER_NAME;
+*q++ = val;
+memcpy(q, slirp->tftp_server_name, val);
+q += val;
+}
+
if (slirp->vdnssearch) {
size_t spaceleft = sizeof(rbp->bp_vend) - (q - rbp->bp_vend);
val = slirp->vdnssearch_len;
diff --git a/slirp/bootp.h b/slirp/bootp.h
index 394525733e..4043489835 100644
--- a/slirp/bootp.h
+++ b/slirp/bootp.h
@@ -70,6 +70,7 @@
#define RFC2132_MAX_SIZE 57
#define RFC2132_RENEWAL_TIME58
#define RFC2132_REBIND_TIME 59
+#define RFC2132_TFTP_SERVER_NAME 66
#define DHCPDISCOVER 1
#define DHCPOFFER 2
diff --git a/slirp/libslirp.h b/slirp/libslirp.h
index 740408a96e..0e84ab9b52 100644
--- a/slirp/libslirp.h
+++ b/slirp/libslirp.h
@@ -13,10 +13,12 @@ Slirp *slirp_init(int restricted, bool in_enabled, struct
in_addr vnetwork,
bool in6_enabled,
struct in6_addr vprefix_addr6, uint8_t vprefix_len,
struct in6_addr vhost6, const char *vhostname,
+ const char *tftp_server_name,
const char *tftp_path, const char *bootfile,
struct in_addr vdhcp_start, struct in_addr vnameserver,
struct in6_addr vnameserver6, const char **vdnssearch,
- const char *vdomainname, void *opaque);
+ const char *vdomainname,
+ void *opaque);
void slirp_cleanup(Slirp *slirp);
void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout);
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 5c3bd6163f..4073b0174f 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -283,10 +283,12 @@ Slirp *slirp_init(int restricted, bool in_enabled, struct
in_addr vnetwork,
bool in6_enabled,
struct in6_addr vprefix_addr6, uint8_t vprefix_len,
struct in6_addr vhost6, const char *vhostname,
+
Re: [Qemu-devel] [PATCH] slirp: Implement RFC2132 TFTP server name
On Sat, 08/25 18:53, Samuel Thibault wrote:
> Hello,
>
> Fam Zheng, le ven. 24 août 2018 21:53:12 +0800, a ecrit:
> >const char *vnameserver, const char
> > *vnameserver6,
> >const char *smb_export, const char *vsmbserver,
> >const char **dnssearch, const char *vdomainname,
> > + const char *tftp_server_name,
>
> I'd say rather put it between the vhostname and tftp_export parameters.
>
> > @@ -321,6 +322,9 @@ Slirp *slirp_init(int restricted, bool in_enabled,
> > struct in_addr vnetwork,
> > slirp->vdhcp_startaddr = vdhcp_start;
> > slirp->vnameserver_addr = vnameserver;
> > slirp->vnameserver_addr6 = vnameserver6;
> > +if (tftp_server_name) {
> > +slirp->tftp_server_name = g_strdup(tftp_server_name);
> > +}
>
> I'd say do not bother testing for tftp_server_name != NULL, just always
> use g_strdup, as is done for other values.
Both sound good. I'll send v2.
Fam
Re: [Qemu-devel] [PATCH] slirp: Implement RFC2132 TFTP server name
On Fri, 08/24 11:21, Eric Blake wrote: > On 08/24/2018 08:53 AM, Fam Zheng wrote: > > This new usernet option can be used to add data for option 66 (tftp > > server name) in the BOOTP reply, which is useful in PXE based automatic > > OS install such as OpenBSD. > > > > Signed-off-by: Fam Zheng > > --- > > > +++ b/qapi/net.json > > @@ -174,6 +174,8 @@ > > # > > # @guestfwd: forward guest TCP connections > > # > > +# @tftp-server-name: (optional) RFC2132 "TFTP server name" string (Since > > 3.1) > > The doc generator automatically marks up optional members these days, so you > can drop '(optional)' from this line. OK, will drop in v2. Thanks! Fam
Re: [Qemu-devel] [RFC v4 1/6] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST
Marcel Apfelbaum 于2018年8月25日周六 下午8:08写道：
>
> Hi Zihan,
>
> On 08/19/2018 04:51 AM, Zihan Yang wrote:
> > Hi Marcel,
> >
> > Marcel Apfelbaum 于2018年8月18日周六 上午1:14写道：
> >> Hi Zihan,
> >>
> >> On 08/09/2018 09:33 AM, Zihan Yang wrote:
> >>> The inner host bridge created by pxb-pcie is TYPE_PXB_PCI_HOST by default,
> >>> change it to a new type TYPE_PXB_PCIE_HOST to better utilize ECAM of PCIe
> >>>
> >>> Signed-off-by: Zihan Yang
> >>> ---
> >>>hw/pci-bridge/pci_expander_bridge.c | 127
> >>> ++--
> >>>1 file changed, 122 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/hw/pci-bridge/pci_expander_bridge.c
> >>> b/hw/pci-bridge/pci_expander_bridge.c
> >>> index e62de42..6dd38de 100644
> >>> --- a/hw/pci-bridge/pci_expander_bridge.c
> >>> +++ b/hw/pci-bridge/pci_expander_bridge.c
> >>> @@ -15,10 +15,12 @@
> >>>#include "hw/pci/pci.h"
> >>>#include "hw/pci/pci_bus.h"
> >>>#include "hw/pci/pci_host.h"
> >>> +#include "hw/pci/pcie_host.h"
> >>>#include "hw/pci/pci_bridge.h"
> >>>#include "qemu/range.h"
> >>>#include "qemu/error-report.h"
> >>>#include "sysemu/numa.h"
> >>> +#include "qapi/visitor.h"
> >>>
> >>>#define TYPE_PXB_BUS "pxb-bus"
> >>>#define PXB_BUS(obj) OBJECT_CHECK(PXBBus, (obj), TYPE_PXB_BUS)
> >>> @@ -40,11 +42,20 @@ typedef struct PXBBus {
> >>>#define TYPE_PXB_PCIE_DEVICE "pxb-pcie"
> >>>#define PXB_PCIE_DEV(obj) OBJECT_CHECK(PXBDev, (obj),
> >>> TYPE_PXB_PCIE_DEVICE)
> >>>
> >>> +#define PROP_PXB_PCIE_DEV "pxbdev"
> >>> +
> >>> +#define PROP_PXB_PCIE_DOMAIN_NR "domain_nr"
> >>> +#define PROP_PXB_PCIE_MAX_BUS "max_bus"
> >>> +#define PROP_PXB_BUS_NR "bus_nr"
> >>> +#define PROP_PXB_NUMA_NODE "numa_node"
> >>> +
> >>>typedef struct PXBDev {
> >>>/*< private >*/
> >>>PCIDevice parent_obj;
> >>>/*< public >*/
> >>>
> >>> +uint32_t domain_nr; /* PCI domain number, non-zero means separate
> >>> domain */
> >> The commit message suggests this patch is only about
> >> re-factoring of the pxb host type, but you add here more fields.
> >> Please use two separate patches.
> >>
> >>> +uint8_t max_bus;/* max bus number to use(including this one) */
> >> That's a great idea! Limiting the max_bus will save us a lot
> >> of resource space, we will not need 256 buses on pxbs probably.
> >>
> >> My concern is what happens with the current mode.
> >> Currently bus_nr is used to divide PCI domain 0 buses between pxbs.
> >> So if you have a pxb with bus_nr 100, and another with bus_nr 200,
> >> we divide them like this:
> >> main host bridge 0...99
> >> pxb1 100 -199
> >> pxb2 200-255
> >>
> >> What will be the meaning of max_bus if we don't use the domain_nr
> >> parameter?
> >> Maybe it will mean that some bus numbers are not assigned at all, for
> >> example:
> >> pxb1: bus_nr 100, max_bus 150
> >> pxb2: bus_nr 200, max_bus 210
> >>
> >> It may work.
> > Yes, it should mean so. Actually max_bus does not have to be specified
> > if domain_nr
> > is not used, but if users decide to use domain_nr and want to save
> > space, max_bus
> > could be used.
> >
> >>>uint8_t bus_nr;
> >>>uint16_t numa_node;
> >>>} PXBDev;
> >>> @@ -58,6 +69,16 @@ static PXBDev *convert_to_pxb(PCIDevice *dev)
> >>>static GList *pxb_dev_list;
> >>>
> >>>#define TYPE_PXB_HOST "pxb-host"
> >>> +#define TYPE_PXB_PCIE_HOST "pxb-pcie-host"
> >>> +#define PXB_PCIE_HOST_DEVICE(obj) \
> >>> + OBJECT_CHECK(PXBPCIEHost, (obj), TYPE_PXB_PCIE_HOST)
> >>> +
> >>> +typedef struct PXBPCIEHost {
> >>> +PCIExpressHost parent_obj;
> >>> +
> >>> +/* pointers to PXBDev */
> >>> +PXBDev *pxbdev;
> >>> +} PXBPCIEHost;
> >>>
> >>>static int pxb_bus_num(PCIBus *bus)
> >>>{
> >>> @@ -111,6 +132,35 @@ static const char
> >>> *pxb_host_root_bus_path(PCIHostState *host_bridge,
> >>>return bus->bus_path;
> >>>}
> >>>
> >>> +/* Use a dedicated function for PCIe since pxb-host does
> >>> + * not have a domain_nr field */
> >>> +static const char *pxb_pcie_host_root_bus_path(PCIHostState *host_bridge,
> >>> + PCIBus *rootbus)
> >>> +{
> >>> +if (!pci_bus_is_express(rootbus)) {
> >>> +/* pxb-pcie-host cannot reside on a PCI bus */
> >>> +return NULL;
> >>> +}
> >>> +PXBBus *bus = PXB_PCIE_BUS(rootbus);
> >>> +
> >>> +/* get the pointer to PXBDev */
> >>> +Object *obj = object_property_get_link(OBJECT(host_bridge),
> >>> + PROP_PXB_PCIE_DEV, NULL);
> >> I don't think you need a link here.
> >> I think rootbus->parent_dev returns the pxb device.
> >> (See the implementation of pxb_bus_num() )
> > OK, I'll change it in next version.
> >
> >>> +
> >>> +snprintf(bus->bus_path, 8, "%04lx:%02x",
> >>> + object_property_get_uint(obj, PROP_PXB_PCIE_DOMAIN_NR,
> >>> NULL),
> >>> + pxb_bus_num(rootbus));
> >>> +
Re: [Qemu-devel] [RFC v4 3/6] i386/acpi-build: describe new pci domain in AML
Marcel Apfelbaum 于2018年8月25日周六 下午7:58写道： > > > > On 08/19/2018 05:00 AM, Zihan Yang wrote: > > Marcel Apfelbaum 于2018年8月18日周六 上午1:49写道： > >> > >> > >> On 08/09/2018 09:34 AM, Zihan Yang wrote: > >>> Describe new pci segments of host bridges in AML as new pci devices, > >>> with _SEG and _BBN to let them be in DSDT. > >>> > >>> Besides, bus_nr indicates the bus number of pxb-pcie under pcie.0 bus, > >>> but since we put it into separate domain, it should be zero, > >> Why should it be 0? Isn't possible we start another domain from bus_nr> 0? > > In the last version, I got a misunderstanding about bus_nr, and you pointed > > me > > out that we should start from bus 0 when in new domain. I can support this > > start_bus in later patch, but I wonder when would we want to start a domain > > from bus_nr > 0? I'm not quite familiar with the use case. > > That is a good point. I can't think of a reason to start a new PCI > domain with a bus number >0 , but is it possible. > So if it doesn't complicate the implementation we don't > want this kind of limitation. > On the other hand, if it makes the code too complicated, > we can argue for requiring it. OK, then I will add it first and see its impact. > Thanks, > Marcel > > [...]
Re: [Qemu-devel] [PATCH] spapr: fix leak of rev array
On Fri, Aug 24, 2018 at 04:31:01PM -0400, Emilio G. Cota wrote:
> Introduced in 04d595b300 ("spapr: do not use CPU_FOREACH_REVERSE",
> 2018-08-23)
>
> Fixes: CID1395181
> Reported-by: Peter Maydell
> Signed-off-by: Emilio G. Cota
> ---
> hw/ppc/spapr.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4edb6c7d16..505d4c84e5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -607,6 +607,7 @@ static void spapr_populate_cpus_dt_node(void *fdt,
> sPAPRMachineState *spapr)
> spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> }
>
> +g_free(rev);
> }
>
> static uint32_t spapr_pc_dimm_node(MemoryDeviceInfoList *list, ram_addr_t
> addr)
Applied, thanks.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] spapr_pci: fix potential NULL pointer dereference
On Fri, Aug 24, 2018 at 05:30:04PM +0200, Greg Kurz wrote: > Commit 2c88b098e76fd added a call to SPAPR_MACHINE_GET_CLASS(spapr) in > spapr_phb_realize() before we check spapr isn't NULL. This causes QEMU > to crash when starting a non-pseries machine with a sPAPR PHB. > > This could be fixed by setting the smc variable after the null check, > but it seems more explicit to use a ternary operator to skip the call > to SPAPR_MACHINE_GET_CLASS() if spapr is NULL, since spapr_phb_realize() > will return immediately in this case. > > This was reported by Coverity (CID 1395170 and 1395183). > > Fixes: 2c88b098e76fde0c7fcc0476dd3f80ce58409505 > Signed-off-by: Greg Kurz Applied, thanks. > --- > hw/ppc/spapr_pci.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 5cd676e4430d..6bcb4f419b6b 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1559,7 +1559,7 @@ static void spapr_phb_realize(DeviceState *dev, Error > **errp) > sPAPRMachineState *spapr = > (sPAPRMachineState *) object_dynamic_cast(qdev_get_machine(), >TYPE_SPAPR_MACHINE); > -sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); > +sPAPRMachineClass *smc = spapr ? SPAPR_MACHINE_GET_CLASS(spapr) : NULL; > SysBusDevice *s = SYS_BUS_DEVICE(dev); > sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s); > PCIHostState *phb = PCI_HOST_BRIDGE(s); > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH 1/5] nvme: PCI/e configuration from specification
Hi Daniel, Thanks for taking a look. Comments are inline. Gersner. On Sun, Jul 15, 2018 at 9:21 AM Daniel Verkamp wrote: > On Fri, Jun 22, 2018 at 4:22 AM, Shimi Gersner wrote: > > PCI/e configuration currently does not meets specifications. > > > > Patch includes various configuration changes to support specifications > > - BAR2 to return zero when read and CMD.IOSE is not set. > > - Expose NVME configuration through IO space (Optional). > > - PCI Power Management v1.2. > > - PCIe Function Level Reset. > > - Disable QEMUs default use of PCIe Link Status (DLLLA). > > - PCIe missing AOC compliance flag. > > - Mask PCIe End-to-End TLP as RO (Unspecified by specification). > [...] > > n->num_namespaces = 1; > > n->num_queues = 64; > > -n->reg_size = pow2ceil(0x1004 + 2 * (n->num_queues + 1) * 4); > > +n->reg_size = pow2ceil(0x2000 + 2 * (n->num_queues + 1) * 4); > > The existing math looks OK to me (maybe already 4 bytes larger than > necessary?). The controller registers should be 0x1000 bytes for the > fixed register set, then 2 * 4 bytes for each doorbell pair (+ 1 is > for the admin queue, and CAP.DSTRD is set to 0, so there is no extra > padding between queues). The result is also rounded up to a power of > two, so the result will typically be 8 KB. What is the rationale for > this change? > You are correct, this change shouldn't be here. It was made during tests against the nvme compliance which failed here. The specification states that bits 0 to 13 are RO, which implicitly suggests that the minimal size of this BAR should be at least 16K as this is a standard way to determine the BAR size on PCI (AFAIK). On the contrary it doesn't fit very well with the memory mapped laid out on 3.1 Register Definition of the 1.3b nvme spec. Any idea? Additionally it does look like it has an extra 4 bytes. > > > n->ns_size = bs_size / (uint64_t)n->num_namespaces; > > > > n->namespaces = g_new0(NvmeNamespace, n->num_namespaces); > > @@ -1245,6 +1295,10 @@ static void nvme_realize(PCIDevice *pci_dev, > Error **errp) > > pci_register_bar(>parent_obj, 0, > > PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64, > > >iomem); > > + > > +// Expose the NVME memory through Address Space IO (Optional by > spec) > > +pci_register_bar(>parent_obj, 2, PCI_BASE_ADDRESS_SPACE_IO, > >iomem); > > This looks like it will register the whole 4KB+ NVMe register set > (n->iomem) as an I/O space BAR, but this doesn't match the spec (see > NVMe 1.3c section 3.2, "Index/Data Pair registers"). The I/O BAR (if > implemented) should just contain two 4-byte registers, Index and Data, > that can be used to indirectly access the NVMe register set. (Just > for curiosity, do you know of any software that uses this feature? It > could be useful for testing the implementation.) > Very nice catch. We indeed totally miss-interpreted the specification here. We use the compliance suit for sanity, but it doesn't actually use this feature, just verifies the configuration of the registers. We will go with rolling back this feature as this is optional. Question - I'm having hard time to determine from the specification - As this is optional, how device drivers determine if it is available? Does it simply the CMD.IOSE flag from the PCI? And if so, what is the standard way in QEMU to disable the flag completely? > > Other minor nitpicks: > - Comment style is inconsistent with the rest of the file (// vs /* */) > - PCIe and NVMe are incorrectly capitalized a few times in comments > Thanks. > > Thanks, > -- Daniel >
Re: [Qemu-devel] [PATCH] Fix ARM v7m gen_intermediate_code()
In the end it was just incorrect alignment for my vector table
[Qemu-devel] [PATCH 3/9] block: Remove child references from bs->{options, explicit_options}
Block drivers allow opening their children using a reference to an
existing BlockDriverState. These references remain stored in the
'options' and 'explicit_options' QDicts, but we don't need to keep
them once everything is open.
What is more important, these values can become wrong if the children
change:
$ qemu-img create -f qcow2 hd0.qcow2 10M
$ qemu-img create -f qcow2 hd1.qcow2 10M
$ qemu-img create -f qcow2 hd2.qcow2 10M
$ $QEMU -drive if=none,file=hd0.qcow2,node-name=hd0 \
-drive if=none,file=hd1.qcow2,node-name=hd1,backing=hd0 \
-drive file=hd2.qcow2,node-name=hd2,backing=hd1
After this hd2 has hd1 as its backing file. Now let's remove it using
block_stream:
(qemu) block_stream hd2 0 hd0.qcow2
Now hd0 is the backing file of hd2, but hd2's options QDicts still
contain backing=hd1.
Signed-off-by: Alberto Garcia
---
block.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 0dbb1fcc7b..2f2484965d 100644
--- a/block.c
+++ b/block.c
@@ -2763,12 +2763,15 @@ static BlockDriverState *bdrv_open_inherit(const char
*filename,
}
}
-/* Remove all children options from bs->options and bs->explicit_options */
+/* Remove all children options and references
+ * from bs->options and bs->explicit_options */
QLIST_FOREACH(child, >children, next) {
char *child_key_dot;
child_key_dot = g_strdup_printf("%s.", child->name);
qdict_extract_subqdict(bs->explicit_options, NULL, child_key_dot);
qdict_extract_subqdict(bs->options, NULL, child_key_dot);
+qdict_del(bs->explicit_options, child->name);
+qdict_del(bs->options, child->name);
g_free(child_key_dot);
}
@@ -3289,6 +3292,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
{
BlockDriver *drv;
BlockDriverState *bs;
+BdrvChild *child;
bool old_can_write, new_can_write;
assert(reopen_state != NULL);
@@ -3313,6 +3317,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
bs->open_flags = reopen_state->flags;
bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
+/* Remove child references from bs->options and bs->explicit_options */
+QLIST_FOREACH(child, >children, next) {
+qdict_del(bs->explicit_options, child->name);
+qdict_del(bs->options, child->name);
+}
+
bdrv_refresh_limits(bs, NULL);
bdrv_set_perm(reopen_state->bs, reopen_state->perm,
--
2.11.0
[Qemu-devel] [PATCH 8/9] block: Allow changing 'detect-zeroes' on reopen
'detect-zeroes' is one of the basic BlockdevOptions available for all
drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
the user cannot change it and doesn't get an error explaining that it
can't be changed.
This should update the detect-zeroes setting, but does nothing:
(qemu) qemu-io virtio0 "reopen -o detect-zeroes=on"
This should return an error but also does nothing:
(qemu) qemu-io virtio0 "reopen -o discard=off,detect-zeroes=unmap"
Since there's no reason why we shouldn't allow changing it and the
implementation is simple let's just do it.
Signed-off-by: Alberto Garcia
---
block.c | 63 +++
include/block/block.h | 1 +
2 files changed, 40 insertions(+), 24 deletions(-)
diff --git a/block.c b/block.c
index 21f1eb9cd1..9aed5c19f4 100644
--- a/block.c
+++ b/block.c
@@ -764,6 +764,30 @@ static void bdrv_join_options(BlockDriverState *bs, QDict
*options,
}
}
+static BlockdevDetectZeroesOptions bdrv_parse_detect_zeroes(QemuOpts *opts,
+int open_flags,
+Error **errp)
+{
+Error *local_err = NULL;
+const char *value = qemu_opt_get(opts, "detect-zeroes");
+BlockdevDetectZeroesOptions detect_zeroes =
+qapi_enum_parse(_lookup, value,
+BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return detect_zeroes;
+}
+
+if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
+!(open_flags & BDRV_O_UNMAP))
+{
+error_setg(errp, "setting detect-zeroes to unmap is not allowed "
+ "without setting discard operation to unmap");
+}
+
+return detect_zeroes;
+}
+
/**
* Set open flags for a given discard mode
*
@@ -1328,7 +1352,6 @@ static int bdrv_open_common(BlockDriverState *bs,
BlockBackend *file,
const char *driver_name = NULL;
const char *node_name = NULL;
const char *discard;
-const char *detect_zeroes;
QemuOpts *opts;
BlockDriver *drv;
Error *local_err = NULL;
@@ -1417,29 +1440,12 @@ static int bdrv_open_common(BlockDriverState *bs,
BlockBackend *file,
}
}
-detect_zeroes = qemu_opt_get(opts, "detect-zeroes");
-if (detect_zeroes) {
-BlockdevDetectZeroesOptions value =
-qapi_enum_parse(_lookup,
-detect_zeroes,
-BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
-_err);
-if (local_err) {
-error_propagate(errp, local_err);
-ret = -EINVAL;
-goto fail_opts;
-}
-
-if (value == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
-!(bs->open_flags & BDRV_O_UNMAP))
-{
-error_setg(errp, "setting detect-zeroes to unmap is not allowed "
- "without setting discard operation to unmap");
-ret = -EINVAL;
-goto fail_opts;
-}
-
-bs->detect_zeroes = value;
+bs->detect_zeroes =
+bdrv_parse_detect_zeroes(opts, bs->open_flags, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail_opts;
}
if (filename != NULL) {
@@ -3187,6 +3193,14 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
}
}
+reopen_state->detect_zeroes =
+bdrv_parse_detect_zeroes(opts, reopen_state->flags, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto error;
+}
+
/* node-name and driver must be unchanged. Put them back into the QDict, so
* that they are checked at the end of this function. */
value = qemu_opt_get(opts, "node-name");
@@ -3344,6 +3358,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
bs->options= reopen_state->options;
bs->open_flags = reopen_state->flags;
bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
+bs->detect_zeroes = reopen_state->detect_zeroes;
/* Remove child references from bs->options and bs->explicit_options */
QLIST_FOREACH(child, >children, next) {
diff --git a/include/block/block.h b/include/block/block.h
index 4e0871aaf9..f71fa5a1c4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -184,6 +184,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue,
BlockReopenQueueEntry) BlockReopenQueue;
typedef struct BDRVReopenState {
BlockDriverState *bs;
int flags;
+BlockdevDetectZeroesOptions detect_zeroes;
uint64_t perm, shared_perm;
QDict *options;
QDict *explicit_options;
--
2.11.0
[Qemu-devel] [PATCH 7/9] block: Allow changing 'discard' on reopen
'discard' is one of the basic BlockdevOptions available for all
drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
the user cannot change it and doesn't get an error explaining that it
can't be changed.
This should update the discard setting, but does nothing:
(qemu) qemu-io virtio0 "reopen -o discard=on"
Since there's no reason why we shouldn't allow changing it and the
implementation is simple let's just do it.
Signed-off-by: Alberto Garcia
---
block.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/block.c b/block.c
index 804d557608..21f1eb9cd1 100644
--- a/block.c
+++ b/block.c
@@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
update_flags_from_options(_state->flags, opts);
+value = qemu_opt_get(opts, "discard");
+if (value != NULL) {
+if (bdrv_parse_discard_flags(value, _state->flags) != 0) {
+error_setg(errp, "Invalid discard option");
+ret = -EINVAL;
+goto error;
+}
+}
+
/* node-name and driver must be unchanged. Put them back into the QDict, so
* that they are checked at the end of this function. */
value = qemu_opt_get(opts, "node-name");
--
2.11.0
[Qemu-devel] [PATCH 0/9] Misc reopen-related patches
Hi,
as part of my blockdev-reopen work here's a new set of patches. This
doesn't implement yet the core functionality of the new reopen
command, but it does fix a few things that help us pave the way.
I believe that the next series after this one will be the last.
The main change is the removal of child references from the options
and explicit_options QDicts. This was already discussed in the
previous series[1], and here's the implementation.
Regards,
Berto
[1] https://lists.gnu.org/archive/html/qemu-block/2018-08/msg00474.html
Alberto Garcia (9):
qemu-io: Fix writethrough check in reopen
file-posix: x-check-cache-dropped should default to false on reopen
block: Remove child references from bs->{options,explicit_options}
block: Don't look for child references in append_open_options()
block: Allow child references on reopen
file-posix: Forbid trying to change unsupported options during reopen
block: Allow changing 'discard' on reopen
block: Allow changing 'detect-zeroes' on reopen
block: Allow changing 'force-share' on reopen
block.c | 146 +++---
block/file-posix.c| 9 +++-
include/block/block.h | 2 +
qemu-io-cmds.c| 2 +-
4 files changed, 113 insertions(+), 46 deletions(-)
--
2.11.0
[Qemu-devel] [PATCH 5/9] block: Allow child references on reopen
In the previous patches we removed all child references from
bs->{options,explicit_options} because keeping them is useless and
wrong.
Because of this, any attempt to reopen a BlockDriverState using a
child reference as one of its options would result in a failure,
because bdrv_reopen_prepare() would detect that there's a new option
(the child reference) that wasn't present in bs->options.
But passing child references on reopen can be useful. It's a way to
specify a BDS's child without having to pass recursively all of the
child's options, and if the reference points to a different BDS then
this can allow us to replace the child.
However, replacing the child is something that needs to be implemented
case by case and only when it makes sense. For now, this patch allows
passing a child reference as long as it points to the current child of
the BlockDriverState.
It's also important to remember that, as a consequence of the
previous patches, this child reference will be removed from
bs->{options,explicit_options} after the reopening has been completed.
Signed-off-by: Alberto Garcia
---
block.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/block.c b/block.c
index 92afb3dcce..804d557608 100644
--- a/block.c
+++ b/block.c
@@ -3241,6 +3241,25 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
QObject *new = entry->value;
QObject *old = qdict_get(reopen_state->bs->options, entry->key);
+/* We allow child references 'child_name'='value'
+ * if 'child_name' is an existing child of the BDS
+ * and 'value' is the child's node name (a string). */
+if (qobject_type(new) == QTYPE_QSTRING) {
+BdrvChild *child;
+QLIST_FOREACH(child, _state->bs->children, next) {
+if (!strcmp(child->name, entry->key)) {
+break;
+}
+}
+
+if (child) {
+const char *str = qobject_get_try_str(new);
+if (!strcmp(child->bs->node_name, str)) {
+continue; /* Found child with this name, skip option */
+}
+}
+}
+
/*
* TODO: When using -drive to specify blockdev options, all values
* will be strings; however, when using -blockdev, blockdev-add or
--
2.11.0
[Qemu-devel] [PATCH 4/9] block: Don't look for child references in append_open_options()
In the previous patch we removed child references from bs->options, so
there's no need to look for them here anymore.
Signed-off-by: Alberto Garcia
---
block.c | 13 +
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/block.c b/block.c
index 2f2484965d..92afb3dcce 100644
--- a/block.c
+++ b/block.c
@@ -5153,23 +5153,12 @@ static bool append_open_options(QDict *d,
BlockDriverState *bs)
{
const QDictEntry *entry;
QemuOptDesc *desc;
-BdrvChild *child;
bool found_any = false;
for (entry = qdict_first(bs->options); entry;
entry = qdict_next(bs->options, entry))
{
-/* Exclude node-name references to children */
-QLIST_FOREACH(child, >children, next) {
-if (!strcmp(entry->key, child->name)) {
-break;
-}
-}
-if (child) {
-continue;
-}
-
-/* And exclude all non-driver-specific options */
+/* Exclude all non-driver-specific options */
for (desc = bdrv_runtime_opts.desc; desc->name; desc++) {
if (!strcmp(qdict_entry_key(entry), desc->name)) {
break;
--
2.11.0
[Qemu-devel] [PATCH 6/9] file-posix: Forbid trying to change unsupported options during reopen
The file-posix code is used for the "file", "host_device" and
"host_cdrom" drivers, and it allows reopening images. However the only
option that is actually processed is "x-check-cache-dropped", and
changes in all other options (e.g. "filename") are silently ignored:
(qemu) qemu-io virtio0 "reopen -o file.filename=no-such-file"
While we could allow changing some of the other options, let's keep
things as they are for now but return an error if the user tries to
change any of them.
Signed-off-by: Alberto Garcia
---
block/file-posix.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index ddac0cc3e6..d4ec2cb3dd 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -850,8 +850,13 @@ static int raw_reopen_prepare(BDRVReopenState *state,
goto out;
}
-rs->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped",
-false);
+rs->check_cache_dropped =
+qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
+
+/* This driver's reopen function doesn't currently allow changing
+ * other options, so let's put them back in the original QDict and
+ * bdrv_reopen_prepare() will detect changes and complain. */
+qemu_opts_to_qdict(opts, state->options);
if (s->type == FTYPE_CD) {
rs->open_flags |= O_NONBLOCK;
--
2.11.0
[Qemu-devel] [PATCH 9/9] block: Allow changing 'force-share' on reopen
'force-share' is one of the basic BlockdevOptions available for all
drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
the user cannot change it and doesn't get an error explaining that it
can't be changed.
One example of how this option is being ignored is that on a
read-write image this should produce an error:
(qemu) qemu-io virtio0 "reopen -o force-share=on"
force-share=on can only be used with read-only images
Since there's no reason why we shouldn't allow changing it and the
implementation is simple let's just do it.
Signed-off-by: Alberto Garcia
---
block.c | 30 --
include/block/block.h | 1 +
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index 9aed5c19f4..b6e495b716 100644
--- a/block.c
+++ b/block.c
@@ -788,6 +788,18 @@ static BlockdevDetectZeroesOptions
bdrv_parse_detect_zeroes(QemuOpts *opts,
return detect_zeroes;
}
+static bool bdrv_parse_force_share(QemuOpts *opts, int flags, Error **errp)
+{
+bool value = qemu_opt_get_bool(opts, BDRV_OPT_FORCE_SHARE, false);
+
+if (value && (flags & BDRV_O_RDWR)) {
+error_setg(errp, BDRV_OPT_FORCE_SHARE
+ "=on can only be used with read-only images");
+}
+
+return value;
+}
+
/**
* Set open flags for a given discard mode
*
@@ -1373,12 +1385,9 @@ static int bdrv_open_common(BlockDriverState *bs,
BlockBackend *file,
drv = bdrv_find_format(driver_name);
assert(drv != NULL);
-bs->force_share = qemu_opt_get_bool(opts, BDRV_OPT_FORCE_SHARE, false);
-
-if (bs->force_share && (bs->open_flags & BDRV_O_RDWR)) {
-error_setg(errp,
- BDRV_OPT_FORCE_SHARE
- "=on can only be used with read-only images");
+bs->force_share = bdrv_parse_force_share(opts, bs->open_flags, _err);
+if (local_err) {
+error_propagate(errp, local_err);
ret = -EINVAL;
goto fail_opts;
}
@@ -3201,6 +3210,14 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
goto error;
}
+reopen_state->force_share =
+bdrv_parse_force_share(opts, reopen_state->flags, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto error;
+}
+
/* node-name and driver must be unchanged. Put them back into the QDict, so
* that they are checked at the end of this function. */
value = qemu_opt_get(opts, "node-name");
@@ -3359,6 +3376,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
bs->open_flags = reopen_state->flags;
bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
bs->detect_zeroes = reopen_state->detect_zeroes;
+bs->force_share= reopen_state->force_share;
/* Remove child references from bs->options and bs->explicit_options */
QLIST_FOREACH(child, >children, next) {
diff --git a/include/block/block.h b/include/block/block.h
index f71fa5a1c4..a49a027c54 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -185,6 +185,7 @@ typedef struct BDRVReopenState {
BlockDriverState *bs;
int flags;
BlockdevDetectZeroesOptions detect_zeroes;
+bool force_share;
uint64_t perm, shared_perm;
QDict *options;
QDict *explicit_options;
--
2.11.0
[Qemu-devel] [PATCH 1/9] qemu-io: Fix writethrough check in reopen
"qemu-io reopen" doesn't allow changing the writethrough setting of
the cache, but the check is wrong, causing an error even on a simple
reopen with the default parameters:
$ qemu-img create -f qcow2 hd.qcow2 1M
$ qemu-system-x86_64 -monitor stdio -drive if=virtio,file=hd.qcow2
(qemu) qemu-io virtio0 reopen
Cannot change cache.writeback: Device attached
Signed-off-by: Alberto Garcia
---
qemu-io-cmds.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 5bf5f28178..db0b3ee5ef 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2025,7 +2025,7 @@ static int reopen_f(BlockBackend *blk, int argc, char
**argv)
return -EINVAL;
}
-if (writethrough != blk_enable_write_cache(blk) &&
+if (!writethrough != blk_enable_write_cache(blk) &&
blk_get_attached_dev(blk))
{
error_report("Cannot change cache.writeback: Device attached");
--
2.11.0
[Qemu-devel] [PATCH 2/9] file-posix: x-check-cache-dropped should default to false on reopen
The default value of x-check-cache-dropped is false. There's no reason
to use the previous value as a default in raw_reopen_prepare() because
bdrv_reopen_queue_child() already takes care of putting the old
options in the BDRVReopenState.options QDict.
If x-check-cache-dropped was previously set but is now missing from
the reopen QDict then it should be reset to false.
Signed-off-by: Alberto Garcia
---
block/file-posix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index fe83cbf0eb..ddac0cc3e6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -851,7 +851,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
}
rs->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped",
-s->check_cache_dropped);
+false);
if (s->type == FTYPE_CD) {
rs->open_flags |= O_NONBLOCK;
--
2.11.0
[Qemu-devel] [PATCH 1/1] vga: honor blink attribute in text modes
Signed-off-by: Michael Slade
---
hw/display/vga.c | 50 +---
hw/display/vga_int.h | 2 ++
2 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 72181330b8..03d9181d58 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -36,8 +36,10 @@
//#define DEBUG_VGA_MEM
//#define DEBUG_VGA_REG
-/* 16 state changes per vertical frame @60 Hz */
+/* state change per 16 frames @60 Hz */
#define VGA_TEXT_CURSOR_PERIOD_MS (1000 * 2 * 16 / 60)
+/* state change per 32 frames @60 Hz */
+#define VGA_TEXT_BLINK_PERIOD_MS(1000 * 2 * 32 / 60)
/*
* Video Graphics Array (VGA)
@@ -1165,6 +1167,7 @@ static void vga_get_text_resolution(VGACommonState *s,
int *pwidth, int *pheight
*pcheight = cheight;
}
+
/*
* Text mode update
* Missing:
@@ -1185,6 +1188,8 @@ static void vga_draw_text(VGACommonState *s, int
full_update)
uint32_t *palette;
uint32_t *ch_attr_ptr;
int64_t now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
+bool cursor_blink = 0;
+bool attr_blink = 0;
/* compute font data address (in plane 2) */
v = sr(s, VGA_SEQ_CHARACTER_MAP);
@@ -1266,6 +1271,13 @@ static void vga_draw_text(VGACommonState *s, int
full_update)
if (now >= s->cursor_blink_time) {
s->cursor_blink_time = now + VGA_TEXT_CURSOR_PERIOD_MS / 2;
s->cursor_visible_phase = !s->cursor_visible_phase;
+cursor_blink = 1;
+}
+bool blink_active = s->ar[VGA_ATC_MODE] & 0x08;
+if (now >= s->attr_blink_time) {
+s->attr_blink_time = now + VGA_TEXT_BLINK_PERIOD_MS / 2;
+s->attr_blink_visible_phase = !s->attr_blink_visible_phase;
+attr_blink = blink_active;
}
dest = surface_data(surface);
@@ -1283,23 +1295,36 @@ static void vga_draw_text(VGACommonState *s, int
full_update)
break;
}
ch_attr = *(uint16_t *)src;
-if (full_update || ch_attr != *ch_attr_ptr || src == cursor_ptr) {
+#ifdef HOST_WORDS_BIGENDIAN
+ch = ch_attr >> 8;
+cattr = ch_attr & 0xff;
+#else
+ch = ch_attr & 0xff;
+cattr = ch_attr >> 8;
+#endif
+
+if (
+full_update ||
+ch_attr != *ch_attr_ptr ||
+(src == cursor_ptr && cursor_blink) ||
+(attr_blink && (cattr & 0x80))
+) {
if (cx < cx_min)
cx_min = cx;
if (cx > cx_max)
cx_max = cx;
*ch_attr_ptr = ch_attr;
-#ifdef HOST_WORDS_BIGENDIAN
-ch = ch_attr >> 8;
-cattr = ch_attr & 0xff;
-#else
-ch = ch_attr & 0xff;
-cattr = ch_attr >> 8;
-#endif
font_ptr = font_base[(cattr >> 3) & 1];
font_ptr += 32 * 4 * ch;
-bgcol = palette[cattr >> 4];
-fgcol = palette[cattr & 0x0f];
+if (blink_active) {
+bgcol = palette[(cattr >> 4) & 7];
+fgcol = (!(cattr & 0x80) || s->attr_blink_visible_phase) ?
+palette[cattr & 0x0f] :
+bgcol;
+} else {
+bgcol = palette[cattr >> 4];
+fgcol = palette[cattr & 0x0f];
+}
if (cw == 16) {
vga_draw_glyph16(d1, linesize,
font_ptr, cheight, fgcol, bgcol);
@@ -1756,7 +1781,8 @@ static void vga_update_display(void *opaque)
}
if (graphic_mode != s->graphic_mode) {
s->graphic_mode = graphic_mode;
-s->cursor_blink_time = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
+s->cursor_blink_time = s->attr_blink_time =
+qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
full_update = 1;
}
switch(graphic_mode) {
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index fe23b81442..775b3e7aa9 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -157,7 +157,9 @@ typedef struct VGACommonState {
bool force_shadow;
uint8_t cursor_start, cursor_end;
bool cursor_visible_phase;
+bool attr_blink_visible_phase;
int64_t cursor_blink_time;
+int64_t attr_blink_time;
uint32_t cursor_offset;
const GraphicHwOps *hw_ops;
bool full_update_text;
--
2.17.1
Re: [Qemu-devel] [PATCH v5 2/3] arm: Add Nordic Semiconductor nRF51 SoC
On 26 August 2018 at 01:48, Joel Stanley wrote: > I agree that it would be neater to do this. I didn't as the flash is > part of the NRF51822 SoC, opposed to some external flash that is on > the microbit board and connected to the SoC. This is mentioned in the > comment at the start of the file: > > /* > * The size and base is for the NRF51822 part. If other parts > * are supported in the future, add a sub-class of NRF51SoC for > * the specific variants > */ Oh, right. I'd assumed it wasn't fixed because it was specified as a property on the object. > What would you prefer we do here? I don't see anything that seems like the really obvious clean thing, so I suggest just doing something that seems reasonable. I think Steffen's patchset also had a change in this area, which might affect the decision. thanks -- PMM
