Re: [Qemu-devel] [PATCH v3] qga: add guest-set-user-password command
On Wed, Feb 11, 2015 at 11:26:12AM +, Daniel P. Berrange wrote:
> Add a new 'guest-set-user-password' command for changing the password
> of guest OS user accounts. This command is needed to enable OpenStack
> to support its API for changing the admin password of guests running
> on KVM/QEMU. It is not practical to provide a command at the QEMU
> level explicitly targetting administrator account password change
> only, since different guest OS have different names for the admin
> account. While UNIX systems use 'root', Windows systems typically
> use 'Administrator' and even that can be renamed. Higher level apps
> like OpenStack have the ability to figure out the correct admin
> account name since they have info that QEMU/libvirt do not.
>
> The command accepts either the clear text password string, encoded
> in base64 to make it 8-bit safe in JSON:
>
> $ echo -n "123456" | base64
> MTIzNDU2
> $ virsh -c qemu:///system qemu-agent-command f21x86_64 \
>'{ "execute": "guest-set-user-password",
> "arguments": { "crypted": false,
> "username": "root",
> "password": "MTIzNDU2" } }'
> {"return":{}}
>
> Or a password that has already been run though a crypt(3) like
> algorithm appropriate for the guest, again then base64 encoded:
>
> $ echo -n '$6$n01A2Tau$e...snip...DfMOP7of9AJ1I8q0' | base64
> JDYkb...snip...YT2Ey
> $ virsh -c qemu:///system qemu-agent-command f21x86_64 \
>'{ "execute": "guest-set-user-password",
> "arguments": { "crypted": true,
> "username": "root",
> "password": "JDYkb...snip...YT2Ey" } }'
>
So how would it look like for user "Daniel P. Berrangé" (with accent
aigu :), or "Роман Каган" (my name in cyrillic letters)? What I'm
trying to say is that if we assume localized usernames we may have
trouble with character encoding.
> +passwd_path = g_find_program_in_path("chpasswd");
> +
> +if (!passwd_path) {
> +error_setg(errp, "cannot find 'passwd' program in PATH");
Minor nitpick: s/passwd/chpasswd/
The patch looks technically correct; however I'm not sold on the
approach, which assumes a responsibility split between the upper
management layer, which is supposed to know the guest OS, the username,
the encryption scheme, and the guest agent which takes care of the
os-specific command to actually change the password. I still tend to
like more the scheme with the management layer having all the necessary
information, including the command to change the password and the proper
way to call it, and using guest-exec to perform it.
IMO the question is how low the bar is to extend the qga protocol with
yet another general-purpose (i.e. not virtual machine-specific) OS
management task. I'd rather see it out of scope for qga. Instead, such
an upper management layer, if necessary, would bring its own agent, with
qga acting as a transport. This way e.g. OpenStack would be able to
uniformly change admin passwords also in ESXi, Parallels Server, LXC,
OpenVz, physical servers, etc.
Roman.
Re: [Qemu-devel] [PATCH v2] qga: add guest-set-admin-password command
On Mon, Jan 12, 2015 at 03:58:14PM +, Daniel P. Berrange wrote:
> Add a new 'guest-set-admin-password' command for changing the
> root/administrator password. This command is needed to allow
> OpenStack to support its API for changing the admin password
> on a running guest.
>
> Accepts either the raw password string:
>
> $ virsh -c qemu:///system qemu-agent-command f21x86_64 \
>'{ "execute": "guest-set-admin-password", "arguments":
> { "crypted": false, "password": "12345678" } }'
> {"return":{}}
>
> Or a pre-encrypted string (recommended)
>
> $ virsh -c qemu:///system qemu-agent-command f21x86_64 \
>'{ "execute": "guest-set-admin-password", "arguments":
> { "crypted": true, "password":
> "$6$T9O/j/aGPrE...sniprQoRN4F0.GG0MPjNUNyml." } }'
Does it have to be a QMP command? Wouldn't the recently (re-)submitted
guest-exec allow to do the same, by running "chpasswd" in the guest and
piping the username:password into its stdin?
Besides I think it makes sense to (optionally) pass the username, to
allow to change the password for arbitrary users. This would make the
functionality useful for systems where root password plays no role as
root logins are disallowed, and the only access to root shell is via
sudo from a user belonging to a particular group (IIRC Ubuntu is usually
set up like that).
> NB windows support is desirable, but not implemented in this
> patch.
Yes Windows may have an issue with username here too, because the admin
user can be any user (and even "Administrator" can be localized).
Roman.
Re: [Qemu-devel] [PATCH 1/2] block: use fallocate(FALLOC_FL_ZERO_RANGE) in handle_aiocb_write_zeroes
On Thu, Dec 25, 2014 at 08:37:29AM +0300, Denis V. Lunev wrote:
> +#ifdef CONFIG_FALLOCATE_ZERO_RANGE
> +do {
> +if (fallocate(s->fd, CONFIG_FALLOCATE_ZERO_RANGE,
Must be a typo, FALLOC_FL_ZERO_RANGE is what you mean.
Roman.
Re: [Qemu-devel] [PATCH 2/2] block: use fallocate(FALLOC_FL_PUNCH_HOLE) & fallocate(0) to write zeroes
On Thu, Dec 25, 2014 at 08:37:30AM +0300, Denis V. Lunev wrote:
> This sequence works efficiently if FALLOC_FL_ZERO_RANGE is not supported.
> The idea is that FALLOC_FL_PUNCH_HOLE could not increase file size
> but it cleans already allocated blocks inside the file. If we have to
> create something new, simple fallocate will do the job.
>
> This should increase performance a bit for not-so-modern kernels or for
> filesystems which do not support FALLOC_FL_ZERO_RANGE.
>
> Signed-off-by: Denis V. Lunev
> CC: Kevin Wolf
> CC: Stefan Hajnoczi
> ---
> block/raw-posix.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 9e66cb7..60972a1 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -930,6 +930,18 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData
> *aiocb)
>
> ret = -errno;
> #endif
> +#ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> +do {
> +if (fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> + aiocb->aio_offset, aiocb->aio_nbytes) == 0 &&
> +fallocate(s->fd, 0,
> + aiocb->aio_offset, aiocb->aio_nbytes) == 0) {
> +return 0;
> +}
> +} while (errno == EINTR);
> +
> +ret = -errno;
> +#endif
This is suboptimal in that fallocate(FALLOC_FL_ZERO_RANGE) would always
be called in vain for such systems. Might be worth another flag in
BDRVRawState?
Roman.
Re: [Qemu-devel] [PATCH 14/19] block/parallels: create catalog_offset helper
On Tue, Dec 30, 2014 at 01:07:07PM +0300, Denis V. Lunev wrote: > to calculate entry offset inside catalog bitmap in parallels image. > This is a matter of convinience. > > Signed-off-by: Denis V. Lunev > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 12 > 1 file changed, 8 insertions(+), 4 deletions(-) Acked-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 15/19] block/parallels: rename catalog_ names to bat_
On Tue, Dec 30, 2014 at 01:07:08PM +0300, Denis V. Lunev wrote: > BAT means 'block allocation table'. Thus this name is clean and shorter > on writing. > > Signed-off-by: Denis V. Lunev > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 48 > 1 file changed, 24 insertions(+), 24 deletions(-) Acked-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 13/19] block/parallels: store ParallelsHeader to the BDRVParallelsState
On Tue, Dec 30, 2014 at 01:07:06PM +0300, Denis V. Lunev wrote: > This would be useful for the future for speed optimizations of new block > creation in the image. At the moment each write to the catalog bitmap > results in read-modify-write transaction. It would be beneficial to > write by pages or sectors. Though in order to do that for the begining > of the image we should keep the header somethere to obtain first sector > of the image properly. BDRVParallelsState would be a good place for that. > > Signed-off-by: Denis V. Lunev > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 19 ++- > 1 file changed, 10 insertions(+), 9 deletions(-) Acked-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 16/19] block/parallels: no need to flush on each block allocation table update
On Tue, Dec 30, 2014 at 01:07:09PM +0300, Denis V. Lunev wrote:
> From the point of guest each write to real disk prior to disk barrier
> operation could be lost. Therefore there is no problem that "not synced"
> new block is lost due to not updated allocation table if QEMU is crashed.
>
> This patch improves writing performance of
> qemu-img create -f parallels -o cluster_size=64k ./1.hds 64G
> qemu-io -f parallels -c "write -P 0x11 0 1024k" 1.hds
> from 45 Mb/sec to 160 Mb/sec on my SSD disk. The gain on rotational media
> is much more sufficient, from 800 Kb/sec to 45 Mb/sec.
>
> Signed-off-by: Denis V. Lunev
> CC: Kevin Wolf
> CC: Stefan Hajnoczi
> ---
> block/parallels.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index ddc3aee..46cf031 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -194,7 +194,7 @@ static int64_t allocate_sector(BlockDriverState *bs,
> int64_t sector_num)
>
> tmp = cpu_to_le32(s->bat[idx]);
>
> -ret = bdrv_pwrite_sync(bs->file, bat_offset(idx), &tmp, sizeof(tmp));
> +ret = bdrv_pwrite(bs->file, bat_offset(idx), &tmp, sizeof(tmp));
> if (ret < 0) {
> return ret;
> }
AFAICT this worsens the problem that existed before this patch:
if the preceding bdrv_truncate(+cluster_size) gets reordered with this
bat entry update, there's a risk that on poweroff (snapshot, etc.) the
bat entry gets written to the storage while the file size is not
updated. As a result, the bat entry in the file would point at a
cluster which hadn't been allocated yet. When a new block is eventually
added to the file, you'd have two bat entries pointing at the same
cluster.
The _sync version used to leave this window quite narrow due to the
flush following the write. The patch makes the reordering more likely.
I'm afraid the only reliable way to handle it is to put a barrier
between truncate and bat update, and mitigate the costs by batching the
file expansions, as you seem to do in the followup patches.
Roman.
Re: [Qemu-devel] [PATCH 16/19] block/parallels: no need to flush on each block allocation table update
On Tue, Jan 13, 2015 at 11:16:04PM +0300, Denis V. Lunev wrote:
> On 13/01/15 18:17, Denis V. Lunev wrote:
> >On 13/01/15 17:50, Roman Kagan wrote:
> >>On Tue, Dec 30, 2014 at 01:07:09PM +0300, Denis V. Lunev wrote:
> >>>--- a/block/parallels.c
> >>>+++ b/block/parallels.c
> >>>@@ -194,7 +194,7 @@ static int64_t allocate_sector(BlockDriverState
> >>>*bs, int64_t sector_num)
> >>>tmp = cpu_to_le32(s->bat[idx]);
> >>> -ret = bdrv_pwrite_sync(bs->file, bat_offset(idx), &tmp,
> >>>sizeof(tmp));
> >>>+ret = bdrv_pwrite(bs->file, bat_offset(idx), &tmp, sizeof(tmp));
> >>> if (ret < 0) {
> >>> return ret;
> >>> }
> >>if the preceding bdrv_truncate(+cluster_size) gets reordered with this
> >>bat entry update, there's a risk that on poweroff (snapshot, etc.) the
> >>bat entry gets written to the storage while the file size is not
> >>updated. As a result, the bat entry in the file would point at a
> >>cluster which hadn't been allocated yet. When a new block is eventually
> >>added to the file, you'd have two bat entries pointing at the same
> >>cluster.
> >>
> >>The _sync version used to leave this window quite narrow due to the
> >>flush following the write. The patch makes the reordering more likely.
> >>
> >>I'm afraid the only reliable way to handle it is to put a barrier
> >>between truncate and bat update, and mitigate the costs by batching the
> >>file expansions, as you seem to do in the followup patches.
>
> Thinking a bit more on this. IMHO there is no problem
> with a normal workflow, the problem could come
> from a crash when the file state and a BAT state will
> be not consistent and the BAT entry will point out
> of the file (could point out of the file).
Yes that's exactly the scenario I've been talking about.
> But it is not possible to fix at all this way. Single sync
> does not provide much warranty.
How's that?
> We should write proper magic into ParallelsHeader->inuse
> on open and call sync immediately. On subsequent
> open of the broken image we should perform
> check consistency. The magic should be set to 0
> on clean close.
This seems to address the problem, but only if this protocol is adhered
to by all the tools that may access the image (Parallels Desktop,
Parallels Server, ploop, etc.). Furthermore, if it is, it *has* to be
adhered to by this code, too, while adding write support, otherwise
we'll break the assumptions of those other tools.
> I think that this stuff could be implemented separately
> in the next patchset. If you this that this is mandatory,
> OK, I can do that, be this will increase patchset a lot.
> Check consistency is not an easy stuff.
I think as a 1st approximation, instead of a full-fledged consistency
check and repair, just refusing write access to the image that has
->inuse set would suffice.
Roman.
Re: [Qemu-devel] [PATCH 17/19] block/parallels: delay writing to BAT till bdrv_co_flush_to_os
On Tue, Dec 30, 2014 at 01:07:10PM +0300, Denis V. Lunev wrote:
> The idea is that we do not need to immediately sync BAT to the image as
> from the guest point of view there is a possibility that IO is lost
> even in the physical controller until flush command was finished.
> bdrv_co_flush_to_os is exactly the right place for this purpose.
>
> Technically the patch aligns writes into BAT to MAX(bdrv_align, 4096),
> which elliminates read-modify-write transactions on BAT update and
> cache ready-to-write content in a special buffer in BDRVParallelsState.
>
> This buffer possibly contains ParallelsHeader if the first page of the
> image should be modified. The header occupies first 64 bytes of the image
> and the BAT starts immediately after it.
>
> It is also possible that BAT end is not aligned to the cluster size.
> ParallelsHeader->data_off is not specified for this case. We should write
> only part of the cache in that case.
>
> This patch speed ups
> qemu-img create -f parallels -o cluster_size=64k ./1.hds 64G
> qemu-io -f parallels -c "write -P 0x11 0 1024k" 1.hds
> writing from 50-60 Mb/sec to 80-90 Mb/sec on rotational media and
> from 160 Mb/sec to 190 Mb/sec on SSD disk.
>
> Signed-off-by: Denis V. Lunev
> CC: Kevin Wolf
> CC: Stefan Hajnoczi
> ---
> block/parallels.c | 99
> ---
> 1 file changed, 94 insertions(+), 5 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 46cf031..18b9267 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -62,6 +62,11 @@ typedef struct BDRVParallelsState {
> uint32_t *bat;
> unsigned int bat_size;
>
> +uint32_t *bat_cache;
> +unsigned bat_cache_size;
> +int bat_cache_off;
> +int data_off;
> +
> unsigned int tracks;
>
> unsigned int off_multiplier;
> @@ -148,6 +153,17 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> le32_to_cpus(&s->bat[i]);
>
> qemu_co_mutex_init(&s->lock);
> +
> +s->bat_cache_off = -1;
> +if (bs->open_flags & BDRV_O_RDWR) {
> +s->bat_cache_size = MAX(bdrv_opt_mem_align(bs->file), 4096);
> +s->bat_cache = qemu_blockalign(bs->file, s->bat_cache_size);
> +}
> +s->data_off = le32_to_cpu(s->ph.data_off) * BDRV_SECTOR_SIZE;
> +if (s->data_off == 0) {
> +s->data_off = ROUND_UP(bat_offset(s->bat_size), BDRV_SECTOR_SIZE);
> +}
> +
> return 0;
>
> fail_format:
> @@ -171,10 +187,71 @@ static int64_t seek_to_sector(BDRVParallelsState *s,
> int64_t sector_num)
> return (uint64_t)s->bat[index] * s->off_multiplier + offset;
> }
>
> +static int write_bat_cache(BlockDriverState *bs)
> +{
> +BDRVParallelsState *s = bs->opaque;
> +int size, off;
> +
> +if (s->bat_cache_off == -1) {
> +/* no cached data */
> +return 0;
> +}
> +
> +size = s->bat_cache_size;
> +if (size + s->bat_cache_off > s->data_off) {
> +/* avoid writing to the first data block */
> +size = s->data_off - s->bat_cache_off;
> +}
> +
> +off = s->bat_cache_off;
> +s->bat_cache_off = -1;
> +return bdrv_pwrite(bs->file, off, s->bat_cache, size);
> +}
> +
> +static int cache_bat(BlockDriverState *bs, uint32_t idx, uint32_t
> new_data_off)
> +{
> +int ret, i, off, cache_off;
> +int64_t first_idx, last_idx;
> +BDRVParallelsState *s = bs->opaque;
> +uint32_t *cache = s->bat_cache;
> +
> +off = bat_offset(idx);
> +cache_off = (off / s->bat_cache_size) * s->bat_cache_size;
> +
> +if (s->bat_cache_off != -1 && s->bat_cache_off != cache_off) {
> +ret = write_bat_cache(bs);
> +if (ret < 0) {
> +return ret;
> +}
> +}
> +
> +first_idx = idx - (off - cache_off) / sizeof(uint32_t);
> +last_idx = first_idx + s->bat_cache_size / sizeof(uint32_t);
> +if (first_idx < 0) {
> +memcpy(s->bat_cache, &s->ph, sizeof(s->ph));
> +first_idx = 0;
> +cache = s->bat_cache + sizeof(s->ph) / sizeof(uint32_t);
> +}
> +
> +if (last_idx > s->bat_size) {
> +memset(cache + s->bat_size - first_idx, 0,
> + sizeof(uint32_t) * (last_idx - s->bat_size));
> +}
> +
> +for (i = 0; i < last_idx - first_idx; i++) {
> +cache[i] = cpu_to_le32(s->bat[first_idx + i]);
> +}
You're re-populating the bat_cache on every bat update. Why?
Shouldn't this be done only with empty cache, i.e. under
if (s->bat_cache_off == -1)?
> +cache[idx - first_idx] = cpu_to_le32(new_data_off);
> +s->bat[idx] = new_data_off;
> +
> +s->bat_cache_off = cache_off;
> +return 0;
> +}
> +
> static int64_t allocate_sector(BlockDriverState *bs, int64_t sector_num)
> {
> BDRVParallelsState *s = bs->opaque;
> -uint32_t idx, offset, tmp;
> +uint32_t idx, offset;
> int64_t pos;
> int ret;
>
> @@ -190,17 +267,27 @@ static int64_t allocate_sector(BlockDriverState *bs,
Re: [Qemu-devel] [PATCH 17/19] block/parallels: delay writing to BAT till bdrv_co_flush_to_os
On Wed, Jan 14, 2015 at 04:08:50PM +0300, Denis V. Lunev wrote:
> On 14/01/15 16:03, Roman Kagan wrote:
> >On Tue, Dec 30, 2014 at 01:07:10PM +0300, Denis V. Lunev wrote:
> >>+static int cache_bat(BlockDriverState *bs, uint32_t idx, uint32_t
> >>new_data_off)
> >>+{
> >>+int ret, i, off, cache_off;
> >>+int64_t first_idx, last_idx;
> >>+BDRVParallelsState *s = bs->opaque;
> >>+uint32_t *cache = s->bat_cache;
> >>+
> >>+off = bat_offset(idx);
> >>+cache_off = (off / s->bat_cache_size) * s->bat_cache_size;
> >>+
> >>+if (s->bat_cache_off != -1 && s->bat_cache_off != cache_off) {
> >>+ret = write_bat_cache(bs);
> >>+if (ret < 0) {
> >>+return ret;
> >>+}
> >>+}
> >>+
> >>+first_idx = idx - (off - cache_off) / sizeof(uint32_t);
> >>+last_idx = first_idx + s->bat_cache_size / sizeof(uint32_t);
> >>+if (first_idx < 0) {
> >>+memcpy(s->bat_cache, &s->ph, sizeof(s->ph));
> >>+first_idx = 0;
> >>+cache = s->bat_cache + sizeof(s->ph) / sizeof(uint32_t);
> >>+}
> >>+
> >>+if (last_idx > s->bat_size) {
> >>+memset(cache + s->bat_size - first_idx, 0,
> >>+ sizeof(uint32_t) * (last_idx - s->bat_size));
> >>+}
> >>+
> >>+for (i = 0; i < last_idx - first_idx; i++) {
> >>+cache[i] = cpu_to_le32(s->bat[first_idx + i]);
> >>+}
> >You're re-populating the bat_cache on every bat update. Why?
> >
> >Shouldn't this be done only with empty cache, i.e. under
> >if (s->bat_cache_off == -1)?
> reasonable, but condition should be a bit different, namely
>
> if (s->bat_cache_off != -1 && s->bat_cache_off != cache_off) {
No, this is the condition to flush the cache which you already do.
Then, either upon flushing it or on the first entry into cache_bat(),
the cache is empty which is indicated by ->bat_cache_off == -1, at which
point you need to populate it from ->bat.
> >> static void parallels_close(BlockDriverState *bs)
> >> {
> >> BDRVParallelsState *s = bs->opaque;
> >>+qemu_vfree(s->bat_cache);
> >Don't you need to flush the bat cache here first?
>
> parallels_co_flush_to_os should happen beforehand
>
>
> void bdrv_close(BlockDriverState *bs)
> {
>
> bdrv_flush(bs); <-- namely inside this
Indeed.
Roman.
Re: [Qemu-devel] [PATCH 18/19] block/parallels: add prealloc-mode and prealloc-size open paramemets
On Tue, Dec 30, 2014 at 01:07:11PM +0300, Denis V. Lunev wrote:
> This is preparational commit for tweaks in Parallels image expansion.
> The idea is that enlarge via truncate by one data block is slow. It
> would be much better to use fallocate via bdrv_write_zeroes and
> expand by some significant amount at once.
>
> This patch just adds proper parameters into BDRVParallelsState and
> performs options parsing in parallels_open.
>
> Signed-off-by: Denis V. Lunev
> CC: Kevin Wolf
> CC: Stefan Hajnoczi
> ---
> block/parallels.c | 72
> +++
> 1 file changed, 72 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 18b9267..12a9cea 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -30,6 +30,7 @@
> #include "qemu-common.h"
> #include "block/block_int.h"
> #include "qemu/module.h"
> +#include "qapi/util.h"
>
> /**/
>
> @@ -54,6 +55,20 @@ typedef struct ParallelsHeader {
> char padding[12];
> } QEMU_PACKED ParallelsHeader;
>
> +
> +typedef enum ParallelsPreallocMode {
> +PRL_PREALLOC_MODE_FALLOCATE = 0,
> +PRL_PREALLOC_MODE_TRUNCATE = 1,
> +PRL_PREALLOC_MODE_MAX = 2,
> +} ParallelsPreallocMode;
> +
> +static const char *prealloc_mode_lookup[] = {
> +"falloc",
> +"truncate",
> +NULL,
There is already generic "preallocaton" option, BLOCK_OPT_PREALLOC,
which is handled by qcow2 and raw-posix. It means slightly different
thing: the *whole* image is preallocated using the method specified.
I think it would make sense to consolidate that option with this new
batched allocation in the generic block code. I guess qcow2 and
raw-posix would benefit from it, too.
At any rate I think it's a matter for a separate patchset.
Roman.
Re: [Qemu-devel] [PATCH 18/19] block/parallels: add prealloc-mode and prealloc-size open paramemets
On Wed, Jan 14, 2015 at 05:31:20PM +0300, Denis V. Lunev wrote:
> On 14/01/15 17:26, Roman Kagan wrote:
> >On Tue, Dec 30, 2014 at 01:07:11PM +0300, Denis V. Lunev wrote:
> >>This is preparational commit for tweaks in Parallels image expansion.
> >>The idea is that enlarge via truncate by one data block is slow. It
> >>would be much better to use fallocate via bdrv_write_zeroes and
> >>expand by some significant amount at once.
> >>
> >>This patch just adds proper parameters into BDRVParallelsState and
> >>performs options parsing in parallels_open.
> >>
> >>Signed-off-by: Denis V. Lunev
> >>CC: Kevin Wolf
> >>CC: Stefan Hajnoczi
> >>---
> >> block/parallels.c | 72
> >> +++
> >> 1 file changed, 72 insertions(+)
> >>
> >>diff --git a/block/parallels.c b/block/parallels.c
> >>index 18b9267..12a9cea 100644
> >>--- a/block/parallels.c
> >>+++ b/block/parallels.c
> >>@@ -30,6 +30,7 @@
> >> #include "qemu-common.h"
> >> #include "block/block_int.h"
> >> #include "qemu/module.h"
> >>+#include "qapi/util.h"
> >> /**/
> >>@@ -54,6 +55,20 @@ typedef struct ParallelsHeader {
> >> char padding[12];
> >> } QEMU_PACKED ParallelsHeader;
> >>+
> >>+typedef enum ParallelsPreallocMode {
> >>+PRL_PREALLOC_MODE_FALLOCATE = 0,
> >>+PRL_PREALLOC_MODE_TRUNCATE = 1,
> >>+PRL_PREALLOC_MODE_MAX = 2,
> >>+} ParallelsPreallocMode;
> >>+
> >>+static const char *prealloc_mode_lookup[] = {
> >>+"falloc",
> >>+"truncate",
> >>+NULL,
> >There is already generic "preallocaton" option, BLOCK_OPT_PREALLOC,
> >which is handled by qcow2 and raw-posix. It means slightly different
> >thing: the *whole* image is preallocated using the method specified.
> >
> >I think it would make sense to consolidate that option with this new
> >batched allocation in the generic block code. I guess qcow2 and
> >raw-posix would benefit from it, too.
> >
> >At any rate I think it's a matter for a separate patchset.
> >
> >Roman.
> it is too early :) I think that I should provide the rationale for
> the preallocation in general. I am working on this with CEPH :)
OK then, maybe indeed it should land in parallels driver first, and move
into the generic code if found appropriate...
Then my only problem with this patch is the naming of the options: they
look too similar to the generic one while the meaning is different.
Maybe "batched_alloc" reflects the meaning better?
Roman.
Re: [Qemu-devel] [PATCH 19/19] block/parallels: optimize linear image expansion
On Tue, Dec 30, 2014 at 01:07:12PM +0300, Denis V. Lunev wrote:
> Plain image expansion spends a lot of time to update image file size.
> This seriously affects the performance. The following simple test
> qemu_img create -f parallels -o cluster_size=64k ./1.hds 64G
> qemu_io -n -c "write -P 0x11 0 1024M" ./1.hds
> could be improved if the format driver will pre-allocate some space
> in the image file with a reasonable chunk.
>
> This patch preallocates 128 Mb using bdrv_write_zeroes, which should
> normally use fallocate() call inside. Fallback to older truncate()
> could be used as a fallback using image open options thanks to the
> previous patch.
>
> The benefit is around 15%.
>
> This patch is final in this series. Block driver has near native
> performance now.
>
> Signed-off-by: Denis V. Lunev
> CC: Kevin Wolf
> CC: Stefan Hajnoczi
> ---
> block/parallels.c | 35 +--
> 1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 12a9cea..5ec4a0d 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -82,6 +82,7 @@ typedef struct BDRVParallelsState {
> int bat_cache_off;
> int data_off;
>
> +int64_t prealloc_off;
This field name confused me. I think "data_end" would fit better.
Otherwise looks good to me.
Roman.
Re: [Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files
On Mon, Dec 15, 2014 at 01:30:03PM +0100, Kevin Wolf wrote: > Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben: > > From: Roman Kagan > > > > Add backing file support to Parallels format driver. > > > > That said, I think backing file operations should end up in the generic > > block layer, but that's a longer story... > > > > Signed-off-by: Roman Kagan > > Signed-off-by: Denis V. Lunev > > CC: Jeff Cody > > CC: Kevin Wolf > > CC: Stefan Hajnoczi > > How are users supposed to make use of this? bs->backing_file isn't set > during parallels_open(), so you generally dont' get a backing file. > > Users might manually add -drive backing=..., but is there really no > support for storing the backing file in the image format? If so, perhaps > it's better not to support backing files at all here. Parallels virtual disks (as also used by ploop, http://openvz.org/Ploop) consist of a descriptor xml file and one or more data files. The data files may have delta/backing relationships; those relationships are only stored in the descriptor file. The original parallels driver in qemu used to support direct access to the data files. This patch makes it possible for such a data file to be a delta WRT a backing file, specified externally (see e.g. the patch to the tests). The code for parsing the descriptor xml and filling in the delta/backing relations isn't there yet; we haven't yet figured out where it fits best. Still it makes up a perfectly valid usecase to figure out those relations by some external means (e.g. eye inspection of the descriptor file :) and construct appropriate JSON filename for use in any scenario where a readonly image can be used in qemu. Roman.
Re: [Qemu-devel] [PATCH 06/27] block/parallels: provide _co_readv routine for parallels format driver
On Tue, Apr 28, 2015 at 10:46:39AM +0300, Denis V. Lunev wrote: > Main approach is taken from qcow2_co_readv. > > The patch drops coroutine lock for the duration of IO operation and > peforms normal scatter-gather IO using standard QEMU backend. > > The patch also adds comment about locking considerations in the driver. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 54 +- > 1 file changed, 33 insertions(+), 21 deletions(-) Reviewed-by: Roman Kagan
Re: [Qemu-devel] [PATCH 08/27] block/parallels: mark parallels format driver as zero inited
On Tue, Apr 28, 2015 at 10:46:41AM +0300, Denis V. Lunev wrote: > From the guest point of view unallocated blocks are zeroed. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Roman Kagan
Re: [Qemu-devel] [PATCH 26/27] block/parallels: optimize linear image expansion
On Tue, Apr 28, 2015 at 10:46:59AM +0300, Denis V. Lunev wrote: > Plain image expansion spends a lot of time to update image file size. > This seriously affects the performance. The following simple test > qemu_img create -f parallels -o cluster_size=64k ./1.hds 64G > qemu_io -n -c "write -P 0x11 0 1024M" ./1.hds > could be improved if the format driver will pre-allocate some space > in the image file with a reasonable chunk. > > This patch preallocates 128 Mb using bdrv_write_zeroes, which should > normally use fallocate() call inside. Fallback to older truncate() > could be used as a fallback using image open options thanks to the > previous patch. > > The benefit is around 15%. > > Signed-off-by: Denis V. Lunev > Reviewed-by: Roman Karan s/Karan/Kagan/ > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 42 -- > 1 file changed, 32 insertions(+), 10 deletions(-) Roman.
Re: [Qemu-devel] [PATCH 09/27] block/parallels: _co_writev callback for Parallels format
On Tue, Apr 28, 2015 at 10:46:42AM +0300, Denis V. Lunev wrote: > Support write on Parallels images. The code is almost the same as one > in the previous patch implemented scatter-gather IO for read. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 90 > +-- > 1 file changed, 88 insertions(+), 2 deletions(-) Reviewed-by: Roman Kagan
Re: [Qemu-devel] [PATCH 25/27] block/parallels: add prealloc-mode and prealloc-size open paramemets
On Tue, Apr 28, 2015 at 10:46:58AM +0300, Denis V. Lunev wrote:
> This is preparational commit for tweaks in Parallels image expansion.
> The idea is that enlarge via truncate by one data block is slow. It
> would be much better to use fallocate via bdrv_write_zeroes and
> expand by some significant amount at once.
>
> Original idea with sequential file writing to the end of the file without
> fallocate/truncate would be slower than this approach if the image is
> expanded with several operations:
> - each image expanding means file metadata update, i.e. filesystem
> journal write. Truncate/write to newly truncated space update file
> metadata twice thus truncate removal helps. With fallocate call
> inside bdrv_write_zeroes file metadata is updated only once and
> this should happen infrequently thus this approach is the best one
> for the image expansion
> - tail writes are ordered, i.e. the guest IO queue could not be sent
> immediately to the host introducing additional IO delays
>
> This patch just adds proper parameters into BDRVParallelsState and
> performs options parsing in parallels_open.
>
> Signed-off-by: Denis V. Lunev
> CC: Roman Kagan
> CC: Kevin Wolf
> CC: Stefan Hajnoczi
> ---
> block/parallels.c | 83
> +++
> 1 file changed, 77 insertions(+), 6 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 05fe030..440938e 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -31,6 +31,7 @@
> #include "block/block_int.h"
> #include "qemu/module.h"
> #include "qemu/bitmap.h"
> +#include "qapi/util.h"
>
> /**/
>
> @@ -56,6 +57,20 @@ typedef struct ParallelsHeader {
> char padding[12];
> } QEMU_PACKED ParallelsHeader;
>
> +
> +typedef enum ParallelsPreallocMode {
> +PRL_PREALLOC_MODE_FALLOCATE = 0,
> +PRL_PREALLOC_MODE_TRUNCATE = 1,
> +PRL_PREALLOC_MODE_MAX = 2,
> +} ParallelsPreallocMode;
> +
> +static const char *prealloc_mode_lookup[] = {
> +"falloc",
> +"truncate",
> +NULL,
> +};
> +
> +
> typedef struct BDRVParallelsState {
> /** Locking is conservative, the lock protects
> * - image file extending (truncate, fallocate)
> @@ -73,14 +88,40 @@ typedef struct BDRVParallelsState {
> uint32_t *bat_bitmap;
> unsigned int bat_size;
>
> +uint64_t prealloc_size;
> +ParallelsPreallocMode prealloc_mode;
> +
> unsigned int tracks;
>
> unsigned int off_multiplier;
> -
> -bool has_truncate;
> } BDRVParallelsState;
>
>
> +#define PARALLELS_OPT_PREALLOC_MODE "prealloc-mode"
> +#define PARALLELS_OPT_PREALLOC_SIZE "prealloc-size"
> +
> +static QemuOptsList parallels_runtime_opts = {
> +.name = "parallels",
> +.head = QTAILQ_HEAD_INITIALIZER(parallels_runtime_opts.head),
> +.desc = {
> +{
> +.name = PARALLELS_OPT_PREALLOC_SIZE,
> +.type = QEMU_OPT_SIZE,
> +.help = "Preallocation size on image expansion",
> +.def_value_str = "128MiB",
> +},
> +{
> +.name = PARALLELS_OPT_PREALLOC_MODE,
> +.type = QEMU_OPT_STRING,
> +.help = "Preallocation mode on image expansion "
> +"(allowed values: falloc, truncate)",
> +.def_value_str = "falloc",
> +},
> +{ /* end of list */ },
> +},
> +};
> +
> +
> static int64_t bat2sect(BDRVParallelsState *s, uint32_t idx)
> {
> return (uint64_t)le32_to_cpu(s->bat_bitmap[idx]) * s->off_multiplier;
> @@ -159,7 +200,7 @@ static int64_t allocate_cluster(BlockDriverState *bs,
> int64_t sector_num)
> }
>
> pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
> -if (s->has_truncate) {
> +if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
> ret = bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
> } else {
> ret = bdrv_write_zeroes(bs->file, pos, s->tracks, 0);
> @@ -509,6 +550,9 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> BDRVParallelsState *s = bs->opaque;
> ParallelsHeader ph;
> int ret, size;
> +QemuOpts *opts = NULL;
> +Error *local_err = NULL;
> +char *buf;
>
> ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
> if (ret < 0) {
> @@ -567,9 +611,6 @@ static i
Re: [Qemu-devel] [PATCH 25/27] block/parallels: add prealloc-mode and prealloc-size open paramemets
On Tue, Apr 28, 2015 at 01:59:56PM +0300, Roman Kagan wrote: > On Tue, Apr 28, 2015 at 10:46:58AM +0300, Denis V. Lunev wrote: > > This is preparational commit for tweaks in Parallels image expansion. > > The idea is that enlarge via truncate by one data block is slow. It > > would be much better to use fallocate via bdrv_write_zeroes and > > expand by some significant amount at once. > > > > Original idea with sequential file writing to the end of the file without > > fallocate/truncate would be slower than this approach if the image is > > expanded with several operations: > > - each image expanding means file metadata update, i.e. filesystem > > journal write. Truncate/write to newly truncated space update file > > metadata twice thus truncate removal helps. With fallocate call > > inside bdrv_write_zeroes file metadata is updated only once and > > this should happen infrequently thus this approach is the best one > > for the image expansion > > - tail writes are ordered, i.e. the guest IO queue could not be sent > > immediately to the host introducing additional IO delays > > > > This patch just adds proper parameters into BDRVParallelsState and > > performs options parsing in parallels_open. > > > > Signed-off-by: Denis V. Lunev > > CC: Roman Kagan > > CC: Kevin Wolf > > CC: Stefan Hajnoczi > > --- > > block/parallels.c | 83 > > +++ > > 1 file changed, 77 insertions(+), 6 deletions(-) > > It may be slightly more logical to leave truncate support out of patch > 09 sticking with bdrv_write_zeros there, and introduce it all in this > patch. > > Otherwise looks good to me. I mean, Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 15/27] block/parallels: keep BAT bitmap data in little endian in memory
On Tue, Mar 10, 2015 at 11:51:09AM +0300, Denis V. Lunev wrote: > This will allow to use this data as buffer to BAT update directly > without any intermediate buffers. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 15 +-- > 1 file changed, 5 insertions(+), 10 deletions(-) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 14/27] block/parallels: create bat2sect helper
On Tue, Mar 10, 2015 at 11:51:08AM +0300, Denis V. Lunev wrote: > deduplicate copy/paste arithmetcs > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 12 +--- > 1 file changed, 9 insertions(+), 3 deletions(-) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 21/27] block/parallels: no need to flush on each block allocation table update
On Tue, Mar 10, 2015 at 11:51:15AM +0300, Denis V. Lunev wrote: > From the point of guest each write to real disk prior to disk barrier > operation could be lost. Therefore there is no problem that "not synced" > new block is lost due to not updated allocation table if QEMU is crashed. > This situation is properly detected and handled now using inuse magic > and in parallels_check > > This patch improves writing performance of > qemu-img create -f parallels -o cluster_size=64k ./1.hds 64G > qemu-io -f parallels -c "write -P 0x11 0 1024k" 1.hds > from 45 Mb/sec to 160 Mb/sec on my SSD disk. The gain on rotational media > is much more sufficient, from 800 Kb/sec to 45 Mb/sec. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 24/27] block/parallels: delay writing to BAT till bdrv_co_flush_to_os
On Tue, Mar 10, 2015 at 11:51:18AM +0300, Denis V. Lunev wrote: > The idea is that we do not need to immediately sync BAT to the image as > from the guest point of view there is a possibility that IO is lost > even in the physical controller until flush command was finished. > bdrv_co_flush_to_os is exactly the right place for this purpose. > > Technically the patch uses loaded BAT data as a cache and performs > actual on-disk metadata updates in parallels_co_flush_to_os callback. > > This patch speed ups > qemu-img create -f parallels -o cluster_size=64k ./1.hds 64G > qemu-io -f parallels -c "write -P 0x11 0 1024k" 1.hds > writing from 50-60 Mb/sec to 80-90 Mb/sec on rotational media and > from 160 Mb/sec to 190 Mb/sec on SSD disk. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 50 ------ > 1 file changed, 44 insertions(+), 6 deletions(-) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 19/27] block/parallels: implement incorrect close detection
On Tue, Mar 10, 2015 at 11:51:13AM +0300, Denis V. Lunev wrote:
> The software driver must set inuse field in Parallels header to
> 0x746F6E59 when the image is opened in read-write mode. The presence of
> this magic in the header on open forces image consistency check.
>
> There is an unfortunate trick here. We can not check for inuse in
> parallels_check as this will happen too late. It is possible to do
> that for simple check, but during the fix this would always report
> an error as the image was opened in BDRV_O_RDWR mode. Thus we save
> the flag in BDRVParallelsState for this.
>
> On the other hand, nothing should be done to clear inuse in
> parallels_check. Generic close will do the job right.
>
> Signed-off-by: Denis V. Lunev
> CC: Roman Kagan
> CC: Kevin Wolf
> CC: Stefan Hajnoczi
> ---
> block/parallels.c | 50 ++
> 1 file changed, 50 insertions(+)
> @@ -462,6 +487,25 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> }
> s->bat_bitmap = (uint32_t *)(s->header + 1);
>
> +if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +/* Image was not closed correctly. The check is mandatory */
> +s->header_unclean = true;
> +if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
> +error_setg(errp, "parallels: Image was not closed correctly; "
> + "cannot be opened read/write");
> +ret = -EACCES;
-EBUSY?
Otherwise looks ok.
Roman.
Re: [Qemu-devel] [PATCH 20/27] iotests, parallels: check for incorrectly closed image in tests
On Tue, Mar 10, 2015 at 11:51:14AM +0300, Denis V. Lunev wrote: > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > tests/qemu-iotests/115 | 9 + > tests/qemu-iotests/115.out | 19 ++- > 2 files changed, 27 insertions(+), 1 deletion(-) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 25/27] block/parallels: add prealloc-mode and prealloc-size open paramemets
On Tue, Mar 10, 2015 at 11:51:19AM +0300, Denis V. Lunev wrote: > This is preparational commit for tweaks in Parallels image expansion. > The idea is that enlarge via truncate by one data block is slow. It > would be much better to use fallocate via bdrv_write_zeroes and > expand by some significant amount at once. > > This patch just adds proper parameters into BDRVParallelsState and > performs options parsing in parallels_open. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 72 > +++ > 1 file changed, 72 insertions(+) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 18/27] block/parallels: implement parallels_check method of block driver
On Tue, Mar 10, 2015 at 11:51:12AM +0300, Denis V. Lunev wrote:
> The check is very simple at the moment. It calculates necessary stats
> and fix only the following errors:
> - space leak at the end of the image. This would happens due to
> preallocation
> - clusters outside the image are zeroed. Nothing else could be done here
>
> Signed-off-by: Denis V. Lunev
> CC: Roman Kagan
> CC: Kevin Wolf
> CC: Stefan Hajnoczi
> ---
> block/parallels.c | 82
> +++
> 1 file changed, 82 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 65f6418..e5b475e 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -228,6 +228,87 @@ fail:
> return ret;
> }
>
> +
> +static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res,
> + BdrvCheckMode fix)
> +{
> +BDRVParallelsState *s = bs->opaque;
> +int64_t size, prev_off, high_off;
> +int ret;
> +uint32_t i;
> +bool flush_bat = false;
> +int cluster_size = s->tracks << BDRV_SECTOR_BITS;
> +
> +size = bdrv_getlength(bs->file);
> +if (size < 0) {
> +res->check_errors++;
> +return size;
> +}
> +
> +res->bfi.total_clusters = s->bat_size;
> +res->bfi.compressed_clusters = 0; /* compression is not supported */
> +
> +high_off = 0;
> +prev_off = 0;
> +for (i = 0; i < s->bat_size; i++) {
> +int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +if (off == 0)
> +continue;
If you don't update prev_off here you can get fragmentation stats wrong
(dunno how important it is)
> +
> +/* cluster outside the image */
> +if (off > size) {
> +fprintf(stderr, "%s cluster %u is outside image\n",
> +fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> +res->corruptions++;
> +if (fix & BDRV_FIX_ERRORS) {
> +s->bat_bitmap[i] = 0;
> +res->corruptions_fixed++;
> +flush_bat = true;
> +continue;
> +}
Ditto
Roman.
Re: [Qemu-devel] [PATCH 16/27] block/parallels: read parallels image header and BAT into single buffer
On Tue, Mar 10, 2015 at 11:51:10AM +0300, Denis V. Lunev wrote: > This metadata cache would allow to properly batch BAT updates to disk > in next patches. These updates will be properly aligned to avoid > read-modify-write transactions on block level. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 24 +--- > 1 file changed, 17 insertions(+), 7 deletions(-) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 23/27] block/parallels: create bat_entry_off helper
On Tue, Mar 10, 2015 at 11:51:17AM +0300, Denis V. Lunev wrote: > calculate offset of the BAT entry in the image file. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 15 +-- > 1 file changed, 9 insertions(+), 6 deletions(-) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 19/27] block/parallels: implement incorrect close detection
On Tue, Mar 10, 2015 at 05:44:19PM +0300, Denis V. Lunev wrote:
> On 10/03/15 17:38, Roman Kagan wrote:
> >On Tue, Mar 10, 2015 at 11:51:13AM +0300, Denis V. Lunev wrote:
> >>The software driver must set inuse field in Parallels header to
> >>0x746F6E59 when the image is opened in read-write mode. The presence of
> >>this magic in the header on open forces image consistency check.
> >>
> >>There is an unfortunate trick here. We can not check for inuse in
> >>parallels_check as this will happen too late. It is possible to do
> >>that for simple check, but during the fix this would always report
> >>an error as the image was opened in BDRV_O_RDWR mode. Thus we save
> >>the flag in BDRVParallelsState for this.
> >>
> >>On the other hand, nothing should be done to clear inuse in
> >>parallels_check. Generic close will do the job right.
> >>
> >>Signed-off-by: Denis V. Lunev
> >>CC: Roman Kagan
> >>CC: Kevin Wolf
> >>CC: Stefan Hajnoczi
> >>---
> >> block/parallels.c | 50 ++
> >> 1 file changed, 50 insertions(+)
> >>@@ -462,6 +487,25 @@ static int parallels_open(BlockDriverState *bs, QDict
> >>*options, int flags,
> >> }
> >> s->bat_bitmap = (uint32_t *)(s->header + 1);
> >>+if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> >>+/* Image was not closed correctly. The check is mandatory */
> >>+s->header_unclean = true;
> >>+if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
> >>+error_setg(errp, "parallels: Image was not closed correctly; "
> >>+ "cannot be opened read/write");
> >>+ret = -EACCES;
> >-EBUSY?
> >
> >Otherwise looks ok.
> >
> >Roman.
> error code is the same as one in qcow2
OK then
Reviewed-by: Roman Kagan
Roman.
Re: [Qemu-devel] [PATCH 17/27] block/parallels: move parallels_open/probe to the very end of the file
On Tue, Mar 10, 2015 at 11:51:11AM +0300, Denis V. Lunev wrote: > This will help to avoid forward declarations for upcoming parallels_check > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 183 > +++--- > 1 file changed, 93 insertions(+), 90 deletions(-) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 22/27] block/parallels: improve image reading performance
On Tue, Mar 10, 2015 at 11:51:16AM +0300, Denis V. Lunev wrote: > Try to perform IO for the biggest continuous block possible. > The performance for sequential read is increased from 220 Gb/sec to > 360 Gb/sec for continous image on my SSD HDD. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 37 - > 1 file changed, 32 insertions(+), 5 deletions(-) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 27/27] block/parallels: improve image writing performance further
On Tue, Mar 10, 2015 at 11:51:21AM +0300, Denis V. Lunev wrote: > Try to perform IO for the biggest continuous block possible. > All blocks abscent in the image are accounted in the same type > and preallocation is made for all of them at once. > > The performance for sequential write is increased from 200 Gb/sec to > 235 Gb/sec on my SSD HDD. > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 44 > 1 file changed, 24 insertions(+), 20 deletions(-) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 18/27] block/parallels: implement parallels_check method of block driver
On Wed, Mar 11, 2015 at 01:28:12PM +0300, Denis V. Lunev wrote: > The check is very simple at the moment. It calculates necessary stats > and fix only the following errors: > - space leak at the end of the image. This would happens due to > preallocation > - clusters outside the image are zeroed. Nothing else could be done here > > Signed-off-by: Denis V. Lunev > CC: Roman Kagan > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 85 > +++ > 1 file changed, 85 insertions(+) Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH 1/2] kvm/x86: Hyper-V synthetic interrupt controller
On Fri, Oct 09, 2015 at 04:42:33PM +0200, Paolo Bonzini wrote: > You need to add SYNIC vectors to the EOI exit bitmap, so that APICv > (Xeon E5 or higher, Ivy Bridge or newer) is handled correctly. You also > need to check the auto EOI exit bitmap in __apic_accept_irq, and avoid > going through kvm_x86_ops->deliver_posted_interrupt for auto EOI > vectors. Something like > > if (kvm_x86_ops->deliver_posted_interrupt && > !test_bit(...)) > > in place of the existing "if (kvm_x86_ops->deliver_posted_interrupt)". Indeed, missed that path, thanks! > I really don't like this auto-EOI extension, but I guess that's the > spec. :( If it wasn't for it, you could do everything very easily in > userspace using Google's proposed MSR exit. I guess you're right. We'd probably have to (ab)use MSI for SINT delivery, though. Anyway the need to implement auto-EOI rules that out. Thanks for the quick review, we'll try to address your comments in the next round. Roman.
Re: [Qemu-devel] [PATCH 2/2] kvm/x86: Hyper-V kvm exit
On Fri, Oct 09, 2015 at 04:41:15PM +0200, Paolo Bonzini wrote: > On 09/10/2015 15:39, Denis V. Lunev wrote: > > A new vcpu exit is introduced to notify the userspace of the > > changes in Hyper-V synic configuraion triggered by guest writing to the > > corresponding MSRs. > > Why is this exit necessary? The guest writes to synic-related MSRs and that should take "immediate" effect. E.g. it may decide to disable or relocate the message page by writing to SIMP MSR. The host is then supposed to stop accessing the old message page before the vCPU proceeds to the next instruction. Hence the exit, to allow the userspace to react accordingly before reentering the guest. Roman.
Re: [Qemu-devel] [PATCH 1/2] kvm/x86: Hyper-V synthetic interrupt controller
On Mon, Oct 12, 2015 at 10:58:36AM +0200, Paolo Bonzini wrote: > On 12/10/2015 10:48, Cornelia Huck wrote: > > Going back to Paolo's original question, I think changing the check > > to !KVM_IRQ_ROUTING_IRQCHIP makes sense, if I understand the code > > correctly. They seem to be the only special one. > > Great. Roman, Denis, can you do this then? Sure, gonna be in the next round. Thanks, Roman.
Re: [Qemu-devel] [PATCH 2/2] kvm/x86: Hyper-V kvm exit
On Mon, Oct 12, 2015 at 07:42:42AM -0600, Eric Blake wrote: > On 10/09/2015 07:39 AM, Denis V. Lunev wrote: > > From: Andrey Smetanin > > > > A new vcpu exit is introduced to notify the userspace of the > > changes in Hyper-V synic configuraion triggered by guest writing to the > Again, is 'synic' intended? Hmm, I see it throughout the patch, so it > looks intentional, but I keep trying to read it as a typo for 'sync'. I tend to mistype it as 'cynic' as better matching what it is ;) Note taken, we'll address that in the next round, thanks. Roman.
Re: [Qemu-devel] [PATCH v3 8/9] kvm/x86: Hyper-V synthetic interrupt controller
On Wed, Oct 28, 2015 at 06:41:45PM +0100, Paolo Bonzini wrote: > Hi Andrey, > > just one question. Is kvm_arch_set_irq actually needed? I think > everything should work fine without it. Can you check? If so, I can > remove it myself and revert the patch that introduced the hook. While Andrey is testing it, I'd like to ask similar question re. MSI: why is there a "shortcut" for KVM_IRQ_ROUTING_MSI case (which we basically modelled after) when it would probably get handled through ->set handler in irqfd_inject() too? Roman.
Re: [Qemu-devel] [kvm-unit-tests PATCH] x86: hyperv_synic: Hyper-V SynIC test
On Mon, Nov 02, 2015 at 01:16:02PM +0100, Paolo Bonzini wrote: > On 26/10/2015 10:56, Andrey Smetanin wrote: > > Hyper-V SynIC is a Hyper-V synthetic interrupt controller. > > > > The test runs on every vCPU and performs the following steps: > > * read from all Hyper-V SynIC MSR's > > * setup Hyper-V SynIC evt/msg pages > > * setup SINT's routing > > * inject SINT's into destination vCPU by 'hyperv-synic-test-device' > > * wait for SINT's isr's completion > > * clear Hyper-V SynIC evt/msg pages and destroy SINT's routing > > > > Signed-off-by: Andrey Smetanin > > Reviewed-by: Roman Kagan > > Signed-off-by: Denis V. Lunev > > CC: Vitaly Kuznetsov > > CC: "K. Y. Srinivasan" > > CC: Gleb Natapov > > CC: Paolo Bonzini > > CC: Roman Kagan > > CC: Denis V. Lunev > > CC: qemu-devel@nongnu.org > > CC: virtualizat...@lists.linux-foundation.org > > Bad news. > > The test breaks with APICv, because of the following sequence of events: Thanks for testing and analyzing this! (... running around looking for an APICv-capable machine to be able to catch this ourselves before we resubmit ...) > The question then is... does Hyper-V actually use auto-EOI interrupts? > If it doesn't, we might as well not implement them... :/ As Den wrote, we've yet to see a hyperv device which doesn't :( Roman.
Re: [Qemu-devel] [PATCH v3 9/9] kvm/x86: Hyper-V kvm exit
On Tue, Nov 03, 2015 at 03:51:16PM +0100, Paolo Bonzini wrote: > > > On 03/11/2015 15:36, Andrey Smetanin wrote: > >> > >> > >> if I run a patched QEMU but I *do not* enable the synthetic interrupt > >> controller. I can fix it by wrapping the calls to synic_exit with "if > >> (!host)", but I haven't checked yet the source---so that may not be the > >> proper fix. Sorry for not having looked more in detail. > >> > > Could you please specify test case(kvm unit tests ?) and kernel/qemu(if > > it's not standard)? > > It happens just by starting QEMU. > > Kernel: kvm/queue > + kvm/irqchip: kvm_arch_irq_routing_update renaming split > + kvm/x86: split ioapic-handled and EOI exit bitmaps > + kvm/x86: Hyper-V synthetic interrupt controller > + kvm/x86: Hyper-V kvm exit > > QEMU: 3a958f559ecd > + standard-headers/x86: add Hyper-V SynIC constants > + target-i386/kvm: Hyper-V SynIC MSR's support > + linux-headers/kvm: add Hyper-V SynIC irq routing type and struct > + kvm: Hyper-V SynIC irq routing support > + linux-headers/kvm: KVM_EXIT_HYPERV type and struct > + target-i386/hyperv: Hyper-V SynIC SINT routing and vCPU exit > + hw/misc: Hyper-V test device 'hyperv-testdev' > > Can be reproduced just with > "../qemu/+build/x86_64-softmmu/qemu-system-x86_64 --enable-kvm -cpu > kvm64 -display none". Thanks! We've figured it out: qemu initializes the MSRs if has_msr_hv_synic is set, which depends only on whether the kernel supports the MSRs and ignores the cpu property. OTOH setting those MSRs (on the host side) triggers a vcpu exit which checks the cpu property and aborts if it's unset. Voila. This way we also discovered that no error was triggered when the cpu property was set but the kernel didn't support it (and this problem was also present in other hyperv-related features). The solution appears to be to bail out when a hyperv property is requested but the host doesn't support it, and then check for the property only when deciding if the relevant actions need to be taken. Protecting vcpu exits with !host in the kernel seems to make sense, too. We're in progress of preparing the updated patches. Thanks, Roman.
Re: [Qemu-devel] [PATCH v7 3/3] i386: populate floppy drive information in DSDT
On Sun, Feb 07, 2016 at 11:08:07AM +0200, Michael S. Tsirkin wrote: > On Tue, Jan 26, 2016 at 02:50:25PM +0100, Igor Mammedov wrote: > > From: Roman Kagan > > -static Aml *build_fdc_device_aml(void) > > +static Aml *build_fdinfo_aml(int idx, uint8_t type, uint8_t cylinders, > > + uint8_t heads, uint8_t sectors) > > acpi spec says these are WORD values. Are they really uint8_t? Yes, see struct FDrive. > > +fdi = aml_package(0x10); > > Why 0x10 and not 16? I'm unaware of the difference... If you have any preference I'll adjust. (Originally I stuck with hex because so did iasl -d and the then hand-written ASL). > > +aml_append(fdi, aml_int(idx)); /* Drive Number */ > > +aml_append(fdi, > > +aml_int(cmos_get_fd_drive_type(type))); /* Device Type */ > > +aml_append(fdi, > > +aml_int(cylinders - 1)); /* Maximum Cylinder Number */ > > +aml_append(fdi, aml_int(sectors)); /* Maximum Sector Number */ > > +aml_append(fdi, aml_int(heads - 1)); /* Maximum Head Number */ > > Doesn't above change on media change? I guess no, because this IMHO is supposed to describe the drive properties, not the diskette properties. But I'll double-check. I'm confused about the status of this patchset: I saw you post a pull request with this series last week, and now you review it. What should I do now in response to your comments: rework and resubmit it or post incremental fixes on top of it? Thanks, Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Fri, Feb 05, 2016 at 07:25:07PM +0100, Igor Mammedov wrote:
> On Thu, 4 Feb 2016 23:54:13 +0200
> "Michael S. Tsirkin" wrote:
> > -static Aml *build_fdc_device_aml(void)
> > +static Aml *build_fdinfo_aml(int idx, uint8_t type, uint8_t cylinders,
> > + uint8_t heads, uint8_t sectors)
> > +{
> > +Aml *dev, *fdi;
> > +
> > +dev = aml_device("FLP%c", 'A' + idx);
> > +
> > +aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
> > +
> > +fdi = aml_package(0x10);
> > +aml_append(fdi, aml_int(idx)); /* Drive Number */
> > +aml_append(fdi,
> > +aml_int(cmos_get_fd_drive_type(type))); /* Device Type */
> > +aml_append(fdi,
> > +aml_int(cylinders - 1)); /* Maximum Cylinder Number */
> this puts uint64_t(-1) in AML i.e. cylinders == 0 and overflow happens here
>
> CCing Jon
I guess this is the effect of John's fdc rework. I used to think zero
geometry was impossible at the time this patch was developed.
I wonder if it hasn't been fixed already by
commit fd9bdbd3459e5b9d51534f0747049bc5b6145e07
Author: John Snow
Date: Wed Feb 3 11:28:55 2016 -0500
fdc: fix detection under Linux
Accidentally, I removed a "feature" where empty drives had geometry
values applied to them, which allows seek on empty drives to work
"by accident," as QEMU actually tries to disallow that.
Seeks on empty drives should work, though, but the easiest thing is to
restore the misfeature where empty drives have non-zero geometries
applied.
Document the hack accordingly.
[Maintainer edit]
This fix corrects a regression introduced in d5d47efc, where
pick_geometry was modified such that it would not operate on empty
drives, and as a result if there is no diskette inserted, QEMU
no longer populates it with geometry bounds. As a result, seek fails
when QEMU denies to move the current track, but reports success anyway.
This can confuse the guest, leading to kernel panics in the guest.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Message-id: 1454106932-17236-1-git-send-email-js...@redhat.com
Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Mon, Feb 08, 2016 at 03:20:47PM -0500, John Snow wrote: > On 02/08/2016 08:14 AM, Roman Kagan wrote: > > On Fri, Feb 05, 2016 at 07:25:07PM +0100, Igor Mammedov wrote: > >>> +aml_append(fdi, > >>> +aml_int(cylinders - 1)); /* Maximum Cylinder Number */ > >> this puts uint64_t(-1) in AML i.e. cylinders == 0 and overflow happens here > >> > >> CCing Jon > > > > I guess this is the effect of John's fdc rework. I used to think zero > > geometry was impossible at the time this patch was developed. > > > > I wonder if it hasn't been fixed already by > > > > commit fd9bdbd3459e5b9d51534f0747049bc5b6145e07 > > Author: John Snow > > Date: Wed Feb 3 11:28:55 2016 -0500 > > > > fdc: fix detection under Linux > > Yes, hopefully solved on my end. The geometry values for an empty disk > are not well defined (they certainly don't have any *meaning*) so if you > are populating tables based on an empty drive, I just hope you also have > the mechanisms needed to update said tables when the media changes. I don't. At the time the patch was developed there basically were no mechanisms to update the geometry at all (and this was what you patchset addressed, in particular, wasn't it?) so I didn't care. Now if it actually has to be fully dynamic it's gonna be more involved... > What do the guests use these values for? Are they fixed at boot? Only Windows guests use it so it's hard to tell. I can only claim that if I stick bogus values into that ACPI object the guest fails to read the floppy. Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Tue, Feb 09, 2016 at 11:22:01AM -0500, John Snow wrote:
> > I don't. At the time the patch was developed there basically were no
> > mechanisms to update the geometry at all (and this was what you patchset
> > addressed, in particular, wasn't it?) so I didn't care.
>
> That's not true.
>
> You could swap different 1.44MB-class diskettes for other geometries,
> check this out:
>
> static const FDFormat fd_formats[] = {
> /* First entry is default format */
> /* 1.44 MB 3"1/2 floppy disks */
> { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
> { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
> { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
> { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
> { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
> { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
> { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
> { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
> ...
>
> You absolutely could get different sector and track counts before my
> patchset.
Indeed (sorry the patch was developed a couple of months ago so I had to
look at the code to refresh my memory).
However, I tried to implement the part of ACPI spec that read
> 9.9.2 _FDI (Floppy Disk Information)
>
> This object returns information about a floppy disk drive. This
> information is the same as that returned by the INT 13 Function 08H on
> IA-PCs.
so I went ahead and looked into what SeaBIOS did for int 0x13/0x08. And
what it did was read the CMOS at 0x10 and obtain the drive type, and
then return a hardcoded set of parameters (including geometry)
associated to that drive type. So this was what I basically did here,
too. (As a matter of fact the first patch I submitted was just pure ASL
which mimicked exactly the SeaBIOS behavior: read the CMOS and return
the corresponding Package with parameters.)
And IIRC the drive type couldn't change at runtime so I thought I wasn't
doing worse than it was.
As for what to do now, I'll try to check how tolerant the guests are of
changing the floppy geometry under them without updating _FDI, and then
decide.
Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Tue, Feb 09, 2016 at 07:36:12PM +0100, Laszlo Ersek wrote: > In my opinion, the real mess in this case is in the ACPI spec itself. If > you re-read the _FDI control method's description, the Package that it > returns contains *dynamic* geometry data, about the *disk* (not *drive*): > > - Maximum Cylinder Number // Integer (WORD) > - Maximum Sector Number // Integer (WORD) > - Maximum Head Number // Integer (WORD) > > What this seems to require is: the firmware developer should write ACPI > code that > - dynamically accesses the floppy drive / controller (using MMIO or IO > port registers), > - retrieves the geometry of the disk actually inserted, > - and returns the data nicely packaged. > > In effect, an ACPI-level driver for the disk. > > Now, John explained to me (and please keep in mind that this is my > personal account of his explanation, not a factual rendering thereof), > that there used to be no *standard* way to interrogate the current > disk's geometry, other than trial and error with seeking. > > Apparently in UEFI Windows, Microsoft didn't want to implement this > trial and error seeking, so -- given there was also no portable > *hardware spec* to retrieve the same data, directly from the disk drive > or controller -- they punted the entire question to ACPI. That is, to > the firmware implementor. Well, as I wrote in another mail, SeaBIOS, which is supposed to provide the same information to int 0x13/0x08, populates it with static data based only on the drive type as encoded in CMOS at initialization time, and everyone seem to have been fine with that so far. I'll need to re-test it with real Windows guests, though. > IMHO, do the *absolute minimum* to adapt this AML generation patch to > John's FDC rework, and ignore all dynamic aspects (like media change). Hopefully that'll suffice. Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Wed, Feb 10, 2016 at 11:14:30AM -0500, John Snow wrote: > On 02/09/2016 01:48 PM, Michael S. Tsirkin wrote: > > On Tue, Feb 09, 2016 at 07:36:12PM +0100, Laszlo Ersek wrote: > >> Implementing this in QEMU would require: > >> - inventing virt-only registers for the FDC that provide the current > >> disk geometry, > > We do secretly have these registers, but of course there's no spec to > interpreting what they mean. For instance, what do they read when the > drive is empty? I am not sure that information is codified in the ACPI spec. There are various possibilities, like returning False for the corresponding drive in _FDE (Floppy Disk Enumerate) object, or returning 0 aka unknown as drive type in _FDI. Anyway I'd hate needing to expose all of that in an ACPI-accessible form, this is going to become too fat. > Could be wrong, you seemed to indicate that the ACPI spec said that the > info matches what you get from a certain legacy bios routine, but I > don't know the specifics of that routine, either. Indeed, it's supposed to do the same as https://en.wikipedia.org/wiki/INT_13H#INT_13h_AH.3D08h:_Read_Drive_Parameters and, as I wrote in another mail, the SeaBIOS implementation here is rather simplistic. > Roman, does the 0xFF "empty disk geometry" hack appear to work for > Windows 10? > > Maybe it's fine enough as-is, as per Laszlo's good synopsis here. I'll test and let you know. Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Wed, Feb 10, 2016 at 12:16:32PM -0500, John Snow wrote:
> On 02/10/2016 12:10 PM, Roman Kagan wrote:
> > Well, as I wrote in another mail, SeaBIOS, which is supposed to provide
> > the same information to int 0x13/0x08, populates it with static data
> > based only on the drive type as encoded in CMOS at initialization time,
> > and everyone seem to have been fine with that so far. I'll need to
> > re-test it with real Windows guests, though.
> >
>
> OK, but what we appear to be doing currently is polling the current
> geometry values for a drive instead of some pre-chosen ones based on the
> drive type.
>
> What values does SeaBIOS use? (Can you point me to the table it uses?)
src/hw/floppy.c:
struct floppyinfo_s FloppyInfo[] VARFSEG = {
// Unknown
{ {0, 0, 0}, 0x00, 0x00},
// 1 - 360KB, 5.25" - 2 heads, 40 tracks, 9 sectors
{ {2, 40, 9}, FLOPPY_SIZE_525, FLOPPY_RATE_300K},
// 2 - 1.2MB, 5.25" - 2 heads, 80 tracks, 15 sectors
{ {2, 80, 15}, FLOPPY_SIZE_525, FLOPPY_RATE_500K},
// 3 - 720KB, 3.5" - 2 heads, 80 tracks, 9 sectors
{ {2, 80, 9}, FLOPPY_SIZE_350, FLOPPY_RATE_250K},
// 4 - 1.44MB, 3.5" - 2 heads, 80 tracks, 18 sectors
{ {2, 80, 18}, FLOPPY_SIZE_350, FLOPPY_RATE_500K},
// 5 - 2.88MB, 3.5" - 2 heads, 80 tracks, 36 sectors
{ {2, 80, 36}, FLOPPY_SIZE_350, FLOPPY_RATE_1M},
// 6 - 160k, 5.25" - 1 heads, 40 tracks, 8 sectors
{ {1, 40, 8}, FLOPPY_SIZE_525, FLOPPY_RATE_250K},
// 7 - 180k, 5.25" - 1 heads, 40 tracks, 9 sectors
{ {1, 40, 9}, FLOPPY_SIZE_525, FLOPPY_RATE_300K},
// 8 - 320k, 5.25" - 2 heads, 40 tracks, 8 sectors
{ {2, 40, 8}, FLOPPY_SIZE_525, FLOPPY_RATE_250K},
};
The array is indexed by the floppy drive type from CMOS
Roman.
Re: [Qemu-devel] [PULL 48/49] i386: populate floppy drive information in DSDT
On Sun, Feb 14, 2016 at 05:02:14PM +0200, Michael S. Tsirkin wrote: > On Sat, Feb 13, 2016 at 12:26:53PM -0500, Kevin O'Connor wrote: > > On Tue, Feb 09, 2016 at 07:36:12PM +0100, Laszlo Ersek wrote: > > > In my opinion, the real mess in this case is in the ACPI spec itself. If > > > you re-read the _FDI control method's description, the Package that it > > > returns contains *dynamic* geometry data, about the *disk* (not *drive*): > > > > > > - Maximum Cylinder Number // Integer (WORD) > > > - Maximum Sector Number // Integer (WORD) > > > - Maximum Head Number // Integer (WORD) > > > > FWIW, that's not how I read the ACPI specification. I read it as > > saying that the information should be filled with the maximum number > > of CHS that the drive can support. So, even if a smaller disk happens > > to be in the drive the maximum the drive supports would not change. > > I agree. It says: > This object returns information about a floppy disk drive. This > information is the same as > that returned by the INT 13 Function 08H on Intel Architecture PCs. > > So disk drive, not disk information. > > And FWIW I have an old "the undocumented PC" book which says > this about int 13h function 8 (read diskette drive parameters): > > "Remember this data is *not* the limits of the media in the drive, > but the maximum capability of the specified drive". Thanks for the info, I've re-implemented the patchset according to it and Windows guests (w2k12, w10) seem to be happy with it, including dynamically (re-)inserting the "diskette" of compatible size. I'll submit the patches shortly. Thanks, Roman.
[Qemu-devel] [PATCH v8 0/4] i386: expose floppy-related objects in SSDT
Windows on UEFI systems is only capable of detecting the presence and the type of floppy drives via corresponding ACPI objects. Those objects are added in patch 4; the preceding ones pave the way to it, by making the necessary data public and by moving the whole floppy drive controller description into runtime-generated SSDT. Roman Kagan (4): i386/acpi: make floppy controller object dynamic i386: expose floppy drive CMOS type fdc: add function to determine drive chs limits i386: populate floppy drive information in DSDT Signed-off-by: Roman Kagan Cc: Igor Mammedov Cc: "Michael S. Tsirkin" Cc: Marcel Apfelbaum Cc: John Snow Cc: Laszlo Ersek Cc: Kevin O'Connor --- changes since v7: - rebased to latest master - use drive max c,h,s rather than the current diskette geometry hw/block/fdc.c | 23 + hw/i386/acpi-build.c | 92 -- hw/i386/pc.c | 2 +- include/hw/block/fdc.h | 2 ++ include/hw/i386/pc.h | 1 + 5 files changed, 94 insertions(+), 26 deletions(-) -- 2.5.0
[Qemu-devel] [PATCH v8 3/4] fdc: add function to determine drive chs limits
When populating ACPI objects for floppy drives one needs to provide the
maximum values for cylinder, sector, and head number the drive supports.
This patch adds a function that iterates through the array of predefined
floppy drive formats and returns the maximum values of c, h, s, out of
those matching the given floppy drive type.
Signed-off-by: Roman Kagan
Cc: Igor Mammedov
Cc: "Michael S. Tsirkin"
Cc: Marcel Apfelbaum
Cc: John Snow
Cc: Laszlo Ersek
Cc: Kevin O'Connor
---
changes since v7:
- use drive max c,h,s rather than the current diskette geometry
hw/block/fdc.c | 23 +++
include/hw/block/fdc.h | 2 ++
2 files changed, 25 insertions(+)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 9838d21..fc3aef9 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2557,6 +2557,29 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc,
int i)
return isa->state.drives[i].drive;
}
+void isa_fdc_get_drive_max_chs(FloppyDriveType type,
+ uint8_t *maxc, uint8_t *maxh, uint8_t *maxs)
+{
+const FDFormat *fdf;
+
+*maxc = *maxh = *maxs = 0;
+for (fdf = fd_formats; fdf->drive != FLOPPY_DRIVE_TYPE_NONE; fdf++) {
+if (fdf->drive != type) {
+continue;
+}
+if (*maxc < fdf->max_track) {
+*maxc = fdf->max_track;
+}
+if (*maxh < fdf->max_head) {
+*maxh = fdf->max_head;
+}
+if (*maxs < fdf->last_sect) {
+*maxs = fdf->last_sect;
+}
+}
+(*maxc)--;
+}
+
static const VMStateDescription vmstate_isa_fdc ={
.name = "fdc",
.version_id = 2,
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index adce14f..1749dab 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -15,5 +15,7 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
DriveInfo **fds, qemu_irq *fdc_tc);
FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
+void isa_fdc_get_drive_max_chs(FloppyDriveType type,
+ uint8_t *maxc, uint8_t *maxh, uint8_t *maxs);
#endif
--
2.5.0
[Qemu-devel] [PATCH v8 2/4] i386: expose floppy drive CMOS type
Make it possible to query the CMOS type of a floppy drive outside of the
source file where it's defined.
It will allow to properly populate the corresponding ACPI objects and
thus enable Windows on BIOS-less systems to access the floppy drives.
Signed-off-by: Roman Kagan
Cc: Igor Mammedov
Cc: "Michael S. Tsirkin"
Cc: Marcel Apfelbaum
Cc: John Snow
Cc: Laszlo Ersek
Cc: Kevin O'Connor
---
changes since v7:
- split out from a bigger patch
hw/i386/pc.c | 2 +-
include/hw/i386/pc.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0aeefd2..d7fea61 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -200,7 +200,7 @@ static void pic_irq_request(void *opaque, int irq, int
level)
#define REG_EQUIPMENT_BYTE 0x14
-static int cmos_get_fd_drive_type(FloppyDriveType fd0)
+int cmos_get_fd_drive_type(FloppyDriveType fd0)
{
int val;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 8b3546e..472754c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -265,6 +265,7 @@ typedef void (*cpu_set_smm_t)(int smm, void *arg);
void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
ISADevice *pc_find_fdc0(void);
+int cmos_get_fd_drive_type(FloppyDriveType fd0);
/* acpi_piix.c */
--
2.5.0
[Qemu-devel] [PATCH v8 4/4] i386: populate floppy drive information in DSDT
On x86-based systems Linux determines the presence and the type of
floppy drives via a query of a CMOS field. So does SeaBIOS when
populating the return data for int 0x13 function 0x08.
However Windows doesn't do it. Instead, it requests this information
from BIOS via int 0x13/0x08 or through ACPI objects _FDE (Floppy Drive
Enumerate) and _FDI (Floppy Drive Information) of the floppy controller
object. On UEFI systems only ACPI-based detection is supported.
QEMU doesn't provide those objects in its ACPI tables and as a result
floppy drives are invisible to Windows on UEFI/OVMF.
This patch adds those objects to the floppy controller in DSDT,
populating them with the information from respective QEMU objects.
Signed-off-by: Roman Kagan
Cc: Igor Mammedov
Cc: "Michael S. Tsirkin"
Cc: Marcel Apfelbaum
Cc: John Snow
Cc: Laszlo Ersek
Cc: Kevin O'Connor
---
changes since v7:
- use drive max c,h,s rather than the current diskette geometry
- better document the changes in the comments
hw/i386/acpi-build.c | 69 +---
1 file changed, 66 insertions(+), 3 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0213ccf..02bffb2 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -37,6 +37,7 @@
#include "hw/acpi/bios-linker-loader.h"
#include "hw/loader.h"
#include "hw/isa/isa.h"
+#include "hw/block/fdc.h"
#include "hw/acpi/memory_hotplug.h"
#include "hw/mem/nvdimm.h"
#include "sysemu/tpm.h"
@@ -1227,11 +1228,60 @@ static void build_hpet_aml(Aml *table)
aml_append(table, scope);
}
-static Aml *build_fdc_device_aml(void)
+static Aml *build_fdinfo_aml(int idx, FloppyDriveType type)
{
+Aml *dev, *fdi;
+uint8_t maxc, maxh, maxs;
+
+isa_fdc_get_drive_max_chs(type, &maxc, &maxh, &maxs);
+
+dev = aml_device("FLP%c", 'A' + idx);
+
+aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
+
+fdi = aml_package(16);
+aml_append(fdi, aml_int(idx)); /* Drive Number */
+aml_append(fdi,
+aml_int(cmos_get_fd_drive_type(type))); /* Device Type */
+/*
+ * the values below are the limits of the drive, and are thus independent
+ * of the inserted media
+ */
+aml_append(fdi, aml_int(maxc)); /* Maximum Cylinder Number */
+aml_append(fdi, aml_int(maxs)); /* Maximum Sector Number */
+aml_append(fdi, aml_int(maxh)); /* Maximum Head Number */
+/*
+ * SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
+ * the drive type, so shall we
+ */
+aml_append(fdi, aml_int(0xAF)); /* disk_specify_1 */
+aml_append(fdi, aml_int(0x02)); /* disk_specify_2 */
+aml_append(fdi, aml_int(0x25)); /* disk_motor_wait */
+aml_append(fdi, aml_int(0x02)); /* disk_sector_siz */
+aml_append(fdi, aml_int(0x12)); /* disk_eot */
+aml_append(fdi, aml_int(0x1B)); /* disk_rw_gap */
+aml_append(fdi, aml_int(0xFF)); /* disk_dtl */
+aml_append(fdi, aml_int(0x6C)); /* disk_formt_gap */
+aml_append(fdi, aml_int(0xF6)); /* disk_fill */
+aml_append(fdi, aml_int(0x0F)); /* disk_head_sttl */
+aml_append(fdi, aml_int(0x08)); /* disk_motor_strt */
+
+aml_append(dev, aml_name_decl("_FDI", fdi));
+return dev;
+}
+
+static Aml *build_fdc_device_aml(ISADevice *fdc)
+{
+int i;
Aml *dev;
Aml *crs;
+#define ACPI_FDE_MAX_FD 4
+uint32_t fde_buf[5] = {
+0, 0, 0, 0, /* presence of floppy drives #0 - #3 */
+cpu_to_le32(2) /* tape presence (2 == never present) */
+};
+
dev = aml_device("FDC0");
aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
@@ -1243,6 +1293,17 @@ static Aml *build_fdc_device_aml(void)
aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
aml_append(dev, aml_name_decl("_CRS", crs));
+for (i = 0; i < MIN(MAX_FD, ACPI_FDE_MAX_FD); i++) {
+FloppyDriveType type = isa_fdc_get_drive_type(fdc, i);
+
+if (type < FLOPPY_DRIVE_TYPE_NONE) {
+fde_buf[i] = cpu_to_le32(1); /* drive present */
+aml_append(dev, build_fdinfo_aml(i, type));
+}
+}
+aml_append(dev, aml_name_decl("_FDE",
+ aml_buffer(sizeof(fde_buf), (uint8_t *)fde_buf)));
+
return dev;
}
@@ -1387,13 +1448,15 @@ static Aml *build_com_device_aml(uint8_t uid)
static void build_isa_devices_aml(Aml *table)
{
+ISADevice *fdc = pc_find_fdc0();
+
Aml *scope = aml_scope("_SB.PCI0.ISA");
aml_append(scope, build_rtc_device_aml());
aml_append(scope, build_kbd_device_aml());
aml_append(scope, build_mouse_device_aml());
-if (pc_find_fdc0()) {
-aml_append(scope, build_fdc_device_aml());
+if (fdc) {
+aml_append(scope, build_fdc_device_aml(fdc));
}
aml_append(scope, build_lpt_device_aml());
aml_append(scope, build_com_device_aml(1));
--
2.5.0
[Qemu-devel] [PATCH v8 1/4] i386/acpi: make floppy controller object dynamic
Instead of statically declaring the floppy controller in DSDT, with its
_STA method depending on some obscure bit in the parent ISA bridge, add
the object dynamically to DSDT via AML API only when the controller is
present.
The _STA method is no longer necessary and is therefore dropped. So are
the declarations of the fields indicating whether the contoller is
enabled.
Signed-off-by: Roman Kagan
Signed-off-by: Igor Mammedov
Reviewed-by: Marcel Apfelbaum
Cc: "Michael S. Tsirkin"
Cc: John Snow
Cc: Laszlo Ersek
Cc: Kevin O'Connor
---
changes since v7:
- remove unnecessary double negation (cosmetic so keeping s-o-b)
hw/i386/acpi-build.c | 27 +++
1 file changed, 3 insertions(+), 24 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4554eb8..0213ccf 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1231,29 +1231,10 @@ static Aml *build_fdc_device_aml(void)
{
Aml *dev;
Aml *crs;
-Aml *method;
-Aml *if_ctx;
-Aml *else_ctx;
-Aml *zero = aml_int(0);
-Aml *is_present = aml_local(0);
dev = aml_device("FDC0");
aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
-method = aml_method("_STA", 0, AML_NOTSERIALIZED);
-aml_append(method, aml_store(aml_name("FDEN"), is_present));
-if_ctx = aml_if(aml_equal(is_present, zero));
-{
-aml_append(if_ctx, aml_return(aml_int(0x00)));
-}
-aml_append(method, if_ctx);
-else_ctx = aml_else();
-{
-aml_append(else_ctx, aml_return(aml_int(0x0f)));
-}
-aml_append(method, else_ctx);
-aml_append(dev, method);
-
crs = aml_resource_template();
aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
@@ -1411,7 +1392,9 @@ static void build_isa_devices_aml(Aml *table)
aml_append(scope, build_rtc_device_aml());
aml_append(scope, build_kbd_device_aml());
aml_append(scope, build_mouse_device_aml());
-aml_append(scope, build_fdc_device_aml());
+if (pc_find_fdc0()) {
+aml_append(scope, build_fdc_device_aml());
+}
aml_append(scope, build_lpt_device_aml());
aml_append(scope, build_com_device_aml(1));
aml_append(scope, build_com_device_aml(2));
@@ -1780,8 +1763,6 @@ static void build_q35_isa_bridge(Aml *table)
aml_append(field, aml_named_field("COMB", 3));
aml_append(field, aml_reserved_field(1));
aml_append(field, aml_named_field("LPTD", 2));
-aml_append(field, aml_reserved_field(2));
-aml_append(field, aml_named_field("FDCD", 2));
aml_append(dev, field);
aml_append(dev, aml_operation_region("LPCE", AML_PCI_CONFIG,
@@ -1791,7 +1772,6 @@ static void build_q35_isa_bridge(Aml *table)
aml_append(field, aml_named_field("CAEN", 1));
aml_append(field, aml_named_field("CBEN", 1));
aml_append(field, aml_named_field("LPEN", 1));
-aml_append(field, aml_named_field("FDEN", 1));
aml_append(dev, field);
aml_append(scope, dev);
@@ -1839,7 +1819,6 @@ static void build_piix4_isa_bridge(Aml *table)
aml_append(field, aml_reserved_field(3));
aml_append(field, aml_named_field("CBEN", 1));
aml_append(dev, field);
-aml_append(dev, aml_name_decl("FDEN", aml_int(1)));
aml_append(scope, dev);
aml_append(table, scope);
--
2.5.0
Re: [Qemu-devel] [RFC 1/1] nbd (specification): add NBD_CMD_WRITE_ZEROES command
On Wed, Feb 17, 2016 at 01:58:47PM -0700, Eric Blake wrote: > On 02/17/2016 11:10 AM, Denis V. Lunev wrote: > > @@ -446,6 +448,11 @@ The following request types exist: > > about the contents of the export affected by this command, until > > overwriting it again with `NBD_CMD_WRITE`. > > > > +* `NBD_CMD_WRITE_ZEROES` (6) > > + > > +A request to write zeroes. The command is functional equivalent of > > +the NBD_WRITE_COMMAND but without payload sent through the channel. > > This lets us push holes during writes. Do we have the converse > operation, that is, an easy way to query if a block of data will read as > all zeroes, and therefore the client can bypass reading that portion of > the disk (in other words, an equivalent to lseek(SEEK_HOLE/SEEK_DATA))? The spec doesn't have anything like that. OTOH, unlike the write case, where you have all the information and just choose whether to send normal write or zero write, the extra round-trip of a separate SEEK_HOLE/SEEK_DATA request may lead to actually degrading the overall throughput. Rather it may be a better idea to add something like sparse read where the server would, instead of sending the full length of data in the response payload, send a smarter variable-length package with a scatter-gather list or a bitmap of used blocks in the beginning, and let the client decode it and fill the gaps with zeros. Roman.
Re: [Qemu-devel] [PATCH v1 7/7] kvm/x86: Hyper-V SynIC timers
On Wed, Nov 25, 2015 at 06:20:21PM +0300, Andrey Smetanin wrote:
> Per Hyper-V specification (and as required by Hyper-V-aware guests),
> SynIC provides 4 per-vCPU timers. Each timer is programmed via a pair
> of MSRs, and signals expiration by delivering a special format message
> to the configured SynIC message slot and triggering the corresponding
> synthetic interrupt.
>
> Note: as implemented by this patch, all periodic timers are "lazy"
> (i.e. if the vCPU wasn't scheduled for more than the timer period the
> timer events are lost), regardless of the corresponding configuration
> MSR. If deemed necessary, the "catch up" mode (the timer period is
> shortened until the timer catches up) will be implemented later.
>
> Signed-off-by: Andrey Smetanin
> CC: Gleb Natapov
> CC: Paolo Bonzini
> CC: "K. Y. Srinivasan"
> CC: Haiyang Zhang
> CC: Vitaly Kuznetsov
> CC: Roman Kagan
> CC: Denis V. Lunev
> CC: qemu-devel@nongnu.org
> ---
> arch/x86/include/asm/kvm_host.h| 13 ++
> arch/x86/include/uapi/asm/hyperv.h | 6 +
> arch/x86/kvm/hyperv.c | 325
> -
> arch/x86/kvm/hyperv.h | 24 +++
> arch/x86/kvm/x86.c | 9 +
> include/linux/kvm_host.h | 1 +
> 6 files changed, 375 insertions(+), 3 deletions(-)
A couple of nitpicks:
> +static void stimer_restart(struct kvm_vcpu_hv_stimer *stimer)
> +{
> + u64 time_now;
> + ktime_t ktime_now;
> + u64 n;
> +
> + time_now = get_time_ref_counter(stimer_to_vcpu(stimer)->kvm);
> + ktime_now = ktime_get();
> +
> + /*
> + * Calculate positive integer n for which condtion -
> + * (stimer->exp_time + n * stimer->count) > time_now
> + * is true. We will use (stimer->exp_time + n * stimer->count)
> + * as new stimer->exp_time.
> + */
> +
> + n = div64_u64(time_now - stimer->exp_time, stimer->count) + 1;
> + stimer->exp_time += n * stimer->count;
This is actually just a reminder calculation so I'd rather do it
directly with div64_u64_rem().
> +void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
> +{
> + struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
> + struct kvm_vcpu_hv_stimer *stimer;
> + u64 time_now;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(hv_vcpu->stimer); i++)
> + if (test_and_clear_bit(i, hv_vcpu->stimer_pending_bitmap)) {
> + stimer = &hv_vcpu->stimer[i];
> + stimer_stop(stimer);
I think there's no need in this explicit stop: I see no way to arrive
here with a running timer, and even if there were, it would be safe as
your timer callback only manipulates the bitmaps atomically.
Neither comment is critical so
Reviewed-by: Roman Kagan
Roman.
Re: [Qemu-devel] [PATCH v1 6/7] kvm/x86: Hyper-V SynIC message slot pending clearing at SINT ack
On Wed, Nov 25, 2015 at 06:20:20PM +0300, Andrey Smetanin wrote: > The SynIC message protocol mandates that the message slot is claimed > by atomically setting message type to something other than HVMSG_NONE. > If another message is to be delivered while the slot is still busy, > message pending flag is asserted to indicate to the guest that the > hypervisor wants to be notified when the slot is released. > > To make sure the protocol works regardless of where the message > sources are (kernel or userspace), clear the pending flag on SINT ACK > notification, and let the message sources compete for the slot again. > > Signed-off-by: Andrey Smetanin > CC: Gleb Natapov > CC: Paolo Bonzini > CC: "K. Y. Srinivasan" > CC: Haiyang Zhang > CC: Vitaly Kuznetsov > CC: Roman Kagan > CC: Denis V. Lunev > CC: qemu-devel@nongnu.org Reviewed-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH v1 7/7] kvm/x86: Hyper-V SynIC timers
On Fri, Nov 27, 2015 at 11:49:40AM +0100, Paolo Bonzini wrote:
[ sorry missed your message on Friday, replying now ]
> On 27/11/2015 09:12, Roman Kagan wrote:
> >> > +n = div64_u64(time_now - stimer->exp_time, stimer->count) + 1;
> >> > +stimer->exp_time += n * stimer->count;
> > This is actually just a reminder calculation so I'd rather do it
> > directly with div64_u64_rem().
>
> It took me a while to understand why it was a remained. :)
It gets easier if you think of it this way: we've slipped a few whole
periods and the remainder of the slack into the current period, so
the time left till the next tick is ("count" is the timer period here)
delta = count - slack % count
where
slack = time_now - exp_time
This gives you immediately your
> exp_time = time_now + (count - (time_now - exp_time) % count)
Roman.
Re: [Qemu-devel] [PATCH for 2.5? 1/1] DSDT: add floppy-related objects
On Mon, Dec 14, 2015 at 04:05:03PM +0100, Igor Mammedov wrote: > On Mon, 14 Dec 2015 11:22:39 +0300 > "Denis V. Lunev" wrote: > > > From: Roman Kagan > > > > On x86-based systems Linux determines the presence and the type of > > floppy drives via a query of a CMOS field. So does SeaBIOS when > > populating the return data for int 0x13 function 0x08. > > > > Windows doesn't; instead, it requests this information from BIOS via > > int 0x13/0x08 or through ACPI objects _FDE (Floppy Drive Enumerate) > > and _FDI (Floppy Drive Information). On UEFI systems only ACPI-based > > detection is supported. > > > > QEMU used not to provide those objects in its DSDT; as a result floppy > > drives were invisible to Windows on UEFI/OVMF. > > > > This patch implements those objects in ASL, making the ACPI > > interpreter query the CMOS field and populate the objects. The data > > values used for _FDI (which, per ACPI spec, is supposed to be > > equivalent to BIOS int 0x13/0x08) are taken from SeaBIOS. > We are in process of removing static DSDT (ASL template) and replacing > it with dynamically generated one. > So please do not add new ASL to it, instead of it > please redo patch using AML API and it would be better if you > do it on top of following series: > "[PATCH 00/74] pc: acpi: convert DSDT to AML API and drop ASL > templates support" > > http://qemu.patchew.org/series/%3c1449704528-289297-1-git-send-email-imamm...@redhat.com%3E Yes we do know about this effort. However we thought of this patch as a bugfix rather than a new feature so we did it against the current tree, to make it easy both to merge in the master and to backport to the branches so that the distros could pick it up in their packages. Do I get you right that we should better direct this patch to qemu-stable, and cook up a new one on top of your series for master? Roman.
Re: [Qemu-devel] [PATCH v2 1/1] i386: expose floppy-related objects in SSDT
On Wed, Dec 16, 2015 at 05:46:57PM +0100, Igor Mammedov wrote:
> On Wed, 16 Dec 2015 10:45:09 +0300 "Denis V. Lunev"
> wrote:
> > @@ -1125,6 +1183,26 @@ build_ssdt(GArray *table_data, GArray *linker,
> > aml_append(ssdt, scope);
> > }
> >
> > +{
> add this block only if floppy controller is present
ATM both qemu master (and stable branches) and your patchset define FDC0
unconditionally, making its _STA depend on FDEN field of the containing
ISA device rather than any sort of qemu internal state. I think it's
more consistent also to always define its _FDE. The whole definition of
FDC0 can then be made contidional on the presence of the floppy
controller later on top of your patchset.
As for the subdevices representing floppy drives they already are
defined only if present.
> to minimize conflicts with existing series under review,
> I'd suggest to put it before:
>
> if (misc->applesmc_io_base) {
No prob, will do in v2.
Thanks,
Roman.
Re: [Qemu-devel] [PATCH v5 0/6] i386: expose floppy-related objects in SSDT
On Mon, Jan 11, 2016 at 02:51:15PM +0100, Igor Mammedov wrote: > On Wed, 30 Dec 2015 23:11:50 +0300 > Roman Kagan wrote: > > > Windows on UEFI systems is only capable of detecting the presence and > > the type of floppy drives via corresponding ACPI objects. > > > > Those objects are added in patch 5; the preceding ones pave the way to > > it, by making the necessary data public and by moving the whole > > floppy drive controller description into runtime-generated SSDT. > > > > Note that the series conflicts with Igor's patchset for dynamic DSDT, in > > particular, with "[PATCH v2 27/51] pc: acpi: move FDC0 device from DSDT > > to SSDT"; I haven't managed to avoid that while trying to meet > > maintainer's comments. > > > Hello Roman, > > I've rebased/rewrote this series on top of current PCI tree. > Could you tell me if I should keep your Author/SoB on following > patches or change/drop it and if it's the case please specify what > should be changed: > > i386/acpi: make floppy controller object dynamic > > https://github.com/imammedo/qemu/commit/f0a3a4761f8f9698d0f0117d47e2353505de37bf > i386: populate floppy drive information in DSDT > > https://github.com/imammedo/qemu/commit/97578d32e0a0b1cea0b6229f5ef51f8e104b7fdb Both patches look good to me (I just noticed an excessive "to" in the log message of the second one, in "QEMU doesn't _to_ provide those objects in its ACPI tables", you may want to delete it before propagating the patch upstream). Now what are the plans re. stable branches? I think the problem of the floppy being unavaliable in Windows on UEFI/OVMF justifies porting it there (we are interested, in particular, in stable-2.3), but I'm now confused as to what state to use as the base. (As a matter of fact I'd been hoping that my patches made it in before your dynamic DSDT rework so the backport would be trivial cherry-pick; as this is no longer the case I'd appreciate your (or anybody else's) advice on how to move on with stable.) Thanks, Roman.
Re: [Qemu-devel] [PATCH v5 0/6] i386: expose floppy-related objects in SSDT
On Mon, Jan 11, 2016 at 03:47:24PM +0100, Igor Mammedov wrote: > On Mon, 11 Jan 2016 17:26:26 +0300 > Roman Kagan wrote: > > > On Mon, Jan 11, 2016 at 02:51:15PM +0100, Igor Mammedov wrote: > > > I've rebased/rewrote this series on top of current PCI tree. > > > Could you tell me if I should keep your Author/SoB on following > > > patches or change/drop it and if it's the case please specify what > > > should be changed: > > > > > > i386/acpi: make floppy controller object dynamic > > > > > > https://github.com/imammedo/qemu/commit/f0a3a4761f8f9698d0f0117d47e2353505de37bf > > > i386: populate floppy drive information in DSDT > > > > > > https://github.com/imammedo/qemu/commit/97578d32e0a0b1cea0b6229f5ef51f8e104b7fdb > > > > > > > Both patches look good to me (I just noticed an excessive "to" in the > > log message of the second one, in "QEMU doesn't _to_ provide those > > objects in its ACPI tables", you may want to delete it before > > propagating the patch upstream). > Just to confirm, so you are agree with me keeping you as Author on > above patches and your SoB on them as well? Yes, sure. Sorry I haven't made it clear. > > Now what are the plans re. stable branches? I think the problem of the > > floppy being unavaliable in Windows on UEFI/OVMF justifies porting it > > there (we are interested, in particular, in stable-2.3), but I'm now > > confused as to what state to use as the base. > > > > (As a matter of fact I'd been hoping that my patches made it in before > > your dynamic DSDT rework so the backport would be trivial cherry-pick; > > as this is no longer the case I'd appreciate your (or anybody else's) > > advice on how to move on with stable.) > Stable could use reviewed v5 if Michael agrees to take fix. OK thanks! Roman.
Re: [Qemu-devel] [PATCH v6 0/3] i386: expose floppy-related objects in SSDT
On Wed, Jan 13, 2016 at 11:25:22AM +0100, Igor Mammedov wrote: > On Mon, 11 Jan 2016 16:35:31 +0100 > Igor Mammedov wrote: > > > v5->v6: > > - rebased on top DSDT converted to AMP API > > - dropped intermediate structs for one time used floppy parameters > >which simplifies code a bit. > Roman, > I don't have OVMF+Win setup, > Could you test this series and reply with Tested-by if it works? Sure. I just did a brief test booting qemu (master + this series) with RHEL7 OVMF off an installation DVD image for w2k8r2sp1 and w2k12r2, connecting either 1 or 2 floopy drives of different size. Each time the floopies were correctly identified and their contents was visibile in the guest. So Tested-by: Roman Kagan Roman.
Re: [Qemu-devel] [PATCH v5 0/6] i386: expose floppy-related objects in SSDT
On Wed, Jan 13, 2016 at 03:36:18PM +0100, Laszlo Ersek wrote: > On 12/30/15 21:11, Roman Kagan wrote: > > Windows on UEFI systems is only capable of detecting the presence and > > the type of floppy drives via corresponding ACPI objects. > > I'm late to the party, but please allow me a question: > > how did you figure out that UEFI Windows requires this? > > In general, what the ACPI specification says is at best a "guideline" > for Windows. So how did you prove this was a requirement for Windows? Well, my statement above that Windows on UEFI can detect floppies *only* via ACPI is probably a bit stronger than I can actually prove but - Windows on OVMF didn't see floppies before the patch, while Linux did (by querying CMOS) - a number of sources on the internet hinted that Windows needed ACPI assistance for that, e.g.: https://www.reactos.org/wiki/UEFI#Floppy https://social.technet.microsoft.com/Forums/windows/en-US/f17db175-d146-4518-b2e9-c12a15031222/legacy-floppy-compatibility-with-uefi-boot?forum=w7itprohardware https://social.technet.microsoft.com/Forums/windows/en-US/e91ec27b-0c2d-44a3-b949-e77fa810a4c0/windows-7-uefi-fdd-how-to?forum=w7itprohardware - the links mentioned the need in _FDE object but indicated it only allowed for successful enumeration of floppies, not the actual access; I proved that experimentally - the ACPI spec stated that _FDE went in concert with _FDI so I tried it and it worked out Voila. Besides, I later discovered that a similar research had been carried out for Parallels proprietary hypervisor, with a similar outcome. Roman.
Re: [Qemu-devel] [PATCH v5 0/6] i386: expose floppy-related objects in SSDT
On Wed, Jan 13, 2016 at 06:49:44PM +0300, Roman Kagan wrote: > On Wed, Jan 13, 2016 at 03:36:18PM +0100, Laszlo Ersek wrote: > > On 12/30/15 21:11, Roman Kagan wrote: > > > Windows on UEFI systems is only capable of detecting the presence and > > > the type of floppy drives via corresponding ACPI objects. > > > > I'm late to the party, but please allow me a question: > > > > how did you figure out that UEFI Windows requires this? > > > > In general, what the ACPI specification says is at best a "guideline" > > for Windows. So how did you prove this was a requirement for Windows? > > Well, my statement above that Windows on UEFI can detect floppies *only* > via ACPI is probably a bit stronger than I can actually prove but > > - Windows on OVMF didn't see floppies before the patch, while Linux did > (by querying CMOS) > > - a number of sources on the internet hinted that Windows needed ACPI > assistance for that, e.g.: > > https://www.reactos.org/wiki/UEFI#Floppy > > https://social.technet.microsoft.com/Forums/windows/en-US/f17db175-d146-4518-b2e9-c12a15031222/legacy-floppy-compatibility-with-uefi-boot?forum=w7itprohardware > > https://social.technet.microsoft.com/Forums/windows/en-US/e91ec27b-0c2d-44a3-b949-e77fa810a4c0/windows-7-uefi-fdd-how-to?forum=w7itprohardware > > - the links mentioned the need in _FDE object but indicated it only > allowed for successful enumeration of floppies, not the actual access; > I proved that experimentally > > - the ACPI spec stated that _FDE went in concert with _FDI so I tried it > and it worked out > > Voila. Besides, I later discovered that a similar research had been > carried out for Parallels proprietary hypervisor, with a similar > outcome. Ah, I wish I saw your comment https://bugzilla.redhat.com/show_bug.cgi?id=1212317#c5 I'd probably proceed directly to trying _FDI :) Roman.
Re: [Qemu-devel] [PATCH v1 4/5] kvm/x86: Hyper-V VMBus hypercall userspace exit
On Thu, Jan 14, 2016 at 11:30:43AM +0300, Pavel Fedin wrote:
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -3359,6 +3359,14 @@ Hyper-V SynIC state change. Notification is used to
> > remap SynIC
> > event/message pages and to enable/disable SynIC messages/events processing
> > in userspace.
> >
> > + /* KVM_EXIT_HYPERV_HCALL */
> > + struct {
> > + __u64 input;
> > + __u64 params[2];
> > + __u64 result;
> > + } hv_hcall;
> > +Indicates that the VCPU exits into userspace to process some guest
> > +Hyper-V hypercalls.
>
> Why introducing this? We already have KVM_EXIT_HYPERCALL, which is exactly
> the same. AFAIK it's not used at the moment.
> Additionally, in theory we could have hypercalls handled in userspace for
> something else except HyperV. And not only for x86.
We thought reusing KVM_EXIT_HYPERCALL was a bad idea exactly because of
that. Hypercalls are not universal, the calling and return conventions
are hypervisor-specific. KVM already has to make the decision that the
particular vmexit is a HyperV hypercall; it appears unnatural to then
pass the data on to userspace in a generic structure and have them make
that decision again.
Roman.
Re: [Qemu-devel] [PATCH v1 4/5] kvm/x86: Hyper-V VMBus hypercall userspace exit
On Thu, Jan 14, 2016 at 01:50:20PM +0300, Pavel Fedin wrote: > > We thought reusing KVM_EXIT_HYPERCALL was a bad idea exactly because of > > that. Hypercalls are not universal, the calling and return conventions > > are hypervisor-specific. > > Treatment of them is hypervisor-specific, but from CPUs point of view they > are the same. You load something into registers, and > execute hypercall instruction. ... and you get something in the registers on return (all that is from guest POV). Right. The differences are, however, which registers are used for parameters and return values, how they are encoded, etc. > So, you just need to pass registers in your structure. Which ones? This is going to be hypervisor- and arch-specific. > Or, you could even use generic register access APIs. With an extra ioctl? This is going to be expensive. > > KVM already has to make the decision that the > > particular vmexit is a HyperV hypercall; it appears unnatural to then > > pass the data on to userspace in a generic structure and have them make > > that decision again. > > Is it so difficult to make such a decision? The userland already knows what > we are emulating. It isn't terribly difficult, but KVM has to do all that already, as it handles some of the hypercalls in the kernel. I don't think duplicating this (and also designing a generic marshalling scheme) is profitable compared to saving a vcpu exit code. > I'm afraid that in future we can end up in having 10 versions of > KVM_EXIT_xxx_HYPERCALL with very small difference between them. > Will it be good? I don't see it to be a problem. Roman.
Re: [Qemu-devel] [PATCH v1] kvm/x86: Hyper-V tsc page setup
On Wed, Jan 20, 2016 at 05:44:49PM +0300, Denis V. Lunev wrote: > On 01/20/2016 05:05 PM, Paolo Bonzini wrote: > > > >On 19/01/2016 08:48, Denis V. Lunev wrote: > >>>diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > >>>index 6877b4d7e..93c9e25 100644 > >>>--- a/include/linux/kvm_host.h > >>>+++ b/include/linux/kvm_host.h > >>>@@ -145,6 +145,7 @@ static inline bool is_error_page(struct page *page) > >>> #define KVM_REQ_HV_RESET 29 > >>> #define KVM_REQ_HV_EXIT 30 > >>> #define KVM_REQ_HV_STIMER 31 > >>>+#define KVM_REQ_HV_TSC_PAGE 32 > >>> #define KVM_REQ_MAX 64 > >>ping > >Applied with this change: > > > >diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > >index d7716c6e2752..047c275717d3 100644 > >--- a/arch/x86/kvm/hyperv.c > >+++ b/arch/x86/kvm/hyperv.c > >@@ -842,7 +842,7 @@ int kvm_hv_setup_tsc_page(struct kvm_vcpu *vcpu) > >tsc_khz, tsc, tsc_scale, tsc_offset); > > tsc_ref.tsc_sequence++; > >-if (tsc_ref.tsc_sequence == 0) > >+if (tsc_ref.tsc_sequence == 0x tsc_ref.tsc_sequence == 0) > > tsc_ref.tsc_sequence = 1; > > tsc_ref.tsc_scale = tsc_scale; > > > >and renumbering KVM_REQ_HV_TSC_PAGE from 32 to 31. > > > >Paolo > the latter does not seem to be correct to me. > MS spec has bug, see this thread: >https://lkml.org/lkml/2015/11/2/655 > and the change was Acked-by: K.Y. What happens in the patched code is this: when the sequence counter wraps around and hits an invalid value, skip to the next valid one. I think Paolo wanted to be compatible not only with the de facto implementation in Windows Hyper-V guests, but also with the Linux driver which followed the buggy spec and thought 0x to be invalid. So the change is fine (as long as there's || rather than a space there ;) Thanks, Roman.
Re: [Qemu-devel] [PATCH v1 4/5] kvm/x86: Hyper-V VMBus hypercall userspace exit
On Wed, Jan 20, 2016 at 02:59:08PM +0100, Paolo Bonzini wrote:
> >> --- a/Documentation/virtual/kvm/api.txt
> >> +++ b/Documentation/virtual/kvm/api.txt
> >> @@ -3359,6 +3359,14 @@ Hyper-V SynIC state change. Notification is used to
> >> remap SynIC
> >> event/message pages and to enable/disable SynIC messages/events processing
> >> in userspace.
> >>
> >> + /* KVM_EXIT_HYPERV_HCALL */
> >> + struct {
> >> + __u64 input;
> >> + __u64 params[2];
> >> + __u64 result;
> >> + } hv_hcall;
> >> +Indicates that the VCPU exits into userspace to process some guest
> >> +Hyper-V hypercalls.
> >
> > Why introducing this? We already have KVM_EXIT_HYPERCALL, which is
> > exactly the same. AFAIK it's not used at the moment.
> > Additionally, in theory we could have hypercalls handled in
> > userspace for something else except HyperV. And not only for x86.
>
> Because, as the docs say, we don't want to do that. We want to use
> KVM_EXIT_IO or KVM_EXIT_MMIO, with two exceptions: s390 and wherever we
> can't do that for compatibility purposes.
I must admit I saw this part in the docs but failed to understand it.
Is it supposed to mean that we want to avoid using hypercalls as a means
of guest communications with the host userspace in general, and use PIO
or MMIO instead, unless hypercalls are mandated by a guest implemenation
we can't affect (as is the case for Hyper-V)?
Because *implementing* Hyper-V hypercalls in terms of KVM_EXIT_IO or
KVM_EXIT_MMIO looked hard at best.
> So we should not add a new exit
Why? VCPU exit codes are not a scarse resource.
> (I suggested elsewhere the existing hyper-v exit)
So far we've envisaged two reasons for VCPU exit related to hyper-v: one
for hyper-v MSRs and the other for hypercalls. Since there was a
discussion on implementing generic MSR access by Peter we thought it
wiser to introduce a new VCPU exit for hyper-v hypercalls to avoid
interfering with the MSR implementation.
Thanks,
Roman.
Re: [Qemu-devel] [PATCH v1 4/5] kvm/x86: Hyper-V VMBus hypercall userspace exit
On Wed, Jan 20, 2016 at 02:43:01PM +0100, Paolo Bonzini wrote:
>
>
> On 12/01/2016 11:50, Andrey Smetanin wrote:
> > The patch implements KVM_EXIT_HV_HCALL functionality
> > for Hyper-V VMBus hypercalls: HV_X64_HCALL_POST_MESSAGE,
> > HV_X64_HCALL_SIGNAL_EVENT.
> >
> > Signed-off-by: Andrey Smetanin
> > Reviewed-by: Roman Kagan
> > CC: Gleb Natapov
> > CC: Paolo Bonzini
> > CC: Joerg Roedel
> > CC: "K. Y. Srinivasan"
> > CC: Haiyang Zhang
> > CC: Roman Kagan
> > CC: Denis V. Lunev
> > CC: qemu-devel@nongnu.org
> > ---
> > Documentation/virtual/kvm/api.txt | 8
> > arch/x86/kvm/hyperv.c | 28 +---
> > arch/x86/kvm/hyperv.h | 1 +
> > arch/x86/kvm/x86.c| 3 +++
> > include/uapi/linux/kvm.h | 7 +++
> > 5 files changed, 40 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/virtual/kvm/api.txt
> > b/Documentation/virtual/kvm/api.txt
> > index 053f613..23d4b9d 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -3359,6 +3359,14 @@ Hyper-V SynIC state change. Notification is used to
> > remap SynIC
> > event/message pages and to enable/disable SynIC messages/events processing
> > in userspace.
> >
> > + /* KVM_EXIT_HYPERV_HCALL */
> > + struct {
> > + __u64 input;
> > + __u64 params[2];
> > + __u64 result;
> > + } hv_hcall;
> > +Indicates that the VCPU exits into userspace to process some guest
> > +Hyper-V hypercalls.
>
> Is this something other than KVM_EXIT_HYPERV because of the previous
> discussion about MSRs? Otherwise, it definitely should be a separate
> type of the existing KVM_EXIT_HYPERV exit.
Mostly. Besides, it wasn't obvious why we needed another level of
dispatch.
> (In fact, I do not think anymore that I will add the MSR exit in 4.5.
> It's kind of against the KVM design to do "CPU things" outside the
> kernel. My conjecture is that MSRs are either simple and thus not
> security sensitive, or complicated and thus require state that resides
> in the kernel).
>
> > /* Fix the size of the union. */
> > char padding[256];
> > };
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 0e7c90f..76c9ec4 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -1087,18 +1087,32 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> > case HV_X64_HCALL_NOTIFY_LONG_SPIN_WAIT:
> > kvm_vcpu_on_spin(vcpu);
> > break;
> > + case HV_X64_HCALL_POST_MESSAGE:
> > + case HV_X64_HCALL_SIGNAL_EVENT:
> > + vcpu->run->exit_reason = KVM_EXIT_HYPERV_HCALL;
> > + vcpu->run->hv_hcall.input = param;
> > + vcpu->run->hv_hcall.params[0] = ingpa;
> > + vcpu->run->hv_hcall.params[1] = outgpa;
> > + return 0;
> > default:
> > res = HV_STATUS_INVALID_HYPERCALL_CODE;
> > break;
> > }
> >
> > ret = res | (((u64)rep_done & 0xfff) << 32);
> > - if (longmode) {
> > - kvm_register_write(vcpu, VCPU_REGS_RAX, ret);
> > - } else {
> > - kvm_register_write(vcpu, VCPU_REGS_RDX, ret >> 32);
> > - kvm_register_write(vcpu, VCPU_REGS_RAX, ret & 0x);
> > - }
> > -
> > + kvm_hv_hypercall_set_result(vcpu, ret);
> > return 1;
> > }
> > +
> > +void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
> > +{
> > + bool longmode;
> > +
> > + longmode = is_64_bit_mode(vcpu);
> > + if (longmode)
> > + kvm_register_write(vcpu, VCPU_REGS_RAX, result);
> > + else {
> > + kvm_register_write(vcpu, VCPU_REGS_RDX, result >> 32);
> > + kvm_register_write(vcpu, VCPU_REGS_RAX, result & 0x);
> > + }
> > +}
> > diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
> > index 60eccd4..64a4a3b 100644
> > --- a/arch/x86/kvm/hyperv.h
> > +++ b/arch/x86/kvm/hyperv.h
> > @@ -52,6 +52,7 @@ int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 msr,
> > u64 *pdata);
> >
> > bool kvm_hv_hypercall_enabled(struct kvm *kvm);
> > int kvm_hv_hypercall(struct kvm_vcpu *vcpu);
> > +void kvm_hv_hypercall_set_result(struct kvm_vcpu *vc
Re: [Qemu-devel] [PATCH v1 4/5] kvm/x86: Hyper-V VMBus hypercall userspace exit
On Wed, Jan 20, 2016 at 06:02:25PM +0100, Paolo Bonzini wrote: > > > On 20/01/2016 16:20, 'Roman Kagan' wrote: > >> > So we should not add a new exit > > Why? VCPU exit codes are not a scarse resource. > > Indeed, but grouping makes things easier to understand. > > > So far we've envisaged two reasons for VCPU exit related to hyper-v: one > > for hyper-v MSRs and the other for hypercalls. Since there was a > > discussion on implementing generic MSR access by Peter we thought it > > wiser to introduce a new VCPU exit for hyper-v hypercalls to avoid > > interfering with the MSR implementation. > > That's a good idea. However, I think I'm not going to accept the MSR > exit feature, and then the current Hyper-V exit API makes some sense > indeed (it's just 3 values, transferring them all at once is not > expensive at all). OK can we please sum up (as I'm now a bit confused) what we do now: 1) use a single vcpu exit for both Hyper-V cases (which implies we need to fix this patchset to add the subcode for hypercalls) or 2) use individual vcpu exits for Hyper-V MSRs and for Hyper-V hypercalls (which implies we need to submit an incremental patch dropping the subcode from Hyper-V MSR exit and renaming it to better describe the reality)? Thanks, Roman.
Re: [Qemu-devel] [Qemu-block] [PATCH v4 00/12] fdc: fix 2.88mb floppy diskette support
On Wed, Jan 20, 2016 at 02:40:14PM -0500, John Snow wrote: > On 01/20/2016 02:55 AM, Denis V. Lunev wrote: > > should we recreate ACPI tables after geometry switch? > > This would be especially interesting for the case of > > Win2k12 (or Win8.1 if you prefer) under OVMF. > > > > Den > > This series doesn't really alter the concept that disk geometry can > change at runtime -- Not knowing much about the ACPI reverse engineering > that happened to make Windows 8/10 happy, does it work currently? Can > you change to different density floppies and have it work out alright? No, exactly because the geometry is determined once startup. > If not, you can submit a patch against master as it is today -- this > series only does two things: > > (1) Alters the heuristics for which type of floppy drive is chosen at > boot time (No change to ACPI table generation should be needed.) > > (2) Allows 1.44MB diskettes to be recognized by 2.88MB drive types. This > might require some changes, but check out pick_geometry both before and > after this patchset -- there's a whole table of different geometries > that we already allow users to switch between at runtime. If the > geometry needs to update there, too, then it's already broken before > this patchset. Right. This series conflicts slightly with the patches to generate ACPI objects for floppies (which haven't made it into the mainstream qemu yet) because of the adjustments in the floppy API. Not a big deal. > It should be easy enough to slide a geometry update in fd_revalidate() > if needed. Now that is a bit trickier: the currently submitted code queries the floppy properties at SSDT build time, and sticks static objects into AML; if that really needs updating at runtime it'll require certain refactoring. That said I'm not certain what exactly has to be done here. Physical machines do not have their floppy drives changable at runtime, do they? So the OSes should be fine assuming that the drive stays the same, and it's only the diskette that can change. I'd guess that the OS driver should do the necessary revalidation on its own, without ACPI assistance; I'll give it a try when I have some time. But again, as you said, people are mainly interested in floppies to bootstrap a Windows installation on virtio disks, so support of floppy geometry update at runtime is non-critical for most users. Thanks, Roman.
Re: [Qemu-devel] [Qemu-block] [PATCH v4 00/12] fdc: fix 2.88mb floppy diskette support
On Thu, Jan 21, 2016 at 09:59:09AM -0500, John Snow wrote: > > Now that is a bit trickier: the currently submitted code queries the > > floppy properties at SSDT build time, and sticks static objects into > > AML; if that really needs updating at runtime it'll require certain > > refactoring. > > > > That said I'm not certain what exactly has to be done here. Physical > > machines do not have their floppy drives changable at runtime, do they? > > So the OSes should be fine assuming that the drive stays the same, and > > it's only the diskette that can change. I'd guess that the OS driver > > should do the necessary revalidation on its own, without ACPI > > assistance; I'll give it a try when I have some time. > > > > But again, as you said, people are mainly interested in floppies to > > bootstrap a Windows installation on virtio disks, so support of floppy > > geometry update at runtime is non-critical for most users. > > I'm a little confused here. I am not proposing the ability to change a > floppy drive type at runtime, just the ability to insert different kinds > of diskettes, which does happen in the real world. > > e.g. a 2.88MB capable floppy drive that can read either 1.44MB or 2.88MB > types. That's right; the question WRT ACPI is whether the Windows driver expects the drive parameters or the currently inserted diskette parameters in the ACPI object; I guess the former, so we should be OK with that data generated once at startup. Roman.
Re: [Qemu-devel] [PATCH v2 1/1] i386: expose floppy-related objects in SSDT
On Wed, Dec 16, 2015 at 11:15:55PM +0100, Igor Mammedov wrote:
> On Wed, 16 Dec 2015 20:34:55 +0300
> Roman Kagan wrote:
>
> > On Wed, Dec 16, 2015 at 05:46:57PM +0100, Igor Mammedov wrote:
> > > On Wed, 16 Dec 2015 10:45:09 +0300 "Denis V. Lunev"
> > > wrote:
> > > > @@ -1125,6 +1183,26 @@ build_ssdt(GArray *table_data, GArray
> > > > *linker, aml_append(ssdt, scope);
> > > > }
> > > >
> > > > +{
> > > add this block only if floppy controller is present
> >
> > ATM both qemu master (and stable branches) and your patchset define
> > FDC0 unconditionally, making its _STA depend on FDEN field of the
> > containing ISA device rather than any sort of qemu internal state. I
> > think it's more consistent also to always define its _FDE. The whole
> > definition of FDC0 can then be made contidional on the presence of
> > the floppy controller later on top of your patchset.
> Ok, I don't insist on it as I'll have to do anyway for whole FDC0 after
> merging SSDT into DSDT.
>
> The reason why I haven't done it in conversion patchset, is to keep
> DSDT exactly the same as an original one, so 'make check' could prove
> conversion correctness.
I just realized that bios-tables-test used to report differences between
the expected and the actual DSDT and SSDT even before the patch but that
would only cause warnings and not a test failure. With the patch
applied (which adds yet another difference) nothing changes: it still
reports warnings but passes.
Roman.
Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit
On Fri, Dec 18, 2015 at 06:19:25PM +0300, Pavel Fedin wrote: > I dislike implementing architecture-dependent exit code where we could > implement an architecture-independent one. > > As far as i understand this code, KVM_EXIT_HYPERV is called when one of > three MSRs are accessed. But, shouldn't we have implemented > instead something more generic, like KVM_EXIT_REG_IO, which would work > similar to KVM_EXIT_PIO or KVM_EXIT_MMIO, but carry register > code and value? > > This would allow us to solve the same task which we have done here, but this > solution would be reusable for other devices and other > archirectures. What if in future we have more system registers to emulate in > userspace? > > I write this because at one point i suggested similar thing for ARM64 (but i > never actually wrote it), to emulate physical CP15 > timer. And it would require exactly the same capability - process some > trapped system register accesses in userspace. Note that, as Paolo pointed out, in case of HyperV the value of the register is of interest not only to the userspace but to the KVM, too. Otherwise I don't see off hand why a generic infrastructure wouldn't fit in our usecase. Roman.
Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit
On Fri, Dec 18, 2015 at 05:01:59PM +0100, Paolo Bonzini wrote: > On 18/12/2015 16:19, Pavel Fedin wrote: > > As far as i understand this code, KVM_EXIT_HYPERV is called when one > > of three MSRs are accessed. But, shouldn't we have implemented > > instead something more generic, like KVM_EXIT_REG_IO, which would > > work similar to KVM_EXIT_PIO or KVM_EXIT_MMIO, but carry register > > code and value? > > Yes, we considered that. There were actually patches for this as well. > However, in this case the register is still emulated in the kernel, and > userspace just gets informed of the new value. > > > This would allow us to solve the same task which we have done here, > > but this solution would be reusable for other devices and other > > archirectures. What if in future we have more system registers to > > emulate in userspace? > > If we do get that, we will just rename KVM_EXIT_HYPERV to > KVM_EXIT_MSR_ACCESS, and KVM_EXIT_HYPERV_SYNIC to > KVM_EXIT_MSR_HYPERV_SYNIC, and struct kvm_hyperv_exit to kvm_msr_exit. A generic implemenation will probably just convey (msr#, value) pair, and KVM_EXIT_MSR_HYPERV_SYNIC wouldn't be needed at all. I don't immediately see why it wouldn't work for us; we'd have reused the infrastructure if it existed when we started our work. I didn't see Peter's patches yet; maybe we can come up with an interim solution to fit in the merge window but expose a sufficiently generic API. Roman.
Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit
On Fri, Dec 18, 2015 at 10:10:11AM -0800, Peter Hornyack wrote: > On Fri, Dec 18, 2015 at 8:01 AM, Paolo Bonzini wrote: > > On 18/12/2015 16:19, Pavel Fedin wrote: > >> As far as i understand this code, KVM_EXIT_HYPERV is called when one > >> of three MSRs are accessed. But, shouldn't we have implemented > >> instead something more generic, like KVM_EXIT_REG_IO, which would > >> work similar to KVM_EXIT_PIO or KVM_EXIT_MMIO, but carry register > >> code and value? > > > > Yes, we considered that. There were actually patches for this as well. > > However, in this case the register is still emulated in the kernel, and > > userspace just gets informed of the new value. > > On brief inspection of Andrey's patch (I have not been following > closely) it looks like the kvm_hyperv_exit struct that's returned to > userspace contains more data (control, evt_page, and msg_page fields) > than simply the value of the MSR, so would the desired SynIC exit fit > into a general-purpose exit for MSR emulation? Frankly I'm struggling trying to recall why we implemented it this way. Actually all three fields are the values of respective MSRs and I don't see any necessity to pass all three at the same time when any of them gets updated. The patch for QEMU adds an exit handler which processes the fields individually, so I have a strong suspicion that union was meant here rather than struct. I hope Andrey will help to shed some light on that when he's back in the office on Monday; meanwhile I think this peculiarity can be ignored. Roman.
[Qemu-devel] [PATCH v3 0/2] i386: expose floppy-related objects in SSDT
Roman Kagan (2): i386: expose floppy-related objects in SSDT tests: update expected SSDT for floppy changes Signed-off-by: Roman Kagan Signed-off-by: Denis V. Lunev CC: Michael S. Tsirkin CC: Igor Mammedov CC: Paolo Bonzini CC: Richard Henderson CC: Eduardo Habkost CC: John Snow CC: Kevin Wolf --- changes since v2: - explicit endianness for buffer data - reorder code to reduce conflicts with dynamic DSDT patchset - update test data hw/block/fdc.c | 11 + hw/i386/acpi-build.c | 78 +++ hw/i386/pc.c | 46 - include/hw/block/fdc.h | 2 + include/hw/i386/pc.h | 3 ++ tests/acpi-test-data/pc/SSDT | Bin 2486 -> 2588 bytes tests/acpi-test-data/pc/SSDT.bridge | Bin 4345 -> 4447 bytes tests/acpi-test-data/q35/SSDT| Bin 691 -> 872 bytes tests/acpi-test-data/q35/SSDT.bridge | Bin 708 -> 889 bytes 9 files changed, 121 insertions(+), 19 deletions(-) -- 2.5.0
[Qemu-devel] [PATCH v3 2/2] tests: update expected SSDT for floppy changes
Update the expected SSDTs to reflect the changes introduced in the
previous patch.
Signed-off-by: Roman Kagan
Signed-off-by: Denis V. Lunev
CC: Michael S. Tsirkin
CC: Igor Mammedov
CC: Paolo Bonzini
CC: Richard Henderson
CC: Eduardo Habkost
CC: John Snow
CC: Kevin Wolf
---
changes since v2:
- new patch
tests/acpi-test-data/pc/SSDT | Bin 2486 -> 2588 bytes
tests/acpi-test-data/pc/SSDT.bridge | Bin 4345 -> 4447 bytes
tests/acpi-test-data/q35/SSDT| Bin 691 -> 872 bytes
tests/acpi-test-data/q35/SSDT.bridge | Bin 708 -> 889 bytes
4 files changed, 0 insertions(+), 0 deletions(-)
diff --git a/tests/acpi-test-data/pc/SSDT b/tests/acpi-test-data/pc/SSDT
index
210d6a71e58aa34ce8e94121d25bcf58c3bd503c..ba3fa272b8235bdea28eadb0eaa3751f1ef4f59a
100644
GIT binary patch
delta 123
zcmdlcJV%5pIM^jbhKqrLQFJ3$G-Hx0TZ}$Se6Uk|fU~E8XRu?un~SqSbd#Z*PkLOY0aE2ED9$Cq$bbr%
KHYYNMasU8QB^ERQ
delta 24
gcmbOuvQ3yPIM^j*8z%z;WJQG-Hx0TZ}$Se6Uk|fU~E8XRu?un~SqSbd#Z*PkLOY0aE2ED9$Cq$bbr%
KHYYO9;0FNT%ono&
delta 24
gcmcbw^iz>5IM^lRrvL*3qryh6XvWQ_8K>|A0BC0i#{d8T
diff --git a/tests/acpi-test-data/q35/SSDT b/tests/acpi-test-data/q35/SSDT
index
0970c67ddb1456707c0111f7e5e659ef385af408..7d12c6a562c6a7d14099d4819ab469797548549d
100644
GIT binary patch
delta 203
zcmdnY`htxsIM^j5gPDPWaoI*Le#Uwi?ihWR_+Y2_0B27F&tS)RHy3Av=q7tNp8!XW
zct@8Y1`eQ*r;wfi0~ZV5e<)ypv$)oCF>$E^u@ILu*MF`Yu5VoYTpSPsoWKS!!VF-<
gVt~>A|JY3cX>t`5=MrIL0J;^3VSs6~DC0av01bIR_5c6?
delta 24
gcmaFCwwaYHIM^j*GZO;?W9>#Re#Xt`7-um809z#o3IG5A
diff --git a/tests/acpi-test-data/q35/SSDT.bridge
b/tests/acpi-test-data/q35/SSDT.bridge
index
a77868861754ce0bb2b7bc8091c03a53754d..f5c4b44b5c0f8a049f321c5aadcc825c261ba99f
100644
GIT binary patch
delta 203
zcmX@Y`jd?-IM^kml9_>l(QP9aKV!WMcZ@zue6Uk|fU~E8XRu?un~SqSbd$ZCPkpxcx*EcSHE)IwRPGAEVVFoZ_
gF~I2mf9xiJG`R|jbBQoA0No12Fu=4~lyN;H0PbQx+5i9m
delta 24
gcmey#c7&BHIM^lR2onPXqwGd5e#Xt`7*{g_09%v>?f?J)
--
2.5.0
[Qemu-devel] [PATCH v3 1/2] i386: expose floppy-related objects in SSDT
On x86-based systems Linux determines the presence and the type of
floppy drives via a query of a CMOS field. So does SeaBIOS when
populating the return data for int 0x13 function 0x08.
Windows doesn't; instead, it requests this information from BIOS via int
0x13/0x08 or through ACPI objects _FDE (Floppy Drive Enumerate) and _FDI
(Floppy Drive Information). On UEFI systems only ACPI-based detection
is supported.
QEMU used not to provide those objects in its ACPI tables; as a result
floppy drives were invisible to Windows on UEFI/OVMF.
This patch implements those objects in SSDT, populating them via AML
API. For that, a couple of functions are extern-ified to facilitate
populating those objects in acpi-build.c.
Signed-off-by: Roman Kagan
Signed-off-by: Denis V. Lunev
CC: Michael S. Tsirkin
CC: Igor Mammedov
CC: Paolo Bonzini
CC: Richard Henderson
CC: Eduardo Habkost
CC: John Snow
CC: Kevin Wolf
--
changes since v2:
- explicit endianness for buffer data
- reorder code to reduce conflicts with dynamic DSDT patchset
hw/block/fdc.c | 11 +++
hw/i386/acpi-build.c | 78 ++
hw/i386/pc.c | 46 +
include/hw/block/fdc.h | 2 ++
include/hw/i386/pc.h | 3 ++
5 files changed, 121 insertions(+), 19 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4292ece..c858c5f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2408,6 +2408,17 @@ FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
return isa->state.drives[i].drive;
}
+void isa_fdc_get_drive_geometry(ISADevice *fdc, int i, uint8_t *cylinders,
+uint8_t *heads, uint8_t *sectors)
+{
+FDCtrlISABus *isa = ISA_FDC(fdc);
+FDrive *drv = &isa->state.drives[i];
+
+*cylinders = drv->max_track;
+*heads = (drv->flags & FDISK_DBL_SIDES) ? 2 : 1;
+*sectors = drv->last_sect;
+}
+
static const VMStateDescription vmstate_isa_fdc ={
.name = "fdc",
.version_id = 2,
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index fa866f6..15e50fb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -38,6 +38,7 @@
#include "hw/acpi/bios-linker-loader.h"
#include "hw/loader.h"
#include "hw/isa/isa.h"
+#include "hw/block/fdc.h"
#include "hw/acpi/memory_hotplug.h"
#include "sysemu/tpm.h"
#include "hw/acpi/tpm.h"
@@ -105,6 +106,13 @@ typedef struct AcpiPmInfo {
uint16_t pcihp_io_len;
} AcpiPmInfo;
+typedef struct AcpiFDInfo {
+uint8_t type;
+uint8_t cylinders;
+uint8_t heads;
+uint8_t sectors;
+} AcpiFDInfo;
+
typedef struct AcpiMiscInfo {
bool has_hpet;
TPMVersion tpm_version;
@@ -112,6 +120,7 @@ typedef struct AcpiMiscInfo {
unsigned dsdt_size;
uint16_t pvpanic_port;
uint16_t applesmc_io_base;
+AcpiFDInfo fdinfo[2];
} AcpiMiscInfo;
typedef struct AcpiBuildPciBusHotplugState {
@@ -235,10 +244,25 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
static void acpi_get_misc_info(AcpiMiscInfo *info)
{
+int i;
+ISADevice *fdc;
+
info->has_hpet = hpet_find();
info->tpm_version = tpm_get_version();
info->pvpanic_port = pvpanic_port();
info->applesmc_io_base = applesmc_port();
+
+fdc = pc_find_fdc0();
+if (fdc) {
+for (i = 0; i < ARRAY_SIZE(info->fdinfo); i++) {
+AcpiFDInfo *fdinfo = &info->fdinfo[i];
+fdinfo->type = isa_fdc_get_drive_type(fdc, i);
+if (fdinfo->type < FDRIVE_DRV_NONE) {
+isa_fdc_get_drive_geometry(fdc, i, &fdinfo->cylinders,
+ &fdinfo->heads, &fdinfo->sectors);
+}
+}
+}
}
/*
@@ -900,6 +924,40 @@ static Aml *build_crs(PCIHostState *host,
return crs;
}
+static Aml *build_fdinfo_aml(int idx, AcpiFDInfo *fdinfo)
+{
+Aml *dev, *fdi;
+
+dev = aml_device("FLP%c", 'A' + idx);
+
+aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
+
+fdi = aml_package(0x10);
+aml_append(fdi, aml_int(idx)); /* Drive Number */
+aml_append(fdi,
+aml_int(cmos_get_fd_drive_type(fdinfo->type))); /* Device Type */
+aml_append(fdi,
+aml_int(fdinfo->cylinders - 1)); /* Maximum Cylinder Number */
+aml_append(fdi, aml_int(fdinfo->sectors)); /* Maximum Sector Number */
+aml_append(fdi, aml_int(fdinfo->heads - 1)); /* Maximum Head Number */
+/* SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
+ * the drive type, so shall we */
+aml_append(fdi, aml_int(0xAF)); /* disk_specify_1 */
+aml_append(fdi, aml_int(0x02)); /* disk_specify_2 */
+aml_append(fdi, aml_int(0x25)); /* disk_motor_wait */
+aml_append(fdi, aml_int(0x02)); /* disk_sector_siz */
+aml_app
Re: [Qemu-devel] [PATCH v3 1/2] i386: expose floppy-related objects in SSDT
On Tue, Dec 22, 2015 at 05:07:16PM +0200, Michael S. Tsirkin wrote: > On Fri, Dec 18, 2015 at 10:32:29PM +0300, Roman Kagan wrote: > > On x86-based systems Linux determines the presence and the type of > > floppy drives via a query of a CMOS field. So does SeaBIOS when > > populating the return data for int 0x13 function 0x08. > > > > Windows doesn't; instead, it requests this information from BIOS via int > > 0x13/0x08 or through ACPI objects _FDE (Floppy Drive Enumerate) and _FDI > > (Floppy Drive Information). On UEFI systems only ACPI-based detection > > is supported. > > > > QEMU used not to provide those objects in its ACPI tables; as a result > > floppy drives were invisible to Windows on UEFI/OVMF. > > > > This patch implements those objects in SSDT, populating them via AML > > API. For that, a couple of functions are extern-ified to facilitate > > populating those objects in acpi-build.c. > > > > Signed-off-by: Roman Kagan > > Signed-off-by: Denis V. Lunev > > CC: Michael S. Tsirkin > > CC: Igor Mammedov > > CC: Paolo Bonzini > > CC: Richard Henderson > > CC: Eduardo Habkost > > CC: John Snow > > CC: Kevin Wolf > > -- > > This must be --- otherwise git am does not strip the diff stat. OOPS. I wonder how that happened... git format-patch certainly did the right thing; I must have deleted one dash by accident when editing the changelog. Want me to resend? Roman.
Re: [Qemu-devel] [Qemu-block] [PATCH 00/10] qcow2: Implement image locking
On Wed, Dec 23, 2015 at 10:47:22AM +, Daniel P. Berrange wrote: > On Wed, Dec 23, 2015 at 11:14:12AM +0800, Fam Zheng wrote: > > As an alternative, can we introduce .bdrv_flock() in protocol drivers, with > > similar semantics to flock(2) or lockf(3)? That way all formats can benefit, > > and a program crash will automatically drop the lock. > > FWIW, the libvirt locking daemon (virtlockd) will already attempt to take > out locks using fcntl()/lockf() on all disk images associated with a VM. Is it even possible without QEMU cooperating? In particular in complex cases with e.g. backing chains? This was exactly the reason why we designed the "lock" option to take an argument describing the locking mechanism to be used (see the tentative patchset Denis posted in this thread). The only one currently implemented is flock()-based; however it can be extended to other mechanisms like network / cluster / SAN lock managers, etc. In particular, it can be made to talk to virtlockd. Roman.
Re: [Qemu-devel] [PATCH v3 2/2] tests: update expected SSDT for floppy changes
On Tue, Dec 22, 2015 at 06:41:47PM +0200, Michael S. Tsirkin wrote: > On Fri, Dec 18, 2015 at 10:32:30PM +0300, Roman Kagan wrote: > > Update the expected SSDTs to reflect the changes introduced in the > > previous patch. > > > > Signed-off-by: Roman Kagan > > Signed-off-by: Denis V. Lunev > > CC: Michael S. Tsirkin > > CC: Igor Mammedov > > CC: Paolo Bonzini > > CC: Richard Henderson > > CC: Eduardo Habkost > > CC: John Snow > > CC: Kevin Wolf > > Something strange is going on here. > If I apply your patch and this one on top, I get > a diff in SSDT. Aren't you by chance applying it on top of other patches that may affect SSDT? I double-checked the series on top of commit 5dc42c186d63b7b338594fc071cf290805dcc5a5 Merge: c595b21 7236975 Author: Peter Maydell Date: Tue Dec 22 14:21:42 2015 + Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging and it passes OK... Roman.
Re: [Qemu-devel] [PATCH v3 2/2] tests: update expected SSDT for floppy changes
On Wed, Dec 23, 2015 at 03:45:29PM +0200, Michael S. Tsirkin wrote:
> This is the actual vs expected diff with both patches applied.
Interesting... The diff suggests that qemu running in your test
environment has no floppy drives, while in mine ...
> +Scope (\_SB.PCI0.ISA.FDC0)
> +{
> +Device (FLPA)
> +{
> +Name (_ADR, Zero) // _ADR: Address
> +Name (_FDI, Package (0x10) // _FDI: Floppy Drive Information
> +{
> +Zero,
> +0x04,
> +0x4F,
> +0x12,
> +One,
[...]
> +})
> +}
> +
> +Name (_FDE, Buffer (0x14) // _FDE: Floppy Disk Enumerate
> +{
> +/* */ 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /*
> */
> +/* 0008 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /*
> */
> +/* 0010 */ 0x02, 0x00, 0x00, 0x00 /*
> */
> +})
> +}
... it has one 1.44M drive with correct geometry for pc machine type,
and ...
> +Scope (\_SB.PCI0.ISA.FDC0)
> +{
> +Device (FLPA)
> +{
> +Name (_ADR, Zero) // _ADR: Address
> +Name (_FDI, Package (0x10) // _FDI: Floppy Drive Information
> +{
> +Zero,
> +0x04,
> +0x,
> +Zero,
> +0x,
[...]
> +})
> +}
> +
> +Device (FLPB)
> +{
> +Name (_ADR, One) // _ADR: Address
> +Name (_FDI, Package (0x10) // _FDI: Floppy Drive Information
> +{
> +One,
> +0x04,
> +0x,
> +Zero,
> +0x,
[...]
> +})
> +}
> +
> +Name (_FDE, Buffer (0x14) // _FDE: Floppy Disk Enumerate
> +{
> +/* */ 0x01, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, /*
> */
> +/* 0008 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /*
> */
> +/* 0010 */ 0x02, 0x00, 0x00, 0x00 /*
> */
> +})
> +}
... two 1.44M drives with bogus geometry for q35.
Looking into this, thanks for the diff.
Roman.
Re: [Qemu-devel] [PATCH v3 2/2] tests: update expected SSDT for floppy changes
On Wed, Dec 23, 2015 at 06:06:31PM +0300, Roman Kagan wrote: > On Wed, Dec 23, 2015 at 03:45:29PM +0200, Michael S. Tsirkin wrote: > > This is the actual vs expected diff with both patches applied. > > Interesting... The diff suggests that qemu running in your test > environment has no floppy drives Actually, no. My patch adds _FDE unconditionally, so I'm positively unable to tell how you could have obtained this result other than by not applying it. > ... two 1.44M drives with bogus geometry for q35. This one is a bug in my patch, indeed: I was tricked by FDRIVE_DRV_NONE being non-zero, and forgot to initialize the respective fields in acpi_get_misc_info() in case there is no floppy controller at all. I'll resubmit with this bug fixed, but it has nothing to do with the diff you're observing. Roman.
Re: [Qemu-devel] [PATCH v3 2/2] tests: update expected SSDT for floppy changes
On Wed, Dec 23, 2015 at 06:47:16PM +0100, Igor Mammedov wrote: > On Wed, 23 Dec 2015 20:20:54 +0300 > Roman Kagan wrote: > > > ... two 1.44M drives with bogus geometry for q35. > > > > This one is a bug in my patch, indeed: I was tricked by FDRIVE_DRV_NONE > > being non-zero, and forgot to initialize the respective fields in > > acpi_get_misc_info() in case there is no floppy controller at all. > so instead of fake initialization, it's worth to make your patch > conditional on presence of controller after all. > i.e. add AML only if controller was present. Indeed :) Roman.
Re: [Qemu-devel] [PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap
On Thu, Dec 24, 2015 at 12:30:26PM +0300, Andrey Smetanin wrote:
> Currently on x86 arch we has already 32 requests defined
> so the newer request bits can't be placed inside
> vcpu->requests(unsigned long) inside x86 32 bit system.
> But we are going to add a new request in x86 arch
> for Hyper-V tsc page support.
>
> To solve the problem the patch replaces vcpu->requests by
> bitmap with 64 bit length and uses bitmap API.
>
> The patch consists of:
> * announce kvm_vcpu_has_requests() to check whether vcpu has
> requests
> * announce kvm_vcpu_requests() to get vcpu requests pointer
I think that if abstracting out the implementation of the request
container is what you're after, you'd better not define this function;
accesses to the request map should be through your accessor functions.
> * announce kvm_clear_request() to clear particular vcpu request
> * replace if (vcpu->requests) by if (kvm_vcpu_has_requests(vcpu))
> * replace clear_bit(req, vcpu->requests) by
> kvm_clear_request(req, vcpu)
Apparently one accessor is missing: test the presence of a request
without clearing it from the bitmap (because kvm_check_request() clears
it).
> diff --git a/arch/powerpc/kvm/trace.h b/arch/powerpc/kvm/trace.h
> index 2e0e67e..4015b88 100644
> --- a/arch/powerpc/kvm/trace.h
> +++ b/arch/powerpc/kvm/trace.h
> @@ -109,7 +109,7 @@ TRACE_EVENT(kvm_check_requests,
>
> TP_fast_assign(
> __entry->cpu_nr = vcpu->vcpu_id;
> - __entry->requests = vcpu->requests;
> + __entry->requests = (__u32)kvm_vcpu_requests(vcpu)[0];
> ),
This doesn't make sense, to expose only subset of the requests. BTW I
don't see this event in Linus tree, nor in linux-next, so I'm not quite
sure why it's formed this way; I guess the interesting part is the
request number and the return value (i.e. whether it's set), not the
whole bitmap.
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5957,7 +5957,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu
> *vcpu)
> if (intr_window_requested && vmx_interrupt_allowed(vcpu))
> return handle_interrupt_window(&vmx->vcpu);
>
> - if (test_bit(KVM_REQ_EVENT, &vcpu->requests))
> + if (test_bit(KVM_REQ_EVENT, kvm_vcpu_requests(vcpu)))
Here you'd rather want that function to test for the request without
clearing it.
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2117,7 +2117,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct
> msr_data *msr_info)
>
> if (ka->boot_vcpu_runs_old_kvmclock != tmp)
> set_bit(KVM_REQ_MASTERCLOCK_UPDATE,
> - &vcpu->requests);
> + kvm_vcpu_requests(vcpu));
This should have been kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
> @@ -8048,7 +8048,7 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu
> *vcpu)
> if (atomic_read(&vcpu->arch.nmi_queued))
> return true;
>
> - if (test_bit(KVM_REQ_SMI, &vcpu->requests))
> + if (test_bit(KVM_REQ_SMI, kvm_vcpu_requests(vcpu)))
Again the test-only accessor is due here.
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -146,6 +146,8 @@ static inline bool is_error_page(struct page *page)
> #define KVM_REQ_HV_EXIT 30
> #define KVM_REQ_HV_STIMER 31
>
> +#define KVM_REQ_MAX 64
> +
> #define KVM_USERSPACE_IRQ_SOURCE_ID 0
> #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1
>
> @@ -233,7 +235,7 @@ struct kvm_vcpu {
> int vcpu_id;
> int srcu_idx;
> int mode;
> - unsigned long requests;
> + DECLARE_BITMAP(requests, KVM_REQ_MAX);
> unsigned long guest_debug;
>
> int pre_pcpu;
> @@ -286,6 +288,16 @@ struct kvm_vcpu {
> struct kvm_vcpu_arch arch;
> };
>
> +static inline bool kvm_vcpu_has_requests(struct kvm_vcpu *vcpu)
> +{
> + return !bitmap_empty(vcpu->requests, KVM_REQ_MAX);
> +}
> +
> +static inline ulong *kvm_vcpu_requests(struct kvm_vcpu *vcpu)
> +{
> + return (ulong *)vcpu->requests;
> +}
As I wrote above, I believe this function doesn't belong in the API.
> +
> static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
> {
> return cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE);
> @@ -1002,7 +1014,7 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm,
> gpa_t gpa)
>
> static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu)
> {
> - set_bit(KVM_REQ_MIGRATE_TIMER, &vcpu->requests);
> + set_bit(KVM_REQ_MIGRATE_TIMER, kvm_vcpu_requests(vcpu));
kvm_make_request(KVM_REQ_MIGRATE_TIMER, vcpu);
> @@ -1118,19 +1130,24 @@ static inline bool kvm_vcpu_compatible(struct
> kvm_vcpu *vcpu) { return true; }
>
> static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
> {
> - set_bit(req, &vcpu->requests);
> + set_bit(req, kvm_vcp
Re: [Qemu-devel] [PATCH v1 2/2] kvm/x86: Hyper-V SynIC timers tracepoints
On Wed, Dec 23, 2015 at 04:54:00PM +0300, Andrey Smetanin wrote: > Trace the following Hyper SynIC timers events: > * periodic timer start > * one-shot timer start > * timer callback > * timer expiration and message delivery result > * timer config setup > * timer count setup > * timer cleanup > > Signed-off-by: Andrey Smetanin > CC: Gleb Natapov > CC: Paolo Bonzini > CC: Roman Kagan > CC: Denis V. Lunev > CC: qemu-devel@nongnu.org > --- > arch/x86/kvm/hyperv.c | 27 +++- > arch/x86/kvm/trace.h | 170 > ++ > 2 files changed, 196 insertions(+), 1 deletion(-) Reviewed-by: Roman Kagan
Re: [Qemu-devel] [PATCH v1 1/2] kvm/x86: Hyper-V SynIC tracepoints
On Wed, Dec 23, 2015 at 04:53:59PM +0300, Andrey Smetanin wrote: > Trace the following Hyper SynIC events: > * set msr > * set sint irq > * ack sint > * sint irq eoi > > Signed-off-by: Andrey Smetanin > CC: Gleb Natapov > CC: Paolo Bonzini > CC: Roman Kagan > CC: Denis V. Lunev > CC: qemu-devel@nongnu.org > --- > arch/x86/kvm/hyperv.c | 10 +++--- > arch/x86/kvm/trace.h | 93 > +++ > 2 files changed, 98 insertions(+), 5 deletions(-) Reviewed-by: Roman Kagan
Re: [Qemu-devel] [PATCH v2] kvm: Make vcpu->requests as 64 bit bitmap
On Thu, Dec 24, 2015 at 04:29:21PM +0300, Andrey Smetanin wrote: > Currently on x86 arch we has already 32 requests defined > so the newer request bits can't be placed inside > vcpu->requests(unsigned long) inside x86 32 bit system. > But we are going to add a new request in x86 arch > for Hyper-V tsc page support. > > To solve the problem the patch replaces vcpu->requests by > bitmap with 64 bit length and uses bitmap API. > > The patch consists of: > * announce kvm_has_requests() to check whether vcpu has > requests > * announce kvm_clear_request() to clear particular vcpu request > * announce kvm_test_request() to test particular vcpu request > * replace if (vcpu->requests) by if (kvm_has_requests(vcpu)) > * replace clear_bit(req, vcpu->requests) by > kvm_clear_request(req, vcpu) > > Changes v2: > * hide internals of vcpu requests bitmap > by interface usage in all places > * replace test_bit(req, vcpu->requests) by > kvm_test_request() > * POWERPC: trace vcpu requests bitmap by > __bitmask, __assign_bitmap, __get_bitmask > > Signed-off-by: Andrey Smetanin > Acked-by: James Hogan > CC: Paolo Bonzini > CC: Gleb Natapov > CC: James Hogan > CC: Paul Burton > CC: Ralf Baechle > CC: Alexander Graf > CC: Christian Borntraeger > CC: Cornelia Huck > CC: linux-m...@linux-mips.org > CC: kvm-...@vger.kernel.org > CC: linux-s...@vger.kernel.org > CC: Roman Kagan > CC: Denis V. Lunev > CC: qemu-devel@nongnu.org > --- > arch/mips/kvm/emulate.c | 4 +--- > arch/powerpc/kvm/book3s_pr.c | 2 +- > arch/powerpc/kvm/book3s_pr_papr.c | 2 +- > arch/powerpc/kvm/booke.c | 6 +++--- > arch/powerpc/kvm/powerpc.c| 6 +++--- > arch/powerpc/kvm/trace.h | 9 + > arch/s390/kvm/kvm-s390.c | 4 ++-- > arch/x86/kvm/vmx.c| 2 +- > arch/x86/kvm/x86.c| 16 ++++ > include/linux/kvm_host.h | 38 +- > 10 files changed, 50 insertions(+), 39 deletions(-) Reviewed-by: Roman Kagan
[Qemu-devel] [PATCH v4 2/4] i386/acpi: make floppy controller object dynamic
Instead of statically declaring the floppy controller in DSDT, with its
_STA method depending on some obscure bit in the parent ISA bridge, add
the object dynamically to SSDT via AML API only when the controller is
present.
The _STA method is no longer necessary and is therefore dropped. So are
the declarations of the fields indicating whether the contoller is
enabled.
Signed-off-by: Roman Kagan
Cc: "Michael S. Tsirkin"
Cc: Eduardo Habkost
Cc: Igor Mammedov
Cc: John Snow
Cc: Kevin Wolf
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: qemu-bl...@nongnu.org
Cc: qemu-sta...@nongnu.org
---
changes since v3:
- new patch (note that it conflicts with "[PATCH 50/74] pc: acpi: move
FDC0 device from DSDT to SSDT" from Igor's series)
- include test data updates to maintain bisectability
hw/i386/acpi-build.c| 24
hw/i386/acpi-dsdt-isa.dsl | 18 --
hw/i386/acpi-dsdt.dsl | 1 -
hw/i386/q35-acpi-dsdt.dsl | 7 ++-
tests/acpi-test-data/pc/DSDT| Bin 3028 -> 2946 bytes
tests/acpi-test-data/pc/SSDT| Bin 2486 -> 2554 bytes
tests/acpi-test-data/pc/SSDT.bridge | Bin 4345 -> 4413 bytes
tests/acpi-test-data/q35/DSDT | Bin 7666 -> 7578 bytes
8 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4cc1440..a01e909 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -113,6 +113,7 @@ typedef struct AcpiMiscInfo {
unsigned dsdt_size;
uint16_t pvpanic_port;
uint16_t applesmc_io_base;
+bool has_fdc;
} AcpiMiscInfo;
typedef struct AcpiBuildPciBusHotplugState {
@@ -236,10 +237,15 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
static void acpi_get_misc_info(AcpiMiscInfo *info)
{
+ISADevice *fdc;
+
info->has_hpet = hpet_find();
info->tpm_version = tpm_get_version();
info->pvpanic_port = pvpanic_port();
info->applesmc_io_base = applesmc_port();
+
+fdc = pc_find_fdc0();
+info->has_fdc = !!fdc;
}
/*
@@ -1099,6 +1105,24 @@ build_ssdt(GArray *table_data, GArray *linker,
aml_append(scope, aml_name_decl("_S5", pkg));
aml_append(ssdt, scope);
+if (misc->has_fdc) {
+scope = aml_scope("\\_SB.PCI0.ISA");
+dev = aml_device("FDC0");
+
+aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
+
+crs = aml_resource_template();
+aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
+aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
+aml_append(crs, aml_irq_no_flags(6));
+aml_append(crs, aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER,
+AML_TRANSFER8, 2));
+aml_append(dev, aml_name_decl("_CRS", crs));
+
+aml_append(scope, dev);
+aml_append(ssdt, scope);
+}
+
if (misc->applesmc_io_base) {
scope = aml_scope("\\_SB.PCI0.ISA");
dev = aml_device("SMC");
diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
index 89caa16..061507d 100644
--- a/hw/i386/acpi-dsdt-isa.dsl
+++ b/hw/i386/acpi-dsdt-isa.dsl
@@ -47,24 +47,6 @@ Scope(\_SB.PCI0.ISA) {
})
}
-Device(FDC0) {
-Name(_HID, EisaId("PNP0700"))
-Method(_STA, 0, NotSerialized) {
-Store(FDEN, Local0)
-If (LEqual(Local0, 0)) {
-Return (0x00)
-} Else {
-Return (0x0F)
-}
-}
-Name(_CRS, ResourceTemplate() {
-IO(Decode16, 0x03F2, 0x03F2, 0x00, 0x04)
-IO(Decode16, 0x03F7, 0x03F7, 0x00, 0x01)
-IRQNoFlags() { 6 }
-DMA(Compatibility, NotBusMaster, Transfer8) { 2 }
-})
-}
-
Device(LPT) {
Name(_HID, EisaId("PNP0400"))
Method(_STA, 0, NotSerialized) {
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index 8dba096..aa50990 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -80,7 +80,6 @@ DefinitionBlock (
, 3,
CBEN, 1, // COM2
}
-Name(FDEN, 1)
}
}
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 7be7b37..fcb9915 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -137,16 +137,13 @@ DefinitionBlock (
COMB, 3,
Offset(0x01),
-LPTD, 2,
-, 2,
-FDCD, 2
+LPTD, 2
}
OperationRegion(LPCE, PCI_Config, 0x82, 0x2)
Field(LPCE, AnyAcc, NoLock, Preserve) {
CAEN, 1,
CBEN, 1,
-LPEN, 1,
-FDEN, 1
+LPEN,
[Qemu-devel] [PATCH v4 0/4] i386: expose floppy-related objects in SSDT
Windows on UEFI systems is only capable of detecting the presence and the type of floppy drives via corresponding ACPI objects. Those objects are added in the last patch of the series; the three preceding ones pave the way to it, by making the necessary data public and by moving the whole floppy drive controller description into runtime-generated SSDT. Note that the series conflicts with Igor's patchset for dynamic DSDT, in particular, with "[PATCH 50/74] pc: acpi: move FDC0 device from DSDT to SSDT"; I haven't managed to avoid that while trying to meet maintainer's comments. Roman Kagan (4): i386/pc: expose identifying the floppy controller i386/acpi: make floppy controller object dynamic expose floppy drive geometry and CMOS type i386: populate floppy drive information in SSDT Signed-off-by: Roman Kagan Cc: "Michael S. Tsirkin" Cc: Eduardo Habkost Cc: Igor Mammedov Cc: John Snow Cc: Kevin Wolf Cc: Paolo Bonzini Cc: Richard Henderson Cc: qemu-bl...@nongnu.org Cc: qemu-sta...@nongnu.org --- changes since v3: - make FDC object fully dynamic in a separate patch - split out support patches - include test data updates with the respective patches to maintain bisectability changes since v2: - explicit endianness for buffer data - reorder code to reduce conflicts with dynamic DSDT patchset - update test data hw/block/fdc.c | 11 + hw/i386/acpi-build.c| 92 hw/i386/acpi-dsdt-isa.dsl | 18 --- hw/i386/acpi-dsdt.dsl | 1 - hw/i386/pc.c| 46 ++ hw/i386/q35-acpi-dsdt.dsl | 7 +-- include/hw/block/fdc.h | 2 + include/hw/i386/pc.h| 3 ++ tests/acpi-test-data/pc/DSDT| Bin 3028 -> 2946 bytes tests/acpi-test-data/pc/SSDT| Bin 2486 -> 2635 bytes tests/acpi-test-data/pc/SSDT.bridge | Bin 4345 -> 4494 bytes tests/acpi-test-data/q35/DSDT | Bin 7666 -> 7578 bytes 12 files changed, 137 insertions(+), 43 deletions(-) -- 2.5.0
[Qemu-devel] [PATCH v4 3/4] expose floppy drive geometry and CMOS type
Make it possible to query the geometry and the CMOS type of a floppy
drive outside of the respective source files.
It will be useful, in particular, when dynamically building ACPI tables,
and will allow to properly populate the corresponding ACPI objects and
thus enable BIOS-less systems to access the floppy drives.
Signed-off-by: Roman Kagan
Cc: "Michael S. Tsirkin"
Cc: Eduardo Habkost
Cc: Igor Mammedov
Cc: John Snow
Cc: Kevin Wolf
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: qemu-bl...@nongnu.org
Cc: qemu-sta...@nongnu.org
---
changes since v3:
- split out into a separate patch to faciliate review
hw/block/fdc.c | 11 +++
hw/i386/pc.c | 2 +-
include/hw/block/fdc.h | 2 ++
include/hw/i386/pc.h | 1 +
4 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4292ece..c858c5f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2408,6 +2408,17 @@ FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
return isa->state.drives[i].drive;
}
+void isa_fdc_get_drive_geometry(ISADevice *fdc, int i, uint8_t *cylinders,
+uint8_t *heads, uint8_t *sectors)
+{
+FDCtrlISABus *isa = ISA_FDC(fdc);
+FDrive *drv = &isa->state.drives[i];
+
+*cylinders = drv->max_track;
+*heads = (drv->flags & FDISK_DBL_SIDES) ? 2 : 1;
+*sectors = drv->last_sect;
+}
+
static const VMStateDescription vmstate_isa_fdc ={
.name = "fdc",
.version_id = 2,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c36b8cf..99fab83 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -199,7 +199,7 @@ static void pic_irq_request(void *opaque, int irq, int
level)
#define REG_EQUIPMENT_BYTE 0x14
-static int cmos_get_fd_drive_type(FDriveType fd0)
+int cmos_get_fd_drive_type(FDriveType fd0)
{
int val;
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index d48b2f8..adaf3dc 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -22,5 +22,7 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
DriveInfo **fds, qemu_irq *fdc_tc);
FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
+void isa_fdc_get_drive_geometry(ISADevice *fdc, int i, uint8_t *cylinders,
+uint8_t *heads, uint8_t *sectors);
#endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 819..d044a9a 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -268,6 +268,7 @@ typedef void (*cpu_set_smm_t)(int smm, void *arg);
void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
ISADevice *pc_find_fdc0(void);
+int cmos_get_fd_drive_type(FDriveType fd0);
/* acpi_piix.c */
--
2.5.0
[Qemu-devel] [PATCH v4 1/4] i386/pc: expose identifying the floppy controller
Factor out and expose the function to locate the floppy controller in
the system.
It will be useful when dynamically populating the relevant objects in
the ACPI tables.
Signed-off-by: Roman Kagan
Cc: "Michael S. Tsirkin"
Cc: Eduardo Habkost
Cc: Igor Mammedov
Cc: John Snow
Cc: Kevin Wolf
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: qemu-bl...@nongnu.org
Cc: qemu-sta...@nongnu.org
---
changes since v3:
- split out into a separate patch to faciliate review
hw/i386/pc.c | 44 ++--
include/hw/i386/pc.h | 2 ++
2 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 459260b..c36b8cf 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -360,6 +360,31 @@ static const char * const fdc_container_path[] = {
"/unattached", "/peripheral", "/peripheral-anon"
};
+/*
+ * Locate the FDC at IO address 0x3f0, in order to configure the CMOS registers
+ * and ACPI objects.
+ */
+ISADevice *pc_find_fdc0(void)
+{
+int i;
+Object *container;
+CheckFdcState state = { 0 };
+
+for (i = 0; i < ARRAY_SIZE(fdc_container_path); i++) {
+container = container_get(qdev_get_machine(), fdc_container_path[i]);
+object_child_foreach(container, check_fdc, &state);
+}
+
+if (state.multiple) {
+error_report("warning: multiple floppy disk controllers with "
+ "iobase=0x3f0 have been found;\n"
+ "the one being picked for CMOS setup might not reflect "
+ "your intent");
+}
+
+return state.floppy;
+}
+
static void pc_cmos_init_late(void *opaque)
{
pc_cmos_init_late_arg *arg = opaque;
@@ -368,8 +393,6 @@ static void pc_cmos_init_late(void *opaque)
int8_t heads, sectors;
int val;
int i, trans;
-Object *container;
-CheckFdcState state = { 0 };
val = 0;
if (ide_get_geometry(arg->idebus[0], 0,
@@ -399,22 +422,7 @@ static void pc_cmos_init_late(void *opaque)
}
rtc_set_memory(s, 0x39, val);
-/*
- * Locate the FDC at IO address 0x3f0, and configure the CMOS registers
- * accordingly.
- */
-for (i = 0; i < ARRAY_SIZE(fdc_container_path); i++) {
-container = container_get(qdev_get_machine(), fdc_container_path[i]);
-object_child_foreach(container, check_fdc, &state);
-}
-
-if (state.multiple) {
-error_report("warning: multiple floppy disk controllers with "
- "iobase=0x3f0 have been found;\n"
- "the one being picked for CMOS setup might not reflect "
- "your intent");
-}
-pc_cmos_init_floppy(s, state.floppy);
+pc_cmos_init_floppy(s, pc_find_fdc0());
qemu_unregister_reset(pc_cmos_init_late, opaque);
}
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index b0d6283..819 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -267,6 +267,8 @@ typedef void (*cpu_set_smm_t)(int smm, void *arg);
void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
+ISADevice *pc_find_fdc0(void);
+
/* acpi_piix.c */
I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
--
2.5.0
[Qemu-devel] [PATCH v4 4/4] i386: populate floppy drive information in SSDT
On x86-based systems Linux determines the presence and the type of
floppy drives via a query of a CMOS field. So does SeaBIOS when
populating the return data for int 0x13 function 0x08.
Windows doesn't; instead, it requests this information from BIOS via int
0x13/0x08 or through ACPI objects _FDE (Floppy Drive Enumerate) and _FDI
(Floppy Drive Information) of the floppy controller object. On UEFI
systems only ACPI-based detection is supported.
QEMU used not to provide those objects in its ACPI tables; as a result
floppy drives were invisible to Windows on UEFI/OVMF.
This patch adds those objects to the floppy controller in SSDT,
populating them with the information from the respective QEMU objects.
Signed-off-by: Roman Kagan
Cc: "Michael S. Tsirkin"
Cc: Eduardo Habkost
Cc: Igor Mammedov
Cc: John Snow
Cc: Kevin Wolf
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: qemu-bl...@nongnu.org
Cc: qemu-sta...@nongnu.org
---
changes since v3:
- build on top of dynamic FDC0 patch
- include test data updates to maintain bisectability
changes since v2:
- explicit endianness for buffer data
- reorder code to reduce conflicts with dynamic DSDT patchset
hw/i386/acpi-build.c| 68
tests/acpi-test-data/pc/SSDT| Bin 2554 -> 2635 bytes
tests/acpi-test-data/pc/SSDT.bridge | Bin 4413 -> 4494 bytes
3 files changed, 68 insertions(+)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a01e909..7b8de59 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -38,6 +38,7 @@
#include "hw/acpi/bios-linker-loader.h"
#include "hw/loader.h"
#include "hw/isa/isa.h"
+#include "hw/block/fdc.h"
#include "hw/acpi/memory_hotplug.h"
#include "hw/mem/nvdimm.h"
#include "sysemu/tpm.h"
@@ -106,6 +107,13 @@ typedef struct AcpiPmInfo {
uint16_t pcihp_io_len;
} AcpiPmInfo;
+typedef struct AcpiFDInfo {
+uint8_t type;
+uint8_t cylinders;
+uint8_t heads;
+uint8_t sectors;
+} AcpiFDInfo;
+
typedef struct AcpiMiscInfo {
bool has_hpet;
TPMVersion tpm_version;
@@ -114,6 +122,7 @@ typedef struct AcpiMiscInfo {
uint16_t pvpanic_port;
uint16_t applesmc_io_base;
bool has_fdc;
+AcpiFDInfo fdinfo[2];
} AcpiMiscInfo;
typedef struct AcpiBuildPciBusHotplugState {
@@ -237,6 +246,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
static void acpi_get_misc_info(AcpiMiscInfo *info)
{
+int i;
ISADevice *fdc;
info->has_hpet = hpet_find();
@@ -246,6 +256,16 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
fdc = pc_find_fdc0();
info->has_fdc = !!fdc;
+if (fdc) {
+for (i = 0; i < ARRAY_SIZE(info->fdinfo); i++) {
+AcpiFDInfo *fdinfo = &info->fdinfo[i];
+fdinfo->type = isa_fdc_get_drive_type(fdc, i);
+if (fdinfo->type < FDRIVE_DRV_NONE) {
+isa_fdc_get_drive_geometry(fdc, i, &fdinfo->cylinders,
+ &fdinfo->heads, &fdinfo->sectors);
+}
+}
+}
}
/*
@@ -935,6 +955,40 @@ static Aml *build_crs(PCIHostState *host,
return crs;
}
+static Aml *build_fdinfo_aml(int idx, AcpiFDInfo *fdinfo)
+{
+Aml *dev, *fdi;
+
+dev = aml_device("FLP%c", 'A' + idx);
+
+aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
+
+fdi = aml_package(0x10);
+aml_append(fdi, aml_int(idx)); /* Drive Number */
+aml_append(fdi,
+aml_int(cmos_get_fd_drive_type(fdinfo->type))); /* Device Type */
+aml_append(fdi,
+aml_int(fdinfo->cylinders - 1)); /* Maximum Cylinder Number */
+aml_append(fdi, aml_int(fdinfo->sectors)); /* Maximum Sector Number */
+aml_append(fdi, aml_int(fdinfo->heads - 1)); /* Maximum Head Number */
+/* SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
+ * the drive type, so shall we */
+aml_append(fdi, aml_int(0xAF)); /* disk_specify_1 */
+aml_append(fdi, aml_int(0x02)); /* disk_specify_2 */
+aml_append(fdi, aml_int(0x25)); /* disk_motor_wait */
+aml_append(fdi, aml_int(0x02)); /* disk_sector_siz */
+aml_append(fdi, aml_int(0x12)); /* disk_eot */
+aml_append(fdi, aml_int(0x1B)); /* disk_rw_gap */
+aml_append(fdi, aml_int(0xFF)); /* disk_dtl */
+aml_append(fdi, aml_int(0x6C)); /* disk_formt_gap */
+aml_append(fdi, aml_int(0xF6)); /* disk_fill */
+aml_append(fdi, aml_int(0x0F)); /* disk_head_sttl */
+aml_append(fdi, aml_int(0x08)); /* disk_motor_strt */
+
+aml_append(dev, aml_name_decl("_FDI", fdi));
+return dev;
+}
+
static void
build_ssdt(GArray *table_data, GArray *linker,
AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
@@ -1106,6 +1160,8 @@ build_ssdt(GArray *table_data, GArray *linker,
aml_append(ssdt, sc
Re: [Qemu-devel] [PATCH v3 2/2] tests: update expected SSDT for floppy changes
On Thu, Dec 24, 2015 at 08:17:45AM +0200, Michael S. Tsirkin wrote: > On Wed, Dec 23, 2015 at 08:51:45PM +0300, Roman Kagan wrote: > > On Wed, Dec 23, 2015 at 06:47:16PM +0100, Igor Mammedov wrote: > > > On Wed, 23 Dec 2015 20:20:54 +0300 > > > Roman Kagan wrote: > > > > > ... two 1.44M drives with bogus geometry for q35. > > > > > > > > This one is a bug in my patch, indeed: I was tricked by FDRIVE_DRV_NONE > > > > being non-zero, and forgot to initialize the respective fields in > > > > acpi_get_misc_info() in case there is no floppy controller at all. > > > so instead of fake initialization, it's worth to make your patch > > > conditional on presence of controller after all. > > > i.e. add AML only if controller was present. > > > > Indeed :) > > > > Roman. > > Or rather, start series with a patch making FDC conditional, > then update expected ssdt, then tweak methods within - > should not change ssdt since we don't create a floppy in > the test. Actually we do. "pc" machine type has it by default regardless of whether anything is attached to it. So I ended up with the patch series v4 I just posted but I'm not sure it addresses all the concerns people have had about v3. Thanks, Roman.
Re: [Qemu-devel] [PATCH v4 0/4] i386: expose floppy-related objects in SSDT
On Tue, Dec 29, 2015 at 03:09:36PM +0100, Igor Mammedov wrote: > On Fri, 25 Dec 2015 18:04:08 +0300 > Roman Kagan wrote: > > > Windows on UEFI systems is only capable of detecting the presence and > > the type of floppy drives via corresponding ACPI objects. > > > > Those objects are added in the last patch of the series; the three > > preceding ones pave the way to it, by making the necessary data > > public and by moving the whole floppy drive controller description into > > runtime-generated SSDT. > > > > Note that the series conflicts with Igor's patchset for dynamic DSDT, in > > particular, with "[PATCH 50/74] pc: acpi: move FDC0 device from DSDT > > to SSDT"; I haven't managed to avoid that while trying to meet > > maintainer's comments. > To remove conflicts and to make it more suitable for stable, I'd drop > "2/4 i386/acpi: make floppy controller object dynamic" Erm... I thought I did what Michael requested: On Thu, Dec 24, 2015 at 08:17:45AM +0200, Michael S. Tsirkin wrote: > Or rather, start series with a patch making FDC conditional, So what do I need to do to get this thing merged? > and split test blob out of > "4/4 i386: populate floppy drive information in SSDT" > into a separate patch so one can see effects of applying 4/4 > and then update blobs if resulting ASL diff is as expected. This will break bisectability, won't it? Roman.
[Qemu-devel] [PATCH v5 3/6] tests/acpi: update test data
to match the preceding commit "i386/acpi: make floppy controller object
dynamic".
Signed-off-by: Roman Kagan
Cc: "Michael S. Tsirkin"
Cc: Eduardo Habkost
Cc: Igor Mammedov
Cc: John Snow
Cc: Kevin Wolf
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: qemu-bl...@nongnu.org
Cc: qemu-sta...@nongnu.org
---
changes since v4:
- new, split out from code changes
tests/acpi-test-data/pc/DSDT| Bin 3028 -> 2946 bytes
tests/acpi-test-data/pc/SSDT| Bin 2486 -> 2554 bytes
tests/acpi-test-data/pc/SSDT.bridge | Bin 4345 -> 4413 bytes
tests/acpi-test-data/q35/DSDT | Bin 7666 -> 7578 bytes
4 files changed, 0 insertions(+), 0 deletions(-)
diff --git a/tests/acpi-test-data/pc/DSDT b/tests/acpi-test-data/pc/DSDT
index
c658203db94a7e7db7c36fde99a7075a8d75498d..d8ebf12cc0ae9f19d131c7b1d7d1b3e681df
100644
GIT binary patch
delta 51
zcmca2-XzZD66_Mv#Ld9KxML%i4I`fet6qGtQ+$B4r$Ka+^W+dlCuRW$@yYWU7jEuh
H^56sjVdo9@
delta 130
zcmZn?zaq}%66_Lkg`0tav1KEd4I`f$t6qGtQ+$B4r$Ka+=j0GZCr%DG7gs+<0Uznf
zGZ`0pc(J&-I2&-pdw9C=I9_095Rr%v4sm2C04YjXz&1I7VF|-RmL**L9P!RU!Gh9U
h67Gzjm_IQyu(&gRXa3I2z^LTFpvA(l*^J4D69A%9AeI0C
diff --git a/tests/acpi-test-data/pc/SSDT b/tests/acpi-test-data/pc/SSDT
index
210d6a71e58aa34ce8e94121d25bcf58c3bd503c..ffb3fe43970123d0e198328429ee04ebfd0564c9
100644
GIT binary patch
delta 91
zcmdlc{7aZCIM^lR7bgP)lIFV=xB*dq@{?
delta 24
gcmew*yiJ%ZIM^j*8z%z;k
diff --git a/tests/acpi-test-data/pc/SSDT.bridge
b/tests/acpi-test-data/pc/SSDT.bridge
index
6e6660b1fbe0bdb7fe0b257cb53c31fa9bb21a14..f0f11bec8108eceb696fb854b51faeb97471146c
100644
GIT binary patch
delta 91
zcmeyVxL1iQIM^k`R*->#@y153XhtVzmKc5J_+Y2_0B27F&tS*+=q3X<7iR;Gcn?n(
v9>)vp3>@*!LBWF3ToUe#pO`-}GqAWberNv9%)qGRz@Wv#P`NpoaT-4WWGfeO
delta 24
gcmdn1^iz>5IM^lRrvL*3qryh6XvWPe8K>|A0A#NRg8%>k
diff --git a/tests/acpi-test-data/q35/DSDT b/tests/acpi-test-data/q35/DSDT
index
4723e5954dccb00995ccaf521b7daf6bf15cf1d4..5e03e26323d50dea63d57a36bd902e061e0442d8
100644
GIT binary patch
delta 91
zcmexlJoQ5GMmP8b
qIJ+`&HE}UTH;RJT49gNC-HHPcCCxxH*}*UkU(Bo)>-q
delta 176
zcmbPb{mGikCD
