[PATCH] ide:do nothing for identify cmd if no any device attached

[PATCH] ide:do nothing for identify cmd if no any device attached

[zhaoxin\RockCuioc]

From: "zhaoxin\\rockcuioc" <RockCui-oc@zhaoxin.com>

This patch is for avoiding win7 IDE driver polling 0x1f7 when
no any device attached. During Win7 VM boot procedure, if use virtio for
disk and there is no any device be attached on hda & hdb, the win7 IDE driver
would poll 0x1f7 for a while. This action may be stop windows LOGO atomic for
a while too on a poor performance CPU.

Signed-off-by: zhaoxin\RockCuioc <RockCui-oc@zhaoxin.com>
---
 hw/ide/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..07968aadfd 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2073,8 +2073,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
     s = idebus_active_if(bus);
     trace_ide_exec_cmd(bus, s, val);
 
-    /* ignore commands to non existent slave */
-    if (s != bus->ifs && !s->blk) {
+    /* ignore commands if no any device exist or non existent slave */
+    if ((!bus->ifs[0].blk && !bus->ifs[1].blk) || 
+	(s != bus->ifs && !s->blk)) {
         return;
     }
 
-- 
2.17.1

Re: [PATCH] ide:do nothing for identify cmd if no any device attached

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200814043657.5815-1-RockCui-oc@zhaoxin.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20200814043657.5815-1-RockCui-oc@zhaoxin.com
Subject: [PATCH] ide:do nothing for identify cmd if no any device attached

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c0ee2f5 ide:do nothing for identify cmd if no any device attached

=== OUTPUT BEGIN ===
ERROR: trailing whitespace
#27: FILE: hw/ide/core.c:2077:
+    if ((!bus->ifs[0].blk && !bus->ifs[1].blk) || $

ERROR: code indent should never use tabs
#28: FILE: hw/ide/core.c:2078:
+^I(s != bus->ifs && !s->blk)) {$

total: 2 errors, 0 warnings, 11 lines checked

Commit c0ee2f5abbf3 (ide:do nothing for identify cmd if no any device attached) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200814043657.5815-1-RockCui-oc@zhaoxin.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] ide:do nothing for identify cmd if no any device attached

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200814043657.5815-1-RockCui-oc@zhaoxin.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-unit: tests/test-char
Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181:
attempt to add duplicate property 'serial-id' to object (type 'container')
ERROR test-char - too few tests run (expected 38, got 9)
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 024
  TEST    iotest-qcow2: 025
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=3d9b744ea9784590902f0fd1b1c1cc94', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-uf3g3t43/src/docker-src.2020-08-14-11.29.38.3773:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=3d9b744ea9784590902f0fd1b1c1cc94
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-uf3g3t43/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    13m29.699s
user    0m8.487s


The full log is available at
http://patchew.org/logs/20200814043657.5815-1-RockCui-oc@zhaoxin.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] ide:do nothing for identify cmd if no any device attached

[John Snow]

On 9/3/20 6:40 AM, Max Reitz wrote:
> On 02.09.20 20:02, John Snow wrote:
>> (CC Max for block backend model confusion, see below)
>>
>> On 8/16/20 11:38 PM, zhaoxin\RockCuioc wrote:
>>> This patch is for avoiding win7 IDE driver polling 0x1f7 when
>>> no any device attached. During Win7 VM boot procedure, if use virtio for
>>> disk and there is no any device be attached on hda & hdb, the win7 IDE
>>> driver
>>> would poll 0x1f7 for a while. This action may be stop windows LOGO
>>> atomic for
>>> a while too on a poor performance CPU.
>>>
>>
>> A few questions:
>>
>> (1) How slow is the probing?
>>
>> (2) If there are no devices attached, why don't you remove the IDE
>> controller so that Windows doesn't have to probe it?
>>
>>> Signed-off-by: zhaoxin\RockCuioc <RockCui-oc@zhaoxin.com>
>>> ---
>>>    hw/ide/core.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index d997a78e47..26d86f4b40 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -2073,8 +2073,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>>        s = idebus_active_if(bus);
>>>        trace_ide_exec_cmd(bus, s, val);
>>>    -    /* ignore commands to non existent slave */
>>> -    if (s != bus->ifs && !s->blk) {
> 
> (Was the first one basically meant to be “s != &bus->ifs[0]”, i.e. to
> check that this doesn’t go to the ma^W primary?  Not too obvious.)
> 

Yeah, I think it was meant to say:

if (s == bus->ifs[1] && !s->blk)

(But I don't know why it was important to guard device1 specifically. 
Knowledge lost to the sands of time.)

By the way, the correct terms are "device0" and "device1" and have been 
since at least ATA2. I believe ATA1 used the terms "disk0" and "disk1". 
"Primary" and "Secondary" are used to refer to the controllers, not the 
devices.

- Primary
   - device0
   - device1
- Secondary
   - device0
   - device1

Thanks for coming to my TED talk!

>>> +    /* ignore commands if no any device exist or non existent slave */
>>> +    if ((!bus->ifs[0].blk && !bus->ifs[1].blk) ||
>>> +        (s != bus->ifs && !s->blk)) {
> 
> (Maybe this could be improved here)
> 
>>>            return;
>>>        }
>>>    
>>
>> I think it's the case that Empty CD-ROM drives will have an anonymous
>> block backend representing the empty drive,
> 
> (As far as I remember,) yes.
> 
> (ide_dev_initfn() ensures all CD drives have one, even if it’s empty.)
> 

(Thanks)

>> so I suppose this is maybe
>> fine?
>>
>> I suppose the idea is that with no drives on the bus that it's fine to
>> ignore the register writes, as there are no devices to record those writes.
>>
>> (But then, why did we ever only check device1? ...)
>>
>> Maybe before the block-backend split we used to have to check to see if
>> we had attached media or not, but I think nowadays we should always have
>> a blk pointer if we have a device model intended to be operating at this
>> address.
> 
> The check in ide_dev_initfn() looks that way to me.
> 
>> So I guess it can be simplified ...?
>>
>> if (!s->blk) {
>>      return;
>> }
> 
> Probably.  Although there’s a difference, of course, namely if you have
> only a secondary device and try to access the primary, this simplified
> version will be a no-op, whereas the more complicated version in this
> patch would still go on.  I don’t know how real hardware would handle
> that case.  Is it even possible to have just a secondary with no primary?
> 

I think so. From what I understand, two drives on a single channel both 
receive all of the same register update commands, including the "SELECT" 
register, which has a bit devoted to which drive we have selected.

When we write to the COMMAND register, only the selective drive should 
actually respond to it.

so what I expect happens on real machines:

- You select device0
- You write to a bunch of registers
- You issue a command
- Nobody responds.

--js

Re: [PATCH] ide:do nothing for identify cmd if no any device attached

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2946 bytes --]

On 02.09.20 20:02, John Snow wrote:
> (CC Max for block backend model confusion, see below)
> 
> On 8/16/20 11:38 PM, zhaoxin\RockCuioc wrote:
>> This patch is for avoiding win7 IDE driver polling 0x1f7 when
>> no any device attached. During Win7 VM boot procedure, if use virtio for
>> disk and there is no any device be attached on hda & hdb, the win7 IDE
>> driver
>> would poll 0x1f7 for a while. This action may be stop windows LOGO
>> atomic for
>> a while too on a poor performance CPU.
>>
> 
> A few questions:
> 
> (1) How slow is the probing?
> 
> (2) If there are no devices attached, why don't you remove the IDE
> controller so that Windows doesn't have to probe it?
> 
>> Signed-off-by: zhaoxin\RockCuioc <RockCui-oc@zhaoxin.com>
>> ---
>>   hw/ide/core.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index d997a78e47..26d86f4b40 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -2073,8 +2073,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>       s = idebus_active_if(bus);
>>       trace_ide_exec_cmd(bus, s, val);
>>   -    /* ignore commands to non existent slave */
>> -    if (s != bus->ifs && !s->blk) {

(Was the first one basically meant to be “s != &bus->ifs[0]”, i.e. to
check that this doesn’t go to the ma^W primary?  Not too obvious.)

>> +    /* ignore commands if no any device exist or non existent slave */
>> +    if ((!bus->ifs[0].blk && !bus->ifs[1].blk) ||
>> +        (s != bus->ifs && !s->blk)) {

(Maybe this could be improved here)

>>           return;
>>       }
>>   
> 
> I think it's the case that Empty CD-ROM drives will have an anonymous
> block backend representing the empty drive,

(As far as I remember,) yes.

(ide_dev_initfn() ensures all CD drives have one, even if it’s empty.)

> so I suppose this is maybe
> fine?
> 
> I suppose the idea is that with no drives on the bus that it's fine to
> ignore the register writes, as there are no devices to record those writes.
> 
> (But then, why did we ever only check device1? ...)
> 
> Maybe before the block-backend split we used to have to check to see if
> we had attached media or not, but I think nowadays we should always have
> a blk pointer if we have a device model intended to be operating at this
> address.

The check in ide_dev_initfn() looks that way to me.

> So I guess it can be simplified ...?
> 
> if (!s->blk) {
>     return;
> }

Probably.  Although there’s a difference, of course, namely if you have
only a secondary device and try to access the primary, this simplified
version will be a no-op, whereas the more complicated version in this
patch would still go on.  I don’t know how real hardware would handle
that case.  Is it even possible to have just a secondary with no primary?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ide:do nothing for identify cmd if no any device attached

[John Snow]

(CC Max for block backend model confusion, see below)

On 8/16/20 11:38 PM, zhaoxin\RockCuioc wrote:
> This patch is for avoiding win7 IDE driver polling 0x1f7 when
> no any device attached. During Win7 VM boot procedure, if use virtio for
> disk and there is no any device be attached on hda & hdb, the win7 IDE driver
> would poll 0x1f7 for a while. This action may be stop windows LOGO atomic for
> a while too on a poor performance CPU.
> 

A few questions:

(1) How slow is the probing?

(2) If there are no devices attached, why don't you remove the IDE 
controller so that Windows doesn't have to probe it?

> Signed-off-by: zhaoxin\RockCuioc <RockCui-oc@zhaoxin.com>
> ---
>   hw/ide/core.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d997a78e47..26d86f4b40 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2073,8 +2073,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>       s = idebus_active_if(bus);
>       trace_ide_exec_cmd(bus, s, val);
>   
> -    /* ignore commands to non existent slave */
> -    if (s != bus->ifs && !s->blk) {
> +    /* ignore commands if no any device exist or non existent slave */
> +    if ((!bus->ifs[0].blk && !bus->ifs[1].blk) ||
> +        (s != bus->ifs && !s->blk)) {
>           return;
>       }
>   

I think it's the case that Empty CD-ROM drives will have an anonymous 
block backend representing the empty drive, so I suppose this is maybe fine?

I suppose the idea is that with no drives on the bus that it's fine to 
ignore the register writes, as there are no devices to record those writes.

(But then, why did we ever only check device1? ...)

Maybe before the block-backend split we used to have to check to see if 
we had attached media or not, but I think nowadays we should always have 
a blk pointer if we have a device model intended to be operating at this 
address.

So I guess it can be simplified ...?

if (!s->blk) {
     return;
}

--js

Re: [PATCH] ide:do nothing for identify cmd if no any device attached

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200817033803.14014-1-RockCui-oc@zhaoxin.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-unit: tests/test-char
Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181:
attempt to add duplicate property 'serial-id' to object (type 'container')
ERROR test-char - too few tests run (expected 38, got 9)
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-qtest-x86_64: tests/qtest/hd-geo-test
  TEST    check-qtest-x86_64: tests/qtest/boot-order-test
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=8c36fb1727574c069b2f14432c2bddf7', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-0cmc1v90/src/docker-src.2020-08-17-00.03.16.18596:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=8c36fb1727574c069b2f14432c2bddf7
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-0cmc1v90/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    12m14.785s
user    0m8.902s


The full log is available at
http://patchew.org/logs/20200817033803.14014-1-RockCui-oc@zhaoxin.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

[PATCH] ide:do nothing for identify cmd if no any device attached

[zhaoxin\RockCuioc]

This patch is for avoiding win7 IDE driver polling 0x1f7 when
no any device attached. During Win7 VM boot procedure, if use virtio for
disk and there is no any device be attached on hda & hdb, the win7 IDE driver
would poll 0x1f7 for a while. This action may be stop windows LOGO atomic for
a while too on a poor performance CPU.

Signed-off-by: zhaoxin\RockCuioc <RockCui-oc@zhaoxin.com>
---
 hw/ide/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..26d86f4b40 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2073,8 +2073,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
     s = idebus_active_if(bus);
     trace_ide_exec_cmd(bus, s, val);
 
-    /* ignore commands to non existent slave */
-    if (s != bus->ifs && !s->blk) {
+    /* ignore commands if no any device exist or non existent slave */
+    if ((!bus->ifs[0].blk && !bus->ifs[1].blk) ||
+        (s != bus->ifs && !s->blk)) {
         return;
     }
 
-- 
2.17.1