Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
On Mon, Nov 08, 2021 at 13:05:10 +0100, Kevin Wolf wrote: > Am 06.11.2021 um 07:34 hat Markus Armbruster geschrieben: > > Kevin Wolf writes: > > > Am 05.11.2021 um 11:08 hat Markus Armbruster geschrieben: > > >> Kevin Wolf writes: > > >> > Am 04.11.2021 um 13:13 hat Markus Armbruster geschrieben: [...] > > > Peter Maydell also wants to do this > > >only after a big user (read: libvirt) has implemented and is > > >using JSON, basically as a proof that the alternative is working. > > > > > >So this can certainly be done for -object. I believe libvirt also > > >uses JSON for -device now, so this should be fine now, too. > > > > The non-sugar options supporting JSON are -audiodev, -blockdev, -compat, > > -display (partially), -machine (I think), -object. > > > > -netdev is QAPIfied, but still uses QemuOpts. Too late for 6.2, I'm > > afraid. > > Ok. Not sure about the libvirt status for some of these, but -object and Interresting. We don't do JSON for -audiodev, -compat, -display or -machine. -audiodev and -compat are recent enough so I suppose those accepted JSON always. Converting them will be trivial. For -display and -machine we'll need a witness to switch to the new syntax but I think I can convert them in libvirt if it helps qemu to have a more consistent message. > -device are the ones that I know are going to be in the way in the > future, so degrading their non-JSON form would already be helpful.
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
Am 06.11.2021 um 07:34 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 05.11.2021 um 11:08 hat Markus Armbruster geschrieben:
> >> Kevin Wolf writes:
> >>
> >> > Am 04.11.2021 um 13:13 hat Markus Armbruster geschrieben:
> >> >> The old syntax almost always has its quirks. Ideally, we'd somehow get
> >> >> from quirky old to boring new in an orderly manner. Sadly, we still
> >> >> don't have good solutions for that. To make progress, we commonly
> >> >> combine JSON new with quirky old.
> >> >>
> >> >> qemu-system-FOO -object works that way. object_option_parse() parses
> >> >> either JSON or QemuOpts. It wraps the former in a QObject visitor, and
> >> >> the latter in an opts visitor.
> >> >>
> >> >> QemuOpts is flat by design[*], so the opts visitor parses flat QemuOpts
> >> >> from a (possibly non-flat) QAPI type. How exactly it flattens, and how
> >> >> it handles clashes I don't remember.
> >> >>
> >> >> Sadly, this means that we get quirky old even for new object types.
> >> >
> >> > For -object in the system emulator (the tools all use the keyval
> >> > visitor, so there it would work as expected), the only reason that we
> >> > need to keep the quirky old code path around is the list handling in
> >> > memory-backend.host-nodes.
> >> >
> >> > The main difficulty there is that the old QemuOpts based code path
> >> > allows specifying the option twice and both of them would effectively be
> >> > combined. Do we have any idea how to replicate this in a keyval parser
> >> > based world?
> >>
> >> I can see just two clean solutions, but both involve upending a lot of
> >> code.
> >>
> >> We can fuse keyval parser and visitor to get a schema-directed parser.
> >>
> >> We can change the abstract keyval syntax to permit repeated keys. This
> >> means replacing QDict in in the abstract syntax tree, with fallout in
> >> the visitor.
> >>
> >> Even if we find a practical solution, I don't like the combination of
> >> "you may give the same parameter multiple times, and the last one wins"
> >> and "for a list-valued parameter, the values of repeated parameters are
> >> collected into a list". Each makes sense on its own. The combination
> >> not so much. Inheriting "last one wins" from QemuOpts may have been a
> >> mistake.
> >>
> >> The keyval way of doing lists (inherited from the block layer's usage of
> >> dotted keys? I don't remember) requires the user to count, which isn't
> >> exactly nice, either.
> >
> > Yes. If we didn't have to maintain compatibility (or actually as soon as
> > we degrade non-JSON option lists to HMP-level support), I would
> > introduce [] and {} syntax for lists and dicts, even if that means that
> > use of these characters in strings doesn't work any more or only in a
> > limited way. I think this would be the best compromise for usability.
> >
> > Anyway, this doesn't help us with the compatibility problem we're
> > discussing here.
> >
> >> > If not, do we want to use the remaining time until 6.2 to deprecate
> >> > this? The nasty part is that the only syntax that works both now and in
> >> > the future is JSON. We can't easily accept the new keyval syntax while
> >> > still using the QemuOpts based code.
> >>
> >> What exactly do you propose to deprecate?
> >
> > We can deprecate on two different levels. I think it's useful to do
> > both:
> >
> > 1. Broad deprecation: Stable non-JSON interfaces are degraded to
> >a HMP-like compatibility promise.
>
> Calling it "deprecation" might be confusing. HMP isn't deprecated, it's
> merely not a stable interface. That's kind of like "deprecated when you
> need stable", but saying "not a stable interface" is clearer.
>
> When I write "deprecate" below, I mean something like "go use something
> else (no conditions)". When I mean "use something else when you need
> stable", I write "degrade" (short for "degrade to an HMP-like
> compatibility promise").
>
> > Obviously, this can only be done
> >for options that support JSON.
>
> We can also degrade or even deprecate sugar options in favor of the real
> ones. Case by case, I guess.
Right. And essentially, the non-JSON form would be considered a sugar
option, even if the option string is the same.
> > Peter Maydell also wants to do this
> >only after a big user (read: libvirt) has implemented and is
> >using JSON, basically as a proof that the alternative is working.
> >
> >So this can certainly be done for -object. I believe libvirt also
> >uses JSON for -device now, so this should be fine now, too.
>
> The non-sugar options supporting JSON are -audiodev, -blockdev, -compat,
> -display (partially), -machine (I think), -object.
>
> -netdev is QAPIfied, but still uses QemuOpts. Too late for 6.2, I'm
> afraid.
Ok. Not sure about the libvirt status for some of these, but -object and
-device are the ones that I know are going to be in the way in t
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
Kevin Wolf writes:
> Am 05.11.2021 um 11:08 hat Markus Armbruster geschrieben:
>> Kevin Wolf writes:
>>
>> > Am 04.11.2021 um 13:13 hat Markus Armbruster geschrieben:
>> >> The old syntax almost always has its quirks. Ideally, we'd somehow get
>> >> from quirky old to boring new in an orderly manner. Sadly, we still
>> >> don't have good solutions for that. To make progress, we commonly
>> >> combine JSON new with quirky old.
>> >>
>> >> qemu-system-FOO -object works that way. object_option_parse() parses
>> >> either JSON or QemuOpts. It wraps the former in a QObject visitor, and
>> >> the latter in an opts visitor.
>> >>
>> >> QemuOpts is flat by design[*], so the opts visitor parses flat QemuOpts
>> >> from a (possibly non-flat) QAPI type. How exactly it flattens, and how
>> >> it handles clashes I don't remember.
>> >>
>> >> Sadly, this means that we get quirky old even for new object types.
>> >
>> > For -object in the system emulator (the tools all use the keyval
>> > visitor, so there it would work as expected), the only reason that we
>> > need to keep the quirky old code path around is the list handling in
>> > memory-backend.host-nodes.
>> >
>> > The main difficulty there is that the old QemuOpts based code path
>> > allows specifying the option twice and both of them would effectively be
>> > combined. Do we have any idea how to replicate this in a keyval parser
>> > based world?
>>
>> I can see just two clean solutions, but both involve upending a lot of
>> code.
>>
>> We can fuse keyval parser and visitor to get a schema-directed parser.
>>
>> We can change the abstract keyval syntax to permit repeated keys. This
>> means replacing QDict in in the abstract syntax tree, with fallout in
>> the visitor.
>>
>> Even if we find a practical solution, I don't like the combination of
>> "you may give the same parameter multiple times, and the last one wins"
>> and "for a list-valued parameter, the values of repeated parameters are
>> collected into a list". Each makes sense on its own. The combination
>> not so much. Inheriting "last one wins" from QemuOpts may have been a
>> mistake.
>>
>> The keyval way of doing lists (inherited from the block layer's usage of
>> dotted keys? I don't remember) requires the user to count, which isn't
>> exactly nice, either.
>
> Yes. If we didn't have to maintain compatibility (or actually as soon as
> we degrade non-JSON option lists to HMP-level support), I would
> introduce [] and {} syntax for lists and dicts, even if that means that
> use of these characters in strings doesn't work any more or only in a
> limited way. I think this would be the best compromise for usability.
>
> Anyway, this doesn't help us with the compatibility problem we're
> discussing here.
>
>> > If not, do we want to use the remaining time until 6.2 to deprecate
>> > this? The nasty part is that the only syntax that works both now and in
>> > the future is JSON. We can't easily accept the new keyval syntax while
>> > still using the QemuOpts based code.
>>
>> What exactly do you propose to deprecate?
>
> We can deprecate on two different levels. I think it's useful to do
> both:
>
> 1. Broad deprecation: Stable non-JSON interfaces are degraded to
>a HMP-like compatibility promise.
Calling it "deprecation" might be confusing. HMP isn't deprecated, it's
merely not a stable interface. That's kind of like "deprecated when you
need stable", but saying "not a stable interface" is clearer.
When I write "deprecate" below, I mean something like "go use something
else (no conditions)". When I mean "use something else when you need
stable", I write "degrade" (short for "degrade to an HMP-like
compatibility promise").
> Obviously, this can only be done
>for options that support JSON.
We can also degrade or even deprecate sugar options in favor of the real
ones. Case by case, I guess.
> Peter Maydell also wants to do this
>only after a big user (read: libvirt) has implemented and is
>using JSON, basically as a proof that the alternative is working.
>
>So this can certainly be done for -object. I believe libvirt also
>uses JSON for -device now, so this should be fine now, too.
The non-sugar options supporting JSON are -audiodev, -blockdev, -compat,
-display (partially), -machine (I think), -object.
-netdev is QAPIfied, but still uses QemuOpts. Too late for 6.2, I'm
afraid.
>Possibly
>-drive (in favour of -blockdev), though I'm not completely sure if we
>have gotten rid of the final users of -drive. (CCing Peter Krempa for
>details.)
The problem with deprecating -drive is configuring onboard block
devices. We need a stable interface for that, and it must be usable
together with -blockdev.
We provided such an interface (machine properties) for some onboard
block devices starting with commit ebc29e1b
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
On Fri, Nov 05, 2021 at 14:19:34 +0100, Kevin Wolf wrote: > Am 05.11.2021 um 11:08 hat Markus Armbruster geschrieben: > > Kevin Wolf writes: > > > > > Am 04.11.2021 um 13:13 hat Markus Armbruster geschrieben: [...] > We can deprecate on two different levels. I think it's useful to do > both: > > 1. Broad deprecation: Stable non-JSON interfaces are degraded to >a HMP-like compatibility promise. Obviously, this can only be done >for options that support JSON. Peter Maydell also wants to do this >only after a big user (read: libvirt) has implemented and is >using JSON, basically as a proof that the alternative is working. > >So this can certainly be done for -object. I believe libvirt also >uses JSON for -device now, so this should be fine now, too. Possibly >-drive (in favour of -blockdev), though I'm not completely sure if we >have gotten rid of the final users of -drive. (CCing Peter Krempa for >details.) Libvirt uses JSON exclusively with -object (starting from qemu-6.0) and with -device (starting from qemu-6.2). We could also easily do -netdev if JSON support will be added for the option as internally we already create the JSON object (so that the code is identical for QMP hotplug) and just convert it back to the commandline syntax. Libvirt also uses JSON with -blockdev since -blockdev support was introduced (IIRC qemu-4.2 added all the necessary bits for us). If -blockdev is used libvirt does not use -drive at all (except for SD cards for ARM boards), but that is not tested very well.
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
Am 05.11.2021 um 11:08 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 04.11.2021 um 13:13 hat Markus Armbruster geschrieben:
> >> The old syntax almost always has its quirks. Ideally, we'd somehow get
> >> from quirky old to boring new in an orderly manner. Sadly, we still
> >> don't have good solutions for that. To make progress, we commonly
> >> combine JSON new with quirky old.
> >>
> >> qemu-system-FOO -object works that way. object_option_parse() parses
> >> either JSON or QemuOpts. It wraps the former in a QObject visitor, and
> >> the latter in an opts visitor.
> >>
> >> QemuOpts is flat by design[*], so the opts visitor parses flat QemuOpts
> >> from a (possibly non-flat) QAPI type. How exactly it flattens, and how
> >> it handles clashes I don't remember.
> >>
> >> Sadly, this means that we get quirky old even for new object types.
> >
> > For -object in the system emulator (the tools all use the keyval
> > visitor, so there it would work as expected), the only reason that we
> > need to keep the quirky old code path around is the list handling in
> > memory-backend.host-nodes.
> >
> > The main difficulty there is that the old QemuOpts based code path
> > allows specifying the option twice and both of them would effectively be
> > combined. Do we have any idea how to replicate this in a keyval parser
> > based world?
>
> I can see just two clean solutions, but both involve upending a lot of
> code.
>
> We can fuse keyval parser and visitor to get a schema-directed parser.
>
> We can change the abstract keyval syntax to permit repeated keys. This
> means replacing QDict in in the abstract syntax tree, with fallout in
> the visitor.
>
> Even if we find a practical solution, I don't like the combination of
> "you may give the same parameter multiple times, and the last one wins"
> and "for a list-valued parameter, the values of repeated parameters are
> collected into a list". Each makes sense on its own. The combination
> not so much. Inheriting "last one wins" from QemuOpts may have been a
> mistake.
>
> The keyval way of doing lists (inherited from the block layer's usage of
> dotted keys? I don't remember) requires the user to count, which isn't
> exactly nice, either.
Yes. If we didn't have to maintain compatibility (or actually as soon as
we degrade non-JSON option lists to HMP-level support), I would
introduce [] and {} syntax for lists and dicts, even if that means that
use of these characters in strings doesn't work any more or only in a
limited way. I think this would be the best compromise for usability.
Anyway, this doesn't help us with the compatibility problem we're
discussing here.
> > If not, do we want to use the remaining time until 6.2 to deprecate
> > this? The nasty part is that the only syntax that works both now and in
> > the future is JSON. We can't easily accept the new keyval syntax while
> > still using the QemuOpts based code.
>
> What exactly do you propose to deprecate?
We can deprecate on two different levels. I think it's useful to do
both:
1. Broad deprecation: Stable non-JSON interfaces are degraded to
a HMP-like compatibility promise. Obviously, this can only be done
for options that support JSON. Peter Maydell also wants to do this
only after a big user (read: libvirt) has implemented and is
using JSON, basically as a proof that the alternative is working.
So this can certainly be done for -object. I believe libvirt also
uses JSON for -device now, so this should be fine now, too. Possibly
-drive (in favour of -blockdev), though I'm not completely sure if we
have gotten rid of the final users of -drive. (CCing Peter Krempa for
details.)
This degradation of the compatibility promise doesn't tell users what
exactly is going to change, which is why doing the second one, too,
might be nice.
2. Narrow deprecation: We can just deprecate the non-JSON form, or
certain aspects of it, of memory-backend.host-nodes. This is the
specific things that stops us from switching -object to keyval.
a. Deprecate the whole option. If you want to use it and need a
stable interface, you have to use JSON. We'll just switch the
non-JSON form on a flag day. Before it, you need to use QemuOpts +
OptsVisitor syntax for the list; after it, you need to use keyval
syntax.
b. Deprecate only repeating the option. memory-backend is changed to
first try visiting a list, and if that fails, it visits a string
and goes through a string visitor locally to keep supporting the
integer range syntax.
c. Deprecate all list values, but keep supporting a single integer
value by using an alternate between list and int.
Picking one of these four options is enough to convert -object to
keyval. I would suggest doing both 1. and one of the options in 2.
Kevin
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
Kevin Wolf writes: > Am 04.11.2021 um 13:13 hat Markus Armbruster geschrieben: >> The old syntax almost always has its quirks. Ideally, we'd somehow get >> from quirky old to boring new in an orderly manner. Sadly, we still >> don't have good solutions for that. To make progress, we commonly >> combine JSON new with quirky old. >> >> qemu-system-FOO -object works that way. object_option_parse() parses >> either JSON or QemuOpts. It wraps the former in a QObject visitor, and >> the latter in an opts visitor. >> >> QemuOpts is flat by design[*], so the opts visitor parses flat QemuOpts >> from a (possibly non-flat) QAPI type. How exactly it flattens, and how >> it handles clashes I don't remember. >> >> Sadly, this means that we get quirky old even for new object types. > > For -object in the system emulator (the tools all use the keyval > visitor, so there it would work as expected), the only reason that we > need to keep the quirky old code path around is the list handling in > memory-backend.host-nodes. > > The main difficulty there is that the old QemuOpts based code path > allows specifying the option twice and both of them would effectively be > combined. Do we have any idea how to replicate this in a keyval parser > based world? I can see just two clean solutions, but both involve upending a lot of code. We can fuse keyval parser and visitor to get a schema-directed parser. We can change the abstract keyval syntax to permit repeated keys. This means replacing QDict in in the abstract syntax tree, with fallout in the visitor. Even if we find a practical solution, I don't like the combination of "you may give the same parameter multiple times, and the last one wins" and "for a list-valued parameter, the values of repeated parameters are collected into a list". Each makes sense on its own. The combination not so much. Inheriting "last one wins" from QemuOpts may have been a mistake. The keyval way of doing lists (inherited from the block layer's usage of dotted keys? I don't remember) requires the user to count, which isn't exactly nice, either. > If not, do we want to use the remaining time until 6.2 to deprecate > this? The nasty part is that the only syntax that works both now and in > the future is JSON. We can't easily accept the new keyval syntax while > still using the QemuOpts based code. What exactly do you propose to deprecate?
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
On Thu, Nov 04, 2021 at 01:13:02PM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi writes:
>
> > On Fri, Oct 29, 2021 at 02:42:49PM +, Jag Raman wrote:
> >> > On Oct 27, 2021, at 11:40 AM, Stefan Hajnoczi
> >> > wrote:
> >> > On Mon, Oct 11, 2021 at 01:31:08AM -0400, Jagannathan Raman wrote:
> >> >> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
> >> >> new file mode 100644
> >> >> index 00..c2a300f0ff
> >> >> --- /dev/null
> >> >> +++ b/hw/remote/vfio-user-obj.c
> >> >> @@ -0,0 +1,173 @@
> >> >> +/**
> >> >> + * QEMU vfio-user-server server object
> >> >> + *
> >> >> + * Copyright © 2021 Oracle and/or its affiliates.
> >> >> + *
> >> >> + * This work is licensed under the terms of the GNU GPL-v2, version 2
> >> >> or later.
> >> >> + *
> >> >> + * See the COPYING file in the top-level directory.
> >> >> + *
> >> >> + */
> >> >> +
> >> >> +/**
> >> >> + * Usage: add options:
> >> >> + * -machine x-remote
> >> >> + * -device ,id=
> >> >> + * -object vfio-user-server,id=,type=unix,path=,
> >> >
> >> > I expected socket.type= and socket.path= based on the QAPI schema. Is
> >> > this command-line example correct?
> >>
> >> When I tried the “socket.path” approach, QEMU was not able to parse the
> >> arguments. So I had to break it down to a series of individual members.
> >>
> >> If “socket.path” is the expected way, I’ll see why the parser is not
> >> working
> >> as expected.
> >
> > CCing Markus regarding QAPI.
> >
> > I'm surprised because the QAPI schema for vfio-user-server objects is:
> >
> > { 'struct': 'VfioUserServerProperties',
> > 'data': { 'socket': 'SocketAddress', 'device': 'str' } }
> >
> > It's not clear to me why the command-line parser flattens the 'socket'
> > field into its 'type' and 'path' sub-fields in your example:
> >
> > -object
> > vfio-user-server,id=,type=unix,path=,device=
> >
> > Maybe because SocketAddress is an enum instead of a struct?
> >
> > Imagine a second SocketAddress field is added to vfio-user-server. How
> > can the parser know which field 'type' and 'path' belong to? I tried it:
> >
> > { 'struct': 'VfioUserServerProperties',
> > 'data': { 'socket': 'SocketAddress', 'socket2': 'SocketAddress',
> > 'device': 'str' } }
> >
> > Now the parser refuses any input I've tried. For example:
> >
> > $ build/qemu-system-x86_64 -object
> > vfio-user-server,id=s,device=asdf,type=unix,path=asdf,type=unix
> > qemu-system-x86_64: -object
> > vfio-user-server,id=s,device=asdf,type=unix,path=asdf,type=unix: Parameter
> > 'type' is missing
> >
> > A similar case happens if the parent struct has 'type' or 'path' fields.
> > They collide with the SocketAddress union fields. I didn't test this
> > though.
> >
> > Questions for Markus:
> > 1. Do you know why the parser behaves like this?
>
> Yes: backward compatibility.
>
> The straightforward way to do a QAPI-based command line option uses
> qobject_input_visitor_new_str(), which parses either JSON or dotted
> keys, and returns the result wrapped in the appropriate QObject visitor.
>
> The JSON syntax is derived from the QAPI schema just like for QMP. For
> the VfioUserServerProperties shown above, it's something like
>
> {"socket": {"type": "unix", "path": "dir/socket"}, "device" "mumble"}
>
> I did not check my derivation by feeding it to QEMU. Beware of
> screwups.
>
> The dotted keys syntax is derived from the JSON syntax as described in
> keyval.c. For the JSON above, it should be
>
> socket.type=unix,socket.path=dir/socket,device=mumble
>
> When we QAPIfy an existing option instead of adding a new QAPI-based
> one, we have an additional problem: the dotted keys syntax has to match
> the old syntax (the JSON syntax is all new, so no problem there).
>
> The old syntax almost always has its quirks. Ideally, we'd somehow get
> from quirky old to boring new in an orderly manner. Sadly, we still
> don't have good solutions for that. To make progress, we commonly
> combine JSON new with quirky old.
>
> qemu-system-FOO -object works that way. object_option_parse() parses
> either JSON or QemuOpts. It wraps the former in a QObject visitor, and
> the latter in an opts visitor.
>
> QemuOpts is flat by design[*], so the opts visitor parses flat QemuOpts
> from a (possibly non-flat) QAPI type. How exactly it flattens, and how
> it handles clashes I don't remember.
>
> Sadly, this means that we get quirky old even for new object types.
>
> Questions?
>
> > 2. Is it good practice to embed SocketAddress into parent structs or
> >should we force it into a struct?
>
> I'm not sure I got your question. An example might help.
I think Question 2 isn't relevant anymore.
Thanks for the explanation!
> [*] You can play games with dotted keys to simulate nesting, but the
> opts visitor predates all that.
>
signature.asc
Description: PGP signature
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
Am 04.11.2021 um 13:13 hat Markus Armbruster geschrieben: > The old syntax almost always has its quirks. Ideally, we'd somehow get > from quirky old to boring new in an orderly manner. Sadly, we still > don't have good solutions for that. To make progress, we commonly > combine JSON new with quirky old. > > qemu-system-FOO -object works that way. object_option_parse() parses > either JSON or QemuOpts. It wraps the former in a QObject visitor, and > the latter in an opts visitor. > > QemuOpts is flat by design[*], so the opts visitor parses flat QemuOpts > from a (possibly non-flat) QAPI type. How exactly it flattens, and how > it handles clashes I don't remember. > > Sadly, this means that we get quirky old even for new object types. For -object in the system emulator (the tools all use the keyval visitor, so there it would work as expected), the only reason that we need to keep the quirky old code path around is the list handling in memory-backend.host-nodes. The main difficulty there is that the old QemuOpts based code path allows specifying the option twice and both of them would effectively be combined. Do we have any idea how to replicate this in a keyval parser based world? If not, do we want to use the remaining time until 6.2 to deprecate this? The nasty part is that the only syntax that works both now and in the future is JSON. We can't easily accept the new keyval syntax while still using the QemuOpts based code. Kevin
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
Stefan Hajnoczi writes:
> On Fri, Oct 29, 2021 at 02:42:49PM +, Jag Raman wrote:
>> > On Oct 27, 2021, at 11:40 AM, Stefan Hajnoczi wrote:
>> > On Mon, Oct 11, 2021 at 01:31:08AM -0400, Jagannathan Raman wrote:
>> >> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>> >> new file mode 100644
>> >> index 00..c2a300f0ff
>> >> --- /dev/null
>> >> +++ b/hw/remote/vfio-user-obj.c
>> >> @@ -0,0 +1,173 @@
>> >> +/**
>> >> + * QEMU vfio-user-server server object
>> >> + *
>> >> + * Copyright © 2021 Oracle and/or its affiliates.
>> >> + *
>> >> + * This work is licensed under the terms of the GNU GPL-v2, version 2 or
>> >> later.
>> >> + *
>> >> + * See the COPYING file in the top-level directory.
>> >> + *
>> >> + */
>> >> +
>> >> +/**
>> >> + * Usage: add options:
>> >> + * -machine x-remote
>> >> + * -device ,id=
>> >> + * -object vfio-user-server,id=,type=unix,path=,
>> >
>> > I expected socket.type= and socket.path= based on the QAPI schema. Is
>> > this command-line example correct?
>>
>> When I tried the “socket.path” approach, QEMU was not able to parse the
>> arguments. So I had to break it down to a series of individual members.
>>
>> If “socket.path” is the expected way, I’ll see why the parser is not working
>> as expected.
>
> CCing Markus regarding QAPI.
>
> I'm surprised because the QAPI schema for vfio-user-server objects is:
>
> { 'struct': 'VfioUserServerProperties',
> 'data': { 'socket': 'SocketAddress', 'device': 'str' } }
>
> It's not clear to me why the command-line parser flattens the 'socket'
> field into its 'type' and 'path' sub-fields in your example:
>
> -object
> vfio-user-server,id=,type=unix,path=,device=
>
> Maybe because SocketAddress is an enum instead of a struct?
>
> Imagine a second SocketAddress field is added to vfio-user-server. How
> can the parser know which field 'type' and 'path' belong to? I tried it:
>
> { 'struct': 'VfioUserServerProperties',
> 'data': { 'socket': 'SocketAddress', 'socket2': 'SocketAddress',
> 'device': 'str' } }
>
> Now the parser refuses any input I've tried. For example:
>
> $ build/qemu-system-x86_64 -object
> vfio-user-server,id=s,device=asdf,type=unix,path=asdf,type=unix
> qemu-system-x86_64: -object
> vfio-user-server,id=s,device=asdf,type=unix,path=asdf,type=unix: Parameter
> 'type' is missing
>
> A similar case happens if the parent struct has 'type' or 'path' fields.
> They collide with the SocketAddress union fields. I didn't test this
> though.
>
> Questions for Markus:
> 1. Do you know why the parser behaves like this?
Yes: backward compatibility.
The straightforward way to do a QAPI-based command line option uses
qobject_input_visitor_new_str(), which parses either JSON or dotted
keys, and returns the result wrapped in the appropriate QObject visitor.
The JSON syntax is derived from the QAPI schema just like for QMP. For
the VfioUserServerProperties shown above, it's something like
{"socket": {"type": "unix", "path": "dir/socket"}, "device" "mumble"}
I did not check my derivation by feeding it to QEMU. Beware of
screwups.
The dotted keys syntax is derived from the JSON syntax as described in
keyval.c. For the JSON above, it should be
socket.type=unix,socket.path=dir/socket,device=mumble
When we QAPIfy an existing option instead of adding a new QAPI-based
one, we have an additional problem: the dotted keys syntax has to match
the old syntax (the JSON syntax is all new, so no problem there).
The old syntax almost always has its quirks. Ideally, we'd somehow get
from quirky old to boring new in an orderly manner. Sadly, we still
don't have good solutions for that. To make progress, we commonly
combine JSON new with quirky old.
qemu-system-FOO -object works that way. object_option_parse() parses
either JSON or QemuOpts. It wraps the former in a QObject visitor, and
the latter in an opts visitor.
QemuOpts is flat by design[*], so the opts visitor parses flat QemuOpts
from a (possibly non-flat) QAPI type. How exactly it flattens, and how
it handles clashes I don't remember.
Sadly, this means that we get quirky old even for new object types.
Questions?
> 2. Is it good practice to embed SocketAddress into parent structs or
>should we force it into a struct?
I'm not sure I got your question. An example might help.
[*] You can play games with dotted keys to simulate nesting, but the
opts visitor predates all that.
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
On Fri, Oct 29, 2021 at 02:42:49PM +, Jag Raman wrote:
> > On Oct 27, 2021, at 11:40 AM, Stefan Hajnoczi wrote:
> > On Mon, Oct 11, 2021 at 01:31:08AM -0400, Jagannathan Raman wrote:
> >> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
> >> new file mode 100644
> >> index 00..c2a300f0ff
> >> --- /dev/null
> >> +++ b/hw/remote/vfio-user-obj.c
> >> @@ -0,0 +1,173 @@
> >> +/**
> >> + * QEMU vfio-user-server server object
> >> + *
> >> + * Copyright © 2021 Oracle and/or its affiliates.
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL-v2, version 2 or
> >> later.
> >> + *
> >> + * See the COPYING file in the top-level directory.
> >> + *
> >> + */
> >> +
> >> +/**
> >> + * Usage: add options:
> >> + * -machine x-remote
> >> + * -device ,id=
> >> + * -object vfio-user-server,id=,type=unix,path=,
> >
> > I expected socket.type= and socket.path= based on the QAPI schema. Is
> > this command-line example correct?
>
> When I tried the “socket.path” approach, QEMU was not able to parse the
> arguments. So I had to break it down to a series of individual members.
>
> If “socket.path” is the expected way, I’ll see why the parser is not working
> as expected.
CCing Markus regarding QAPI.
I'm surprised because the QAPI schema for vfio-user-server objects is:
{ 'struct': 'VfioUserServerProperties',
'data': { 'socket': 'SocketAddress', 'device': 'str' } }
It's not clear to me why the command-line parser flattens the 'socket'
field into its 'type' and 'path' sub-fields in your example:
-object
vfio-user-server,id=,type=unix,path=,device=
Maybe because SocketAddress is an enum instead of a struct?
Imagine a second SocketAddress field is added to vfio-user-server. How
can the parser know which field 'type' and 'path' belong to? I tried it:
{ 'struct': 'VfioUserServerProperties',
'data': { 'socket': 'SocketAddress', 'socket2': 'SocketAddress', 'device':
'str' } }
Now the parser refuses any input I've tried. For example:
$ build/qemu-system-x86_64 -object
vfio-user-server,id=s,device=asdf,type=unix,path=asdf,type=unix
qemu-system-x86_64: -object
vfio-user-server,id=s,device=asdf,type=unix,path=asdf,type=unix: Parameter
'type' is missing
A similar case happens if the parent struct has 'type' or 'path' fields.
They collide with the SocketAddress union fields. I didn't test this
though.
Questions for Markus:
1. Do you know why the parser behaves like this?
2. Is it good practice to embed SocketAddress into parent structs or
should we force it into a struct?
Stefan
signature.asc
Description: PGP signature
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
> On Oct 27, 2021, at 11:40 AM, Stefan Hajnoczi wrote:
>
> On Mon, Oct 11, 2021 at 01:31:08AM -0400, Jagannathan Raman wrote:
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index 0222bb4506..97de79cc36 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -705,6 +705,20 @@
>> { 'struct': 'RemoteObjectProperties',
>> 'data': { 'fd': 'str', 'devid': 'str' } }
>>
>> +##
>> +# @VfioUserServerProperties:
>> +#
>> +# Properties for vfio-user-server objects.
>> +#
>> +# @socket: socket to be used by the libvfiouser library
>> +#
>> +# @device: the id of the device to be emulated at the server
>> +#
>> +# Since: 6.0
>
> 6.2
>
>> +##
>> +{ 'struct': 'VfioUserServerProperties',
>> + 'data': { 'socket': 'SocketAddress', 'device': 'str' } }
>> +
>> ##
>> # @RngProperties:
>> #
>> @@ -830,7 +844,8 @@
>> 'tls-creds-psk',
>> 'tls-creds-x509',
>> 'tls-cipher-suites',
>> -'x-remote-object'
>> +'x-remote-object',
>> +'vfio-user-server'
>
> Should it have the experimental prefix ('x-') or is the QAPI interface
> stable? I think some things to think about are whether a single process
> can host multiple device servers, whether hotplug is possible, etc. If
> the interface is stable then it must be able to accomodate future
> features (at least ones we can anticipate right now :)).
We did test out hotplug support.
We’ll get back to you on the multiple device servers scenario.
>
>> ] }
>>
>> ##
>> @@ -887,7 +902,8 @@
>> 'tls-creds-psk': 'TlsCredsPskProperties',
>> 'tls-creds-x509': 'TlsCredsX509Properties',
>> 'tls-cipher-suites': 'TlsCredsProperties',
>> - 'x-remote-object':'RemoteObjectProperties'
>> + 'x-remote-object':'RemoteObjectProperties',
>> + 'vfio-user-server': 'VfioUserServerProperties'
>> } }
>>
>> ##
>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>> new file mode 100644
>> index 00..c2a300f0ff
>> --- /dev/null
>> +++ b/hw/remote/vfio-user-obj.c
>> @@ -0,0 +1,173 @@
>> +/**
>> + * QEMU vfio-user-server server object
>> + *
>> + * Copyright © 2021 Oracle and/or its affiliates.
>> + *
>> + * This work is licensed under the terms of the GNU GPL-v2, version 2 or
>> later.
>> + *
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +/**
>> + * Usage: add options:
>> + * -machine x-remote
>> + * -device ,id=
>> + * -object vfio-user-server,id=,type=unix,path=,
>
> I expected socket.type= and socket.path= based on the QAPI schema. Is
> this command-line example correct?
When I tried the “socket.path” approach, QEMU was not able to parse the
arguments. So I had to break it down to a series of individual members.
If “socket.path” is the expected way, I’ll see why the parser is not working
as expected.
>
>> + * device=
>> + *
>> + * Note that vfio-user-server object must be used with x-remote machine
>> only.
>> + * This server could only support PCI devices for now.
>> + *
>> + * type - SocketAddress type - presently "unix" alone is supported. Required
>> + *option
>> + *
>> + * path - named unix socket, it will be created by the server. It is
>> + *a required option
>> + *
>> + * device - id of a device on the server, a required option. PCI devices
>> + * alone are supported presently.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +
>> +#include "qom/object.h"
>> +#include "qom/object_interfaces.h"
>> +#include "qemu/error-report.h"
>> +#include "trace.h"
>> +#include "sysemu/runstate.h"
>> +#include "hw/boards.h"
>> +#include "hw/remote/machine.h"
>> +#include "qapi/error.h"
>> +#include "qapi/qapi-visit-sockets.h"
>> +
>> +#define TYPE_VFU_OBJECT "vfio-user-server"
>> +OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>> +
>> +struct VfuObjectClass {
>> +ObjectClass parent_class;
>> +
>> +unsigned int nr_devs;
>> +
>> +/* Maximum number of devices the server could support */
>> +unsigned int max_devs;
>> +};
>> +
>> +struct VfuObject {
>> +/* private */
>> +Object parent;
>> +
>> +SocketAddress *socket;
>> +
>> +char *device;
>> +};
>> +
>> +static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
>> + void *opaque, Error **errp)
>> +{
>> +VfuObject *o = VFU_OBJECT(obj);
>> +
>> +g_free(o->socket);
>
> qapi_free_SocketAddress(o->socket)?
OK, will use that.
Didn’t know the visitors also define a function for free’ing. Thank you!
>
>> +
>> +o->socket = NULL;
>> +
>> +visit_type_SocketAddress(v, name, &o->socket, errp);
>> +
>> +if (o->socket->type != SOCKET_ADDRESS_TYPE_UNIX) {
>> +error_setg(&error_abort, "vfu: Unsupported socket type - %s",
>> + o->socket->u.q_unix.path);
>
> o->socket must be freed and set it to NULL again, otherwise setting the
> property appears to fail but the SocketAdd
Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object
On Mon, Oct 11, 2021 at 01:31:08AM -0400, Jagannathan Raman wrote:
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 0222bb4506..97de79cc36 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -705,6 +705,20 @@
> { 'struct': 'RemoteObjectProperties',
>'data': { 'fd': 'str', 'devid': 'str' } }
>
> +##
> +# @VfioUserServerProperties:
> +#
> +# Properties for vfio-user-server objects.
> +#
> +# @socket: socket to be used by the libvfiouser library
> +#
> +# @device: the id of the device to be emulated at the server
> +#
> +# Since: 6.0
6.2
> +##
> +{ 'struct': 'VfioUserServerProperties',
> + 'data': { 'socket': 'SocketAddress', 'device': 'str' } }
> +
> ##
> # @RngProperties:
> #
> @@ -830,7 +844,8 @@
> 'tls-creds-psk',
> 'tls-creds-x509',
> 'tls-cipher-suites',
> -'x-remote-object'
> +'x-remote-object',
> +'vfio-user-server'
Should it have the experimental prefix ('x-') or is the QAPI interface
stable? I think some things to think about are whether a single process
can host multiple device servers, whether hotplug is possible, etc. If
the interface is stable then it must be able to accomodate future
features (at least ones we can anticipate right now :)).
>] }
>
> ##
> @@ -887,7 +902,8 @@
>'tls-creds-psk': 'TlsCredsPskProperties',
>'tls-creds-x509': 'TlsCredsX509Properties',
>'tls-cipher-suites': 'TlsCredsProperties',
> - 'x-remote-object':'RemoteObjectProperties'
> + 'x-remote-object':'RemoteObjectProperties',
> + 'vfio-user-server': 'VfioUserServerProperties'
>} }
>
> ##
> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
> new file mode 100644
> index 00..c2a300f0ff
> --- /dev/null
> +++ b/hw/remote/vfio-user-obj.c
> @@ -0,0 +1,173 @@
> +/**
> + * QEMU vfio-user-server server object
> + *
> + * Copyright © 2021 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL-v2, version 2 or
> later.
> + *
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +/**
> + * Usage: add options:
> + * -machine x-remote
> + * -device ,id=
> + * -object vfio-user-server,id=,type=unix,path=,
I expected socket.type= and socket.path= based on the QAPI schema. Is
this command-line example correct?
> + * device=
> + *
> + * Note that vfio-user-server object must be used with x-remote machine only.
> + * This server could only support PCI devices for now.
> + *
> + * type - SocketAddress type - presently "unix" alone is supported. Required
> + *option
> + *
> + * path - named unix socket, it will be created by the server. It is
> + *a required option
> + *
> + * device - id of a device on the server, a required option. PCI devices
> + * alone are supported presently.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +
> +#include "qom/object.h"
> +#include "qom/object_interfaces.h"
> +#include "qemu/error-report.h"
> +#include "trace.h"
> +#include "sysemu/runstate.h"
> +#include "hw/boards.h"
> +#include "hw/remote/machine.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-visit-sockets.h"
> +
> +#define TYPE_VFU_OBJECT "vfio-user-server"
> +OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
> +
> +struct VfuObjectClass {
> +ObjectClass parent_class;
> +
> +unsigned int nr_devs;
> +
> +/* Maximum number of devices the server could support */
> +unsigned int max_devs;
> +};
> +
> +struct VfuObject {
> +/* private */
> +Object parent;
> +
> +SocketAddress *socket;
> +
> +char *device;
> +};
> +
> +static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> +VfuObject *o = VFU_OBJECT(obj);
> +
> +g_free(o->socket);
qapi_free_SocketAddress(o->socket)?
> +
> +o->socket = NULL;
> +
> +visit_type_SocketAddress(v, name, &o->socket, errp);
> +
> +if (o->socket->type != SOCKET_ADDRESS_TYPE_UNIX) {
> +error_setg(&error_abort, "vfu: Unsupported socket type - %s",
> + o->socket->u.q_unix.path);
o->socket must be freed and set it to NULL again, otherwise setting the
property appears to fail but the SocketAddress actually retains the
invalid value.
> +return;
> +}
> +
> +trace_vfu_prop("socket", o->socket->u.q_unix.path);
> +}
> +
> +static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
> +{
> +VfuObject *o = VFU_OBJECT(obj);
> +
> +g_free(o->device);
> +
> +o->device = g_strdup(str);
> +
> +trace_vfu_prop("device", str);
> +}
> +
> +static void vfu_object_init(Object *obj)
> +{
> +VfuObjectClass *k = VFU_OBJECT_GET_CLASS(obj);
> +
> +if (!object_dynamic_cast(OBJECT(current_machine), TYPE_REMOTE_MACHINE)) {
> +error_setg(&error_abort, "vfu: %s only compatible with %s machine",
