Re: [Qemu-devel] [PATCH for-2.3? 7/7] tests/tcg: Enable runcom test by default
> Am 11.04.2015 um 23:46 schrieb Andreas Färber :
>
>> Am 11.04.2015 um 23:33 schrieb Peter Maydell:
>>> On 11 April 2015 at 21:28, Andreas Färber wrote:
>>> Are you sure that QEMU / runcom / pi_10.com still try to map to that
>>> address zero?
>>
>> It's the first thing runcom.c's main() does after checking you've
>> passed it enough command line arguments.
>
> This?
>
>vm86_mem = mmap((void *)0x, 0x11,
>PROT_WRITE | PROT_READ | PROT_EXEC,
>MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
>if (vm86_mem == MAP_FAILED) {
>perror("mmap");
>exit(1);
>}
>
> For whatever reason it does not take the MAP_FAILED path here...
Wasn't a NULL argument for the address a hint saying "map wherever you please"?
I also feel like I'm missing context here - what exactly is broken?
Alex
>
> But I can certainly live with dropping this patch, it's the last for a
> reason. :)
>
> Andreas
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
> Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH for-2.3? 7/7] tests/tcg: Enable runcom test by default
Am 12.04.2015 um 10:34 schrieb Alexander Graf:
>> Am 11.04.2015 um 23:46 schrieb Andreas Färber :
>>
>>> Am 11.04.2015 um 23:33 schrieb Peter Maydell:
On 11 April 2015 at 21:28, Andreas Färber wrote:
Are you sure that QEMU / runcom / pi_10.com still try to map to that
address zero?
>>>
>>> It's the first thing runcom.c's main() does after checking you've
>>> passed it enough command line arguments.
>>
>> This?
>>
>>vm86_mem = mmap((void *)0x, 0x11,
>>PROT_WRITE | PROT_READ | PROT_EXEC,
>>MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
>>if (vm86_mem == MAP_FAILED) {
>>perror("mmap");
>>exit(1);
>>}
>>
>> For whatever reason it does not take the MAP_FAILED path here...
>
> Wasn't a NULL argument for the address a hint saying "map wherever you
> please"? I also feel like I'm missing context here - what exactly is broken?
Sorry for CC'ing you so late, I believe you and rth had been fiddling
with guest bases and that stuff back in the day... Please just take a
look at the whole series, in particular patches 3 and 7.
Thanks,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH] Fix crash with illegal "-net nic, model=xxx" option
On Thu, Apr 09, 2015 at 03:32:45PM +0200, Thomas Huth wrote:
> Current QEMU crashes when specifying an illegal model with the
> "-net nic,model=xxx" option, e.g.:
>
> $ qemu-system-x86_64 -net nic,model=n/a
> qemu-system-x86_64: Unsupported NIC model: n/a
>
> Program received signal SIGSEGV, Segmentation fault.
>
> The gdb backtrace looks like this:
>
> 0x55965fe0 in error_get_pretty (err=0x0) at util/error.c:152
> 152 return err->msg;
> (gdb) bt
> 0 0x55965fe0 in error_get_pretty (err=0x0) at util/error.c:152
> 1 0x55965ffd in error_report_err (err=0x0) at util/error.c:157
> 2 0x55809c90 in pci_nic_init_nofail (nd=0x55e49860 ,
> rootbus=0x564409b0,
> default_model=0x5598c37b "e1000", default_devaddr=0x0) at
> hw/pci/pci.c:1663
> 3 0x55691e42 in pc_nic_init (isa_bus=0x56f71900,
> pci_bus=0x564409b0)
> at hw/i386/pc.c:1506
> 4 0x5569396b in pc_init1 (machine=0x562abbf0, pci_enabled=1,
> kvmclock_enabled=1)
> at hw/i386/pc_piix.c:248
> 5 0x55693d27 in pc_init_pci (machine=0x562abbf0) at
> hw/i386/pc_piix.c:310
> 6 0x5572ddf5 in main (argc=3, argv=0x7fffe018,
> envp=0x7fffe038) at vl.c:4226
>
> The problem is that pci_nic_init_nofail() does not check whether the err
> parameter from pci_nic_init has been set up and thus passes a NULL pointer
> to error_report_err(). Fix it by correctly checking the err parameter.
>
> Signed-off-by: Thomas Huth
Reviewed-by: Michael S. Tsirkin
> ---
> hw/pci/pci.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 6941a82..b3d5100 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1660,7 +1660,9 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus
> *rootbus,
>
> res = pci_nic_init(nd, rootbus, default_model, default_devaddr, &err);
> if (!res) {
> -error_report_err(err);
> +if (err) {
> +error_report_err(err);
> +}
> exit(1);
> }
> return res;
> --
> 1.8.3.1
Re: [Qemu-devel] [PATCH] Fix crash with illegal "-net nic, model=xxx" option
On Thu, Apr 09, 2015 at 03:48:57PM +0100, Peter Maydell wrote: > On 9 April 2015 at 14:37, Michael S. Tsirkin wrote: > > On Thu, Apr 09, 2015 at 03:32:45PM +0200, Thomas Huth wrote: > >> Current QEMU crashes when specifying an illegal model with the > >> "-net nic,model=xxx" option, e.g.: > >> > >> $ qemu-system-x86_64 -net nic,model=n/a > >> qemu-system-x86_64: Unsupported NIC model: n/a > >> > >> Program received signal SIGSEGV, Segmentation fault. > >> > >> The gdb backtrace looks like this: > >> > >> 0x55965fe0 in error_get_pretty (err=0x0) at util/error.c:152 > >> 152 return err->msg; > >> (gdb) bt > >> 0 0x55965fe0 in error_get_pretty (err=0x0) at util/error.c:152 > >> 1 0x55965ffd in error_report_err (err=0x0) at util/error.c:157 > >> 2 0x55809c90 in pci_nic_init_nofail (nd=0x55e49860 > >> , rootbus=0x564409b0, > >> default_model=0x5598c37b "e1000", default_devaddr=0x0) at > >> hw/pci/pci.c:1663 > >> 3 0x55691e42 in pc_nic_init (isa_bus=0x56f71900, > >> pci_bus=0x564409b0) > >> at hw/i386/pc.c:1506 > >> 4 0x5569396b in pc_init1 (machine=0x562abbf0, pci_enabled=1, > >> kvmclock_enabled=1) > >> at hw/i386/pc_piix.c:248 > >> 5 0x55693d27 in pc_init_pci (machine=0x562abbf0) at > >> hw/i386/pc_piix.c:310 > >> 6 0x5572ddf5 in main (argc=3, argv=0x7fffe018, > >> envp=0x7fffe038) at vl.c:4226 > >> > >> The problem is that pci_nic_init_nofail() does not check whether the err > >> parameter from pci_nic_init has been set up and thus passes a NULL pointer > >> to error_report_err(). Fix it by correctly checking the err parameter. > >> > >> Signed-off-by: Thomas Huth > > > > Thanks! > > Given that this is a legacy -net option, I'm inclined > > to fix it post-2.3, and Cc stable. > > Unfortunately I won't be able to do a pull request before rc3. > > Since this is a pretty safe and simple change I'm happy to apply > it direct to master if you like. Do you want to provide a reviewed-by > tag? > > thanks > -- PMM I reviewed the patch, and sent a tag. I'm fine with you making the decision on whether it's appropriate for 2.3. -- MST
Re: [Qemu-devel] [PATCH for-2.3? 7/7] tests/tcg: Enable runcom test by default
On 12 April 2015 at 09:34, Alexander Graf wrote:
>
>
>> Am 11.04.2015 um 23:46 schrieb Andreas Färber :
>>
>>> Am 11.04.2015 um 23:33 schrieb Peter Maydell:
On 11 April 2015 at 21:28, Andreas Färber wrote:
Are you sure that QEMU / runcom / pi_10.com still try to map to that
address zero?
>>>
>>> It's the first thing runcom.c's main() does after checking you've
>>> passed it enough command line arguments.
>>
>> This?
>>
>>vm86_mem = mmap((void *)0x, 0x11,
>>PROT_WRITE | PROT_READ | PROT_EXEC,
>>MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
>>if (vm86_mem == MAP_FAILED) {
>>perror("mmap");
>>exit(1);
>>}
>>
>> For whatever reason it does not take the MAP_FAILED path here...
>
> Wasn't a NULL argument for the address a hint saying "map wherever
> you please"?
Only if you don't also say MAP_FIXED.
That aside, I've now looked more carefully at the makefile,
and we're not running 'runcom' directly, we're running it
within QEMU's user-mode. So a guest allocation at vaddr 0
isn't going to turn into a host mmap at vaddr 0 (we actually
query the host mmap_min_addr parameter in linux-user/main.c).
So I think the issue noted in the comment on the makefile
(back in 2010) would have been a bug which we've presumably
fixed in the meantime by adding support for guest_base and
making sure it works correctly. (Looking at the revision
log suggests that guest_base should in theory have already
supported adjustment based on mmap_min_addr at the point
when runcom was taken out of the makefile, but presumably
there was a bug.)
-- PMM
Re: [Qemu-devel] [PATCH] Fix crash with illegal "-net nic, model=xxx" option
Am 12.04.2015 um 13:14 schrieb Michael S. Tsirkin: > On Thu, Apr 09, 2015 at 03:48:57PM +0100, Peter Maydell wrote: >> On 9 April 2015 at 14:37, Michael S. Tsirkin wrote: >>> On Thu, Apr 09, 2015 at 03:32:45PM +0200, Thomas Huth wrote: Current QEMU crashes when specifying an illegal model with the "-net nic,model=xxx" option, e.g.: $ qemu-system-x86_64 -net nic,model=n/a qemu-system-x86_64: Unsupported NIC model: n/a Program received signal SIGSEGV, Segmentation fault. The gdb backtrace looks like this: 0x55965fe0 in error_get_pretty (err=0x0) at util/error.c:152 152 return err->msg; (gdb) bt 0 0x55965fe0 in error_get_pretty (err=0x0) at util/error.c:152 1 0x55965ffd in error_report_err (err=0x0) at util/error.c:157 2 0x55809c90 in pci_nic_init_nofail (nd=0x55e49860 , rootbus=0x564409b0, default_model=0x5598c37b "e1000", default_devaddr=0x0) at hw/pci/pci.c:1663 3 0x55691e42 in pc_nic_init (isa_bus=0x56f71900, pci_bus=0x564409b0) at hw/i386/pc.c:1506 4 0x5569396b in pc_init1 (machine=0x562abbf0, pci_enabled=1, kvmclock_enabled=1) at hw/i386/pc_piix.c:248 5 0x55693d27 in pc_init_pci (machine=0x562abbf0) at hw/i386/pc_piix.c:310 6 0x5572ddf5 in main (argc=3, argv=0x7fffe018, envp=0x7fffe038) at vl.c:4226 The problem is that pci_nic_init_nofail() does not check whether the err parameter from pci_nic_init has been set up and thus passes a NULL pointer to error_report_err(). Fix it by correctly checking the err parameter. Signed-off-by: Thomas Huth >>> >>> Thanks! >>> Given that this is a legacy -net option, I'm inclined >>> to fix it post-2.3, and Cc stable. >>> Unfortunately I won't be able to do a pull request before rc3. >> >> Since this is a pretty safe and simple change I'm happy to apply >> it direct to master if you like. Do you want to provide a reviewed-by >> tag? > > I reviewed the patch, and sent a tag. > I'm fine with you making the decision on whether it's > appropriate for 2.3. Hope you added a "pci:" topic? :) Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files
I've done some digging from the client side. As is mentioned in Miklos'
original patch (which appears to have been shot down) the higher level
implementation appear to be broken for this. Here's what the code looks
like for fstat in fs/stat.c:
int vfs_fstat(unsigned int fd, struct kstat *stat)
{
struct fd f = fdget_raw(fd);
int error = -EBADF;
if (f.file) {
error = vfs_getattr(&f.file->f_path, stat);
fdput(f);
}
return error;
}
In other words, it only uses the open fd to derrive a path and then
executes the getattr off of that path. If that path no longer exists
(because of unlink or remove) then you are hosed. In my understanding, the
"work around" I suppose is the so-called 'silly renaming' where
remove/unlink simply does a rename until all open instances are closed.
The frustrating thing is that the 9p protocol is setup to work off of the
fid, which maps to the fd -- so its perfectly capable of the original
semantic but the high level code in fs/stat.c only wants to give a path.
I can do a work around on the client where a getattr "favors" open fids for
the operation or I can do the sillyrename hack that NFS and CIFS has used
but both of those look god-awful.
-eric
On Fri, Apr 10, 2015 at 7:30 AM, Mark Glines wrote:
> This bug makes it difficult to run a Debian Jessie guest with a 9pfs
> root filesystem, because "apt-get update" uses the open-unlink-fstat
> idiom. With this bug present, apt dies with the following error:
>
> E: Unable to determine file size for fd 7 - fstat (2: No such file or
> directory)
>
> --
> You received this bug notification because you are a member of qemu-
> devel-ml, which is subscribed to QEMU.
> https://bugs.launchpad.net/bugs/1336794
>
> Title:
> 9pfs does not honor open file handles on unlinked files
>
> Status in QEMU:
> New
>
> Bug description:
> This was originally filed over here:
> https://bugzilla.redhat.com/show_bug.cgi?id=1114221
>
> The open-unlink-fstat idiom used in some places to create an anonymous
> private temporary file does not work in a QEMU guest over a virtio-9p
> filesystem.
>
> Version-Release number of selected component (if applicable):
>
> qemu-kvm-1.6.2-6.fc20.x86_64
> qemu-system-x86-1.6.2-6.fc20.x86_64
> (those are fedora RPMs)
>
> How reproducible:
>
> Always. See this example C program:
>
> https://bugzilla.redhat.com/attachment.cgi?id=913069
>
> Steps to Reproduce:
> 1. Export a filesystem with virt-manager for the guest.
> (type: mount, driver: default, mode: passthrough)
> 2. Start guest and mount that filesystem
> (mount -t 9p -o trans=virtio,version=9p2000.L ...)
> 3. Run a program that uses open-unlink-fstat
> (in my case it was trying to compile Perl 5.20)
>
> Actual results:
>
> fstat fails:
>
> open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
> unlink("/home/tst/filename")= 0
> fstat(3, 0x23aa1a8) = -1 ENOENT (No such file or
> directory)
> close(3)
>
> Expected results:
>
> open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
> unlink("/home/tst/filename")= 0
> fstat(3, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
> fcntl(3, F_SETFD, FD_CLOEXEC) = 0
> close(3)
>
> Additional info:
>
> There was a patch put into the kernel back in '07 to handle this very
> problem for other filesystems; maybe its helpful:
>
> http://lwn.net/Articles/251228/
>
> There is also a thread on LKML from last December specifically about
> this very problem:
>
> https://lkml.org/lkml/2013/12/31/163
>
> There was a discussion on the QEMU list back in '11 that doesn't seem
> to have come to a conclusion, but did provide the test program that
> i've attached to this report:
>
> http://marc.info/?l=qemu-devel&m=130443605720648&w=2
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/qemu/+bug/1336794/+subscriptions
>
>
[Qemu-devel] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files
** Bug watch added: Red Hat Bugzilla #1114221
https://bugzilla.redhat.com/show_bug.cgi?id=1114221
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1336794
Title:
9pfs does not honor open file handles on unlinked files
Status in QEMU:
New
Bug description:
This was originally filed over here:
https://bugzilla.redhat.com/show_bug.cgi?id=1114221
The open-unlink-fstat idiom used in some places to create an anonymous
private temporary file does not work in a QEMU guest over a virtio-9p
filesystem.
Version-Release number of selected component (if applicable):
qemu-kvm-1.6.2-6.fc20.x86_64
qemu-system-x86-1.6.2-6.fc20.x86_64
(those are fedora RPMs)
How reproducible:
Always. See this example C program:
https://bugzilla.redhat.com/attachment.cgi?id=913069
Steps to Reproduce:
1. Export a filesystem with virt-manager for the guest.
(type: mount, driver: default, mode: passthrough)
2. Start guest and mount that filesystem
(mount -t 9p -o trans=virtio,version=9p2000.L ...)
3. Run a program that uses open-unlink-fstat
(in my case it was trying to compile Perl 5.20)
Actual results:
fstat fails:
open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
unlink("/home/tst/filename")= 0
fstat(3, 0x23aa1a8) = -1 ENOENT (No such file or
directory)
close(3)
Expected results:
open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
unlink("/home/tst/filename")= 0
fstat(3, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
fcntl(3, F_SETFD, FD_CLOEXEC) = 0
close(3)
Additional info:
There was a patch put into the kernel back in '07 to handle this very
problem for other filesystems; maybe its helpful:
http://lwn.net/Articles/251228/
There is also a thread on LKML from last December specifically about
this very problem:
https://lkml.org/lkml/2013/12/31/163
There was a discussion on the QEMU list back in '11 that doesn't seem
to have come to a conclusion, but did provide the test program that
i've attached to this report:
http://marc.info/?l=qemu-devel&m=130443605720648&w=2
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1336794/+subscriptions
Re: [Qemu-devel] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files
On Sun, Apr 12, 2015 at 12:42:35PM -, Eric Van Hensbergen wrote: > In other words, it only uses the open fd to derrive a path and then > executes the getattr off of that path. If that path no longer exists > (because of unlink or remove) then you are hosed. In my understanding, the > "work around" I suppose is the so-called 'silly renaming' where > remove/unlink simply does a rename until all open instances are closed. What do you mean, "no longer exists"? Don't confuse path with pathname - it's a mount,dentry pair. And dentry in question bloody well ought to still have the FID associated with it - you shouldn't use the same FID for TREMOVE and for TREAD/TWRITE. TREMOVE clunks the FID passed to it; on some servers you really have no choice - server discards the file completely and on any FID that used to refer to it you get an error from that point on. On those you'd really have to do something like sillyrename - the only way to keep IO going for a file sitting on such server is to have it visible somewhere. Normal fs(4) is that way; e.g. u9fs(4) isn't - there FID maps to opened file descriptor on host and TREMOVE on another FID doesn't break it, as long as host supports IO on opened-but-unlinked files. I don't remember where qemu 9pfs falls in that respect, but I'd expect it to be more like u9fs... Which FID are you passing to server on unlink()?
[Qemu-devel] [PATCH 2/2] virtio-balloon: virtio 1 support
Virtio 1.0 doesn't include a modern balloon device. At some point we'll
likely define an incompatible interface with a different ID and
different semantics. But for now, it's not a big effort to support a
transitional balloon device: this has the advantage of supporting
existing drivers, transparently, as well as transports that don't allow
mixing virtio 0 and virtio 1 devices. And balloon is an easy device to
test, so it's also useful for people to test virtio core handling of
transitional devices.
The only interface issue is with the stats buffer, which has misaligned
fields. We could leave it as is, but this sets a bad precedent that
others might copy by mistake.
Signed-off-by: Michael S. Tsirkin
---
hw/virtio/virtio-balloon.c | 31 +++
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index d2d7c3e..568a008 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -239,7 +239,8 @@ static void virtio_balloon_receive_stats(VirtIODevice
*vdev, VirtQueue *vq)
{
VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
VirtQueueElement *elem = &s->stats_vq_elem;
-VirtIOBalloonStat stat;
+VirtIOBalloonStat legacy_stat;
+VirtIOBalloonStatModern modern_stat;
size_t offset = 0;
qemu_timeval tv;
@@ -253,14 +254,28 @@ static void virtio_balloon_receive_stats(VirtIODevice
*vdev, VirtQueue *vq)
*/
reset_stats(s);
-while (iov_to_buf(elem->out_sg, elem->out_num, offset, &stat, sizeof(stat))
- == sizeof(stat)) {
-uint16_t tag = virtio_tswap16(vdev, stat.tag);
-uint64_t val = virtio_tswap64(vdev, stat.val);
+if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+while (iov_to_buf(elem->out_sg, elem->out_num, offset,
+ &modern_stat, sizeof(modern_stat))
+ == sizeof(modern_stat)) {
+uint16_t tag = le16_to_cpu(modern_stat.tag);
+uint64_t val = le64_to_cpu(modern_stat.val);
-offset += sizeof(stat);
-if (tag < VIRTIO_BALLOON_S_NR)
-s->stats[tag] = val;
+offset += sizeof(modern_stat);
+if (tag < VIRTIO_BALLOON_S_NR)
+s->stats[tag] = val;
+}
+} else {
+while (iov_to_buf(elem->out_sg, elem->out_num, offset,
+ &legacy_stat, sizeof(legacy_stat))
+ == sizeof(legacy_stat)) {
+uint16_t tag = virtio_tswap16(vdev, legacy_stat.tag);
+uint64_t val = virtio_tswap64(vdev, legacy_stat.val);
+
+offset += sizeof(legacy_stat);
+if (tag < VIRTIO_BALLOON_S_NR)
+s->stats[tag] = val;
+}
}
s->stats_vq_offset = offset;
--
MST
[Qemu-devel] [PATCH 1/2] virtio_balloon: header update for virtio 1
add modern header.
This patch is for virtio 1.0 branch, doesn't
apply to master.
Signed-off-by: Michael S. Tsirkin
---
include/hw/virtio/virtio-balloon.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/include/hw/virtio/virtio-balloon.h
b/include/hw/virtio/virtio-balloon.h
index f863bfe..79eca67 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -56,6 +56,12 @@ typedef struct VirtIOBalloonStat {
uint64_t val;
} QEMU_PACKED VirtIOBalloonStat;
+typedef struct virtio_balloon_stat_modern {
+ uint16_t tag;
+ uint8_t reserved[6];
+ uint64_t val;
+} VirtIOBalloonStatModern;
+
typedef struct VirtIOBalloon {
VirtIODevice parent_obj;
VirtQueue *ivq, *dvq, *svq;
--
MST
[Qemu-devel] [PATCH] virtio_blk: comment fix
update virtio blk header from latest linux, include comment fixups.
Signed-off-by: Michael S. Tsirkin
---
include/standard-headers/linux/virtio_blk.h | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/standard-headers/linux/virtio_blk.h
b/include/standard-headers/linux/virtio_blk.h
index 12016b4..cd601f4 100644
--- a/include/standard-headers/linux/virtio_blk.h
+++ b/include/standard-headers/linux/virtio_blk.h
@@ -58,7 +58,7 @@ struct virtio_blk_config {
uint32_t size_max;
/* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */
uint32_t seg_max;
- /* geometry the device (if VIRTIO_BLK_F_GEOMETRY) */
+ /* geometry of the device (if VIRTIO_BLK_F_GEOMETRY) */
struct virtio_blk_geometry {
uint16_t cylinders;
uint8_t heads;
@@ -117,7 +117,11 @@ struct virtio_blk_config {
#define VIRTIO_BLK_T_BARRIER 0x8000
#endif /* !VIRTIO_BLK_NO_LEGACY */
-/* This is the first element of the read scatter-gather list. */
+/*
+ * This comes first in the read scatter-gather list.
+ * For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated,
+ * this is the first element of the read scatter-gather list.
+ */
struct virtio_blk_outhdr {
/* VIRTIO_BLK_T* */
__virtio32 type;
--
MST
Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry
On Tue, Apr 07, 2015 at 02:51:39PM -0500, miny...@acm.org wrote:
> From: Corey Minyard
>
> There was no way to directly add a table entry to the SMBIOS table,
> even though the BIOS supports this. So add a function to do this.
> This is in preparation for the IPMI handler adding it's SMBIOS table
> entry.
>
> Signed-off-by: Corey Minyard
Do we have to use a callback for this?
It seems that if smbios_table_entry_add just added the table
itself to some array or a linked list, then devices could just call
smbios_table_entry_add instead of registering a handler.
And smbios_get_tables would scan that and get the tables.
On systems without smbios, this function could be a nop stub.
Did I miss something?
> ---
> hw/i386/smbios.c | 149
> ++-
> include/hw/i386/smbios.h | 13 +
> 2 files changed, 110 insertions(+), 52 deletions(-)
>
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index 1341e02..fe15325 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -831,6 +831,25 @@ static void smbios_entry_point_setup(void)
> ep.structure_table_address = cpu_to_le32(0);
> }
>
> +struct smbios_device_handler {
> +void (*handle_device_table)(void *opaque);
> +void *opaque;
> +struct smbios_device_handler *prev;
> +};
> +static struct smbios_device_handler *device_handlers;
> +
> +void smbios_register_device_table_handler(void (*handle_device_table)
> +(void *opaque),
> + void *opaque)
> +{
> +struct smbios_device_handler *handler = g_malloc(sizeof(*handler));
> +
> +handler->handle_device_table = handle_device_table;
> +handler->opaque = opaque;
> +handler->prev = device_handlers;
> +device_handlers = handler;
> +}
> +
> void smbios_get_tables(uint8_t **tables, size_t *tables_len,
> uint8_t **anchor, size_t *anchor_len)
> {
> @@ -875,6 +894,14 @@ void smbios_get_tables(uint8_t **tables, size_t
> *tables_len,
> }
>
> smbios_build_type_32_table();
> +
> +while (device_handlers) {
> +struct smbios_device_handler *handler = device_handlers;
> +device_handlers = handler->prev;
> +handler->handle_device_table(handler->opaque);
> +g_free(handler);
> +}
> +
> smbios_build_type_127_table();
>
> smbios_validate_table();
> @@ -898,6 +925,71 @@ static void save_opt(const char **dest, QemuOpts *opts,
> const char *name)
> }
> }
>
> +int smbios_table_entry_add(void *data, int size, bool append_zeros)
> +{
> +struct smbios_structure_header *header;
> +struct smbios_table *table; /* legacy mode only */
> +
> +/*
> + * NOTE: standard double '\0' terminator expected, per smbios spec.
> + * (except in legacy mode, where the second '\0' is implicit and
> + * will be inserted by the BIOS).
> + */
> +if (append_zeros) {
> +size += 2;
> +}
> +smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size);
> +header = (struct smbios_structure_header *)(smbios_tables +
> +smbios_tables_len);
> +
> +memcpy(header, data, size);
> +
> +if (test_bit(header->type, have_fields_bitmap)) {
> +error_report("can't load type %d struct, fields already specified!",
> + header->type);
> +exit(1);
> +}
> +set_bit(header->type, have_binfile_bitmap);
> +
> +if (header->type == 4) {
> +smbios_type4_count++;
> +}
> +
> +smbios_tables_len += size;
> +if (size > smbios_table_max) {
> +smbios_table_max = size;
> +}
> +smbios_table_cnt++;
> +
> +/* add a copy of the newly loaded blob to legacy smbios_entries */
> +/* NOTE: This code runs before smbios_set_defaults(), so we don't
> + * yet know which mode (legacy vs. aggregate-table) will be
> + * required. We therefore add the binary blob to both legacy
> + * (smbios_entries) and aggregate (smbios_tables) tables, and
> + * delete the one we don't need from smbios_set_defaults(),
> + * once we know which machine version has been requested.
> + */
> +if (!smbios_entries) {
> +smbios_entries_len = sizeof(uint16_t);
> +smbios_entries = g_malloc0(smbios_entries_len);
> +}
> +if (append_zeros) {
> +size -= 1; /* The BIOS adds the second zero in legacy mode. */
> +}
> +smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> + size + sizeof(*table));
> +table = (struct smbios_table *)(smbios_entries + smbios_entries_len);
> +table->header.type = SMBIOS_TABLE_ENTRY;
> +table->header.length = cpu_to_le16(sizeof(*table) + size);
> +memcpy(table->data, header, size);
> +smbios_entries_len += sizeof(*table) + size;
> +(*(uint16_t
Re: [Qemu-devel] [PATCH 14/15] acpi: Add hooks for adding things to the SSDT table
On Tue, Apr 07, 2015 at 02:51:43PM -0500, miny...@acm.org wrote:
> From: Corey Minyard
>
> This way devices can tie in when then SSDT is built and can add their
> own entries. This didn't seem to fit anyplace else, primarily because it
> required the Aml type, so I added a new file for it.
>
> Signed-off-by: Corey Minyard
Hmm, I don't see patch 15/15 so I don't know how this is used.
Presumably devices register using add_device_ssdt_encoder?
Can't they, instead, add their tables to some list at that point?
It also would seem a bit easier to debug to have devices add their own
tables rather than patch SSDT.
Somewhere near the line
/* Add tables supplied by user (if any) */
would be a good place to stick this.
> ---
> hw/acpi/Makefile.objs| 1 +
> hw/acpi/acpi-hooks.c | 55
>
> hw/i386/acpi-build.c | 3 +++
> include/hw/acpi/acpi-hooks.h | 31 +
> 4 files changed, 90 insertions(+)
> create mode 100644 hw/acpi/acpi-hooks.c
> create mode 100644 include/hw/acpi/acpi-hooks.h
>
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index b9fefa7..f60b12a 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -3,3 +3,4 @@ common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> common-obj-$(CONFIG_ACPI) += acpi_interface.o
> common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
> common-obj-$(CONFIG_ACPI) += aml-build.o
> +common-obj-$(CONFIG_ACPI) += acpi-hooks.o
> diff --git a/hw/acpi/acpi-hooks.c b/hw/acpi/acpi-hooks.c
> new file mode 100644
> index 000..c499208
> --- /dev/null
> +++ b/hw/acpi/acpi-hooks.c
> @@ -0,0 +1,55 @@
> +/*
> + * ACPI hooks for inserting table entries from devices into the SSDT table.
> + *
> + * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> deal
> + * in the Software without restriction, including without limitation the
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include
> +#include
> +
> +struct ssdt_device_encoder {
> +void (*encode)(Aml *ssdt, void *opaque);
> +void *opaque;
> +QSLIST_ENTRY(ssdt_device_encoder) next;
> +};
> +
> +static QSLIST_HEAD(ssdt_device_encoders, ssdt_device_encoder)
> + ssdt_device_encoders = QSLIST_HEAD_INITIALIZER(&ssdt_device_encoders);
> +
> +void
> +add_device_ssdt_encoder(void (*encode)(Aml *ssdt, void *opaque), void
> *opaque)
> +{
> +struct ssdt_device_encoder *e = g_new0(struct ssdt_device_encoder, 1);
> +
> +e->encode = encode;
> +e->opaque = opaque;
> +QSLIST_INSERT_HEAD(&ssdt_device_encoders, e, next);
> +}
> +
> +void
> +call_device_ssdt_encoders(Aml *ssdt)
> +{
> +struct ssdt_device_encoder *e;
> +
> +QSLIST_FOREACH(e, &ssdt_device_encoders, next) {
> +e->encode(ssdt, e->opaque);
> +}
> +}
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e761005..3a4b1ce 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -35,6 +35,7 @@
> #include "hw/timer/hpet.h"
> #include "hw/i386/acpi-defs.h"
> #include "hw/acpi/acpi.h"
> +#include "hw/acpi/acpi-hooks.h"
> #include "hw/nvram/fw_cfg.h"
> #include "hw/acpi/bios-linker-loader.h"
> #include "hw/loader.h"
> @@ -1008,6 +1009,8 @@ build_ssdt(GArray *table_data, GArray *linker,
> aml_append(ssdt, sb_scope);
> }
>
> +call_device_ssdt_encoders(ssdt);
> +
> /* copy AML table into ACPI tables blob and patch header there */
> g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> build_header(linker, table_data,
> diff --git a/include/hw/acpi/acpi-hooks.h b/include/hw/acpi/acpi-hooks.h
> new file mode 100644
> index 000..ae66925
> --- /dev/null
> +++ b/include/hw/acpi/acpi-hooks.h
> @@ -0,0 +1,31 @@
> +/*
> + * Hooks for dynamically construct ACPI tables in devices
> + *
> + * Copyright (C) 2015 Corey Minyard
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of t
Re: [Qemu-devel] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files
On Sun, Apr 12, 2015 at 9:09 AM, Al Viro wrote: > On Sun, Apr 12, 2015 at 12:42:35PM -, Eric Van Hensbergen wrote: > > > In other words, it only uses the open fd to derrive a path and then > > executes the getattr off of that path. If that path no longer exists > > (because of unlink or remove) then you are hosed. In my understanding, > the > > "work around" I suppose is the so-called 'silly renaming' where > > remove/unlink simply does a rename until all open instances are closed. > > What do you mean, "no longer exists"? Don't confuse path with pathname - > it's a mount,dentry pair. And dentry in question bloody well ought to > still > have the FID associated with it - you shouldn't use the same FID for > TREMOVE and for TREAD/TWRITE. Quite right, the fid is still there, I just don't have an easy way to get at it. vfs_file operations have a direct notion of the fid because they cache it in the struct file private data. dentries have several fids associated with them and stored in thier private data, so I've got to "guess" the right one. In most cases this probably won't cause a problem, but it just feels a bit off. There was a thread a few years back where Miklos was arguing for fstat to pass through struct file information since the the fd given the fstat had a file structure associated with it (which in 9p's case has a direct pointer to the "right" fid): http://lwn.net/Articles/251228/ In any case, I've drafted a quick patch which takes the approach of searching the dentry fid list for the fid, but it doesn't feel like the right answer and I'm fairly certain I need to iterate on it a few times to make sure I haven't hosed something up. > TREMOVE clunks the FID passed to it; on > some servers you really have no choice - server discards the file > completely > and on any FID that used to refer to it you get an error from that point > on. > On those you'd really have to do something like sillyrename - the only > way to keep IO going for a file sitting on such server is to have it > visible somewhere. Normal fs(4) is that way; e.g. u9fs(4) isn't - there > FID maps to opened file descriptor on host and TREMOVE on another FID > doesn't break it, as long as host supports IO on opened-but-unlinked files. > I don't remember where qemu 9pfs falls in that respect, but I'd expect it > to be more like u9fs... > > Sort of, the servers in kvmtool and qemu (and diod) have a fid with the open handle. However, all of them seem to implement getattr assuming they have to re-walk to the file. In order to test my aforementioned changes to the client, I also did a quick patch to kvmtool which checks and sees if the fid it receives has an fd and just uses fstat instead of lstat. Patch is here at the moment, I'll send upstream once I'm happy with the client side changes and look into creating a patch for qemu/diod: https://github.com/ericvh/linux-kvm/commit/2fa2f7e728ac08a7d9006516870db1a986aa6acc > Which FID are you passing to server on unlink()? > > Unlink/remove look to be getting a proper fid (in other words, not using the one that is open). The problem is that getattr is using a reference fid (an open fid that's already walked to the name). From a protocol semantics perspective the fixes mentioned above probably don't help that we may have some dangling unopen fids pointing at a name that is no longer valid, but that is just a consequence of the imperfect nature of the mapping of dentries to fids and I'm not sure it does any harm. A trace from the original problem on diod (which appears to not implement unlink and is falling back to remove). diod: P9_TWALK tag 1 fid 1 newfid 2 nwname 1 'foobar' diod: P9_RLERROR tag 1 ecode 2 diod: P9_TWALK tag 1 fid 1 newfid 2 nwname 0 diod: P9_RWALK tag 1 nwqid 0e diod: P9_TLCREATE tag 1 fid 2 name 'foobar' flags 0x8042 mode 0100644 gid 0 diod: P9_RLCREATE tag 1 qid (0012524b 0 '') iounit 0 diod: P9_TWALK tag 1 fid 1 newfid 3 nwname 1 'foobar' diod: P9_RWALK tag 1 nwqid 1 (0012524b 0 '') diod: P9_TGETATTR tag 1 fid 3 request_mask 0x17ff diod: P9_RGETATTR tag 1 valid 0x17ff qid (0012524b 0 '') mode 0100644 uid 0 gid 0 nlink 1 rdev 0 size 0 blksize 4096 blocks 0 atime Mon Apr 6 11:11:08 2015 mtime Mon Apr 6 11:11:08 2015 ctime Mon Apr 6 11:11:08 2015 btime X gen 0 data_version X diod: P9_TUNLINKAT tag 1 dirfid 1 name 'foobar' flags 0 diod: P9_RLERROR tag 1 ecode 95 diod: P9_TWALK tag 1 fid 3 newfid 4 nwname 0 diod: P9_RWALK tag 1 nwqid 0 diod: P9_TREMOVE tag 1 fid 4 diod: P9_RREMOVE tag 1 diod: P9_TGETATTR tag 1 fid 3 request_mask 0x3fff diod: P9_RLERROR tag 1 ecode 2 diod: P9_TCLUNK tag 1 fid 2 diod: P9_RCLUNK tag 1 diod: P9_TCLUNK tag 1 fid 3 diod: P9_RCLUNK tag 1 The client cloning 3 to 4 before the remove seems a bit unnecessary, but is probably done in the case that the remove fails on the server so that we still have a dentry reference.
Re: [Qemu-devel] [PATCH v5 6/6] Qemu-Xen-vTPM: Add a parameter indicating whether the command that was a selftest
On 04/10/2015 02:59 AM, Quan Xu wrote:
and whether it completed successfully. Move tpm_passthrough_is_selftest() into
tpm_util.c and rename it to tpm_util_is_selftest().
Signed-off-by: Quan Xu
---
hw/tpm/Makefile.objs | 2 +-
hw/tpm/tpm_passthrough.c | 13 +--
hw/tpm/tpm_util.c| 50
hw/tpm/tpm_xenstubdoms.c | 36 +++--
include/sysemu/tpm_backend_int.h | 1 +
5 files changed, 82 insertions(+), 20 deletions(-)
create mode 100644 hw/tpm/tpm_util.c
diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
index 190e776..cba961c 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -1,3 +1,3 @@
-common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
+common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o tpm_util.o
common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
common-obj-$(CONFIG_TPM_XENSTUBDOMS) += tpm_xenstubdoms.o xen_vtpm_frontend.o
diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index 2a45071..ff08e15 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -112,17 +112,6 @@ static void tpm_write_fatal_error_response(uint8_t *out,
uint32_t out_len)
}
}
-static bool tpm_passthrough_is_selftest(const uint8_t *in, uint32_t in_len)
-{
-struct tpm_req_hdr *hdr = (struct tpm_req_hdr *)in;
-
-if (in_len >= sizeof(*hdr)) {
-return (be32_to_cpu(hdr->ordinal) == TPM_ORD_ContinueSelfTest);
-}
-
-return false;
-}
-
static int tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
const uint8_t *in, uint32_t in_len,
uint8_t *out, uint32_t out_len,
@@ -136,7 +125,7 @@ static int tpm_passthrough_unix_tx_bufs(TPMPassthruState
*tpm_pt,
tpm_pt->tpm_executing = true;
*selftest_done = false;
-is_selftest = tpm_passthrough_is_selftest(in, in_len);
+is_selftest = tpm_util_is_selftest(in, in_len);
ret = tpm_passthrough_unix_write(tpm_pt->tpm_fd, in, in_len);
if (ret != in_len) {
diff --git a/hw/tpm/tpm_util.c b/hw/tpm/tpm_util.c
new file mode 100644
index 000..8566781
--- /dev/null
+++ b/hw/tpm/tpm_util.c
@@ -0,0 +1,50 @@
+/*
+ * TPM util functions
+ *
+ * * Copyright (c) 2015 Intel Corporation
+ * Authors:
+ *Quan Xu
+ *
+ * Copyright (c) 2010 - 2013 IBM Corporation
+ * Authors:
+ *Stefan Berger
+ *
+ * Copyright (C) 2011 IAIK, Graz University of Technology
+ *Author: Andreas Niederl
I don't think that for this particular function this Copyright applies.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
Re: [Qemu-devel] [PATCH 2/3] tpm: Probe for connected TPM 1.2 or TPM 2
On 04/07/2015 04:54 AM, Xu, Quan wrote:
-Original Message-
From: Stefan Berger [mailto:stef...@linux.vnet.ibm.com]
Sent: Wednesday, April 01, 2015 3:40 AM
To: qemu-devel@nongnu.org; m...@redhat.com
Cc: Xu, Quan; Stefan Berger; Stefan Berger
Subject: [PATCH 2/3] tpm: Probe for connected TPM 1.2 or TPM 2
In the TPM passthrough backend driver, modify the probing code so that we can
check whether a TPM 1.2 or TPM 2 is being used and adapt the behavior of the
TPM TIS accordingly.
Move the code that tested for a TPM 1.2 into tpm_utils.c and extend it with test
for probing for TPM 2. Have the function return the version of TPM found.
Signed-off-by: Stefan Berger
---
hw/tpm/Makefile.objs | 2 +-
hw/tpm/tpm_int.h | 6 +++
hw/tpm/tpm_passthrough.c | 59 +++---
hw/tpm/tpm_util.c| 126
+++
hw/tpm/tpm_util.h| 28 +++
5 files changed, 167 insertions(+), 54 deletions(-) create mode 100644
hw/tpm/tpm_util.c create mode 100644 hw/tpm/tpm_util.h
diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs index
99f5983..64cecc3 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -1,2 +1,2 @@
common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
-common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
+common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
tpm_util.o
diff --git a/hw/tpm/tpm_int.h b/hw/tpm/tpm_int.h index 24e12ce..edab824
100644
--- a/hw/tpm/tpm_int.h
+++ b/hw/tpm/tpm_int.h
@@ -66,4 +66,10 @@ struct tpm_resp_hdr { #define
TPM_ORD_ContinueSelfTest 0x53
#define TPM_ORD_GetTicks 0xf1
+
+/* TPM2 defines */
+#define TPM_ST_NO_SESSIONS0x8001
+
+#define TPM_CC_ReadClock 0x0181
+
Could you define TPM2 macro definitions beginning with 'TPM2_*'?
Ok, will do.
[...]
+/*
+ * Probe for the TPM device in the back
+ * Returns 0 on success with the version of the probed TPM set, 1 on failure.
+ */
+int tpm_util_test_tpmdev(int tpm_fd, enum TPMVersion *tpm_version) {
+/*
+ * Sending a TPM1.2 command to a TPM2 should return a TPM1.2
+ * header (tag = 0xc4) and error code (TPM_BADTAG = 0x1e)
+ *
+ * Sending a TPM2 command to a TPM 2 will give a TPM 2 tag in the
+ * header.
+ * Sending a TPM2 command to a TPM 1.2 will give a TPM 1.2 tag
+ * in the header and an error code.
+ */
+const struct tpm_req_hdr test_req = {
+.tag = cpu_to_be16(TPM_TAG_RQU_COMMAND),
+.len = cpu_to_be32(sizeof(test_req)),
+.ordinal = cpu_to_be32(TPM_ORD_GetTicks),
+};
+
+const struct tpm_req_hdr test_req_tpm2 = {
+.tag = cpu_to_be16(TPM_ST_NO_SESSIONS),
+.len = cpu_to_be32(sizeof(test_req_tpm2)),
+.ordinal = cpu_to_be32(TPM_CC_ReadClock),
+};
+uint16_t returnTag;
+int ret;
+
+/* Send TPM 2 command */
+ret = tpm_util_test(tpm_fd, (unsigned char *)&test_req_tpm2,
+sizeof(test_req_tpm2), &returnTag);
+/* TPM 2 would respond with a tag of TPM_ST_NO_SESSIONS */
+if (!ret && returnTag == TPM_ST_NO_SESSIONS) {
+*tpm_version = TPMVersion2_0;
+return 0;
+}
+
+/* Send TPM 1.2 command */
+ret = tpm_util_test(tpm_fd, (unsigned char *)&test_req,
+sizeof(test_req), &returnTag);
+if (!ret && returnTag == TPM_TAG_RSP_COMMAND) {
+*tpm_version = TPMVersion1_2;
+/* this is a TPM 1.2 */
+return 0;
+}
+
+*tpm_version = TPMVersion_Unspec;
+
+return 1;
+}
In my opinion, I prefer to point out tpm_version in QEMU command line options,
then
tpm_util_test_tpmdev() tries to verify it.
The only reason why I am not doing this was that libvirt for example
will need to probe for whether the additional parameter indicating the
TPM version is supported. Besides that I thought it should be possible
to probe on any platform and get a reliable result.
Maybe Eric has a comment. I have recently seen a discussion where an
additional parameter to an existing option was to be added, but cannot
remember which option that was.
Stefan
[Qemu-devel] [PATCH 2/2] target-i386: kvm: Disable KVM quirks
KVM has quirks to overcome legacy QEMU bugs that are already resolved. Using a
new KVM feature for disabling these quirks.
Signed-off-by: Nadav Amit
---
linux-headers/asm-x86/kvm.h | 4
linux-headers/linux/kvm.h | 1 +
target-i386/kvm.c | 8
3 files changed, 13 insertions(+)
diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h
index d7dcef5..f8fbb4a 100644
--- a/linux-headers/asm-x86/kvm.h
+++ b/linux-headers/asm-x86/kvm.h
@@ -345,4 +345,8 @@ struct kvm_xcrs {
struct kvm_sync_regs {
};
+/* KVM legacy quirks for use with KVM_CAP_DISABLE_QUIRKS */
+#define KVM_QUIRK_LINT0_DISABLED (1 << 0)
+#define KVM_QUIRK_CD_NW_CLEARED(1 << 1)
+
#endif /* _ASM_X86_KVM_H */
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 60a54c8..757e869 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -760,6 +760,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_PPC_ENABLE_HCALL 104
#define KVM_CAP_CHECK_EXTENSION_VM 105
#define KVM_CAP_S390_USER_SIGP 106
+#define KVM_CAP_DISABLE_QUIRKS 115
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 41d09e5..3b28122 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -898,6 +898,14 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
return ret;
}
}
+if (kvm_check_extension(s, KVM_CAP_ENABLE_CAP_VM)) {
+ret = kvm_vm_enable_cap(s, KVM_CAP_DISABLE_QUIRKS, 0,
+KVM_QUIRK_LINT0_DISABLED |
+KVM_QUIRK_CD_NW_CLEARED);
+if (ret < 0) {
+return ret;
+}
+}
return 0;
}
--
1.9.1
[Qemu-devel] [PATCH 0/2] target-i386: disable LINT0 after reset and init
LINT0 is currently reenabled after reset to circumvent old seabios bug, which violates x86 specifications. This patch-set handles this issue, by removing the old hack from qemu and reporting to kvm that this quirk is no longer needed. In addition, we disable another kvm quirk that clears CD and NW from CR0 after reset. Thanks for reviewing these patches. Nadav Amit (2): target-i386: disable LINT0 after reset target-i386: kvm: Disable KVM quirks hw/intc/apic_common.c | 9 - linux-headers/asm-x86/kvm.h | 4 linux-headers/linux/kvm.h | 1 + target-i386/kvm.c | 8 4 files changed, 13 insertions(+), 9 deletions(-) -- 1.9.1
[Qemu-devel] [PATCH 1/2] target-i386: disable LINT0 after reset
Due to old Seabios bug, QEMU reenable LINT0 after reset. This bug is long gone
and therefore this hack is no longer needed. Since it violates the
specifications, it is removed.
Signed-off-by: Nadav Amit
---
hw/intc/apic_common.c | 9 -
1 file changed, 9 deletions(-)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 042e960..d38d24b 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -243,15 +243,6 @@ static void apic_reset_common(DeviceState *dev)
info->vapic_base_update(s);
apic_init_reset(dev);
-
-if (bsp) {
-/*
- * LINT0 delivery mode on CPU #0 is set to ExtInt at initialization
- * time typically by BIOS, so PIC interrupt can be delivered to the
- * processor when local APIC is enabled.
- */
-s->lvt[APIC_LVT_LINT0] = 0x700;
-}
}
/* This function is only used for old state version 1 and 2 */
--
1.9.1
Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry
On 04/12/2015 11:05 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 07, 2015 at 02:51:39PM -0500, miny...@acm.org wrote:
>> From: Corey Minyard
>>
>> There was no way to directly add a table entry to the SMBIOS table,
>> even though the BIOS supports this. So add a function to do this.
>> This is in preparation for the IPMI handler adding it's SMBIOS table
>> entry.
>>
>> Signed-off-by: Corey Minyard
> Do we have to use a callback for this?
>
> It seems that if smbios_table_entry_add just added the table
> itself to some array or a linked list, then devices could just call
> smbios_table_entry_add instead of registering a handler.
>
> And smbios_get_tables would scan that and get the tables.
Well, there are many ways you could handle this, and it doesn't seem
much different in the end to me. You either have a list of items to add
to the table or a list of callbacks to fill in the data.
I made this the same as the ACPI code, which you have to have as a
callback if you are adding it to a common SSDT. Plus it reduced the
amount of dynamic allocation required.
>
> On systems without smbios, this function could be a nop stub.
One nice things about the callbacks is you can eliminate any stubs. I
find stubs kind of ugly, personally.
-corey
>
> Did I miss something?
>
>> ---
>> hw/i386/smbios.c | 149
>> ++-
>> include/hw/i386/smbios.h | 13 +
>> 2 files changed, 110 insertions(+), 52 deletions(-)
>>
>> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
>> index 1341e02..fe15325 100644
>> --- a/hw/i386/smbios.c
>> +++ b/hw/i386/smbios.c
>> @@ -831,6 +831,25 @@ static void smbios_entry_point_setup(void)
>> ep.structure_table_address = cpu_to_le32(0);
>> }
>>
>> +struct smbios_device_handler {
>> +void (*handle_device_table)(void *opaque);
>> +void *opaque;
>> +struct smbios_device_handler *prev;
>> +};
>> +static struct smbios_device_handler *device_handlers;
>> +
>> +void smbios_register_device_table_handler(void (*handle_device_table)
>> +(void *opaque),
>> + void *opaque)
>> +{
>> +struct smbios_device_handler *handler = g_malloc(sizeof(*handler));
>> +
>> +handler->handle_device_table = handle_device_table;
>> +handler->opaque = opaque;
>> +handler->prev = device_handlers;
>> +device_handlers = handler;
>> +}
>> +
>> void smbios_get_tables(uint8_t **tables, size_t *tables_len,
>> uint8_t **anchor, size_t *anchor_len)
>> {
>> @@ -875,6 +894,14 @@ void smbios_get_tables(uint8_t **tables, size_t
>> *tables_len,
>> }
>>
>> smbios_build_type_32_table();
>> +
>> +while (device_handlers) {
>> +struct smbios_device_handler *handler = device_handlers;
>> +device_handlers = handler->prev;
>> +handler->handle_device_table(handler->opaque);
>> +g_free(handler);
>> +}
>> +
>> smbios_build_type_127_table();
>>
>> smbios_validate_table();
>> @@ -898,6 +925,71 @@ static void save_opt(const char **dest, QemuOpts *opts,
>> const char *name)
>> }
>> }
>>
>> +int smbios_table_entry_add(void *data, int size, bool append_zeros)
>> +{
>> +struct smbios_structure_header *header;
>> +struct smbios_table *table; /* legacy mode only */
>> +
>> +/*
>> + * NOTE: standard double '\0' terminator expected, per smbios spec.
>> + * (except in legacy mode, where the second '\0' is implicit and
>> + * will be inserted by the BIOS).
>> + */
>> +if (append_zeros) {
>> +size += 2;
>> +}
>> +smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size);
>> +header = (struct smbios_structure_header *)(smbios_tables +
>> +smbios_tables_len);
>> +
>> +memcpy(header, data, size);
>> +
>> +if (test_bit(header->type, have_fields_bitmap)) {
>> +error_report("can't load type %d struct, fields already specified!",
>> + header->type);
>> +exit(1);
>> +}
>> +set_bit(header->type, have_binfile_bitmap);
>> +
>> +if (header->type == 4) {
>> +smbios_type4_count++;
>> +}
>> +
>> +smbios_tables_len += size;
>> +if (size > smbios_table_max) {
>> +smbios_table_max = size;
>> +}
>> +smbios_table_cnt++;
>> +
>> +/* add a copy of the newly loaded blob to legacy smbios_entries */
>> +/* NOTE: This code runs before smbios_set_defaults(), so we don't
>> + * yet know which mode (legacy vs. aggregate-table) will be
>> + * required. We therefore add the binary blob to both legacy
>> + * (smbios_entries) and aggregate (smbios_tables) tables, and
>> + * delete the one we don't need from smbios_set_defaults(),
>> + * once we know which machine version has been requested.
>> + */
>> +if (!smbios
Re: [Qemu-devel] [PATCH 14/15] acpi: Add hooks for adding things to the SSDT table
On 04/12/2015 11:17 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 07, 2015 at 02:51:43PM -0500, miny...@acm.org wrote:
>> From: Corey Minyard
>>
>> This way devices can tie in when then SSDT is built and can add their
>> own entries. This didn't seem to fit anyplace else, primarily because it
>> required the Aml type, so I added a new file for it.
>>
>> Signed-off-by: Corey Minyard
> Hmm, I don't see patch 15/15 so I don't know how this is used.
Yeah, I resent and I don't know what's happened. All the others were
sent exactly the same way. Something doesn't like that patch.
>
> Presumably devices register using add_device_ssdt_encoder?
> Can't they, instead, add their tables to some list at that point?
Not if they are adding to a common SSDT.
>
> It also would seem a bit easier to debug to have devices add their own
> tables rather than patch SSDT.
> Somewhere near the line
> /* Add tables supplied by user (if any) */
> would be a good place to stick this.
So you are suggesting each device add it's own SSDT? I'm not sure how
that helps debugging, it seems simpler to add it to a common one. But
it's not a big deal either way.
-corey
>
>
>> ---
>> hw/acpi/Makefile.objs| 1 +
>> hw/acpi/acpi-hooks.c | 55
>>
>> hw/i386/acpi-build.c | 3 +++
>> include/hw/acpi/acpi-hooks.h | 31 +
>> 4 files changed, 90 insertions(+)
>> create mode 100644 hw/acpi/acpi-hooks.c
>> create mode 100644 include/hw/acpi/acpi-hooks.h
>>
>> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
>> index b9fefa7..f60b12a 100644
>> --- a/hw/acpi/Makefile.objs
>> +++ b/hw/acpi/Makefile.objs
>> @@ -3,3 +3,4 @@ common-obj-$(CONFIG_ACPI) += memory_hotplug.o
>> common-obj-$(CONFIG_ACPI) += acpi_interface.o
>> common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
>> common-obj-$(CONFIG_ACPI) += aml-build.o
>> +common-obj-$(CONFIG_ACPI) += acpi-hooks.o
>> diff --git a/hw/acpi/acpi-hooks.c b/hw/acpi/acpi-hooks.c
>> new file mode 100644
>> index 000..c499208
>> --- /dev/null
>> +++ b/hw/acpi/acpi-hooks.c
>> @@ -0,0 +1,55 @@
>> +/*
>> + * ACPI hooks for inserting table entries from devices into the SSDT table.
>> + *
>> + * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> deal
>> + * in the Software without restriction, including without limitation the
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include
>> +#include
>> +
>> +struct ssdt_device_encoder {
>> +void (*encode)(Aml *ssdt, void *opaque);
>> +void *opaque;
>> +QSLIST_ENTRY(ssdt_device_encoder) next;
>> +};
>> +
>> +static QSLIST_HEAD(ssdt_device_encoders, ssdt_device_encoder)
>> + ssdt_device_encoders = QSLIST_HEAD_INITIALIZER(&ssdt_device_encoders);
>> +
>> +void
>> +add_device_ssdt_encoder(void (*encode)(Aml *ssdt, void *opaque), void
>> *opaque)
>> +{
>> +struct ssdt_device_encoder *e = g_new0(struct ssdt_device_encoder, 1);
>> +
>> +e->encode = encode;
>> +e->opaque = opaque;
>> +QSLIST_INSERT_HEAD(&ssdt_device_encoders, e, next);
>> +}
>> +
>> +void
>> +call_device_ssdt_encoders(Aml *ssdt)
>> +{
>> +struct ssdt_device_encoder *e;
>> +
>> +QSLIST_FOREACH(e, &ssdt_device_encoders, next) {
>> +e->encode(ssdt, e->opaque);
>> +}
>> +}
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index e761005..3a4b1ce 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -35,6 +35,7 @@
>> #include "hw/timer/hpet.h"
>> #include "hw/i386/acpi-defs.h"
>> #include "hw/acpi/acpi.h"
>> +#include "hw/acpi/acpi-hooks.h"
>> #include "hw/nvram/fw_cfg.h"
>> #include "hw/acpi/bios-linker-loader.h"
>> #include "hw/loader.h"
>> @@ -1008,6 +1009,8 @@ build_ssdt(GArray *table_data, GArray *linker,
>> aml_append(ssdt, sb_scope);
>> }
>>
>> +call_device_ssdt_encoders(ssdt);
>> +
>> /* copy AML table into ACPI tables blob and p
Re: [Qemu-devel] Very poor IO performance which looks like some design problem.
On Fri, 04/10 22:38, ein wrote: > Qemu creates more than 70 threads and everyone of them tries to write to > disk, which results in: > 1. High I/O time. > 2. Large latency. > 2. Poor sequential read/write speeds. > > When I limited number of cores, I guess I limited number of threads as > well. That's why I got better numbers. > > I've tried to combine AIO native and thread setting with deadline > scheduler. Native AIO was much more worse. > > The final question, is there any way to prevent Qemu for making so large > number of processes when VM does only one sequential R/W operation? aio=native will make QEMU only submit IO from the IO thread, so you shouldn't see 70 threads with that. And that should usually be a better option for performance. Fam
Re: [Qemu-devel] [PATCH v6 8/8] qmp-event: add event notification for memory hot unplug error
On 04/10/2015 11:37 PM, Eric Blake wrote:
On 04/02/2015 03:50 AM, Zhu Guihua wrote:
When memory hot unplug fails, this patch adds support to send
QMP event to notify mgmt about this failure.
Signed-off-by: Zhu Guihua
---
docs/qmp/qmp-events.txt | 17 +
hw/acpi/memory_hotplug.c | 10 +-
monitor.c| 1 +
qapi/event.json | 14 ++
trace-events | 1 +
5 files changed, 42 insertions(+), 1 deletion(-)
+++ b/monitor.c
@@ -587,6 +587,7 @@ static void monitor_qapi_event_init(void)
monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
+monitor_qapi_event_throttle(QAPI_EVENT_MEM_UNPLUG_ERROR, 1000);
Is this event triggerable under guest control, or only under management
control? We MUST throttle events that the guest can trigger, but it
makes no sense to trigger something that can only happen under host control.
It's under management control, this event should not be throttled, it's
my mistake
and I will delete this. Thanks.
+++ b/qapi/event.json
@@ -330,3 +330,17 @@
##
{ 'event': 'VSERPORT_CHANGE',
'data': { 'id': 'str', 'open': 'bool' } }
+
+##
+# @MEM_UNPLUG_ERROR
+#
+# Emitted when memory hot unplug error occurs.
+#
+# @device: device name
+#
+# @msg: Informative message
Any reason you abbreviated instead of spelling it out as 'message'?
This only refer to the spelling of event BLOCK_IMAGE_CORRUPTED
in docs/qmp/qmp-events.txt.
Thanks,
Zhu
Re: [Qemu-devel] [PATCH v5 6/6] Qemu-Xen-vTPM: Add a parameter indicating whether the command that was a selftest
> -Original Message-
> From: Stefan Berger [mailto:stef...@linux.vnet.ibm.com]
> Sent: Monday, April 13, 2015 4:50 AM
> To: Xu, Quan; stefano.stabell...@eu.citrix.com; ebl...@redhat.com
> Cc: pbonz...@redhat.com; qemu-devel@nongnu.org; aligu...@amazon.com;
> wei.l...@citrix.com; dgde...@tycho.nsa.gov; xen-de...@lists.xen.org
> Subject: Re: [PATCH v5 6/6] Qemu-Xen-vTPM: Add a parameter indicating
> whether the command that was a selftest
>
> On 04/10/2015 02:59 AM, Quan Xu wrote:
> > and whether it completed successfully. Move
> > tpm_passthrough_is_selftest() into tpm_util.c and rename it to
> tpm_util_is_selftest().
> >
> > Signed-off-by: Quan Xu
> > ---
> > hw/tpm/Makefile.objs | 2 +-
> > hw/tpm/tpm_passthrough.c | 13 +--
> > hw/tpm/tpm_util.c| 50
>
> > hw/tpm/tpm_xenstubdoms.c | 36
> +++--
> > include/sysemu/tpm_backend_int.h | 1 +
> > 5 files changed, 82 insertions(+), 20 deletions(-)
> > create mode 100644 hw/tpm/tpm_util.c
> >
> > diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs index
> > 190e776..cba961c 100644
> > --- a/hw/tpm/Makefile.objs
> > +++ b/hw/tpm/Makefile.objs
> > @@ -1,3 +1,3 @@
> > -common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
> > +common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o tpm_util.o
> > common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
> > common-obj-$(CONFIG_TPM_XENSTUBDOMS) += tpm_xenstubdoms.o
> > xen_vtpm_frontend.o diff --git a/hw/tpm/tpm_passthrough.c
> > b/hw/tpm/tpm_passthrough.c index 2a45071..ff08e15 100644
> > --- a/hw/tpm/tpm_passthrough.c
> > +++ b/hw/tpm/tpm_passthrough.c
> > @@ -112,17 +112,6 @@ static void tpm_write_fatal_error_response(uint8_t
> *out, uint32_t out_len)
> > }
> > }
> >
> > -static bool tpm_passthrough_is_selftest(const uint8_t *in, uint32_t
> > in_len) -{
> > -struct tpm_req_hdr *hdr = (struct tpm_req_hdr *)in;
> > -
> > -if (in_len >= sizeof(*hdr)) {
> > -return (be32_to_cpu(hdr->ordinal) == TPM_ORD_ContinueSelfTest);
> > -}
> > -
> > -return false;
> > -}
> > -
> > static int tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
> > const uint8_t *in, uint32_t
> in_len,
> > uint8_t *out, uint32_t
> > out_len, @@ -136,7 +125,7 @@ static int
> tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
> > tpm_pt->tpm_executing = true;
> > *selftest_done = false;
> >
> > -is_selftest = tpm_passthrough_is_selftest(in, in_len);
> > +is_selftest = tpm_util_is_selftest(in, in_len);
> >
> > ret = tpm_passthrough_unix_write(tpm_pt->tpm_fd, in, in_len);
> > if (ret != in_len) {
> > diff --git a/hw/tpm/tpm_util.c b/hw/tpm/tpm_util.c new file mode
> > 100644 index 000..8566781
> > --- /dev/null
> > +++ b/hw/tpm/tpm_util.c
> > @@ -0,0 +1,50 @@
> > +/*
> > + * TPM util functions
> > + *
> > + * * Copyright (c) 2015 Intel Corporation
> > + * Authors:
> > + *Quan Xu
> > + *
> > + * Copyright (c) 2010 - 2013 IBM Corporation
> > + * Authors:
> > + *Stefan Berger
> > + *
> > + * Copyright (C) 2011 IAIK, Graz University of Technology
> > + *Author: Andreas Niederl
>
> I don't think that for this particular function this Copyright applies.
Thanks Stefan.
Could I just delete "Copyright (C) 2011 IAIK.."? share me more specific
copyright, if I am not correct.
>
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see
> > +
Re: [Qemu-devel] [PATCH v5 1/6] Qemu-Xen-vTPM: Support for Xen stubdom vTPM command line options
> -Original Message-
> From: Eric Blake [mailto:ebl...@redhat.com]
> Sent: Friday, April 10, 2015 9:22 PM
> To: Xu, Quan; stefano.stabell...@eu.citrix.com; stef...@linux.vnet.ibm.com
> Cc: pbonz...@redhat.com; qemu-devel@nongnu.org; aligu...@amazon.com;
> wei.l...@citrix.com; dgde...@tycho.nsa.gov; xen-de...@lists.xen.org
> Subject: Re: [PATCH v5 1/6] Qemu-Xen-vTPM: Support for Xen stubdom vTPM
> command line options
>
> On 04/10/2015 12:59 AM, Quan Xu wrote:
> > Signed-off-by: Quan Xu
> >
> > --Changes in v5:
> > -qapi schema enhancement.
> > ---
> > configure| 14 ++
> > hmp.c| 2 ++
> > qapi-schema.json | 17 +++-- qemu-options.hx | 13
> > +++--
> > tpm.c| 7 ++-
> > 5 files changed, 48 insertions(+), 5 deletions(-)
> >
>
> > +++ b/qapi-schema.json
> > @@ -2975,9 +2975,11 @@
> > #
> > # @passthrough: TPM passthrough type
> > #
> > +# @xenstubdoms: TPM xenstubdoms type (since 2.4) #
> > # Since: 1.5
> > ##
> > -{ 'enum': 'TpmType', 'data': [ 'passthrough' ] }
> > +{ 'enum': 'TpmType', 'data': [ 'passthrough', 'xenstubdoms' ] }
> >
> > ##
> > # @query-tpm-types:
> > @@ -3006,6 +3008,15 @@
> > '*cancel-path' : 'str'}
> > }
> >
> > ##
> > +# @TPMXenstubdomsOptions:
> > +#
> > +# Information about the TPM xenstubdoms type # # Since: 2.4 ## {
> > +'type': 'TPMXenstubdomsOptions', 'data': { } }
> > +
> > +##
> > # @TpmTypeOptions:
> > #
> > # A union referencing different TPM backend types' configuration
> > options @@ -3015,7 +3026,9 @@ # Since: 1.5 ## { 'union':
> > 'TpmTypeOptions',
> > - 'data': { 'passthrough' : 'TPMPassthroughOptions' } }
> > + 'data': { 'passthrough' : 'TPMPassthroughOptions',
> > +'xenstubdoms' : 'TPMXenstubdomsOptions' } }
>
> Hmm, maybe it is time for me to follow through with my threat of allowing
> anonymous types in union branches.
>
> > +##
>
> Stray insertion. Please remove it.
Thanks Eric..
I will send out the next version soon.
Quan
>
> >
> > ##
> > # @TpmInfo:
> > diff --git a/qemu-options.hx b/qemu-options.hx index 319d971..9254902
> > 100644
>
>
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [RFC PATCH v2 21/23] spapr: Initialize hotplug memory address space
On Wed, Mar 25, 2015 at 04:58:18PM +1100, David Gibson wrote:
> On Mon, Mar 23, 2015 at 07:06:02PM +0530, Bharata B Rao wrote:
> > Initialize a hotplug memory region under which all the hotplugged
> > memory is accommodated. Also enable memory hotplug by setting
> > CONFIG_MEM_HOTPLUG.
> >
> > Modelled on i386 memory hotplug.
> >
> > Signed-off-by: Bharata B Rao
> > ---
> > default-configs/ppc64-softmmu.mak | 1 +
> > hw/ppc/spapr.c| 50
> > +++
> > include/hw/ppc/spapr.h| 12 ++
> > 3 files changed, 63 insertions(+)
> >
> > diff --git a/default-configs/ppc64-softmmu.mak
> > b/default-configs/ppc64-softmmu.mak
> > index 22ef132..16b3011 100644
> > --- a/default-configs/ppc64-softmmu.mak
> > +++ b/default-configs/ppc64-softmmu.mak
> > @@ -51,3 +51,4 @@ CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
> > # For PReP
> > CONFIG_MC146818RTC=y
> > CONFIG_ISA_TESTDEV=y
> > +CONFIG_MEM_HOTPLUG=y
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 3e56d9e..e43bb49 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -125,8 +125,13 @@ struct sPAPRMachineState {
> >
> > /*< public >*/
> > char *kvm_type;
> > +ram_addr_t hotplug_memory_base;
> > +MemoryRegion hotplug_memory;
> > +bool enforce_aligned_dimm;
> > };
> >
> > +#define SPAPR_MACHINE_ENFORCE_ALIGNED_DIMM "enforce-aligned-dimm"
>
> What's the rationale for including this option?
I couldn't see any use, but added it to be similar with x86. May be Igor
can tell us ?
Regards,
Bharata.
Re: [Qemu-devel] [PATCH] Fix crash with illegal "-net nic, model=xxx" option
On Thu, Apr 9, 2015 at 9:32 PM, Thomas Huth wrote:
Current QEMU crashes when specifying an illegal model with the
"-net nic,model=xxx" option, e.g.:
$ qemu-system-x86_64 -net nic,model=n/a
qemu-system-x86_64: Unsupported NIC model: n/a
Program received signal SIGSEGV, Segmentation fault.
The gdb backtrace looks like this:
0x55965fe0 in error_get_pretty (err=0x0) at util/error.c:152
152 return err->msg;
(gdb) bt
0 0x55965fe0 in error_get_pretty (err=0x0) at
util/error.c:152
1 0x55965ffd in error_report_err (err=0x0) at
util/error.c:157
2 0x55809c90 in pci_nic_init_nofail (nd=0x55e49860
, rootbus=0x564409b0,
default_model=0x5598c37b "e1000", default_devaddr=0x0) at
hw/pci/pci.c:1663
3 0x55691e42 in pc_nic_init (isa_bus=0x56f71900,
pci_bus=0x564409b0)
at hw/i386/pc.c:1506
4 0x5569396b in pc_init1 (machine=0x562abbf0,
pci_enabled=1, kvmclock_enabled=1)
at hw/i386/pc_piix.c:248
5 0x55693d27 in pc_init_pci (machine=0x562abbf0) at
hw/i386/pc_piix.c:310
6 0x5572ddf5 in main (argc=3, argv=0x7fffe018,
envp=0x7fffe038) at vl.c:4226
The problem is that pci_nic_init_nofail() does not check whether the
err
parameter from pci_nic_init has been set up and thus passes a NULL
pointer
to error_report_err(). Fix it by correctly checking the err parameter.
Signed-off-by: Thomas Huth
---
hw/pci/pci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 6941a82..b3d5100 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1660,7 +1660,9 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd,
PCIBus *rootbus,
res = pci_nic_init(nd, rootbus, default_model, default_devaddr,
&err);
if (!res) {
-error_report_err(err);
+if (err) {
+error_report_err(err);
+}
exit(1);
}
return res;
--
1.8.3.1
Reviewed-by: Jason Wang
Re: [Qemu-devel] [RFC PATCH v2 23/23] spapr: Memory hotplug support
On Thu, Mar 26, 2015 at 02:57:45PM +1100, David Gibson wrote:
> On Mon, Mar 23, 2015 at 07:06:04PM +0530, Bharata B Rao wrote:
> > Make use of pc-dimm infrastructure to support memory hotplug
> > for PowerPC.
> >
> > Modelled on i386 memory hotplug.
> >
> > Signed-off-by: Bharata B Rao
> > ---
> > hw/ppc/spapr.c| 119
> > +-
> > hw/ppc/spapr_events.c | 3 ++
> > 2 files changed, 120 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 4e844ab..bc46acd 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -61,7 +61,8 @@
> > #include "hw/nmi.h"
> >
> > #include "hw/compat.h"
> > -
> > +#include "hw/mem/pc-dimm.h"
> > +#include "qapi/qmp/qerror.h"
> > #include
> >
> > /* SLOF memory layout:
> > @@ -902,6 +903,10 @@ int spapr_h_cas_compose_response(target_ulong addr,
> > target_ulong size,
> > _FDT((spapr_populate_memory(spapr, fdt)));
> > }
> >
> > +if (spapr->dr_lmb_enabled) {
> > +_FDT(spapr_drc_populate_dt(fdt, 0, NULL,
> > SPAPR_DR_CONNECTOR_TYPE_LMB));
> > +}
> > +
> > /* Pack resulting tree */
> > _FDT((fdt_pack(fdt)));
> >
> > @@ -2185,6 +2190,109 @@ static void spapr_cpu_socket_unplug(HotplugHandler
> > *hotplug_dev,
> > object_child_foreach(OBJECT(dev), spapr_cpu_core_unplug, errp);
> > }
> >
> > +static void spapr_add_lmbs(uint64_t addr, uint64_t size, Error **errp)
> > +{
> > +sPAPRDRConnector *drc;
> > +uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> > +int i;
> > +
> > +if (size % SPAPR_MEMORY_BLOCK_SIZE) {
> > +error_setg(errp, "Hotplugged memory size must be a multiple of "
> > + "%d MB", SPAPR_MEMORY_BLOCK_SIZE/(1024 * 1024));
>
> s/MB/MiB/ there seems to be precedent for using the mebibyte term in
> qemu.
>
> Also you can use the MiB #define instead of (1024 * 1024).
>
> > +return;
> > +}
> > +
> > +for (i = 0; i < nr_lmbs; i++) {
> > +drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> > +addr/SPAPR_MEMORY_BLOCK_SIZE);
> > +g_assert(drc);
> > +spapr_hotplug_req_add_event(drc);
> > +addr += SPAPR_MEMORY_BLOCK_SIZE;
> > +}
> > +}
> > +
> > +static void spapr_memory_plug(HotplugHandler *hotplug_dev,
> > + DeviceState *dev, Error **errp)
> > +{
> > +int slot;
> > +Error *local_err = NULL;
> > +sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> > +MachineState *machine = MACHINE(hotplug_dev);
> > +PCDIMMDevice *dimm = PC_DIMM(dev);
> > +PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > +MemoryRegion *mr = ddc->get_memory_region(dimm);
> > +uint64_t existing_dimms_capacity = 0;
> > +uint64_t align = TARGET_PAGE_SIZE;
> > +uint64_t addr;
> > +
> > +addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> > &local_err);
> > +if (local_err) {
> > +goto out;
> > +}
> > +
> > +if (memory_region_get_alignment(mr) && ms->enforce_aligned_dimm) {
> > +align = memory_region_get_alignment(mr);
> > +}
> > +
> > +addr = pc_dimm_get_free_addr(ms->hotplug_memory_base,
> > + memory_region_size(&ms->hotplug_memory),
> > + !addr ? NULL : &addr, align,
> > + memory_region_size(mr), &local_err);
> > +if (local_err) {
> > +goto out;
> > +}
> > +
> > +existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> > +if (local_err) {
> > +goto out;
> > +}
> > +
> > +if (existing_dimms_capacity + memory_region_size(mr) >
> > +machine->maxram_size - machine->ram_size) {
> > +error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> > + " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> > + existing_dimms_capacity,
> > + machine->maxram_size - machine->ram_size);
> > +goto out;
> > +}
> > +
> > +object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP,
> > &local_err);
> > +if (local_err) {
> > +goto out;
> > +}
> > +trace_mhp_pc_dimm_assigned_address(addr);
> > +
> > +slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> > &local_err);
> > +if (local_err) {
> > +goto out;
> > +}
> > +
> > +slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL :
> > &slot,
> > + machine->ram_slots, &local_err);
> > +if (local_err) {
> > +goto out;
> > +}
> > +object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP,
> > &local_err);
> > +if (local_err) {
> > +goto out;
> > +}
> > +trace_mhp_pc_dimm_assigned_slot(slot);
> > +
> > +if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> > +error_setg(&local_err, "hypervisor has n
Re: [Qemu-devel] How address_space_rw works?
> If that's the case, you could also add your check to > memory_region_section_get_iotlb. Search for PHYS_SECTION_WATCH, > watch_mem_ops and io_mem_watch, and do the same for your new special > case. This is where QEMU decides between using the slow path or the > fast path. > > However this will not catch instruction fetches. How to do that depends > on the details of what you are doing. In particular, if you need to > trap on _all_ instruction fetches and not just the first, it's likely > that QEMU is not the best project to base your changes on. A simulator > would be more appropriate. > Hi, Paolo. I need more time to review and debug relevant code. It will be a time-consuming process. I appreciate your suggestions. Thank you very much. -Kaiyuan Liang
Re: [Qemu-devel] [PATCH 3/3] TPM2 ACPI table support
> @@ -1428,12 +1444,22 @@ void acpi_build(PcGuestInfo *guest_info,
> AcpiBuildTables *tables)
> acpi_add_table(table_offsets, tables_blob);
> build_hpet(tables_blob, tables->linker);
> }
> -if (misc.has_tpm) {
> +if (misc.tpm_version != TPMVersion_Unspec) {
> acpi_add_table(table_offsets, tables_blob);
> build_tpm_tcpa(tables_blob, tables->linker, tables->tcpalog);
>
> acpi_add_table(table_offsets, tables_blob);
> -build_tpm_ssdt(tables_blob, tables->linker);
> +switch (misc.tpm_version) {
> +case TPMVersion1_2:
> +build_tpm_ssdt(tables_blob, tables->linker);
> +break;
> +case TPMVersion2_0:
> +build_tpm2(tables_blob, tables->linker);
> +break;
> +case TPMVersion_Unspec:
> +/* not possible */
> +break;
> +}
> }
> if (guest_info->numa_nodes) {
> acpi_add_table(table_offsets, tables_blob);
I think we should fix QEMU coding style violations in TPM code.
In particular, mixing of _ and camel case, enum and define values
not all upper case, local variables not all lower case.
This was less of an issue when the violations were contained
within hw/tpm/, but it's more of an issue now that this affects
other code.
--
MST
Re: [Qemu-devel] [PATCH 14/15] acpi: Add hooks for adding things to the SSDT table
On Sun, Apr 12, 2015 at 08:30:45PM -0500, Corey Minyard wrote:
> On 04/12/2015 11:17 AM, Michael S. Tsirkin wrote:
> > On Tue, Apr 07, 2015 at 02:51:43PM -0500, miny...@acm.org wrote:
> >> From: Corey Minyard
> >>
> >> This way devices can tie in when then SSDT is built and can add their
> >> own entries. This didn't seem to fit anyplace else, primarily because it
> >> required the Aml type, so I added a new file for it.
> >>
> >> Signed-off-by: Corey Minyard
> > Hmm, I don't see patch 15/15 so I don't know how this is used.
>
> Yeah, I resent and I don't know what's happened. All the others were
> sent exactly the same way. Something doesn't like that patch.
>
> >
> > Presumably devices register using add_device_ssdt_encoder?
> > Can't they, instead, add their tables to some list at that point?
>
> Not if they are adding to a common SSDT.
Why not? Just walk the list after building the rest of the SSDT.
> >
> > It also would seem a bit easier to debug to have devices add their own
> > tables rather than patch SSDT.
> > Somewhere near the line
> > /* Add tables supplied by user (if any) */
> > would be a good place to stick this.
>
> So you are suggesting each device add it's own SSDT? I'm not sure how
> that helps debugging, it seems simpler to add it to a common one.
For example, it's easier to write a unit test: encode something specific
in one of the IDs, then look it up.
Which is, btw, something that's missing in this patchset.
> But
> it's not a big deal either way.
>
> -corey
Yes, there isn't a lot of difference really. Seems slightly neater.
> >
> >
> >> ---
> >> hw/acpi/Makefile.objs| 1 +
> >> hw/acpi/acpi-hooks.c | 55
> >>
> >> hw/i386/acpi-build.c | 3 +++
> >> include/hw/acpi/acpi-hooks.h | 31 +
> >> 4 files changed, 90 insertions(+)
> >> create mode 100644 hw/acpi/acpi-hooks.c
> >> create mode 100644 include/hw/acpi/acpi-hooks.h
> >>
> >> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> >> index b9fefa7..f60b12a 100644
> >> --- a/hw/acpi/Makefile.objs
> >> +++ b/hw/acpi/Makefile.objs
> >> @@ -3,3 +3,4 @@ common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> >> common-obj-$(CONFIG_ACPI) += acpi_interface.o
> >> common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
> >> common-obj-$(CONFIG_ACPI) += aml-build.o
> >> +common-obj-$(CONFIG_ACPI) += acpi-hooks.o
> >> diff --git a/hw/acpi/acpi-hooks.c b/hw/acpi/acpi-hooks.c
> >> new file mode 100644
> >> index 000..c499208
> >> --- /dev/null
> >> +++ b/hw/acpi/acpi-hooks.c
> >> @@ -0,0 +1,55 @@
> >> +/*
> >> + * ACPI hooks for inserting table entries from devices into the SSDT
> >> table.
> >> + *
> >> + * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining
> >> a copy
> >> + * of this software and associated documentation files (the "Software"),
> >> to deal
> >> + * in the Software without restriction, including without limitation the
> >> rights
> >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> >> sell
> >> + * copies of the Software, and to permit persons to whom the Software is
> >> + * furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice shall be
> >> included in
> >> + * all copies or substantial portions of the Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> >> EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> >> MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> >> OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> >> ARISING FROM,
> >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> >> IN
> >> + * THE SOFTWARE.
> >> + */
> >> +
> >> +#include
> >> +#include
> >> +
> >> +struct ssdt_device_encoder {
> >> +void (*encode)(Aml *ssdt, void *opaque);
> >> +void *opaque;
> >> +QSLIST_ENTRY(ssdt_device_encoder) next;
> >> +};
> >> +
> >> +static QSLIST_HEAD(ssdt_device_encoders, ssdt_device_encoder)
> >> + ssdt_device_encoders =
> >> QSLIST_HEAD_INITIALIZER(&ssdt_device_encoders);
> >> +
> >> +void
> >> +add_device_ssdt_encoder(void (*encode)(Aml *ssdt, void *opaque), void
> >> *opaque)
> >> +{
> >> +struct ssdt_device_encoder *e = g_new0(struct ssdt_device_encoder, 1);
> >> +
> >> +e->encode = encode;
> >> +e->opaque = opaque;
> >> +QSLIST_INSERT_HEAD(&ssdt_device_encoders, e, next);
> >> +}
> >> +
> >> +void
> >> +call_device_ssdt_encoders(Aml *ssdt)
> >> +{
> >> +struct ssdt_device_encoder *e;
> >> +
> >> +QSLIST_FOREACH(e, &ssdt_device_encoders, next) {
> >> +e->encode(ssdt, e->opaque);
> >> +}
>
